Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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?
Add multipartfile model in Azure.Core #1458
Changes from 5 commits
b2e27a9
83f9978
5ff3ab0
8f24258
9aaaf8d
4e313dc
c9b2e75
f33c1b6
fee2a89
ca536c8
b424fcd
4a54e81
860df87
eedeae2
dffe8db
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
Should we have e.g. contentType: "application/octet-stream", if this must be a required property?
-> It is OK for Python since the default content-type in RFC is also
application/octect-stream
. @chunyu3 / @MaryGao / @qiaozha what's your opinion?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
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 inTypeSpec.Http.File
. IfcontentType
be required here, it should be required inTypeSpec.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.
So Java hopes the core model only change
filename
to required.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 asWithRequiredMetadata
but actually onlyWithRequiredFilename
?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 fromFile
but overrides the properties to be required instead of optionalThere 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 requiredThen 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)