From dcd0a56e370505a96dbced3baabef47bc7859860 Mon Sep 17 00:00:00 2001 From: Jonathon Herbert Date: Sat, 22 Nov 2025 13:14:02 +0000 Subject: [PATCH 1/7] Add position-based mapping tests, and begin work on smoke test --- lib/cql/src/cqlInput/editor/utils.spec.ts | 141 ++++++++++++++++++++++ lib/cql/src/cqlInput/editor/utils.ts | 4 +- lib/cql/src/lang/utils.ts | 12 ++ 3 files changed, 155 insertions(+), 2 deletions(-) diff --git a/lib/cql/src/cqlInput/editor/utils.spec.ts b/lib/cql/src/cqlInput/editor/utils.spec.ts index 3bde018..1b9b7eb 100644 --- a/lib/cql/src/cqlInput/editor/utils.spec.ts +++ b/lib/cql/src/cqlInput/editor/utils.spec.ts @@ -12,6 +12,7 @@ import { POLARITY, schema } from "./schema"; import { builders } from "prosemirror-test-builder"; import { createParser } from "../../lang/Cql"; import { Node } from "prosemirror-model"; +import { getNPermutations, getPermutations } from "../../lang/utils"; describe("utils", () => { const { chip, chipKey, chipValue, doc, queryStr } = builders(schema); @@ -322,6 +323,146 @@ describe("utils", () => { 4, ); }); + + const getCqlStrPositions = ( + queryWithPos: string, + positions: Record = {}, + ): { query: string; positions: Record } => { + const regex = new RegExp("<(?.*?)>"); + const result = regex.exec(queryWithPos); + if (!result || !result.groups?.position) { + return { query: queryWithPos, positions }; + } + + return getCqlStrPositions(queryWithPos.replace(regex, ""), { + ...positions, + [result.groups?.position]: result.index, + }); + }; + + const mappingSpecs = [ + { + name: "a query string", + queryAndPositions: "a", + doc: doc(queryStr("a")), + }, + { + name: "a chip", + queryAndPositions: "+b:c d", + doc: doc( + queryStr(""), + // Empty chipKey + chip(chipKey("b"), chipValue("c")), + // Empty queryStr + queryStr("d"), + ), + }, + { + name: "an empty chipKey", + queryAndPositions: "+: ", + doc: doc( + queryStr(""), + // Empty chipKey + chip(chipKey(""), chipValue()), + // Empty queryStr + queryStr(""), + ), + }, + + { + name: "abutting empty chipKeys", + queryAndPositions: "+: +: ", + doc: doc( + queryStr(""), + // Empty chipKey + chip(chipKey(""), chipValue()), + // Empty queryStr + queryStr(""), + // Empty chipKey + chip(chipKey(""), chipValue()), + ), + }, + { + name: "an empty chipValue", + queryAndPositions: "+b: d", + doc: doc( + queryStr(""), + // Empty chipKey + chip(chipKey("b"), chipValue("")), + // Empty queryStr + queryStr("d"), + ), + }, + ]; + + const smokeTestPrimitives: Array< + (key: string, key2: string) => [string, Node] + > = [ + // Empty queryStr + () => ["", queryStr()], + // queryStr w/ content + (key) => [ + `<${key}1>${key}<${key}2> `, + queryStr(`<${key}1>${key}<${key}2>`), + ], + // Chip w/ empty chipKey + (key) => [`+<${key}>: `, chip(chipKey(`<${key}>`), chipValue())], + // Chip w/ empty chipValue + (key) => [ + `+chipKey:<${key}> `, + chip(chipKey(`chipKey`), chipValue(`<${key}>`)), + ], + // Chip + (key, key2) => [ + `+<${key}1>${key}<${key}2>:<${key2}1>${key2}<${key2}2>`, + chip( + chipKey(`<${key}1>${key}<${key}2>`), + chipValue(`<${key2}1>${key2}<${key2}2>`), + ), + ], + ]; + + const smokeTestSpecs = getNPermutations( + [...smokeTestPrimitives, ...smokeTestPrimitives], + 1000, + ).map(primitives => { + + }) + + mappingSpecs.forEach(({ queryAndPositions, doc, name }) => { + it.only(`should map ${name}`, () => { + const { query, positions } = getCqlStrPositions(queryAndPositions); + const tokens = queryToProseMirrorTokens(query); + const mapping = + createProseMirrorTokenToDocumentMapping(tokens).invert(); + + // Sanity check that this equals the text string + expect( + docToCqlStr(doc), + `Expected the doc to match the query spec, minus any position information`, + ).toBe(query); + + const mappedPositions: Record = {}; + const mappedPositionDebugInfo: Record< + string, + { docPos: number; queryPos: number } + > = {}; + Object.entries(doc.tag).forEach(([docKey, docPos]) => { + if (positions[docKey] === undefined) { + throw new Error(`Expected a document position for key ${docKey}`); + } + + const queryPos = mapping.map(docPos); + mappedPositions[docKey] = queryPos; + mappedPositionDebugInfo[docKey] = { docPos, queryPos }; + }); + + expect( + mappedPositions, + `Mapping didn't match - see the diff. The output was: ${JSON.stringify(mappedPositionDebugInfo, null, " ")}`, + ).toEqual(positions); + }); + }); }); }); diff --git a/lib/cql/src/cqlInput/editor/utils.ts b/lib/cql/src/cqlInput/editor/utils.ts index 715fc46..5dfedb5 100644 --- a/lib/cql/src/cqlInput/editor/utils.ts +++ b/lib/cql/src/cqlInput/editor/utils.ts @@ -144,12 +144,12 @@ const maybeQueryStrRanges = ( index: number, ) => { // If this field is at the start of the document, or preceded by a - // field value, the editor will add a queryStr node to conform to + // field key/value, the editor will add a queryStr node to conform to // the schema, so we add a queryStr mapping to account for the // additional node. const shouldAddQueryStrMapping = - token?.tokenType === "CHIP_VALUE" || index === 0; + token?.tokenType === "CHIP_KEY" || token?.tokenType === "CHIP_VALUE" || index === 0; const queryStrFrom = token ? token?.to + 1 : 0; return shouldAddQueryStrMapping ? getQueryStrRanges(queryStrFrom, queryStrFrom) diff --git a/lib/cql/src/lang/utils.ts b/lib/cql/src/lang/utils.ts index 8ad929c..7fddf4b 100644 --- a/lib/cql/src/lang/utils.ts +++ b/lib/cql/src/lang/utils.ts @@ -15,6 +15,18 @@ export const escapeQuotes = (str: string) => str.replaceAll(`"`, `\\"`); export const shouldQuoteFieldValue = (literal: string) => hasWhitespace(literal) || hasReservedChar(literal); +export function getNPermutations(arr: T[], n: number): T[][] { + const generator = getPermutations(arr); + const permutations: T[][] = []; + for (let i = 0; i < n; i++) { + const next = generator.next(); + if (!next.done) { + permutations.push(next.value); + } + } + return permutations +} + export function* getPermutations( permutation: T[], ): Generator { From 6f440f3e497cfd9bcd0362363d7e8d1833a48db4 Mon Sep 17 00:00:00 2001 From: Jonathon Herbert Date: Mon, 3 Nov 2025 20:17:50 +0000 Subject: [PATCH 2/7] Add property-based testing to discover edge cases --- lib/cql/src/cqlInput/editor/utils.spec.ts | 176 ++++++++++++++++------ lib/cql/src/utils/test.ts | 9 ++ 2 files changed, 140 insertions(+), 45 deletions(-) diff --git a/lib/cql/src/cqlInput/editor/utils.spec.ts b/lib/cql/src/cqlInput/editor/utils.spec.ts index 1b9b7eb..9a17710 100644 --- a/lib/cql/src/cqlInput/editor/utils.spec.ts +++ b/lib/cql/src/cqlInput/editor/utils.spec.ts @@ -1,4 +1,4 @@ -import { describe, expect, it } from "bun:test"; +import { describe, expect, it, test } from "bun:test"; import { createProseMirrorTokenToDocumentMap as createProseMirrorTokenToDocumentMapping, docToCqlStr, @@ -12,7 +12,14 @@ import { POLARITY, schema } from "./schema"; import { builders } from "prosemirror-test-builder"; import { createParser } from "../../lang/Cql"; import { Node } from "prosemirror-model"; -import { getNPermutations, getPermutations } from "../../lang/utils"; +import { + escapeQuotes, + getNPermutations, + getPermutations, + shouldQuoteFieldValue, +} from "../../lang/utils"; +import { pseudoRandom } from "../../utils/test"; +import { logNode } from "./debug"; describe("utils", () => { const { chip, chipKey, chipValue, doc, queryStr } = builders(schema); @@ -347,94 +354,170 @@ describe("utils", () => { doc: doc(queryStr("a")), }, { - name: "a chip", - queryAndPositions: "+b:c d", + name: "an empty chipKey surrounded by empty queryStrs", + queryAndPositions: "+: ", doc: doc( queryStr(""), // Empty chipKey - chip(chipKey("b"), chipValue("c")), + chip(chipKey(""), chipValue("")), // Empty queryStr - queryStr("d"), + queryStr(""), ), }, { - name: "an empty chipKey", - queryAndPositions: "+: ", + name: "an empty chipKey surrounded by contentful queryStrs ", + queryAndPositions: "a +: c", doc: doc( - queryStr(""), + queryStr("a"), // Empty chipKey - chip(chipKey(""), chipValue()), + chip(chipKey(""), chipValue("")), // Empty queryStr - queryStr(""), + queryStr("c"), ), }, - { - name: "abutting empty chipKeys", - queryAndPositions: "+: +: ", + name: "an empty chipValue", + queryAndPositions: "+b: d", doc: doc( queryStr(""), // Empty chipKey - chip(chipKey(""), chipValue()), + chip(chipKey("b"), chipValue("")), // Empty queryStr - queryStr(""), - // Empty chipKey - chip(chipKey(""), chipValue()), + queryStr("d"), ), }, { - name: "an empty chipValue", - queryAndPositions: "+b: d", + name: "a chip", + queryAndPositions: "+b:c d", doc: doc( queryStr(""), // Empty chipKey - chip(chipKey("b"), chipValue("")), + chip(chipKey("b"), chipValue("c")), // Empty queryStr queryStr("d"), ), }, + { + name: "abutting empty chipKeys", + queryAndPositions: "+: +: ", + doc: doc( + queryStr(""), + // Empty chipKey + chip(chipKey(""), chipValue()), + // Empty queryStr + queryStr(""), + // Empty chipKey + chip(chipKey(""), chipValue()), + ), + }, ]; - const smokeTestPrimitives: Array< - (key: string, key2: string) => [string, Node] + const smokeTestLiterals = [ + "u", + // '"', + "unquoted_string", + // "quoted string", + // 'escaped"string', + ]; + + const getSmokeTestString = ( + generator: Generator, + ): string => + smokeTestLiterals[generator.next().value % smokeTestLiterals.length]; + + const getQuotedEscapedStr = (str: string) => + shouldQuoteFieldValue(str) ? `"${escapeQuotes(str)}"` : str; + + const smokeTestQueryStrs: Array< + (key: string, value: string) => [string, Node] > = [ // Empty queryStr () => ["", queryStr()], // queryStr w/ content - (key) => [ - `<${key}1>${key}<${key}2> `, - queryStr(`<${key}1>${key}<${key}2>`), + (key, value) => [ + `<${key}1>${getQuotedEscapedStr(value)}<${key}2> `, + queryStr(`<${key}1>${getQuotedEscapedStr(value)}<${key}2>`), ], + ]; + + const smokeTestChipKeys: Array< + ( + key: string, + value: string, + key2: string, + value2: string, + ) => [string, Node] + > = [ // Chip w/ empty chipKey (key) => [`+<${key}>: `, chip(chipKey(`<${key}>`), chipValue())], // Chip w/ empty chipValue - (key) => [ - `+chipKey:<${key}> `, - chip(chipKey(`chipKey`), chipValue(`<${key}>`)), + (key, value) => [ + `+<${key}1>${getQuotedEscapedStr(value)}<${key}2>:<${key}3> `, + chip(chipKey(`<${key}1>${value}<${key}2>`), chipValue(`<${key}3>`)), ], // Chip - (key, key2) => [ - `+<${key}1>${key}<${key}2>:<${key2}1>${key2}<${key2}2>`, + (key, value, key2, value2) => [ + `+<${key}1>${getQuotedEscapedStr(value)}<${key}2>:<${key2}1>${getQuotedEscapedStr(value2)}<${key2}2> `, chip( - chipKey(`<${key}1>${key}<${key}2>`), - chipValue(`<${key2}1>${key2}<${key2}2>`), + chipKey(`<${key}1>${value}<${key}2>`), + chipValue(`<${key2}1>${value2}<${key2}2>`), ), ], ]; - const smokeTestSpecs = getNPermutations( - [...smokeTestPrimitives, ...smokeTestPrimitives], - 1000, - ).map(primitives => { + const randomGenerator = pseudoRandom(1); + for (let specNo = 0; specNo < 1; specNo++) { + let queryAndPositions = ""; + const docNodes: Node[] = []; + const specDocLength = 1 + (randomGenerator.next().value % 5); + for (let docIndex = 0; docIndex < specDocLength; docIndex++) { + const char = 97 + docIndex * 3; + const queryIndex = + randomGenerator.next().value % smokeTestQueryStrs.length; + const chipKeyIndex = + randomGenerator.next().value % smokeTestChipKeys.length; + + const [queryStrStr, queryStrNode] = smokeTestQueryStrs[queryIndex]( + String.fromCharCode(char), + getSmokeTestString(randomGenerator), + ); + + const [chipStr, chipNode] = smokeTestChipKeys[chipKeyIndex]( + String.fromCharCode(char + 1), + getSmokeTestString(randomGenerator), + String.fromCharCode(char + 2), + getSmokeTestString(randomGenerator), + ); + + queryAndPositions += queryStrStr + chipStr; + docNodes.push(queryStrNode, chipNode); + } + + const docNode = doc(...docNodes, queryStr()); + try { + docNode.check(); + // eslint-disable-next-line @typescript-eslint/no-unused-vars + } catch (_) { + logNode(docNode); + throw new Error( + "Property test created an invalid node — the structure is logged above", + ); + } - }) + mappingSpecs.push({ + name: `property test ${specNo} (\`${queryAndPositions}\`)`, + queryAndPositions, + doc: doc(...docNodes, queryStr()), + }); + } mappingSpecs.forEach(({ queryAndPositions, doc, name }) => { it.only(`should map ${name}`, () => { const { query, positions } = getCqlStrPositions(queryAndPositions); const tokens = queryToProseMirrorTokens(query); - const mapping = - createProseMirrorTokenToDocumentMapping(tokens).invert(); + const queryStrToDocMapping = + createProseMirrorTokenToDocumentMapping(tokens); + const docToQueryStrMapping = queryStrToDocMapping.invert(); // Sanity check that this equals the text string expect( @@ -442,7 +525,7 @@ describe("utils", () => { `Expected the doc to match the query spec, minus any position information`, ).toBe(query); - const mappedPositions: Record = {}; + const positionsMappedFromDocToQueryStr: Record = {}; const mappedPositionDebugInfo: Record< string, { docPos: number; queryPos: number } @@ -452,14 +535,17 @@ describe("utils", () => { throw new Error(`Expected a document position for key ${docKey}`); } - const queryPos = mapping.map(docPos); - mappedPositions[docKey] = queryPos; + const queryPos = docToQueryStrMapping.map(docPos); + + const remappedDocPos = queryStrToDocMapping.map(queryPos, 1); + expect(remappedDocPos, `${queryAndPositions} -> ${queryPos} -> ${docKey}${docPos}`).toBe(docPos); + positionsMappedFromDocToQueryStr[docKey] = queryPos; mappedPositionDebugInfo[docKey] = { docPos, queryPos }; }); expect( - mappedPositions, - `Mapping didn't match - see the diff. The output was: ${JSON.stringify(mappedPositionDebugInfo, null, " ")}`, + positionsMappedFromDocToQueryStr, + `Mapping didn't match - see the diff. The output for the query \`${query}\`, with positions at \`${queryAndPositions}\` was: ${JSON.stringify(mappedPositionDebugInfo, null, " ")}`, ).toEqual(positions); }); }); diff --git a/lib/cql/src/utils/test.ts b/lib/cql/src/utils/test.ts index 1b381d4..e67ec28 100644 --- a/lib/cql/src/utils/test.ts +++ b/lib/cql/src/utils/test.ts @@ -104,3 +104,12 @@ export const docToCqlStrWithSelection = (_state: EditorState) => { ); return docToCqlStr(newState.doc); }; + +export function* pseudoRandom(seed: number): Generator { + let value = seed; + + while(true) { + value = value * 16807 % 2147483647; + yield value; + } +}; From f5241830a600cbbf204ad715345147d14171d19b Mon Sep 17 00:00:00 2001 From: Jonathon Herbert Date: Mon, 3 Nov 2025 20:19:14 +0000 Subject: [PATCH 3/7] Rewrite mappings using tests as a guide to make them simpler and more comprehensible ... but although the tests pass, they are not reversible, which stretches my mental model a bit ... I need to understand better why this wasn't a problem before --- lib/cql/src/cqlInput/editor/utils.ts | 74 +++++++++++++--------------- 1 file changed, 33 insertions(+), 41 deletions(-) diff --git a/lib/cql/src/cqlInput/editor/utils.ts b/lib/cql/src/cqlInput/editor/utils.ts index 5dfedb5..14e6004 100644 --- a/lib/cql/src/cqlInput/editor/utils.ts +++ b/lib/cql/src/cqlInput/editor/utils.ts @@ -82,38 +82,26 @@ export const joinQueryStrTokens = (tokens: ProseMirrorToken[]) => const getFieldKeyRange = ( from: number, to: number, - isQuoted: boolean, - isFollowedByEmptyValue: boolean, + literalOffsetStart: number, + literalOffsetEnd: number, + isFollowedByChipValue: boolean, ): [number, number, number][] => { - const quoteOffset = isQuoted ? 1 : 0; - const emptyValueOffset = isFollowedByEmptyValue ? 1 : 0; return [ - // chip begin (-1) - // chipKey begin (-1) - [from, 0, 2], - // maybe start quote (+1 to remove from str) - [from, quoteOffset, 0], - // maybe end quote (+1 to remove from str) - // offset `:` into chipValue (+1 to node) - [to - 1, quoteOffset, 1 + emptyValueOffset], + [from, literalOffsetStart, 2 /* start, start */], + [to - 1, literalOffsetEnd - 1 /* -1 to account for the `:` following the key */, 0], + ...(!isFollowedByChipValue ? getFieldValueRanges(to, to, 0, 0) : []), ]; }; const getFieldValueRanges = ( from: number, to: number, - isQuoted: boolean, + literalOffsetStart: number, + literalOffsetEnd: number, ): [number, number, number][] => { - const quoteOffset = isQuoted ? 1 : 0; - return [ - // chipKey end, chipValue start (-1) - // remove offset from `:` - // maybe start quote - [from, 1 + quoteOffset, 1], - // chipValue end (-1) - // maybe end quote (+1) - [to, quoteOffset, 1], + [from, literalOffsetStart, 1 /* end / start */], + [to, literalOffsetEnd, 1 /* end */], ]; }; @@ -121,21 +109,19 @@ const getQueryStrRanges = ( from: number, to: number, ): [number, number, number][] => [ - [from, -1, 0], // queryStr begin (+1) - // If the queryStr node has content, it will begin with whitespace in the - // query, which pushes subsequent content forward one position. This cancels - // out queryStr end (-1), so only add a mapping for the node end if the - // queryStr node has content. - // - // This is a slightly obscure solution to this problem - we could also use the - // gaps between the token positions and the token literal length to account - // for this discrepancy, too. - ...(from === to ? [[to, -1, 0] as [number, number, number]] : []), + [from, 0, 1 /* node begin */], + [ + to, + from !== to + ? 1 + : 0 /* whitespace after queryStr when queryStr has content */, + 1 /* node end */, + ], ]; const polarityRanges = (from: number): [number, number, number] => [ - from - 1, - 1, + from, + 1, // Polarity char (`-`|`+`) 0, ]; @@ -149,7 +135,9 @@ const maybeQueryStrRanges = ( // additional node. const shouldAddQueryStrMapping = - token?.tokenType === "CHIP_KEY" || token?.tokenType === "CHIP_VALUE" || index === 0; + token?.tokenType === "CHIP_KEY" || + token?.tokenType === "CHIP_VALUE" || + index === 0; const queryStrFrom = token ? token?.to + 1 : 0; return shouldAddQueryStrMapping ? getQueryStrRanges(queryStrFrom, queryStrFrom) @@ -188,8 +176,12 @@ export const createProseMirrorTokenToDocumentMap = ( const compactedTokenRanges = joinQueryStrTokens(tokens); const ranges = compactedTokenRanges.reduce<[number, number, number][]>( - (accRanges, { tokenType, from, to, literal }, index, tokens) => { + (accRanges, { tokenType, from, to, lexeme, literal }, index, tokens) => { const previousToken = tokens[index - 1] as ProseMirrorToken | undefined; + const nextToken = tokens[index + 1] as ProseMirrorToken | undefined; + const literalOffsetStart = literal ? lexeme.indexOf(literal) : 0; + const literalOffsetEnd = + lexeme.length - (literal?.length ?? 0) - literalOffsetStart; switch (tokenType) { case TokenType.PLUS: @@ -200,15 +192,14 @@ export const createProseMirrorTokenToDocumentMap = ( ]); } case TokenType.CHIP_KEY: { - const isFollowedByFieldValueToken = - tokens[index + 1]?.tokenType === "CHIP_VALUE"; return accRanges.concat( ...maybeQueryStrRanges(previousToken, index), getFieldKeyRange( from, to, - shouldQuoteFieldValue(literal ?? ""), - !isFollowedByFieldValueToken, + literalOffsetStart, + literalOffsetEnd, + nextToken?.tokenType === TokenType.CHIP_VALUE, ), ); } @@ -217,7 +208,8 @@ export const createProseMirrorTokenToDocumentMap = ( ...getFieldValueRanges( from, to, - shouldQuoteFieldValue(literal ?? ""), + literalOffsetStart, + literalOffsetEnd, ), ); } From a5ddead2d47f6724dd8e3e6e1728dd40207e9d59 Mon Sep 17 00:00:00 2001 From: Jonathon Herbert Date: Mon, 3 Nov 2025 21:16:45 +0000 Subject: [PATCH 4/7] Correct mappings, reversing the specs to test queryStr -> doc first, which makes the behaviour clearer I was testing an inverted mapping, making changes unintuitive D:< Having said that, I'm not quite sure why getFieldValueRanges requires a negative oldSize rather than a positive toSize for its chipValue end, which is something I arrived at via experimentation rather than a complete understanding ... dislike --- lib/cql/src/cqlInput/editor/diff.ts | 2 +- lib/cql/src/cqlInput/editor/editor.spec.ts | 13 ----- .../src/cqlInput/editor/plugins/cql.spec.ts | 4 +- lib/cql/src/cqlInput/editor/utils.spec.ts | 49 +++++++++++++------ lib/cql/src/cqlInput/editor/utils.ts | 4 +- 5 files changed, 40 insertions(+), 32 deletions(-) diff --git a/lib/cql/src/cqlInput/editor/diff.ts b/lib/cql/src/cqlInput/editor/diff.ts index 9913d56..a829da5 100644 --- a/lib/cql/src/cqlInput/editor/diff.ts +++ b/lib/cql/src/cqlInput/editor/diff.ts @@ -1,7 +1,7 @@ import { Fragment } from "prosemirror-model"; /** - * These diff functions are vendored to ignore attributes and markup, as we only care about markup. + * These diff functions are vendored to ignore attributes, as we only care about markup. * Source: https://github.com/ProseMirror/prosemirror-model/blob/c8c7b62645d2a8293fa6b7f52aa2b04a97821f34/src/diff.ts */ diff --git a/lib/cql/src/cqlInput/editor/editor.spec.ts b/lib/cql/src/cqlInput/editor/editor.spec.ts index 67a816f..3d2ca61 100644 --- a/lib/cql/src/cqlInput/editor/editor.spec.ts +++ b/lib/cql/src/cqlInput/editor/editor.spec.ts @@ -127,17 +127,4 @@ describe("updateEditorViewWithQueryStr", () => { "+tag:^$x +tag:y ", ); }); - - it("should preserve the selection state insofar as possible - adding tag", () => { - const initialQuery = "tag:"; - const { editorView, updateEditorView } = - createEditorFromInitialState(initialQuery); - setQueryPosAsSelection(initialQuery.length, editorView); - - expect(docToCqlStrWithSelection(editorView.state)).toEqual("+tag:^$ "); - - updateEditorView("+tag:"); - - expect(docToCqlStrWithSelection(editorView.state)).toEqual("+tag:^$ "); - }); }); diff --git a/lib/cql/src/cqlInput/editor/plugins/cql.spec.ts b/lib/cql/src/cqlInput/editor/plugins/cql.spec.ts index ecc1d56..8e2138b 100644 --- a/lib/cql/src/cqlInput/editor/plugins/cql.spec.ts +++ b/lib/cql/src/cqlInput/editor/plugins/cql.spec.ts @@ -926,7 +926,7 @@ describe("cql plugin", () => { const { editor, waitFor, getPosFromQueryPos } = createCqlEditor(queryStr); - editor.selectText(getPosFromQueryPos(queryStr.indexOf(":"))); + editor.selectText(getPosFromQueryPos(queryStr.indexOf("+"))); await editor.press(key); @@ -938,7 +938,7 @@ describe("cql plugin", () => { const { editor, waitFor, getPosFromQueryPos } = createCqlEditor(queryStr); - editor.selectText(getPosFromQueryPos(queryStr.indexOf(":"))); + editor.selectText(getPosFromQueryPos(queryStr.indexOf(":") + 1)); await editor.press(key); diff --git a/lib/cql/src/cqlInput/editor/utils.spec.ts b/lib/cql/src/cqlInput/editor/utils.spec.ts index 9a17710..12bcaa2 100644 --- a/lib/cql/src/cqlInput/editor/utils.spec.ts +++ b/lib/cql/src/cqlInput/editor/utils.spec.ts @@ -1,4 +1,4 @@ -import { describe, expect, it, test } from "bun:test"; +import { describe, expect, it } from "bun:test"; import { createProseMirrorTokenToDocumentMap as createProseMirrorTokenToDocumentMapping, docToCqlStr, @@ -12,12 +12,7 @@ import { POLARITY, schema } from "./schema"; import { builders } from "prosemirror-test-builder"; import { createParser } from "../../lang/Cql"; import { Node } from "prosemirror-model"; -import { - escapeQuotes, - getNPermutations, - getPermutations, - shouldQuoteFieldValue, -} from "../../lang/utils"; +import { escapeQuotes, shouldQuoteFieldValue } from "../../lang/utils"; import { pseudoRandom } from "../../utils/test"; import { logNode } from "./debug"; @@ -466,7 +461,7 @@ describe("utils", () => { ]; const randomGenerator = pseudoRandom(1); - for (let specNo = 0; specNo < 1; specNo++) { + for (let specNo = 0; specNo < 100; specNo++) { let queryAndPositions = ""; const docNodes: Node[] = []; const specDocLength = 1 + (randomGenerator.next().value % 5); @@ -512,7 +507,7 @@ describe("utils", () => { } mappingSpecs.forEach(({ queryAndPositions, doc, name }) => { - it.only(`should map ${name}`, () => { + it(`should map ${name}`, () => { const { query, positions } = getCqlStrPositions(queryAndPositions); const tokens = queryToProseMirrorTokens(query); const queryStrToDocMapping = @@ -525,8 +520,36 @@ describe("utils", () => { `Expected the doc to match the query spec, minus any position information`, ).toBe(query); + const positionsMappedFromQueryStrToDoc: Record = {}; + const mappedDocPositionDebugInfo: Record< + string, + { docPos: number; queryPos: number } + > = {}; + Object.entries(positions).forEach(([queryKey, queryPos]) => { + if (positions[queryKey] === undefined) { + throw new Error( + `Expected a document position for key ${queryKey}`, + ); + } + + const docPos = queryStrToDocMapping.map(queryPos); + + const remappedDocPos = queryStrToDocMapping.invert().map(docPos); + expect( + remappedDocPos, + `${queryAndPositions} -> ${queryPos} -> ${queryKey}${docPos}`, + ).toBe(queryPos); + positionsMappedFromQueryStrToDoc[queryKey] = docPos; + mappedDocPositionDebugInfo[queryKey] = { docPos, queryPos }; + }); + + expect( + positionsMappedFromQueryStrToDoc, + `Mapping didn't match from queryStr to doc - see the diff. The output for the query \`${query}\`, with positions at \`${queryAndPositions}\` was: ${JSON.stringify(mappedDocPositionDebugInfo, null, " ")}`, + ).toEqual(doc.tag); + const positionsMappedFromDocToQueryStr: Record = {}; - const mappedPositionDebugInfo: Record< + const mappedQueryPositionDebugInfo: Record< string, { docPos: number; queryPos: number } > = {}; @@ -537,15 +560,13 @@ describe("utils", () => { const queryPos = docToQueryStrMapping.map(docPos); - const remappedDocPos = queryStrToDocMapping.map(queryPos, 1); - expect(remappedDocPos, `${queryAndPositions} -> ${queryPos} -> ${docKey}${docPos}`).toBe(docPos); positionsMappedFromDocToQueryStr[docKey] = queryPos; - mappedPositionDebugInfo[docKey] = { docPos, queryPos }; + mappedQueryPositionDebugInfo[docKey] = { docPos, queryPos }; }); expect( positionsMappedFromDocToQueryStr, - `Mapping didn't match - see the diff. The output for the query \`${query}\`, with positions at \`${queryAndPositions}\` was: ${JSON.stringify(mappedPositionDebugInfo, null, " ")}`, + `Mapping didn't match from doc to queryStr - see the diff. The output for the query \`${query}\`, with positions at \`${queryAndPositions}\` was: ${JSON.stringify(mappedQueryPositionDebugInfo, null, " ")}`, ).toEqual(positions); }); }); diff --git a/lib/cql/src/cqlInput/editor/utils.ts b/lib/cql/src/cqlInput/editor/utils.ts index 14e6004..90c5239 100644 --- a/lib/cql/src/cqlInput/editor/utils.ts +++ b/lib/cql/src/cqlInput/editor/utils.ts @@ -101,7 +101,7 @@ const getFieldValueRanges = ( ): [number, number, number][] => { return [ [from, literalOffsetStart, 1 /* end / start */], - [to, literalOffsetEnd, 1 /* end */], + [to + 1, literalOffsetEnd - 1, 0 /* end */], ]; }; @@ -120,7 +120,7 @@ const getQueryStrRanges = ( ]; const polarityRanges = (from: number): [number, number, number] => [ - from, + from - 1, 1, // Polarity char (`-`|`+`) 0, ]; From 5fd800299f9a3e03bcafb1995229829b339cd657 Mon Sep 17 00:00:00 2001 From: Jonathon Herbert Date: Tue, 4 Nov 2025 22:18:49 +0000 Subject: [PATCH 5/7] Add mappings for quoted and escaped chipKeys and chipValues cleaning up the ranges to remove the odd inversion, and expanding the property-based tests to verify they are correct --- lib/cql/src/cqlInput/editor/utils.spec.ts | 36 ++++++++++++++++------- lib/cql/src/cqlInput/editor/utils.ts | 26 +++++++++++----- 2 files changed, 44 insertions(+), 18 deletions(-) diff --git a/lib/cql/src/cqlInput/editor/utils.spec.ts b/lib/cql/src/cqlInput/editor/utils.spec.ts index 12bcaa2..0232757 100644 --- a/lib/cql/src/cqlInput/editor/utils.spec.ts +++ b/lib/cql/src/cqlInput/editor/utils.spec.ts @@ -405,14 +405,36 @@ describe("utils", () => { chip(chipKey(""), chipValue()), ), }, + { + name: "a chip with quoted key", + queryAndPositions: '+"\\"":c d', + doc: doc( + queryStr(""), + // Empty chipKey + chip(chipKey('"'), chipValue("c")), + // Empty queryStr + queryStr("d"), + ), + }, + { + name: "a chip with quoted value", + queryAndPositions: '+"\\"":"\\"" d', + doc: doc( + queryStr(""), + // Empty chipKey + chip(chipKey('"'), chipValue("\"")), + // Empty queryStr + queryStr("d"), + ), + }, ]; const smokeTestLiterals = [ "u", - // '"', + '"', "unquoted_string", - // "quoted string", - // 'escaped"string', + "quoted string", + 'escaped"string', ]; const getSmokeTestString = ( @@ -533,12 +555,6 @@ describe("utils", () => { } const docPos = queryStrToDocMapping.map(queryPos); - - const remappedDocPos = queryStrToDocMapping.invert().map(docPos); - expect( - remappedDocPos, - `${queryAndPositions} -> ${queryPos} -> ${queryKey}${docPos}`, - ).toBe(queryPos); positionsMappedFromQueryStrToDoc[queryKey] = docPos; mappedDocPositionDebugInfo[queryKey] = { docPos, queryPos }; }); @@ -566,7 +582,7 @@ describe("utils", () => { expect( positionsMappedFromDocToQueryStr, - `Mapping didn't match from doc to queryStr - see the diff. The output for the query \`${query}\`, with positions at \`${queryAndPositions}\` was: ${JSON.stringify(mappedQueryPositionDebugInfo, null, " ")}`, + `Mapping didn't match from doc to queryStr (inverted mapping) - see the diff. The output for the query \`${query}\`, with positions at \`${queryAndPositions}\` was: ${JSON.stringify(mappedQueryPositionDebugInfo, null, " ")}`, ).toEqual(positions); }); }); diff --git a/lib/cql/src/cqlInput/editor/utils.ts b/lib/cql/src/cqlInput/editor/utils.ts index 90c5239..8f58bc2 100644 --- a/lib/cql/src/cqlInput/editor/utils.ts +++ b/lib/cql/src/cqlInput/editor/utils.ts @@ -87,8 +87,13 @@ const getFieldKeyRange = ( isFollowedByChipValue: boolean, ): [number, number, number][] => { return [ - [from, literalOffsetStart, 2 /* start, start */], - [to - 1, literalOffsetEnd - 1 /* -1 to account for the `:` following the key */, 0], + [from, 0, 2 /* start, start */], + [from, literalOffsetStart, 0], + [ + to - literalOffsetEnd, + literalOffsetEnd - 1, + 0, + ], ...(!isFollowedByChipValue ? getFieldValueRanges(to, to, 0, 0) : []), ]; }; @@ -99,9 +104,12 @@ const getFieldValueRanges = ( literalOffsetStart: number, literalOffsetEnd: number, ): [number, number, number][] => { + return [ - [from, literalOffsetStart, 1 /* end / start */], - [to + 1, literalOffsetEnd - 1, 0 /* end */], + [from, 0, 1 /* end / start */], + [from, literalOffsetStart, 0], + [to - literalOffsetEnd, literalOffsetEnd, 0], + [to, 0, 1 /* end */], ]; }; @@ -179,17 +187,19 @@ export const createProseMirrorTokenToDocumentMap = ( (accRanges, { tokenType, from, to, lexeme, literal }, index, tokens) => { const previousToken = tokens[index - 1] as ProseMirrorToken | undefined; const nextToken = tokens[index + 1] as ProseMirrorToken | undefined; - const literalOffsetStart = literal ? lexeme.indexOf(literal) : 0; + + const unescapedLiteral = unescapeQuotes(literal ?? ""); + const literalOffsetStart = unescapedLiteral ? lexeme.indexOf(unescapedLiteral) : 0; const literalOffsetEnd = - lexeme.length - (literal?.length ?? 0) - literalOffsetStart; + lexeme.length - (unescapedLiteral?.length ?? 0) - literalOffsetStart; switch (tokenType) { case TokenType.PLUS: case TokenType.MINUS: { - return accRanges.concat([ + return accRanges.concat( ...maybeQueryStrRanges(previousToken, index), polarityRanges(from), - ]); + ); } case TokenType.CHIP_KEY: { return accRanges.concat( From ee1475516ac07026df412472533924e3ef858ceb Mon Sep 17 00:00:00 2001 From: Jonathon Herbert Date: Tue, 4 Nov 2025 22:34:46 +0000 Subject: [PATCH 6/7] Tidy up a few things I spotted during the refactor --- lib/cql/src/cqlInput/editor/utils.ts | 10 ++++------ 1 file changed, 4 insertions(+), 6 deletions(-) diff --git a/lib/cql/src/cqlInput/editor/utils.ts b/lib/cql/src/cqlInput/editor/utils.ts index 8f58bc2..8a92b08 100644 --- a/lib/cql/src/cqlInput/editor/utils.ts +++ b/lib/cql/src/cqlInput/editor/utils.ts @@ -89,11 +89,7 @@ const getFieldKeyRange = ( return [ [from, 0, 2 /* start, start */], [from, literalOffsetStart, 0], - [ - to - literalOffsetEnd, - literalOffsetEnd - 1, - 0, - ], + [to - literalOffsetEnd, literalOffsetEnd - 1, 0], ...(!isFollowedByChipValue ? getFieldValueRanges(to, to, 0, 0) : []), ]; }; @@ -189,7 +185,9 @@ export const createProseMirrorTokenToDocumentMap = ( const nextToken = tokens[index + 1] as ProseMirrorToken | undefined; const unescapedLiteral = unescapeQuotes(literal ?? ""); - const literalOffsetStart = unescapedLiteral ? lexeme.indexOf(unescapedLiteral) : 0; + const literalOffsetStart = unescapedLiteral + ? lexeme.indexOf(unescapedLiteral) + : 0; const literalOffsetEnd = lexeme.length - (unescapedLiteral?.length ?? 0) - literalOffsetStart; From 07822bdee13af99f521bd8854df8770523c9d8b3 Mon Sep 17 00:00:00 2001 From: Jonathon Herbert Date: Sun, 23 Nov 2025 12:30:24 +0000 Subject: [PATCH 7/7] Backport a few things missed from jsh/pos-based-suggestions --- lib/cql/src/cqlInput/editor/plugins/cql.spec.ts | 6 +++--- lib/cql/src/cqlInput/editor/utils.spec.ts | 2 +- lib/cql/src/cqlInput/editor/utils.ts | 2 +- lib/cql/src/lang/typeahead.spec.ts | 14 +++++++------- lib/cql/src/lang/typeahead.ts | 4 ++-- 5 files changed, 14 insertions(+), 14 deletions(-) diff --git a/lib/cql/src/cqlInput/editor/plugins/cql.spec.ts b/lib/cql/src/cqlInput/editor/plugins/cql.spec.ts index 8e2138b..ef85bf9 100644 --- a/lib/cql/src/cqlInput/editor/plugins/cql.spec.ts +++ b/lib/cql/src/cqlInput/editor/plugins/cql.spec.ts @@ -432,7 +432,7 @@ describe("cql plugin", () => { const { editor, container, moveCaretToQueryPos } = createCqlEditor("example +tag:"); - await moveCaretToQueryPos(queryStr.length - 1); + await moveCaretToQueryPos(queryStr.length); await editor.insertText("t"); const popoverContainer = await findByTestId( @@ -448,7 +448,7 @@ describe("cql plugin", () => { const { editor, container, waitFor, moveCaretToQueryPos } = createCqlEditor("example +tag:"); - await moveCaretToQueryPos(queryStr.length - 1); + await moveCaretToQueryPos(queryStr.length); await editor.insertText("t"); await selectPopoverOptionWithEnter( editor, @@ -464,7 +464,7 @@ describe("cql plugin", () => { const { editor, container, waitFor, moveCaretToQueryPos } = createCqlEditor("example +tag:"); - await moveCaretToQueryPos(queryStr.length - 1); + await moveCaretToQueryPos(queryStr.length); await editor.insertText("t"); await selectPopoverOptionWithClick( container, diff --git a/lib/cql/src/cqlInput/editor/utils.spec.ts b/lib/cql/src/cqlInput/editor/utils.spec.ts index 0232757..b934605 100644 --- a/lib/cql/src/cqlInput/editor/utils.spec.ts +++ b/lib/cql/src/cqlInput/editor/utils.spec.ts @@ -314,7 +314,7 @@ describe("utils", () => { assertCqlStrPosFromDocPos( "+a:", (node) => findNodeAt(0, node, schema.nodes.chipValue) + 1, - 2, + 3, ); }); diff --git a/lib/cql/src/cqlInput/editor/utils.ts b/lib/cql/src/cqlInput/editor/utils.ts index 8a92b08..49c8d3c 100644 --- a/lib/cql/src/cqlInput/editor/utils.ts +++ b/lib/cql/src/cqlInput/editor/utils.ts @@ -202,7 +202,7 @@ export const createProseMirrorTokenToDocumentMap = ( case TokenType.CHIP_KEY: { return accRanges.concat( ...maybeQueryStrRanges(previousToken, index), - getFieldKeyRange( + ...getFieldKeyRange( from, to, literalOffsetStart, diff --git a/lib/cql/src/lang/typeahead.spec.ts b/lib/cql/src/lang/typeahead.spec.ts index 354bf32..bb8892f 100644 --- a/lib/cql/src/lang/typeahead.spec.ts +++ b/lib/cql/src/lang/typeahead.spec.ts @@ -104,7 +104,7 @@ describe("typeahead", () => { suffix: ":", }, { - from: 4, + from: 5, to: 18, position: "chipValue", suggestions: testTags, @@ -131,7 +131,7 @@ describe("typeahead", () => { type: "TEXT", }, { - from: 5, + from: 6, to: 8, position: "chipValue", suggestions: [ @@ -186,8 +186,8 @@ describe("typeahead", () => { suffix: ":", }, { - from: 4, - to: 4, + from: 5, + to: 5, position: "chipValue", suggestions: testTags, type: "TEXT", @@ -215,8 +215,8 @@ describe("typeahead", () => { suffix: ":", }, { - from: 10, - to: 10, + from: 11, + to: 11, position: "chipValue", suggestions: [ new DateSuggestionOption("1 day ago", "-1d"), @@ -250,7 +250,7 @@ describe("typeahead", () => { suffix: ":", }, { - from: 4, + from: 5, to: 5, position: "chipValue", suggestions: testTags, diff --git a/lib/cql/src/lang/typeahead.ts b/lib/cql/src/lang/typeahead.ts index c1d9274..071f92f 100644 --- a/lib/cql/src/lang/typeahead.ts +++ b/lib/cql/src/lang/typeahead.ts @@ -148,8 +148,8 @@ export class Typeahead { return maybeValueSuggestions.suggestions.then((suggestions) => [ ...keySuggestions, { - from: value ? value.start - 1 : key.end, // Extend backwards into chipKey's ':' - to: value ? value.end : key.end, + from: value ? value.start : key.end + 1, + to: value ? value.end : key.end + 1, position: "chipValue", suggestions, type: maybeValueSuggestions.type,