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

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

Closed
3 tasks done
RoboPhred opened this issue Feb 19, 2020 · 6 comments
Closed
3 tasks done

Comments

@RoboPhred
Copy link

RoboPhred commented Feb 19, 2020

Prerequisites

Description

When a custom validator is passed through the validate property, all properties except the stack property on onError errors are wiped.

Steps to Reproduce

See https://codesandbox.io/s/react-example-22s01
Click submit to see proper errors. Click "Enable custom validation" to add a no-op custom validator, and submit again. Observe errors now are missing most of their properties.

Expected behavior

Existing AJV errors should mantain their properties. Custom validator errors should at the very least specify the property property so consumers of onError know what field is having the error.

Actual behavior

All errors, both custom and ajv, only have a stack property.

Error with custom validation off:
(all properties specified as expected)

[
  {
    "name": "maxLength",
    "property": ".test",
    "message": "should NOT be longer than 3 characters",
    "params": {
      "limit": 3
    },
    "stack": ".test should NOT be longer than 3 characters",
    "schemaPath": "#/properties/test/maxLength"
  }
]

Error with custom validation on:
(No properties aside from stack specified for both schema and custom errors)

[
  {
    "stack": "test: should NOT be longer than 3 characters"
  },
  {
    "stack": "test: This is a custom error"
  }
]

Version

1.8.1

Cause:

The error properties are generated by transformAjvErrors here. If custom validation is off, this is the error object returned here.

However, if a custom validator is used, the error list is entirely regenerated by toErrorList here. This function only populates the stack property.

Fix

Fixing this might be difficult if we want to completely regenerate the list to allow the validate handler to delete existing errors.
However, at the very least we should populate the property key in toErrorList so that the consumer knows where the errors occurred. This would be enough to fix our particular use case.

Edit: validate does not receive the existing errors, but a new schema object. This means the user cannot delete existing errors and we can simply compute the toErrorList of the validator errors and concat it.

We can accomplish this by replacing the code here

  const errorHandler = customValidate(formData, createErrorHandler(formData));
  const userErrorSchema = unwrapErrorHandler(errorHandler);
  const newErrorSchema = mergeObjects(errorSchema, userErrorSchema, true);
  // XXX: The errors list produced is not fully compliant with the format
  // exposed by the jsonschema lib, which contains full field paths and other
  // properties.
  const newErrors = toErrorList(newErrorSchema);

with

  const errorHandler = customValidate(formData, createErrorHandler(formData));
  const userErrorSchema = unwrapErrorHandler(errorHandler);
  const newErrorSchema = mergeObjects(errorSchema, userErrorSchema, true);
  // Concat the new errors to the existing error list
  const newErrors = [].concat(errors, toErrorList(userErrorSchema));
@epicfaace
Copy link
Member

Yeah, your analysis / fix seem to be right. Can you make a PR that fixes this?

@RoboPhred
Copy link
Author

PR: #1608

@RoboPhred
Copy link
Author

I rebooted this issue with a MR against 2.x: #2002

@stale
Copy link

stale bot commented Jul 22, 2022

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Please leave a comment if this is still an issue for you. Thank you.

@stale stale bot added the wontfix label Jul 22, 2022
@penx
Copy link
Collaborator

penx commented Jul 24, 2022

Not stale. Open PR at #2002

@stale stale bot removed the wontfix label Jul 24, 2022
heath-freenome added a commit to heath-freenome/react-jsonschema-form that referenced this issue Aug 26, 2022
…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
heath-freenome added a commit to heath-freenome/react-jsonschema-form that referenced this issue Aug 26, 2022
…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
heath-freenome added a commit to heath-freenome/react-jsonschema-form that referenced this issue Aug 26, 2022
…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
heath-freenome added a commit that referenced this issue Aug 26, 2022
* 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
heath-freenome added a commit to heath-freenome/react-jsonschema-form that referenced this issue Aug 27, 2022
…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
Copy link
Member

heath-freenome commented Aug 28, 2022

Fixed in v5 beta via #3047, see the 5.x migration guide

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

No branches or pull requests

4 participants