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

multipart/form-data request body schemas with additionalProperties are a bit confusing to use, also doesn't compile #596

Closed
jakepetroules opened this issue Jul 21, 2024 · 7 comments · Fixed by #597
Assignees
Labels
kind/bug Feature doesn't work as expected. status/triage Collecting information required to triage the issue.

Comments

@jakepetroules
Copy link
Member

Description

OpenAPI allows using the following construct to represent a [String: String] dictionary:

type: object
additionalProperties:
  type: string

However, this doesn't work correctly with multipart/form-data

Reproduction

Considering the following code...

Schema request body:

      requestBody:
        required: true
        content:
          multipart/form-data:
            schema:
              $ref: '#/components/schemas/JobParameters'

Schema components:

components:
  schemas:
    JobParameters:
      type: object
      additionalProperties:
        type: string
    JobParameters2:
      type: object
      additionalProperties:
        type: string

This generated the following Swift code:

            /// - Remark: Generated from `#/paths/job/{jobName}/buildWithParameters/POST/requestBody`.
            @frozen public enum Body: Sendable, Hashable {
                /// - Remark: Generated from `#/paths/job/{jobName}/buildWithParameters/POST/requestBody/content/multipart\/form-data`.
                case multipartForm(OpenAPIRuntime.MultipartBody<Components.Schemas.JobParameters>)
            }
/// Types generated from the components section of the OpenAPI document.
public enum Components {
    /// Types generated from the `#/components/schemas` section of the OpenAPI document.
    public enum Schemas {
        /// - Remark: Generated from `#/components/schemas/JobParameters`.
        @frozen public enum JobParameters: Sendable, Hashable {
            case additionalProperties(OpenAPIRuntime.MultipartDynamicallyNamedPart<Swift.String>)
        }
        /// - Remark: Generated from `#/components/schemas/JobParameters2`.
        public struct JobParameters2: Codable, Hashable, Sendable {
            /// A container of undocumented properties.
            public var additionalProperties: [String: Swift.String]
            /// Creates a new `JobParameters2`.
            ///
            /// - Parameters:
            ///   - additionalProperties: A container of undocumented properties.
            public init(additionalProperties: [String: Swift.String] = .init()) {
                self.additionalProperties = additionalProperties
            }
            public init(from decoder: any Decoder) throws {
                additionalProperties = try decoder.decodeAdditionalProperties(knownKeys: [])
            }
            public func encode(to encoder: any Encoder) throws {
                try encoder.encodeAdditionalProperties(additionalProperties)
            }
        }
        ...

Package version(s)

swift-openapi-generator 1.2.1
swift-openapi-runtime 1.4.0

Expected behavior

The inclusion of JobParameters2 is merely for illustration. That's what I would have expected JobParameters to look like.

It was initially quite unclear to me how to pass a [String: String] into this construct, as I would have expected something like this at the call site:

body: .multipartForm(.init(additionalProperties: parameters))

but you actually need to use this:

body: .multipartForm(.init(parameters.map {
            .additionalProperties(.init(payload: $0.value, name: $0.key))
        }))

However, the final problem is this:

                        encoding: { part in
                            switch part {
                            case let .additionalProperties(wrapped):
                                var headerFields: HTTPTypes.HTTPFields = .init()
                                let value = wrapped.payload
                                let body = try converter.setRequiredRequestBodyAsBinary(
                                    value,
                                    headerFields: &headerFields,
                                    contentType: "text/plain"
                                )
                                return .init(
                                    name: wrapped.name,
                                    filename: wrapped.filename,
                                    headerFields: headerFields,
                                    body: body
                                )
                            }
                        }

The generated code doesn't compile, as value is a String, but an HTTPBody is expected there.

I was able to work around the compile error by adding to my target:

@_spi(Generated) import OpenAPIRuntime
extension Converter {
    func setRequiredRequestBodyAsBinary(_ value: String, headerFields: inout HTTPFields, contentType: String) throws -> HTTPBody {
        let body = HTTPBody(value)
        headerFields[.contentType] = contentType
        if case let .known(length) = body.length { headerFields[.contentLength] = String(length) }
        return body
    }
}

That seems to work at runtime as well.

Environment

N/A

Additional information

No response

@jakepetroules jakepetroules added kind/bug Feature doesn't work as expected. status/triage Collecting information required to triage the issue. labels Jul 21, 2024
@czechboy0
Copy link
Contributor

Thanks @jakepetroules, let us take a look at this. The combination of multipart and typed additional properties might not be something I've seen before. But I do think your OpenAPI inputs are valid, so we should make this work.

@czechboy0
Copy link
Contributor

The inclusion of JobParameters2 is merely for illustration. That's what I would have expected JobParameters to look like.

Multipart has special handling in Swift OpenAPI Generator. The schema is generated differently because it's used as the top level object that describes the possible multipart parts. More on that in the original proposal: https://swiftpackageindex.com/apple/swift-openapi-generator/1.2.1/documentation/swift-openapi-generator/soar-0009#Different-enum-case-types

So JobParameters and JobParameters2 will not look the same, and both are generated correctly.

The issue I'm now looking into is why the code doesn't compile, that's the only problem I see right now.

@czechboy0
Copy link
Contributor

It was initially quite unclear to me how to pass a [String: String] into this construct, as I would have expected something like this at the call site:

body: .multipartForm(.init(additionalProperties: parameters))
but you actually need to use this:

body: .multipartForm(.init(parameters.map {
.additionalProperties(.init(payload: $0.value, name: $0.key))
}))

Yes, this is also expected. The top level object of a multipart schema is used as the blueprint that lists the various multipart parts that are recognized. More docs on that:

@czechboy0
Copy link
Contributor

Yikes, so this has at least one bug: the proposal dictates the additional properties enum case to be called "other", but it's only called that in some cases, but "additionalProperties" in other. That's my oversight, and I'll think about how to fix this without breaking backwards compatibility. That's issue 1. Still digging into issue 2, as reported by Jake.

@czechboy0
Copy link
Contributor

Ok issue 2 is a genuine bug in the mapping of type: string to the Swift type. Should not be Swift.String, as in raw bodies (which multipart parts are a type of), we represent raw strings as a streaming HTTPBody instead.

So once fixed, the underlying type will go from:

@frozen public enum JobParameters: Sendable, Hashable {
    case additionalProperties(OpenAPIRuntime.MultipartDynamicallyNamedPart<Swift.String>)
}

to

@frozen public enum JobParameters: Sendable, Hashable {
    case additionalProperties(OpenAPIRuntime.MultipartDynamicallyNamedPart<OpenAPIRuntime.HTTPBody>)
}

A reminder that you can easily go from HTTPBody to a String using https://swiftpackageindex.com/apple/swift-openapi-runtime/1.4.0/documentation/openapiruntime/swift/string/init(collecting:upto:)

And since this never compiled, I think it should be okay to do this "breaking change"? WDYT, @simonjbeaumont?

@czechboy0
Copy link
Contributor

czechboy0 commented Jul 22, 2024

@jakepetroules A fix incoming here: #597

The other issue (other vs additionalProperties) is not a blocker, and we'll fix that separately: #598

@czechboy0
Copy link
Contributor

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/bug Feature doesn't work as expected. status/triage Collecting information required to triage the issue.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants