-
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
Enable the batch tsp & chat api tsp #2090
Conversation
return context.path("/", ).post({...operationOptionsToRequestParameters(options), | ||
headers: { | ||
"required-header": requiredHeader, | ||
...(options?.optionalHeader !== undefined ? {"optional-header": options?.optionalHeader} : {}) |
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 kind of error will we get if it's undefined in the hearders?
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.
If it's from core client rest, should we improve the core ?
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.
The header definition is a dictionary of string | number | boolean
items. If the value is optional which could be undefined
, it would cause compile error.
headers?: RawHttpHeadersInput;
...
export declare type RawHttpHeadersInput = Record<string, string | number | boolean>;
I think the core interface is accurate which only allows valid value as record items. I think it's the codegen's role to prepare headers object correctly.
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.
Have you considered null in header ?
In my opinion, even if codegen has a way to prepare headers object correctly, we can not stop customer from pass undefined/null with as any, are you sure it can work as expected in such case ? After all, core is the one handle the ultimate request we send, I think it should be robust.
I am fine to temporary fix this in the codegen to unblock our customer, but I think we need some discussion here.
@joheredi @xirzec Could you help share your thoughts here ? Thanks
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 general, it's recommended to work with strings in HTTP headers because the HTTP specification (RFC 7230) defines header fields as comprising a field name and a string value. While you can represent numbers, booleans, or other data as strings in headers, the native format for header values is string-based.
If we set undefined or null it's a string containing "undefined" or "null" that goes out to the wire. We support numbers and boolean as they can be serialized as strings easily, but on the wire it is strings.
I would prefer to not support null and if the value is set to undefined not included it. If customers want to pass those values they can do it as strings and would have the same effect if we supported them.
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 found in openapi3, it has specified that headers could be primitives, arrays, objects here. https://swagger.io/docs/specification/describing-parameters/
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.
per https://swagger.io/docs/specification/serialization/ it seems that Header parameters always use the simple style, that is, comma-separated values.
so arrays/objects are always flattened into a csv of some sort.
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.
that's is the serialization, but in the generated code, I don't think we should just generate string at least in Modular ?
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.
To clarify something after having some offline discussion with @MaryGao,
- For complex arrays and objects, codegen should generate the serialize helper functions, for modular, we should leverage the rlc helper functions to build the object/array as string.
- For primitive types, boolean/number/string, core can handle this now.
- We need to generate compilable code if it's undefined or null in typespec, which means we need to have a serializer helper function for undefined or null as there's type mismatch. And because we currently don't have model based overall serialier yet, we will have to put them inline the send request function body.
@xirzec @joheredi let me know if you have any concerns about it. Thanks
Also, sorry about the previous confusions.
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 for Qiaoqiao's summary and I supported the omit logic for both null and undefined in latest pr and feel free to review it again.
* - undefined, no credentialScopes and relevant settings would be generated | ||
* - [], which means we would generate TokenCredential but no credentialScopes and relevant settings | ||
* - ["..."], which means we would generate credentialScopes and relevant settings with the given values | ||
*/ |
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.
Now we handle the undefined
and empty array as different cases.
undefined
means we don't have OAuth2 defined;
[]
means we have OAuth2 defined but with empty scopes.
Previously we treat the above two cases the same so all of them will not generate TokenCredential in constructor. But now they are different. The first would not but the latter would generate.
Please notice that we will not set the credentialScopes default value as []
. If the value is absent the core would populate with {endpoint}/.default
automatically.
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.
This is another place where we should consider if we are generating an Azure SDK or not since the {endpoint}/.default
value only makes sense for Azure services.
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.
Haha, there may be another question - how to handle the empty scopes for non-Azure? How about explicitly set this as an empty array? Do we need to change the default logic for in @typespec/ts-http-runtime
?
credentialScopes: []
But I think this topic would not block current pr considering we have a on-going one for integration #2083.
param: Parameter | ||
): string { | ||
if (!param.optional && param.type.nullable === true) { | ||
reportDiagnostic(program, { |
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.
Report warning if null value is allowed in required header item.
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.
LGTM, left some minor comments
undefined
andnull
are not allowed as header value which would happen for optional headers so adding conditions to omit these values${baseurl}/.default