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

Add server side validation support #7938

Merged
merged 9 commits into from
Feb 24, 2023
Merged

Add server side validation support #7938

merged 9 commits into from
Feb 24, 2023

Conversation

djhi
Copy link
Contributor

@djhi djhi commented Jul 7, 2022

Problem

  • Server side validation is cumbersome to set up
  • When using middlewares and as the SaveButton relied on the SaveContext.saving prop, the SaveButton was only disabled for while the main mutation was loading. However, additional work may still be running.

Solution

For pessismistic mode only, we now transparently support server side validation. When a server validation occurs, the DataProvider should throw an error that contains an errors object matching the form shape.

Besides, we now await the mutation in useCreateController and useEditController. In pessimistic mode, that means the SaveButton will stay disabled until the mutation is resolved including its middlewares if any.

That does not change anything for the other mutation modes.

## Problem

- Server side validation is cumbersome to set up
- When using middlewares and as the SaveButton relied on the SaveContext.saving prop, the SaveButton was only disabled for while the main mutation was loading. However, additional work may still be running.

## Solution

For `pessismistic` mode only, we now transparently support server side validation. When a server validation occurs, the DataProvider should throw a ServerValidationError that contains an `errors` object matching the form shape.

Besides, we now await the mutation in `useCreateController` and `useEditController`. In `pessimistic` mode, that means the `SaveButton` will stay disabled until the mutation is resolved including its middlewares if any.

That does not change anything for the other mutation modes.
docs/Validation.md Outdated Show resolved Hide resolved
examples/simple/src/dataProvider.tsx Outdated Show resolved Hide resolved
```
{% endraw %}
{
body: {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why the "body" key? Why can't the dataProvider only reject with ({ errors: { source: 'error message' } }?

Also, your snippet is both a type description and an example, and so it's not enough informative. I'd use a more complete example:

{ 
  errors: {
    title: 'An article with this title already exists. The title must be unique.',
    date: 'The date is required',
    tags: { message: "The tag 'agrriculture' doesn't exist" },
  }
}

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Because that's the current shape of our HttpError. Our data providers put the response json inside the body property of the error they throw


**Tip**: The returned validation errors might have any validation format we support (simple strings or object with message and args) for each key.

**Tip**: If your data provider leverages our [`httpClient`](https://marmelab.com/react-admin/DataProviderWriting.html#example-rest-implementation), this will be handled automatically when your server returns an invalid response with a json body containing the `errors` key.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this isn't super clear

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I tried to make this section clearer. Please let me know if that's enough.


**Tip**: If your data provider leverages our [`httpClient`](https://marmelab.com/react-admin/DataProviderWriting.html#example-rest-implementation), this will be handled automatically when your server returns an invalid response with a json body containing the `errors` key.

Here's an example of a dataProvider not using our `httpClient`:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

again, this is confusing. You don't give an eample of the general case but you give one of a particular case.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same

);
} catch (error) {
if ((error as HttpError).body?.errors != null) {
return (error as HttpError).body.errors;
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

otherwise, rethrow

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nope, this is the only type of errors we want to handle here. Otherwise, errors will be handled through the onError side effect function

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree with you @fzaninotto . This might even be the origin of the failing E2E test. I'll have a look.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay so after verification, @djhi is correct, both the onError block and the catch block will be executed. So we don't need to rethrow the error.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Than please add a comment explaining why we don't rethrow and that this is not a mistake

);
} catch (error) {
if ((error as HttpError).body?.errors != null) {
return (error as HttpError).body.errors;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is weird: the save promise resolves when there is a validation error in the save process? I'd expect it to reject.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is how the useAugmentedForm has been implemented. If the submit function returns something, we consider this is an error object. This was done like this probably because it worked that way in final-form.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can't we improve that in a backward compatible way?

@@ -431,7 +431,7 @@ export type UseUpdateOptions<
RecordType,
MutationError,
Partial<UseUpdateMutateParams<RecordType>>
> & { mutationMode?: MutationMode };
> & { mutationMode?: MutationMode; returnPromise?: boolean };
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

please test and document that option

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is used in the useEditController in pessimistic mode, so it is tested. I'm not sure we really need to document this option as it's pretty advanced, and the option was not documented in the updateOptions type in the first place.

@@ -431,7 +431,7 @@ export type UseUpdateOptions<
RecordType,
MutationError,
Partial<UseUpdateMutateParams<RecordType>>
> & { mutationMode?: MutationMode };
> & { mutationMode?: MutationMode; returnPromise?: boolean };
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is used in the useEditController in pessimistic mode, so it is tested. I'm not sure we really need to document this option as it's pretty advanced, and the option was not documented in the updateOptions type in the first place.

docs/Validation.md Outdated Show resolved Hide resolved
Copy link
Contributor Author

@djhi djhi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💯

@@ -362,52 +362,88 @@ const CustomerCreate = () => (

## Server-Side Validation

You can use the errors returned by the dataProvider mutation as a source for the validation. In order to display the validation errors, a custom `save` function needs to be used:
Server-side validation is supported out of the box. It requires that the dataProvider throws an error with the following shape:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does that work even in optimistic mode?

@slax57 slax57 added this to the 4.9.0 milestone Feb 24, 2023
@slax57 slax57 merged commit c49228b into next Feb 24, 2023
@slax57 slax57 deleted the refactor-saving-status branch February 24, 2023 14:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
RFR Ready For Review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants