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

Add `consuming` Modifier to Middleware Intercept Method Parameters #599

Closed ojun9 closed 4 months ago

ojun9 commented 4 months ago

Motivation

Currently, the Swift OpenAPI Generator's ClientMiddleware protocol does not utilize the consuming modifier for its intercept method parameters. This results in the necessity of local copies of request and body objects when modifying them, which can lead to increased memory usage and reduced performance. For example, developers currently have to create a local copy of the request object to modify its properties:

func intercept(
    _ request: HTTPRequest,
    body: HTTPBody?,
    baseURL: URL,
    operationID: String,
    next: @Sendable (HTTPRequest, HTTPBody?, URL) async throws -> (HTTPResponse, HTTPBody?)
) async throws -> (HTTPResponse, HTTPBody?) {

    var request = request
    request.headerFields[.authorization] = "Bearer \(token)"
}

Proposed solution

Introduce the consuming modifier to the HTTPRequest and HTTPBody parameters in the ClientMiddleware's intercept method. By introducing the consuming modifier, the method can directly modify the request and body objects without creating local copies, thus improving efficiency. The updated method signature would look like this:

func intercept(
    _ request: consuming HTTPRequest,
    body: consuming HTTPBody?,
    baseURL: URL,
    operationID: String,
    next: @Sendable (HTTPRequest, HTTPBody?, URL) async throws -> (HTTPResponse, HTTPBody?)
) async throws -> (HTTPResponse, HTTPBody?) {

    request.headerFields[.authorization] = "Bearer \(token)"  // consuming modifier allows direct modification
}

Alternatives considered

No response

Additional information

No response

czechboy0 commented 4 months ago

Hi @ojun9, this is an interesting idea. A few questions:

  1. Is this a backwards-compatible change?
  2. Would this mean that when chaining middlewares, one middleware, after calling next, couldn't use the request anymore? That could be problematic in e.g. the logging middleware: https://github.com/apple/swift-openapi-generator/blob/edf766a8b999ea120f1f5db59eebd1feb10f4508/Examples/logging-middleware-swift-log-example/Sources/LoggingMiddleware/LoggingMiddleware.swift#L44
  3. What about the parameters of the next closure?
czechboy0 commented 4 months ago

Some more info here: https://github.com/swiftlang/swift-evolution/blob/main/proposals/0377-parameter-ownership-modifiers.md#source-compatibility

Changing parameter modifiers from borrowing to consuming may however break source of any client code that also adopts those parameter modifiers, since the change may affect where copies need to occur in the caller. Going from consuming to borrowing however should generally not be source-breaking for a copyable type. A change in either direction is source-breaking if the parameter type is noncopyable.

Since request/response types are not noncopyable, I guess the last sentence doesn't really apply here.

But the previous do apply - are you also reading it to mean that this could break working code for folks?

FranzBusch commented 4 months ago

You should be able to adopt consuming on the middleware protocols. Since your input/output is Copyable the compiler can just insert a defensive copy before passing the value on. Every Copyable type can be passed to a consuming method. The parameter of the next closure would also need to adopt consuming.

Now the other problem at hand is if all supported Swift versions of this repo support consuming already.

czechboy0 commented 4 months ago

Now the other problem at hand is if all supported Swift versions of this repo support consuming already.

Yes, we are 5.9+: https://github.com/swiftlang/swift-evolution/blob/main/proposals/0377-parameter-ownership-modifiers.md

Every Copyable type can be passed to a consuming method

Right, but what about this sentence?

Changing parameter modifiers from borrowing to consuming may however break source of any client code that also adopts those parameter modifiers, since the change may affect where copies need to occur in the caller.

Doesn't that mean that if someone wrote a generic method and used these annotations, and uses it to call the method, that it might actually break their build? Or is there no way to compose previously working Swift code that used these protocols that'd break by adding the annotations?

ojun9 commented 4 months ago

Thank you, @FranzBusch

Now the other problem at hand is if all supported Swift versions of this repo support consuming already.

consuming modifier has already been introduced in Swift 5.9. And the minimum Swift version for Swift OpenAPI Generator is Swift 5.9.

ojun9 commented 4 months ago

@czechboy0

Certainly, when passing request itself to other methods or functions, instead of just traversing within request, it will be consumed at that point, leading to potential issues. Therefore, rather than attaching the consuming modifier to the protocol itself, it seems more appropriate for developers to use the consuming modifier at their discretion when conforming to ClientMiddleware. Thank you.

extension LoggingMiddleware: ClientMiddleware {
    package func intercept(
      /* ... */     
    ) async throws -> (HTTPTypes.HTTPResponse, OpenAPIRuntime.HTTPBody?) {

       // ...

        log(request, requestBodyToLog)
        let (response, responseBody) = try await next(request, requestBodyForNext, metadata)

        // ...
    }
}
czechboy0 commented 4 months ago

Ok - is it okay to close this one then?

ojun9 commented 4 months ago

Yes, it's okay to close this one. Thank you!