Support multipart/form-data as opt-in feature#172
Merged
ainame merged 8 commits intoCreateAPI:mainCreateAPI/CreateAPI:mainfrom Dec 14, 2022
ainame:ai/multipart-formdataainame/CreateAPI:ai/multipart-formdataCopy head branch name to clipboard
Merged
Support multipart/form-data as opt-in feature#172ainame merged 8 commits intoCreateAPI:mainCreateAPI/CreateAPI:mainfrom ainame:ai/multipart-formdataainame/CreateAPI:ai/multipart-formdataCopy head branch name to clipboard
ainame merged 8 commits intoCreateAPI:mainCreateAPI/CreateAPI:mainfrom
ainame:ai/multipart-formdataainame/CreateAPI:ai/multipart-formdataCopy head branch name to clipboard
Conversation
1f9e77f to
31469ad
Compare
liamnichols
reviewed
Nov 29, 2022
Member
liamnichols
left a comment
There was a problem hiding this comment.
This is great! Thanks for the contribution 🚀 I just added some notes about naming of the property and docs, but I am looking forward to shipping this as part of 0.2.0 🙌
ainame
commented
Nov 30, 2022
Docs/ConfigOptions.md
Outdated
| - [entities](#renameentities) | ||
| - [operations](#renameoperations) | ||
| - [collectionElements](#renamecollectionelements) | ||
| - [useDataForMultipartFormDataRequestBody](#usedataformultipartformdatarequestbody) |
Contributor
Author
There was a problem hiding this comment.
Maybe the option should have been a part of paths 🤔
Contributor
Author
|
I noticed that the option only applies to |
liamnichols
approved these changes
Dec 13, 2022
Member
liamnichols
left a comment
There was a problem hiding this comment.
Thanks @ainame! These changes look good to me 🚀
Is there anything else that needs doing to support this or is it ready to merge?
Contributor
Author
|
No. The blocker was only me figuring out how to encode an |
This file contains hidden or 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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Background
I want to add multipart/form-data support to CreateAPI. The typical reason people need
multipart/form-datais the lack of native support for binary data in JSON format. Let’s say you have an endpoint to post not only a comment but also a photo, you would need to send both data in a payload and want to usemultipart/form-data. Looking at the codebase, currently, CreateAPI seems to postpone supporting the format. When an endpoint supports onlymultipart/form-datain the request body, it becomes Data type. That leaves CreateAPI unusable for such an endpoint.CreateAPI/Sources/CreateAPI/Generator/Generator+Paths.swift
Line 7 in 2084450
CreateAPI/Sources/CreateAPI/Generator/Generator+Schemas.swift
Line 6 in d9a7f7d
I can see why it wasn't supported initially because https://github.com/kean/Get doesn't support request body serialization other than
JSONEncoder.What I changed
I added a new option called
useStructuredMultipartFormDataRequestto have CreateAPI generate a structured request body, just like normalapplication/jsonbased requests. The default value isfalse. Nothing is affected by existing users here. Previously it was limited to just a Data but now it’s easier to compose data with a documented structure. The idea is that once this was implemented, the responsibility is down toCreateAPIusers to handle encoding request body. You would need something likeMultipartFormDataEncoderinstead ofJSONEncoderin their HTTP clients.This is an example of what a
multipart/form-datarequest looks like when the option is enabled.31469ad#diff-b304b36e06a0c0a28709263be23a00d9ab31e810f924da2a1468e1109e277227
And also, I changed the default type for binary data from
StringtoData. Despite the JSON format doesn’t support binary data,JSONEncoderandJSONDecoderdoes supportDatatype using Base64 encoding/decoding.Datawould still work with existingJSONEncoderandJSONDecoder.This part would be a breaking change. Given the version is still
0.1.1and theoretically, CreateAPI users have never utilised binary data in their request body just yet (sincemultipart/form-datasupport isn't in place!), I hope this is an acceptable change🙏Another thought on using
Datafor binary field. In amultipart/form-datarequest, we normally also need to specify a MIME type.Datatype alone can do nothing to identify the type. I think we can solve that by leveraging customstructtype having bothmimeTypeanddataproperties or assuming MIME type from magic bytes, like https://github.com/sendyhalim/Swime.