From 3d2f0d6856f771a070c23d1fb3ac31434bb38b73 Mon Sep 17 00:00:00 2001 From: Todd Schiller Date: Sun, 16 Jun 2024 15:05:30 -0400 Subject: [PATCH] 8636: fix search/insert for variable popover using optional chaining 8636: extract getFullVariableName 8636: handle optional chaining corner cases Clarify method comment --- .../widgets/varPopup/VarPopup.tsx | 13 +++- .../varPopup/likelyVariableUtils.test.ts | 31 ++++++++ .../widgets/varPopup/likelyVariableUtils.ts | 44 +++++++++++ .../widgets/varPopup/menuFilters.test.ts | 9 +++ .../widgets/varPopup/menuFilters.ts | 48 ++++++------ src/runtime/pathHelpers.test.ts | 77 ++++++++++++------- src/runtime/pathHelpers.ts | 75 +++++++++++++++--- 7 files changed, 228 insertions(+), 69 deletions(-) diff --git a/src/components/fields/schemaFields/widgets/varPopup/VarPopup.tsx b/src/components/fields/schemaFields/widgets/varPopup/VarPopup.tsx index 9efb2d63cf..6ff3a725bc 100644 --- a/src/components/fields/schemaFields/widgets/varPopup/VarPopup.tsx +++ b/src/components/fields/schemaFields/widgets/varPopup/VarPopup.tsx @@ -17,10 +17,12 @@ import React, { useCallback, useEffect } from "react"; import { type FieldInputMode } from "@/components/fields/schemaFields/fieldInputMode"; -import { replaceLikelyVariable } from "./likelyVariableUtils"; +import { + getFullVariableName, + replaceLikelyVariable, +} from "./likelyVariableUtils"; import VarMenu from "./VarMenu"; import fitTextarea from "fit-textarea"; -import { getPathFromArray } from "@/runtime/pathHelpers"; import useAttachPopup from "@/components/fields/schemaFields/widgets/varPopup/useAttachPopup"; import reportEvent from "@/telemetry/reportEvent"; import { Events } from "@/telemetry/events"; @@ -71,7 +73,10 @@ const VarPopup: React.FunctionComponent = ({ (selectedPath: string[]) => { reportEvent(Events.VAR_POPOVER_SELECT); - const fullVariableName = getPathFromArray(selectedPath); + const fullVariableName = getFullVariableName( + likelyVariable, + selectedPath, + ); switch (inputMode) { case "var": { @@ -117,7 +122,7 @@ const VarPopup: React.FunctionComponent = ({ hideMenu(); }, - [hideMenu, inputElementRef, inputMode, setValue, value], + [hideMenu, inputElementRef, inputMode, setValue, value, likelyVariable], ); if (!isMenuShowing) { diff --git a/src/components/fields/schemaFields/widgets/varPopup/likelyVariableUtils.test.ts b/src/components/fields/schemaFields/widgets/varPopup/likelyVariableUtils.test.ts index d922adc61c..2202b3e710 100644 --- a/src/components/fields/schemaFields/widgets/varPopup/likelyVariableUtils.test.ts +++ b/src/components/fields/schemaFields/widgets/varPopup/likelyVariableUtils.test.ts @@ -16,6 +16,7 @@ */ import { + getFullVariableName, getLikelyVariableAtPosition, replaceLikelyVariable, } from "./likelyVariableUtils"; @@ -314,3 +315,33 @@ describe("replaceLikelyVariable", () => { expect(newCursorPosition).toEqual(endOfVariableIndex); }); }); + +describe("getFullVariableName", () => { + it("preserves optional chaining", () => { + expect(getFullVariableName("@foo", ["@foo", "bar"])).toBe("@foo.bar"); + expect(getFullVariableName("@foo?", ["@foo", "bar"])).toBe("@foo?.bar"); + expect(getFullVariableName("@foo?.bar?", ["@foo", "bar", "baz"])).toBe( + "@foo?.bar?.baz", + ); + }); + + it("handles optional chaining with bracket notation", () => { + expect( + getFullVariableName("@foo.bar?.[42]", ["@foo", "bar", "42", "qux"]), + ).toBe("@foo.bar?.[42].qux"); + expect( + getFullVariableName("@foo.bar[42]?", ["@foo", "bar", "42", "qux"]), + ).toBe("@foo.bar[42]?.qux"); + expect( + getFullVariableName('@foo.bar[""]?', ["@foo", "bar", "", "qux"]), + ).toBe('@foo.bar[""]?.qux'); + expect( + getFullVariableName('@foo.bar["hello world?"]?', [ + "@foo", + "bar", + "hello world?", + "qux", + ]), + ).toBe('@foo.bar["hello world?"]?.qux'); + }); +}); diff --git a/src/components/fields/schemaFields/widgets/varPopup/likelyVariableUtils.ts b/src/components/fields/schemaFields/widgets/varPopup/likelyVariableUtils.ts index 1dd882fa23..bd88c79129 100644 --- a/src/components/fields/schemaFields/widgets/varPopup/likelyVariableUtils.ts +++ b/src/components/fields/schemaFields/widgets/varPopup/likelyVariableUtils.ts @@ -15,6 +15,9 @@ * along with this program. If not, see . */ +import { toPath } from "lodash"; +import { getPathFromArray } from "@/runtime/pathHelpers"; + const varRegex = /(?@(\.|\w|(\[\d+])|(\[("|')[\s\w]+("|')]))*)/g; type LikelyVariable = { @@ -189,3 +192,44 @@ export function replaceLikelyVariable( newCursorPosition: endOfVariableIndex, }; } + +/** + * Select the full variable name based on the selected path and user's expression so far. + */ +export function getFullVariableName( + likelyVariable: string, + selectedPath: string[], +): string { + // `toPath` will create a separate element for the ? symbol. So we need to merge them back. Eventually we need to + // switch from `toPath` to our own implementation/parser. + // @foo.bar[42]? -> [@foo, bar, 42, ?] + const pathWithChainElements = toPath(likelyVariable); + + const likelyPath: string[] = []; + for (let i = 0; i < pathWithChainElements.length; i++) { + // eslint-disable-next-line security/detect-object-injection -- numeric index + const base: string = pathWithChainElements[i]!; + + if (pathWithChainElements[i + 1] === "?") { + likelyPath.push(base + "?"); + i++; + } else { + likelyPath.push(base); + } + } + + return getPathFromArray( + selectedPath.map((part, index) => { + // Preserve optional chaining from what the use has typed so far + if ( + // eslint-disable-next-line security/detect-object-injection -- numeric index + likelyPath[index]?.endsWith("?") && + index !== selectedPath.length - 1 + ) { + return { part, isOptional: true }; + } + + return part; + }), + ); +} diff --git a/src/components/fields/schemaFields/widgets/varPopup/menuFilters.test.ts b/src/components/fields/schemaFields/widgets/varPopup/menuFilters.test.ts index 6290a94d18..85050a6adc 100644 --- a/src/components/fields/schemaFields/widgets/varPopup/menuFilters.test.ts +++ b/src/components/fields/schemaFields/widgets/varPopup/menuFilters.test.ts @@ -91,6 +91,15 @@ describe("filterVarMapByVariable", () => { }), ); + // Exact match with chaining + expect(filterVarMapByVariable(inputMap, "@input?.foo")).toEqual( + expect.objectContaining({ + "@input": expect.objectContaining({ + foo: expect.toBeObject(), + }), + }), + ); + // Empty because trailing period indicates final variable name expect(filterVarMapByVariable(inputMap, "@input.fo.")).toEqual( expect.objectContaining({ diff --git a/src/components/fields/schemaFields/widgets/varPopup/menuFilters.ts b/src/components/fields/schemaFields/widgets/varPopup/menuFilters.ts index 0b7ef5bba8..55c67f054f 100644 --- a/src/components/fields/schemaFields/widgets/varPopup/menuFilters.ts +++ b/src/components/fields/schemaFields/widgets/varPopup/menuFilters.ts @@ -31,6 +31,21 @@ import { getIn } from "formik"; */ export type MenuOptions = Array<[string, UnknownRecord]>; +/** + * Returns true if the value is null or is likely plain text/not a variable. + */ +function isTextOrNull(value: string | null): boolean { + return value == null || value === "@" || !value.startsWith("@"); +} + +/** + * Convert a variable to a normalized variable path, removing optional chaining. Result is suitable for filtering + * by path prefix. + */ +function toVarPath(value: string): string[] { + return toPath(value.replaceAll("?.", ".")); +} + /** * Filter top-level variables by source type. Currently, excludes integration variables because they're managed * automatically by the Page Editor. @@ -54,15 +69,11 @@ export function filterOptionsByVariable( options: MenuOptions, likelyVariable: string, ): MenuOptions { - if ( - likelyVariable == null || - likelyVariable === "@" || - !likelyVariable.startsWith("@") - ) { + if (isTextOrNull(likelyVariable)) { return options; } - const [base, ...rest] = toPath(likelyVariable); + const [base, ...rest] = toVarPath(likelyVariable); return options.filter(([source, vars]) => { if (rest.length === 0) { return Object.keys(vars).some((x) => x.startsWith(base)); @@ -114,15 +125,11 @@ export function filterVarMapByVariable( varMap: UnknownRecord, likelyVariable: string, ): UnknownRecord { - if ( - likelyVariable == null || - likelyVariable === "@" || - !likelyVariable.startsWith("@") - ) { + if (isTextOrNull(likelyVariable)) { return varMap; } - return filterVarMapByPath(varMap, toPath(likelyVariable)); + return filterVarMapByPath(varMap, toVarPath(likelyVariable)); } /** @@ -134,17 +141,13 @@ export function expandCurrentVariableLevel( varMap: UnknownRecord, likelyVariable: string, ): ShouldExpandNodeInitially { - if ( - likelyVariable == null || - likelyVariable === "@" || - !likelyVariable.startsWith("@") - ) { + if (isTextOrNull(likelyVariable)) { return () => false; } // If likelyVariable ends with ".", there's a part for the empty string at the end of the path. So can just use // as normal without logic for trailing "." - const parts = toPath(likelyVariable); + const parts = toVarPath(likelyVariable); return (keyPath, data, level) => { // Key path from JSONTree is in reverse order const reverseKeyPath = [...keyPath].reverse(); @@ -241,12 +244,7 @@ export function defaultMenuOption( return null; } - if ( - likelyVariable == null || - likelyVariable === "@" || - !likelyVariable.startsWith("@") || - toPath(likelyVariable).length === 0 - ) { + if (isTextOrNull(likelyVariable) || toVarPath(likelyVariable).length === 0) { // Must always have at least one option (e.g., the `@input`) // Prefer the last option, because that's the latest output @@ -255,7 +253,7 @@ export function defaultMenuOption( return [first]; } - const parts = toPath(likelyVariable); + const parts = toVarPath(likelyVariable); const [head, ...rest] = parts; diff --git a/src/runtime/pathHelpers.test.ts b/src/runtime/pathHelpers.test.ts index 99411b50ee..0e8f2f101f 100644 --- a/src/runtime/pathHelpers.test.ts +++ b/src/runtime/pathHelpers.test.ts @@ -145,35 +145,54 @@ describe("getFieldNamesFromPathString", () => { }); }); -test("getPathFromArray", () => { - const expectMatch = ( - pathArray: Array, - expectedPathString: string, - ) => { - const pathString = getPathFromArray(pathArray); - const lodashArray = toPath(pathString); - - // Compare the array to the expected string - expect(pathString).toBe(expectedPathString); - - // Expect the same input, except that lodash only returns strings even for numbers - expect(lodashArray).toEqual(pathArray.map(String)); - }; - - expectMatch(["user"], "user"); - expectMatch(["users", 0], "users[0]"); - expectMatch(["users", 0, "name"], "users[0].name"); - expectMatch(["users", ""], 'users[""]'); - expectMatch(["names", "Dante Alighieri"], 'names["Dante Alighieri"]'); - expectMatch( - ["Ugo Foscolo", "User Location"], - '["Ugo Foscolo"]["User Location"]', - ); - expectMatch(["User List", 100, "id"], '["User List"][100].id'); - expectMatch( - ["User List", 100_000_000, "The name"], - '["User List"][100000000]["The name"]', - ); +describe("getPathFromArray", () => { + test("required path parts", () => { + const expectMatch = ( + pathArray: Array, + expectedPathString: string, + ) => { + const pathString = getPathFromArray(pathArray); + const lodashArray = toPath(pathString); + + // Compare the array to the expected string + expect(pathString).toBe(expectedPathString); + + // Expect the same input, except that lodash only returns strings even for numbers + expect(lodashArray).toEqual(pathArray.map(String)); + }; + + expectMatch(["user"], "user"); + expectMatch(["users", 0], "users[0]"); + expectMatch(["users", 0, "name"], "users[0].name"); + expectMatch(["users", ""], 'users[""]'); + expectMatch(["names", "Dante Alighieri"], 'names["Dante Alighieri"]'); + expectMatch( + ["Ugo Foscolo", "User Location"], + '["Ugo Foscolo"]["User Location"]', + ); + expectMatch(["User List", 100, "id"], '["User List"][100].id'); + expectMatch( + ["User List", 100_000_000, "The name"], + '["User List"][100000000]["The name"]', + ); + }); + + test("optional chaining path parts", () => { + expect( + getPathFromArray(["users", { part: 0, isOptional: true }, "name"]), + ).toBe("users[0]?.name"); + expect( + getPathFromArray(["users?", { part: 0, isOptional: true }, "name"]), + ).toBe("users?.[0]?.name"); + expect( + getPathFromArray([ + "users", + "foo bar?", + { part: 0, isOptional: true }, + "name", + ]), + ).toBe('users["foo bar?"].[0]?.name'); + }); }); test.each([ diff --git a/src/runtime/pathHelpers.ts b/src/runtime/pathHelpers.ts index 7971f719e5..54e3e6822d 100644 --- a/src/runtime/pathHelpers.ts +++ b/src/runtime/pathHelpers.ts @@ -15,7 +15,7 @@ * along with this program. If not, see . */ -import { identity, toPath } from "lodash"; +import { identity, toPath, trimEnd } from "lodash"; import { getErrorMessage } from "@/errors/errorHelpers"; import { cleanValue, isObject } from "@/utils/objectUtils"; import { joinName } from "@/utils/formUtils"; @@ -151,6 +151,52 @@ export function getFieldNamesFromPathString( return [parentFieldName, fieldName]; } +/** + * Normalize an array of parts into an explicit format simpler for joining the path back together. + */ +function normalizePart( + partOrRecord: + | string + | number + | { part: string | number; isOptional: boolean }, +): { part: string; isOptional: boolean; isBrackets: boolean } { + if (typeof partOrRecord === "string") { + if (partOrRecord === "" || /[ .]/.test(partOrRecord)) { + return { + part: `["${partOrRecord}"]`, + isOptional: false, + isBrackets: true, + }; + } + + // Treat numeric strings as array access + if (/\d+/.test(partOrRecord)) { + return { + part: `[${partOrRecord}]`, + isOptional: false, + isBrackets: true, + }; + } + + return { + part: trimEnd(partOrRecord, "?"), + isOptional: partOrRecord.endsWith("?"), + isBrackets: false, + }; + } + + if (typeof partOrRecord === "number") { + return { part: `[${partOrRecord}]`, isOptional: false, isBrackets: true }; + } + + const normalized = normalizePart(partOrRecord.part); + + return { + ...normalized, + isOptional: partOrRecord.isOptional || normalized.isOptional, + }; +} + // Counterpart to _.toPath: https://lodash.com/docs/4.17.15#toPath // `fromPath` Missing from lodash: https://github.com/lodash/lodash/issues/2169 /** @@ -160,18 +206,25 @@ export function getFieldNamesFromPathString( * @example getPathFromArray(["user", "name"]) // => "user.name" * @example getPathFromArray(["title", "Divine Comedy"]) // => "title["Divine Comedy"]" */ -export function getPathFromArray(parts: Array): string { - return parts - .map((part, index) => { - if (part === "" || (typeof part === "string" && /[ .]/.test(part))) { - return `["${part}"]`; - } - - if (typeof part === "number" || /^\d+$/.test(part)) { - return `[${part}]`; +export function getPathFromArray( + parts: Array< + string | number | { part: string | number; isOptional: boolean } + >, +): string { + const normalizedParts = parts.map((x) => normalizePart(x)); + + return normalizedParts + .map(({ part, isOptional, isBrackets }, index) => { + let modified = part; + + if (isOptional) { + modified = `${part}?`; } - return index === 0 ? part : `.${part}`; + return index === 0 || + (isBrackets && !normalizedParts[index - 1]?.isOptional) + ? modified + : `.${modified}`; }) .join(""); }