[tcgc] add getHttpOperationParameter helper function
#2010
Conversation
tadelesh
requested review from
lmazuel,
m-nash,
iscai-msft,
srnagar,
joheredi,
bterlson,
markcowl,
allenjzhang and
timotheeguerin
as code owners
|
All changed packages have been documented.
Show changes
|
|
You can try these changes here
|
| } | ||
| } | ||
| } | ||
| if (result.length === 0 && param?.type.kind === "model" && !visited.has(param.type)) { |
There was a problem hiding this comment.
Seems grouping? Again, better to add comment.
BTW, do we have a UT on grouping (@override)?
There was a problem hiding this comment.
it is for @bodyRoot, for example:
model TestRequest {
@header
h: string;
@query
q: string;
prop1: string;
prop2: string;
}
op test(@bodyRoot request: TestRequest): void;
There was a problem hiding this comment.
added a test with @override.
| } | ||
| } | ||
| } | ||
| if (result.length === 0 && param?.type.kind === "model" && !visited.has(param.type)) { |
There was a problem hiding this comment.
Seems grouping? Again, better to add comment.
BTW, do we have a UT on grouping (@override)?
packages/typespec-client-generator-core/test/public-utils/get-http-operation-parameter.test.ts
Outdated
Show resolved
Hide resolved
| // 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. |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
a little bit force and back for this. just want to make it simple. current spec does not contain such mix usage at least.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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...
There was a problem hiding this comment.
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.
|
@qiaozha please help to review. current solution is kind of a tradeoff which should meet all language's requirement, but not so convenience for exact value mapping. |
resolve: #1441