Open kendallb opened 4 years ago
Ok here is a more simplified yaml schema:
openapi: 3.0.0
info:
title: oneOf bug
version: 1.0
components:
schemas:
request_body:
type: object
additionalProperties: false
oneOf:
- $ref: '#/components/schemas/first_id_request'
- $ref: '#/components/schemas/second_id_request'
first_id_request:
type: object
additionalProperties: false
properties:
first_id:
type: string
second_id_request:
type: object
additionalProperties: false
properties:
second_id:
type: string
and we get this:
public partial class RequestBody
{
public string FirstId { get; set; }
}
public partial class SecondIdRequest
{
public string SecondId { get; set; }
}
rather than the correct result:
public partial class RequestBody
{
public string FirstId { get; set; }
public string SecondId { get; set; }
}
Ok I have fixed the code for my own use, and there is a pull request for it here:
https://github.com/RicoSuter/NJsonSchema/pull/1228
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?
Duplicate of/related to #2970 ?
Merging entities doesn't seem ideal. The generated entity name can't be RequestBody
because some other API could use RequestBody
only. Maybe generate a RequestBodyOrSecondIdRequest
? Names will easily get long...
Now, how would you distinguish if that's a RequestBody
or a SecondIdRequest
? What if RequestBody
declares a property string A
and SecondIdRequest
a int A
?
I think that multiple "oneOf" is doable in languages like typescript, but with the C# strong typing, it's not easily doable without a custom parsing logic I'm afraid.
Names don't get along, no. Using oneOf is nasty IMHO, period. I would never design an API that way myself and if you generated a swagger spec from a set of C# controllers with a nicely designed API, you would not end up with this kind of problem. But clearly real world projects like ShipEngine are not designed with C# in mind from the get go and is probably implemented server side in node.js where everything is loosely typed. So that permeates itself to the API design and causes havoc creating clients.
So yeah, things get nasty when trying to merge the entities, but it's literally the only way to get a set of C# classes that will actually be able to generate valid JSON to call such an API.
Now I am not sure what you mean by another API using the name RequestBody? That's not relevant here at all, its just a name I picked for this example. Clearly nothing else in this particular client is going to use RequestBody since it's already used and in a real client ti would be something specific to whatever API I am calling, but that is neither here nor there.
Now as for type conflicts, yes, that can happen. The way I wrote the code is checks for type mismatches and will only allow the entities to be merged if they are the same type and it will throw an exception if they do not match. I already ran into that with ShipEngine where there was a type mis-match and I changed it the spec to be a string in both cases and asked them to change it.
But the fundamental problem is there is no way to generate a set of clean C# POCO classes that will actually work without merging the entities. Otherwise you simply end up with an API that is not usable (as I ran into using NSwag out of the box).
I have not checked to see what kind of client classes get generated for TypeScript from the same set of ShipEngine API's, but I plan to have a look at that at some point.
I know my code changes may not be suitable for anything other than generating working classes in C#, so something I was thinking about last night was adding an option to activate that code path only if it's needed, so it won't break anything else.
To help illustrate the problem, you can look at the worst case scenario in the ShipEngine API here:
https://github.com/kendallb/shipengine-openapi/blob/master/openapi.yaml#L5120
and then the actual generated class:
and then the actual documentation for this API entry point:
https://shipengine.github.io/shipengine-openapi/#operation/connect_carrier
The core problem is because the request is expecting the members from the oneOf entities to be in the base request JSON, there is no way to represent that in C# without the gross hack of merging the member entities. So basically you have a C# class with a bunch of optional parameters and you fill in the ones needed for the request. It's dirty and nasty, but it's the only viable way to actually implement a client to call the API the way they designed it.
If it was me, I would never have designed the API spec this way if you need to support strongly typed clients. A much better solution that solves this problem is to get rid of the {carrierName} from the actual API URL in the spec file:
https://github.com/kendallb/shipengine-openapi/blob/master/openapi.yaml#L1012
'/v1/connections/carriers/{carrier_name}':
I would have made an entry in the spec for every specific carrier, rather than making that a parameter that's an enum, as then we would have a strongly type API call for every carrier, along with a strongly typed request packet. But that's now how the spec is written and while it makes it completely nasty for strongly typed languages like Java and C#, it's a perfectly clean way to represent the API and works perfectly well in languages like regular Javascript.
FWIW, I just tested generation of TypeScript clients for ShipEngine, and with my code changes it produces the correct result and without them, it produces client classes that are not functional. So this affects both Typescript and C# client generation.
export class Calculate_rates_request_body extends Rate_request_options implements ICalculate_rates_request_body {
/** A string that uniquely identifies the shipment */
shipment_id?: string;
/** The shipment object */
shipment?: Address_validating_shipment;
}
export interface IRate_request_options {
rate_options?: Rate_request_body;
}
export interface ICalculate_rates_request_body extends IRate_request_options {
/** A string that uniquely identifies the shipment */
shipment_id?: string;
/** The shipment object */
shipment?: Address_validating_shipment;
}
versus
export class Calculate_rates_request_body implements ICalculate_rates_request_body {
/** A string that uniquely identifies the shipment */
shipment_id?: string;
}
export interface ICalculate_rates_request_body {
/** A string that uniquely identifies the shipment */
shipment_id?: string;
}
oneOf can be accomodated by polymorphism. From what I understood here allOf is already supported. anyOf could be potentially supported the same way as oneOf with polymorphism with having different requirements for each class while they are also merged together, but having a validation logic would seem better at that point.
Isn't this pretty major issue if NSwag doesn't really work properly with the standard? Can this issue get a bit more traction?
Yes I think it is a major issue. If a spec is generated from a strongly typed class structure like C#, then it’s easier to go back the other way. But when you have a spec like ShipEngine that is clearly written to match an API service written in Node.js, it’s easy to build a spec that just won’t work with NSwag. Mind you it does not work with the Swagger code generator either (throws internal errors) which is how I found NSwag in the first place as I needed a code generator and ShipEngine themselves didn’t know how to solve it (and they canned their old, hand written library in preference to using OpenAPI specs).
So my code changes got it all working for us and we are now in production with ShipEngine, but I imagine others will be in the same boat with the official tools.
My library generated using my modified tools can be found here:
https://github.com/kendallb/ShipEngineApi
My modified OpenAPI schema can be found here (waiting on ShipEngine to accept my changes).
https://github.com/kendallb/shipengine-openapi
My changes mostly revolve around fixing errors in their spec related to what their server actually returns in practice :) But you can try generating a client spec from my final spec with the official NSwag and see that what it produces is completely non-functional.
I'm already happy that NSwag/NJS supports AllOf inheritance, as many client gens do not even support that.
Of course OpenAPI/JSON Schema support also oneOf, if, and many other constructs which might be almost impossible to generate with a statically type language as C#...
It would be great to support these cases of course but it needs to be very well designed, the generated code must be as simple as possible and the thing must not break existing scenarios and must be very well tested.
Here some issues/PRs:
Well my pull request here https://github.com/RicoSuter/NJsonSchema/pull/1228 already gets it to work correctly for C# code generation, but as I said the unit test break. I don't want to change the unit tests to ahere to how my new code works without someone else who is more familiar with it taking a look.
But considering that AnyOf and OneOf did not work at all prior to my changes, I honestly do not believe any code is going to break with my changes because it never worked in the first place. I am pretty sure the changes are localized to just supporting those two cases and not break other cases (certainly nothing else has broken in my ShipEngine client, and it is a VERY large spec).
So I suspect the solution is for someone more familiar with the code to fix the unit tests that fail with my changes and go over what I did to decide to either accept it as is or tweak it as necessary.
The only not so clean impact I have seen with the ShipEngine client generation is some classes do get replicated at times when it is not possible to use the class names originally intended in the spec, but I don't think that is related to these changes. For instance this class:
public partial class Manifests
{
[Newtonsoft.Json.JsonProperty("manifests", Required = Newtonsoft.Json.Required.DisallowNull, NullValueHandling = Newtonsoft.Json.NullValueHandling.Ignore)]
public System.Collections.Generic.IList<Manifest> Manifests1 { get; set; }
}
Give the wrapper class is called 'Manifests' and the internal property is called 'Manifests', it has to be changed to avoid generating code that won't compile. Can't fix that really other than changing the spec, but that's not likely to happen.
Any news on this topic?
At my employer I'm working currently on a proof-of-concept for a new configuration management system. Basic idea is, that the teams will register schemas and we will provide a generic form UI for these and publish the results for consumption. For this, we have also a use case for oneOf: if one enum-value is provided, another known property must be provided as well.
Validation works fine, just the code generation is stopping me at the moment.
Following a simple schema:
{
"$schema": "http://json-schema.org/draft-07/schema#",
"title": "ConfigPOC",
"definitions": {
"mltext": {
"type": "object",
"patternProperties": {
"^[a-z]{2}$": {
"type": "string"
}
},
"additionalProperties": false
}
},
"type": "object",
"required": [
"numberValue"
],
"properties": {
"tooltip": {
"$ref": "#/definitions/mltext",
"label": {
"de": "Ein mehrsprachiger Text",
"en": "Multi-language Text"
}
},
"numberValue": {
"type": "number",
"minimum": -1.0,
"label": {
"de": "Eine Zahl >= -1.0",
"en": "A number >= -1.0"
}
},
"timeValue": {
"type": "string",
"format": "time",
"label": {
"de": "Ein Zeitpunkt",
"en": "A point of time"
}
},
"slotType": {
"type": "string",
"enum": [ "link", "flyout", "fragment" ],
"label": {
"de": "Slot-Typ",
"en": "Slot type"
}
},
"link": {
"type": "object",
"properties": {
"linkUrl": {
"type": "string",
"pattern": "^https?://",
"label": {
"de": "Link-Ziel-Url",
"en": "Link target url"
}
},
"linkText": {
"type": "string"
}
},
"required": [ "linkUrl", "linkText" ]
},
"flyout": {
"type": "object",
"properties": {
"flyoutName": {
"type": "string"
},
"fullWidth": {
"type": "boolean"
}
},
"required": [ "flyoutName" ]
},
"fragment": {
"type": "object",
"properties": {
"fragmentUrl": {
"type": "string"
}
},
"required": [ "fragmentUrl" ]
}
},
"oneOf": [
{
"properties": {
"slotType": {
"enum": [ "link" ]
}
},
"required": [ "link" ]
},
{
"properties": {
"slotType": {
"enum": [ "flyout" ]
}
},
"required": [ "flyout" ]
},
{
"properties": {
"slotType": {
"enum": [ "fragment" ]
}
},
"required": [ "fragment" ]
}
]
}
This generates the following C# classes:
[System.CodeDom.Compiler.GeneratedCode("NJsonSchema", "10.3.1.0 (Newtonsoft.Json v9.0.0.0)")]
public partial class ConfigPOC
{
[Newtonsoft.Json.JsonProperty("slotType", Required = Newtonsoft.Json.Required.DisallowNull, NullValueHandling = Newtonsoft.Json.NullValueHandling.Ignore)]
public ConfigPOCSlotType SlotType { get; set; }= default!;
private System.Collections.Generic.IDictionary<string, object> _additionalProperties = new System.Collections.Generic.Dictionary<string, object>();
[Newtonsoft.Json.JsonExtensionData]
public System.Collections.Generic.IDictionary<string, object> AdditionalProperties
{
get { return _additionalProperties; }
set { _additionalProperties = value; }
}
}
[System.CodeDom.Compiler.GeneratedCode("NJsonSchema", "10.3.1.0 (Newtonsoft.Json v9.0.0.0)")]
public enum ConfigPOCSlotType
{
[System.Runtime.Serialization.EnumMember(Value = @"link")]
Link = 0,
}
Generated with these code generation settings:
var settings = new CSharpGeneratorSettings
{
Namespace = "ConfigurationPoc.Generated",
ClassStyle = CSharpClassStyle.Poco,
RequiredPropertiesMustBeDefined = true,
GenerateDataAnnotations = true,
GenerateNullableReferenceTypes = true,
InlineNamedAny = false,
InlineNamedArrays = false,
InlineNamedDictionaries = false,
InlineNamedTuples = false
};
As you can see, even basic properties are not generated at all.
If I omit the oneOf part, then code generation is fine for the price of loosing the one validation aspect. I could image for our use case to solve this by providing some extra annotations (like our labels one) and find a work around. Feels just wrong.
How could I support to make it happen? Thanks.
Support for oneOf, anyOf and allOf is not very easy for strongly typed languages like C# that doesn't have discriminated unions.
If we want to make it in one property, we could create a base class Base
and make the subtypes inherit that base type, but we wouldn't be able to support built-in types like MyType | string
(which can easily be done in typescript for example)
We could make it an object
, but that would not be strongly typed for the C# compiler.
I'm currently trying to create an experimental OpenApi to C# generator, and the oneOf/anyOf/allOf issues are on my list.
I currently made an attempt like this :
OneOf<TLeft, TRight>
With this kind of properties and the correct JsonConverter attribute, I think I can serialize/deserialize those kind of properties, but I'm not sure how well it would play with NSwag, because I'm currently targetting net50 and System.Text.Json.
Stay tuned...
I've been working on a tool to work with OneOf : https://github.com/jeremyVignelles/ooak The idea would get that, for such scenarii, the following would be generated:
(pseudo code, not actually tested)
[JsonConverter(typeof(OneOfConverter<TLeft, TRight>))]
TypeUnion<TLeft, TRight> OneOfProperty {get;set;}
That would require to add a dependency on Ooak in the generated code, but would allow to solve the issue. Feedback is welcome :)
I've only just noticed this thread but FYI in case it helps, I've had a PR open in NJsonSchema for a long time which aims to add this feature and still hasn't had anyone review it yet: https://github.com/RicoSuter/NJsonSchema/pull/1142.
@jeremyVignelles could you provide an example on how to use ooak with an actual json schema/OpenApi doc with OneOf/Anyof? I am keen to see if this could possibly work, as support for Oneof and polymorphic lists have been open issues for years now in the NJsonSchema, and it's still unknown when the author will actually look at the open issues...
@hideintheclouds I didn't had the time yet to have a look at how to integrate that with NJsonSchema, and I'm not sure I ever will... I have the idea of writing my own OpenApi code generator, or more precisely, create a library that helps everybody write their own generator. I already have the name for it : RYOOAG (Roll Your Own Open Api Generator). To be honest, I lack time to work on such projects, so I can't announce any ETA.
I keep seeing this sentiment:
Of course OpenAPI/JSON Schema support also oneOf, if, and many other constructs which might be almost impossible to generate with a statically type language as C#...
Support for oneOf, anyOf and allOf is not very easy for strongly typed languages like C#
My focus is on oneOf and you can still get strong typing easily.
Thing:
oneof:
- "$ref": "#/components/schemas/Table"
- "$ref": "#/components/schemas/Chair"
Table:
type: object
properties:
thingType:
type: string
enum: ['TABLE']
color:
type: string
required:
- thingType
Chair:
type: object
properties:
thingType:
type: string
enum: ['CHAIR']
color:
type: string
required:
- thingType
In terms of generation, generate a Thing interface (with no methods), generated Table and Chair classes implement the Thing interface (and possibly other interfaces if they participate in other oneOf relations) and you add whatever you need in terms of class attributes or setup so that the JSON serializer knows how to deserialize a Thing (register the subclasses).
I've helped out the discrimination with the constant property thingType but there are other methods for discrimination of course.
I don't see any issue modelling anyOf or allOf in generated classes with a simliar approach and very disappointed that basic polymorphism isn't supported in csharp generation. openapi-generator has java generation working fine for oneOf (though only csharp netcore sorted in the dotnet space, again disappointing).
After assessing three openapi generation tools for csharp that all fail with oneOf it seems that openapi 3 support in the csharp world is absolutely woeful!
You might want to try my fork:
https://github.com/kendallb/NJsonSchema
I fixed the oneOf support so it works, at least for the use case we have (ShipEngine API). I don't think any of my changes ever got accepted upstream, so we always use my own tree. It has not been patched against anything recently, but we use it all the time to generate new ShipEngine clients when they change the spec (or usually when I find a bug in their spec and fix it for them :) ).
Thanks @kendallb we'll check it out.
For the convenience of my teammates here are the changes that @kendallb made:
@RicoSuter another option for our team is to extract the JSON Schema from the openapi definition and generate classes from that. The big question of course is will code generation for oneOf work with https://github.com/RicoSuter/NJsonSchema?
My fork if nswag works for the code generation side. I forget if I have my own nuget package or if I build it all as one project.
@kendallb whitespace changes make reviewing those changes of yours quite slow. I have to admit I've been guilty of that myself though!
Yeah, I changed my IDE to only trim whitespace on the lines I edit now for open source projects for that reason. It used to change every line (nice for our own projects, annoying for open source ones!). I usually use the perforce diff tool which allows me to view changes and ignore whitespace.
We eventually figured out that openapi-generator can generate working code from oneOf
elements. It took a couple of days to bump into the right parameters on a revisit (they were a bit hidden in the documentation). Our guy used netcore
generator with a targetFramework
of net4.7
and all was well.
I also realized that this stackoverflow answer ("replace the oneOf with a reference to an abstract type") was a viable workaround that might work with generators like this one (it did with openapi-generator java generator).
Interesting. What options did you end up needing?
Interesting. What options did you end up needing?
From https://github.com/OpenAPITools/openapi-generator/blob/master/docs/generators/csharp-netcore.md:
-g csharp-netcore -targetFramework net47
I've asked our dotnet guy to comment if there's more to it.
Interesting. What options did you end up needing?
From https://github.com/OpenAPITools/openapi-generator/blob/master/docs/generators/csharp-netcore.md:
-g csharp-netcore -targetFramework net47
I've asked our dotnet guy to comment if there's more to it.
@kendallb, @davidmoten
Dave basically had it, this what we're using (besides some project specific parameters).
openapi-generator-cli generate -i <schemafile> -g csharp-netcore --additional-properties targetFramework=net47
The hidden part was just that the csharp-netcore generator also supports .Net 4.7. Didn't realise that until I had a proper look at its docs.
Thanks, I might take a look at that.
Thank you @kendallb for your fork! I am not very experienced in this area, but a question came to me, when I was looking at some generated code based on a (Soap) schema that includes choices too. Taking your code posted on Aug 8 as the base for my example, it would generate the following:
public partial class RequestBody {
[System.Xml.Serialization.XmlElementAttribute("FirstId", typeof(string)]
[System.Xml.Serialization.XmlElementAttribute("SecondId", typeof(string)]
[System.Xml.Serialization.XmlChoiceIdentifierAttribute("ItemElementName")]
public object Item { get; set; }
[System.Xml.Serialization.XmlIgnoreAttribute()]
public ItemChoiceType1 ItemElementName { get; set; }
}
public enum ItemChoiceType1 {
FirstId,
SecondId,
}
Notes:
I was wondering if this would be the solution to this problem. From my understanding, this would also solve the problem of distinguishing properties with similar names as mentioned by @jeremyVignelles on Aug 11. Any thoughts on that?
We're currently also facing this issue. We generate OpenAPI specifications from our .NET services and then generate Javascript/React and .NET clients for use in frontends. Our OpenAPI specification contains oneOf, and the .NET client is generated using the first of the four as the static type. This being a .NET service, the four possible oneOf values all inherit the same base class, so we expected the base class to be used as the type. But alas.
Of course, by the time the .NET client is generated, the knowledge of a .NET service is long gone. The fact that there is a base class, on the other hand, can be deduced from the schema in the OpenAPI specification, but I can't see that this is done.
It seems a reasonable way to generate a .NET/C# client is to find the nearest superclass of all the oneOf types. If that ends up being object, isn't that fine? And isn't that much better than using the first subclass discovered, which is guaranteed to be wrong most of the time?
openapi-generator might worth a try in generating the C# clients for an OpenApi with oneOf
The issue for us is that the base type is abstract. It therefore isn't included in the oneOf list. If we make it non-abstract, it is included and is always first. This was probably a conscious choice at some point, just not very well documented.
We have switched from nswag (as it seems there is no progress on oneof, anyof etc and the first schema always get generated..) to openapi-generator. While there are probably still some open bugs related to polymorphism affecting the csharp-netcore generator, they do seem to support oneOf in a better way (check their readme, short term plan + open and closed GitHub issues)
Might have to check it out and see if it works better for the ShipEngine API - that is why I did most of my changes that got the oneOf stuff working for me. But it never got accepted into the upstream code so I maintain it myself.
The problem is in fact rather simple to handle. But it has to be understood what is to be solved here.
The oneOf in OAS is for sure very important, and useful. The theory here is not that simple but a record/object is a product type and this oneOf is a sum type. C# mainly supports product types.
The use cases are many, and its much simpler cognitive to think about "one of these things" and having your programming language, spesification and what not support that.
the classic use case is payment which then can be EITHER card or cash. And then having a really static typed language supporting this EITHER then the model in code, specification in OAS and generation of code are quite simple.
To top it is also possible to do in c# if some of the underlying theory is understood together with understanding also of the use cases of course. And yes it will obviously be a bit opiniated in the end since c# not really (yet!) support this kind of types. The names are many:
https://en.wikipedia.org/wiki/Tagged_union https://en.wikipedia.org/wiki/Product_type
More can be found around the net with eaxamples like: https://messerli-informatik-ag.github.io/code-style/algebraic-datatypes.html
I created a simple example in f# and made sharplab compile it and then create c# of it... Mind that it does in fact resemble also the way to model it in previous link (ok, if squinting a bit).
Are there any updates on supporting oneOf in generated C# code?
Any news with this?
When trying to generate an API client for ShipEngine, NSwag is having issues when using oneOf to combine related entities to indicate that the request can have one of the two values. It ends up just bringing in the type for the first value, but the second is never included. Here is the simplified yaml file:
The C# class generated is the following. Note that the it only contains the base ShipmentId field, and not the second Shipment field which the schema happily supports.
What I would have expected to be generated is the following:
If I change it to be allOf rather than oneOf, I get the following which does work. But in reality the spec is correct in saying it's a oneOf and not allOf, because only one can be sent in the request at a time, not all of them:
So it seems to me when generating a C# class for this specification, we should really be generating the same schema as the allOf case. Any ideas on the best way to fix this in the code? I assume it's probably in the JsonSchema rendering code, not NSwag itself?