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 the multipart content type #36

Closed
czechboy0 opened this issue May 25, 2023 · 15 comments · Fixed by #366
Closed

Support the multipart content type #36

czechboy0 opened this issue May 25, 2023 · 15 comments · Fixed by #366
Assignees
Labels
area/openapi Adding/updating a feature defined in OpenAPI. kind/feature New feature. status/needs-design Needs further discussion and a concrete proposal.
Milestone

Comments

@czechboy0
Copy link
Collaborator

czechboy0 commented May 25, 2023

https://github.com/OAI/OpenAPI-Specification/blob/main/versions/3.1.0.md#special-considerations-for-multipart-content

Let's do a proposal first, as there are a few ways the generated code could go here.

@czechboy0 czechboy0 added status/needs-design Needs further discussion and a concrete proposal. size/L Large task. (A couple of weeks of work.) area/openapi Adding/updating a feature defined in OpenAPI. labels May 25, 2023
@czechboy0 czechboy0 added this to the 1.0 milestone Aug 25, 2023
czechboy0 added a commit that referenced this issue Aug 30, 2023
[Generator] Integrate the new URI and String coders

### Motivation

Depends on runtime changes from apple/swift-openapi-runtime#45.

Up until now, we relied on a series of marker and helper protocols `_StringConvertible` and `_AutoLosslessStringConvertible` to handle converting between various types and their string representation.

This has been very manual and required a non-trivial amount of work to support any extra type, especially `Date` and generated string enums.

Well, turns out this was an unnecessarily difficult way to approach the problem - we had a better solution available for a long time - `Codable`.

Since all the generated types and all the built-in types we reference are already `Codable`, there is no need to reinvent a way to serialize and deserialize types, and we should just embrace it.

While a JSON encoder and decoder already exists in Foundation, we didn't have one handy for encoding to and from URIs (used by headers, query and path parameters), and raw string representation (using `LosslessStringConvertible`). We created those in the runtime library in PRs apple/swift-openapi-runtime#44 and apple/swift-openapi-runtime#41, and integrated them into our helper functions (which got significantly simplified this way) in apple/swift-openapi-runtime#45.

Out of scope of this PR, but this also opens the door to supporting URL form encoded bodies (#182), multipart (#36), and base64 (#11).

While this should be mostly invisible to our adopters, this refactoring creates space for implementing more complex features and overall simplifies our serialization story.

### Modifications

- Updated the generator to use the new helper functions.
- Updated the article about serialization, shows how we reduced the number of helper functions by moving to `Codable`.
- Set the `lineLength` to 120 on the formatter configuration, it was inconsistent with our `.swift-format` file, and lead to the soundness script trying to update the reference files, but then the reference tests were failing. Since we're planning to sync these in #40, this is a step closer to it, but it means that it's probably best to review this PR's diff with whitespace ignored.

### Result

Now the generated code uses the new helper functions, allowing us to delete all the deprecated helpers in 0.2.0.

### Test Plan

Updated file-based reference, snippet, and unit tests.


Reviewed by: glbrntt

Builds:
     ✔︎ pull request validation (5.8) - Build finished. 
     ✔︎ pull request validation (5.9) - Build finished. 
     ✔︎ pull request validation (docc test) - Build finished. 
     ✔︎ pull request validation (integration test) - Build finished. 
     ✔︎ pull request validation (nightly) - Build finished. 
     ✔︎ pull request validation (soundness) - Build finished. 

#226
@czechboy0
Copy link
Collaborator Author

This one needs a bit of design, but should be possible.

A simple idea:

  • generate a struct for the whole multipart payload
  • each property is one content type + payload, decoded as we'd normally decode it if it were a full body
  • this struct would not itself be Codable, as we'd generate custom code to decode/encode each of the properties, just like we do for type-safe properties for each operation

@czechboy0 czechboy0 added size/M Medium task. (A couple of days of work.) and removed size/L Large task. (A couple of weeks of work.) labels Sep 8, 2023
@czechboy0
Copy link
Collaborator Author

Additional reading: https://datatracker.ietf.org/doc/html/rfc7578

@czechboy0
Copy link
Collaborator Author

czechboy0 commented Sep 8, 2023

There are also headers associated with each part, not just content type + payload: https://github.com/OAI/OpenAPI-Specification/blob/main/versions/3.1.0.md#encoding-object-example

@czechboy0 czechboy0 added kind/feature New feature. and removed size/M Medium task. (A couple of days of work.) labels Sep 8, 2023
@czechboy0
Copy link
Collaborator Author

Can also be represented as an array property at the top level, at which point it could lend itself well to being a stream of values?

@czechboy0
Copy link
Collaborator Author

czechboy0 commented Sep 8, 2023

Should the parts be available as they become available, or all at the end in the generated struct? Seems we have to decide between more type-safety and delivering parts as they're ready instead of only returning them all at the end.

@czechboy0
Copy link
Collaborator Author

I'm working on this now, is taking a bit of time as it's a relatively large feature. A proposal with the new API/generated code is coming hopefully next week.

In the meantime, please drop here your use cases of multipart, such as:

  • request body or response body?
  • many parts of the same type (e.g. file), or heterogeneous, i.e. a raw log, a JSON object, and an image all in our request/response
  • what are you representing with the parts? (files, images, logs, JSON blobs, ...)

Thanks! 🙏

@anthonybishopric
Copy link

Hi @czechboy0 thank you for taking on this issue, as I'm currently running into a related issue with the generated client while performing a multipart upload.

I'm writing code that updates a user's profile photo, they are submitting the photo Data after retrieving that value from their library.

 let res = try await client.post_sol_users_sol__lcub_userId_rcub__sol_photo(
                .init(path: .init(userId: user.userId),
                      headers: .init(),
                      body: .multipartForm(data) // image data here
                     )
            )

Unfortunately the server is producing an error when I attempt to complete this operation:

no multipart boundary param in Content-Type

This makes sense, as the client currently hardcodes the content type to multipart/form-data. From the relevant part of the generated client;

                    request.body = try converter.setOptionalRequestBodyAsBinary(
                        value,
                        headerFields: &request.headerFields,
                        contentType: "multipart/form-data"
                    )

Even without fully fleshing out the multipart spec as outlined above, it would be great to address this in a point release of the client.

Thank you for taking the time to work on this!

@czechboy0
Copy link
Collaborator Author

Hi @anthonybishopric,

unfortunately this is expected, as what you're sending is not a valid multipart request.

While I hope to have a proposal up for multipart support this week, and hopefully have everything landed by end of next week, here's a temporary workaround:

  1. Write a client middleware that adds a constant "boundary" parameter to the content type header before it's sent to the server.
  2. Change the raw body you're providing by making it look like a multipart request, i.e. boundary to start, part headers section, then your image body, then an ending boundary.

Lmk if you need more details here.

@czechboy0
Copy link
Collaborator Author

czechboy0 commented Nov 7, 2023

More details.

In your middleware, you'd modify the header from

content-type: multipart/form-data

to

content-type: multipart/form-data; boundary=__X_SWIFT_OPENAPI_GENERATOR_BOUNDARY__

And then the body would need to look something like:

--__X_SWIFT_OPENAPI_GENERATOR_BOUNDARY__
Content-Disposition: form-data; name="photo"

<raw photo bytes here>
--__X_SWIFT_OPENAPI_GENERATOR_BOUNDARY__--

Note that each newline needs to be a CRLF, not just a simple newline.

Hopefully this should be accepted by the server and can serve as a workaround for the new few weeks before proper multipart support lands.

@anthonybishopric
Copy link

Thanks for the suggestions @czechboy0! I found a workaround that is okay for the time being. I changed the upload parameters to accept an application/octet-stream instead and simply pass data unadorned.

      requestBody:
        content:
          application/octet-stream:
            schema:
              type: string
              format: binary

This only works because in my particular case I don't have other metadata to upload in the body, so I will keep watching progress on this issue.

@czechboy0
Copy link
Collaborator Author

Oh yeah if you only have a single payload, don't use multipart, use "single"part 🙂 (what you're doing, just one body payload).

@czechboy0
Copy link
Collaborator Author

Proposal: #369

Please, have a read and chime in with questions, comments.

@czechboy0
Copy link
Collaborator Author

@czechboy0
Copy link
Collaborator Author

All the runtime changes landed in main, will be released in 1.0.0-alpha.1 next week.

Generator changes are in a PR: #366.

czechboy0 added a commit that referenced this issue Nov 24, 2023
### Motivation

Implement SOAR-0009. Fixes #36.

### Modifications

It's a large diff, and adds some complexity, however I tried to avoid
needlessly complicating existing code and opted for duplicating parts of
the code specifically for multipart, for easier reasoning.

The implementation very much maps to the changes described in the
proposal, which I won't repeat here.

Some notable highlights before you dive in:
- Testing is mainly done through snippet tests, and I added one request
and one response operation to make sure things work end to end. Maybe
review those first.
- The logic for generating request and response bodies got slightly
extended to detect when we're generating multipart content, and branches
off to bespoke code specifically for multipart, all in the new
`Multipart` directory.
- Made a few related changes, but tried to keep this isolated to
multipart as much as reasonable.
- Let me know which parts I should elaborate on, I'm happy to explain in
more detail.

### Result

Multipart content now works, as proposed in SOAR-0009.

### Test Plan

Added a bunch of snippet tests and two new file-based reference test
operations, including petstore consumer tests to verify that it all
works at runtime as well.
@czechboy0
Copy link
Collaborator Author

Runtime changes landed, all going out next week.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/openapi Adding/updating a feature defined in OpenAPI. kind/feature New feature. status/needs-design Needs further discussion and a concrete proposal.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants