marmelab / react-admin

A frontend Framework for single-page applications on top of REST/GraphQL APIs, using TypeScript, React and Material Design
http://marmelab.com/react-admin
MIT License
24.81k stars 5.23k forks source link

Data Provider 'update' function: params.data has updated ID but also carries old body JSON fields #4861

Closed paulito415 closed 4 years ago

paulito415 commented 4 years ago

What you were expecting:

I have two resources Article and Author and an Article can have one single Author field with name author.

My expectation is that when I change the value of the author field in an Article during Edit, the submission of an Article update should trigger a call to my data provider's update function passing in a JSON of Article that simply updates the ID of the nested author field, i.e. something that looks like:

{
   "id":2,
   "title":"Some Title",
   "url":"https://some.url.com",
   "imageUrl":"https://some.image.url.com",
   "visible":true,
   "startVisible":"2019-09-29T11:11:00Z",
   "endVisible":"2019-10-29T07:02:00Z",
   "author":{
      "id":6
   }
}

What happened instead:

Contrary to my expectation, when I change the value of the author field in an Article during Edit, the submission of an Article update actually triggered a call to my data provider's update function passing in a JSON of Article that embeds the complete JSON of the nested author field, i.e. something that looks like:

{
   "id":2,
   "title":"Some Title",
   "url":"https://some.url.com",
   "imageUrl":"https://some.image.url.com",
   "visible":true,
   "startVisible":"2019-09-29T11:11:00Z",
   "endVisible":"2019-10-29T07:02:00Z",
   "author":{
      "id":6,
      "blogSubTitle":"Old Blog Subtitle",
      "blogTitle":"Old Blog Title",
      "name":"Old Author Name1",
      "introduction":"Old Intro",
      "imageUrl":"https://old.image.url.com"
   }
}

Steps to reproduce:

In my Edit for Article I have:

export const ArticleEdit = props => (
    <Edit {...props}>
        <TabbedForm>
            <FormTab label="summary">
                <TextInput disabled source="id"/>
                <TextInput source="title" />
                <TextInput source="url" />
                <TextInput source="imageUrl" />
                <ReferenceInput source="author.id" reference="author" label="resources.feed/article.fields.author">
                    <SelectInput optionText="name" />
                </ReferenceInput>
            </FormTab>
            <FormTab label="campaign">
                <DateInput source="startVisible" parse={dateParser}/>
                <DateInput source="endVisible" parse={dateParser}/>
                <BooleanInput source="visible"/>
            </FormTab>
        </TabbedForm>
    </Edit>
);

At first glance, everything seems fine in that when I load the Article Edit page, I see the right REST API Data Provider functions (getList and getMany) get called for Author and thereafter the proper value show up for the author field, using thenameattribute ofAuthor`.

And when I choose a different Author in the drop down, the right REST API Data Provider calls get made for getMany too.

But when I click on the SAVE button, and having printed out the the params.data in my custom data provider, I see an unexpected JSON body as mentioned above.

Other information:

Environment

fzaninotto commented 4 years ago

I can't reproduce the problem you're describing in the simple example. I suspect a problem in your dataProvider.

Please provide a link to a reproducible test case based on the Simple Example CodeSandbox.

paulito415 commented 4 years ago

Thanks @fzaninotto I suspected as much. Maybe it's because my response for getMany or getManyReference was returning a full JSON for Author and somehow it was packaging that up for setting the body of the Article update? Is there any place where this "specification" is documented?

paulito415 commented 4 years ago

Actually I just checked the documentation again and it seems I should be doing the right thing for getMany and getManyReference (actually I don't even think getManyReference was being called in my case).

djhi commented 4 years ago

Please provide a link to a reproducible test case based on the Simple Example CodeSandbox.

paulito415 commented 4 years ago

I think I understand now. Whatever react-admin uses as a basis for update is what it first got using getOne of my Article. So put it another way, if the result of getOne was:

{
   "id":2,
   "title":"Some Title",
   "url":"https://some.url.com",
   "imageUrl":"https://some.image.url.com",
   "visible":true,
   "startVisible":"2019-09-29T11:11:00Z",
   "endVisible":"2019-10-29T07:02:00Z",
   "author":{
      "id":5,
      "blogSubTitle":"Old Blog Subtitle",
      "blogTitle":"Old Blog Title",
      "name":"Old Author Name1",
      "introduction":"Old Intro",
      "imageUrl":"https://old.image.url.com"
   }
}

then the body used for update (from Author ID 5 to 6) will just be:

{
   "id":2,
   "title":"Some Title",
   "url":"https://some.url.com",
   "imageUrl":"https://some.image.url.com",
   "visible":true,
   "startVisible":"2019-09-29T11:11:00Z",
   "endVisible":"2019-10-29T07:02:00Z",
   "author":{
      "id":6,
      "blogSubTitle":"Old Blog Subtitle",
      "blogTitle":"Old Blog Title",
      "name":"Old Author Name1",
      "introduction":"Old Intro",
      "imageUrl":"https://old.image.url.com"
   }
}

I did a test on this by adding some arbitrary field in the author JSON and as I suspected, it showed up in the body of author during an update for Article.

Given this discovery, I can fix my problem by basically removing the rest of the fields like name, blogTitle, ... inside of the author JSON and only keeping the id field be done with it. And I guess it's because the usual expected call pattern for such a REST API is one where the "client" would get:

{
   "id":2,
   "title":"Some Title",
   "url":"https://some.url.com",
   "imageUrl":"https://some.image.url.com",
   "visible":true,
   "startVisible":"2019-09-29T11:11:00Z",
   "endVisible":"2019-10-29T07:02:00Z",
   "author":{
      "id":6
   }
}

from the Article's getOne call with ID 2 and then have to follow up with an Author's getOne call with ID 6. But I posit that this expectation is a rather big limitation and doesn't always actually sound like the right thing to do. My REST API (and more specifically its client) requires the "full" Article fields including its embedded author fields to be sent in such a call. Breaking it up into two calls will impact performance at runtime. And I choose not to prioritize the functionality of my Admin UI over the runtime performance of my REST API. Dare I say that there must be a better way to do this?

I propose that no matter what the returned JSON body of the getOne call of the Article is, an update is always done with only the ID of the embedded object. Another reason for doing this is that at least with my implementation of the REST API using Grails, the default behavior is one where if the author fields are included, it actually binds the embedded author with said field values and modifies the target author 😱 . In any case, I'd love to hear any other differing thoughts on this proposal. If it does get "approved", would this be classified as an Enhancement Request and require that I file a new issue for that?

fzaninotto commented 4 years ago

React-admin keeps a local store of all the records your app has requested. These records are shared between views, and requests. So yes, if the getOne() call returned an embedded record, the update() record will contain it, to.

That's a design choice, and that's what allows us to provide significant performance improvements and optimistic rendering. This centralized store cannot be disabled.

In your case, you probably need to tweak the update() method in your dataProvider to remove the embedded record if the resource is article.

Anyway, as this is not a react-admin bug, I'm closing the issue.

paulito415 commented 4 years ago

Thanks @fzaninotto for your comment. I understand and agree with your position with respect to keeping the centralized local store as a reason to keep these performance improvements and optimistic rendering. In fact, I was not actually suggesting that this store be removed though. My point was that even though I can do some pre-processing in my dataProvider's update() implementation for article, as you correctly pointed out, it would not be architecturally as elegant because I would need to do this not only for articles / authors but also other entities down the road that may have such an embedding relationship.

It boils down to whether we see this pre-processing as generic enough to generalize in my dataProvider's update method (for all my entities), or more generic than that to push this into the react-admin pre-processing. I guess the answer is that it's generic enough to put in my dataProvider but not generic enough to put in the calling code of react-admin.

I understand and appreciate that decision and know that I might have some more work cut out for me to make this work. But since I have control over my Grails REST controller I can devise something on that end to reduce work on the react-admin usage end. What I actually ended up deciding was to introduce a URL parameter that makes it render shallow objects like the way react-admin expects and in my dataProvider's getOne call, I post-pend my URL with this URL parameter. Not the most elegant either but beats having to process JSON in JS code and remove select fields before updating.

Thanks again for looking into this and for the explanation on the local store.