OpenAPITools / openapi-generator

OpenAPI Generator allows generation of API client libraries (SDK generation), server stubs, documentation and configuration automatically given an OpenAPI Spec (v2, v3)
https://openapi-generator.tech
Apache License 2.0
21.35k stars 6.46k forks source link

[csharp-netcore] conditionalSerialization doesn't serialize required fields set in constructor and always serializes bools #13128

Open panayot-cankov opened 2 years ago

panayot-cankov commented 2 years ago

I have a model with some optional (not-required) fields. The csharp-netcore will generate code that mostly serializes nullable and bool properties even when not explicitly set.

The optionalEmitDefaultValues would set EmitDefaultValues to true and output properties even more aggressively.

The conditionalSerialization would make some things better but:

There is something off in the way optional nullable and primitive types are handled.

For optional properties, I expected if the properties are not set on the model object explicitly, that the properties will not be serialized and sent in the JSON payload.

Then in case optional properties are set on the object, even if using a default value, that they still would be serialized and sent in the JSON payload.

Issue 1: Required fields are never marked in the constructor when using conditionalSerialization

Here is what conditionalSerialization generates for the required string "Type" property:

            // to ensure "type" is required (not null)
            if (type == null)
            {
                throw new ArgumentNullException("type is a required property for ComponentUsed and cannot be null");
            }
            this._Type = type;

It misses something like:

            this._Type = type;
            if (this.Type != null)
            {
                this._flagType = true;
            }

And then it would be good.

I think the code from this: https://github.com/OpenAPITools/openapi-generator/blob/032e1a42d66517cfc2d384826164ced097ac2e6b/modules/openapi-generator/src/main/resources/csharp-netcore/modelGeneric.mustache#L189-L192

Should be copied here: https://github.com/OpenAPITools/openapi-generator/blob/032e1a42d66517cfc2d384826164ced097ac2e6b/modules/openapi-generator/src/main/resources/csharp-netcore/modelGeneric.mustache#L151-L163

Since the constructor is not setting the _flagType, the Type is then not serialized.

Currently the "Type" is not sent unless:

Issue 2: Optional primitive type fields are always marked in the constructor when using conditionalSerialization

Here: https://github.com/OpenAPITools/openapi-generator/blob/032e1a42d66517cfc2d384826164ced097ac2e6b/modules/openapi-generator/src/main/resources/csharp-netcore/modelGeneric.mustache#L189-L192

If the model has optional (but non-nullable) primitive type like bool, the generator would generate:

            this._IsTrial = isTrial;
            if (this.IsTrial != null)
            {
                this._flagIsTrial = true;
            }

The IDE would show a bunch of warnings:

Severity    Code    Description Project File    Line    Suppression State
Warning CS0472  The result of the expression is always 'true' since a value of type 'bool' is never equal to 'null' of type 'bool?'

And since the expression is always true, the _flagIsTrial is always marked and then serialized.

I don't know how to remedy this unless I make all primitive types also nullable and pass nullable around.

I also don't know how to fix that. Ideally the bool should become bool? in the constructor and if it is provided as null it should be treated as "not provided", otherwise if it has value - set the value to the field and raise the _flag*. But then I am not sure how nullable model types would be affected.

Feature Request

The way my models are, they are becoming a bit complex with some multiple inheritance, I have some code that can assign the properties from smaller types to larger types. Ideally If I could switch off constructor based null checking and have just an empty constructor for the model classes, then use always properties to assign values so they work with the conditionalSerialization. And if any validation is still necessary - have an explicit method to perform the validation after the properties are populated, perhaps like using validatable and the generated validation code, it would be perfect.


I may also be missing something, I'd be happy on some suggestions.

jornhd commented 1 year ago

I have found the same bugs, and have fixed them in modelGeneric.mustache locally. I will create a pull-request with my changes soon.

wlandgraf commented 7 months ago

This is so important that I understand conditionalSerialization is broken with this fix. Is anyone using conditionalSerialization? Why the pull request was not merged into the new csharp generator yet?

RedZone908 commented 6 months ago

I've seen this issue with conditionalSerialization too!