Wrap operation methods in specialized methods (1 success response, 1 content type)

[MahdiBM]

Here's an example of some code i'm repeating again and again in Penny:

func getFilesChanges() async throws -> [FileDiff] {
    let response = try await githubClient.pulls_list_files(.init(
        path: .init(...),
        body: .init(...)
    ))

    guard case let .ok(ok) = response,
            case let .json(json) = ok.body else {
        throw Errors.httpRequestFailed(response: response)
    }

    return json
}

I think this function should roughly look like this. optimally:

func getFilesChanges() async throws -> [FileDiff] {
    try await githubClient.pulls_list_files(
        path1: ...,
        path2: ...,
        body: .init(...)
    ).decode()
}

So let's analyze how the second function is better:

• It's a single step so easier to use.
• Cleaner, so nicer to read. Doesn't need weird case let or nested switch statements.
• It is still what 99% of users want anyway.
• It also decode() the response only after requested by user.
  • I prefer this as sometimes you don't need the response at all, but it's lower on the priority list.
• The decode() function itself is supposed to throw a proper error if the response isn't a success response.
  • There can be another decodeError() function or the like to not check for success of the request considering one might be in fact trying to decode the error response.

What i mentioned above is basically what i've tried and implemented in DiscordBM, and i've really liked, which is achieved by:

• A base HTTP-response type
• Another HTTP-response type that wraps the base one.
  • Only thing extra it has is that it takes a generic parameter.
    • So it can automatically decode() the response to that type when needed.
  • This is purely for ease of usage by users. So they don't need to specify the decode type or make one.
• Then the DiscordClient functions will return one of those types (for the most part), depending on if Discord's response is documented to have a body.

There are somethings worth noting:

• One negative thing about this approach is that it requires a ton of work when done manually.
  • Luckily, this doesn't apply to this package as this a code-generation package.
• Secondly, it might be too much too ask for this library.
  • It's not fair to compare it DiscordBM to this library as this library is supposed to implement the full scope of the OpenAPI spec.

I still think a lot of this is achievable in relatively short term, and almost all of it in long term.

Right now there is no way for one to make the decoding process easier for themselves.
For example if all json responses conformed to a single protocol, i could have at least set up some functions to ease my work.

[czechboy0]

Hi @MahdiBM, thanks for writing this up in a lot of detail.

A few things to consider, that'll hopefully explain why what you suggest hasn't been done yet. We certainly could make some improvements in this area, but we so far focused on just getting the building blocks correct (since users can build conveniences on top of the generated code). That said, we are tracking generating more convenience sugar with #22.

In your example, you flattened the inputs into one namespace. That assumes that the names are unique, but they might not be. You can totally have a header named foo and also a query parameter named foo in the same operation, so we can't simply flatten them. And we have so far tried to avoid complicating the naming logic by having to know other context. The naming logic today only needs to know the item itself, not its siblings, so we don't currently have a nice way to conditionalize naming to avoid conflicts. The generated Swift code matches the scopes of the OpenAPI specification, and OpenAPI has one scope for path params, another for query params, etc, so does the generated code. It's one of the project principles - to faithfully reflect the OpenAPI document, instead of us making too many assumptions that could make legal OpenAPI documents fail in Swift OpenAPI Generator.

The second thing your example assumes, but isn't actually part of the OpenAPI specification, is knowing which response represents the single success that you'd want to return, and which responses should throw an error. An OpenAPI document can totally have an operation with 2 or more successful responses, and only the user knows what's "success" and "failure" to them.

It's also a bit of an anti-pattern to throw indiscriminately on all non-success status codes, since the author of the service that provides the OpenAPI document might have done the hard work to specify multiple error response status codes, each of which should be handled differently by the user. For example, 401, 404, and 429 should presumably be handled very differently - one should maybe refresh auth, one should fail outright, and one should retry with a backoff. If, as a client, you want to handle all the same way, that's fine, but the generator doesn't really want to encourage that - it's up to you to write that guard/else, but ideally you'd write a switch for each response and handle each response appropriately. And you'd write wrappers that do that for you to avoid having to repeat it at all the call sites.

On the decode method, I'm not sure what you mean - are you saying the response body shouldn't be decoded unless the user explicitly calls the method? Why is that?

Thanks again for taking the time and hopefully some of the explanations above shed more light on why, if we want to support the full flexibility of the OpenAPI specification, we can't easily make assumptions like: a single success response, unique parameter names, etc, that could make the generated code a bit more convenient.

That said, we do want to make improvements in this area, so we'd like to continue the conversation. It'll just be a bit more work than might seem at first, and needs to be well designed, to ensure we don't create limitations on top of the OpenAPI speciation or overly complicate the generator logic.

One way to go in this area I've been thinking about, and you already mentioned, is to generate some extra "hooks" in code that people can write generic functions on. For example, if we had a protocol SingleSuccessResponding that we add on all generated Output types that have exactly one response in the 2xx range, suddenly users can write their own conveniences without us having to get too opinionated. That's the direction I suspect we'll be able to go to help in this area.

[sliemeobn]

huge +1 from me for the simplified access to the response body. A protocol like SingleSuccessResponding feels right to me.

I understand the focus on supporting theoretically complicated response types, but I think we can all agree that <insert high percentage here> of real-life API usage is "get the body on 2xx or throw"

Its great that you can dig into all the details of a response for the complex cases, but for the (my guess) most used use-case the API is not very ergonomic.

In terms of naming: .decode() does not feel great to me, I am thinking about Task.value or Result.get().

So, something like this would feel swifty to me:

do { let body = try response.successBody }
catch let error as ClientError { ... }

[czechboy0]

Just to clarify here – I'm not disputing that <some high percentage> of operations have a single success response. I'm just explaining that this project aims to be the low-level building block for building more ergonomic APIs, while also allowing to generate OpenAPI documents with multiple success types. There are many generators in the wider OpenAPI ecosystem that bake in a ton of similar assumptions and make it impossible to generate things like multiple success responses, but that's not the path we chose here. We aim to represent, as faithfully as possible, OpenAPI in Swift, and gradually we can build syntactic sugar on top of the low-level, fully type-safe layer. But it's important that people with uncommon OpenAPI patterns still can successfully use the generator, even if they don't get all the conveniences that we might create in the future (such as SingleSuccessResponding).

For even more detail, check out the Project scope and goals and API stability of generated code, which outline how we think about the generated code, and how we recommend adopters use it. TL;DR - it's not meant to fully replace hand-written packages for specific services. But it can help you write those hand-written packages much more safely, without any manual copy-pasting of string JSON keys, and encourages you to handle all responses, both successes and failures.

Hope this sheds some light on why the current design of the project is like this, and how we aim to gradually improve the ergonomics over time. 🙂

[czechboy0]

In case someone wants to pick up designing a solution here – we very much want to minimize how much extra code needs to be generated, and how much complexity has to be added to the generator in this process. So a modest proposal that makes meaningful progress has a higher chance of success than a proposal of an overly specific sugar that doesn't generalize very much, and is complex to implement. It's a spectrum of course, just wanted to publicize some of the criteria that'll be important during review. But of course, making the adopter experience as good as possible overall is the most important thing 🙂

[MahdiBM]

@czechboy0 Thanks for the response.

In your example, you flattened the inputs into one namespace. That assumes that the names are unique, but they might not be....

Right. Fair point. I still think this could be a very low priority feature request which is fine to possibly take a few years to happen.
There could be a mechanism that accepts function parameters and makes sure they are unique.

The second thing your example assumes, but isn't actually part of the OpenAPI specification, is knowing which response represents the single success that you'd want to return...

Valid point, but i don't see a real problem. There could be multiple functions to decodeOk()/decodeCreated() etc...

It's also a bit of an anti-pattern to throw indiscriminately on all non-success status codes...

Right again, I understand your concerns, but i don't think those are deal breakers. Assuming there is a function like decode(), users who are interested in managing the requests properly can just don't do decode() and read the status code instead. Even if they do decode() and it fails, they can catch the error and the error should have enough/full context to be able to recover from the error.

are you saying the response body shouldn't be decoded unless the user explicitly calls the method? Why is that?

The reasons are all basically related to performance:

• 1- Why would I spend cpu cycles decoding when i don't need the decoded body?
• 2- Some people might want to be more specific about their JSON decoder. For example swift-protobuf's JSON-decoding stuff are quite faster than Foundation's JSONDecoder, and in the past i had to switch a few decoding processes to use swift-protobuf which needs the raw body data, just so i don't need to pay for more resources to my service provider. These decoding processes were basically 40% of what the whole app was doing, and they were very worth optimizing.
• 3- Someone might want to only partially decode the response. In which case they might need to both provide a custom decoding model, as well as using another JSON decoder like IkigaJSON which doesn't fully parse the whole JSON upfront.

One way to go in this area I've been thinking about, and you already mentioned, is to generate some extra "hooks" in code...

Yeah i think that's something to improve on as well. I like the SingleSuccessResponding, the only problem is if we can generalize that even more.
Like having DoubleSuccessResponding, TripleSuccessResponding etc...
Just throwing what i think out there. I don't really think this is necessarily the right way to have protocols with those names.

I also understand one of the main problems in front of what i'm proposing.
It's a very valid worry to make sure these new feature don't get in the way of other people who use less-common OpenAPI documents.

All in all, I'm still not sure exactly which way to go. I definitely need to think more about this, and possibly take a deeper look at the OpenAPI specification.
I'm not sure i'll have the time/mood to put a proposal together. I hope so, though :)

[bfrearson]

My first thought as a user when hitting a similar issue was to try to add convenience methods that were unique to my implementation but that's not possible at the moment as each type is different.

Could we wrap all generated types in a protocol (e.g. HeaderProtocol, ResponseBodyProtocol) so that users can add in their own extensions?

I agree that the generator itself should probably not make assumptions about use cases—this way, a user has full control over how they chose to handle the response.

[czechboy0]

Agreed, I think the best way would be to add hooks on the generated types (such as marker and real protocols), which then adopters can extend using generics or macros, matching their preferred style.

[MahdiBM]

I was thinking we could also go full in the macro way instead of adding protocols etc...
Haven't fully thought it through, but it's an option.

[czechboy0]

Macros could help here for sure, but calling a macro still needs to typecheck correctly (before expansion), and I don't think we today have enough type information on the generated types to make even macros useful. And once we add those, some combination of generic functions and macros might help us make the ergonomics for simple cases nicer.