Inconsistency between documentation and implementation on fields filter

[githistory]

The fields filter in this library accepts array of field names where as the documentation says

{ fields: {id: true, make: true, model: true} }

[superkhau]

@crandmck ^

[crandmck]

@superkhau Did this change when loopback-filters was added? It was almost certainly originally the way it's documented.

Does the Node API only accept the array form and not the {propertyName: <true|false>, propertyName: <true|false>, ... } form?

So, IIUC, this example should be:
{ fields: [{id: true}, {make: true}, {model: true}] }
Is that right?

[superkhau]

I believe it takes both, gotta verify though:

loopback-filters/index.js

Line 230 in 2f44e21

function selectFields(fields) {

.

@githistory Can you provide a sample repo for me to verify with?

[FraserChapman]

@crandmck @superkhau

FWIW I'm using [email protected] and the only thing that works for me is an inclusive array of property names. e.g. To use the example given.

{ fields: ["id", "make", "model"] }

Anything other format supplied to fields results in empty objects being returned from the call to applyFilter.

Puzzled me for about half and hour.

[crandmck]

@FraserChapman Could you please provide a sample repo, so we can reproduce? I know that's what @superkhau will ask! :-)

[ewrayjohnson]

No offense, but IMHO, a sample repo is unnecessary as any data will do. This is a obvious bug and very easy to fix. I wonder if this code is being maintained since this bug report is so old. The documentation says the pattern is { fields: {id: true, make: true, model: true} } but only something like { fields : [ 'id', 'make', 'model' ] } will ever work. The code should "normalize" the filter.fields object by converting it from a map of fields to include/exclude, to an array of strings of fields to include. I recommend the following fix. Replace the if (filter.fields) {... } on or about line 29 of loopback-filters/lib/index.js with this:

 // field selection
    if (filter.fields) {
       var fieldMap = filter.fields;
       var fieldSet = [];
       for(var key in fieldMap) {
        if (fieldMap.hasOwnProperty(key)) {
          if (fieldMap[key]) {
            fieldSet.push(key);
          }
        }
      }

loopback-filters/test/filter.test.js should also be updated to provide a regression test of this capability.

An ultimate fix would cache this fieldSet for reuse and would also use the declared properties (and relations) of a model and would consider the strict property - but I digress. See loopback=datasource-juggler\lib\utils.js\fieldsToArray(..) for a full implementation.

On this page LoopBack 3.0 Querying data
It states: "Note: We plan to convert all modules to use loopback-filter[s], so it will become LoopBack’s common “built-in” filtering mechanism."
However that all such efforts seems to have been abandoned as the code (Copyright IBM) has not been updated since Dec 9, 2016 and this was reported on way back in Jan 25, 2016, with no one taking it seriously.

[crandmck]

@Amir-61
@superkhau
@loay

[crandmck]

OK @ewrayjohnson I updated the docs. However, it's not clear to me how you exclude fields with the syntax you indicate. Do you know?

[ewrayjohnson]

The LoopBack documentation WAS correct and this is how LoopBack currently works. It looks like the documentation now does not match the released code. "A fields filter specifies properties (fields) to include or exclude from the results." true=include false=exclude By default, queries return all model properties in results. However, if you specify a fields filter, then the query will include those fields specified with a value of true and exclude those with false. Why then is the true/false necessary? Here's why: when the model is defined with strict=false, undefined fields can be stored (in a schema-less database ) and a query without a fields filter will return undefined fields. However, if a query has a fields filter, only those fields defined in the fields filter with a true value will be returned. The false therefore allows a client to not have to know if strict is set.

…

On May 17, 2017, at 6:33 PM, Rand McKinney ***@***.***> wrote: OK @ewrayjohnson I updated the docs. However, it's not clear to me how you exclude fields with the syntax you indicate. Do you know? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or mute the thread.

[crandmck]

You said:

The documentation says the pattern is { fields: {id: true, make: true, model: true} } but only something like { fields : [ 'id', 'make', 'model' ] } will ever work.

So I changed fields filter accordingly to have (in the example) { fields: ["id", "make", "model"] }.

Now you're saying that it WAS correct but now is not. So which is correct? The doc should of course state what actually works, not what should work.

[ewrayjohnson]

Now I see how I confused you. THIS thread is for strongloop/loopback-filters NOT strongloop/loopback (the real LoopBack). The documentation I referred to was for LoopBack, which WAS correct for the real LoopBack. However in "loopback-filters" filter.fields does NOT work the same as in the real LoopBack. I provided the code fix for loopback-filters so that IT would work like the thousands of existing "real" LoopBack projects since the LoopBack documnation states "Note: We plan to convert all modules to use loopback-filter[s], so it will become LoopBack’s common “built-in” filtering mechanism." So changing the documentation in LoopBack will cause lots of people problems. Hope this helps.

So what I mean to say in my ealier post is:
The real LoopBack documentation says the pattern is { fields: {id: true, make: true, model: true} } but for loopback-filters, only something like { fields : [ 'id', 'make', 'model' ] } will ever work.

[ewrayjohnson]

Here is a snippet of code from the real LoopBack. Note the excludeUnknown param which is mapped to the "model.strict" property and the properties param, which is a list of the names of properties explicitly defined on the model:

/**
 * Normalize fields to an array of included properties
 * @param {String|String[]|Object} fields Fields filter
 * @param {String[]} properties Property names
 * @param {Boolean} excludeUnknown To exclude fields that are unknown properties
 * @returns {String[]} An array of included property names
 */
function fieldsToArray(fields, properties, excludeUnknown) {
  if (!fields) return;

  // include all properties by default
  var result = properties;
  var i, n;

  if (typeof fields === 'string') {
    result = [fields];
  } else if (Array.isArray(fields) && fields.length > 0) {
    // No empty array, including all the fields
    result = fields;
  } else if ('object' === typeof fields) {
    // { field1: boolean, field2: boolean ... }
    var included = [];
    var excluded = [];
    var keys = Object.keys(fields);
    if (!keys.length) return;

    for (i = 0, n = keys.length; i < n; i++) {
      var k = keys[i];
      if (fields[k]) {
        included.push(k);
      } else if ((k in fields) && !fields[k]) {
        excluded.push(k);
      }
    }
    if (included.length > 0) {
      result = included;
    } else if (excluded.length > 0) {
      for (i = 0, n = excluded.length; i < n; i++) {
        var index = result.indexOf(excluded[i]);
        result.splice(index, 1);
      }
    }
  }

  var fieldArray = [];
  if (excludeUnknown) {
    for (i = 0, n = result.length; i < n; i++) {
      if (properties.indexOf(result[i]) !== -1) {
        fieldArray.push(result[i]);
      }
    }
  } else {
    fieldArray = result;
  }
  return fieldArray;
}

[crandmck]

Oh! Thanks for the clarification @ewrayjohnson. I certainly was confused :-)
I'll back out my change to the LB docs.

[bestvow]

Do not add everything if some fields you don't want to see even a flase ?

[bajtos]

Hello @ewrayjohnson, sorry for joining this discussion late. You offered a code snippet to change the behavior of loopback-filters and make it consistent with "regular" loopback. Could you please turn it into a pull request that we can land? (Assuming this issue is still relevant.)

[stale]

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

[stale]

This issue has been closed due to continued inactivity. Thank you for your understanding. If you believe this to be in error, please contact one of the code owners, listed in the CODEOWNERS file at the top-level of this repository.