-
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
support-server-parameter-in-modular #2032
support-server-parameter-in-modular #2032
Conversation
packages/typespec-test/test/eventgrid_modular/generated/typespec-ts/src/api/EventGridContext.ts
Show resolved
Hide resolved
Pending on adding a few test cases for encoding. |
const result = await client.requestBody.default( | ||
stringToUint8Array("dGVzdA==", "base64"), | ||
{ | ||
requestOptions: { headers: { "content-type": "application/json" } } |
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.
we need to specify contentType as application/json if the body is string type. otherwise, cadl ranch will report parse error from raw body parser. in autorest testserver case, we will generate a contentType parameter like
export interface StringPutNullMediaTypesParam {
/** Request content type */
contentType?: "application/json";
}
@MaryGao do you have any pointer on why we could generate the contentType in this case but not from the TypeSpec ? Thank you.
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.
We don't have this kind of filter for tsp input. There is no contentType defined in tsp so we will not generate it for them.
I think the issue is introduced in core https://github.com/Azure/azure-sdk-for-js/blob/main/sdk/core/core-client-rest/src/sendRequest.ts#L157-L174 when getting body content.
If we specify contentType, the core would format as JSON.stringify(body) in our case it would be '"dGVzdA=="'
; it not, it would be pure string that is "dGVzdA=="
.
The former would be a correct json but the latter would not, so cadl-ranch throws exceptions.
...ipt/test/rlcIntegration/generated/bodyStringRest/samples-dev/stringGetBase64EncodedSample.ts
Show resolved
Hide resolved
clientParam: string, | ||
options: ClientOptions = {} | ||
): ServiceClient { | ||
const baseUrl = | ||
options.baseUrl ?? `http://localhost:3000/client/structure/${clientParam}`; | ||
options.baseUrl ?? `${endpoint}/client/structure/${clientParam}`; |
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.
What is the difference between endpoint and baseUrl?
If they are the same I think we should change to
const baseUrl = `${options.baseUrl ?? endpoint}/client/structure/${clientParam}`;
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.
@xirzec @joheredi previously we had an issue here #1337 which implies that baseUrl and endpoint are the same thing, and I have this PR Azure/azure-sdk-for-js#21125 to mark the baseUrl as deprecated to encourage customer to use endpoint instead, but from the core-client-rest https://github.com/Azure/azure-sdk-for-js/blob/main/sdk/core/core-client-rest/src/common.ts#L319C1-L322C20, we are still using baseUrl in the RLC, are they not the same thing in the data plane context ? What's your suggestion here ?
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.
We should use endpoint, I'll have to dig a bit deeper in core-client-rest and see why we kept baseUrl. But in this case I think we should go with endpoint
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.
Prefer endpoint over baseUrl (it aligns better with our terminology in docs), AFAIK baseUrl is only for compat and can be deprecated/removed when possible
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.
Thanks @xirzec and @joheredi for confirming about this. I want to get things a little more clearer.
First of all, when we say baseUrl and endpoint are the same thing, we are referring to the one in the ClientOptions here not the endpoint parameter in this case, right ?
Secondly, If the answer for the first question is yes, based on the above comments, our action items would be,
- add an endpoint parameter, and mark baseUrl as deprecated in core-client-rest ClientOptions.
- change the codegen to use the
options.endpoint
like:or like the below if we care about the variable name.const baseUrl = options.endpoint ?? options.baseUrl ?? `http://localhost:3000/client/structure/${clientParam}`;
const endpoint = options.endpoint ?? options.baseUrl ?? `http://localhost:3000/client/structure/${clientParam}`; // other logic code const client = getClient(endpoint, options) as ServiceClient;
For this specific case, I prefer to interpret the endpoint is just a parameter which is part of the options.endpoint and happens to have the same name as endpoint, we can rename it to any other name if possible, so for @MaryGao's suggestion of changing something like below, it doesn't quite make sense to me.
const baseUrl = `${options.baseUrl ?? endpoint}/client/structure/${clientParam}`;
I prefer to do the codegen side change i.e. action item 2 after action item 1 is completed. @MaryGao let me know if you have other concerns ?
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.
I am okay with endpoint over baseUrl, but still confused about what extract value for endpoint. Say we have a url http://localhost:3000/client/structure/${clientParam}
it seems that the endpoint and baseUrl in options or the endpoint in typespec are the same, that is http://localhost:3000
.
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.
In typespec example, endpoint refers to the first parameter in the @ server decorator, and the typespec for this test case is like this way
@server(
"{endpoint}/client/structure/{client}",
"",
{
@doc("Need to be set as 'http://localhost:3000' in client.")
endpoint: url = "http://localhost:3000",
@doc("Need to be set as 'default', 'multi-client', 'renamed-operation', 'two-operation-group' in client.")
client: ClientType,
}
)
so using http://localhost:3000/client/structure/${clientParam}
to represent options.endpoint
makes more sense to me.
To me, domain name and ip:port is more like a host instead of endpoint.
Another point is, it may bring some implementation complexity to codegen if we use options.endpoint to represent the host instead of including the whole url prefix.
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.
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.
Sure thing! Thanks for the confirmation!
will handle the default value for parameterized host in another issue #2049 |
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.
Approved and thanks for identifying these details and we could improve in future pr.
I will merge this first, but @xirzec feel free to let me know if you have different thoughts about the above discussion, we can always revisit this. |
fix Azure/autorest.typescript#2032 (comment) fix Azure/autorest.typescript#2207 In this PR, 1. we allow customer to set their customized content type via options.contentType or content-type header. where options.contentType has higher priority than content-type header. 2. we set the default content-type to text/plain if it's non-json string in the body, but remain to be application/json if it's json string. --------- Co-authored-by: Deyaaeldeen Almahallawi <[email protected]>
pendIng Azure/cadl-ranch#425 and Azure/cadl-ranch#424
In this PR, it contains the following changes:
fixes #2040