Maintain AJV properties when custom validator is in use. #2002

RoboPhred · 2020-08-19T21:54:07Z

Reasons for making this change

This fixes the issue where AJV errors are stripped of important properties when a custom validator is in use.

This fixes issue #1596

Checklist

I'm updating documentation
- I've checked the rendering of the Markdown text I've added
I'm adding or updating code
- I've added and/or updated tests
- I've updated docs if needed
I'm adding a new feature
- I've updated the playground with an example use of the feature

RoboPhred · 2020-08-19T21:56:01Z

packages/core/test/validate_test.js

-        expect(errors[0].stack).eql("pass2: passwords don't match.");
+        expect(errors).to.have.length.of(2);
+        expect(errors[0].stack).eql(".numberWithMinimum should be >= 5");
+        expect(errors[1].stack).eql("pass2: passwords don't match.");


Note how the stack message is subtly different here:
RJSF custom errors use fieldName: message
AJV errors use .field.path message

I am slightly worried that this might be considered a breaking change, as previously our mangling of AJV errors would have changed the stack message to the RJSF format, so anything expecting the stack to remain the same will suffer.

However, if someone did not make use of custom validation, then I think all errors would be using the AJV error form. This might indicate we should in fact change custom validation stacks to match the AJV format, to make it consistent across both usages.

I would like some input on how to approach this. My recommendation is changing RJSF to use AJV style messages in all cases, to make custom validation use the same format as non-custom validation. But if we want to be strictly backwards compatible with these messages, we can re-mangle AJV errors only when custom validation is being used, and leave the user to extract the error path from the now-usable AJV error properties.

My recommendation is changing RJSF to use AJV style messages in all cases

I like this approach. I think rjsf is due for a new breaking change soon (there are several PRs that would require breaking changes but are probably needed), so I think we should be fine if we just change this PR to do this and then release in v3.x.

I switched custom errors to use AJV style error messages.

Note that errors on the root now show up with a single dot, as in ". This is a root error". This appears a bit strange, but is indeed the way AJV shows root errors.

packages/core/src/validate.js

epicfaace · 2020-08-30T22:28:33Z

packages/core/test/validate_test.js

-        expect(errors[0].stack).eql("pass2: passwords don't match.");
+        expect(errors).to.have.length.of(2);
+        expect(errors[0].stack).eql(".numberWithMinimum should be >= 5");
+        expect(errors[1].stack).eql("pass2: passwords don't match.");


My recommendation is changing RJSF to use AJV style messages in all cases

I like this approach. I think rjsf is due for a new breaking change soon (there are several PRs that would require breaking changes but are probably needed), so I think we should be fine if we just change this PR to do this and then release in v3.x.

Co-authored-by: Ashwin Ramaswami <[email protected]>

BREAKING CHANGE: This changes the message format of custom validation errors.

…a-form into 2.x-issue-1596

epicfaace · 2021-03-10T15:18:21Z

packages/core/src/validate.js

@@ -80,21 +80,22 @@ function toErrorSchema(errors) {
  }, {});
 }

-export function toErrorList(errorSchema, fieldName = "root") {
-  // XXX: We should transform fieldName as a full field path string.
+export function toErrorList(errorSchema, fieldPath = []) {


Sorry, I'm a bit confused as to what the changes in toErrorList have to do with fixing #1596. Doesn't the change in line 259 already ensure that the original error list is untouched?

This change is talked about in the above comment

I made this change on approval from your reply

toErrorList in its original form only tracked the current field's name.
For example, given an object:

{ foo: { bar: null }, baz: { bar: "hello world" } }

where foo.bar must be a string, the old version would generate an error:
bar: value must be a string.

However, this is not clear on what field received the error (there are two 'bar' properties, and it does not specify which one). It is also inconsistent with AJV's own error format.

This change generates messages in the same format as AJV, by giving a dot-separated path to the field. It's error will be:
.foo.bar value must be a string

Ah, thanks for the pointer.

packages/core/src/validate.js

epicfaace

@RoboPhred , since this is a breaking change, can you add an entry to the 3.x upgrade guide that explains to users what has changed and what they may need to change in their own code?

The guide isn't yet on the master branch (it's in #2211), but here's the version of the guide that's in that branch: https://github.com/rjsf-team/react-jsonschema-form/blob/26d34051bbc063777645aa825ed4192e260930ab/docs/3.x%20upgrade%20guide.md You can just create the docs/3.x upgrade guide.md file on this branch.

saadq · 2021-07-13T22:56:24Z

Hi all, wanted to check on the status of this PR. What's still needed for this change to get merged in? Is it just updating the upgrade guide?

penx · 2021-09-05T11:18:26Z

@saadq looks like it missed v3 so it's on the roadmap for v4: #2437

AlimovSV · 2022-03-07T08:00:47Z

Any progress?

heath-freenome · 2022-08-19T15:08:18Z

@RoboPhred With the upcoming beta 5 that already has a bunch of breaking changes, are you willing to port this to the rjsf-v5 branch into the validator-ajv6 package?

…f v5 - Added a new `mergeValidationData()` method in `@rjsf/utils` to handle the appending of errors onto the end of the validationData from an additional error schema - Added this to the `schema` directory `index.ts` along with exposing it on the `SchemaUtils` type and implementation - Also fixed the type of `toErrorList()` in `ValidatorType` to change from `fieldName: string` to `fieldPath: string[]` - Added reusable `mergeValidationDataTest.ts`, calling it in the utils - Update the `@rjsf/validator-ajv6` to pick up the breaking change from rjsf-team#2002 around `AJV6Validator.toErrorList()` - Also modified the `validateFormData()` function to return the result of `mergeValidationData()` when the user has a custom validator - Updated tests for the new structure of the `toErrorList()` data - Also updated the `schema.tests` to add the new `mergeValidationDataTest()` - Updated `Form` to use the `mergeValidationData()` function in the few places where `extraErrors` was being merged into the schema validation - Updated tests for the new structor of the `toErrorList()` data - Updated the `CHANGELOG.md` to describe this fix - Updated the `5.x upgrade guide.md` to describe all the new utility functions added - Updated the `validation.md` documentation for the `ErrorListTemplate` change along with making the `RJSFValidationError` interface describe the optional properties

…f v5 - Added a new `mergeValidationData()` method in `@rjsf/utils` to handle the appending of errors onto the end of the validationData from an additional error schema - Added this to the `schema` directory `index.ts` along with exposing it on the `SchemaUtils` type and implementation - Also fixed the type of `toErrorList()` in `ValidatorType` to change from `fieldName: string` to `fieldPath: string[]` - Added reusable `mergeValidationDataTest.ts`, calling it in the utils - Update the `@rjsf/validator-ajv6` to pick up the breaking change from rjsf-team#2002 around `AJV6Validator.toErrorList()` - Also modified the `validateFormData()` function to return the result of `mergeValidationData()` when the user has a custom validator - Updated tests for the new structure of the `toErrorList()` data - Also updated the `schema.tests` to add the new `mergeValidationDataTest()` - Updated `Form` to use the `mergeValidationData()` function in the few places where `extraErrors` was being merged into the schema validation - Updated tests for the new structor of the `toErrorList()` data - Updated the `CHANGELOG.md` to describe this fix - Updated the `5.x upgrade guide.md` to describe all the new utility functions added and describe util.js and validator.js breaking changes - Updated the `validation.md` documentation for the `ErrorListTemplate` change along with making the `RJSFValidationError` interface describe the optional properties

* Fixed #1596 by adapting the fix from #2002 into rjsf v5 - Added a new `mergeValidationData()` method in `@rjsf/utils` to handle the appending of errors onto the end of the validationData from an additional error schema - Added this to the `schema` directory `index.ts` along with exposing it on the `SchemaUtils` type and implementation - Also fixed the type of `toErrorList()` in `ValidatorType` to change from `fieldName: string` to `fieldPath: string[]` - Added reusable `mergeValidationDataTest.ts`, calling it in the utils - Update the `@rjsf/validator-ajv6` to pick up the breaking change from #2002 around `AJV6Validator.toErrorList()` - Also modified the `validateFormData()` function to return the result of `mergeValidationData()` when the user has a custom validator - Updated tests for the new structure of the `toErrorList()` data - Also updated the `schema.tests` to add the new `mergeValidationDataTest()` - Updated `Form` to use the `mergeValidationData()` function in the few places where `extraErrors` was being merged into the schema validation - Updated tests for the new structor of the `toErrorList()` data - Updated the `CHANGELOG.md` to describe this fix - Updated the `5.x upgrade guide.md` to describe all the new utility functions added and describe util.js and validator.js breaking changes - Updated the `validation.md` documentation for the `ErrorListTemplate` change along with making the `RJSFValidationError` interface describe the optional properties * - Responded to reviewer feedback... also, removed the `:` after the property in the `stack` to match AJV stack, adding `message` to also match AJV - Added migration guide changes

…f v5 (rjsf-team#3047) * Fixed rjsf-team#1596 by adapting the fix from rjsf-team#2002 into rjsf v5 - Added a new `mergeValidationData()` method in `@rjsf/utils` to handle the appending of errors onto the end of the validationData from an additional error schema - Added this to the `schema` directory `index.ts` along with exposing it on the `SchemaUtils` type and implementation - Also fixed the type of `toErrorList()` in `ValidatorType` to change from `fieldName: string` to `fieldPath: string[]` - Added reusable `mergeValidationDataTest.ts`, calling it in the utils - Update the `@rjsf/validator-ajv6` to pick up the breaking change from rjsf-team#2002 around `AJV6Validator.toErrorList()` - Also modified the `validateFormData()` function to return the result of `mergeValidationData()` when the user has a custom validator - Updated tests for the new structure of the `toErrorList()` data - Also updated the `schema.tests` to add the new `mergeValidationDataTest()` - Updated `Form` to use the `mergeValidationData()` function in the few places where `extraErrors` was being merged into the schema validation - Updated tests for the new structor of the `toErrorList()` data - Updated the `CHANGELOG.md` to describe this fix - Updated the `5.x upgrade guide.md` to describe all the new utility functions added and describe util.js and validator.js breaking changes - Updated the `validation.md` documentation for the `ErrorListTemplate` change along with making the `RJSFValidationError` interface describe the optional properties * - Responded to reviewer feedback... also, removed the `:` after the property in the `stack` to match AJV stack, adding `message` to also match AJV - Added migration guide changes

heath-freenome · 2022-08-28T16:57:28Z

Refactored into v5 beta via #3047

RoboPhred added 2 commits August 19, 2020 14:33

fix(#1596): Produce property paths for custom errors.

82e1876

fix(#1596): Test that AJV errors merge with custom errors

3802798

RoboPhred commented Aug 19, 2020

View reviewed changes

RoboPhred mentioned this pull request Aug 19, 2020

Errors in onError loose all ajv properties when a custom validator property is specified. #1596

Closed

3 tasks

epicfaace reviewed Aug 30, 2020

View reviewed changes

epicfaace added the breaking change label Aug 30, 2020

RoboPhred and others added 3 commits August 31, 2020 09:33

Fix spelling in packages/core/src/validate.js

54a4993

Co-authored-by: Ashwin Ramaswami <[email protected]>

feat(#1596): Use AJV style error messages

ae336e4

BREAKING CHANGE: This changes the message format of custom validation errors.

Merge branch '2.x-issue-1596' of github.com:RoboPhred/react-jsonschem…

52615b5

…a-form into 2.x-issue-1596

epicfaace reviewed Mar 10, 2021

View reviewed changes

epicfaace mentioned this pull request Mar 10, 2021

fix the issue which errors in onError loose all ajv properties when… #2242

Closed

7 tasks

epicfaace self-assigned this Mar 10, 2021

epicfaace mentioned this pull request Mar 10, 2021

Maintain json schema error properties when using custom validation #1608

Closed

7 tasks

epicfaace reviewed Mar 22, 2021

View reviewed changes

epicfaace mentioned this pull request Mar 22, 2021

ANNOUNCEMENT: Breaking changes planned for v3 #2297

Closed

5 tasks

epicfaace mentioned this pull request Jun 25, 2021

ANNOUNCEMENT: Breaking changes planned for v4 #2437

Closed

3 tasks

heath-freenome mentioned this pull request Aug 26, 2022

Fixed #1596 by adapting the fix from #2002 into rjsf v5 #3047

Merged

8 tasks

heath-freenome closed this Aug 28, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Maintain AJV properties when custom validator is in use. #2002

Maintain AJV properties when custom validator is in use. #2002

RoboPhred commented Aug 19, 2020 •

edited

Loading

RoboPhred Aug 19, 2020 •

edited

Loading

epicfaace Aug 30, 2020 •

edited

Loading

RoboPhred Sep 3, 2020

epicfaace Aug 30, 2020 •

edited

Loading

epicfaace Mar 10, 2021

RoboPhred Mar 11, 2021 •

edited

Loading

epicfaace Mar 11, 2021

epicfaace left a comment

saadq commented Jul 13, 2021

penx commented Sep 5, 2021

AlimovSV commented Mar 7, 2022

heath-freenome commented Aug 19, 2022

heath-freenome commented Aug 28, 2022

Maintain AJV properties when custom validator is in use. #2002

Maintain AJV properties when custom validator is in use. #2002

Conversation

RoboPhred commented Aug 19, 2020 • edited Loading

Reasons for making this change

Checklist

RoboPhred Aug 19, 2020 • edited Loading

Choose a reason for hiding this comment

epicfaace Aug 30, 2020 • edited Loading

Choose a reason for hiding this comment

RoboPhred Sep 3, 2020

Choose a reason for hiding this comment

epicfaace Aug 30, 2020 • edited Loading

Choose a reason for hiding this comment

epicfaace Mar 10, 2021

Choose a reason for hiding this comment

RoboPhred Mar 11, 2021 • edited Loading

Choose a reason for hiding this comment

epicfaace Mar 11, 2021

Choose a reason for hiding this comment

epicfaace left a comment

Choose a reason for hiding this comment

saadq commented Jul 13, 2021

penx commented Sep 5, 2021

AlimovSV commented Mar 7, 2022

heath-freenome commented Aug 19, 2022

heath-freenome commented Aug 28, 2022

RoboPhred commented Aug 19, 2020 •

edited

Loading

RoboPhred Aug 19, 2020 •

edited

Loading

epicfaace Aug 30, 2020 •

edited

Loading

epicfaace Aug 30, 2020 •

edited

Loading

RoboPhred Mar 11, 2021 •

edited

Loading