Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Support for 200 and 201 responses that have different models #2317

Open
darrelmiller opened this issue Feb 19, 2023 · 11 comments
Open

Support for 200 and 201 responses that have different models #2317

darrelmiller opened this issue Feb 19, 2023 · 11 comments
Labels
enhancement New feature or request generator Issues or improvements relater to generation capabilities.
Milestone

Comments

@darrelmiller
Copy link
Member

darrelmiller commented Feb 19, 2023

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 darrelmiller added the enhancement New feature or request label Feb 19, 2023
@baywet baywet added needs more information generator Issues or improvements relater to generation capabilities. and removed Needs: Triage 🔍 labels Feb 20, 2023
@baywet baywet added this to Kiota Feb 20, 2023
@baywet baywet added this to the Kiota post-GA milestone Feb 20, 2023
@github-project-automation github-project-automation bot moved this to Todo in Kiota Feb 20, 2023
@baywet
Copy link
Member

baywet commented Feb 20, 2023

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.

@darrelmiller
Copy link
Member Author

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.

@baywet
Copy link
Member

baywet commented Feb 21, 2023

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 baywet modified the milestones: Kiota post-GA, Kiota 1.1 Feb 24, 2023
@sebastienlevert sebastienlevert added blocked This work can't be done until an external dependent work is done. needs more information and removed needs more information blocked This work can't be done until an external dependent work is done. labels Mar 24, 2023
@darrelmiller
Copy link
Member Author

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.

@baywet baywet modified the milestones: Kiota v1.2, Kiota v1.4 Apr 11, 2023
@calebkiage
Copy link
Contributor

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?

@baywet
Copy link
Member

baywet commented Apr 18, 2023

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.

@calebkiage
Copy link
Contributor

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?

@baywet
Copy link
Member

baywet commented Apr 19, 2023

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 baywet removed this from the Kiota v1.4 milestone Jun 9, 2023
@baywet baywet added this to the Kiota v1.5 milestone Jun 9, 2023
@RabebOthmani RabebOthmani modified the milestones: Kiota v1.5, Kiota v1.6 Jul 7, 2023
@sebastienlevert sebastienlevert self-assigned this Aug 4, 2023
@baywet
Copy link
Member

baywet commented Aug 25, 2023

related MicrosoftDocs/openapi-docs#20

@baywet
Copy link
Member

baywet commented Aug 25, 2023

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 baywet modified the milestones: Kiota v1.6, Kiota v3.0 Aug 25, 2023
@baywet baywet removed the blocked This work can't be done until an external dependent work is done. label Aug 25, 2023
@baywet baywet assigned baywet and unassigned darrelmiller and sebastienlevert Aug 25, 2023
@darrelmiller
Copy link
Member Author

darrelmiller commented Sep 15, 2023

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request generator Issues or improvements relater to generation capabilities.
Projects
Status: New📃
Development

No branches or pull requests

5 participants