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

Jump to bottom

Unable to override HTTP method from operation template - issue #3636 #3637

Closed
wants to merge 3 commits into from
Closed

Unable to override HTTP method from operation template - issue #3636 #3637

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
663455e
Unable to override HTTP method from operation template - issue #3636
johanste Jun 20, 2024
67d60bd
Removed disable warning flag from and kept setOperationVerb internal/…
johanste Jun 20, 2024
d336564
Create overridehttpverb-2024-5-20-22-53-48.md
johanste Jun 20, 2024
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
8 changes: 8 additions & 0 deletions .chronus/changes/overridehttpverb-2024-5-20-22-53-48.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
---
# Change versionKind to one of: internal, fix, dependencies, feature, deprecation, breaking
changeKind: fix
packages:
- "@typespec/http"
---

Unable to override HTTP method from operation template - issue #3636
7 changes: 3 additions & 4 deletions packages/http/src/decorators.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -362,17 +362,16 @@ function rangeDescription(start: number, end: number) {
return undefined;
}

function setOperationVerb(program: Program, entity: Type, verb: HttpVerb): void {
export function setOperationVerb(program: Program, entity: Type, verb: HttpVerb): void {
if (entity.kind === "Operation") {
if (!program.stateMap(HttpStateKeys.verbs).has(entity)) {
program.stateMap(HttpStateKeys.verbs).set(entity, verb);
} else {
if (program.stateMap(HttpStateKeys.verbs).has(entity)) {
reportDiagnostic(program, {
code: "http-verb-duplicate",
format: { entityName: entity.name },
target: entity,
});
}
program.stateMap(HttpStateKeys.verbs).set(entity, verb);
} else {
reportDiagnostic(program, {
code: "http-verb-wrong-type",
Expand Down
2 changes: 1 addition & 1 deletion packages/http/src/lib.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@ export const $lib = createTypeSpecLibrary({
name: "@typespec/http",
diagnostics: {
"http-verb-duplicate": {
severity: "error",
Copy link
Member

Choose a reason for hiding this comment

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

I don’t feel like I agree with making this a warning, we have the same pattern for other decorators. It doesn’t make sense to specify 2 verb on the exact same node. We should just allow it to override but complain if it’s specified on the same node not operation. Having to suppress that warning when you want to pverrride with op is just feels wrong anyway

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If we want to remove the warning altogether, I would not be opposed to it.

I don't think I'd bother with checking/warn if you specified it twice on the same node, though - I think you get exactly what you asked for.

Copy link
Member

@timotheeguerin timotheeguerin Jun 21, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

So i think the following should be an error

@get @get op op1(): void;
@get @post op op2(): void;

but this should not emit any errors/warning

@get op base(): void;

@post op override is base;

Playground

Copy link
Member

@timotheeguerin timotheeguerin Jun 21, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Hhm I guess duplicate-decorator which is the common diagnostic we emit for duplicate use is a warning not error so that can stay a warning, still it shouldn't need to be suppress and only emit this diagnositc when annotating the same node twice(existing logic is validateDecoratorUniqueOnNode)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Is getting warnings for multiple decorator instances on the same node here a blocker? I can add another utility function to handle the case where there can only be one instance in a "family" of decorators on a given node, but it feels like a low priority thing. If I wasn't digging around in this code to begin with, I would have said it goes on the backlog... and I'm tempted to say so even if I'm digging around in this code.

Copy link
Member

@timotheeguerin timotheeguerin Jun 24, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Well from experience seeing how a significant amount of azure spec written don’t see those as warning but errors(warn as error) I do think it is quite a bad experience that will lead to more question we have to keep answering

severity: "warning",
messages: {
default: paramMessage`HTTP verb already applied to ${"entityName"}`,
},
Expand Down
17 changes: 17 additions & 0 deletions packages/http/test/http-decorators.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -154,6 +154,23 @@ describe("http: decorators", () => {
]);
});

describe("overrideHttpMethod", () => {
it("emit diagnostics when @post is added to operation with preexisting http method ", async () => {
const diagnostics = await runner.diagnose(`
@get op test<T>(): T;

@post op test2 is test<string>;
`);

expectDiagnostics(diagnostics, [
{
code: "@typespec/http/http-verb-duplicate",
message: "HTTP verb already applied to test2",
},
]);
});
});

it("emit diagnostics when query name is not a string or of type QueryOptions", async () => {
const diagnostics = await runner.diagnose(`
op test(@query(123) MyQuery: string): string;
Expand Down
25 changes: 25 additions & 0 deletions packages/http/test/routes.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -293,6 +293,31 @@ describe("http: routes", () => {
deepStrictEqual(routes, [{ verb: "get", path: "/", params: [] }]);
});

it("produces the right http method", async () => {
const routes = await getRoutesFor(
`
#suppress "@typespec/http/http-verb-duplicate" "For testing"
@get @post op get(): {};
#suppress "@typespec/http/http-verb-duplicate" "For testing"
@post @get op post(): {};
#suppress "@typespec/http/http-verb-duplicate" "For testing"
@put @put @get op put(): {};
#suppress "@typespec/http/http-verb-duplicate" "For testing"
@patch @delete op patch(): {};
#suppress "@typespec/http/http-verb-duplicate" "For testing"
@delete @get @post op delete(): {};
`
);

deepStrictEqual(routes, [
{ verb: "get", path: "/", params: [] },
{ verb: "post", path: "/", params: [] },
{ verb: "put", path: "/", params: [] },
{ verb: "patch", path: "/", params: [] },
{ verb: "delete", path: "/", params: [] },
]);
});

it("always produces a route starting with /", async () => {
const routes = await getRoutesFor(
`
Expand Down
Loading