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

[tcgc] add getHttpOperationParameter helper function #2010

Open
wants to merge 5 commits into
base: release/december-2024
Choose a base branch
from
+606 −7
Open

[tcgc] add getHttpOperationParameter helper function #2010

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
f8149e2
add `getHttpOperationParameter` helper function
tadelesh Dec 23, 2024
b92bbb6
changeset
tadelesh Dec 23, 2024
8aded78
format
tadelesh Dec 23, 2024
7336f94
refine with review
tadelesh Dec 24, 2024
902f728
remove useless code
tadelesh Dec 24, 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
7 changes: 7 additions & 0 deletions .chronus/changes/add_method_parameter_helper-2024-11-23-18-47-42.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
---
changeKind: feature
packages:
- "@azure-tools/typespec-client-generator-core"
---

add `getHttpOperationParameter` helper function
26 changes: 20 additions & 6 deletions packages/typespec-client-generator-core/src/http.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -523,6 +523,7 @@ export function getCorrespondingMethodParams(
): [SdkModelPropertyType[], readonly Diagnostic[]] {
const diagnostics = createDiagnosticCollector();

// 1. To see if the service parameter is a client parameter.
const operationLocation = getLocationOfOperation(operation)!;
let clientParams = context.__clientToParameters.get(operationLocation);
if (!clientParams) {
Expand All @@ -535,8 +536,11 @@ export function getCorrespondingMethodParams(
twoParamsEquivalent(context, x.__raw, serviceParam.__raw) ||
(x.__raw?.kind === "ModelProperty" && getParamAlias(context, x.__raw) === serviceParam.name),
);
if (correspondingClientParams.length > 0) return diagnostics.wrap(correspondingClientParams);
if (correspondingClientParams.length > 0) {
return diagnostics.wrap(correspondingClientParams);
}

// 2. To see if the service parameter is api version parameter that has been elevated to client.
if (serviceParam.isApiVersionParam && serviceParam.onClient) {
const existingApiVersion = clientParams?.find((x) => isApiVersion(context, x));
if (!existingApiVersion) {
Expand All @@ -552,8 +556,10 @@ export function getCorrespondingMethodParams(
);
return diagnostics.wrap([]);
}
return diagnostics.wrap(clientParams.filter((x) => isApiVersion(context, x)));
return diagnostics.wrap(existingApiVersion ? [existingApiVersion] : []);
}

// 3. To see if the service parameter is subscription parameter that has been elevated to client (only for arm service).
if (isSubscriptionId(context, serviceParam)) {
const subId = clientParams.find((x) => isSubscriptionId(context, x));
if (!subId) {
Expand All @@ -572,13 +578,13 @@ export function getCorrespondingMethodParams(
return diagnostics.wrap(subId ? [subId] : []);
}

// to see if the service parameter is a method parameter or a property of a method parameter
// 4. To see if the service parameter is a method parameter or a property of a method parameter.
const directMapping = findMapping(methodParameters, serviceParam);
if (directMapping) {
return diagnostics.wrap([directMapping]);
}

// to see if all the property of service parameter could be mapped to a method parameter or a property of a method parameter
// 5. To see if all the property of the service parameter could be mapped to a method parameter or a property of a method parameter.
if (serviceParam.kind === "body" && serviceParam.type.kind === "model") {
const retVal = [];
for (const serviceParamProp of serviceParam.type.properties) {
Expand All @@ -592,6 +598,7 @@ export function getCorrespondingMethodParams(
}
}

// If mapping could not be found, TCGC will report error since we can't generate the client code without this mapping.
diagnostics.add(
createDiagnostic({
code: "no-corresponding-method-param",
Expand All @@ -605,6 +612,12 @@ export function getCorrespondingMethodParams(
return diagnostics.wrap([]);
}

/**
* Try to find the mapping of a service paramete or a property of a service parameter to a method parameter or a property of a method parameter.
* @param methodParameters
* @param serviceParam
* @returns
*/
function findMapping(
methodParameters: SdkModelPropertyType[],
serviceParam: SdkHttpParameter | SdkModelPropertyType,
Expand All @@ -613,15 +626,15 @@ function findMapping(
const visited: Set<SdkModelType> = new Set();
while (queue.length > 0) {
const methodParam = queue.shift()!;
// http operation parameter/body parameter/property of body parameter could either from an operation parameter directly or from a property of an operation parameter
// HTTP operation parameter/body parameter/property of body parameter could either from an operation parameter directly or from a property of an operation parameter.
if (
methodParam.__raw &&
serviceParam.__raw &&
findRootSourceProperty(methodParam.__raw) === findRootSourceProperty(serviceParam.__raw)
) {
return methodParam;
}
// this following two hard code mapping is for the case that TCGC help to add content type and accept header is not exist
// Two following two hard coded mapping is for the case that TCGC help to add content type and accept header when not exists.
if (
serviceParam.kind === "header" &&
serviceParam.serializedName === "Content-Type" &&
Expand All @@ -636,6 +649,7 @@ function findMapping(
) {
return methodParam;
}
// BFS to find the mapping.
if (methodParam.type.kind === "model" && !visited.has(methodParam.type)) {
visited.add(methodParam.type);
let current: SdkModelType | undefined = methodParam.type;
Expand Down
67 changes: 67 additions & 0 deletions packages/typespec-client-generator-core/src/public-utils.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,8 +31,17 @@ import {
listOperationsInOperationGroup,
} from "./decorators.js";
import {
SdkBodyParameter,
SdkClientType,
SdkCookieParameter,
SdkHeaderParameter,
SdkHttpOperation,
SdkHttpOperationExample,
SdkModelPropertyType,
SdkModelType,
SdkPathParameter,
SdkQueryParameter,
SdkServiceMethod,
SdkServiceOperation,
SdkType,
TCGCContext,
Expand Down Expand Up @@ -698,3 +707,61 @@ export function isAzureCoreModel(t: SdkType): boolean {
export function isPagedResultModel(context: TCGCContext, t: SdkType): boolean {
return context.__pagedResultSet.has(t);
}

/**
* Find corresponding http parameter list for a client initialization or service method parameter or the property of that parameter.
*
* @param method
* @param param
* @returns
*/
export function getHttpOperationParameter(
method: SdkServiceMethod<SdkHttpOperation>,
param: SdkModelPropertyType,
):
| SdkPathParameter
| SdkQueryParameter
| SdkHeaderParameter
| SdkCookieParameter
| SdkBodyParameter
| undefined {
const operation = method.operation;
const queue: SdkModelPropertyType[] = [param];
const visited: Set<SdkModelType> = new Set();
// BFS to find the corresponding http parameter.
// An http parameter will be mapped to a method/client parameter, several method/client parameters (body spread case), or one property of a method property (metadata on property case).
// So, when we try to find which http parameter a method parameter corresponds to, we compare the `correspondingMethodParams` list directly.
// When we try to find which http parameter a property corresponds to, we need to consider the following cases:
// 1. The property itself is an http parameter.
// 2. The property is a model, and the properties of the model are http parameters, recursively.
// 3. The property is a model, and the base model of the model has http parameters, recursively.
// When we find one, we return directly since a method/client parameter or property could only be used in one http parameter.
Copy link
Member

@weidongxu-microsoft weidongxu-microsoft Dec 24, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Is this assert valid for below case?

      model Params {
        @query foo: string;
        @query bar: string;
      }
      op func(...Params): void;
      `,
      `
      namespace MyCustomizations;
      op func(params: MyService.Params): void;
      @@override(MyService.func, MyCustomizations.func);

Example of this: grouping long list of query params in a e.g. List

Copy link
Member Author

@tadelesh tadelesh Dec 24, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

i think so. it is applied to 1. for params, its corresponding http parameters is undefined. you need to get them with foo and bar property. see my last test.

Copy link
Member

@weidongxu-microsoft weidongxu-microsoft Dec 24, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Ok, so a method parameter would map to at most 1 parameter, and strictly an HTTP parameter (body param or query param or header param or undefined, but not to property in body).

It is somewhat unexpected that one can go to the properties inside the model and call getHttpOperationParameter on it. Especially the @bodyRoot case of Shelf.name which most language won't generate a name in Shelf.

      model Shelf {
        @query
        name: string;
        theme?: string;
      }
      model CreateShelfRequest {
        @bodyRoot
        body: Shelf;
      }

Let's see what other reviewers think.

Copy link
Member Author

Choose a reason for hiding this comment

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

a little bit force and back for this. just want to make it simple. current spec does not contain such mix usage at least.

Copy link
Member

@weidongxu-microsoft weidongxu-microsoft Dec 25, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

One concern is, if I call the method, and find the parameter maps to a kind=body, I seems not be able to tell at this state, whether the parameter is the body, or it actually be a property within that body?

Copy link
Member Author

@tadelesh tadelesh Dec 25, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

for current code, it will tell you this parameter is used as body. but it could not tell you the property of this parameter is also used as a query parameter.

Copy link
Member

@weidongxu-microsoft weidongxu-microsoft Dec 25, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

What I mean is,

model Model { key: string }

op myOp(@header h: string, @query q: string, @path p: string, @body body: Model): void;

call on "body" method param would map to body body param of the Model.

And

op myOp(@header h: string, @query q: string, @path p: string, ...Model): void;

call on "key" method param would also map to body body param of the Model.

Maybe it is not that bad, maybe we can use the mapping from the other side to determine...

Copy link
Member Author

Choose a reason for hiding this comment

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

i got your point. you mean there is no way to tell how to map the exact value. then you are right, tcgc suggest to map from http parameter to method parameter.

while (queue.length > 0) {
const param = queue.pop();
for (const p of operation.parameters) {
for (const cp of p.correspondingMethodParams) {
if (cp === param) {
return p;
}
}
}
if (operation.bodyParam) {
archerzz marked this conversation as resolved.
Show resolved Hide resolved
for (const cp of operation.bodyParam.correspondingMethodParams) {
if (cp === param) {
return operation.bodyParam;
}
}
}
if (param?.kind === "property" && param?.type.kind === "model" && !visited.has(param.type)) {
visited.add(param.type);
let current: SdkModelType | undefined = param.type;
while (current) {
for (const prop of param.type.properties) {
queue.push(prop);
}
current = current.baseModel;
}
}
}
return undefined;
}
Loading
Loading