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

Fix required declarations #76

Merged
merged 4 commits into from
Nov 29, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
5 changes: 5 additions & 0 deletions .changeset/heavy-geckos-exist.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
"codemirror-json-schema": patch
---

fix required fields validation
2 changes: 1 addition & 1 deletion README.md
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,7 @@ Based on whether you want to support json4, json5 or both, you will need to inst

### Breaking Changes:

- 0.5.0 - this breaking change _does not_ effect users using `jsonSchema()` or `json5Schema()` modes, but those using the "custom path".
- 0.5.0 - this breaking change only impacts those following the "custom usage" approach, it _does not_ effect users using the high level, "bundled" `jsonSchema()` or `json5Schema()` modes. See the custom usages below to learn how to use the new `stateExtensions` and `handleRefresh` exports.

### json4

Expand Down
33 changes: 32 additions & 1 deletion src/__tests__/__fixtures__/schemas.ts
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@ export const testSchema2 = {
description: "an elegant string",
},
},
required: ["foo"],
additionalProperties: false,
},
oneOfEg: {
Expand Down Expand Up @@ -71,7 +72,7 @@ export const testSchema2 = {
type: "boolean",
},
},
required: ["foo", "object.foo"],
required: ["foo", "object"],
additionalProperties: false,
definitions: {
fancyObject: {
Expand All @@ -90,3 +91,33 @@ export const testSchema2 = {
},
},
} as JSONSchema7;

export const testSchema3 = {
$ref: "#/definitions/fancyObject",
definitions: {
fancyObject: {
type: "object",
properties: {
foo: { type: "string" },
bar: { type: "number" },
},
},
},
} as JSONSchema7;

export const testSchema4 = {
allOf: [
{
$ref: "#/definitions/fancyObject",
},
],
definitions: {
fancyObject: {
type: "object",
properties: {
foo: { type: "string" },
bar: { type: "number" },
},
},
},
} as JSONSchema7;
22 changes: 18 additions & 4 deletions src/__tests__/json-validation.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@ const getErrors = (jsonString: string, schema?: JSONSchema7) => {
};
const expectErrors = (
jsonString: string,
errors: [from: number, to: number, message: string][],
errors: [from: number | undefined, to: number | undefined, message: string][],
schema?: JSONSchema7
) => {
expect(getErrors(jsonString, schema)).toEqual(
Expand All @@ -42,7 +42,9 @@ describe("json-validation", () => {
]);
});
it("should not handle invalid json", () => {
expectErrors('{"foo": "example" "bar": 123}', []);
expectErrors('{"foo": "example" "bar": 123}', [
[undefined, undefined, "Expected `object` but received `null`"],
]);
});
it("should provide range for invalid multline json", () => {
expectErrors(
Expand All @@ -53,23 +55,35 @@ describe("json-validation", () => {
[[32, 37, "Additional property `bar` in `#` is not allowed"]]
);
});
it("should provide formatted error message when required fields are missing", () => {
expectErrors(
`{
"foo": "example",
"object": {}
}`,
[[46, 48, "The required property `foo` is missing at `object`"]],
testSchema2
);
});
it("should provide formatted error message for oneOf fields with more than 2 items", () => {
expectErrors(
`{
"foo": "example",
"object": { "foo": "true" },
"oneOfEg": 123
}`,
[[43, 46, 'Expected one of `"string"`, `"array"`, or `"boolean"`']],
[[80, 83, 'Expected one of `"string"`, `"array"`, or `"boolean"`']],
testSchema2
);
});
it("should provide formatted error message for oneOf fields with less than 2 items", () => {
expectErrors(
`{
"foo": "example",
"object": { "foo": "true" },
"oneOfEg2": 123
}`,
[[44, 47, 'Expected one of `"string"` or `"array"`']],
[[81, 84, 'Expected one of `"string"` or `"array"`']],
testSchema2
);
});
Expand Down
17 changes: 13 additions & 4 deletions src/json-validation.ts
Original file line number Diff line number Diff line change
Expand Up @@ -106,24 +106,33 @@ export class JSONValidation {
try {
errors = this.schema.validate(json.data);
} catch {}

if (!errors.length) return [];
// reduce() because we want to filter out errors that don't have a pointer
return errors.reduce((acc, error) => {
const errorPath = getErrorPath(error);
const pointer = json.pointers.get(errorPath) as JSONPointerData;
if (pointer) {
// if the error is a property error, use the key position
const isPropertyError = error.name === "NoAdditionalPropertiesError";
const isKeyError =
error.name === "NoAdditionalPropertiesError" ||
error.name === "RequiredPropertyError";
acc.push({
from: isPropertyError ? pointer.keyFrom : pointer.valueFrom,
to: isPropertyError ? pointer.keyTo : pointer.valueTo,
from: isKeyError ? pointer.keyFrom : pointer.valueFrom,
to: isKeyError ? pointer.keyTo : pointer.valueTo,
// TODO: create a domnode and replace `` with <code></code>
// renderMessage: () => error.message,
message: this.rewriteError(error),
severity: "error",
source: this.schemaTitle,
});
} else {
acc.push({
from: 0,
to: 0,
message: this.rewriteError(error),
severity: "error",
source: this.schemaTitle,
});
}
return acc;
}, [] as Diagnostic[]);
Expand Down
21 changes: 20 additions & 1 deletion src/utils/__tests__/jsonPointers.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -63,7 +63,7 @@ describe("jsonPointerForPosition for json5", () => {
describe("getJsonPointers", () => {
it("should return a map of all pointers for a document", () => {
const state = EditorState.create({
doc: '{"object": { "foo": true }, "bar": 123}',
doc: '{"object": { "foo": true }, "bar": 123, "baz": [1,2,3], "boop": [{"foo": true}]}',
extensions: [json()],
});
const pointers = getJsonPointers(state);
Expand All @@ -79,6 +79,25 @@ describe("getJsonPointers", () => {
valueFrom: 35,
valueTo: 38,
});
expect(pointers.get("/baz")).toEqual({
keyFrom: 40,
keyTo: 45,
valueFrom: 47,
valueTo: 54,
});
expect(pointers.get("/boop/0")).toEqual({
keyFrom: 65,
keyTo: 78,
valueFrom: 78,
valueTo: 79,
});
// TODO: return pointers for all array indexes, not just objects
// expect(pointers.get("/baz/0")).toEqual({
// keyFrom: 40,
// keyTo: 45,
// valueFrom: 47,
// valueTo: 55,
// });
});
});

Expand Down
2 changes: 2 additions & 0 deletions src/utils/__tests__/parseJSONDocument.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@ describe("parseJSONDocument", () => {
const doc = parseJSONDocument(`{"object": { "foo": true }, "bar": 123}`);
expect(doc.data).toEqual({ object: { foo: true }, bar: 123 });
expect(Array.from(doc.pointers.keys())).toEqual([
"",
"/object",
"/object/foo",
"/bar",
Expand All @@ -20,6 +21,7 @@ describe("parseJSON5Document", () => {
const doc = parseJSON5Document(`{'obj"ect': { foo: true }, "bar": 123}`);
expect(doc.data).toEqual({ ['obj"ect']: { foo: true }, bar: 123 });
expect(Array.from(doc.pointers.keys())).toEqual([
"",
'/obj"ect',
'/obj"ect/foo',
"/bar",
Expand Down
3 changes: 2 additions & 1 deletion src/utils/jsonPointers.ts
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,7 @@ export function getJsonPointerAt(docText: Text, node: SyntaxNode): string {
}
}
path.unshift("");

return path.join("/");
}

Expand Down Expand Up @@ -61,7 +62,7 @@ export const getJsonPointers = (
const pointers: JSONPointersMap = new Map();
json.iterate({
enter: (type: SyntaxNodeRef) => {
if (type.name === "PropertyName") {
if (type.name === "PropertyName" || type.name === "Object") {
const pointer = getJsonPointerAt(state.doc, type.node);

const { from: keyFrom, to: keyTo } = type.node;
Expand Down