feathersjs / feathers

The API and real-time application framework
https://feathersjs.com
MIT License
14.98k stars 742 forks source link

rest-client - incorrect array bracket format breaks on large queries #2583

Open skinofstars opened 2 years ago

skinofstars commented 2 years ago

The rest-client request for $in doesn't match what the docs say. This can cause problems with parsing large arrays.

Docs https://docs.feathersjs.com/api/databases/querying.html#in-nin

Steps to reproduce

app.service('messages').find({
  query: {
    roomId: {
      $in: [ 2, 5 ]
    }
  }
});

Expected behavior

GET /messages?roomId[$in][]=2&roomId[$in][]=5

Actual behavior

GET /messages?roomId[$in][0]=2&roomId[$in][1]=5

In a large array, you get an error along the lines of

"select count(\"messages\".\"id\") as \"total\" from \"messages\" where \"$in\" = $1 and \"$in\" = $2 and \"$in\" = $3 and \"$in\" = $4 and \"$in\" = $5 and \"$in\" = $6 and \"$in\" = $7 and \"$in\" = $8 and \"$in\" = $9 and \"$in\" = $10 and \"$in\" = $11 and \"$in\" = $12 and \"$in\" = $13 and \"$in\" = $14 and \"$in\" = $15 and \"$in\" = $16 and \"$in\" = $17 and \"$in\" = $18 and \"$in\" = $19 and \"$in\" = $20 and \"$in\" = $21 and \"$in\" = $22 and \"$in\" = $23 and \"$in\" =…131 and \"$in\" = $132 and \"$in\" = $133 and \"$in\" = $134 and \"$in\" = $135 and \"$in\" = $136 and \"$in\" = $137 and \"$in\" = $138 and \"$in\" = $139 and \"$in\" = $140 and \"$in\" = $141 and \"$in\" = $142 and \"$in\" = $143 and \"$in\" = $144 and \"$in\" = $145 and \"$in\" = $146 and \"$in\" = $147 and \"$in\" = $148 and \"$in\" = $149 and \"$in\" = $150 and \"$in\" = $151 and \"$in\" = $152 and \"$in\" = $153 and \"$in\" = $154 and \"$in\" = $155 and \"$in\" = $156 - column \"$in\" does not exist"

System configuration

Solution/work around

Initially it appears to be an issue with the qs limit of 21 array items, however the recommended workaround of updating the parser didn't work https://github.com/feathersjs/docs/blob/ec326978d46ba33d98d48aa61735b72b5f329a53/help/faq.md#why-are-queries-with-arrays-failing

var qs = require('qs');

app.set('query parser', function (str) {
  return qs.parse(str, {
    arrayLimit: 100
  });
});

What did work was changing the query to match the docs by applying the arrayFormat: 'brackets' option.

class CustomFetch extends FetchClient {
  getQuery(query) {
    if (Object.keys(query).length !== 0) {
      const queryString = qs.stringify(query, {
        arrayFormat: 'brackets',
      });

      return `?${queryString}`;
    }

    return '';
  }
}

app.configure(restClient.fetch(window.fetch, CustomFetch));
daffl commented 2 years ago

This is documented in the FAQ at https://docs.feathersjs.com/help/faq.html#why-are-queries-with-arrays-failing

skinofstars commented 2 years ago

Thanks for getting back so quick. My bug report is that I believe both the main docs are wrong, and that the FAQ documentation/workaround is also wrong.

daffl commented 2 years ago

Ah sorry about that. It sounds like the arrayFormat should always be brackets when using qs then?

skinofstars commented 2 years ago

I believe so, yes. Happy to create a PR if we don't think it'll break people's apps.

DaddyWarbucks commented 2 years ago

This seems odd that using arrayLimit does not solve this. That seems like a bug with qs. Weird.

This problem definitely comes into play when using feathers-dataloader from the client, which produces lots of $in: [...] queries. Which can be seen here: https://test-feathers-client-joins.herokuapp.com/

Browsers also enforce a max URL length. See this Stackoverflow. Setting arrayFormat: 'brackets' will combat that because it will naturally create shorter URLs.

I have done some previous work in this area that can be seen here too: https://github.com/DaddyWarbucks/feathers-fletching/blob/master/src/plugins/strictRestQuery.js

If we were to change the parser/stringifier for feathers, we could consider https://www.npmjs.com/package/query-string as well. It also handles parsing number and booleans (similar to the strictRestQuery above). But there are caveats to that, for example the user may actually want string "true" and not boolean true. Or they may want string 001 instead of number 1. That is really a job for feathers-schema now, but I figured I would throw it out there.