Request not being chained for larger `limit` values

[braughtg]

Describe the bug The documentation (here: https://github.com/farmOS/farmOS.js/blob/main/docs/entities.md#limiting-fetch-requests) suggests that when the limit is set to a value larger than the internal drupal page size that 'fetchwill "chain together successive requests until a givenlimit` option is reached." This does not appear to be happening.

To Reproduce Here is a snippet of sample code that I am using that demonstrates the issue:

  const land = await farm.asset.fetch({
    filter: {
      type: ["asset--land"],
      land_type: ["field", "bed"],
    },
    limit: 30,
  });

farm is a farm instance created as shown in the demo code in the "Quick start" guide.

The database in used has 70 assets that meet the filter criteria, however, the returned land.data array has only 25 assets in it. If limit is set to 30, the result still contains only 25 assets. If the limit is set to 10, then the result correctly contains only 10 assets.

Expected behavior When limit is provided to fetch it is expected that the result will contain up to a total of limit records matching the filter criteria. When limit is set to Infinity the result should contain all records matching the filter.

Desktop (please complete the following information):

• Linux, Node.js

[jgaehring]

Is it the same if you exclude the land_type property from your filter?

[jgaehring]

I've been testing this against one of our test servers, test.farmos.dev (login required), using both Node and the browser. I used the parseFilter() function farmOS.js uses internally on the land_type portion of your filter, @braughtg:

const parsed = parseFilter({
  land_type: ["field", "bed"],
});
console.log(parsed);

That yielded the following url query parameters:

filter%5Bgroup-1%5D%5Bgroup%5D%5Bconjunction%5D=OR&filter%5Bland_type-0-filter%5D%5Bcondition%5D%5Bpath%5D=land_type&filter%5Bland_type-0-filter%5D%5Bcondition%5D%5Boperator%5D=%3D&filter%5Bland_type-0-filter%5D%5Bcondition%5D%5Bvalue%5D=field&filter%5Bland_type-0-filter%5D%5Bcondition%5D%5BmemberOf%5D=group-1&filter%5Bland_type-0-filter%5D%5Bcondition%5D%5Bpath%5D=land_type&filter%5Bland_type-0-filter%5D%5Bcondition%5D%5Boperator%5D=%3D&filter%5Bland_type-0-filter%5D%5Bcondition%5D%5Bvalue%5D=bed&filter%5Bland_type-0-filter%5D%5Bcondition%5D%5BmemberOf%5D=group-1

Cleaned up a bit for easier reading

?filter[group-1][group][conjunction]=OR
&filter[land_type-0-filter][condition][path]=land_type
&filter[land_type-0-filter][condition][operator]=%3D
&filter[land_type-0-filter][condition][value]=field
&filter[land_type-0-filter][condition][memberOf]=group-1
&filter[land_type-0-filter][condition][path]=land_type
&filter[land_type-0-filter][condition][operator]=%3D
&filter[land_type-0-filter][condition][value]=bed
&filter[land_type-0-filter][condition][memberOf]=group-1

Curiously enough, that test server has 25 beds and 8 fields (again, login required; but @mstenta you should be able to see these). The JSON payload I get back, however, only has the 25 beds. So perhaps it's something wrong with the way the filter parameters are structured for Drupal's JSON:API module? There is also no next page indicated by that JSON response's links property:

{
  "links": {
    "self": {
      "href": "https://test.farmos.dev/api/asset/land?filter%5Bgroup-1%5D%5Bgroup%5D%5Bconjunction%5D=OR&filter%5Bland_type-0-filter%5D%5Bcondition%5D%5Bpath%5D=land_type&filter%5Bland_type-0-filter%5D%5Bcondition%5D%5Boperator%5D=%3D&filter%5Bland_type-0-filter%5D%5Bcondition%5D%5Bvalue%5D=bed&filter%5Bland_type-0-filter%5D%5Bcondition%5D%5BmemberOf%5D=group-1"
    }
  }
}

Which would imply to me that there are no other records for that query. Compare a simpler request for all asset--plant entities from the same server, for which there should be 55 records in total:

{
  "links": {
    "next": {
      "href": "https://test.farmos.dev/api/asset/plant?page%5Boffset%5D=50&page%5Blimit%5D=50"
    },
    "self": {
      "href": "https://test.farmos.dev/api/asset/plant"
    }
  }
}

I tested that same asset--plant query using farmOS.js in Node:

const farm = farmOS({ remote });
farm.remote.authorize(username, password)
  .then(() => farm.schema.fetch())
  .then(() => {
      const filter = { type: 'asset--plant' };
      return farm.asset.fetch({ filter });
    })
    .then((response) => {
      console.log('PLANTS:\n', response.data.length);
    });

Which yielded:

PLANTS:
 55

So that's all to say I'm not exactly sure where the issue lies yet. We should first confirm that there are indeed more than 25 land assets with a land_type of either "bed" or "field", excluding all other land_type's. If that's the case, I suspect it's something to do with how the query is being parsed and formatted for Drupal. The chaining functionality in farmOS.js depends on that links.next url, and if there isn't a next page because there aren't any more records, then all is well. But if I'm mistaken in that assumption, that there will not always be a links.next url, even when there are more records, or if the query parameters are just being formatted incorrectly, then we'll need to figure out what the disconnect is between the JavaScript implementation and the Drupal specification.

I likely won't have the bandwidth to investigate this again until next week at the soonest, but hopefully with that added info, and with anything else @braughtg can learn from their end, @mstenta and others might be able to put the other pieces together. I created a gist with all my test code that others can try out, modify and adapt by cloning:

git clone https://gist.github.com/8fa99079e7597fffef88af41c9ebe01e.git limit-farmos-js
cd limit-farmos-js
npm i

Then create a .env file at the root directory with your credentials (assuming your server allows the fieldkit OAuth flow):

TEST_HOST="https://<your-farmos-url>"
TEST_CLIENT_ID="fieldkit"
TEST_USERNAME="<your-username>"
TEST_PASSWORD="<your-password>"

Then run:

npm test

[braughtg]

Is it the same if you exclude the land_type property from your filter?

No. If I exclude the the land_type property then I get 65 records which is what I would expect because I've got 5 asset-structure records.

Interestingly, if I do land_type: ["bed"] I get 25 records, which is the number of beds. But if I do land_type["field"] then I get 40 records, which is the number of fields.

So seems suspiciously as if the result is driven by the second (last?) value in the the land_type array.

To further confirm:

• If I use land_type: ["field", "bed"] I get the 25 records as documented in the original issue (which is the number of beds). but
• If I use land_type["bed", "field"] then I get the 40 records corresponding to the fields.

[braughtg]

Weirdly enough the following, which I think is supposed to be equivalent, returns no records.

  const land = await farm.asset.fetch({
    filter: {
      type: ["asset--land"],
      land_type: {
        $or: 
        [
          {$eq: "bed" },
          {$eq: "field"},
        ],
      }
    },
    limit: Infinity,

As does the abbreviated version without the $or.

[jgaehring]

So seems suspiciously as if the result is driven by the second (last?) value in the the land_type array.

Ah, OK, yea that makes me think again that there is something awry with the query params themselves, could be worth testing those in browser or REST API testing utility. Meanwhile, @mstenta, care to audit those params I pasted above against the Drupal JSON:API filter spec?

Responding from mobile so that's all I can recommend for now.

[paul121]

I think this is a minor issue with the query params. The land_type path is used twice, but both have the same "filter ID" land_type-0-filter so only one is used. This should be unique to each filter path/condition, like land_type-1-filter:

?filter[group-1][group][conjunction]=OR
&filter[land_type-0-filter][condition][path]=land_type
.....
&filter[land_type-0-filter][condition][path]=land_type

We should double check that this scenario is covered in farmOS.py as well. I forget if I didn't just use a random unique ID.. also might be good to shorten it as much as possible. Maybe find a strategy we should standardize on for this?

[jgaehring]

I think this is a minor issue with the query params. The land_type path is used twice, but both have the same \"filter ID\" land_type-0-filter so only one is used. This should be unique to each filter path/condition, like land_type-1-filter

Oh good catch! I'll try fix those params based on that and see if that does it. 🤞 If so, it hopefully will be a simple enough fix to the parsing logic.

[jgaehring]

I'll try fix those params based on that and see if that does it.

Bingo, right on the money, @paul121. :raised_hands:

I just quickly tested this out on the test server again, and got the expected 33 results, including land_type's of both bed and field. Here are those amended query parameters for easier viewing:

?filter[group-1][group][conjunction]=OR
&filter[land_type-0-filter][condition][path]=land_type
&filter[land_type-0-filter][condition][operator]=%3D
&filter[land_type-0-filter][condition][value]=field
&filter[land_type-0-filter][condition][memberOf]=group-1
&filter[land_type-1-filter][condition][path]=land_type
&filter[land_type-1-filter][condition][operator]=%3D
&filter[land_type-1-filter][condition][value]=bed
&filter[land_type-1-filter][condition][memberOf]=group-1

And if I had to guess, this is probably the culprit:

https://github.com/farmOS/farmOS.js/blob/63c047934a4cae22b5ab404ca208027f68fd3970/src/client/parse-filter.js#L76

Changing that to,

(params, f, i) => mergeParams(params, parser(f, label, logicDepth + i))

or even just,

(params, f, i) => mergeParams(params, parser(f, label, i))

could be all it takes, but I won't have a chance to test it out on my local machine until next week.

There's also a chance that the depth parameter needs to be included in the calls to parseComparison() here:

https://github.com/farmOS/farmOS.js/blob/63c047934a4cae22b5ab404ca208027f68fd3970/src/client/parse-filter.js#L83

and here:

https://github.com/farmOS/farmOS.js/blob/63c047934a4cae22b5ab404ca208027f68fd3970/src/client/parse-filter.js#L103

since the parameter is missing there, but I'm less confident about that; the "depth" or index in that function was really only intended to be used in the recursive call,

https://github.com/farmOS/farmOS.js/blob/63c047934a4cae22b5ab404ca208027f68fd3970/src/client/parse-filter.js#L67

but it probably wouldn't hurt anything to increment it there, too.

Weirdly enough the following, which I think is supposed to be equivalent, returns no records.

That is very strange, @braughtg, and I can't be sure, but I suspect it might also be caused by non-unique filter labels (eg, [land_type-0-filter]) .

[braughtg]

This all sounds great! Thanks for the quick diagnosis. I have also in the meantime implemented out a suitable workaround using 2 API calls instead of the one. So that will keep me moving. Thanks!

[braughtg]

We ran into another situation affected by this bug, but this one didn't have a decent work around. It really requires the filtering to happen on the servers side. So I took a whack at a PR to fix this as per the ideas above (which were spot on!) and also updated the tests.