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.26k stars 91 forks source link

Improve access to undocumented HTTP data #299

Closed groue closed 6 months ago

groue commented 8 months ago

Hello,

OpenAPIRuntime v0.2.5 comes with an UndocumentedPayload type that is returned for undocumented status codes.

This is all and good, but this type is empty and does not provide any information about the actual response returned by the server. This means that the client is 100% unable to process the response (and maybe look for information present in the raw body response, who knows).

The tutorial gives this example:

let client = Client(...)
let response = try await client.getGreeting(...)

switch response {
case .ok(let okResponse):
    print(okResponse)
case .undocumented(statusCode: let statusCode, _):
    // ALL INFORMATION IS LOST HERE (but the status code)
    print("πŸ₯Ί undocumented response: \(statusCode)")
}

I don't find this very convenient:

Sure, 100% accurate and complete openapi.yaml files are ideal. But what about people who did not reach 100% yet, or are just developing or prototyping a server?

Is there another technique I should know about, that would allow the generated code to expose raw information? Maybe some technique at the level of openapi.yaml?

czechboy0 commented 8 months ago

Hi @groue πŸ™‚

It really depends on what you'd like to do with the undocumented payload.

If you just want to print it for debugging, I'd recommend implementing a custom ClientMiddleware that logs the response information, including the headers and body however you need. There, you can decide whether/how to decode the payload.

As an alternative, if you know the payload will be JSON, in the OpenAPI document, add a default response like this:

responses:
  '200':
    ... successful response
  default:
    content:
      application/json: {}

That way, a default case will be generated in the response enum that holds a freeform parsed JSON object for you, allowing you to inspect it.


That all said, I'd only recommend using this as a temporary workaround, and document the payloads you want to extract in the OpenAPI document.

Hope this helps!

czechboy0 commented 8 months ago

To add more detail here, since the undocumented case is, well, undocumented, we cannot assume anything about it - its content type, size, whether there are any bytes at all. So if UndocumentedPayload gained some representation of the returned bytes, the best we could do would probably be the raw bytes – which you can already get access to in the middlewares today.

groue commented 8 months ago

Hello @czechboy0, thanks for taking the time to answer.

It really depends on what you'd like to do with the undocumented payload.

If you just want to print it for debugging, I'd recommend implementing a custom ClientMiddleware that logs the response information, including the headers and body however you need.

Indeed, ClientMiddleware lets me log all responses (body and headers). That's better than nothing πŸ‘

As an alternative, if you know the payload will be JSON, in the OpenAPI document, add a default response like this:

For 5xx responses it's difficult to "know" it will be JSON, so I tried a wildcard:

paths:
  /my/path:
    post:
      responses:
        '201': ...
        default:
          $ref: '#/components/responses/DefaultResponse'

components:
  responses:
    DefaultResponse:
      description: A raw default response
      content:
        '*/*':
          schema:
            type: string

I did not try it at runtime, but the generator gives me:

/// Types generated from the `#/components/responses` section of the OpenAPI document.
public enum Responses {
    public struct DefaultResponse: Sendable, Hashable {
        /// - Remark: Generated from `#/components/responses/DefaultResponse/content`.
        @frozen public enum Body: Sendable, Hashable {
            /// - Remark: Generated from `#/components/responses/DefaultResponse/content/*\/*`.
            case any(Foundation.Data)
        }
        /// Received HTTP response body
        public var body: Components.Responses.DefaultResponse.Body
        // (snip: initializer)
    }
}

It looks workable, but I'm missing the headers.


I think I'll take the middleware route, thanks to your advice. I will be able to work.

But I won't work blissfully. I regret that information is difficult to access. When the middleware runs, it does not know if the response is expected or not. When the response has been decoded, we have lost all debugging information (response body and headers). Everything is closeted, and I must make choices and sacrifices. When you develop and prototype, those are never welcome.

UndocumentedPayload was sooooo promising (body and headers 🀩), until I discovered it was an empty shell (no body and no header). What's the purpose of this struct?

czechboy0 commented 8 months ago

It's there to hold additional information we might want to add later, and it's there from the beginning so that the number of associated values doesn't change (as that's generally a difficult migration).

Let me clarify the distinction between the different layers here, which should hopefully explain why certain information is only available in the middlewares.

The generated code represents the documented structured of the API in the OpenAPI document. This layer is what you use to access documented information.

The ClientTransport/ClientMiddleware is the translation layer between the type-safe code generated from the OpenAPI document and the generic HTTP library.

To access the documented data, you use the former. To access undocumented data, you use the latter.

The former only does work required to fulfill the OpenAPI contract, so even if the response contains 100 headers, only the documented ones are extracted - this is important for giving the OpenAPI author control over the performance.

Out of curiosity, what's the use case here? What information are you trying to extract (not just log for debugging) that cannot be documented in the OpenAPI document? That should help us understand if a feature is missing here.

groue commented 8 months ago

The generated code represents the documented structured of the API in the OpenAPI document. This layer is what you use to access documented information.

The ClientTransport/ClientMiddleware is the translation layer between the type-safe code generated from the OpenAPI document and the generic HTTP library.

To access the documented data, you use the former. To access undocumented data, you use the latter.

Thanks for replying the layers and their purposes.

This does make it easy to work with under-documented apis. Under-documented apis exist, and they are frequent. The lack of documentation can not always be fixed promptly, and sometime can never be fixed at all. The communication between the client developers (users of the generated code) and the server developers may be slow (different team), or impossible (product vendor).

To access the documented data, you use the former (ClientTransport). To access undocumented data, you use the latter (ClientMiddleware).

Middlewares are unaware of the purpose of the requests and responses they intercept.

How can one work with under-documented endpoints? Should one instantiate a distinct Client with a specific ClientMiddleware type, and have this specific middleware throw ad-hoc errors, as in the sample code below?

struct ClientWrapper {
    func underSpecifiedEndpoint(_ input: ...) async throws -> ... {
        /// An error that represents an undocumented feature
        enum MiddlewareError: Error {
            case specialCase1(/* special payload 1 */)
            case specialCase2(/* special payload 2 */)
        }

        /// A middleware that detects undocumented features
        struct UndocumentedFeaturesMiddleware: ClientMiddleware {
            func intercept(_ request: Request, baseURL: URL, operationID: String, next: (Request, URL) async throws -> Response) async throws -> Response {
                let response = try await next(request, baseURL)
                if /* detect special undocumented case */ {
                    throw MiddlewareError.specialCase1(...)
                } else if /* detect special undocumented case */ {
                    throw MiddlewareError.specialCase2(...)
                }
                return response
            }
        }

        let client = Client(
            serverURL: ...,
            transport: ...,
            middlewares: [UndocumentedFeaturesMiddleware()])

        do {
            let response = try await client.underSpecifiedEndpoint(...)
            switch response {
            case .ok:
                // Process documented response
            case let .undocumented(statusCode: statusCode, _):
                // Process generic undocumented response
            }
        } catch let MiddlewareError.specialCase1(payload) {
            // Process special undocumented response 1
        } catch let MiddlewareError.specialCase2(payload) {
            // Process special undocumented response 2
        }
    }
}

Is this what one is supposed to write?

I can live with such code, but I have to ask if it matches the expectations of the designers of the library, because it looks convoluted.

EDIT: If UndocumentedPayload would not hide information (body + headers), it would be much easier to write, and read:

struct ClientWrapper {
    let client: Client
    func underSpecifiedEndpoint(_ input: ...) async throws -> ... {
        // No specific error type
        // No specific middleware type
        // No need to instantiate a specific Client
        let response = try await client.underSpecifiedEndpoint(...)
        switch response {
        case .ok:
            // Process documented response
        case let .undocumented(statusCode: statusCode, payload):
            if /* detect special undocumented case from payload */ {
                // Process special undocumented response 1
            } else if /* detect special undocumented case from payload */ {
                // Process special undocumented response 2
            } else {
                // Process generic undocumented response
            }
        }
    }
}
groue commented 8 months ago

Out of curiosity, what's the use case here? What information are you trying to extract (not just log for debugging) that cannot be documented in the OpenAPI document? That should help us understand if a feature is missing here.

I'm developing and prototyping both the server and the client :-) My server connects to other apis on the internet. Those apis are rather well documented, but I have to choose where I spend my time. In particular, my server currently passes 3rd-party errors unmodified. Those errors are complex beasts, and I don't want to spend the time to document or wrap them now.

This means that 4xx and 5xx codes are under-documented in my openapi.yml, and they will be until I can work on those topics.

I can write a middleware that handles 4xx and 5xx on the client so that I have full debugging information.

This is already better than specifying a dummy "documented" response in openapi.yml, IMHO, because well there is no real documentation.

My middleware can not deal with unexpected success codes (such as 200 when 201 is expected in a specific endpoint), because the middleware lacks information and does not know the purpose of the request. I'd have to use the complex setup described above.

I thus feel somewhat uncomfortable during my prototyping/development phase. I'm writing code that wants to stay there when I know it's just workarounds for limitations in the code generation.

I hope I could express my discomfort in a way that you understand πŸ˜…

czechboy0 commented 8 months ago

Sorry, I can see how you understood this slightly differently than what I intended:

The ClientTransport/ClientMiddleware is the translation layer between the type-safe code generated from the OpenAPI document and the generic HTTP library. To access the documented data, you use the former. To access undocumented data, you use the latter.

What I meant is that you use the type-safe code generated from the OpenAPI document to access the documented data, and you use the generic HTTP request and response values you are provided in ClientTransport/ClientMiddleware to access undocumented data.

What you're asking in this issue for is accessing undocumented data in the generated code, which is not how it's meant to be used.

That said, this workaround gives you access to the body, at least:

responses:
  '200':
    ...
  default:
    content:
      */*: {}

Unfortunately, you cannot access the undocumented headers this way.


Let me provide more details on how the generator is meant to be used with undocumented APIs.

First, let's consider what you can do with the undocumented payload: Data for the body, and e.g. [String: String] for undocumented headers (no access to those is provided in the generated code, this is just an illustration about what they are in the parsed generic HTTP response).

In your code that would handle the freeform Data/[String: String] based data, you'd probably need to look up some headers whose names you know upfront, and whose type you know. For example, let's assume you have a header called x-my-header whose value is an integer.

guard let headerStringValue = undocumentedHeaders["x-my-header"] else {
  // header not found
}
guard let headerIntValue = Int(headerStringValue) else {
  // header not a valid integer
}
// use the header value

You'd need to extract it and parse its value.

You can achieve the same thing by documenting this as an optional response header, such as:

responses:
  '200':
    ...
  default:
+    headers:
+      'x-my-header':
+        schema:
+          type: integer
    content:
      */*: {}

Then, you can get the value, already parsed as an integer, in the default response's headers struct.

Similar with the body. If it's e.g. JSON, document it in your local copy of the OpenAPI document and rebuild, and you'll get type-safe access to the payload.

You can use {} to represent a completely freeform JSON object, so you don't have to document (yet) what the keys/values are, as you're gradually discovering what's on the wire and expanding the OpenAPI document.

Once you're happy with it, you could send this copy of the document to the team responsible for the backend, but in the meantime keep using your local copy that documents all the payload you actually want to extract at runtime.

The rule of thumb here is: instead of writing custom code that extracts the unvalidated, unparsed response data (headers and body), document what you know about the payloads in the document, regenerate, figure out more about the payload, improve the OpenAPI document, repeat.


This should work for you as an iterative workflow to make the underdocumented API less underdocumented, and it also saves you from writing custom Swift code of unwrapping the payload, by letting the generator do it for you.

That's what I'd recommend when you're programatically handling the response data. If you just need to inspect it, a logging middleware that you provide to the Client(...) initializer would probably be easier.

czechboy0 commented 8 months ago

In particular, my server currently passes 3rd-party errors unmodified. Those errors are complex beasts, and I don't want to spend the time to document or wrap them now.

Understood. If you always want to pass through all 4xx and 5xx responses to the caller, then something like you prototyped here should work well: https://github.com/apple/swift-openapi-generator/issues/299#issuecomment-1735774813

czechboy0 commented 8 months ago

it's just workarounds for limitations in the code generation

Can you speak more to the limitations you're seeing in code generation? Are you trying to use an OpenAPI feature the generator doesn't support yet? We use feedback like this to help us prioritize, so if that's the case, let us know! πŸ™‚

groue commented 8 months ago

Hello again, @czechboy0

What you're asking in this issue for is accessing undocumented data in the generated code, which is not how it's meant to be used.

I totally agree that the generated code ought to provide convenient access to documented data :-)

But does this prevent the non-generated runtime support (OpenAPIRuntime) from giving access to undocumented data?

And does this prevent the generated code itself from giving access to undocumented data (raw body and headers)?

// IDEAL
let response = try await client.someEndpoint(...)
switch response {
case let .ok(okResponse):
    switch okResponse.format {
    case let .json(payload):
        // Here we can conveniently use the documented JSON payload
        // thanks to code generation πŸ‘
        //
        // One could also want to use okResponse.body and okResponse.headers
        // in order to access eventual undocumented information.
    }
case let .undocumented(statusCode: statusCode, payload):
    // `UndocumentedPayload` could provide access to raw body and headers.
}

Convenient access to documented data does not forbid (less convenient) access to undocumented information.

That said, this workaround gives you access to the body, at least:

responses:
  '200':
    ...
  default:
    content:
      */*: {}

Unfortunately, you cannot access the undocumented headers this way.

Yes.

Let me provide more details on how the generator is meant to be used with undocumented APIs.

First, let's consider what you can do with the undocumented payload: Data for the body, and e.g. [String: String] for undocumented headers (no access to those is provided in the generated code, this is just an illustration about what they are in the parsed generic HTTP response).

In your code that would handle the freeform Data/[String: String] based data, you'd probably need to look up some headers whose names you know upfront, and whose type you know. For example, let's assume you have a header called x-my-header whose value is an integer.

guard let headerStringValue = undocumentedHeaders["x-my-header"] else {
  // header not found
}
guard let headerIntValue = Int(headerStringValue) else {
  // header not a valid integer
}
// use the header value

This solution requires a specific middleware for the specific endpoints that need this special processing, am I correct? As in my previous comment? It's difficult for the middleware to communicate the information it gathers.

You can achieve the same thing by documenting this as an optional response header, such as:

responses:
  '200':
    ...
  default:
+    headers:
+      'x-my-header':
+        schema:
+          type: integer
    content:
      */*: {}

Then, you can get the value, already parsed as an integer, in the default response's headers struct.

Once you're happy with it, you could send this copy of the document to the team responsible for the backend, but in the meantime keep using your local copy that documents all the payload you actually want to extract at runtime.

This solution makes two assumptions:

  1. That one can always patch the openapi.yaml file.
  2. That the server team accepts patches.

I don't really have to argue for the second point: the user of apple/swift-openapi-generator does not control the server team. This means that one must assume that some server teams just do not accept patches. How to help in such a situation?

So we face the first point: can we always patch an OpenAPI spec file? Sometimes, yes. And there are the other specs. The specs that are huge, provided by some third-party, updated, etc, and generally difficult to patch.

If apple/swift-openapi-generator indeed requires developers to patch a spec just to access undocumented (or not-documented yet, etc) data, I would kindly ask to reconsider this position. I described above some realistic scenarios where this creates a surprising amount of work. This is not the kind of surprise (extra delay plus extra costs) you expect from such a fundamental api.

groue commented 8 months ago

Can you speak more to the limitations you're seeing in code generation? Are you trying to use an OpenAPI feature the generator doesn't support yet? We use feedback like this to help us prioritize, so if that's the case, let us know! πŸ™‚

So far, in the early stages of my developments, I don't face blocking limitations.

While evaluating the relevance and the risks of this library for my project, and future ones, I see one huge benefit, and one drawback that I have described above:

  1. The huge benefit is the code generation, the ability to use the OpenAPI spec as a source of truth. Thank you so much for working on this topic πŸ™
  2. The known drawback is the immense inconvenience for working with undocumented data. Since I'm working on both stacks today (client and server), and they are small, I can workaround this. But I'm not sure apple/swift-openapi-generator, in its current state, is able to deal with big apis that are unsufficiently documented (I've seen a lot of them in the wild).

I'm optimistic. There's certainly room for improvements. I hope more testimonials like mine will come up.

czechboy0 commented 8 months ago

I appreciate the details you're providing, they're definitely helpful. So far, you're the first person to bring this up since we went open-source, so I really want to make sure I understand the issues and try to come up with solutions that can be maintained over the long term.

That one can always patch the openapi.yaml file.

I think it's worth focusing on this point a bit more. The generator requires (both when used as a plugin and as a CLI) a local copy of the OpenAPI document, which you can make arbitrary changes to. While in some circumstances it might be valuable to try to contribute your changes back upstream to the server team, that's not required for you to be able to unwrap responses using generated type-safe code.

Personally, I've written OpenAPI documents for myself for APIs that don't vend an OpenAPI document at all, mainly because it's significantly quicker than to write the equivalent code by hand (to see how much code you're not having to write, just peek at the generated Types.swift and Client.swift). The generated code provides really clear errors, validates the input, and provides you with type-safe access. Writing that code by hand is much more work than to edit a few lines of YAML in a local file.

Now, this isn't to convince you that you're not using the tool correctly, it's more to explain that the workflow of maintaining an edited copy of an OpenAPI document you received from someone else is not only okay, it's very common and the generator shines in that workflow, because when used as a plugin, after you've edited your OpenAPI document, just hit cmd+B and your Swift code is ready. Both on client and server, and especially on server I recommend starting with spec-driven development, where you're implementing the server (by filling in the type that conforms to the generated APIProtocol) gradually, as you expand the OpenAPI document, operation by operation.


To provide more details on why it's not as easy to just provide access to the undocumented data, we have to consider performance.

First, let's talk about headers. There might be dozens or even hundreds of headers in a response, injected by various proxies. But as a client, you should only pay the cost of decoding and validating the ones that you want to extract, which is the case currently, by documenting the headers you want to extract in the OpenAPI document.

Second, bodies. For undocumented bodies, there's nothing that can be assumed, so at best, you could get access to the raw received bytes, and any JSON parsing and so on would have you live in your code. But you can already do this today, using the default response with a */* content type and {} as the schema.


To summarize, the reason the undocumented headers aren't provided today are for performance reasons, as we don't want to make the code slower for the majority of users who have a well-documented API. For bodies, the workaround already provides you with the data the same way as if we actually put the data property on the UndocumentedPayload struct.

However, it's important that I mention that this project is governed completely in the open using proposals that anyone can author and put up: https://swiftpackageindex.com/apple/swift-openapi-generator/0.2.3/documentation/swift-openapi-generator/proposals If you feel strongly that this feature is important, and are happy to prototype a solution and show how we can offer this flexibility without worsening the performance for users with well-documented APIs, I'd encourage you to do so! We already accepted community-authored proposal before, and are always open to feedback πŸ™‚

At the same time, this project aims to maintain quality by keeping the scope narrow and focused, more on that in: https://swiftpackageindex.com/apple/swift-openapi-generator/0.2.3/documentation/swift-openapi-generator/project-scope-and-goals

groue commented 8 months ago

I think it's worth focusing on this point a bit more. The generator requires (both when used as a plugin and as a CLI) a local copy of the OpenAPI document, which you can make arbitrary changes to. While in some circumstances it might be valuable to try to contribute your changes back upstream to the server team, that's not required for you to be able to unwrap responses using generated type-safe code.

Personally, I've written OpenAPI documents for myself for APIs that don't vend an OpenAPI document at all, mainly because it's significantly quicker than to write the equivalent code by hand (to see how much code you're not having to write, just peek at the generated Types.swift and Client.swift).

Yes, and I'm very happy that you can control your OpenAPI documents. Not all users can, though (not at all, or not practically), and this issue is a reminder that the current api is far from ideal for them. You never commented on my convoluted sample code above, which shows how impractical it can be. I assume that this convoluted code is the correct way to use the api, in its current state, when one does not control the OpenAPI document.

I was reading the new SOAR-0008 proposal, and it contains a paragraph that exactly matches the point of view I try to defend here:

Furthermore, it's common that Swift OpenAPI Generator adopters do not own the OpenAPI document, and simply vendor it from the service owner. In these cases, it presents a user experience hurdle to have to edit the document, and a maintenance burden to continue to do so when updating the document to a new version.


So far, this issue does not affect the project I'm currently working on, because I do control the OpenAPI document: there is no risk that I have to spend time writing convoluted middlewares, or patching the spec, or debating here about the relevance of my issues. My only real blocking issue so far is #302 πŸ˜…

groue commented 8 months ago

However, it's important that I mention that this project is governed completely in the open using proposals that anyone can author and put up: https://swiftpackageindex.com/apple/swift-openapi-generator/0.2.3/documentation/swift-openapi-generator/proposals

Thank you. I'm not familiar with the project so I'm not sure I can write a real proposal. At this point, I'm just giving feedback as a new user. What I'm looking for is mainly acknowledgement that my issues exist, and pragmatic suggestions for their resolution. The "patch your OpenAPI document" solution is ok in my specific case, but I also see how it can not always be applied. It's important to me that the maintainers of the library first acknowledge that such use cases exist: only then I can help looking for suitable solutions (guided by maintainers, who know the project).

EDIT: I was reading a good thread on Mastodon last week, that I find very relevant in our discussion: https://mastodon.social/@mattiem/111114264106852623

[...] I’d like to see us evolve from β€œready for production?” to β€œwhat are the pain points?”.

czechboy0 commented 8 months ago

@groue I apologize if I made it sound like I'm not acknowledging the use case, all of my responses were part of me acknowledging, considering, and trying to help you resolve the issue.

Since you're the first person to bring this up, I ask a lot of clarifying questions to allow me to internalize the cause of the mismatch and potential solutions, ranging from ugly hacks all the way to first class features in the generator.

In particular, I continue to be curious about what you mean by "controlling the OpenAPI document". My previous comment points out that it's a file on disk that you can change, so I'm confused in what way you're not able to edit it for yourself. I understand that the ideal situation is to then upstream your fixes to the server team, but not doing that also doesn't block you from having an edited document locally temporarily.

I think part of what I'm trying to say is that the logic you describe wanting to write (extract keys you know will come from the server, but aren't documented in the OpenAPI you received) is something you don't have to write at all if you just edit the document. And as I pointed out above, doing that is much quicker and leads to less code than unwrapping dynamic content at runtime. Plus, you immediately have a diff you can send to the server team to incorporate into their OpenAPI document, so eventually you can again switch back to an unmodified document provided by them.

I should acknowledge that while I think this generator is great to use when you have an OpenAPI document (either provided by you or written by you based on reading some markdown docs or inspecting network traffic). If you want dynamic access to the freeform request for some requests, it might be better to use the underlying HTTP client directly (URLSession or AsyncHTTPClient).

Middlewares and the other workarounds mentioned above allow you to temporarily help you debug issues if your goal is to end up with an OpenAPI document that describes all the info you're trying to unwrap from the responses. However if that's not your goal, and you prefer to handle free-form responses and unwrap them in Swift code by hand, using the underlying HTTP client directly will be better than trying to pipe the free-form data through the generated code (as that extra layer isn't adding anything for you in that case).

So based on where you want to end up, I can provide further recommendations or we can discuss what could be improved in the generator. But what the generated code is not made for is for passing through fully dynamic requests and responses, in fact it's the opposite, it's to make all access to the HTTP traffic as typesafe as possible.

Hope this helps.

czechboy0 commented 8 months ago

Regarding your question about the middleware, note that it passes through the "operationId", so you could have a single middleware that inspects the operationId and handles each differently. Again that works best as a temporary workaround while you're improving your local copy of the OpenAPI document, but probably won't be very enjoyable long term, in that case I'd recommend using the underlying HTTP library instead.

groue commented 8 months ago

In particular, I continue to be curious about what you mean by "controlling the OpenAPI document". My previous comment points out that it's a file on disk that you can change, so I'm confused in what way you're not able to edit it for yourself. I understand that the ideal situation is to then upstream your fixes to the server team, but not doing that also doesn't block you from having an edited document locally temporarily.

[...] something you don't have to write at all if you just edit the document. And as I pointed out above, doing that is much quicker [...]

Wouldn't this argument make SOAR-0008 useless, since one can edit their OpenAPI document are remove the unused endpoints?

The difference between blocking and non-blocking is whether it is feasible or not to edit the OpenAPI document.

When one is the author, no problem, of course.

When one has to patch a document that contains hundreds of endpoints, it becomes less quick, less practical.

When that document is updated and the patch has to be redone, it is even less quick and practical.

Eventually one can reach the point where the energy/time/money spent patching/testing/etc is unbearable.

That's why I have been suggesting that when patching the document is not feasible, it should be possible to use the runtime instead. Given that such a runtime technique does not look good today (sample code above), I think there's room for improvement πŸ€“

groue commented 8 months ago

So based on where you want to end up, I can provide further recommendations or we can discuss what could be improved in the generator. But what the generated code is not made for is for passing through fully dynamic requests and responses, in fact it's the opposite, it's to make all access to the HTTP traffic as typesafe as possible.

Thanks. And yes, be assured I wholeheartedly agree that generated code should focus on documented OpenAPI. I mean, please don't doubt this πŸ˜…

However if that's not your goal, and you prefer to handle free-form responses and unwrap them in Swift code by hand, using the underlying HTTP client directly will be better than trying to pipe the free-form data through the generated code (as that extra layer isn't adding anything for you in that case).

Going back to raw URLSession and not profit from generated code?

This library is so close to perfection (look for "IDEAL" in the above sample code). Here, "ideal" means "1. super helpful, and at the same time 2. does not come in the way".

groue commented 8 months ago

Regarding your question about the middleware, note that it passes through the "operationId", so you could have a single middleware that inspects the operationId and handles each differently. Again that works best as a temporary workaround while you're improving your local copy of the OpenAPI document, but probably won't be very enjoyable long term, in that case I'd recommend using the underlying HTTP library instead.

Interesting, I'll keep this in mind πŸ‘

czechboy0 commented 8 months ago

Let me ask about what you'd like to see.

If we added the raw bytes on UndocumentedPayload, and a [String: String] dictionary of headers, would that be enough?

Or do you also need access to the unparsed headers in documented responses? Unparsed JSON keys in documented JSON objects?

To fully understand what the improvement should look like, I need to understand what you're trying to do. If it's a somewhat theoretical concern and you don't today actually need any changes, I think it's okay to leave the great context we put together here and pause, and wait for an adopter who has a concrete use case where the generator doesn't work for them.

If you or anyone has a concrete use case where they need more dynamic information to be propagated through, we can talk specifics and see what could be changed. It's just difficult to design a good solution without aiming at supporting a concrete use case without weakening the type safety of the project. πŸ™‚

simonjbeaumont commented 8 months ago

Here, "ideal" means "1. super helpful, and at the same time 2. does not come in the way".

If we added the raw bytes on UndocumentedPayload, and a [String: String] dictionary of headers, would that be enough?

Why would we not return the OpenAPIRuntime.Response given we have it?

Note that this is purely in the case of the undocumented case. In this case, we can't do any better than provide that response. This type is already part of the OpenAPIRuntime API, so we're committed to it; what are the risks of putting it in?

Furthermore, it's common that Swift OpenAPI Generator adopters do not own the OpenAPI document, and simply vendor it from the service owner. In these cases, it presents a user experience hurdle to have to edit the document, and a maintenance burden to continue to do so when updating the document to a new version.

My guess is that this is the majority of users of the client parts of OpenAPI Generator.

czechboy0 commented 8 months ago

Yeah the UndocumentedPayload type was created because it was foreseen than we might need to propagate more dynamic information. We just didn't have a use case in mind, so we didn't attempt to yet design the API. For example, it's unclear whether the body representation should be raw bytes, or instead be Any? and be parsed as JSON if a JSON content type is detected, etc.

In case of headers, propagating the response for an undocumented code works, but doesn't work if someone is parsing a documented response and they want to get an undocumented header. Maybe the answer is to return the raw response in addition to the typesafe one (I currently don't think so, just pointing out there are a few ways to go).

I think we can consider any and all of these changes, but so far we've responded to specific, well understood use cases when adding API. It'd be good to get a better understanding here of what the use case here is that isn't already better served by existing features. If I understood correctly (I might have missed something though), this issue was filed more to address a hypothetical need, not one that someone has and can provide concrete details about. That's what makes designing a good API for tricky.

groue commented 8 months ago

If I understood correctly (I might have missed something though), this issue was filed more to address a hypothetical need, not one that someone has and can provide concrete details about.

This is very correct. πŸ™‚

I'm used to exploring hypothetical use cases a lot, due to my work on GRDB the Swift SQLite toolkit πŸ˜…

I tend to consider this exploration game as personally interesting, and socially necessary. Personally interesting because it is an opportunity to discover the wild landscape of how people use a technology (HTTP and OpenAPI here, SQLite there), and learn. Socially necessary because a "serious" public API should avoid blocking developers from doing their job at all costs. There are few things more frustrating than hitting a wall after one has spent weeks or months trusting an api.

No one cares about the elegance of an API design if this design doesn't work or gets in the way.

Practically speaking, a technique that I have found useful achieving those goals is to, when in doubt, fully expose the naked low-level truth. This makes sure users are never blocked. Later on, as time passes, experience grows, feedback comes in: use cases are identified, doubts are lifted, and the api is refined. Eventually, the low-level stuff, once useful, becomes superseded by the new and improved APIs. It leaves the front page of the documentation, and even becomes deprecated.

The advantage of exposing the low-level truth is that the library developers can take the time to think about the best ways to improve the api. Meanwhile, users are not blocked, which means their trust is not betrayed, and they're happy.

simonjbeaumont commented 8 months ago

I'm definitely sympathetic to the view that, when we don't have anything better, we don't obfuscate unnecessarily.

We had a similar request regarding exposing the "raw" HTTP request to the server handlers. In that case, it was a stopgap for a feature that we hadn't implemented yet (cookies, IIRC), so potentially less desirable to make a permanent API change for a temporary solution.

In both cases however (client and server), offering an "escape hatch" of the untyped HTTP request/response is surely better than offering nothing.

A common answer to wanting to do something with the untyped HTTP request/response is usually a middleware, but in the case of this issue, I don't think that's appropriate because you don't know if it's the undocumented case at that layer.

We just didn't have a use case in mind, so we didn't attempt to yet design the API. For example, it's unclear whether the body representation should be raw bytes, or instead be Any? and be parsed as JSON if a JSON content type is detected, etc.

Maybe I'm missing something, but I don't understand why this would be anything other than OpenAPIRuntime.Response. We don't know what it is, other than it is an HTTP response, so we shouldn't be trying to do any more. The whole point of this enum case is for when we got something that we don't expect to get according to the API documentation, so passing it through seems like a good thing to do IMO.

czechboy0 commented 8 months ago

I think that's fair and never blocking adopters from falling back one layer down is why both the URLSession and AsyncHTTPClient transports allow you to provide the underlying client, and use to make any arbitrary calls (as opposed to the library enforcing a single HTTP library that can't be swapped out).

The generated API is a convenience, type-safe way to access the documented parts of the HTTP traffic.

This has benefits for testability, because you can write unit tests completely free of any network types, purely on an input-output basis.

Once we start mixing network types into generated type-safe types, some of the type safety and the benefits that come with it will be weakened, so we're careful to only do that where justified.

In this instance, I'm happy to consider piping through some more dynamic data through the generated layer once I understand how the existing features don't serve a concrete use case. So if at any point you encounter a situation like that, it'll allow us to discuss what can be improved to cover that use case.

But right now it's still not clear, out of the several options I outlined above, which solution would work best, because we haven't established which use case we want to support and isn't supported today.

czechboy0 commented 8 months ago

Notes for ourselves about things to consider when considering extra API for accessing undocumented payloads.

There might be more. It'd be good to consider what our general solution to "access to undocumented content" is, and make sure it works across the API consistently. Since it's likely to result in API breakages, worth ensuring we consider this pre-1.0.

groue commented 8 months ago

This has benefits for testability, because you can write unit tests completely free of any network types, purely on an input-output basis.

Do you mean, even without OpenAPIRuntime.Response? Oh... I notice that this type is gone in 0.3.0 - it's probably due to SOAR-0005: Adopting the Swift HTTP Types package.

Do you have a link to a repository with some demonstration of testing techniques? I wish I could learn about the testing strategies made possible by the Swift OpenAPI libraries.

czechboy0 commented 8 months ago

Sure, it's one of the topics of the WWDC talk! https://developer.apple.com/wwdc23/10171

More detailed documentation is likely to come as we get closer to 1.0 and the API has stabilized.

The important thing here is that the generated APIProtocol uses the generated Input/Output types, and no network types. The client makes calls on a generated type that implements the protocol, and on the server you implement the type and let the transport route requests to the right method.

That means, if you have client code that uses an APIProtocol, and a server that implements APIProtocol, you could write an integration test that connects them directly, without any client or server transports, without any networking or even the need to serialize and deserialize the data.

Also looking into the PetstoreConsumerTests module could be useful, where we test the generator code itself.

czechboy0 commented 6 months ago

@groue See the PRs above, will this work for you?

groue commented 6 months ago

Yes, pretty much yes :-) πŸ™

groue commented 6 months ago

Closing in favor of #488

czechboy0 commented 6 months ago

Oh we can keep this issue open until both PRs land and get released πŸ™‚

czechboy0 commented 6 months ago

Fixed in 1.1.0.