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

SOAR-0009 - Typesafe multipart with streaming #369

Merged
merged 4 commits into from
Nov 24, 2023

Conversation

czechboy0
Copy link
Contributor

@czechboy0 czechboy0 commented Nov 8, 2023

@simonjbeaumont
Copy link
Collaborator

This is now in review and will run until Nov 15th. Forum link: https://forums.swift.org/t/proposal-soar-0009-type-safe-streaming-multipart-support/68331.

@nkarczmarzyk
Copy link

+1 It would be an enrichment to have the multipart generator merged in a timely manner. Good work!

@simonjbeaumont
Copy link
Collaborator

+1 It would be an enrichment to have the multipart generator merged in a timely manner. Good work!

@nkarczmarzyk Just to keep you reassured, the work has already begun and several PRs have been landed into the runtime package to provide the necessary support. The generator PR will follow.

@czechboy0
Copy link
Contributor Author

@nkarczmarzyk As Si said, this work is already ongoing. I'm glad you're looking forward to it, it's a large feature with substantial changes in the generator, so once we release 1.0.0-alpha.1 next week, it would be great if you could give it a try in your project - we want to catch any final issues before publishing 1.0.0 two weeks later.

@simonjbeaumont - can you update the review please? It already finished, so this and the forums post can be updated.

@nkarczmarzyk
Copy link

@nkarczmarzyk As Si said, this work is already ongoing. I'm glad you're looking forward to it, it's a large feature with substantial changes in the generator, so once we release 1.0.0-alpha.1 next week, it would be great if you could give it a try in your project - we want to catch any final issues before publishing 1.0.0 two weeks later.

@simonjbeaumont - can you update the review please? It already finished, so this and the forums post can be updated.

I am looking forward to it. I also took a look on the feature (wip) PR #366 . Great work in there! Seems like a very complex topic. When it is ready for testing, I will give it a try and give feedback. Until then I am trying to implement a workaround.

@czechboy0
Copy link
Contributor Author

czechboy0 commented Nov 24, 2023

@nkarczmarzyk As Si said, this work is already ongoing. I'm glad you're looking forward to it, it's a large feature with substantial changes in the generator, so once we release 1.0.0-alpha.1 next week, it would be great if you could give it a try in your project - we want to catch any final issues before publishing 1.0.0 two weeks later.
@simonjbeaumont - can you update the review please? It already finished, so this and the forums post can be updated.

I am looking forward to it. I also took a look on the feature (wip) PR #366 . Great work in there! Seems like a very complex topic. When it is ready for testing, I will give it a try and give feedback. Until then I am trying to implement a workaround.

I just got everything working end to end, so you can try the hd-multipart branch of the generator + hd-multipart-converter-methods branch of the runtime library. I'll work to clean up the code and start landing it today.

@nkarczmarzyk
Copy link

@czechboy0 I have found https://github.com/czechboy0/swift-openapi-generator/tree/hd-multipart for the generator. Could you please give me an hint where to find hd-multipart-converter-methods for the runtime?

@czechboy0
Copy link
Contributor Author

@czechboy0 I have found https://github.com/czechboy0/swift-openapi-generator/tree/hd-multipart for the generator. Could you please give me an hint where to find hd-multipart-converter-methods for the runtime?

Since I wrote that comment, we already landed it all on main - so you can use that branch.

@nkarczmarzyk
Copy link

nkarczmarzyk commented Nov 24, 2023

@czechboy0

I really don't want to blow up this pr but currently it seems like something is broken. Using latest of your forked generator (hd-multipart branch) and latest runtime I am getting the following result.

open-api.yaml (v3.0.1):

paths:
[...]
  /Post/addFile:
    post:
      tags:
        - Post
      requestBody:
        content:
          multipart/form-data:
            schema:
              required:
                - File
                - PostId
                - Type
              type: object
              properties:
                PostId:
                  type: string
                  format: uuid
                File:
                  type: string
                  format: binary
                Type:
                  $ref: '#/components/schemas/FileType'
            encoding:
              PostId:
                style: form
              File:
                style: form
              Type:
                style: form
      responses:
        '200':
          description: Success
[...]
components:
  schemas:
    FileType:
      enum:
        - Picture
        - Audio
      type: string
[...]

Generated type:

[...]
    /// - Remark: HTTP `POST /Post/addFile`.
    /// - Remark: Generated from `#/paths//Post/addFile/post`.
    public enum post_sol_Post_sol_addFile {
        public static let id: Swift.String = "post/Post/addFile"
        public struct Input: Sendable, Hashable {
            /// Creates a new `Input`.
            public init() {}
        }
[...]

It is also possible that I am doing something wrong. I am quite new to the world of swift.

@czechboy0
Copy link
Contributor Author

@nkarczmarzyk Try to remove the encoding key, since none of the 3 parts are URL-encoded strings, this won't have an effect. If it still doesn't work then, I'll try to repro locally.

@czechboy0
Copy link
Contributor Author

czechboy0 commented Nov 24, 2023

Also, the request body must be marked as required, multipart bodies cannot be optional (which is the default).

@czechboy0
Copy link
Contributor Author

One more question, is the generator emitting any diagnostics when running? That could hint at why it's skipping the content.

@czechboy0
Copy link
Contributor Author

@nkarczmarzyk I verified in your example, that you just need to add required: true to the request body and then it works.

@nkarczmarzyk
Copy link

nkarczmarzyk commented Nov 24, 2023

@czechboy0 Thanks for the advice. Setting required: true results in a correct generation. I will continue testing 👍

@czechboy0 czechboy0 merged commit e5ce6ac into apple:main Nov 24, 2023
8 checks passed
@czechboy0 czechboy0 deleted the hd-soar-0009 branch November 24, 2023 12:57
@nkarczmarzyk
Copy link

Hey @czechboy0 hope you had a great weekend. I am off testing again. Currently its very complicated to initialise the components in the correct way but I will report if I got more results.

Currently my code looks like this:

        let body = HTTPBody()

        let postIdPayload: MultipartPart<Operations.post_sol_Post_sol_addFile.Input.Body.multipartFormPayload.PostIdPayload> = MultipartPart(payload: Operations.post_sol_Post_sol_addFile.Input.Body.multipartFormPayload.PostIdPayload(body: body))
        
        let filePayload: MultipartPart<Operations.post_sol_Post_sol_addFile.Input.Body.multipartFormPayload.FilePayload> = MultipartPart(payload: Operations.post_sol_Post_sol_addFile.Input.Body.multipartFormPayload.FilePayload(body: body))
        
        let typePayload: MultipartPart<Operations.post_sol_Post_sol_addFile.Input.Body.multipartFormPayload._TypePayload> = MultipartPart(payload: Operations.post_sol_Post_sol_addFile.Input.Body.multipartFormPayload._TypePayload(body: body))

        let multipartBody: OpenAPIRuntime.MultipartBody<Operations.post_sol_Post_sol_addFile.Input.Body.multipartFormPayload> = .init(
            [.PostId(postIdPayload), .File(filePayload), ._Type(typePayload)])
        
        let response = try await ClientProvider.sharedClient.post_sol_Post_sol_addFile(body: .multipartForm(multipartBody))

@nkarczmarzyk
Copy link

nkarczmarzyk commented Nov 27, 2023

@czechboy0 I just found out that you are not passing the correct parameter type in https://github.com/apple/swift-openapi-generator/blob/main/Sources/swift-openapi-generator/Documentation.docc/Proposals/SOAR-0009.md.

Instead of:
let response = try await client.uploadPhoto(body: multipartBody)

It should be:
let response = try await client.uploadPhoto(body: .multipartForm(multipartBody))

@nkarczmarzyk
Copy link

@czechboy0
And actually I do not fully understand the aspect of type safety. The generated types look like this:

[...]
                    public struct PostIdPayload: Sendable, Hashable {
                        public var body: OpenAPIRuntime.HTTPBody
                        /// Creates a new `PostIdPayload`.
                        ///
                        /// - Parameters:
                        ///   - body:
                        public init(body: OpenAPIRuntime.HTTPBody) {
                            self.body = body
                        }
                    }
                    case PostId(OpenAPIRuntime.MultipartPart<Operations.post_sol_Post_sol_addFile.Input.Body.multipartFormPayload.PostIdPayload>)
                    /// - Remark: Generated from `#/paths/Post/addFile/POST/requestBody/multipartForm/File`.
                    public struct FilePayload: Sendable, Hashable {
                        public var body: OpenAPIRuntime.HTTPBody
                        /// Creates a new `FilePayload`.
                        ///
                        /// - Parameters:
                        ///   - body:
                        public init(body: OpenAPIRuntime.HTTPBody) {
                            self.body = body
                        }
                    }
                    case File(OpenAPIRuntime.MultipartPart<Operations.post_sol_Post_sol_addFile.Input.Body.multipartFormPayload.FilePayload>)
                    /// - Remark: Generated from `#/paths/Post/addFile/POST/requestBody/multipartForm/Type`.
                    public struct _TypePayload: Sendable, Hashable {
                        public var body: OpenAPIRuntime.HTTPBody
                        /// Creates a new `_TypePayload`.
                        ///
                        /// - Parameters:
                        ///   - body:
                        public init(body: OpenAPIRuntime.HTTPBody) {
                            self.body = body
                        }
                    }
                    case _Type(OpenAPIRuntime.MultipartPart<Operations.post_sol_Post_sol_addFile.Input.Body.multipartFormPayload._TypePayload>)
[...]

Couldn't I just pass any HTTPBody as payload, which is independent of the OpenAPI definition type?

@czechboy0
Copy link
Contributor Author

Streaming types, such as raw strings and raw bytes are represented by the HTTPBody type. If you instead had, for example, a JSON object as a multipart part, you'd get a fully typesafe Codable struct generated.

@czechboy0
Copy link
Contributor Author

czechboy0 commented Nov 27, 2023

Hey @czechboy0 hope you had a great weekend. I am off testing again. Currently its very complicated to initialise the components in the correct way but I will report if I got more results.

Currently my code looks like this:

        let body = HTTPBody()

        let postIdPayload: MultipartPart<Operations.post_sol_Post_sol_addFile.Input.Body.multipartFormPayload.PostIdPayload> = MultipartPart(payload: Operations.post_sol_Post_sol_addFile.Input.Body.multipartFormPayload.PostIdPayload(body: body))
        
        let filePayload: MultipartPart<Operations.post_sol_Post_sol_addFile.Input.Body.multipartFormPayload.FilePayload> = MultipartPart(payload: Operations.post_sol_Post_sol_addFile.Input.Body.multipartFormPayload.FilePayload(body: body))
        
        let typePayload: MultipartPart<Operations.post_sol_Post_sol_addFile.Input.Body.multipartFormPayload._TypePayload> = MultipartPart(payload: Operations.post_sol_Post_sol_addFile.Input.Body.multipartFormPayload._TypePayload(body: body))

        let multipartBody: OpenAPIRuntime.MultipartBody<Operations.post_sol_Post_sol_addFile.Input.Body.multipartFormPayload> = .init(
            [.PostId(postIdPayload), .File(filePayload), ._Type(typePayload)])
        
        let response = try await ClientProvider.sharedClient.post_sol_Post_sol_addFile(body: .multipartForm(multipartBody))

Right, I'd probably write it like this instead:

let response = try await ClientProvider.sharedClient.post_sol_Post_sol_addFile(body: .multipartForm([
    .PostId(.init(payload: .init(body: "my-post-id"))),
    .File(.init(payload: .init(body: "<file contents go here>"), filename: "file.txt")),
    ._Type(.init(payload: .init(body: "my-type"))),
]))

@czechboy0 czechboy0 added the semver/none No version bump required. label Nov 29, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
semver/none No version bump required.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants