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

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 all 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: 9 additions & 8 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,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 = []) {
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.

let errorList = [];
if ("__errors" in errorSchema) {
errorList = errorList.concat(
errorSchema.__errors.map(stack => {
const property = "." + fieldPath.join(".");
return {
stack: `${fieldName}: ${stack}`,
property,
stack: `${property} ${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 +253,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
// Maintain the original error list to keep its AJV-provided detail properties.
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.");
});

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: ". err1" },
{ property: ".", stack: ". err2" },
{ property: ".a.b", stack: ".a.b err3" },
{ property: ".a.b", stack: ".a.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: ". Invalid" },
]);
});

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

sinon.assert.calledWithMatch(onChange.lastCall, {
errorSchema: { __errors: ["Invalid"] },
errors: [{ stack: "root: Invalid" }],
errors: [{ property: ".", stack: ". 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: ".0.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: ". Forbidden value: bbb" },
]);
});
});
Expand Down