kiota icon indicating copy to clipboard operation
kiota copied to clipboard

Support for 200 and 201 responses that have different models

Open darrelmiller opened this issue 2 years ago • 11 comments

Example: https://github.com/apidescriptions/nightscout/blob/main/spec/cadl-output/%40cadl-lang/openapi3/openapi.yaml#L76

This is going to be challenging because we don't know what signature to return. We could pretend this is a special kind of oneOf and create a wrapper that has the 200 response and the 201 response as properties.

We need to be careful though because it would mean that adding a new 201 response with a different shape would be a breaking change to the client.

darrelmiller avatar Feb 19 '23 15:02 darrelmiller

The alternative being generating multiple executor methods that'd expect a specific response code, but that'd be ugly in terms of usage and also result in breaking change if new success responses are introduced when only one was present.

baywet avatar Feb 20 '23 14:02 baywet

The specific API that does this determines which response is going to come back based on whether the target document exists or not. A client cannot know this in advance.

This is not very common use case, but one we should track for post-GA work.

darrelmiller avatar Feb 21 '23 04:02 darrelmiller

Going the union type way would probably be a breaking change though since we'll need to pass the status code to the deserialization code (not the case today) and/or pass a flag to the request adapter so that it knows that it needs to do triage based on the status code.

baywet avatar Feb 21 '23 14:02 baywet

CSDL cannot describe this currently, therefore we are going to defer the work on this. Currently there are no open requests from the community for this.

darrelmiller avatar Mar 24 '23 15:03 darrelmiller

Thinking about this a little more, what would the user experience in an API SDK function that returns something different based on response code look like cross-language? Unions aren't supported in all languages so it's probably going to be a tuple in the unsupported ones. This means the return type must have a tag to let the user know what type was returned so they can more easily do conditional processing (in case language reflection support is weak - this is more of an issue in lower-level languages like C. Interestingly, Rust could support this scenario with a combination of sum types + pattern matching). I thought about using generics but I'm not aware of any language that supports generic return types without requiring an explicit type at the call site, so that was a non-starter. @baywet, how would your idea for multiple executor methods work in practice? Different method names with a conditional call? What would the API call-site look like?

calebkiage avatar Apr 18 '23 16:04 calebkiage

It still remains to be determined. We could use Union types, and we have wrappers for those in languages that don't support it. But this would be a very easy way to ship breaking changes over multiple generations as adding a new status code in the description would change the return type.

baywet avatar Apr 18 '23 19:04 baywet

Isn't a change in an API's return type a breaking change on the API too? Any hand-written code will need to be fixed as well, not just generated code, right?

calebkiage avatar Apr 18 '23 22:04 calebkiage

It depends on the assumption the API producers make about the client. If they assume clients rely on a dynamically typed language, one could argue that starting to return a new "type" would not be a breaking change. Even though it could derail the client logic. For statically typed languages the wrapper solution would be binary breaking when introducing a second type when it was a single one. That's unless we always include a wrapper, but that's going to increase the size of the clients significantly and degrade the experience.

baywet avatar Apr 19 '23 10:04 baywet

related https://github.com/MicrosoftDocs/openapi-docs/issues/20

baywet avatar Aug 25 '23 14:08 baywet

We did some further thinking on this issue. Effectively using methods with status codes would generate breaking changes over time, plus it'd be cumbersome to use as the client doesn't always have control over the service behavior. Not great. So we need to stick to a single executor method in those cases. The alternative for that scenario would be to project union types but now we can't do the routing between the status code and the type instance. For that reason we'd need to project multiple factories, and be able to map them to the status codes. In the majority of times, we'll have only a single mapping. In that scenario we'll have multiple mappings. We already have an infrastructure for that mapping: the error types map we pass to the request adapter. We should:

  1. generalize it.
  2. add the relevant mappings at code gen.
  3. remove the additional parameter for the successful response type from the request adapter interface.
  4. project multiple factories for each status code.
  5. project a union type in that scenario

This is a major breaking change and will be pushed to v3.

Obviously adding response types with different schemas would be a breaking change over time, but this is something we can document.

baywet avatar Aug 25 '23 15:08 baywet

To pile onto this conversation, we also need to think about the consequences of supporting different media types for structured formats. Normally, if multiple media types are supported for a structured type (e.g. application/xml and application/json) then the type is usually the same. But what if it isn't?
This means that not only HTTP status codes, but media types could cause a response to have different models.

It is worth noting that the list of structured media types supported by the generated client should be considered the candidate "accept" header list for the client and the actually header value sent by a request builder should be intersection of what is supported by the client and what is advertised by the operation response.

Also, note that media type strings could include quality values to indicate preferences.

darrelmiller avatar Sep 15 '23 21:09 darrelmiller