From b69de71029dcb46c5e5e8a326df48beb56be3dde Mon Sep 17 00:00:00 2001 From: vince Date: Fri, 19 Jun 2026 12:21:15 +0200 Subject: [PATCH] fix(agent-config): stabilize patch and target resolution --- .../server/agent-config/agent-config.test.ts | 149 ++++++++++++++++ src/lib/server/agent-config/index.ts | 159 ++++++++++++++---- 2 files changed, 271 insertions(+), 37 deletions(-) diff --git a/src/lib/server/agent-config/agent-config.test.ts b/src/lib/server/agent-config/agent-config.test.ts index 5ee535f..87449e8 100644 --- a/src/lib/server/agent-config/agent-config.test.ts +++ b/src/lib/server/agent-config/agent-config.test.ts @@ -3,6 +3,7 @@ import { mkdtemp } from "node:fs/promises"; import { tmpdir } from "node:os"; import { join } from "node:path"; import { afterEach, describe, expect, test } from "vitest"; +import type { DashboardDocument } from "$lib/model"; import { genericDashboardFixture } from "$lib/model/fixtures/generic"; import { createDashboardStore, type DashboardStore } from "$lib/server/db/dashboard-store"; import { @@ -13,6 +14,7 @@ import { publishDashboardChanges, rollbackDashboardRevision, type AgentDashboardOperation, + type JsonPatchOperation, } from "."; const stores: DashboardStore[] = []; @@ -125,6 +127,82 @@ describe("agent dashboard configuration API", () => { expect(result.patch.map((operation) => operation.op)).toContain("remove"); }); + test("returns RFC 6902-applicable patches for multiple array removals", () => { + const result = previewDashboardChanges(genericDashboardFixture, [ + { + type: "remove_item", + target: { kind: "statusItem", stripId: "runtime", id: "status" }, + }, + { + type: "remove_item", + target: { kind: "statusItem", stripId: "runtime", id: "sync" }, + }, + ]); + + expect(result.ok).toBe(true); + if (!result.ok) throw new Error("expected remove success"); + expect( + result.patch + .filter((operation) => operation.op === "remove") + .map((operation) => operation.path), + ).toEqual(["/statusStrips/0/items/1", "/statusStrips/0/items/0"]); + expect(applyJsonPatch(genericDashboardFixture, result.patch)).toMatchObject({ + statusStrips: [{ items: [] }], + }); + }); + + test("rejects ambiguous service and status targets", () => { + const duplicateDocument = documentWithDuplicateNestedIds(); + + const connect = previewDashboardChanges(duplicateDocument, [ + { + type: "connect_datasource", + target: { kind: "service", id: "identity" }, + datasource: { + type: "external", + adapter: "http-status", + reference: "GET https://identity.example.test/health", + }, + }, + ]); + expect(connect.ok).toBe(false); + if (connect.ok) throw new Error("expected ambiguous service failure"); + expect(connect.errors[0]).toMatchObject({ + code: "ambiguous_target", + operationIndex: 0, + path: "/serviceGroups", + }); + + const remove = previewDashboardChanges(duplicateDocument, [ + { + type: "remove_item", + target: { kind: "service", id: "identity" }, + }, + ]); + expect(remove.ok).toBe(false); + if (remove.ok) throw new Error("expected ambiguous removal failure"); + expect(remove.errors[0]).toMatchObject({ + code: "ambiguous_target", + operationIndex: 0, + path: "/serviceGroups", + }); + + const status = previewDashboardChanges(duplicateDocument, [ + { + type: "set_status_rule", + target: { kind: "statusItem", id: "status" }, + value: "Healthy", + }, + ]); + expect(status.ok).toBe(false); + if (status.ok) throw new Error("expected ambiguous status failure"); + expect(status.errors[0]).toMatchObject({ + code: "ambiguous_target", + operationIndex: 0, + path: "/statusStrips", + }); + }); + test("rejects create_dashboard when it is not the first operation", () => { const result = previewDashboardChanges(genericDashboardFixture, [ { @@ -434,6 +512,77 @@ function exampleService() { }; } +function documentWithDuplicateNestedIds(): DashboardDocument { + const document = structuredClone(genericDashboardFixture); + document.layout.serviceGroups.push("secondary-services"); + document.serviceGroups.push({ + id: "secondary-services", + title: "Secondary Services", + layout: "list", + services: [ + { + ...document.serviceGroups[0].services[0], + label: "Shadow Identity", + }, + ], + }); + document.layout.statusStrips.push("secondary-runtime"); + document.statusStrips.push({ + id: "secondary-runtime", + items: [ + { + ...document.statusStrips[0].items[0], + value: "Healthy", + }, + ], + }); + return document; +} + +function applyJsonPatch(value: T, patch: JsonPatchOperation[]): T { + const next = structuredClone(value); + for (const operation of patch) { + const { parent, key } = jsonPointerTarget(next, operation.path); + if (operation.op === "remove") { + if (Array.isArray(parent)) { + parent.splice(Number(key), 1); + } else { + delete parent[key]; + } + } else if (operation.op === "add") { + if (Array.isArray(parent)) { + parent.splice(Number(key), 0, operation.value); + } else { + parent[key] = operation.value; + } + } else if (operation.op === "replace") { + if (Array.isArray(parent)) { + parent[Number(key)] = operation.value; + } else { + parent[key] = operation.value; + } + } + } + return next; +} + +function jsonPointerTarget(value: unknown, path: string) { + const segments = path + .split("/") + .slice(1) + .map((segment) => segment.replace(/~1/g, "/").replace(/~0/g, "~")); + const key = segments.pop(); + if (key === undefined) throw new Error(`Invalid JSON pointer: ${path}`); + + let parent = value as Record | unknown[]; + for (const segment of segments) { + parent = Array.isArray(parent) + ? (parent[Number(segment)] as Record | unknown[]) + : (parent[segment] as Record | unknown[]); + } + return { parent, key }; +} + function jsonRequest(body: unknown) { return new Request("https://dimensionlab.test/api/agent/dashboard", { body: JSON.stringify(body), diff --git a/src/lib/server/agent-config/index.ts b/src/lib/server/agent-config/index.ts index c42de7d..ffa7dd8 100644 --- a/src/lib/server/agent-config/index.ts +++ b/src/lib/server/agent-config/index.ts @@ -746,16 +746,12 @@ function connectDatasource( ); } - const target = resolveTarget(document, operation.target); - if (!target) { - return failure( - "item_not_found", - `Target not found: ${operation.target.id}`, - operationIndex, - targetPath(operation.target), - ); + const targetResolution = resolveTarget(document, operation.target, operationIndex); + if (!targetResolution.ok) { + return { ok: false, errors: targetResolution.errors }; } + const target = targetResolution.target; (target as DatasourceAgentTarget).datasource = operation.datasource; return { ok: true, document }; } @@ -798,16 +794,12 @@ function setStatusRule( ); } - const target = resolveTarget(document, operation.target); - if (!target) { - return failure( - "item_not_found", - `Target not found: ${operation.target.id}`, - operationIndex, - targetPath(operation.target), - ); + const targetResolution = resolveTarget(document, operation.target, operationIndex); + if (!targetResolution.ok) { + return { ok: false, errors: targetResolution.errors }; } + const target = targetResolution.target; if (operation.thresholds && operation.target.kind === "telemetry") { (target as TelemetryCard).thresholds = operation.thresholds; } @@ -881,8 +873,8 @@ function removeItem( } if (operation.target.kind === "service") { - const location = findService(document, operation.target.id, operation.target.groupId); - if (!location) { + const matches = findServices(document, operation.target.id, operation.target.groupId); + if (!matches.length) { return failure( "item_not_found", `Service not found: ${operation.target.id}`, @@ -890,7 +882,17 @@ function removeItem( "/serviceGroups", ); } - location.group.services = location.group.services.filter((service) => service.id !== operation.target.id); + if (!operation.target.groupId && matches.length > 1) { + return failure( + "ambiguous_target", + `Service exists in multiple groups: ${operation.target.id}`, + operationIndex, + "/serviceGroups", + ); + } + + const match = matches[0]; + match.group.services = match.group.services.filter((service) => service.id !== operation.target.id); return { ok: true, document }; } @@ -938,25 +940,83 @@ function removeItem( function resolveTarget( document: DashboardDocument, target: ConnectDatasourceOperation["target"], -): ResolvedAgentTarget | null { + operationIndex: number, +): ResolveTargetResult { if (target.kind === "telemetry") { - return document.telemetry.find((card) => card.id === target.id) || null; + const card = document.telemetry.find((item) => item.id === target.id); + if (!card) { + return resolutionFailure( + "item_not_found", + `Telemetry card not found: ${target.id}`, + operationIndex, + "/telemetry", + ); + } + return { ok: true, target: card }; } if (target.kind === "service") { - return findService(document, target.id, target.groupId)?.service || null; + const matches = findServices(document, target.id, target.groupId); + if (!matches.length) { + return resolutionFailure( + "item_not_found", + `Service not found: ${target.id}`, + operationIndex, + "/serviceGroups", + ); + } + if (!target.groupId && matches.length > 1) { + return resolutionFailure( + "ambiguous_target", + `Service exists in multiple groups: ${target.id}`, + operationIndex, + "/serviceGroups", + ); + } + return { ok: true, target: matches[0].service }; } if (target.kind === "statusItem") { - const strips = target.stripId - ? document.statusStrips.filter((strip) => strip.id === target.stripId) - : document.statusStrips; - return strips.flatMap((strip) => strip.items).find((item) => item.id === target.id) || null; + const matches = findStatusItems(document, target.id, target.stripId); + if (!matches.length) { + return resolutionFailure( + "item_not_found", + `Status item not found: ${target.id}`, + operationIndex, + "/statusStrips", + ); + } + if (!target.stripId && matches.length > 1) { + return resolutionFailure( + "ambiguous_target", + `Status item exists in multiple strips: ${target.id}`, + operationIndex, + "/statusStrips", + ); + } + return { ok: true, target: matches[0].item }; } if (target.kind === "module") { - return (document.modules || []).find((module) => module.id === target.id) || null; + const module = (document.modules || []).find((item) => item.id === target.id); + if (!module) { + return resolutionFailure( + "item_not_found", + `Module not found: ${target.id}`, + operationIndex, + "/modules", + ); + } + return { ok: true, target: module }; } - return null; + return resolutionFailure( + "item_not_found", + `Target not found: ${target.id}`, + operationIndex, + targetPath(target), + ); } +type ResolveTargetResult = + | { ok: true; target: ResolvedAgentTarget } + | { ok: false; errors: AgentConfigError[] }; type ResolvedAgentTarget = TelemetryCard | ServiceEntry | StatusItem | DashboardModule; type DatasourceAgentTarget = TelemetryCard | ServiceEntry | DashboardModule; type DetailAgentTarget = TelemetryCard | ServiceEntry | DashboardModule; @@ -995,15 +1055,15 @@ function supportsStringValue( return kind === "statusItem" || kind === "module"; } -function findService(document: DashboardDocument, id: string, groupId?: string) { +function findServices(document: DashboardDocument, id: string, groupId?: string) { const groups = groupId ? document.serviceGroups.filter((group) => group.id === groupId) : document.serviceGroups; - for (const group of groups) { - const service = group.services.find((item) => item.id === id); - if (service) return { group, service }; - } - return null; + return groups.flatMap((group) => + group.services.flatMap((service) => + service.id === id ? [{ group, service }] : [], + ), + ); } function findStatusItems(document: DashboardDocument, id: string, stripId?: string) { @@ -1011,10 +1071,29 @@ function findStatusItems(document: DashboardDocument, id: string, stripId?: stri ? document.statusStrips.filter((strip) => strip.id === stripId) : document.statusStrips; return strips.flatMap((strip) => - strip.items.some((item) => item.id === id) ? [{ strip }] : [], + strip.items.flatMap((item) => (item.id === id ? [{ strip, item }] : [])), ); } +function resolutionFailure( + code: string, + message: string, + operationIndex: number, + path: string, +): ResolveTargetResult { + return { + ok: false, + errors: [ + { + code, + message, + operationIndex, + path, + }, + ], + }; +} + function targetPath(target: ConnectDatasourceOperation["target"]): string { if (target.kind === "telemetry") return "/telemetry"; if (target.kind === "service") return "/serviceGroups"; @@ -1140,12 +1219,18 @@ function createJsonPatch( return [{ op: "replace", path: path || "/", value: after }]; } const operations: JsonPatchOperation[] = []; - const max = Math.max(before.length, after.length); - for (let index = 0; index < max; index += 1) { + const sharedLength = Math.min(before.length, after.length); + for (let index = 0; index < sharedLength; index += 1) { operations.push( ...createJsonPatch(before[index], after[index], `${path}/${index}`), ); } + for (let index = before.length - 1; index >= after.length; index -= 1) { + operations.push({ op: "remove", path: `${path}/${index}` }); + } + for (let index = before.length; index < after.length; index += 1) { + operations.push({ op: "add", path: `${path}/${index}`, value: after[index] }); + } return operations; }