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

tsp, path allowReserved #2904

Merged
merged 4 commits into from
Aug 9, 2024
Merged

tsp, path allowReserved #2904

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
bf5614c
tsp, path allowReserved
weidongxu-microsoft Aug 9, 2024
d656cd0
update for tsp-client 0.19.0
weidongxu-microsoft Aug 9, 2024
4b5d7ad
use option from param
weidongxu-microsoft Aug 9, 2024
5264c01
nit
weidongxu-microsoft Aug 9, 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
15 changes: 13 additions & 2 deletions typespec-extension/src/code-model-builder.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -118,6 +118,7 @@ import {
getHeaderFieldName,
getHeaderFieldOptions,
getPathParamName,
getPathParamOptions,
getQueryParamName,
getQueryParamOptions,
getServers,
Expand Down Expand Up @@ -306,6 +307,7 @@ export class CodeModelBuilder {
},
},
extensions: {
// TODO: deprecate this logic of string/url for x-ms-skip-url-encoding
"x-ms-skip-url-encoding": schema instanceof UriSchema,
},
// // make the logic same as TCGC, which takes the server-side default of host as client-side default
Expand Down Expand Up @@ -1072,14 +1074,23 @@ export class CodeModelBuilder {
schema = this.processSchemaFromSdkType(sdkType, param.param.name);
}

// skip-url-encoding
let extensions: { [id: string]: any } | undefined = undefined;
// skip-url-encoding
if (param.type === "path") {
const pathParamOptions = getPathParamOptions(this.program, param.param);
if (pathParamOptions.allowReserved) {
extensions = extensions ?? {};
extensions["x-ms-skip-url-encoding"] = true;
}
}
// TODO: deprecate this logic of string/url for x-ms-skip-url-encoding
if (
(param.type === "query" || param.type === "path") &&
param.param.type.kind === "Scalar" &&
schema instanceof UriSchema
) {
extensions = { "x-ms-skip-url-encoding": true };
extensions = extensions ?? {};
extensions["x-ms-skip-url-encoding"] = true;
}
Comment on lines +1079 to 1087
Copy link
Member Author

@weidongxu-microsoft weidongxu-microsoft Aug 9, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

I didn't deprecate the old logic of uri for skip encoding, string for not skip encoding. I added TODO to these part.

Plan to check later. Let's have support for allowReserved first.

It should be OK to drop such logic in TCGC SdkPackage migration.

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, TCGC should handle it and returns correct collection format. I am integrating with latest TCGC. I will pay attention to the test case.


if (this.supportsAdvancedVersioning()) {
Expand Down
4 changes: 2 additions & 2 deletions typespec-tests/tsp/server.tsp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -59,8 +59,8 @@ namespace Cadl.ContosoServer {
name: "Contoso.Sub.ContosoClient",
service: Cadl.ContosoServer,
})
@route("/contoso/{group}")
@route("/contoso/")
interface ServerOp {
get(@path group: url): OkResponse | NoContentResponse;
get(@path(#{ allowReserved: true }) group: string): OkResponse | NoContentResponse;
}
Comment on lines -62 to 65
Copy link
Member Author

@weidongxu-microsoft weidongxu-microsoft Aug 9, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

0.59.0 requires either way

  • /contoso/{+group} without allowReserved (URI template)
  • /contoso/ with allowReserved. compiler or http lib would append the group to path (kind of the autoRoute)

Copy link
Member

Choose a reason for hiding this comment

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

Do we want to add another test case using URI template? (/contoso/{+group})? Or cadl-ranch already has case to cover it?

Copy link
Member Author

@weidongxu-microsoft weidongxu-microsoft Aug 9, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Timothee is adding a cadl-ranch test.

My local test, that allowReserved does not work if route written as @route("/contoso/{+group}"). Could be the http lib just don't want to support this (say it require we use URI template directly), or it could be a bug.

Same effect on playground (Swagger having x-ms-skip-url-encoding or not).

Copy link
Member Author

@weidongxu-microsoft weidongxu-microsoft Aug 9, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Ok, it need to be (no @path)

  @route("/contoso/{+group}")
  interface ServerOp {
    get(group: string): OkResponse | NoContentResponse;
  }

Copy link
Member Author

@weidongxu-microsoft weidongxu-microsoft Aug 9, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Timothee is adding a cadl-ranch test.

My local test, that allowReserved does not work if route written as @route("/contoso/{+group}"). Could be the http lib just don't want to support this (say it require we use URI template directly), or it could be a bug.

Same effect on playground (Swagger having x-ms-skip-url-encoding or not).

This appears to be a bug in http lib. microsoft/typespec#4132

The getPathParamOptions gives undefined. But param.allowReserved is true. Will report a bug there.

Therefore I switched to param.allowReserved.

}
Loading