From a9e10d9b8b2331146fb73f6084540ba94f4be751 Mon Sep 17 00:00:00 2001 From: catalinaperalta <9859037+catalinaperalta@users.noreply.github.com> Date: Fri, 16 Oct 2026 17:55:38 -0700 Subject: [PATCH] fix: compare record values and normalize HTTP paths safely Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: eddbfa67-cd77-4acf-99f6-8fd275d6c7d6 --- ...-change-records-paths-2026-9-16-17-53-7.md | 7 +++ .../src/diff/diff-types.ts | 61 ++++++++++++++++--- .../src/diff/operation-identity.ts | 13 +++- .../test/operation-identity.test.ts | 15 ++--- .../test/record-diffs.test.ts | 50 +++++++++++++++ 5 files changed, 130 insertions(+), 16 deletions(-) create mode 100644 .chronus/changes/fix-breaking-change-records-paths-2026-9-16-17-53-7.md create mode 100644 packages/typespec-breaking-change/test/record-diffs.test.ts diff --git a/.chronus/changes/fix-breaking-change-records-paths-2026-9-16-17-53-7.md b/.chronus/changes/fix-breaking-change-records-paths-2026-9-16-17-53-7.md new file mode 100644 index 0000000000..797f0ebc04 --- /dev/null +++ b/.chronus/changes/fix-breaking-change-records-paths-2026-9-16-17-53-7.md @@ -0,0 +1,7 @@ +--- +changeKind: fix +packages: + - "@azure-tools/typespec-breaking-change" +--- + +Detect record value and indexed model shape changes, preserve approval targets, and normalize operation paths in linear time. \ No newline at end of file diff --git a/packages/typespec-breaking-change/src/diff/diff-types.ts b/packages/typespec-breaking-change/src/diff/diff-types.ts index f9ffdd8f59..948bcf3884 100644 --- a/packages/typespec-breaking-change/src/diff/diff-types.ts +++ b/packages/typespec-breaking-change/src/diff/diff-types.ts @@ -12,8 +12,8 @@ import { } from "@typespec/compiler"; import type { HttpCanonicalization } from "@typespec/http-canonicalization"; import type { DiffKind } from "../diff-kind.js"; -import { resolveOrigin } from "./origin.js"; import type { ApiDiff, DiffComponent, OperationDiffIdentity, OperationIdentity } from "../types.js"; +import { resolveOrigin } from "./origin.js"; export interface DiffContext { operation: OperationIdentity; @@ -76,6 +76,21 @@ export function compareTypes(baseType: Type, headType: Type, ctx: DiffContext): } function compareModels(base: Model, head: Model, ctx: DiffContext): ApiDiff[] { + if ( + Boolean(base.indexer) !== Boolean(head.indexer) || + isArrayModelType(base) !== isArrayModelType(head) + ) { + return [ + makeDiff( + typeKindChangedKind(ctx), + ctx, + ctx.elementPath, + base, + head, + `${componentLabel(ctx.component)} indexed model shape changed at ${ctx.elementPath}.`, + ), + ]; + } if (isArrayModelType(base) && isArrayModelType(head)) { // Array item type changes — the violation is about the array model, not the inner item type const diffs = compareTypes(base.indexer.value, head.indexer.value, ctx); @@ -92,6 +107,21 @@ function compareModels(base: Model, head: Model, ctx: DiffContext): ApiDiff[] { const diffs: ApiDiff[] = []; + if (base.indexer && head.indexer) { + diffs.push(...compareTypes(base.indexer.key, head.indexer.key, ctx)); + const valueDiffs = compareTypes(base.indexer.value, head.indexer.value, ctx); + for (const diff of valueDiffs) { + if (diff.headType === head.indexer.value || diff.baseType === base.indexer.value) { + diff.baseType = base; + diff.headType = head; + diff.baseSourceLocation = getTypeSourceLocation(base); + diff.headSourceLocation = getTypeSourceLocation(head); + diff.origin = resolveOrigin(head); + } + } + diffs.push(...valueDiffs); + } + for (const [name, baseProp] of base.properties) { const headProp = head.properties.get(name); const elementPath = propertyElementPath(ctx.elementPath, name); @@ -405,6 +435,8 @@ function getTypeKey(type: Type): string { } function getNamedTypeKey(type: Type): string | undefined { + // Different instantiations of Record and Array share their declared name. + if (type.kind === "Model" && type.indexer) return undefined; switch (type.kind) { case "Model": case "Scalar": @@ -415,7 +447,9 @@ function getNamedTypeKey(type: Type): string | undefined { ? `${getTypeKey(type.model)}.properties.${type.name}` : `${type.kind}:${type.name}`; case "Union": - return type.name ? qualifiedName(type.namespace, type.name) ?? `${type.kind}:${type.name}` : undefined; + return type.name + ? (qualifiedName(type.namespace, type.name) ?? `${type.kind}:${type.name}`) + : undefined; case "UnionVariant": return typeof type.name === "string" ? `${getTypeKey(type.union)}.variants.${type.name}` @@ -449,7 +483,9 @@ function propertyMadeOptionalKind(component: DiffComponent): DiffKind { function typeChangedKind(ctx: DiffContext): DiffKind { if (isPropertyElement(ctx.elementPath)) { - return ctx.component === "request" ? "RequestPropertyTypeChanged" : "ResponsePropertyTypeChanged"; + return ctx.component === "request" + ? "RequestPropertyTypeChanged" + : "ResponsePropertyTypeChanged"; } return ctx.component === "request" ? "RequestTypeChanged" : "ResponseTypeChanged"; @@ -457,7 +493,9 @@ function typeChangedKind(ctx: DiffContext): DiffKind { function typeNarrowedKind(ctx: DiffContext): DiffKind { if (isPropertyElement(ctx.elementPath)) { - return ctx.component === "request" ? "RequestPropertyTypeNarrowed" : "ResponsePropertyTypeNarrowed"; + return ctx.component === "request" + ? "RequestPropertyTypeNarrowed" + : "ResponsePropertyTypeNarrowed"; } return ctx.component === "request" ? "RequestTypeNarrowed" : "ResponseTypeNarrowed"; @@ -465,7 +503,9 @@ function typeNarrowedKind(ctx: DiffContext): DiffKind { function typeWidenedKind(ctx: DiffContext): DiffKind { if (isPropertyElement(ctx.elementPath)) { - return ctx.component === "request" ? "RequestPropertyTypeWidened" : "ResponsePropertyTypeWidened"; + return ctx.component === "request" + ? "RequestPropertyTypeWidened" + : "ResponsePropertyTypeWidened"; } return ctx.component === "request" ? "RequestTypeWidened" : "ResponseTypeWidened"; @@ -473,7 +513,9 @@ function typeWidenedKind(ctx: DiffContext): DiffKind { function typeKindChangedKind(ctx: DiffContext): DiffKind { if (isPropertyElement(ctx.elementPath)) { - return ctx.component === "request" ? "RequestPropertyTypeChanged" : "ResponsePropertyTypeChanged"; + return ctx.component === "request" + ? "RequestPropertyTypeChanged" + : "ResponsePropertyTypeChanged"; } return ctx.component === "request" ? "RequestTypeKindChanged" : "ResponseTypeKindChanged"; @@ -526,7 +568,8 @@ function qualifiedName( if (current.name) { namespaceParts.unshift(current.name); } - current = current.namespace as { name: string; namespace?: { name: string; namespace?: unknown } } | undefined; + current = current.namespace as + { name: string; namespace?: { name: string; namespace?: unknown } } | undefined; } namespaceParts.push(name); @@ -553,7 +596,9 @@ function describeType(type: Type): string { case "Enum": return qualifiedName(type.namespace, type.name) ?? type.name; case "Union": - return type.name ? qualifiedName(type.namespace, type.name) ?? type.name : "anonymous-union"; + return type.name + ? (qualifiedName(type.namespace, type.name) ?? type.name) + : "anonymous-union"; case "String": return JSON.stringify(type.value); case "Number": diff --git a/packages/typespec-breaking-change/src/diff/operation-identity.ts b/packages/typespec-breaking-change/src/diff/operation-identity.ts index e014063189..04460b3436 100644 --- a/packages/typespec-breaking-change/src/diff/operation-identity.ts +++ b/packages/typespec-breaking-change/src/diff/operation-identity.ts @@ -28,7 +28,18 @@ export interface ResolvedOperation { * "/subscriptions/{subscriptionId}/providers/{rpName}" → "/subscriptions/{}/providers/{}" */ export function normalizePath(path: string): string { - return path.replace(/\{[^}]+\}/g, "{}"); + const parts: string[] = []; + let position = 0; + while (position < path.length) { + const open = path.indexOf("{", position); + if (open === -1) break; + const close = path.indexOf("}", open + 1); + if (close === -1) break; + parts.push(path.slice(position, open), "{}"); + position = close + 1; + } + parts.push(path.slice(position)); + return parts.join(""); } /** diff --git a/packages/typespec-breaking-change/test/operation-identity.test.ts b/packages/typespec-breaking-change/test/operation-identity.test.ts index 7968388283..35bb9f732f 100644 --- a/packages/typespec-breaking-change/test/operation-identity.test.ts +++ b/packages/typespec-breaking-change/test/operation-identity.test.ts @@ -1,15 +1,16 @@ import { describe, expect, it } from "vitest"; -import { - resolveOperationIdentities, - normalizePath, - getOperationIdentity, - identityKey, -} from "../src/diff/operation-identity.js"; +import { normalizePath, resolveOperationIdentities } from "../src/diff/operation-identity.js"; +import { createVersionedView, enumerateVersions } from "../src/pipeline/versions.js"; import { matchOperations } from "../src/suppression/match.js"; -import { enumerateVersions, createVersionedView } from "../src/pipeline/versions.js"; import { Tester } from "./test-host.js"; describe("normalizePath", () => { + it("preserves embedded parameters and malformed paths without backtracking", () => { + expect(normalizePath("/widgets/prefix-{id}.{format}/{}")).toBe("/widgets/prefix-{}.{}/{}"); + expect(normalizePath("/widgets/{{id}")).toBe("/widgets/{}"); + const unmatched = "/widgets/" + "{".repeat(100_000); + expect(normalizePath(unmatched)).toBe(unmatched); + }); it("replaces parameter names with {}", () => { expect(normalizePath("/widgets/{widgetId}")).toBe("/widgets/{}"); }); diff --git a/packages/typespec-breaking-change/test/record-diffs.test.ts b/packages/typespec-breaking-change/test/record-diffs.test.ts new file mode 100644 index 0000000000..39e19be8d8 --- /dev/null +++ b/packages/typespec-breaking-change/test/record-diffs.test.ts @@ -0,0 +1,50 @@ +import { describe, expect, it } from "vitest"; +import { analyzeProgram } from "../src/pipeline/orchestrator.js"; +import { TesterWithSuppressions } from "./test-host.js"; + +describe("record comparisons", () => { + it.each([ + ["Record", "Record", "ResourcePropertyTypeChanged"], + ["Record", "Record", undefined], + ["Record", "string[]", "ResourcePropertyTypeChanged"], + ["Record", "{}", "ResourcePropertyTypeChanged"], + ])("compares %s to %s", async (base, head, expectedKind) => { + const { program } = await TesterWithSuppressions.compile(` + @service @versioned(Versions) namespace Test; + enum Versions { v1: "2024-01-01", v2: "2025-01-01" } + model Widget { + @typeChangedFrom(Versions.v2, ${base}) + tags: ${head}; + } + @route("/widgets") @post op create(@body widget: Widget): Widget; + `); + const result = analyzeProgram(program); + expect(result.findings.map((finding) => finding.diff.kind)).toEqual( + expectedKind ? [expectedKind] : [], + ); + if (expectedKind) { + expect(result.findings[0].diff.identity.element).toBe("body.properties.tags"); + expect(result.findings[0].diff.origin?.declarationPath).toBe("Test.Widget.tags"); + } + }); + + it("compares nested record values and supports approval on the owning property", async () => { + const { program } = await TesterWithSuppressions.compile(` + @service @versioned(Versions) namespace Test; + enum Versions { v1: "2024-01-01", v2: "2025-01-01" } + model Widget { + @approvedBreakingChange("Reviewed.", #{ kind: "ResourcePropertyTypeChanged" }) + @typeChangedFrom(Versions.v2, Record>) + tags: Record>; + } + @route("/widgets") @post op create(@body widget: Widget): Widget; + `); + const result = analyzeProgram(program); + expect(result.findings).toHaveLength(1); + expect(result.findings[0]).toMatchObject({ + suppressed: true, + suppressionReason: "Reviewed.", + diff: { kind: "ResourcePropertyTypeChanged", identity: { element: "body.properties.tags" } }, + }); + }); +});