Skip to content
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

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

Merged
merged 8 commits into from Dec 14, 2022

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 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
@@ -103,6 +103,7 @@ Below you can find the complete documentation for all available options.
- [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

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.

None yet

2 participants