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 all commits
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
2 changes: 1 addition & 1 deletion eng/sdk/sync_sdk.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -77,7 +77,7 @@ def update_emitter(package_json_path: str, use_dev_package: bool):
logging.error('Failed to locate the dev package.')

logging.info('Update emitter-package-lock.json')
subprocess.check_call(['tsp-client', '--generate-lock-file'], cwd=sdk_root)
subprocess.check_call(['tsp-client', 'generate-lock-file'], cwd=sdk_root)
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.

tsp-client 0.19.0

$ tsp-client --help

Usage: tsp-client <COMMAND> [OPTIONS]

Use one of the supported commands to get started generating clients from a
TypeSpec project. This tool will default to using your current working directory
to generate clients in and will use it to look for relevant configuration files.
To specify a different directory, use the -o or --output-dir option.

Commands:
  init                         Initialize the SDK project folder from a
                               tspconfig.yaml
  sync                         Sync TypeSpec project specified in
                               tsp-location.yaml
  generate                     Generate from a TypeSpec project
  update                       Sync and generate from a TypeSpec project
  convert                      Convert a swagger specification to TypeSpec
  generate-lock-file           Generate a lock file under the eng/ directory
                               from an existing emitter-package.json
  sort-swagger <swagger-file>  Sort a swagger file to be the same content order
                               with TypeSpec generated swagger

Options:
      --version     Show version number                                [boolean]
  -d, --debug       Enable debug logging                               [boolean]
  -o, --output-dir  Specify an alternate output directory for the generated
                    files.                               [string] [default: "."]
  -y, --no-prompt   Skip any interactive prompts.                      [boolean]
      --help        Show help                                          [boolean]



def get_generated_folder_from_artifact(module_path: str, artifact: str, type: str) -> str:
Expand Down
48 changes: 25 additions & 23 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 @@ -67,7 +67,6 @@ import {
getHttpOperationWithCache,
getWireName,
isApiVersion,
isInternal,
isSdkBuiltInKind,
isSdkIntKind,
listClients,
Expand Down Expand Up @@ -116,10 +115,8 @@ import {
Visibility,
getAuthentication,
getHeaderFieldName,
getHeaderFieldOptions,
getPathParamName,
getQueryParamName,
getQueryParamOptions,
getServers,
getStatusCodeDescription,
isHeader,
Expand Down Expand Up @@ -306,6 +303,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 @@ -401,14 +399,12 @@ export class CodeModelBuilder {
return !this.options["flavor"] || this.options["flavor"].toLocaleLowerCase() === "azure";
}

private isInternal(context: SdkContext, operation: Operation): boolean {
private isInternal(operation: Operation): boolean {
const access = getAccess(operation);
if (access) {
return access === "internal";
} else {
// TODO: deprecate "internal"
// eslint-disable-next-line deprecation/deprecation
return isInternal(context, operation);
return false;
weidongxu-microsoft marked this conversation as resolved.
Show resolved Hide resolved
}
}

Expand Down Expand Up @@ -759,7 +755,7 @@ export class CodeModelBuilder {
this.sdkContext,
operation,
);
codeModelOperation.internalApi = this.isInternal(this.sdkContext, operation);
codeModelOperation.internalApi = this.isInternal(operation);

const convenienceApiName = this.getConvenienceApiName(operation);
let generateConvenienceApi: boolean = Boolean(convenienceApiName);
Expand Down Expand Up @@ -1072,14 +1068,22 @@ 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") {
if (param.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 All @@ -1096,9 +1100,8 @@ export class CodeModelBuilder {
let explode = undefined;
if (param.param.type.kind === "Model" && isArrayModelType(this.program, param.param.type)) {
if (param.type === "query") {
const queryParamOptions = getQueryParamOptions(this.program, param.param);
// eslint-disable-next-line deprecation/deprecation
const queryParamFormat = queryParamOptions?.format;
const queryParamFormat = param?.format;
if (queryParamFormat) {
switch (queryParamFormat) {
case "csv":
Expand Down Expand Up @@ -1128,17 +1131,16 @@ export class CodeModelBuilder {
}
}
} else if (param.type === "header") {
const headerFieldOptions = getHeaderFieldOptions(this.program, param.param);
switch (headerFieldOptions?.format) {
case "csv":
style = SerializationStyle.Simple;
break;

default:
if (headerFieldOptions?.format) {
this.logWarning(`Unrecognized header parameter format: '${headerFieldOptions?.format}'.`);
}
break;
if (param.format) {
switch (param.format) {
case "csv":
style = SerializationStyle.Simple;
break;

default:
this.logWarning(`Unrecognized header parameter format: '${param.format}'.`);
break;
}
}
}
}
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