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

Fixed support for oneOf and anyOf to produce the right result for C# classes #1228

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

kendallb
Copy link

Referenced in this issue:

RicoSuter/NSwag#2991

The only valid solution to generation of classes in C# to support anyOf and oneOf, is to merge all the properties together into the request and response classes, including flattening the inherited properties as inheritance cannot be used here. It's kind of ugly to use oneOf and anyOf IMHO anyway, but that's how some API's are designed (like ShipEngine), and without these changes this will generate bogus request and response classes.

I have added some unit tests for the new code I wrote, but it breaks some of the other tests so it's not clear if those tests need to be fixed to suit the new code, or if we need to find a different way to enable this support for C# code generation if it will break other languages?

…they should be collected in there or we get nothing in the output.

Also no idea why we are returning the schema for the first non-nullable oneOf entry, as that is completely wrong and will not allow oneOf to work correctly even with nullable entries?

Added unit tests.
…the underlying type is the same. That is the only viable way to represent that kind of type in C#.
…se inheritance here and we end up with empty request/response classes.
@RicoSuter
Copy link
Owner

CI build is failing...

@kendallb
Copy link
Author

Yes as mentioned this has broken other unit tests, but it's not clear to me if I should fix the tests. Someone needs to take a closer look because it's possible the tests are testing for broken behavior. There might be a reason some of the things I changed were done the way they are, but as I pointed out, without these changes it does NOT produce working client code for C# or Typescript. So I am not clear on what exactly was expected with the code written the way it was previously.

@Niranjan-S
Copy link

@RicoSuter @kendallb We are waiting for this check-in, Please let it go. Thanks

@kendallb
Copy link
Author

As I said originally, these changes actually get the proxy generator to generate usable clients for me in both C# and Typescript. But I don't want to change the unit tests to match my new code as it is entirely unclear to me why it was written the way it was in the first place and the unit test are clearly testing for the behavior. So someone more familiar with why things were done the other way really needs to take a look at this.

IMHO without these changes NSwag is useless for quite a lot of folks where the OpenAPI spec is using AllOf and OneOf etc, as it will not generate useable clients at all. My custom NSwag generates useable clients for me now (not entirely clean IMHO, but it works and it's not really possible to generate a super clean API for C# if the spec is written that way - see the ShipEngine API I referenced earlier here RicoSuter/NSwag#2991 (comment)).

StefanWetterActiware added a commit to StefanWetterActiware/NJsonSchema that referenced this pull request Nov 15, 2022
@dontboyle
Copy link

2 years later - is this implemented in another way? still not seeing correct results.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants