React-admin: sanitizeEmptyValues will insert null value in object when removing an array item

[sebitoelcheater]

What you were expecting:
Let's say we have a resource /quotes and an array attribute providers of a quote.

quote = {
    providers: [
        {id: 1, name: "John", score: 10},
        {id: 2, name: "Steve"},
        {id: 3, name: "Mike"}
    ]
}

In the edit component of quotes, if I remove the first provider "John", the elements in providers sent to the server should be the last two providers.

quote = {
    providers: [
        {id: 2, name: "Steve"},
        {id: 3, name: "Mike"}
    ]
}

What happened instead:
The first element of providers includes the score key with null value.

quote = {
    providers: [
        {id: 2, name: "Steve", score: null},
        {id: 3, name: "Mike"}
    ]
}

Steps to reproduce:
In a component inside the Edit form, change the value of the form:

const form = useForm();
form.change(
    'providers',
    [
        {id: 2, name: "Steve"},
        {id: 3, name: "Mike"}
    ]
)

Other information:
It seems like the responsible for this issue is the function sanitizeEmptyValues as it will include a null value if a key was present initially in the first element of the array an not in the new value of that first array element.
This condition:

acc[key] = typeof values[key] === 'undefined' ? null : values[key];

Seems to be responsible of it. Shouldn't the values[key] have reference equality with initialValues[key] in order to the undefined element be replaced by null?

Environment

• React-admin version: 3.5.5
• Last version that did not exhibit the issue (if applicable): 2.x.x
• React version: 16.13.1
• Browser: Chrome
• Stack trace (in case of a JS error): -

[fzaninotto]

Thanks for the report.

I reproduced the bug with this unit test:

    it('should not touch existing objects when removing removing objects in array of objects', () => {
        expect(
            sanitizeEmptyValues(
                { obj: [{ foo: 1, foo2: 2 }, { foo: 3 }, { foo: 4 }] },
                { obj: [{ foo: 3 }, { foo: 4 }] }
            )
        ).toEqual({ obj: [{ foo: 3 }, { foo: 4 }] });
    });

- Expected
+ Received

@@ -1,9 +1,10 @@
    Object {
    "obj": Array [
        Object {
        "foo": 3,
+       "foo2": null,
        },
        Object {
        "foo": 4,
        },
    ],

[burnes]

Having a similar issue also when inserting an item. It should be possible to disable sanitizing empty values.

[burnes]

Yesterday I was further investigating in this issue. Seems like the sanitizeEmptyValues output is a little bit dependent on the data provider / database sub system. In our case (using GraphQL) it only makes sense to sanitize empty values for scalar values. Sanitizing objects or object-arrays adds additional properties which may not be allowed on server side.

For me these tests explain the problem:

  it('flat values will be sanitized if they are empty', () => {
    const initial = { a: 1 }
    const changed = { b: 2 }
    const res = sanitizeEmptyValues(initial, changed)
    expect(res).toStrictEqual({ a: null, b: 2 })
  })

  it('objects will not be sanitizing empty values', () => {
    const initial = { a: { x: 1, y: 2 } }
    const changed = { a: { y: 3, z: 4 } }
    const res = sanitizeEmptyValues(initial, changed)
    // original implementation would return { a: { x: null, y: 3, z: 4 }] }
    expect(res).toStrictEqual(changed)
  })

  it('arrays will not be sanitized on empty values', () => {
    const initial = { a: [{ x: 1 }] }
    const changed = { a: [{ y: 2 }, { x: 1 }] }
    const res = sanitizeEmptyValues(initial, changed)
    // original implementation would return { a: [{ x: null, y: 2 }, { x: 1 }] }
    expect(res).toStrictEqual(changed)
  })

Seems like the requirements differ from project to project, so I think this should be configurable in some way. Any ideas on how to make this configurable @fzaninotto? If we agree on a clear path I can implement that and make a pull request.

[fzaninotto]

The empty value sanitization is currently done at the Form level, and it's not dependent on the dataProvider. I don't see how this can be done in a configurable way without a huge (and not backward compatible) change.

But we can fall back to a basic sanitization in the form (i.e. no object / array sanitization), and reimplement the object/array refill in simpleRestDataProvider (since the update method also receives the previousValue).

[macrozone]

wow! just ran in the same issue, but at a completly unexpected spot:

we use https://github.com/react-page/react-page for content. This is just a field with a js object as its value.

sanitizeEmptyValues will mess with this object and will add nulls at places where you would not expect (when you had some property before there that you want to remove)

sanitizeEmptyValues should treat these objects as opaque and not try to sanitize them at all.

Edit: in particular, _sanitizeEmptyValues should not touch keys that do not refer to form fields at all._ E.g. if a form field has an object as value

current workaround is to downgrade to react-admin 3.5.3 and make sure all deps (ra-core, etc.) are also forced to be 3.5.3. The array sanitization was introduced in react admin 3.5.5

[fzaninotto]

Fixed by #5077