dataProvider reuses filter object between requests

[Dreamsorcerer]

I'm not sure if you want to add a warning to the documentation, or use a fresh object in future to make the code more robust, but this totally caught me out.

To save me creating a custom referenceMany endpoint, I just did this in the dataProvider:

getManyReference: (resource, params) => {
    params["filter"][params["target"]] = params["id"];
    return dataRequest(resource, "get_list", params);
}

(compared to getList: getList: (resource, params) => dataRequest(resource, "get_list", params), it just adds to the filter)

But, the filter object is actually reused across requests, so each time I navigate to a different object, I was actually adding another filter value, thus breaking the requests.

Fixed by doing:

getManyReference: (resource, params) => {
    params["filter"] = {...params["filter"], [params["target"]]: params["id"]};
    return dataRequest(resource, "get_list", params);
},

[fzaninotto]

I don't understand the bug. Can you give an example and a reproduction?

[Dreamsorcerer]

I'm running a little short on time, but imagine you have 2 different resources, which both have a ReferenceManyField with different targets (e.g. foo_id and bar_id) in the list view.

If you use the code from the first example (replacing dataRequest() with whatever you need to do a get_list request), then when you view the list for the first resource and look at the network request made, you'll see filter={"foo_id": 1} or similar, which is the expected behaviour.

Then if you navigate to view the list of the second resource, you'll see requests made with filter={"foo_id": 1, "bar_id": 1} or similar. Likely the second resource has no foo_id, so the API will probably error or return no results. You need to refresh the page in order for it to create the correct request (e.g. filter={"bar_id": 1}).

This is because params.filter that is passed in is not a newly created object for each dataProvider call, but appears to be reused each time, so modifying the values actually persists across calls. Hence the second version shown above, which clones the filter object instead of modifying the original (so filter={"foo_id": 1, "bar_id": 1} never happens with this approach).

[fzaninotto]

I still don't understand. Would you mind building a reproduction based on the simple example StackBlitz?

https://stackblitz.com/github/marmelab/react-admin/tree/master/examples/simple

[Dreamsorcerer]

I need to run out, and can't get the ReferencManyField working properly in time.

But, you can still see the root of the problem here:
https://stackblitz.com/edit/github-tvi2ny?file=src/dataProvider.tsx

If you look at the console logs when the page loads the posts view for the first time, you'll see the first getMany log has a filter of {}, but the second request has {post_id: 2}. This is because the filter object has been reused across requests (and we modified it in the first request). Expected behaviour is that it would be {} for every request.

If you get a 2nd ReferenceManyField working on comments or something, then after loading the comments view you'd see a filter of {post_id: 2, comment_id: 2} or similar, which is when the API is likely to start failing.

[fzaninotto]

I still don't understand what I'm supposed to look at. Please be more specific.

Also, you should not modify the parameters passed as arguments to a function inside a function. Lots of react-admin code (and React code) is based on the assumption that you will not do that:

    getManyReference: (resource, params) => {
        params["filter"][params["target"]] = params["id"];
        return  fakeProvider.getList(resource, params)
    },

Otherwise, every function call should be done by value and not by reference, and we'd have to clone everything.

[Dreamsorcerer]

Also, you should not modify the parameters passed as arguments to a function inside a function. Lots of react-admin code (and React code) is based on the assumption that you will not do that

That is the issue, I was just suggesting that it be documented if sending a new object in every call is not something you want to do (which sounds like that is the react way...).

we'd have to clone everything.

Which is exactly the solution I've used, as shown in the original comment.

[slax57]

Thanks for your reply.
I'm not in favor of documenting this in the react-admin documentation either, because, to me, this is really down to the way JavaScript works, and this is not at all specific to react-admin.
As you figured out, you need to fix this in your dataProvider implementation by cloning the params before mutating them.
Hence I'll close this issue.

[Dreamsorcerer]

Maybe it's normal for react, but I think it has nothing to do with the way JavaScript works.

In a typical application, I'd expect a call to look something like:

function makeTheRequest() {
    let params = {filter: {post_id: 3}, ...}
    .... # Code that fills in any other values
    dataProvider.getMany(params)
}

I'm not sure why how it actually works in react, but it seems like the behaviour is something more like:

params = {filter: {}}  # global variable
function makeTheRequest() {
    params.filter.post_id = 3  # Code that fills in other values by modifying the global
    dataProvider.getMany(params)
}

As I say, I'm not familiar with React, but in other projects/languages that latter would be considered bad practice, and that's why this caught me out. Reusing the same mutable object for every request just seems unusual to me.

If that's the React way, then feel free to leave it, but as a non-React developer I did not expect this behaviour.

[Dreamsorcerer]

If you also use async code, I don't understand how this is meant to work correctly.

e.g. If some code is awaiting inside a getMany() request, and a new, different, getMany() request is made, then the object in the first request is going to be changed and potentially break the code. (Probably, there are not multiple simultaneous calls to getMany() for most people, so maybe it never happens in reality).

[slax57]

What I meant by "it's the way JavaScript works" was referring to passing the parameters by value vs. by reference, and the need to clone the parameters with the latter.
In any case, react-admin expects dataProvider functions to be pure functions, which is a good practice in general.
I don't think it's relevant to mention this in the RA docs, because it's a common assumption to make, but if the community disagrees (and you are free to disagree), I'll reopen this issue and document this caveat.

[Dreamsorcerer]

react-admin expects dataProvider functions to be pure functions

Sure, but from that link:

pure functions are functions that don’t modify the global state and don’t depend on any external input

What I provided was a pure function, it only modified the input given to it as an argument. The fact that a global, reusable object was passed to the function is the only thing I'm questioning (why pass the object as an argument if it's global anyway?).