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

oneOf support in C# generator #808

Open
cpjolly opened this issue Nov 8, 2018 · 18 comments
Open

oneOf support in C# generator #808

cpjolly opened this issue Nov 8, 2018 · 18 comments

Comments

@cpjolly
Copy link

cpjolly commented Nov 8, 2018

oneOf is not currently supported in SwaggerToCSharpClientGenerator. See this issue for full explanation and example

@RicoSuter RicoSuter changed the title oneOf support in NSwag.CodeGeneration.CSharp.SwaggerToCSharpClientGenerator missing oneOf support in C# generator Nov 8, 2018
@RicoSuter
Copy link
Owner

Scenarios to consider:

@adamjones1
Copy link
Contributor

adamjones1 commented Jun 23, 2019

In the inheritance scenario described in #13 this is really just using inheritance to model a union type so I don't think we have two separate cases here. I would suggest then modelling oneOf as a union type in the same way as F# does when consumed from C#: as an abstract base class with a private constructor and nested sub classes each with a public constructor and a property of the case type. This would cover both cases as one in a nice way (to traverse as a union type, pattern matching can be used). Any extra properties on the oneOf type could be made base class members and any 'required' properties (common to all cases) could be base class members implemented to delegate to their case's property. I'd love to see support for this released.

@WarrenFerrell
Copy link

WarrenFerrell commented Feb 6, 2020

@RicoSuter I believe this should be closed as a duplicate of RicoSuter/NSwag#2313 ?

@RicoSuter
Copy link
Owner

@cpjolly can we close?

@cpjolly
Copy link
Author

cpjolly commented Feb 9, 2020

@RicoSuter - yes, I agree this is a duplicate and I'll close it.

@cpjolly cpjolly closed this as completed Feb 9, 2020
@adamjones1
Copy link
Contributor

Can someone explain to me how 2313 duplicates this? Is the idea that if we want to use oneOf, instead we can use non-required fields corresponding to each case? If so I don't think I agree this should be closed - firstly the fields correspond to an anyOf schema, not a oneOf schema; secondly, what if the OpenAPI spec being consumed is beyond your control and uses oneOf?

@WarrenFerrell
Copy link

WarrenFerrell commented Feb 11, 2020

@RicoSuter I actually agree with @adamjones1 . I This shouldn't be considered a duplicate. I personally had nullable problems because I couldn't get generateOptionalPropertiesAsNullable to work with but for some reason I conflated that problem with the oneOf problem that I had to solve. FWIW I implemented this manually as a generic for each oneOf option and squashed the serialization using the AdditionalProperties dictionary and some cached reflection but this was only because I didn't want to write rewrite out all the properties because I am consuming another party's openapi definition for my client.

PS FWIW This issue is a duplicate of
#13 , #673 ,
#839 ,

and related to
#302
#378

Might be nice to consolidate some of these issues.

@cpjolly cpjolly reopened this Feb 11, 2020
@cpjolly
Copy link
Author

cpjolly commented Feb 11, 2020

@RicoSuter , @adamjones1, @WarrenFerrell - I was wrong... this isn't a duplicate so I'm reopening. Hope thats what you guys want... Feel free to merge, close, etc as you see fit...

@RicoSuter
Copy link
Owner

Yeah, i was also wondering because oneOf is not really supported.. lets keep this open

@WarrenFerrell
Copy link

Thoughts on referencing https://github.com/mcintyre321/OneOf and using that to add oneOf support? It would make it extremely easy.

@adamjones1
Copy link
Contributor

I've used this library in another project before and can confirm it works well for the problem it aims to solve. I also think what it represents (an untagged union type) is a better model for oneOf payloads than my suggestion above on 23rd June 19, which models a tagged union type.

I'd be very wary of the cost of forcing a new package dependency on all clients consuming a oneOf schema though. We can just about get away with it with Json.Net but new ones should be added only when absolutely needed IMO. They can't be versioned by NSwag, adding risk when breaking API changes or even malignant changes are made. They add another point of configuration (and thus another point of failure) with NSwag clients, requiring them to add the reference to their project file/Paket files, which itself may involve gaining permission/passing vetting if their NuGet feeds are subject to a corporate whitelist. Given how little used this library is, permission might be hard to get for these clients. Given the library is apparently a one-man effort, it might not be wise to rely on its future anyway.

There's also the issue that OneOf<...> as implemented in the library isn't extendable with new properties, which is good for its typical use case, but in this situation we may want common required properties on a oneOf schema to translate to properties directly on the oneOf type rather than its child types.

In short I think I would be in favour of representing oneOf schemas in C# with types which use the approach to modelling unions in the way that the OneOf library does, but I don't think it itself should be referenced. It would be better to keep NSwag self-contained and slim (we don't need all the features of OneOf) and generate the classes (which are trivial, especially with a code generator) as part of the client instead. I'd also note I don't think it's necessary (or perhaps, even desirable) to fully replicate all the public properties it offers on OneOf<...> - just Match and Switch would be enough for me, and would keep the type run-time safe.

@adamjones1
Copy link
Contributor

I had a look at doing the above in my previous comment this weekend - submitted in PR #1142. It's not fully tested yet but I thought by this point I should try and get some feedback from you.

The approach is that when encountering a oneOf schema with no name it will generate an 'anonymous' OneOf<,> class (with the suitable arity) and a OneOfBase<,> base class. So you'll get something like

public OneOf<string, int> MyPropertyWhichIsEitherAStringOrAnInt { get; }

Thus non-named "one-ofs" are considered equivalent structurally, ie. whether they have the same case and value for that case.

If a oneOf schema does have a name it will generate a class like

public class MyNamedOneOf : OneOfBase<string, int> { ... }

and be referenced like

public MyNamedOneOf MyPropertyWhichIsEitherAStringOrAnInt { get; }

So named "one-ofs" are considered equivalent nominally. It seemed worth having support for both rather than generating names for the anonymous ones, since they make it immediately clear what their structure is just from the type signature and they save code generated.

I only added support for C# because I'm not too familiar with TypeScript, though my understanding is that TS supports untagged union types natively so it should be quite trivial to add them there for someone in the know.

Also, note that it currently is quite restrictive with the scope of what it considers a union type: there must be no properties or additional properties, etc. I thought this would be appropriate for a first pass, but it should be clear how to extend it later if wanted - a named OneOf should be generated with the extra stuff added on.

@keels4444
Copy link

Hi Adam,

With your fix, would the following schema definition be supported?

"nena-lgt:LogEventBodyType": {
"properties": {
"OneOf": [
{
"$ref": "#/components/schemas/nena-lgt%3AAdditionalAgencyBodyType"
},
{
"$ref": "#/components/schemas/nena-lgt%3AAdditionalDataQueryBodyType"
}
]
},
"type": "object"
}

Thanks,
Ryan

@adamjones1
Copy link
Contributor

@keels4444 I haven't tested that, but I believe it should do. The criteria which it tests for a schema to be considered a union type is:

bool IsUnionType => 
	Properties.Count == 0 &&
	!AllowAdditionalProperties &&
	(Items == null || !Items.Any()) &&
	AdditionalItemsSchema == null &&
	!IsEnumeration &&
	Reference == null &&
	Not == null &&
	AllOf.Count == 0 &&
	AnyOf.Count == 0 &&
	OneOf.Count > 1;

@keels4444
Copy link

@adamjones1 - Thanks for the reply. Do you know when you expect the next release to be ready? Is it possible to get a pre-release and I can help test the condition above?

@adamjones1
Copy link
Contributor

@keels4444 That would be a question for @RicoSuter, I'm not a maintainer - I would guess it would be best to wait until the PR is reviewed before doing deeper testing though as it could change a fair bit in code review.

@MariaCobretti
Copy link

this issue with oneOf not being supported is lingering around for 2 years now. @adamjones1 thank you for getting at it and I hope @RicoSuter gets a chance to review it soon. I also would appreciate a pre release package for c#

@AndreTheHunter
Copy link

@RicoSuter any update on this issue?

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

No branches or pull requests

7 participants