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.89k stars 5.23k forks source link

Add option to ignore server validation errors for non-registered fields #8448

Closed ghost91- closed 1 year ago

ghost91- commented 1 year ago

Is your feature request related to a problem? Please describe.

We have a server that performs validation based on OpenAPI, and in our schema, we have a discriminated union, implemented via oneOf. When validation for this fails, the OpenAPI validation unfortunately does not know which of the possible union values was intended, so it returns errors for all of them, explaining why they didn't match.

In our react-admin frontend, we transform these errors, so that they match the form inputs, in order to display the validation errors in the form (as described in https://marmelab.com/react-admin/Validation.html#server-side-validation). We do this by using a custom function that wraps create/update, which we then pass as the onSubmit callback to SimpleForm. However, at this point, we don't know which fields are actually registered in the form. In particular, only the fields for one of the unions possible values are registered, the others are not. But since we get errors for all of them, the errors for non-registered fields are set, but cannot be displayed or cleared. So when the user corrects the (visible) errors and tries to resubmit the form, they get an error, saying that the form is invalid, because it still contains the (invisible) errors for the fields that have not actually been registered.

Describe the solution you'd like

In order to solve this issue, it would be nice to have a way to ignore server validation errors for fields that are not registered. In order to keep things backwards compatible, this could be done via a new boolean prop that is passed into SimpleForm, Form, etc.

Describe alternatives you've considered

At the moment, we don't validate that part with OpenAPI, but instead validate it in our backend code, which allows us to provide more specific validation errors, so we don't have errors for non-registered fields. But it would be great to be able to cover this via OpenAPI instead.

slax57 commented 1 year ago

Hi,

If I understand your issue correctly, it seems to me that the problem rather comes from the API returning errors regarding fields that are not relevant, rather than react-admin not ignoring errors regarding fields that are not registered. So I am not really in favor of supporting this in react-admin. However, as for all enhancement requests, I'll keep it open to see if there is demand for this feature, and if somebody is willing to work on its implementation.

One last thing, have you tried setting shouldUnregister to true (see this section of the doc)? I'm not 100% sure but this could prevent react-hook-form from considering unregistered fields as erroneous (see RHF doc).

ghost91- commented 1 year ago

Unfortunately, it doesn't seem possible to convince OpenAPI validation to just return errors for the fields that exist. Let me give you an example to make it more clear, why that is not possible. I'll show the types in TypeScript syntax, but they work basically the same in OpenAPI. Consider

type Kind = 'Kind1' | 'Kind2';

type Option1 = {
  kind: 'Kind1';
  value1: number;
};

type Option2 = {
  kind: 'Kind2';
  value2: string;
}

type Option = Option1 | Option2;

Now let's say, somebody tries to pass

{
  "kind": "Kind1"
}

as an Option. Obviously, it doesn't match, but it's not 100 % clear why. OpenAPI validation just sees that the object doesn't match any of the possible values, but it doesn't prefer any of them (for example, because there is a partial match). Typescript actually does this:

const option: Option = { kind: 'Kind2' }; // Type '{ kind: "Kind2"; }' is not assignable to type 'Option'. Property 'value2' is missing in type '{ kind: "Kind2"; }' but required in type 'Option2'.

But unfortunately, OpenAPI validation doesn't do that. It considers all possible (non-matching) options equal, so it returns errors for all of them.

So yeah, I totally see where you are coming from, it's not really an issue with react-admin, but it's about how the errors are returned. It's just that (from my understanding), it would probably be a lot easier to solve on the react-admin side.

Also, in my opinion, conceptually, it doesn't really make sense to allow errors for non-registered fields at all, since they cannot be displayed, and thus the user can't really do anything about them. To be honest, I don't really understand why react-hook-form even allows this, but it is even mentioned explicitly at https://www.react-hook-form.com/api/useform/seterror/:

An error that is not associated with an input field will be persisted until cleared with clearErrors.

setError('notRegisteredInput', { type: 'custom', message: 'custom message' });
// clearErrors() need to invoked manually to remove that custom error 

The only reason why I asked for an option to ignore validation errors for non-registered fields, is for backwards compatibility. From my perspective, having this as the default behavior would actually be preferrable, because I just don't see any use case for having validation errors for non-registered fields.

Regarding shouldUnregister: We are already using that for the related fields, in order remove properties from the form when fields are removed, due to some field being changed (slightly related to https://discord.com/channels/830711057643208724/1042885749579726878/1042885749579726878). It does not help with this problem.

slax57 commented 1 year ago

Alright, thanks for the clarifications. I'm still not really in favor of supporting this in react-admin because, as you pointed out, it should rather be RHF's responsibility to do it. But let's keep this enhancement request open for now, in case somebody wants to implement it.

fzaninotto commented 1 year ago

I understand the use case, but I don't think we should add more logic in react-admin to support that. Our custom useForm hook already does too much IMHO, trying to support too many use cases at once.

<SimpleForm> doesn't allow this use case, and it won't. So your best option is to write your own form component using react-hook-form primitives.

newfylox commented 2 weeks ago

@ghost91- I know it's been years ago but I'm struggling with the same situation

https://github.com/marmelab/react-admin/issues/7969

Have you found a solution for this?