From 5cb6c1f153ee895805440066f2ab5a9b72f4adfb Mon Sep 17 00:00:00 2001 From: Fa!ko Date: Mon, 21 Sep 2026 14:24:46 +0200 Subject: [PATCH] fix: highlight invalid property values and array items accurately --- .changeset/tidy-diagnostic-ranges.md | 5 + .../__tests__/diagnostic-ranges.spec.ts | 130 ++++++++++++++++++ src/features/validation.ts | 12 +- src/types.ts | 4 +- src/utils/__tests__/json-pointers.spec.ts | 119 ++++++++++------ src/utils/json-pointers.ts | 48 ++++--- 6 files changed, 247 insertions(+), 71 deletions(-) create mode 100644 .changeset/tidy-diagnostic-ranges.md create mode 100644 src/features/__tests__/diagnostic-ranges.spec.ts diff --git a/.changeset/tidy-diagnostic-ranges.md b/.changeset/tidy-diagnostic-ranges.md new file mode 100644 index 0000000..8de4406 --- /dev/null +++ b/.changeset/tidy-diagnostic-ranges.md @@ -0,0 +1,5 @@ +--- +"codemirror-json-schema": patch +--- + +Fix diagnostic ranges for primitive and nested array items in JSON, JSON5, and YAML. Locate property values by their syntax nodes rather than fixed sibling offsets, so newer JSON grammars highlight the invalid value instead of its colon. Preserve property key ranges for object-valued properties and keep missing-required-property diagnostics on their containing values. diff --git a/src/features/__tests__/diagnostic-ranges.spec.ts b/src/features/__tests__/diagnostic-ranges.spec.ts new file mode 100644 index 0000000..87cc253 --- /dev/null +++ b/src/features/__tests__/diagnostic-ranges.spec.ts @@ -0,0 +1,130 @@ +import { EditorView } from "@codemirror/view"; +import type { JSONSchema7 } from "json-schema"; +import { describe, expect, it } from "vitest"; +import { MODES } from "../../constants"; +import { JSONValidation } from "../validation"; +import { getExtensions } from "./__helpers__"; + +const integerArray: JSONSchema7 = { + type: "array", + items: { type: "integer" }, +}; + +const cases: { + name: string; + doc: string; + schema: JSONSchema7; + highlighted: string; +}[] = [ + { + name: "a property value, not its colon", + doc: '{"count": "invalid"}', + schema: { type: "object", properties: { count: { type: "integer" } } }, + highlighted: '"invalid"', + }, + ...['"invalid"', "1.5", "true", "null", "{}", "[]"].map((value) => ({ + name: `an invalid array item (${value})`, + doc: `{"values": [0, ${value}, 2]}`, + schema: { type: "object", properties: { values: integerArray } }, + highlighted: value, + })), + { + name: "a primitive item in a root array", + doc: '[0, "invalid", 2]', + schema: integerArray, + highlighted: '"invalid"', + }, + { + name: "an item in a nested array", + doc: '{"groups": [[0], [1, "invalid"]]}', + schema: { + type: "object", + properties: { groups: { type: "array", items: integerArray } }, + }, + highlighted: '"invalid"', + }, + { + name: "an invalid object value without losing its property range", + doc: '{"value": {"child": 1}}', + schema: { type: "object", properties: { value: { type: "string" } } }, + highlighted: '{"child": 1}', + }, + { + name: "an additional property key with an object value", + doc: '{"extra": {"child": 1}}', + schema: { type: "object", additionalProperties: false }, + highlighted: '"extra"', + }, + { + name: "an array object missing a required property", + doc: '[{"present": 1}]', + schema: { + type: "array", + items: { type: "object", required: ["missing"] }, + }, + highlighted: '{"present": 1}', + }, +]; + +describe.each(Object.values(MODES))("diagnostic ranges (%s)", (mode) => { + const languageCases = + mode === MODES.YAML + ? ([ + { + name: "a block sequence item without its dash", + doc: "values:\n - 0\n - invalid\n - 2\n", + schema: { type: "object", properties: { values: integerArray } }, + highlighted: "invalid", + }, + { + name: "an item in a nested block sequence", + doc: "groups:\n - values:\n - 0\n - invalid\n", + schema: { + type: "object", + properties: { + groups: { + type: "array", + items: { + type: "object", + properties: { values: integerArray }, + }, + }, + }, + }, + highlighted: "invalid", + }, + ] satisfies typeof cases) + : mode === MODES.JSON5 + ? ([ + { + name: "a property value separated by comments", + doc: "{count /* key */: /* value */ 'invalid'}", + schema: { + type: "object", + properties: { count: { type: "integer" } }, + }, + highlighted: "'invalid'", + }, + ] satisfies typeof cases) + : []; + it.each([...cases, ...languageCases])( + "highlights $name", + ({ doc, schema, highlighted }) => { + const view = new EditorView({ + doc, + extensions: [getExtensions(mode, schema)], + }); + try { + const diagnostics = new JSONValidation({ mode }).doValidation(view); + expect(diagnostics).toHaveLength(1); + expect(diagnostics[0]).toMatchObject({ + from: doc.indexOf(highlighted), + to: doc.indexOf(highlighted) + highlighted.length, + severity: "error", + }); + } finally { + view.destroy(); + } + }, + ); +}); diff --git a/src/features/validation.ts b/src/features/validation.ts index 3468f1d..c8a2a94 100644 --- a/src/features/validation.ts +++ b/src/features/validation.ts @@ -53,7 +53,6 @@ export function jsonSchemaLinter(options?: JSONValidationOptions) { // all the error types that apply to a specific key or value const positionalErrors = [ "NoAdditionalPropertiesError", - "RequiredPropertyError", "InvalidPropertyNameError", "ForbiddenPropertyError", "UndefinedValueError", @@ -154,11 +153,16 @@ export class JSONValidation { ) { pushRoot(); } else if (pointer) { - // if the error is a property error, use the key position + // Highlight existing keys for property errors. Missing required keys + // have no range, so keep highlighting their containing value. const isKeyError = positionalErrors.includes(error.name); const errorString = this.rewriteError(error); - const from = isKeyError ? pointer.keyFrom : pointer.valueFrom; - const to = isKeyError ? pointer.keyTo : pointer.valueTo; + const from = isKeyError + ? (pointer.keyFrom ?? pointer.valueFrom) + : pointer.valueFrom; + const to = isKeyError + ? (pointer.keyTo ?? pointer.valueTo) + : pointer.valueTo; // skip error if no from/to value is found if (to !== undefined && from !== undefined) { acc.push({ diff --git a/src/types.ts b/src/types.ts index 26161e3..680a0b5 100644 --- a/src/types.ts +++ b/src/types.ts @@ -9,8 +9,8 @@ export type JSONPartialPointerData = { }; export type JSONPointerData = { - keyFrom: number; - keyTo: number; + keyFrom?: number; + keyTo?: number; valueFrom: number; valueTo: number; }; diff --git a/src/utils/__tests__/json-pointers.spec.ts b/src/utils/__tests__/json-pointers.spec.ts index c942adc..12cffa1 100644 --- a/src/utils/__tests__/json-pointers.spec.ts +++ b/src/utils/__tests__/json-pointers.spec.ts @@ -93,19 +93,59 @@ bar: 123 }); const pointer = jsonPointerForPosition(state, pos, 1, mode); expect(pointer).toEqual(expected); - } + }, ); }); describe("getJsonPointers", () => { + it.each(Object.values(MODES))( + "escapes property names and preserves ranges for nested arrays (%s)", + (mode) => { + const doc = '{"a/b~c": [[0, "invalid"]]}'; + const state = EditorState.create({ + doc, + extensions: [getExtensions(mode)], + }); + const pointers = getJsonPointers(state, mode); + expect(pointers.get("/a~1b~0c/0/1")).toEqual({ + valueFrom: doc.indexOf('"invalid"'), + valueTo: doc.indexOf('"invalid"') + '"invalid"'.length, + }); + expect(pointers.get("/a~1b~0c/0")).toEqual({ + valueFrom: doc.indexOf('[0, "invalid"]'), + valueTo: doc.indexOf('[0, "invalid"]') + '[0, "invalid"]'.length, + }); + }, + ); + + it.each([ + { mode: MODES.JSON, doc: '{"value":' }, + { mode: MODES.JSON5, doc: "{value:" }, + { mode: MODES.YAML, doc: "value:" }, + ])( + "keeps key-only ranges for unfinished properties (%s)", + ({ mode, doc }) => { + const state = EditorState.create({ + doc, + extensions: [getExtensions(mode)], + }); + const pointers = getJsonPointers(state, mode); + const key = mode === MODES.JSON ? '"value"' : "value"; + expect(pointers.get("/value")).toEqual({ + keyFrom: doc.indexOf(key), + keyTo: doc.indexOf(key) + key.length, + }); + }, + ); + it.each([ { doc: '{"object": { "foo": true }, "bar": 123, "baz": [1,2,3], "boop": [{"foo": true}]}', mode: MODES.JSON, expected: { "": { - keyFrom: 0, - keyTo: 80, + valueFrom: 0, + valueTo: 80, }, "/bar": { keyFrom: 28, @@ -119,13 +159,9 @@ describe("getJsonPointers", () => { valueFrom: 47, valueTo: 54, }, - // TODO: return pointers for all array indexes, not just objects - // "/baz/0": { - // keyFrom: 40, - // keyTo: 45, - // valueFrom: 47, - // valueTo: 55, - // }, + "/baz/0": { valueFrom: 48, valueTo: 49 }, + "/baz/1": { valueFrom: 50, valueTo: 51 }, + "/baz/2": { valueFrom: 52, valueTo: 53 }, "/boop": { keyFrom: 56, keyTo: 62, @@ -133,11 +169,8 @@ describe("getJsonPointers", () => { valueTo: 79, }, "/boop/0": { - keyFrom: 65, - keyTo: 78, - // TODO: These look erroneous. There is no key-value pair for array items - valueFrom: 78, - valueTo: 79, + valueFrom: 65, + valueTo: 78, }, "/boop/0/foo": { keyFrom: 66, @@ -146,8 +179,10 @@ describe("getJsonPointers", () => { valueTo: 77, }, "/object": { - keyFrom: 11, - keyTo: 26, + keyFrom: 1, + keyTo: 9, + valueFrom: 11, + valueTo: 26, }, "/object/foo": { keyFrom: 13, @@ -162,8 +197,8 @@ describe("getJsonPointers", () => { mode: MODES.JSON5, expected: { "": { - keyFrom: 0, - keyTo: 72, + valueFrom: 0, + valueTo: 72, }, "/bar": { keyFrom: 26, @@ -177,13 +212,9 @@ describe("getJsonPointers", () => { valueFrom: 43, valueTo: 50, }, - // TODO: return pointers for all array indexes, not just objects - // "/baz/0": { - // keyFrom: 40, - // keyTo: 45, - // valueFrom: 47, - // valueTo: 55, - // }, + "/baz/0": { valueFrom: 44, valueTo: 45 }, + "/baz/1": { valueFrom: 46, valueTo: 47 }, + "/baz/2": { valueFrom: 48, valueTo: 49 }, "/boop": { keyFrom: 52, keyTo: 56, @@ -191,8 +222,8 @@ describe("getJsonPointers", () => { valueTo: 71, }, "/boop/0": { - keyFrom: 59, - keyTo: 70, + valueFrom: 59, + valueTo: 70, }, "/boop/0/foo": { keyFrom: 60, @@ -201,8 +232,10 @@ describe("getJsonPointers", () => { valueTo: 69, }, "/object": { - keyFrom: 11, - keyTo: 24, + keyFrom: 1, + keyTo: 9, + valueFrom: 11, + valueTo: 24, }, "/object/foo": { keyFrom: 13, @@ -227,8 +260,8 @@ boop: mode: MODES.YAML, expected: { "": { - keyFrom: 4, - keyTo: 75, + valueFrom: 0, + valueTo: 75, }, "/bar": { keyFrom: 24, @@ -242,13 +275,9 @@ boop: valueFrom: 40, valueTo: 55, }, - // TODO: return pointers for all array indexes, not just objects - // "/baz/0": { - // keyFrom: 40, - // keyTo: 45, - // valueFrom: 47, - // valueTo: 55, - // }, + "/baz/0": { valueFrom: 42, valueTo: 43 }, + "/baz/1": { valueFrom: 48, valueTo: 49 }, + "/baz/2": { valueFrom: 54, valueTo: 55 }, "/boop": { keyFrom: 56, keyTo: 60, @@ -256,8 +285,8 @@ boop: valueTo: 75, }, "/boop/0": { - keyFrom: 66, - keyTo: 75, + valueFrom: 66, + valueTo: 75, }, "/boop/0/foo": { keyFrom: 66, @@ -266,8 +295,10 @@ boop: valueTo: 75, }, "/object": { - keyFrom: 14, - keyTo: 23, + keyFrom: 4, + keyTo: 10, + valueFrom: 14, + valueTo: 23, }, "/object/foo": { keyFrom: 14, @@ -286,6 +317,6 @@ boop: }); const pointers = getJsonPointers(state, mode); expect(Object.fromEntries(pointers.entries())).toEqual(expected); - } + }, ); }); diff --git a/src/utils/json-pointers.ts b/src/utils/json-pointers.ts index a8e2366..78fbc5f 100644 --- a/src/utils/json-pointers.ts +++ b/src/utils/json-pointers.ts @@ -94,31 +94,37 @@ export const getJsonPointers = ( const pointers: JSONPointersMap = new Map(); tree.iterate({ enter: (type: SyntaxNodeRef) => { - if ( - [TOKENS.PROPERTY_NAME, TOKENS.OBJECT].includes( - resolveTokenName(type.name, mode) as any, - ) - ) { + const node = type.node; + const tokenName = resolveTokenName(node.name, mode); + if (tokenName === TOKENS.PROPERTY_NAME) { const pointer = getJsonPointerAt(state.doc, type.node, mode); - const { from: keyFrom, to: keyTo } = type.node; - // if there's no value, we can't get the valueFrom/to - if (!type.node?.nextSibling?.node) { - pointers.set(pointer, { keyFrom, keyTo }); - return true; + const { from: keyFrom, to: keyTo } = node; + // Grammar versions differ in whether punctuation is a named node. + // Find the value instead of assuming a fixed number of siblings. + let value = node.nextSibling; + while ( + value && + (!isValueNode(value, mode) || value.from === value.to) + ) { + value = value.nextSibling; } - // TODO: Make this generic enough to avoid mode-specific checks - const nextNode = - mode === MODES.JSON - ? type.node?.nextSibling?.node - : type.node?.nextSibling?.node?.nextSibling?.node; - if (!nextNode) { - pointers.set(pointer, { keyFrom, keyTo }); - return true; + pointers.set( + pointer, + value + ? { keyFrom, keyTo, valueFrom: value.from, valueTo: value.to } + : { keyFrom, keyTo }, + ); + // YAML keys contain scalar nodes; those are not property values. + return false; + } + if (isValueNode(node, mode) && tokenName !== TOKENS.ITEM) { + const pointer = getJsonPointerAt(state.doc, node, mode); + // Preserve property key/value ranges when visiting their value nodes. + // Root values and array items have no property key of their own. + if (!pointers.has(pointer)) { + pointers.set(pointer, { valueFrom: node.from, valueTo: node.to }); } - const { from: valueFrom, to: valueTo } = nextNode as SyntaxNode; - pointers.set(pointer, { keyFrom, keyTo, valueFrom, valueTo }); - return true; } }, });