This repository has been archived by the owner. It is now read-only.

$in operator doesn't work correctly through rest #88

Closed
sunabozu opened this Issue Nov 22, 2016 · 15 comments

Comments

Projects
None yet
2 participants
@sunabozu

sunabozu commented Nov 22, 2016

I post it here since exactly the same code works fine with feathers-socketio.

So when I use the $in operator and provide an array of values for a field, the result query consists of an object, not an array. For example, the following query

.find({
  query: {
    subfile: state.subfile._id,
    number: {
      $in: [0, 1, 2]
    }
  }
})

results in the following hook.params.query value:

{
  number: {
    $in: { 0: '0', 1: '1', 2: '2' }
  }
...
}

or see the debug console output:

screen shot 2016-11-22 at 19 16 03

This obviously doesn't pass as a correct query (in my case for Mongoose). As I mentioned, I see no such problem using socketio.

@daffl

This comment has been minimized.

Member

daffl commented Nov 22, 2016

What library are you using and what is the query string that gets passed?

@sunabozu

This comment has been minimized.

sunabozu commented Nov 22, 2016

I'm not sure what you mean by library. I use the standard Feathers client.

The query string is

http://localhost:3030/subtitles?subfile=581b5574cde74e349157c5f2&number%5B%24in%5D%5B0%5D=98&number%5B%24in%5D%5B1%5D=99&number%5B%24in%5D%5B2%5D=100&number%5B%24in%5D%5B3%5D=101&number%5B%24in%5D%5B4%5D=102&number%5B%24in%5D%5B5%5D=103&number%5B%24in%5D%5B6%5D=104&number%5B%24in%5D%5B7%5D=105&number%5B%24in%5D%5B8%5D=106
@daffl

This comment has been minimized.

Member

daffl commented Nov 22, 2016

I meant what REST client.

@sunabozu

This comment has been minimized.

sunabozu commented Nov 22, 2016

Well, I use feathers/client and feathers-rest/client.

@daffl

This comment has been minimized.

Member

daffl commented Nov 22, 2016

feathers-rest/client has to be configured with a library like jQuery, fetch or superagent.

Also, try setting app.set('query parser', 'extended'). When I try the following:

const qs = require('qs');

qs.parse('subfile=581b5574cde74e349157c5f2&number[$in][0]=98&number[$in][1]=99&number[$in][2]=100&number[$in][3]=101&number[$in][4]=102&number[$in][5]=103&number[$in][6]=104&number[$in][7]=105&number[$in][8]=106');

I get

{ subfile: '581b5574cde74e349157c5f2',
  number: { '$in': [ '98', '99', '100', '101', '102', '103', '104', '105', '106' ] } }
@sunabozu

This comment has been minimized.

sunabozu commented Nov 22, 2016

Oh, I understand now. I use regular window.fetch in Chrome.

I tried to set the query parser option, but it didn't help.

@sunabozu

This comment has been minimized.

sunabozu commented Nov 23, 2016

I just checked, the same happens when I use jQuery. Though qs gives me correct output, too.

@daffl

This comment has been minimized.

Member

daffl commented Nov 23, 2016

Yeah, I don't think it's the client. Pretty weird though because Express claims that it is using qs to do extended query string parsing. I'll add some tests but for now you could just add your own middleware that does that, something like

const url = require('url');
const qs = require('qs');

app.use(function(req, res, next) {
  const query = url.parse(req.url).query;
  req.query = qs.parse(query);

  next();
});

And see if that does it.

@daffl

This comment has been minimized.

Member

daffl commented Nov 26, 2016

I just added tests for this in #90 and can't reproduce the problem. It looks like at least on the test server it is coming through fine. Maybe it is some other configuration issue?

@daffl daffl closed this in #90 Nov 26, 2016

@sunabozu

This comment has been minimized.

sunabozu commented Nov 26, 2016

Okay, I just made a query manually and got a correct result ($in is array). But when I do the same query through the client I get $in as an object and it doesn't work. That's weird. Have you checked it with the client?

@daffl

This comment has been minimized.

Member

daffl commented Nov 26, 2016

I added tests for it for all REST clients in #90 (see e.g. https://github.com/feathersjs/feathers-rest/blob/master/test/client/fetch.test.js#L47) and they all pass.

Can you share a setup that reproduces your problem?

@daffl daffl reopened this Nov 26, 2016

@sunabozu

This comment has been minimized.

sunabozu commented Nov 27, 2016

I spent like an hour to figure out what's the problem here, and it seems super weird.

So, just try to pass the following array:

[1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13, 14, 15, 16, 17, 18, 19, 20, 21, 22]

That's how I reproduced it: the $in field becomes an object. However, if you delete just one element in that array, it works as intended. So I guess the issue has something to do with the length of the query string. Maybe the parser switches to a different mode when you pass it a string of some particular length. I suppose you use a different parser for socket.io requests, because I don't have such problem with it.

The weirdest part is that only the length of that array matters. If you add another fields to the query, it doesn't change anything.

@daffl

This comment has been minimized.

Member

daffl commented Nov 29, 2016

That did sound strange but there seems to be an explanation for it in the qs documentation on array limiting.

qs will also limit specifying indices in an array to a maximum index of 20. Any array members with an index of greater than 20 will instead be converted to an object with the index as the key.
...
This limit can be overridden by passing an arrayLimit option:

Looks like the solution is to implement your own query parser using qs with that option (it might be possible to set the options in the one used in Express as well but I'm not sure how).

@daffl daffl closed this Nov 29, 2016

@sunabozu

This comment has been minimized.

sunabozu commented Nov 29, 2016

Thanks, that explains everything. The solution is simple, you just need to set the query parser option to a custom function, such as:

app.set('query parser', str => qs.parse(str, {arrayLimit: 1000}))

It would be nice if you added it into the core, it may save a lot of hours of debugging for some people.

@daffl

This comment has been minimized.

Member

daffl commented Nov 29, 2016

This is more of a problem with Express's default settings and we usually try to not change those.

It would be great if you could add it in the Pro tip at https://docs.feathersjs.com/rest/readme.html#query-route-and-middleware-parameters (there is an "Edit page" link at the top) though.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.