josx / ra-data-feathers

A feathers rest client for react-admin
MIT License
157 stars 58 forks source link

Nested query filter support is now broken #90

Closed lijoantony closed 5 years ago

lijoantony commented 5 years ago

I have recently updated my project to use react-admin and ra-data-feathers. After the update I have found that the nested query filters are not working any more. I have found the following after investigation.

As an example consider react admin is sending this request to ra-data-feathers,

restClient(GET_LIST, 'users', {
    filter: { createdAt: { $gte: "2018-12-24T14:56:14.274Z" } },
    sort: { field: 'createdAt', order: 'DESC' },
    pagination: { page: 1, perPage: 100 },
})

ra-data-feathers would receive this filter,

filter: {
    createdAt: {
        $gte: "2018-12-24T14:56:14.274Z"
    }
}

This is the format expected by the feathers-client library. Earlier ra-data-feathers versions would simply sent this to feathers-client, and it would encode it correctly as createdAt[$gte]: 2018-12-24T15:24:05.521Z before sending it to feathers-server.

However, this line of code

Object.assign(query, fetchUtils.flattenObject(params.filter));

at https://github.com/josx/ra-data-feathers/blob/master/src/restClient.js#L53 flattens the filter object to,

createdAt.$gte: "2018-12-24T14:56:14.274Z"

and sends this to feathers-client. Since this is not the filter format expected by the feathers-client it sends it as it is to the feathers-server. As a result server issues a db query with aWHERE user.createdAt.$gte="2018-12-24T14:56:14.274Z" to the database and the database throws a column does not exist error.

I see that the problematic line (ie, flattening of filters) was added in #53. I am not sure about's it's objective. If the flattening was intended for some client library other than feathers-client, then I believe the flattening should be done within that client library. Hence I feel the fix would be to simply revert it back to

Object.assign(query, params.filter);

I can send a PR for the same. @josx What do you think?

josx commented 5 years ago

Sorry the delay i was in vacations. A few things:

  1. Your are right on how to send $lte, $gt and this kind of queries
  2. Tests are not passing, because of your change (because doesnt support deep props)
  3. how to manage deep props on filter keys on feathers

Example of 3 (right now it is working like this):

from react-admin

{
         name: 'john',
         address: {
           city: 'London',
         }
}

Must send to feathers-client:

{
         name: 'john',
         'address.city': 'London',
}

So I think that the current work you have done, must be changed with a fn that flat only props not in ($lte, $gt, and the other on the feathers doc) check https://docs.feathersjs.com/api/databases/querying.html

In other words We need a conditional flatting object function. Can you do a PR for that?

nicholasnelson commented 5 years ago

EDIT Sorry, nevermind, I was doing it wrong. Disabled the "make" command from npm run test because I'm using Windows currently. Forgot to re-run the babel build manually after making changes.

Hey josx,

Tests are not passing, because of your change (because doesnt support deep props)

Are you sure about this? I've just run the test suite at the current commit (104260ea1077d0f6fdd5761f1871e2b1f82b7d5f) and it appears there is a test failing in the latest master commit at least.

1) Rest Client when called with GET_LIST formats params into a query and pass it to client: AssertionError: expected false to be true

When I add in the changes from #91 , I get the same failed test + no new failing tests.

josx commented 5 years ago

Can you @nicholasnelson @lijoantony validate my approach in nested props? (the difference between nested props and modifiers like $lte )

nicholasnelson commented 5 years ago

I'll take a look now @josx. Back in a Linux environment finally so should have less trouble than I did with my last attempt.

nicholasnelson commented 5 years ago

In other words We need a conditional flatting object function.

Feathers Client seems to already handle this. We should just be able to pass in a query like: service.find({ someValue: { $gt: 10 } })

Feathers Client then (when using Rest) uses the qs library to stringify this query to: someValue%5B%24gt%5D=10 or someValue[$gt]=10 without URL encoding, which is the correct format for built in operands per https://docs.feathersjs.com/api/client/rest.html#find

There is no mention in the Feathers documentation about using operands with queries in socketio. I believe the query format should be identical when using the Rest client and the socketio client.

I've created a PR #94 fixing this and updating the relevant test.