benwinding / react-admin-import-csv

A csv file import button for react-admin
https://benwinding.github.io/react-admin-import-csv
MIT License
135 stars 46 forks source link

Is updateMany the right method to call? #55

Closed Carac0les closed 3 years ago

Carac0les commented 3 years ago

Hello,

First thank you very much for the work, this feature is awesome. I came accros something while getting this to work for me.

Context

My use case is to only allow updating records, so I used the disableImportNew: true option. When updating records, react-admin-import-csv calls the dataProvider method updateMany.

I realised that the format of the data sent to the updateMany method of the data provider is as follow:

{
    ids : [1,2],
    data : [
        {
            id: 1,
            text: "Some text"
        },
        {
            id: 2,
            text: "Some other text"
        }
    ]
}

I am not sure this is the intended format for updateMany in React admin. From the documentation it seems like updateMany is more intended to update many records with the same data, rather than a different change for each record. (see https://marmelab.com/react-admin/DataProviders.html#request-format and the example implementation).

In my case, I am using feathersjs for my API, and using the ra-data-feathers module to create the react-admin dataProvider. That module is definetly written in a way that updateMany expects the same change for many records.

//extract from ra-data-feathers/src/restClient.js line 94 - 102
case UPDATE_MANY:
        if (usePatch) {
          const dataPatch = params.previousData
            ? diff(params.previousData, params.data) : params.data;
          return Promise.all(params.ids.map(id => (service.patch(id, dataPatch)))); //--> Send the same data to all ids
        }
        const dataUpdate = (idKey !== defaultIdKey)
          ? deleteProp(params.data, defaultIdKey) : params.data;
        return Promise.all(params.ids.map(id => (service.update(id, dataUpdate))));

So in the very specific case of feathersjs, I could get it to work by writting a before hook that would isolate the id in the data.

My question:

Is updateMany the appropriate dataProvider method to call here, or should update be called for every single record to update instead? I know this can potentially increase the number of requests, but may be more in line with the intended use of the methods?

In the case of feathersjs and ra-data-feathers, the updateMany method triggers 1 patch call per id anyway. I don't know how other dataProviders are written, so it might not be an issue for others.

benwinding commented 3 years ago

Hi @Carac0les, Hmm, yes it seems you are correct, the dataprovider method updateMany is used for bulk updating of the same data, not an array of existing data.

I don't know how other dataProviders are written

Yeah I think some of them are implemented incorrectly.

Perhaps a new method like updateManyArray would be useful.

Try Dataprovider Method Fallback
createMany create
updateManyArray update

Might take a little while to implement, but we'll see

Carac0les commented 3 years ago

Hello,

Thanks for the fast reply.

Your suggestion seems sensible to me. I really lack the knowledge of other dataProviders and how common it is to accomodate for an updateManyArray method is, so it is hard for me to bring anything else to the table unfortunately.

Thanks.