-
Notifications
You must be signed in to change notification settings - Fork 75
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.
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
Try not flatten payload #2020
Try not flatten payload #2020
Conversation
...pespec-test/test/contentsafety_modular/generated/typespec-ts/review/ai-content-safety.api.md
Outdated
Show resolved
Hide resolved
...pespec-test/test/contentsafety_modular/generated/typespec-ts/review/ai-content-safety.api.md
Outdated
Show resolved
Hide resolved
packages/typespec-test/test/widget_dpg/generated/typespec-ts/src/models/models.ts
Outdated
Show resolved
Hide resolved
packages/typespec-test/test/eventgrid_modular/generated/typespec-ts/review/eventgrid.api.md
Show resolved
Hide resolved
...es/typespec-test/test/contentsafety_modular/generated/typespec-ts/src/ContentSafetyClient.ts
Show resolved
Hide resolved
packages/typespec-test/test/openai_modular/generated/typespec-ts/review/openai_modular.api.md
Show resolved
Hide resolved
@joheredi @xirzec I want to confirm about the parameter order in flattened case.
what should the update method signature be ? Option1: Option2: Another question about the path parameters order is what if the path is something like As parameters order doesn't really matter to a API, so the way how OpenAPI3 emitter handles the parameter order may not be very helpful to us. see the typespec playground link here |
@qiaozha I think my preference is option 1 where we follow the spec order, since then the spec author can control it by reordering the parameters. We'd still extract any optional values and put them in an options bag at the end, right? |
As much as I prefer the order that is defined in typespec, I have to point out sometimes service team could be too casual about the parameters order from version to version which is not an api level breaking but is a SDK breaking. and we have to do things either asking service team to change the order back or adding workaround manually to avoid unnecessary SDK breaking changes. I am hoping we could do something in modular to reduce the communicating and manual effort. |
Yeah, if you think it will reduce churn to follow the same convention as Python I'm okay with it, though they have some behavior we can't directly map like headers always being kwargs (we could pull them into options but that might mean we have required options) |
Confirmed with @joheredi offline, we will keep the original order, as Option2 can only resolve parameter order changes between different parameter location and if there's changes inside each parameter location, it will still be problematic. |
packages/typespec-test/test/eventgrid_modular/generated/typespec-ts/src/api/operations.ts
Show resolved
Hide resolved
It feels a bit odd to have the new body params models be suffixed with It makes it tempting to flatten when there is exactly one required property in the body... in theory the service can never change to add another required body property without a break, yes? If they added an optional body property we could pull that into the actual options bag. |
@xirzec I am not sure I totally get your point. In the below case, prop2, prop3 and prop5 all belong to a body parameter's properties. excapt the body parameter's own name is ""
As we agree to flatten the body payload in this scenario, we will flatten all of them to the top level and because prop2 is required, it will go into the operation signature, prop3 and prop5 are optional, they will go into the option bag the same as the other optional ordinary query/header parameters if there's any.
And we build our payload like
If there're more required body properties in the above case, they will just go to the operation signature the same as other prop2. not in the option bag. Let me know if it makes sense to you. Thanks |
To answer this question, I think it's a breaking no matter we flatten it or not. if we don't flatten, the breaking will be a model layer breaking, if we flatten it, it will be an operation signature breaking. |
Sorry, I think it will help if I give specific examples that I was looking at in the generated output: export interface AddOrUpdateBlockItemsOptions {
blockItems: TextBlockItemInfo[];
}
export class ContentSafetyClient {
addOrUpdateBlockItems(blocklistName: string, body: AddOrUpdateBlockItemsOptions, options?: AddOrUpdateBlockItemsRequestOptions): Promise<AddOrUpdateBlockItemsResult>;
} In the above it would feel better to name the first interface something like So now it would look like: export interface AddOrUpdateBlockItemsBody {
blockItems: TextBlockItemInfo[];
}
export class ContentSafetyClient {
addOrUpdateBlockItems(blocklistName: string, body: AddOrUpdateBlockItemsBody, options?: AddOrUpdateBlockItemsRequestOptions): Promise<AddOrUpdateBlockItemsResult>;
} In this specific case though, I was thinking it is a little silly to have an entire object just for one property, so as an optimization we could flatten bodies that have a single required property so that the above looked like: export class ContentSafetyClient {
addOrUpdateBlockItems(blocklistName: string, blockItems: TextBlockItemInfo[], options?: AddOrUpdateBlockItemsRequestOptions): Promise<AddOrUpdateBlockItemsResult>;
} Does that make sense? So less of a global rule and more of a specific optimization for body objects with only one required property. |
I have to say, I have mixed feelings about flatten one required property body payload.
My preference would be we could support one required property flatten in model based flatten when we get to the x-ms-client-flatten thing ? |
packages/typespec-ts/test/modularIntegration/generated/parameters/spread/src/SpreadClient.ts
Show resolved
Hide resolved
I agree with Qiaoqiao that not setting this rule for one required property. This may introduce un-necessary breakings and also introduce complexity for specific conditions. For the overall pr it looks good to me and as we discussed we still have the requirement of flattening especially for mgmt SDKs and I prefer we continue monitoring this topic.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Left small comments, overall LGTM.
...pespec-test/test/contentsafety_modular/generated/typespec-ts/review/ai-content-safety.api.md
Show resolved
Hide resolved
@joheredi I will merge this PR first, let me know if you have other concerns. Also, please note, we will need to work with OpenAI to use spread of alias if they want a flattened operation sigature. |
* try-not-flatten-payload * fix tests * should not generate duplicate models and should not generate body schema properties in options * fix model property as path parameter * format * fix ci * add ut for alias flattening * add ut for not flatten if spread models * add deserialize for flattened body * enable cadl ranch test for spread in modular
fixes #2016