Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Appearance settings

Comments

Close side panel

Support multipart/form-data as opt-in feature#172

Merged
ainame merged 8 commits intoCreateAPI:mainCreateAPI/CreateAPI:mainfrom
ainame:ai/multipart-formdataainame/CreateAPI:ai/multipart-formdataCopy head branch name to clipboard
Dec 14, 2022
Merged

Support multipart/form-data as opt-in feature#172
ainame merged 8 commits intoCreateAPI:mainCreateAPI/CreateAPI:mainfrom
ainame:ai/multipart-formdataainame/CreateAPI:ai/multipart-formdataCopy head branch name to clipboard

Conversation

@ainame
Copy link
Contributor

@ainame ainame commented Nov 28, 2022

Background

I want to add multipart/form-data support to CreateAPI. The typical reason people need multipart/form-data is 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 use multipart/form-data. Looking at the codebase, currently, CreateAPI seems to postpone supporting the format. When an endpoint supports only multipart/form-data in the request body, it becomes Data type. That leaves CreateAPI unusable for such an endpoint.

// TODO: Add support for multipart form data (currently defaults to `Data`)

// TODO: stirng with format "binary"?

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 useStructuredMultipartFormDataRequest to have CreateAPI generate a structured request body, just like normal application/json based requests. The default value is false. 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 to CreateAPI users to handle encoding request body. You would need something like MultipartFormDataEncoder instead of JSONEncoder in their HTTP clients.

This is an example of what a multipart/form-data request looks like when the option is enabled.
31469ad#diff-b304b36e06a0c0a28709263be23a00d9ab31e810f924da2a1468e1109e277227

And also, I changed the default type for binary data from String to Data. Despite the JSON format doesn’t support binary data, JSONEncoder and JSONDecoder does support Data type using Base64 encoding/decoding. Data would still work with existing JSONEncoder and JSONDecoder.

This part would be a breaking change. Given the version is still 0.1.1 and theoretically, CreateAPI users have never utilised binary data in their request body just yet (since multipart/form-data support isn't in place!), I hope this is an acceptable change🙏


Another thought on using Data for binary field. In a multipart/form-data request, we normally also need to specify a MIME type. Data type alone can do nothing to identify the type. I think we can solve that by leveraging custom struct type having both mimeType and data properties or assuming MIME type from magic bytes, like https://github.com/sendyhalim/Swime.

@ainame ainame marked this pull request as ready for review November 28, 2022 15:57
@ainame ainame marked this pull request as draft November 28, 2022 15:57
@ainame ainame force-pushed the ai/multipart-formdata branch from 1f9e77f to 31469ad Compare November 28, 2022 16:02
@ainame ainame marked this pull request as ready for review November 28, 2022 16:27
@liamnichols liamnichols self-requested a review November 29, 2022 10:47
Copy link
Member

@liamnichols liamnichols left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 🙌

Sources/CreateOptions/ConfigOptions.swift Outdated Show resolved Hide resolved
Sources/CreateOptions/ConfigOptions.swift Outdated Show resolved Hide resolved
- [entities](#renameentities)
- [operations](#renameoperations)
- [collectionElements](#renamecollectionelements)
- [useDataForMultipartFormDataRequestBody](#usedataformultipartformdatarequestbody)
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe the option should have been a part of paths 🤔

@ainame
Copy link
Contributor Author

ainame commented Nov 30, 2022

I noticed that the option only applies to Paths, so I moved it to under paths section.

23d4a39

@ainame ainame requested a review from liamnichols November 30, 2022 17:13
Copy link
Member

@liamnichols liamnichols left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @ainame! These changes look good to me 🚀

Is there anything else that needs doing to support this or is it ready to merge?

@ainame
Copy link
Contributor Author

ainame commented Dec 14, 2022

No. The blocker was only me figuring out how to encode an Encodable struct to multipart/form-data format. I was able to do that with this change. I'm confident it's working now.

@ainame ainame merged commit da8730b into CreateAPI:main Dec 14, 2022
@ainame ainame deleted the ai/multipart-formdata branch December 14, 2022 10:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants

Morty Proxy This is a proxified and sanitized view of the page, visit original site.