oneOf is not produce the right results for C# generated classes?

[kendallb]

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:

openapi: 3.0.0
info:
  title: oneOf bug
  version: 1.0
components:
  schemas:
    se_id:
      title: se_id
      type: string

    calculate_rates_request_body:
      title: calculate_rates_request_body
      type: object
      required:
        - shipment_id
        - shipment
      additionalProperties: false
      oneOf:
        - $ref: '#/components/schemas/shipment_id_request'
        - $ref: '#/components/schemas/shipment_request'

    shipment_id_request:
      title: shipment_id_request
      type: object
      additionalProperties: false
      properties:
        shipment_id:
          allOf:
            - $ref: '#/components/schemas/se_id'

    shipment_request:
      title: shipment_request
      type: object
      additionalProperties: false
      properties:
        shipment:
          allOf:
            - $ref: '#/components/schemas/address_validating_shipment'

    address_validating_shipment:
      title: address_validating_shipment
      type: object
      required:
        - shipment_id
        - carrier_id
      additionalProperties: false
      properties:
        shipment_id:
          allOf:
            - $ref: '#/components/schemas/se_id'
        carrier_id:
          allOf:
            - $ref: '#/components/schemas/se_id'

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.

    public partial class CalculateRatesRequestBody 
    {
        public string ShipmentId { get; set; }
    }
    
    public partial class ShipmentRequest 
    {
        public AddressValidatingShipment Shipment { get; set; }
    }
    
    public partial class AddressValidatingShipment 
    {
        public string ShipmentId { get; set; }
        public string CarrierId { get; set; }
    }

What I would have expected to be generated is the following:

    public partial class CalculateRatesRequestBody
    {
        public string ShipmentId { get; set; }
        public AddressValidatingShipment Shipment { get; set; }
    }

    public partial class AddressValidatingShipment
    {
        public string ShipmentId { get; set; }
        public string CarrierId { get; set; }
    }

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:

    public partial class CalculateRatesRequestBody : ShipmentIdRequest
    {
    }
    
    public partial class ShipmentIdRequest 
    {
        public string ShipmentId { get; set; }
    }
    
    public partial class ShipmentRequest 
    {
        public AddressValidatingShipment Shipment { get; set; }
    }
    
    public partial class AddressValidatingShipment 
    {
        public string ShipmentId { get; set; }
        public string CarrierId { get; set; }
    }

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?

[kendallb]

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; }
    }

[kendallb]

Ok I have fixed the code for my own use, and there is a pull request for it here:

RicoSuter/NJsonSchema#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?

[jeremyVignelles]

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.

[kendallb]

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.

[kendallb]

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:

https://github.com/kendallb/ShipEngineApi/blob/master/src/ShipEngineApi/IShipEngineClient.generated.cs#L2172

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.

[kendallb]

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;
}

[mladedav]

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?

[kendallb]

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.

[RicoSuter]

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:

• Fixed support for oneOf and anyOf to produce the right result for C# classes NJsonSchema#1228
• Add code generation support for C# OneOf schemas NJsonSchema#1142

[kendallb]

Well my pull request here RicoSuter/NJsonSchema#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.

[jeremyVignelles]

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...