From 3e6a374bac27697d193e0b15c52a5cd08e2d485e Mon Sep 17 00:00:00 2001 From: Travis Prescott Date: Thu, 9 Nov 2023 16:30:52 -0800 Subject: [PATCH 1/2] Add test scenario. --- .../test/incompatible-versioning.test.ts | 34 +++++++++++++++++++ 1 file changed, 34 insertions(+) diff --git a/packages/versioning/test/incompatible-versioning.test.ts b/packages/versioning/test/incompatible-versioning.test.ts index 578a16b0c7..7db4f4dd8b 100644 --- a/packages/versioning/test/incompatible-versioning.test.ts +++ b/packages/versioning/test/incompatible-versioning.test.ts @@ -102,6 +102,40 @@ describe("versioning: validate incompatible references", () => { }); }); + describe("operation", () => { + it("emit diagnostic when unversioned op has a versioned model as a parameter", async () => { + const diagnostics = await runner.diagnose(` + @added(Versions.v2) + model Foo {} + + op test(param: Foo): void; + `); + expectDiagnostics(diagnostics, { + code: "@typespec/versioning/incompatible-versioned-reference", + message: + "'TestService.test' is referencing versioned type 'TestService.Foo' but is not versioned itself.", + }); + }); + + it("emit diagnostic when unversioned op based on a template has a versioned model as a parameter", async () => { + const diagnostics = await runner.diagnose(` + @added(Versions.v2) + model Foo {} + + model Template { + op test(param: T): void; + } + + op test is Template; + `); + expectDiagnostics(diagnostics, { + code: "@typespec/versioning/incompatible-versioned-reference", + message: + "'TestService.test' is referencing versioned type 'TestService.Foo' but is not versioned itself.", + }); + }); + }); + describe("operation return type", () => { it("emit diagnostic when unversioned op is returning versioned model", async () => { const diagnostics = await runner.diagnose(` From a0b9c13ad743d4315f30e5f1b372632882cee2d3 Mon Sep 17 00:00:00 2001 From: Travis Prescott Date: Fri, 10 Nov 2023 08:17:22 -0800 Subject: [PATCH 2/2] Fix issue. --- .../versioning-OpParams_2023-11-10-16-19.json | 10 +++ packages/versioning/src/lib.ts | 8 +-- packages/versioning/src/validate.ts | 5 +- .../test/incompatible-versioning.test.ts | 62 ++++++++++++++----- 4 files changed, 63 insertions(+), 22 deletions(-) create mode 100644 common/changes/@typespec/versioning/versioning-OpParams_2023-11-10-16-19.json diff --git a/common/changes/@typespec/versioning/versioning-OpParams_2023-11-10-16-19.json b/common/changes/@typespec/versioning/versioning-OpParams_2023-11-10-16-19.json new file mode 100644 index 0000000000..885c527c79 --- /dev/null +++ b/common/changes/@typespec/versioning/versioning-OpParams_2023-11-10-16-19.json @@ -0,0 +1,10 @@ +{ + "changes": [ + { + "packageName": "@typespec/versioning", + "comment": "Fix issue where the version compatability of parameters in operation signatures was not checked.", + "type": "none" + } + ], + "packageName": "@typespec/versioning" +} \ No newline at end of file diff --git a/packages/versioning/src/lib.ts b/packages/versioning/src/lib.ts index 8d2a6628d1..9a4d74df89 100644 --- a/packages/versioning/src/lib.ts +++ b/packages/versioning/src/lib.ts @@ -67,10 +67,10 @@ const libDef = { severity: "error", messages: { default: paramMessage`'${"sourceName"}' is referencing versioned type '${"targetName"}' but is not versioned itself.`, - addedAfter: paramMessage`'${"sourceName"}' was added on version '${"sourceAddedOn"}' but referencing type '${"targetName"}' added in version '${"targetAddedOn"}'.`, - dependentAddedAfter: paramMessage`'${"sourceName"}' was added on version '${"sourceAddedOn"}' but contains type '${"targetName"}' added in version '${"targetAddedOn"}'.`, - removedBefore: paramMessage`'${"sourceName"}' was removed on version '${"sourceRemovedOn"}' but referencing type '${"targetName"}' removed in version '${"targetRemovedOn"}'.`, - dependentRemovedBefore: paramMessage`'${"sourceName"}' was removed on version '${"sourceRemovedOn"}' but contains type '${"targetName"}' removed in version '${"targetRemovedOn"}'.`, + addedAfter: paramMessage`'${"sourceName"}' was added in version '${"sourceAddedOn"}' but referencing type '${"targetName"}' added in version '${"targetAddedOn"}'.`, + dependentAddedAfter: paramMessage`'${"sourceName"}' was added in version '${"sourceAddedOn"}' but contains type '${"targetName"}' added in version '${"targetAddedOn"}'.`, + removedBefore: paramMessage`'${"sourceName"}' was removed in version '${"sourceRemovedOn"}' but referencing type '${"targetName"}' removed in version '${"targetRemovedOn"}'.`, + dependentRemovedBefore: paramMessage`'${"sourceName"}' was removed in version '${"sourceRemovedOn"}' but contains type '${"targetName"}' removed in version '${"targetRemovedOn"}'.`, versionedDependencyAddedAfter: paramMessage`'${"sourceName"}' is referencing type '${"targetName"}' added in version '${"targetAddedOn"}' but version used is ${"dependencyVersion"}.`, versionedDependencyRemovedBefore: paramMessage`'${"sourceName"}' is referencing type '${"targetName"}' removed in version '${"targetAddedOn"}' but version used is ${"dependencyVersion"}.`, doesNotExist: paramMessage`'${"sourceName"}' is referencing type '${"targetName"}' which does not exist in version '${"version"}'.`, diff --git a/packages/versioning/src/validate.ts b/packages/versioning/src/validate.ts index eb79e45db0..5748c727c1 100644 --- a/packages/versioning/src/validate.ts +++ b/packages/versioning/src/validate.ts @@ -93,10 +93,11 @@ export function $onValidate(program: Program) { addDependency(namespace, op.returnType); if (op.interface) { - // Validate model -> property have correct versioning validateTargetVersionCompatible(program, op.interface, op, { isTargetADependent: true }); } - + for (const prop of op.parameters.properties.values()) { + validateReference(program, op, prop.type); + } validateReference(program, op, op.returnType); }, interface: (iface) => { diff --git a/packages/versioning/test/incompatible-versioning.test.ts b/packages/versioning/test/incompatible-versioning.test.ts index 7db4f4dd8b..84a5172c7a 100644 --- a/packages/versioning/test/incompatible-versioning.test.ts +++ b/packages/versioning/test/incompatible-versioning.test.ts @@ -122,9 +122,7 @@ describe("versioning: validate incompatible references", () => { @added(Versions.v2) model Foo {} - model Template { - op test(param: T): void; - } + op Template(param: T): void; op test is Template; `); @@ -134,6 +132,38 @@ describe("versioning: validate incompatible references", () => { "'TestService.test' is referencing versioned type 'TestService.Foo' but is not versioned itself.", }); }); + + it("emit diagnostic when when op was added before parameter", async () => { + const diagnostics = await runner.diagnose(` + @added(Versions.v2) + model Foo {} + + @added(Versions.v1) + op test(param: Foo): void; + `); + expectDiagnostics(diagnostics, { + code: "@typespec/versioning/incompatible-versioned-reference", + message: + "'TestService.test' was added in version 'v1' but referencing type 'TestService.Foo' added in version 'v2'.", + }); + }); + + it("emit diagnostic when op based on a template was added before parameter", async () => { + const diagnostics = await runner.diagnose(` + @added(Versions.v2) + model Foo {} + + op Template(param: T): void; + + @added(Versions.v1) + op test is Template; + `); + expectDiagnostics(diagnostics, { + code: "@typespec/versioning/incompatible-versioned-reference", + message: + "'TestService.test' was added in version 'v1' but referencing type 'TestService.Foo' added in version 'v2'.", + }); + }); }); describe("operation return type", () => { @@ -162,7 +192,7 @@ describe("versioning: validate incompatible references", () => { expectDiagnostics(diagnostics, { code: "@typespec/versioning/incompatible-versioned-reference", message: - "'TestService.test' was added on version 'v2' but referencing type 'TestService.Foo' added in version 'v3'.", + "'TestService.test' was added in version 'v2' but referencing type 'TestService.Foo' added in version 'v3'.", }); }); @@ -177,7 +207,7 @@ describe("versioning: validate incompatible references", () => { expectDiagnostics(diagnostics, { code: "@typespec/versioning/incompatible-versioned-reference", message: - "'TestService.test' was removed on version 'v3' but referencing type 'TestService.Foo' removed in version 'v2'.", + "'TestService.test' was removed in version 'v3' but referencing type 'TestService.Foo' removed in version 'v2'.", }); }); @@ -240,7 +270,7 @@ describe("versioning: validate incompatible references", () => { expectDiagnostics(diagnostics, { code: "@typespec/versioning/incompatible-versioned-reference", message: - "'TestService.Bar.foo' was added on version 'v2' but referencing type 'TestService.Foo' added in version 'v3'.", + "'TestService.Bar.foo' was added in version 'v2' but referencing type 'TestService.Foo' added in version 'v3'.", }); }); @@ -257,7 +287,7 @@ describe("versioning: validate incompatible references", () => { expectDiagnostics(diagnostics, { code: "@typespec/versioning/incompatible-versioned-reference", message: - "'TestService.Bar.foo' was removed on version 'v3' but referencing type 'TestService.Foo' removed in version 'v2'.", + "'TestService.Bar.foo' was removed in version 'v3' but referencing type 'TestService.Foo' removed in version 'v2'.", }); }); @@ -362,7 +392,7 @@ describe("versioning: validate incompatible references", () => { expectDiagnostics(diagnostics, { code: "@typespec/versioning/incompatible-versioned-reference", message: - "'TestService.Bar' was added on version 'v3' but contains type 'TestService.Bar.foo' added in version 'v2'.", + "'TestService.Bar' was added in version 'v3' but contains type 'TestService.Bar.foo' added in version 'v2'.", }); }); @@ -377,7 +407,7 @@ describe("versioning: validate incompatible references", () => { expectDiagnostics(diagnostics, { code: "@typespec/versioning/incompatible-versioned-reference", message: - "'TestService.Bar' was removed on version 'v2' but contains type 'TestService.Bar.foo' removed in version 'v3'.", + "'TestService.Bar' was removed in version 'v2' but contains type 'TestService.Bar.foo' removed in version 'v3'.", }); }); @@ -438,7 +468,7 @@ describe("versioning: validate incompatible references", () => { "'TestService.Bar.foo' is referencing versioned type 'TestService.Versioned' but is not versioned itself.", }); }); - it("emit diagnostic when using versioned union variant in non versioned operation return type", async () => { + it("emit diagnostic when using versioned union variant in nin versioned operation return type", async () => { const diagnostics = await runner.diagnose(` @added(Versions.v2) model Versioned {} @@ -451,7 +481,7 @@ describe("versioning: validate incompatible references", () => { }); }); - it("emit diagnostic when using versioned array element in non versioned operation return type", async () => { + it("emit diagnostic when using versioned array element in nin versioned operation return type", async () => { const diagnostics = await runner.diagnose(` @added(Versions.v2) model Versioned {} @@ -464,7 +494,7 @@ describe("versioning: validate incompatible references", () => { }); }); - it("emit diagnostic when using versioned tuple element in non versioned operation return type", async () => { + it("emit diagnostic when using versioned tuple element in nin versioned operation return type", async () => { const diagnostics = await runner.diagnose(` @added(Versions.v2) model Versioned {} @@ -500,7 +530,7 @@ describe("versioning: validate incompatible references", () => { expectDiagnostics(diagnostics, { code: "@typespec/versioning/incompatible-versioned-reference", message: - "'TestService.Bar' was added on version 'v3' but contains type 'TestService.foo' added in version 'v2'.", + "'TestService.Bar' was added in version 'v3' but contains type 'TestService.foo' added in version 'v2'.", }); }); @@ -515,7 +545,7 @@ describe("versioning: validate incompatible references", () => { expectDiagnostics(diagnostics, { code: "@typespec/versioning/incompatible-versioned-reference", message: - "'TestService.Bar' was removed on version 'v2' but contains type 'TestService.foo' removed in version 'v3'.", + "'TestService.Bar' was removed in version 'v2' but contains type 'TestService.foo' removed in version 'v3'.", }); }); @@ -578,7 +608,7 @@ describe("versioning: validate incompatible references", () => { expectDiagnostics(diagnostics, { code: "@typespec/versioning/incompatible-versioned-reference", message: - "'TestService.WidgetService' was added on version 'v1' but referencing type 'TestService.Widget' added in version 'v2'.", + "'TestService.WidgetService' was added in version 'v1' but referencing type 'TestService.Widget' added in version 'v2'.", }); }); @@ -635,7 +665,7 @@ describe("versioning: validate incompatible references", () => { expectDiagnostics(diagnostics, { code: "@typespec/versioning/incompatible-versioned-reference", message: - "'TestService.test' was added on version 'v1' but referencing type 'VersionedLib.Foo' added in version 'v3'.", + "'TestService.test' was added in version 'v1' but referencing type 'VersionedLib.Foo' added in version 'v3'.", }); });