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 da1948db47..1850b804ff 100644 --- a/packages/typespec-breaking-change/src/diff/diff-types.ts +++ b/packages/typespec-breaking-change/src/diff/diff-types.ts @@ -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": 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..7e77e3f52b --- /dev/null +++ b/packages/typespec-breaking-change/test/record-diffs.test.ts @@ -0,0 +1,55 @@ +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", "Array", "ResourcePropertyTypeChanged"], + ["Array", "Record", "ResourcePropertyTypeChanged"], + ["Array", "Array", undefined], + ["Array", "string[]", undefined], + ["string[]", "Array", undefined], + ["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" } }, + }); + }); +});