Update dependencies and add http part support fo ToDo #32
Conversation
| @body contents: TodoAttachment, | ||
| ): WithStandardErrors<NoContentResponse | NotFoundErrorResponse>; | ||
| @body contents: TodoFileAttachment, | ||
| ): WithStandardErrors<NoContentResponse | NotFoundResponse>; |
There was a problem hiding this comment.
Yeah, that was just a mistake, good catch!
| @post | ||
| op createForm( | ||
| @header contentType: "multipart/form-data", | ||
| @body body: ToDoItemMultipartRequest, |
There was a problem hiding this comment.
@multipartBody?
Also attachments?: HttpPart<TodoAttachment[]>; within ToDoItemMultipartRequest seems not right. For multiple parts on a same field name (especially for File), it would be HttpPart<>[].
There was a problem hiding this comment.
@markcowl Since language SDK including Python/Java doesn't support union mixed File with common model in multipart request body, we hope to abandon Union in multipart request like:
@sharedRoute
@post
op createFormFile(
@header contentType: "multipart/form-data",
@multipartBody body: {item: HttpPart<TodoItem>; attachments?: HttpPart<TodoFileAttachment>[];},
): WithStandardErrors<TodoItem | InvalidTodoItem>;
@sharedRoute
@post
op createFormUrl(
@header contentType: "multipart/form-data",
@multipartBody body: {item: HttpPart<TodoItem>; attachments?: HttpPart<TodoUrlAttachment>[];},
): WithStandardErrors<TodoItem | InvalidTodoItem>;
There was a problem hiding this comment.
On the parts, I think you are correct, I was not confident in exactly how to model this, but an array of simple types parts rather than a single part with multiple files makes much more sense.
Not allowing the union is semantically different though, and would disallow mixing url attachments with File attachments. But for this early version of the demo, we can simplify. I think we might limit the multipart attachment to only files, and allow files or Urls through the json endpoint
There was a problem hiding this comment.
@markcowl
I think we might limit the multipart attachment to only files, and allow files or Urls through the json endpoint
-> That makes sense and we can update
typespec-e2e-demo/todoApp/spec/main.tsp
Lines 82 to 85 in 052143e
model ToDoItemMultipartRequest {
item: HttpPart<TodoItem>;
attachments?: HttpPart<TodoFileAttachment>[];
}
``
| @path itemId: TodoItem.id, | ||
| @body contents: TodoAttachment, | ||
| ): WithStandardErrors<NoContentResponse | NotFoundErrorResponse>; | ||
| @body contents: TodoFileAttachment, |
There was a problem hiding this comment.
@multipartBody body: { contents: HttpPart<TodoFileAttachment> }
There was a problem hiding this comment.
yeah, I think the original operation has ambiguity where it stands for 1) one part with contentsand a File type OR 2) three parts with contentType, filename or contents?
https://github.com/microsoft/typespec/blob/main/packages/http/lib/http.tsp#L110-L114
There was a problem hiding this comment.
FileType can be a part of multipart payload but can't be the whole payload type. So current version is not valid case while https://github.com/allenjzhang/typespec-e2e-demo/pull/32/files#r1883276729 is valid.
There was a problem hiding this comment.
I was wondering if we have any limitation for below case? If no, it would be a valid spec standing for a mfd operation which will upload three parts
- string part: contentType
- string part: filename
- bytes(file) part: contents
@post
op createFileAttachment(
@header contentType: "multipart/form-data",
@body contents: File,
): NoContentResponse | NotFoundResponse;
There was a problem hiding this comment.
I was wondering if we have any limitation for below case? If no, it would be a valid spec standing for a mfd operation which will upload three parts
- string part: contentType
- string part: filename
- bytes(file) part: contents
@post op createFileAttachment( @header contentType: "multipart/form-data", @body contents: File, ): NoContentResponse | NotFoundResponse;
As I said in https://github.com/allenjzhang/typespec-e2e-demo/pull/32/files/052143e392c541fd03a2eaffd7a80f689b499720#r1887849929, I think we should follow @weidongxu-microsoft's comment https://github.com/allenjzhang/typespec-e2e-demo/pull/32/files/052143e392c541fd03a2eaffd7a80f689b499720#r1883276729
| @@ -79,6 +79,11 @@ model TodoItem { | |||
| @visibility("create") _dummy?: string; | |||
| } | |||
|
|
|||
| model ToDoItemMultipartRequest { | |||
| item: HttpPart<TodoItem>; | |||
| attachments?: HttpPart<TodoAttachment[]>; | |||
There was a problem hiding this comment.
@markcowl @lmazuel @bterlson Could you help confirm if we allow to share models among mfd and non-mfd ops: microsoft/typespec#5376?
Also I created an issue to clarify the todo spec's scope: #34.
There was a problem hiding this comment.
I don't think we should share the model, it's quite unclear what an operation that takes a JSON in would understand to do with an HttpPart type. I think we discussed before that if HttpPart is used, then it's a MFD usage, and that's all.
There was a problem hiding this comment.
In todo spec the shared model is not ToDoItemMultipartRequest or HttpPart but TodoAttachment or TodoItem in HttpPart.
There was a problem hiding this comment.
Personal opinion: TypeSpec File (or model extends from it) should not appear to JSON model.
It should only be used together with HttpPart as HttpPart<File>, at the property of a multipart payload, e.g.
@multipartBody body: {
file: HttpPart<File>;
}It is possible that certain JSON model do have the same 3 properties, but I think in such case, dev better just write a new model.
No description provided.