From 93cc51e48d9764a98f17d7814bc3b3868d7a063c Mon Sep 17 00:00:00 2001 From: Travis Prescott Date: Tue, 30 Apr 2024 09:26:37 -0700 Subject: [PATCH 1/4] =?UTF-8?q?=EF=BB=BFFixes=20#3239.?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- packages/versioning/src/versioning.ts | 63 +++++++++++++-------- packages/versioning/test/versioning.test.ts | 13 +++-- 2 files changed, 48 insertions(+), 28 deletions(-) diff --git a/packages/versioning/src/versioning.ts b/packages/versioning/src/versioning.ts index b08852a962..bde61dead3 100644 --- a/packages/versioning/src/versioning.ts +++ b/packages/versioning/src/versioning.ts @@ -1,5 +1,11 @@ -import type { Enum, EnumMember, Namespace, Program, Type } from "@typespec/compiler"; -import { compilerAssert, getNamespaceFullName } from "@typespec/compiler"; +import { + getNamespaceFullName, + type Enum, + type EnumMember, + type Namespace, + type Program, + type Type, +} from "@typespec/compiler"; import { getAddedOnVersions, getRemovedOnVersions, @@ -189,6 +195,26 @@ export enum Availability { Removed = "Removed", } +function getParentAddedVersion( + program: Program, + type: Type, + versions: Version[] +): Version | undefined { + let parentMap: Map | undefined = undefined; + if (type.kind === "ModelProperty" && type.model !== undefined) { + parentMap = getAvailabilityMap(program, type.model); + } else if (type.kind === "Operation" && type.interface !== undefined) { + parentMap = getAvailabilityMap(program, type.interface); + } + if (parentMap === undefined) return undefined; + for (const [key, value] of parentMap.entries()) { + if (value === Availability.Added) { + return versions.find((x) => x.name === key); + } + } + return undefined; +} + export function getAvailabilityMap( program: Program, type: Type @@ -199,7 +225,8 @@ export function getAvailabilityMap( // if unversioned then everything exists if (allVersions === undefined) return undefined; - const added = getAddedOnVersions(program, type) ?? []; + const parentAdded = getParentAddedVersion(program, type, allVersions); + let added = getAddedOnVersions(program, type) ?? []; const removed = getRemovedOnVersions(program, type) ?? []; const typeChanged = getTypeChangedFrom(program, type); const returnTypeChanged = getReturnTypeChangedFrom(program, type); @@ -214,27 +241,15 @@ export function getAvailabilityMap( ) return undefined; - let parentMap: Map | undefined = undefined; - if (type.kind === "ModelProperty" && type.model !== undefined) { - parentMap = getAvailabilityMap(program, type.model); - } else if (type.kind === "Operation" && type.interface !== undefined) { - parentMap = getAvailabilityMap(program, type.interface); - } - - // implicitly, all versioned things are assumed to have been added at - // v1 if not specified - if (!added.length) { - if (parentMap !== undefined) { - parentMap.forEach((key, value) => { - if (key === Availability.Added.valueOf()) { - const match = allVersions.find((x) => x.name === value); - compilerAssert(match !== undefined, "Version not found"); - added.push(match); - } - }); - } else { - added.push(allVersions[0]); - } + if (!added.length && !parentAdded) { + // no version information on the item or its parent implicitly means it has always been available + added.push(allVersions[0]); + } else if (!added.length && parentAdded) { + // if no version info on type but is on parent, inherit that parent's "added" version + added.push(parentAdded); + } else if (added.length && parentAdded) { + // if "added" info on both the type and parent, combine them + added = [parentAdded, ...added]; } // something isn't available by default diff --git a/packages/versioning/test/versioning.test.ts b/packages/versioning/test/versioning.test.ts index 78ba856e2a..6556d377c1 100644 --- a/packages/versioning/test/versioning.test.ts +++ b/packages/versioning/test/versioning.test.ts @@ -290,14 +290,16 @@ describe("versioning: logic", () => { `@added(Versions.v2) model Test { a: int32; - @removed(Versions.v3) b: int32; + @removed(Versions.v3) + @added(Versions.v4) + b: int32; } ` ); assertHasProperties(v2, ["a", "b"]); assertHasProperties(v3, ["a"]); - assertHasProperties(v4, ["a"]); + assertHasProperties(v4, ["a", "b"]); assertModelProjectsTo( [ @@ -1403,12 +1405,15 @@ describe("versioning: logic", () => { `@added(Versions.v2) interface Test { allVersions(): void; - @removed(Versions.v3) version2Only(): void; + @removed(Versions.v3) + @added(Versions.v4) + foo(): void; } ` ); - assertHasOperations(v2, ["allVersions", "version2Only"]); + assertHasOperations(v2, ["allVersions", "foo"]); assertHasOperations(v3, ["allVersions"]); + assertHasOperations(v4, ["allVersions", "foo"]); assertInterfaceProjectsTo( [ [v2, "v2"], From 0abf16749d16d22ac282d9922ef5add288867a62 Mon Sep 17 00:00:00 2001 From: Travis Prescott Date: Wed, 22 May 2024 10:58:54 -0700 Subject: [PATCH 2/4] Create timeline version of other fix. --- packages/versioning/src/versioning.ts | 38 +++++++++++++++++++-- packages/versioning/test/versioning.test.ts | 2 +- 2 files changed, 36 insertions(+), 4 deletions(-) diff --git a/packages/versioning/src/versioning.ts b/packages/versioning/src/versioning.ts index bde61dead3..85965bc4b1 100644 --- a/packages/versioning/src/versioning.ts +++ b/packages/versioning/src/versioning.ts @@ -215,6 +215,28 @@ function getParentAddedVersion( return undefined; } +function getParentAddedVersionInTimeline( + program: Program, + type: Type, + timeline: VersioningTimeline +): Version | undefined { + let parentMap: Map | undefined = undefined; + if (type.kind === "ModelProperty" && type.model !== undefined) { + parentMap = getAvailabilityMapInTimeline(program, type.model, timeline); + } else if (type.kind === "Operation" && type.interface !== undefined) { + parentMap = getAvailabilityMapInTimeline(program, type.interface, timeline); + } + if (parentMap === undefined) return undefined; + for (const [moment, availability] of parentMap.entries()) { + if (availability === Availability.Added) { + // FIXME: is this actually correct? + // We essentially want to return the first (earliest) time this is added. + return moment.versions().next().value; + } + } + return undefined; +} + export function getAvailabilityMap( program: Program, type: Type @@ -241,6 +263,8 @@ export function getAvailabilityMap( ) return undefined; + // implicitly, all versioned things are assumed to have been added at + // v1 if not specified, or inherited from their parent. if (!added.length && !parentAdded) { // no version information on the item or its parent implicitly means it has always been available added.push(allVersions[0]); @@ -279,7 +303,8 @@ export function getAvailabilityMapInTimeline( ): Map | undefined { const avail = new Map(); - const added = getAddedOnVersions(program, type) ?? []; + const parentAdded = getParentAddedVersionInTimeline(program, type, timeline); + let added = getAddedOnVersions(program, type) ?? []; const removed = getRemovedOnVersions(program, type) ?? []; const typeChanged = getTypeChangedFrom(program, type); const returnTypeChanged = getReturnTypeChangedFrom(program, type); @@ -295,9 +320,16 @@ export function getAvailabilityMapInTimeline( return undefined; // implicitly, all versioned things are assumed to have been added at - // v1 if not specified - if (!added.length) { + // v1 if not specified, or inherited from their parent. + if (!added.length && !parentAdded) { + // no version information on the item or its parent implicitly means it has always been available added.push(timeline.first().versions().next().value); + } else if (!added.length && parentAdded) { + // if no version info on type but is on parent, inherit that parent's "added" version + added.push(parentAdded); + } else if (added.length && parentAdded) { + // if "added" info on both the type and parent, combine them + added = [parentAdded, ...added]; } // something isn't available by default diff --git a/packages/versioning/test/versioning.test.ts b/packages/versioning/test/versioning.test.ts index 6556d377c1..17cefbe3eb 100644 --- a/packages/versioning/test/versioning.test.ts +++ b/packages/versioning/test/versioning.test.ts @@ -305,7 +305,7 @@ describe("versioning: logic", () => { [ [v2, "v2"], [v3, "v3"], - [v3, "v4"], + [v4, "v4"], ], source ); From 40a77ef43dee0b7493faf35577c7640f047b825c Mon Sep 17 00:00:00 2001 From: Travis Prescott Date: Wed, 22 May 2024 13:42:46 -0700 Subject: [PATCH 3/4] Create versioning-FixVersioningBugs-2024-4-22-20-42-27.md --- .../versioning-FixVersioningBugs-2024-4-22-20-42-27.md | 8 ++++++++ packages/versioning/src/versioning.ts | 2 -- 2 files changed, 8 insertions(+), 2 deletions(-) create mode 100644 .chronus/changes/versioning-FixVersioningBugs-2024-4-22-20-42-27.md diff --git a/.chronus/changes/versioning-FixVersioningBugs-2024-4-22-20-42-27.md b/.chronus/changes/versioning-FixVersioningBugs-2024-4-22-20-42-27.md new file mode 100644 index 0000000000..bad367a8f9 --- /dev/null +++ b/.chronus/changes/versioning-FixVersioningBugs-2024-4-22-20-42-27.md @@ -0,0 +1,8 @@ +--- +# Change versionKind to one of: internal, fix, dependencies, feature, deprecation, breaking +changeKind: fix +packages: + - "@typespec/versioning" +--- + +If a property were marked with @added on a later version, the logic that said it was originally added on the first version was erroneously removed, resulting in incorrect projections. diff --git a/packages/versioning/src/versioning.ts b/packages/versioning/src/versioning.ts index 85965bc4b1..78755ff279 100644 --- a/packages/versioning/src/versioning.ts +++ b/packages/versioning/src/versioning.ts @@ -229,8 +229,6 @@ function getParentAddedVersionInTimeline( if (parentMap === undefined) return undefined; for (const [moment, availability] of parentMap.entries()) { if (availability === Availability.Added) { - // FIXME: is this actually correct? - // We essentially want to return the first (earliest) time this is added. return moment.versions().next().value; } } From 82f53764ce5d38b74ca71ac7a9702abca749d900 Mon Sep 17 00:00:00 2001 From: Travis Prescott Date: Tue, 4 Jun 2024 11:36:49 -0700 Subject: [PATCH 4/4] Add test case. --- packages/versioning/src/versioning.ts | 28 +++++++++++++++++-- packages/versioning/test/versioning.test.ts | 31 ++++++++++++++++++++- 2 files changed, 55 insertions(+), 4 deletions(-) diff --git a/packages/versioning/src/versioning.ts b/packages/versioning/src/versioning.ts index 78755ff279..9dded3dbfd 100644 --- a/packages/versioning/src/versioning.ts +++ b/packages/versioning/src/versioning.ts @@ -235,6 +235,15 @@ function getParentAddedVersionInTimeline( return undefined; } +/** + * Returns true if the first version modifier was @added, false if @removed. + */ +function resolveAddedFirst(added: Version[], removed: Version[]): boolean { + if (added.length === 0) return false; + if (removed.length === 0) return true; + return added[0].index < removed[0].index; +} + export function getAvailabilityMap( program: Program, type: Type @@ -261,9 +270,15 @@ export function getAvailabilityMap( ) return undefined; + const wasAddedFirst = resolveAddedFirst(added, removed); + // implicitly, all versioned things are assumed to have been added at // v1 if not specified, or inherited from their parent. - if (!added.length && !parentAdded) { + if (!wasAddedFirst && !parentAdded) { + // if the first version modifier was @removed, and the parent is implicitly available, + // then assume the type was available. + added = [allVersions[0], ...added]; + } else if (!added.length && !parentAdded) { // no version information on the item or its parent implicitly means it has always been available added.push(allVersions[0]); } else if (!added.length && parentAdded) { @@ -317,11 +332,18 @@ export function getAvailabilityMapInTimeline( ) return undefined; + const wasAddedFirst = resolveAddedFirst(added, removed); + // implicitly, all versioned things are assumed to have been added at // v1 if not specified, or inherited from their parent. - if (!added.length && !parentAdded) { + const firstVersion = timeline.first().versions().next().value; + if (!wasAddedFirst && !parentAdded) { + // if the first version modifier was @removed, and the parent is implicitly available, + // then assume the type was available. + added = [firstVersion, ...added]; + } else if (!added.length && !parentAdded) { // no version information on the item or its parent implicitly means it has always been available - added.push(timeline.first().versions().next().value); + added.push(firstVersion); } else if (!added.length && parentAdded) { // if no version info on type but is on parent, inherit that parent's "added" version added.push(parentAdded); diff --git a/packages/versioning/test/versioning.test.ts b/packages/versioning/test/versioning.test.ts index 17cefbe3eb..e2635e891f 100644 --- a/packages/versioning/test/versioning.test.ts +++ b/packages/versioning/test/versioning.test.ts @@ -281,7 +281,7 @@ describe("versioning: logic", () => { ); }); - it("can be removed respecting model versioning", async () => { + it("can be removed respecting model versioning with explicit versions", async () => { const { source, projections: [v2, v3, v4], @@ -311,6 +311,35 @@ describe("versioning: logic", () => { ); }); + it("can be removed respecting model versioning with implicit versions", async () => { + const { + source, + projections: [v1, v2, v3], + } = await versionedModel( + ["v1", "v2", "v3"], + `model Test { + a: int32; + @removed(Versions.v2) + @added(Versions.v3) + b: int32; + } + ` + ); + + assertHasProperties(v1, ["a", "b"]); + assertHasProperties(v2, ["a"]); + assertHasProperties(v3, ["a", "b"]); + + assertModelProjectsTo( + [ + [v1, "v1"], + [v2, "v2"], + [v3, "v3"], + ], + source + ); + }); + it("can be renamed", async () => { const { source,