Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

RA v4 blocks validation due to empty array #7500

Closed
andrico1234 opened this issue Apr 7, 2022 · 9 comments · Fixed by #7526
Closed

RA v4 blocks validation due to empty array #7500

andrico1234 opened this issue Apr 7, 2022 · 9 comments · Fixed by #7526
Labels

Comments

@andrico1234
Copy link
Contributor

What you were expecting:
Before RA v4, returning an empty array from the validator meant that RA treated the form as being valid. After the new update it seems that this is no longer the case, and we get a popup saying that form isn't valid.

Not sure if this is a bug, or if it's new behaviour that needs to be documented.

You can test this out here

You can enter some text into the array field.

If the value is less than 2 characters, it will add an array to the backlinks' error array. if it's larger, then it'll just return the empty array.

What do you suggest, is this a bug or just a change in behaviour?

What happened instead:

Steps to reproduce:

Related code:

insert short code snippets here

Other information:

Environment

  • React-admin version:
  • Last version that did not exhibit the issue (if applicable):
  • React version:
  • Browser:
  • Stack trace (in case of a JS error):
@WiXSL WiXSL added the bug label Apr 7, 2022
@WiXSL
Copy link
Contributor

WiXSL commented Apr 7, 2022

Reproduced. Thanks!
We will look into it.

@andrico1234
Copy link
Contributor Author

Just discovered that this also happens for empty objects too, not just arrays. This wasn't a problem in RA v3

@fzaninotto
Copy link
Member

The return type of the validator must be a flat object. When using array inputs, you must add an identifier matching the field name, e.g.

{
   title: 'title too short',
   'backlinks.0.url': 'url too short',
}

instead of

{
   title: 'title too short',
   backlinks: [{ url: 'url too short' }],
}

So your validator should be written as follows on v4:

const validator = record => {
    const errors = {};
    record.backlinks?.forEach((val, i) => {
        if (val.url.length < 2) {
            errors[`backlinks.${i}.url`] = 'The backlink is not long enough';
        }
    });

    return errors;
};

I'll check further to see if this is indeed a regression, in which case we should mention in in the Upgrade guide.

@fzaninotto
Copy link
Member

I see no mention of the object syntax for the error object in the react-admin v3 documentation. Where did you see it documented?

@WiXSL
Copy link
Contributor

WiXSL commented Apr 8, 2022

@fzaninotto, I don't think @andrico1234 read it anywhere, he is just implying that react-final-form allowed this "undocumented" behavior, and now react-admin v4 doesn't.

@fzaninotto
Copy link
Member

Right: it's in the react-final-form documentation:

Validation errors must be in the same shape as the values of the form.

So I see two options:

  1. We add a mention in the Upgrade guide
  2. When transforming the validate output in getSimpleValidationResolver, if there are nested objects, we flatten them (e.g. using solutions suggested in https://stackoverflow.com/questions/61889456/flatten-an-object-using-lodash). We should also log a warning explaining that it's not the right way to do stuff in v4

WDYT?

In any case, the Validation documentation should mention the fact that the return object should be flat, and give the example of an embedded array input.

@WiXSL
Copy link
Contributor

WiXSL commented Apr 8, 2022

Looks like option number 2 is better.

@fzaninotto
Copy link
Member

yep, I agree. I'll try it in the next 5 minutes.

@fzaninotto fzaninotto self-assigned this Apr 8, 2022
@fzaninotto
Copy link
Member

OK, I didn't manage to do it in 5 minutes, so I'll let it open for anyone willing to try. The solution probably comes from lodash/lodash#2240.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants