Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix issue with operation parameter versioning #2691

Merged
merged 2 commits into from
Nov 22, 2023
Merged

Fix issue with operation parameter versioning #2691

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
98a7b27
Add test.
tjprescott Nov 22, 2023
1d40bd5
Fix #2688.
tjprescott Nov 22, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 10 additions & 0 deletions common/changes/@typespec/versioning/versioning-OpParams_2023-11-22-18-08.json
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,10 @@
{
"changes": [
{
"packageName": "@typespec/versioning",
"comment": "",
"type": "none"
}
],
"packageName": "@typespec/versioning"
}
18 changes: 0 additions & 18 deletions packages/versioning/src/validate.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -532,24 +532,6 @@ function validateOperationParameter(
return;
}
}

if (paramAvailability !== undefined) {
validateAvailabilityForContains(
program,
operationAvailability,
paramAvailability,
operation,
parameter
);
} else if (paramTypeAvailability !== undefined) {
validateAvailabilityForRef(
program,
operationAvailability,
paramTypeAvailability,
operation,
parameter.type
);
}
Comment on lines -536 to -552
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For this to work, I would actually need to build the availability map based on ALL references in the spec, which would be considerably more design and implementation effort. The new test showcases the problem. Thus, I've just removed this check.

}

/**
Expand Down
46 changes: 0 additions & 46 deletions packages/versioning/test/incompatible-versioning.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -146,52 +146,6 @@ describe("versioning: validate incompatible references", () => {
});
});

it("emit diagnostic when when op was added before parameter type", 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 type", async () => {
const diagnostics = await runner.diagnose(`
@added(Versions.v2)
model Foo {}

op Template<T>(param: T): void;

@added(Versions.v1)
op test is Template<Foo>;
`);
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 when parameter was added before op", async () => {
const diagnostics = await runner.diagnose(`
model Foo {}

@added(Versions.v2)
op test(@added(Versions.v1) param: Foo): void;
`);
expectDiagnostics(diagnostics, {
code: "@typespec/versioning/incompatible-versioned-reference",
message:
"'TestService.test' was added in version 'v2' but contains type 'TestService.(anonymous model).param' added in version 'v1'.",
});
});

it("emit diagnostic when type changed to types that don't exist", async () => {
const diagnostics = await runner.diagnose(`
@added(Versions.v3)
Expand Down
28 changes: 28 additions & 0 deletions packages/versioning/test/versioning.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -912,6 +912,34 @@ describe("versioning: logic", () => {
w2.operations.get("create")?.parameters.properties.size === 2;
});

it("can share a model reference between operations with different versions", async () => {
const code = `
@test("MyService")
@versioned(Versions)
namespace MyService;

enum Versions { v1, v2, v3 };

model Foo {
prop: string;
}

model Parameters {
name: string;

@added(Versions.v2)
age: Foo;
}

@added(Versions.v1)
op oldOp(...Parameters): void;

@added(Versions.v3)
op newOp(...Parameters): void;
Comment on lines +930 to +938
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is the crux of issue #2688. While this appears to be an error just looking at Parameters and newOp, it makes sense when you consider the existence of oldOp.

`;
ok(await runner.compile(code));
});

it("can be removed", async () => {
const {
projections: [v1, v2],
Expand Down