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.

Sign up for GitHub

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

Jump to bottom

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

Closed
wants to merge 5 commits into from
Closed

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

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
82e1876
fix(#1596): Produce property paths for custom errors.
RoboPhred Aug 19, 2020
3802798
fix(#1596): Test that AJV errors merge with custom errors
RoboPhred Aug 19, 2020
54a4993
Fix spelling in packages/core/src/validate.js
RoboPhred Aug 31, 2020
ae336e4
feat(#1596): Use AJV style error messages
RoboPhred Sep 3, 2020
52615b5
Merge branch '2.x-issue-1596' of github.com:RoboPhred/react-jsonschem…
RoboPhred Sep 3, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
17 changes: 10 additions & 7 deletions packages/core/src/validate.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -80,21 +80,24 @@ 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 = []) {
Copy link
Member

Choose a reason for hiding this comment

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

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?

Copy link
Author

@RoboPhred RoboPhred Mar 11, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

Ah, thanks for the pointer.

const fieldName =
fieldPath.length > 0 ? fieldPath[fieldPath.length - 1] : "root";

let errorList = [];
if ("__errors" in errorSchema) {
errorList = errorList.concat(
errorSchema.__errors.map(stack => {
return {
property: "." + fieldPath.join("."),
stack: `${fieldName}: ${stack}`,
};
})
);
}
return Object.keys(errorSchema).reduce((acc, key) => {
if (key !== "__errors") {
acc = acc.concat(toErrorList(errorSchema[key], key));
acc = acc.concat(toErrorList(errorSchema[key], [...fieldPath, key]));
}
return acc;
}, errorList);
Expand Down Expand Up @@ -252,10 +255,10 @@ export default function validateFormData(
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);

// Append the user's errors to the current error list
// Mantain the original error list to keep its AJV provided detail properties.
RoboPhred marked this conversation as resolved.
Show resolved Hide resolved
epicfaace marked this conversation as resolved.
Show resolved Hide resolved
const newErrors = [].concat(errors, toErrorList(userErrorSchema));

return {
errors: newErrors,
Expand Down
42 changes: 28 additions & 14 deletions packages/core/test/validate_test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -229,6 +229,7 @@ describe("Validation", () => {
properties: {
pass1: { type: "string" },
pass2: { type: "string" },
numberWithMinimum: { type: "number", minimum: 5 },
},
};

Expand All @@ -239,20 +240,23 @@ describe("Validation", () => {
}
return errors;
};
const formData = { pass1: "a", pass2: "b" };
const formData = { pass1: "a", pass2: "b", numberWithMinimum: 2 };
const result = validateFormData(formData, schema, validate);
errors = result.errors;
errorSchema = result.errorSchema;
});

it("should return an error list", () => {
expect(errors).to.have.length.of(1);
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.");
Copy link
Author

@RoboPhred RoboPhred Aug 19, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

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.

Copy link
Member

@epicfaace epicfaace Aug 30, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

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.

Copy link
Author

Choose a reason for hiding this comment

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

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.

});

it("should return an errorSchema", () => {
expect(errorSchema.pass2.__errors).to.have.length.of(1);
expect(errorSchema.pass2.__errors[0]).eql("passwords don't match.");
expect(errorSchema.numberWithMinimum.__errors).to.have.length.of(1);
expect(errorSchema.numberWithMinimum.__errors[0]).eql("should be >= 5");
});
});

Expand Down Expand Up @@ -335,11 +339,11 @@ describe("Validation", () => {
},
})
).eql([
{ stack: "root: err1" },
{ stack: "root: err2" },
{ stack: "b: err3" },
{ stack: "b: err4" },
{ stack: "c: err5" },
{ property: ".", stack: "root: err1" },
{ property: ".", stack: "root: err2" },
{ property: ".a.b", stack: "b: err3" },
{ property: ".a.b", stack: "b: err4" },
{ property: ".c", stack: "c: err5" },
]);
});
});
Expand Down Expand Up @@ -502,7 +506,7 @@ describe("Validation", () => {

submitForm(node);
sinon.assert.calledWithMatch(onError.lastCall, [
{ stack: "root: Invalid" },
{ property: ".", stack: "root: Invalid" },
]);
});

Expand All @@ -529,7 +533,7 @@ describe("Validation", () => {

sinon.assert.calledWithMatch(onChange.lastCall, {
errorSchema: { __errors: ["Invalid"] },
errors: [{ stack: "root: Invalid" }],
errors: [{ property: ".", stack: "root: Invalid" }],
formData: "1234",
});
});
Expand Down Expand Up @@ -611,8 +615,18 @@ describe("Validation", () => {
});
submitForm(node);
sinon.assert.calledWithMatch(onError.lastCall, [
{ stack: "pass2: should NOT be shorter than 3 characters" },
{ stack: "pass2: Passwords don't match" },
{
message: "should NOT be shorter than 3 characters",
name: "minLength",
params: { limit: 3 },
property: ".pass2",
schemaPath: "#/properties/pass2/minLength",
stack: ".pass2 should NOT be shorter than 3 characters",
},
{
property: ".pass2",
stack: "pass2: Passwords don't match",
},
]);
});

Expand Down Expand Up @@ -650,7 +664,7 @@ describe("Validation", () => {

submitForm(node);
sinon.assert.calledWithMatch(onError.lastCall, [
{ stack: "pass2: Passwords don't match" },
{ property: ".0.pass2", stack: "pass2: Passwords don't match" },
]);
});

Expand Down Expand Up @@ -678,7 +692,7 @@ describe("Validation", () => {
});
submitForm(node);
sinon.assert.calledWithMatch(onError.lastCall, [
{ stack: "root: Forbidden value: bbb" },
{ property: ".", stack: "root: Forbidden value: bbb" },
]);
});
});
Expand Down