-
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
Open
msyyc
wants to merge
16
commits into
main
Choose a base branch
from
add-multipartfile-model
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
+105
−0
Open
Changes from all commits
Commits
Show all changes
16 commits
Select commit
Hold shift + click to select a range
b2e27a9
add model for multipart file
msyyc 83f9978
changelog
msyyc 5ff3ab0
format
msyyc 8f24258
for ci
msyyc 9aaaf8d
Merge branch 'main' into add-multipartfile-model
msyyc 4e313dc
review
msyyc c9b2e75
Merge branch 'add-multipartfile-model' of https://github.com/Azure/ty…
msyyc f33c1b6
Merge branch 'main' into add-multipartfile-model
msyyc fee2a89
review
msyyc ca536c8
Merge branch 'main' into add-multipartfile-model
msyyc b424fcd
Merge branch 'add-multipartfile-model' of https://github.com/Azure/ty…
msyyc 4a54e81
Merge branch 'main' into add-multipartfile-model
msyyc 860df87
review
msyyc eedeae2
Merge branch 'main' into add-multipartfile-model
msyyc dffe8db
Merge branch 'main' into add-multipartfile-model
msyyc f175576
Merge branch 'main' into add-multipartfile-model
markcowl File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
7 changes: 7 additions & 0 deletions
7
.chronus/changes/add-multipartfile-model-2024-8-2-16-33-33.md
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,7 @@ | ||
--- | ||
changeKind: feature | ||
packages: | ||
- "@azure-tools/typespec-azure-core" | ||
--- | ||
|
||
Add model MultiPartFile with required `filename` and `contentType` |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,26 @@ | ||
import { getHttpPart, isOrExtendsHttpFile } from "@typespec/http"; | ||
import { ok, strictEqual } from "assert"; | ||
import { it } from "vitest"; | ||
import { getOperations } from "../test-host.js"; | ||
|
||
it("FileWithRequiredMetadata could be recognized by @typespec/http", async () => { | ||
const [operations, _, runner] = await getOperations( | ||
` | ||
model TestModel { | ||
file: HttpPart<FileWithRequiredMetadata>; | ||
}; | ||
|
||
@post op TestOperation(@header contentType: "multipart/form-date", @multipartBody body: TestModel): void; | ||
` | ||
); | ||
|
||
ok(operations.length === 1); | ||
ok(operations[0].parameters.body); | ||
strictEqual(operations[0].parameters.body.bodyKind, "multipart"); | ||
strictEqual(operations[0].parameters.body.type.kind, "Model"); | ||
const fileHttpPart = operations[0].parameters.body.type.properties.get("file"); | ||
ok(fileHttpPart); | ||
const file = getHttpPart(runner.program, fileHttpPart.type); | ||
ok(file !== undefined); | ||
ok(isOrExtendsHttpFile(runner.program, file.type)); | ||
msyyc marked this conversation as resolved.
Show resolved
Hide resolved
|
||
}); |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
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.
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.
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)
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)?