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.21k stars 87 forks source link

Streamlining Use #542

Open JetForMe opened 2 months ago

JetForMe commented 2 months ago

Motivation

Migrating from writing Swift Vapor controllers directly to using Open API, I've found the latter to be substantially more cumbersome. A simple POST handler in Vapor might look like this:

func registerAPNDevice(req inReq: Request)  async throws -> HTTPStatus {
    let req = try inReq.content.decode(RegisterAPNRequest.self)
    print("DeviceID: \(req.deviceID)")
    return .created
}

To my novice use of the generated code, I have to write this:

func registerAPNDevice(_ inInput: Operations.registerAPNDevice.Input) async throws -> Operations.registerAPNDevice.Output {
    guard
        case let .json(json) = inInput.body,
        let deviceIDs = json.deviceID,
        let deviceID = UUID(uuidString: deviceIDs)
    else
    {
        throw Abort(.badRequest)
    }

    print("DeviceID: \(deviceID)")

    return .created(.init())
}

Additionally, it took me far too long to figure out how to write this code. Xcode’s code completion rarely works reliably for me, which didn’t help. But I spent a lot of time just trying to drill down through the hierarchy of data structures to get at the data I needed (the request content).

The client side isn't much better:

try await client.registerAPNDevice(body: .json(.init(deviceId: "foo")))

This exposes in the client the internal structure of the request (JSON in the body). It would be better to have Swift struct as an abstraction that one hands to the generated code, which is responsible for packaging it and sending it to the server.

I was directed to this example recently, but honestly it doesn't feel any better. The code is littered with boilerplate (lots of .init(), for example).

Proposed solution

The generator should generate additional accessor functions to streamline access to the values. I'm not really sure exactly what the form of this should take is, but the Input object should have methods or properties that let me get directly at the content I want. Don't make my handler dig through the HTTP structure to get at it. Similarly for the client side, don't make the code expose the HTTP structure.

Alternatives considered

No response

Additional information

No response

czechboy0 commented 2 months ago

Hi @JetForMe,

I understand your desire to write against code that is more ergonomic, but note that this project's goal is to generate a type-safe Swift representation of an OpenAPI document, which in turn describes HTTP communication (read more about this project's scope and goals here: https://swiftpackageindex.com/apple/swift-openapi-generator/1.2.1/documentation/swift-openapi-generator/project-scope-and-goals).

So not only is the fact that you work with the type-safe representation of the HTTP communication necessary, it's very much intentional. The generated code is meant to help developers only send and receive valid HTTP requests and responses according to the OpenAPI document. It saves you from writing the hundreds of lines of boilerplate code per operation, however at the end of the day, you are still responsible for interpreting what the individual HTTP requests and responses mean for your application. There's no way that the generator can know that, so it can't generate it for you either.

Note that for client code, last year we added a shorthand API, as often a client only wants to handle one specific status code and content type, and simply throw an error if it's different. That's because there's usually a single service implementation but many different client implementations (so each might need a different subset of the API).

We didn't add such shorthand API for unwrapping the Input, because the server must handle all the request flavors that the OpenAPI document describes, so in your case, it's likely that you want a switch statement for the content types supported (even if you currently only support one - JSON).

let deviceIDs = json.deviceID

This is unexpected - if the device must have a valid deviceID, you should mark it as required in the OpenAPI document, meaning that the handler won't even be called if the ID is missing, simplifying your code further.

Then, your handler could look something like this (with the recommended error handling):

func registerAPNDevice(_ inInput: Operations.registerAPNDevice.Input) async throws -> Operations.registerAPNDevice.Output {
    let requestBody: Operations.registerAPNDevice.Input.bodyPayload
    switch inInput.body {
    case .json(let json):
      requestBody = json
    }
    guard let deviceID = UUID(uuidString: requestBody.deviceID) else {
      // return 400, as the deviceID is not a valid UUID string, so this is client request error
    }
    print("DeviceID: \(deviceID)")
    return .created(.init())
}

If you have feedback that can make working with the HTTP requests/responses more ergonomic without trying to wrap them or hide away any expressivity, please file individual issues where we can discuss each one. But hopefully the above sheds more light on why the generated code works this way, and that at a high level, it's in line with this project's goals.

simonjbeaumont commented 2 months ago

This exposes in the client the internal structure of the request (JSON in the body). It would be better to have Swift struct as an abstraction that one hands to the generated code, which is responsible for packaging it and sending it to the server.

To this point, I think that's not quite accurate. There is no need to deal with any seeialization when using the generated client or server, be that JSON or otherwise.

The server author provides a type that conforms to a Swift protocol that has method requirements determined entirely by Swift input and output types.

The .json here is an enum case of the request type. We have enum cases for each of the document request and response content types. As a server you will need to handle any of the document request types, which your Vapor server does not show, and we structure the generated code with API evolution in mind. Specifically, if an API grows the ability to accept another request content type (perfectly backwards compatible from a HTTP API perspective), the enum just grows another case. The server code will need to handle this to be a compliant server. And, ofc, there's a symmetric discussion to be had about the use of enum values for the response types in the generated client code.

Hope that helps motivate the shape of the generated code a little more.

JetForMe commented 2 months ago

I guess I really don't understand. I wasn't proposing removing any of the existing generated code. But what I see with the way things are right now is I express a request/response in the YAML file, and then I express it again in my Swift code. I want the code that sits between my Swift code and the server to abstract all of that detail away.

Vapor does this for me, in a much more readable (and writeable) fashion. I ask for a Swift type representing the provided data, and it handles unmashaling it. It also validates and throws exceptions that get translated into a response to the client.

I really struggle to see the benefit of the generator for me as the server developer (its benefits for documentation are a bit more evident, but I could probably get that from a tool encountered that generates the YAML from my Vapor code).

czechboy0 commented 2 months ago

I express a request/response in the YAML file, and then I express it again in my Swift code

Not exactly, you document your request/response types in the YAML file, and then you use those generated types in your Swift code.

I ask for a Swift type representing the provided data, and it handles unmashaling it. It also validates and throws exceptions that get translated into a response to the client.

That's exactly what the generated code does for you.

I really struggle to see the benefit of the generator for me as the server developer (its benefits for documentation are a bit more evident, but I could probably get that from a tool encountered that generates the YAML from my Vapor code).

Have you had a chance to watch the server section of the WWDC talk? That might show those benefits more: https://developer.apple.com/wwdc23/10171?time=972

simonjbeaumont commented 2 months ago

I want the code that sits between my Swift code and the server to abstract all of that detail away.

Vapor does this for me, in a much more readable (and writeable) fashion. I ask for a Swift type representing the provided data, and it handles unmashaling it. It also validates and throws exceptions that get translated into a response to the client.

The generated code goes one step further. In your Vapor example, you're making a function call into Vapor to decode the incoming HTTP request type into a typed domain-specific request type:

let req = try inReq.content.decode(RegisterAPNRequest.self)

You, as the author, had to pick the right type to pass to decode(_:) here and I'm curious to know how you'd write this code to generalise over multiple request and response content types, which are used in many HTTP APIs.

By the time control flow hits your generated code all the unmarshaling has already happened. The value of type Operations.<OperationID>.Input that is passed to your handler is the decoded Swift type. All the error handling for malformed requests has already taken place for you, behind the scenes, in the generated code.

I'm wondering if you've been tripped up by the .json enum case, which made you think that the decoding hadn't happened yet?

All this being said, we genuinely appreciate feedback on how to make the APIs more expressive and have made changes based on user feedback in the past. If you have any ideas on how we can retain the expressivity of the OpenAPI specification but make things more usable for either client or server adopters, we'd be open to discussing them in detail.

JetForMe commented 2 months ago

You, as the author, had to pick the right type to pass to decode(_:) here and I'm curious to know how you'd write this code to generalise over multiple request and response content types, which are used in many HTTP APIs.

I am really only concerned here with writing new APIs, and I have done quite a few. I have never written them to accept more than one type of request. That is, for any given endpoint, it only ever accepts one type of request (e.g. a GET request might have request parameters or path components, a POST or PUT will have JSON or binary data). The only response I can think of that might return different types would be, say, something returning an image, which might be PNG or JPEG, but that can almost be ignored at the REST specification level.

My actual code that handles a request knows that it needs some data and it knows it needs to produce some data. I don't want it to have to care how it got the data (e.g. that it was JSON-encoded or whatever). Can't quite do that on requests that might have some data embedded in the request path and some in query parameters, but it’s otherwise pretty close.

The API produced by this generator exposes all of that, and it adds additional layers of hierarchy that require a sprinkling of .init, which I generally dislike because it makes it a lot harder to understand what is going on when reading the code (unless you're already quite familiar with it).

Alas, I've decided to abandon using the API generator for now. I may in the future find a way to auto-generate the specification file from my code, but I don't foresee using the generated code in my server or client.

What I'd love is a tool to generate Vapor Controller stubs based on an Open API specification. Even these controllers have to know a bit too much about the transport (e.g. was the information in the path? the query string? the body?).

czechboy0 commented 2 months ago

Understood - is there anything else we can help with? If not, please close the issue.

simonjbeaumont commented 2 months ago

It sounds like this tool isn’t the right fit for your needs. It’s designed to support the full expressivity of OpenAPI, which caters to a wider range of API semantics than you don't need or desire for your API server.

As such, the generated API has to account for this. For example, we separate the different parameter locations into their own namespaces to avoid conflicts. There are also plenty of HTTP APIs that respect the Accept HTTP header field in the request, and use it to determine the content type of the response.

JetForMe commented 2 months ago

Again, I'm not saying you have to remove that expressivity. I'm saying it should generate additional code so that I'm not forced to use it. The whole point of a code generator is to relieve me of the burden of minutiae and boilerplate. I can work in app-specific data structures, and the framework handles translating that for clients.

But you're right. As-is, this tool does not meet my needs.

simonjbeaumont commented 2 months ago

To be clear, we’re genuinely open to additive changes if they can be expressed in a way that generalises well.

Maybe you could suggest an example extension that you think would solve the ergonomic issues you’re having. If it’s generalisable, we could consider generating it.