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 b08852a962..9dded3dbfd 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,55 @@ 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; +} + +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) { + return moment.versions().next().value; + } + } + 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 @@ -199,7 +254,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 +270,23 @@ 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); - } + const wasAddedFirst = resolveAddedFirst(added, removed); // 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]); - } + // v1 if not specified, or inherited from their parent. + 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) { + // 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 @@ -264,7 +316,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); @@ -279,10 +332,24 @@ 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 - if (!added.length) { - added.push(timeline.first().versions().next().value); + // v1 if not specified, or inherited from their parent. + 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(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); + } 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..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], @@ -290,20 +290,51 @@ 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( + [ + [v2, "v2"], + [v3, "v3"], + [v4, "v4"], + ], + source + ); + }); + + 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"], - [v3, "v4"], ], source ); @@ -1403,12 +1434,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"],