-
Notifications
You must be signed in to change notification settings - Fork 40
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
Add multipartfile model in Azure.Core #1458
base: main
Are you sure you want to change the base?
Conversation
@doc("Used in file part of multipart request body") | ||
model MultiPartFile extends File { | ||
filename: string; | ||
contentType: string; |
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.
context: #1380 (comment)
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.
Why contentType
is required? Are user required to fill-in the contentType; or it just mean client required to send the contentType, but value may not be filled by user?
Should we have e.g. contentType: "application/octet-stream"
, if this must be a required property?
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 comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
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.
Java would typically want
this contentType must to be sent on wire, but if user does not set anything, SDK would use "application/octet-stream" for File
I assume this cannot be expressed in TypeSpec without client default value.
But the point here, is that I didn't see why the contentType
property be different from the definition in TypeSpec.Http.File
. If contentType
be required here, it should be required in TypeSpec.Http.File
too.
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.
Share my two cents here.
Does the required/optional stand for the client code's optionality or payload's absence?
I would expect typespec definition here stands for the client code's optionality, which means if optional user don't need to fill-in that value. That is aligned with TypeSpec principle to represent some logic stuff of API. And we could mitigate the gaps according to RFC if contentType is absent. We do similar things in normal request if content type is missing in core(at least JS).
Do we prefer the contentType here is optional or required?
I prefer optional
, we have seen serveral mfd RPs and I didn't realize one which require contentType as required parameter yet, so I would expect optional contentType would be more common cases. Also contentType as optional would be more aligned with browser behavior in JS(see blob api).
One thing I am not sure is if service team wants this to be required, what is their best practice to define tsp? can they leverage our FileWithRequiredMetadata
? and is this wired we name this as WithRequiredMetadata
but actually only WithRequiredFilename
?
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 model is explicitly for a file that has more restrictions on it than a normal File
. It extends from File
but overrides the properties to be required instead of optional
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.
With more flexibility for customers, We could add 2 models for customer:
FileWithRequiredMetadata
: both filename and contentType are required (already in this PR)FileWithRequiredFileName
: only filename is required
Then azure service team could choose any one which meets their requirements.
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'd assume Java azure-core would only add one class.
loop @srnagar for opinion as well.
So far, I think I've seen 1 service require user input the contentType.
https://github.com/Azure/azure-rest-api-specs/blob/main/specification/translation/Azure.AI.DocumentTranslation/models.tsp#L11-L18
(this is before TypeSpec.Http.File, service asks SDK to allow set contentType)
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.
@srnagar What's your opinion about #1458 (comment)?
All changed packages have been documented.
Show changes
|
You can try these changes here
|
packages/typespec-client-generator-core/test/types/multipart-types.test.ts
Outdated
Show resolved
Hide resolved
…pespec-azure into add-multipartfile-model
…pespec-azure into add-multipartfile-model
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 Azure.Core part looks good. Need someone from TCGC to sign off the TCGC changes
fix #1380