Fix versioning issue when @added is used implicitly and explicitly

.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.

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<string, Availability> | 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<TimelineMoment, Availability> | 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<string, Availability> | 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<TimelineMoment, Availability> | undefined {
  const avail = new Map<TimelineMoment, Availability>();
 
- 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

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"],