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

Code Generation: Non-Required Open Api 3.0 scalar properties are non nullable #2313

Closed
andi0b opened this issue Jul 18, 2019 · 14 comments
Closed

Comments

@andi0b
Copy link

andi0b commented Jul 18, 2019

I defined an Open API 3.0 spec with optional parameters. Optional parameters mean in my understanding, that they are nullable in C# and omitted in JSON serialization if they are null in C#.

But when I generate C# code, now the generated properties are never nullable, and I have no idea how to specify now not to serialize them. Am I doing something wrong or is this a major issue?

If I set the same spec to Open API 2.0, it behaves like I expect. The integers get generated as int? as I expect.

See this spec:

openapi: "3.0.0"
servers:
  - url: //petstore.swagger.io/v2
    description: Default server

components:
  schemas:
  
    Pet:
      type: object
      required:
        - id

      properties:
        id:
          type: integer
          
        id2:
          type: integer

Generated code:

//----------------------
// <auto-generated>
//     Generated using the NSwag toolchain v13.0.2.0 (NJsonSchema v10.0.20.0 (Newtonsoft.Json v11.0.0.0)) (http://NSwag.org)
// </auto-generated>
//----------------------


    [System.CodeDom.Compiler.GeneratedCode("NJsonSchema", "10.0.20.0 (Newtonsoft.Json v11.0.0.0)")]
    public partial class Pet 
    {
        [Newtonsoft.Json.JsonProperty("id", Required = Newtonsoft.Json.Required.Always)]
        public int Id { get; set; }
    
        [Newtonsoft.Json.JsonProperty("id2", Required = Newtonsoft.Json.Required.DisallowNull, NullValueHandling = Newtonsoft.Json.NullValueHandling.Ignore)]
        public int Id2 { get; set; }
    
        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; }
        }
    
    
    }

I would like to send this Json as a response (set required id and leave out id2)

{
  "id" : 20
}
@timmvonputtkamer
Copy link

We currently face the same problem. Is there some setting we need to tweak?

@RicoSuter
Copy link
Owner

We’d need to set defaultvaluehandling to ignore, correct?

@andi0b
Copy link
Author

andi0b commented Jul 30, 2019

We’d need to set defaultvaluehandling to ignore, correct?

The problem is that the generated property is not nullable (the type is int but I expect it to be int?)

If the generated property is an object or a string, it can be set to null. If it is a number (int) it can't be set to null. It would need to be generated as int?, then it can be set to null

@RicoSuter
Copy link
Owner

The reason it is not nullable is because it is not specified as nullable in the spec, you'd need:

    id2:
      type: integer
      nullable: true

@RicoSuter
Copy link
Owner

But yes you could argue that non-required properties also should be nullable and just ignored when null whereas nullable properties should serialize "null", right?

@andi0b
Copy link
Author

andi0b commented Aug 2, 2019

But yes you could argue that non-required properties also should be nullable and just ignored when null whereas nullable properties should serialize "null", right?

That's what I mean. It's easy to deal with nullable and required properties (null is serialized as null) and for nun-nullable and non-required properties (null is left out/ignored).

This is already handled very well by the generated Json.NET attributes. But just not for value types. Just change int to int? and it works!

It only get's difficult for nullable and non-required properties; should null result in null or in a missing property. But those combination is anyway a bit useless ;)

@timmvonputtkamer
Copy link

I figured out where the problem is. Namely, the code in the NJsonSchema project needs to be updated, specifically the IsNullable method of the JsonSchemaProperty class, see https://github.com/RicoSuter/NJsonSchema/blob/master/src/NJsonSchema/JsonSchemaProperty.cs

The relevant snippet currently reads:

        public override bool IsNullable(SchemaType schemaType)
        {
            if (schemaType == SchemaType.Swagger2 && IsRequired == false)
            {
                return true;
            }

            return base.IsNullable(schemaType);
        }

It should read:

        public override bool IsNullable(SchemaType schemaType)
        {
            if ((schemaType == SchemaType.Swagger2 || schemaType == SchemaType.OpenApi3) && IsRequired == false)
            {
                return true;
            }

            return base.IsNullable(schemaType);
        }

@RicoSuter
Copy link
Owner

The problem here is that this is a huge breaking change for some...

Wouldnt it be possible to generate DefaultValueHandling

[Newtonsoft.Json.JsonProperty("id2", 
Required = Newtonsoft.Json.Required.DisallowNull, 
NullValueHandling = Newtonsoft.Json.NullValueHandling.Ignore,
DefaultValueHandling = DefaultValueHandling.Ignore)]
public int Id2 { get; set; }

And then leaving it to the default (0) would ignore it in the JSON?

@RicoSuter
Copy link
Owner

My proposal:
RicoSuter/NJsonSchema#1044

We should not fix JsonSchemaProperty class (because the schema is not really nullable) but just enhance the C# generator with an additional setting (to avoid breaking changes).

@andi0b
Copy link
Author

andi0b commented Aug 20, 2019

@RicoSuter: I think you‘re right. The property is optional and not nullable. That’s a difference C# can’t make.

Making it configurable makes sense (not breaking other people’s code), although I think the defaulting to enabled would be the correct behaviour.

@RicoSuter
Copy link
Owner

Making it configurable makes sense (not breaking other people’s code), although I think the defaulting to enabled would be the correct behaviour.

It makes sense to change this default for the C# generator - but only in the next major version (breaking change) => RicoSuter/NJsonSchema#1045

@RicoSuter
Copy link
Owner

Do you think GenerateNullableOptionalProperties is a good name? Any ideas? Can you review the PR?

@andi0b
Copy link
Author

andi0b commented Aug 21, 2019

I'll try to review it until Friday 👍

@timmvonputtkamer
Copy link

Okay, that looks like a good solution. Maybe GenerateOptionalPropertiesAsNullable is a better name for the setting?

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

No branches or pull requests

3 participants