apple / swift-openapi-generator

Generate Swift client and server code from an OpenAPI document.
https://swiftpackageindex.com/apple/swift-openapi-generator/documentation
Apache License 2.0
1.45k stars 120 forks source link

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

Closed jakepetroules closed 4 months ago

jakepetroules commented 4 months ago

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

czechboy0 commented 4 months ago

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 commented 4 months ago

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 commented 4 months ago

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 commented 4 months ago

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 commented 4 months ago

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 commented 4 months ago

@jakepetroules A fix incoming here: https://github.com/apple/swift-openapi-generator/pull/597

The other issue (other vs additionalProperties) is not a blocker, and we'll fix that separately: https://github.com/apple/swift-openapi-generator/issues/598

czechboy0 commented 4 months ago

@jakepetroules Ok fixed in 1.3.0: https://github.com/apple/swift-openapi-generator/releases/tag/1.3.0