strongloop / strong-remoting

Communicate between objects in servers, mobile apps, and other servers.
www.strongloop.com
Other
105 stars 93 forks source link

Querystring behaviour is inconsistent when integrating other Loopback APIs #324

Closed horiaradu closed 7 years ago

horiaradu commented 8 years ago

Loopback has a particular interface for REST APIs. It allows you to write a query like so:

Model.find({where: {id: {inq: [1, 2, 3]}}})

However, this query is also valid: Model.find({where: {id: {inq: []}}}), but it will obviously not return any results.

In this case, the generated query looks like this:

http://localhost:3010/api/model?filter=%7B%22where%22%3A%20%7B%22id%22%3A%20%7B%22inq%22%3A%20%5B%5D%7D%7D%7D

and decoded, like this:

http://localhost:3010/api/model?filter={"where": {"id": {"inq": []}}}

The problem is inside http-invocation.js, line: 215.

// query is an object: query = {where: {id: {inq: []}}}
qs.stringify(query) //this will return an empty string

Querystring behaviour (from node REPL):

> qs.stringify({where: {id: {inq: [1]}}})
'where%5Bid%5D%5Binq%5D%5B0%5D=1'
> qs.stringify({where: {id: {inq: []}}})
''
>

If you are integrating another loopback API, when you do the query with an empty array, you expect to get back an empty collection of items, but instead, because of the behaviour of querystring, you get back all the items.

See related issues:

BoLaMN commented 8 years ago

same as #320

richardpringle commented 8 years ago

Hey @horiaradu, what version of loopback, strong-remoting, and loopback-datasource-juggler are you using?

I just tried to reproduce the issue and it was working as expected. Either that, or I don't entirely understand the issue. You're saying that when you use an empty array, it returns all the results instead of none?

BoLaMN commented 8 years ago

The issue I posted is still relevant in how querystring handles arrays throwing object ids on top just makes the whole scenario worse.

I was getting 413 errors due to the querystring being to large for nginx to process, check my referenced issue to find an example in tonicdev you can play with.

In the mean time I too have forked and introduced a similar patch.

cheers

horiaradu commented 8 years ago

@richardpringle

loopback: 2.29.1 strong-remoting: 2.28.1 loopback-datasource-juggler: 2.47.0

More detailed scenario:

There is a model Car who is backed by a MySQL DB (classic model). This has a belongsTo relationship to another model: Owner. The Owner is backed by a remote datasource (via strong-remoting) which points to a second loopback API.

I am doing a query like bellow:

Car.find({where: {id: -1}, include: ['owner']})

I've put the id == -1 predicate to stress the fact that I don't get any results from Car.

So, I don't get any results from Car, but because I did the include, the juggler, was trying to still query the Owner (this is now optimized via: https://github.com/strongloop/loopback-datasource-juggler/pull/1007). When doing the query, it wanted to "get all the owners with id included in []", which translated to the REST call I detailed above:

http://localhost:3010/api/model?filter={"where": {"id": {"inq": []}}}

And here is where querystring appears into play and strips out the array and reformulating the query to be: "get all the owners".

richardpringle commented 8 years ago

Sorry @horiaradu, I didn't quite understand what you were describing right away. I still can't reproduce the issue with strong-remoting alone (I'm on 2.29.0, no sure if that makes a difference).

I get an empty array when I try to hit the following: http://localhost:3010/api/model?filter={"where": {"id": {"inq": []}}}

I do however see that when you pass {where: {id: {inq: []}}} into qs.stringify(), it returns an empty string... I'm just not quite sure where this is happening.

Thanks for your patience, I'm sure this is easier to understand than I am making it.

horiaradu commented 8 years ago

@richardpringle

I have updated my PR so that the tests pass (forgot a line...).

gunjpan commented 7 years ago

Landed.