react-admin icon indicating copy to clipboard operation
react-admin copied to clipboard

submitting edit form creates fields with empty string ("") value in a record instead of keeping them uninitialized

Open SerhiyAndryeyev opened this issue 3 years ago • 2 comments

What you were expecting: I have SimpleForm form with a <TextInput source="subtitle" /> but subtitle does not exist in fetched data. When a user saves the form without changing the "subtitle" I expect this field to be empty or undefined because the user did not modify it.

What happened instead: When the form is submitted the data field "subtitle" gets an empty string ("") value. This breaks the integrity of our data because its field is not supposed to be assigned in this way. In the previous version (v3) of react-admin the field was not created if it is not explicitly filled by a user

Steps to reproduce: original data loaded to form:

{
  "userId": 1,
  "id": 3,
  "title": "ea molestias quasi exercitationem repellat qui ipsa sit aut",
  "body": "et iusto sed quo iure\nvoluptatem occaecati omnis eligendi aut ad\nvoluptatem doloribus vel accusantium quis pariatur\nmolestiae porro eius odio et labore et velit aut"
}

The data after submit(pay attention to "subtitle" field added:

{
  "userId": 1,
  "id": 3,
  "title": "ea molestias quasi exercitationem repellat qui ipsa sit aut",
  "body": "et iusto sed quo iure\nvoluptatem occaecati omnis eligendi aut ad\nvoluptatem doloribus vel accusantium quis \nmolestiae porro eius odio et labore et velit aut",
  "subtitle": ""
}

Related code: the code of the form below. This is actually from react-admin tutorial

export const PostEdit = props => (
    <Edit mutationMode="pessimistic">
        <SimpleForm>
            <TextInput disabled source="id" />
            <ReferenceInput source="userId" reference="users">
                <SelectInput optionText="name" />
            </ReferenceInput>
            <TextInput source="title" />
            <TextInput source="subtitle" />
            <TextInput multiline source="body" />
        </SimpleForm>
    </Edit>
);
  • CodeSandbox:
  • https://codesandbox.io/s/react-admin--empty-string-issue-m15d7o?file=/src/App.js

Other information: Using parse method to convert empty strings to undefined does not help because empty string value ("") is also a valid value for some fields. I tried to implement detection of dirty fields using a combination of useFormState and transform callback but it seems strange that this functionality is not supported out of the box

SerhiyAndryeyev avatar Jul 13 '22 15:07 SerhiyAndryeyev

Related to https://github.com/marmelab/react-admin/issues/7782?

djhi avatar Jul 13 '22 15:07 djhi

a bit similar but not. In my case API simply doesn't have some values. But they are assigned to "" when data is sent back on submit

SerhiyAndryeyev avatar Jul 13 '22 15:07 SerhiyAndryeyev

Hi @SerhiyAndryeyev I have been trying to reproduce your issue, but the codesandbox you provided leads to too many errors, and it is not easy for me to work with it. Could you rather create a new codesandbox based from https://codesandbox.io/s/github/marmelab/react-admin/tree/master/examples/simple and change the minimum amount of code to reproduce? This would also allow us to see all request and responses made from and to the dataProvider, because the logs are enabled by default in our example. Thanks

slax57 avatar Aug 22 '22 16:08 slax57

I'm seeing the same, we have a NumberInput in an ArrayInput and it's now submitting an empty string instead of nothing in v4 compared to v3, which of course breaks our graphql backend. Using parse has no impact unless the input is interacted with.

If Serhiy doesn't come through with a codesandbox, I'll see if I can come up with one.

martdavidson avatar Aug 22 '22 18:08 martdavidson

Hi, @slax57 I modified the CommentEdit.tsx file(line 135), and also added log on update call in dataProvider. You can see that the "newField" is set to "" after submit.

https://codesandbox.io/s/react-admin-demo-issue-0hted9?file=/src/comments/CommentEdit.tsx Screenshot 2022-08-23 133423

SerhiyAndryeyev avatar Aug 23 '22 10:08 SerhiyAndryeyev

Same as #8060 ?

dylanlt avatar Sep 06 '22 23:09 dylanlt

Same as #8060 ?

yes, the same

SerhiyAndryeyev avatar Sep 07 '22 09:09 SerhiyAndryeyev

Hi guys, Sorry I took so long to answer.

So indeed, this is actually a breaking change from v3 to v4, due to the change of the forms library. As explained here, react-hook-form does not handle undefined values (because it uses uncontrolled inputs by default).

native input will not return undefined, and this library doesn't support undefined as well

Native web inputs don't return undefined, only empty strings in case of an empty value.

This is what is implemented in react-admin.

If you need to workaround this, you can use the parse prop on a per-input basis, or the transform prop on the <Create>, <Edit>, or <SaveButton> components. See the docs here: https://marmelab.com/react-admin/Upgrade.html#sanitizeemptyvalues-has-been-removed

Since we do not plan to change this, I'm going to close this issue.

slax57 avatar Sep 08 '22 07:09 slax57

Probably there was misunderstanding of the prolem. We for instance work with mongodb which doesnt have fixed data scheme. Some values (randomly) are simply not fetched to the form. So I expected react admin to handle correctly the case when those fields stay untouched on save. So they should not be written back to DB. Unfortunatelly because the way react-hook-form was integrated it became impossible in v4.
To overcome this problem it is not suficcient to parse values you also need to rack which values are untouched (not dirty). Unfortunatelly the transform of the Edit does not have an access to the Form state (cant use useFormState), so this context need somehow to be transfered from inside the form to the ancestor Edit conrol.
I made some quick and dirty solution , but I believe it can be made in much more ellegant way inside reacr-admin edit or other related contexts

const SafeEditForm = ({ children }: { children: ReactNode }) => {
   
    const dirtyFields = useRef({});
    
    const handleTransform: TransformData = (data, options: any) => {
        const { previousData } = options;
        const updates = filter(dirtyFields.current, data);
        const final = merge({ ...previousData }, updates);
        return final;
    }

    const WrapChildren = useCallback(() => {
        const { dirtyFields: dirty } = useFormState();
        dirtyFields.current = dirty;
        return <>{children}</>
    }, [children])


    return (
        <Edit transform={handleTransform}>
            <Form>
                .....

            </Form>
        </Edit>
    )
}


I hope to see it reflected in some future version of ra

pablos44 avatar Sep 09 '22 20:09 pablos44

@pablos44 I think you're complaining to the wrong guy.

You're right that the behavior of our forms about default values have changed between v3 and v4. In v3, react-final-form automatically sanitized empty inputs, and it created problems for some users. in v4, react-hook-form doesn't sanitize empty inputs, and it's also not the expected behavior for all. What I mean is that there is not one-size-fits-all solution for this problem.

We try to avoid adding custom logic on top of the frameworks we use, and we embrace their philosophy as much as possible. So if react-hook-form doesn't sanitize empty values by default, so does react-admin.

If you think this is a flaw, it's probably a flaw in the underlying form framework, and it should be fixed there, not here. So I invite you to open an issue on the react-hook-form repository about your problem.

Unfortunatelly because the way react-hook-form was integrated it became impossible in v4.

If there is a way to pass the form state (and not only the form data) to handleSubmit in react-hook-form, please tell me how. React-hook-form's handleSubmit API is pretty simple and doesn't allow that.

Also, you can use a form resolver to transform the data before it's submitted based on a context object that you manipulate.

We're doing our best to communicate about this gotcha (cf #8156), but I'm afraid it can't be fixed in react-admin.

fzaninotto avatar Sep 10 '22 06:09 fzaninotto