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

Non-Required fields generate Non-Nullable properties #838

Open
pazoozooCH opened this issue Dec 6, 2018 · 32 comments
Open

Non-Required fields generate Non-Nullable properties #838

pazoozooCH opened this issue Dec 6, 2018 · 32 comments

Comments

@pazoozooCH
Copy link

My issue is related to the now closed issue #82 where you took the position that if we wanted a field to be nullable, it should not only be optional (= not required), but also nullable.

While I understand that these are two distinct concepts (a field not being submitted at all vs. a field being submitted as null), this distinction is not mappable to a simple C# class. My property can either be null or not null, it cannot be "not defined".

So in the context of C#, I would expect that a non-required field results in a nullable property (e.g. int? instead of int). Otherwise, it simply won't be possible to convert a perfectly valid JSON document into the corresponding C# object. Would you agree to this?

Furthermore, it is currently not possible to have nullable enums in NSwag (even if they have "type": "null" in JSON Schema). While this might be seen as a different issue, it could also be fixed (at least in my context) with an implementation treating non-required fields as nullable.

I would prepare a fix for this issue as we vitaly need this in our project. Would you generally support to merge such a fix or do you have any other considerations?

btw. I've also discussed this with @lbovet which supports this approach (we're using NSwag in combination with https://github.com/swisspush/apikana).

@lbovet
Copy link

lbovet commented Dec 6, 2018

I suggest that we provide this feature "behind a flag", i.e. that the current behavior stays the default and that we activate the "non-required-as-nullable" feature explicitely and on demand.

@pazoozooCH
Copy link
Author

@RSuter , could you maybe comment on how you see this and give some hints on where this would need to be fixed? Unless you have immediate resources to fix it yourself, of course... 😎

@lbovet
Copy link

lbovet commented Dec 6, 2018

This should actually be solved in the NJsonSchema project, I think.
This line sets the nullability (the "?") of the generated property: https://github.com/RSuter/NJsonSchema/blob/master/src/NJsonSchema.CodeGeneration.CSharp/Models/PropertyModel.cs#L42
It uses only the "nullable" property and not the "required/optional" information.
I guess we should change this line to:

public override string Type => _resolver.Resolve(_property, _property.IsNullable(_settings.SchemaType) || !property.IsRequired && _settings.NonRequiredAsNullable, GetTypeNameHint());

And also add this "NonRequiredAsNullable" property in the configuration.

@RicoSuter
Copy link
Owner

This should actually be solved in the NJsonSchema project, I think.

Correct all DTO schema handling stuff is in NJsonSchema and not in NSwag.

It uses only the "nullable" property and not the "required/optional" information.

This is not 100% correct, see: https://github.com/RSuter/NJsonSchema/blob/master/src/NJsonSchema/JsonProperty.cs#L92

@RicoSuter
Copy link
Owner

I already spent days for the nullability and undefined handling and I think it is fine as is. The problem is that there are three different "dimensions" which change the actual handling:

  1. Schema generator: The used schema type (JSON Schema, Swagger 2 and OpenAPI 3) have a big impact on how the schema looks like:
    • JSON Schema: "required" => cannot be undefined but can be null if "type" = "null" => nullable
    • Swagger 2: "required" => cannot be undefined and cannot be null (there is no way in Swagger 2 to express nullability => that's why we introduced x-nullable)
    • OpenAPI 3: "required" => cannot be undefined but can be null if "nullable" = true => nullable
  2. Client generator:
    • In TypeScript, the target TS version is important
  3. Settings and annotations

So I think it a good start is to actually document the different behaviors with samples in the NJS wiki and then decide whether we need to change something. The problem with changing something is that it is a huge breaking change for many users...

@pazoozooCH
Copy link
Author

Thanks for the clarifications. I feel this is getting a bit complex for my problem. Let me get one step back and outline my use case again:

  • Input: JSON Schema v4 (actually I start from TypeScript, but that doesn't really change too much here)
    • Field: not required, no nullable information (i.e. not nullable); primitive type: number, integer, enum, ...
  • Output: C# class, generated by NSwag.MSBuild

So I want to be able to parse JSON to C# classes and generate JSON from C# classes, pretty standard use case I assume.

When I get a JSON now without the field that is not required (which is 100% legit according to the Schema), I simply cannot parse that into the C# class if the type is not nullable. So I would say the required handling in this (not too unusual) context is not fine at all.

I guess I'm just using a small part of NSwag (and NJsonSchema for that matter) and cannot easily predict any side effects in other areas. So I was hoping that this adaptation could be made as isolated as possible in order to avoid breaking changes in other areas that might have more options in differing null and undefined as C# has. But in the context of C# classes generation from JSON Schema, I don't see how this change would break anything as nobody would be able to actually use it as is...

@RicoSuter
Copy link
Owner

Can you provide a sample schema, some C# output and sample data?

@RicoSuter
Copy link
Owner

So you are talking about JSON Schema, not Swagger or OpenAPI? If this is the case, we should move this issue over to NJsonSchema

@pazoozooCH
Copy link
Author

pazoozooCH commented Dec 7, 2018

Ok, here's a very simplistic sample demonstrating the issue:

TypeScript (just for completeness, dont' worry too much about this one):

export enum MyEnum {
    Val1,
    Val2
}

export interface MySimpleType {
    myOptionalValue?: number;
    myOptionalEnum?: MyEnum;
}

This generates the following JSON Schema v4 files:

my-enum.json:

{
  "$schema": "http://json-schema.org/draft-04/schema#",
  "definitions": {},
  "type": "string",
  "enum": [
    "Val1",
    "Val2"
  ],
  "id": "MyEnum"
}

my-simple-type.json:

{
  "$schema": "http://json-schema.org/draft-04/schema#",
  "definitions": {},
  "type": "object",
  "properties": {
    "myOptionalValue": {
      "type": "number"
    },
    "myOptionalEnum": {
      "$ref": "my-enum.json"
    }
  },
  "additionalProperties": false,
  "id": "MySimpleType"
}

Now I take that JSON Schema and use NSwag.MSBuild to generate C# classes: <Exec Command="$(NSwagExe_Core20) jsonschema2csclient /name:MySimpleType /output:$(ProjectDir)/gen/MySchema.cs /input:$(ProjectDir)/dist/model/json-schema-v4/my-simple-type.json" />

This will create a class that looks as this (excerpt):

    public partial class MySimpleType : System.ComponentModel.INotifyPropertyChanged
    {
        private double _myOptionalValue;
        private MyEnum _myOptionalEnum;
        ...

Now consider I have an empty JSON document {} as input which is perfectly legal with this Schema as both fields are not required. It will not be mappable to the class. Both fields / properties should / must be optional (double?, MyEnum?) in the generated class to support this case.

@pazoozooCH
Copy link
Author

btw in my case, it's JSON Schema. But I feel it's more an issue of treating optional and nullable different in the context of C# class generation while they cannot really be distinguished in a POCO. So I'm not sure it wouldn't be the same problem with Swagger or OpenAPI as well...

@pazoozooCH
Copy link
Author

And yes, for the number field, I could add "null" as type and it would result in double?. But when I do the same with a $ref enum, NSwag starts introducing weird, empty helper classes...
So I figured it would be better to fix the unfortunate non-required-primitive handling instead of fiddling with "null" in the JSON-Schema...

@RicoSuter
Copy link
Owner

RicoSuter commented Dec 7, 2018

I used the playground here: https://apiverse.io/tools

With this schema:

{
  "$schema": "http://json-schema.org/draft-04/schema#",
  "definitions": {},
  "type": "object",
  "properties": {
    "myOptionalValue": {
      "type": "number"
    },
    "myOptionalEnum": {
      "enum": [
        "Val1",
        "Val2"
      ]
    }
  },
  "additionalProperties": false,
  "id": "MySimpleType"
}

With schemaType = JsonSchema, I get the following:

        [Newtonsoft.Json.JsonProperty("myOptionalValue", Required = Newtonsoft.Json.Required.DisallowNull, NullValueHandling = Newtonsoft.Json.NullValueHandling.Ignore)]
        public double MyOptionalValue { get; set; }
    
        [Newtonsoft.Json.JsonProperty("myOptionalEnum", Required = Newtonsoft.Json.Required.DisallowNull, NullValueHandling = Newtonsoft.Json.NullValueHandling.Ignore)]
        public RootMyOptionalEnum MyOptionalEnum { get; set; }

As you can see, all properties are not nullable but allow undefined - in this case the default of the enum/double is used. This is a compromise for JSON Schema so that not all properties are nullable but do not allow null (this is even more confusing).

If you use schemaType = Swagger2, you'll get what you expect (because Swagger2 does not know null):

        [Newtonsoft.Json.JsonProperty("myOptionalValue", Required = Newtonsoft.Json.Required.Default, NullValueHandling = Newtonsoft.Json.NullValueHandling.Ignore)]
        public double? MyOptionalValue { get; set; }
    
        [Newtonsoft.Json.JsonProperty("myOptionalEnum", Required = Newtonsoft.Json.Required.Default, NullValueHandling = Newtonsoft.Json.NullValueHandling.Ignore)]
        public RootMyOptionalEnum? MyOptionalEnum { get; set; }

But for your scenario, we'd need to add a new setting for the C# generator and schemaType = JsonSchema. But this is purely an NJsonSchema issue...

@RicoSuter RicoSuter transferred this issue from RicoSuter/NSwag Dec 7, 2018
@pazoozooCH
Copy link
Author

Ok - so would you say the input from @lbovet (#838 (comment)) is appropriate then?

How do we proceed? Should I make a fix and submit a PR or do you have time to have a look at this yourself (in the near future)?

@RicoSuter RicoSuter added this to the vNext milestone Dec 7, 2018
@RicoSuter
Copy link
Owner

I think @lbovet's suggestion more or less sums it up... I'll look into it.

@RicoSuter
Copy link
Owner

For now, you could implement your own resolver based on CSharpTypeResolver and override Resolve like this:

public override string Resolve(JsonSchema4 schema, bool isNullable, string typeNameHint)
{
    if (!isNullable && schema is JsonProperty property)
    {
        isNullable = !property.IsRequired;
    }

    return Resolve(schema, isNullable, typeNameHint, true);
}

This should do the trick for you (not tested).
The question is whether this should be in the library itself behind a setting...

@pazoozooCH
Copy link
Author

Hm, what's the most pracmatic way to do this? Do I need to make a custom version of NJsonSchema and reference that from a custom version of NSwag or can I override this somehow?

@RicoSuter
Copy link
Owner

You would need to write your own cli and reference the NJsonSchema library via NuGet... NSwag is just the CLI host and is not used in this scenario. This way you have access to much more extension points than just the settings.

@pazoozooCH
Copy link
Author

Ok, I'll look into it.

Thanks for your fast support so far! 👍

@pazoozooCH
Copy link
Author

@RSuter , I've performed the C# generation with a patched CSharpTypeResolver as suggested by you. Worked for both number and enum 👍

Can you make any kind of forecast on how fast you might have processed this issue into an official NSwag.MSBuild release (days, weeks, months, ...)?

@chriscerk
Copy link

Voicing the same issue - I would expect NJsonSchema to have a setting in order to generate a C# attribute with Newtonsoft.Json.Required.Default instead Newtonsoft.Json.Required.DisallowNull for any arbitrary JSON Schema property. I found NullHandling at https://github.com/RSuter/NJsonSchema/wiki/CodeGeneratorSettingsBase, but couldn't find it anywhere in the codebase.

Example:
for JSON Schema property

...
"properties":
 "name": {
          "title": "Name",
          "type": "string",
          "description": "The display name of the resource."
        }
...etc

generates

[Newtonsoft.Json.JsonProperty("name", Required = Newtonsoft.Json.Required.DisallowNull, NullValueHandling = Newtonsoft.Json.NullValueHandling.Ignore)]
public string Name { get; set; }

@Kharos
Copy link

Kharos commented Jul 3, 2019

We just got hit by this using OpenApi. It is not a big issue as we can easily work around it by adding nullable: true to the OpenApi file, but it is very much not intuititve. We have a entity that has properties (a dictionary) and a optional id. Our schema looks like the following:

ApiObject:
  required: 
    - properties
  properties:
    id:
      type: integer
      format: int64
    properties:
      type: object
      additionalProperties:
        type: object

The generated client is "broken" as when I do the following:

var object = new MyObject();
// add some properties

The serialized json has Id: 0 even though I never set the Id to a value. So I think this should be considered a generator bug.

@BertusVanZyl
Copy link

I have come across this issue as well.

All my API calls return this c# class:

public class StandardResponse<T>
    {
        public T Payload { get; set; }
        public bool Success { get; set; }
        public string ErrorMessage { get; set; }
        public int ErrorCode { get; set; }
        public string APICallGuid { get; set; }
    }

If there is some kind of error, then Success, is false, and ErrorMessage and ErrorCode is populated. Payload is not populated, because there was an error. Since I upgraded all my swagger related nuget packages (because of moving to core 3.0.0), the c# client generated by nswag chokes on every single error coming back from the API, because the class generated has Required = Newtonsoft.Json.Required.DisallowNull set on the Payload property.

@AdamDiament
Copy link

AdamDiament commented Jun 16, 2020

I have come across this issue as well.

All my API calls return this c# class:

public class StandardResponse<T>
    {
        public T Payload { get; set; }
        public bool Success { get; set; }
        public string ErrorMessage { get; set; }
        public int ErrorCode { get; set; }
        public string APICallGuid { get; set; }
    }

If there is some kind of error, then Success, is false, and ErrorMessage and ErrorCode is populated. Payload is not populated, because there was an error. Since I upgraded all my swagger related nuget packages (because of moving to core 3.0.0), the c# client generated by nswag chokes on every single error coming back from the API, because the class generated has Required = Newtonsoft.Json.Required.DisallowNull set on the Payload property.

@BertusVanZyl I have the exact same issue with nullable generic payload property after upgrading to OpenApi 3.0.0. Did you find any workaround for this? I've tried

[JsonProperty(Required = Required.AllowNull)]
public T Payload { get; set; }

but it doesn't seem to affect the swagger schema file that swashbuckle generates for me.

At the moment the only workaround I've found is to find and replace
[Newtonsoft.Json.JsonProperty("payload", Required = Newtonsoft.Json.Required.DisallowNull
with
[Newtonsoft.Json.JsonProperty("payload", Required = Newtonsoft.Json.Required.AllowNull

in my generated CSharp client.

I'd prefer to find an option in swashbuckle (or even .net core intercept routing) to edit the schema so it goes from this:

 "properties": {
          "payload": {
            "$ref": "#/components/schemas/MyResultClass"
          },

to this:

 "properties": {
          "payload": {
             "nullable": true,
            "$ref": "#/components/schemas/MyResultClass"
          },

But I haven't found a way to do that - @RicoSuter do you know of any way round this?

@RicoSuter
Copy link
Owner

In NSwag you'd do this:
RicoSuter/NSwag#2986 (comment)

But I dont know Swashbuckle enough...

@andresodio
Copy link

@AdamDiament I was about to share a fix I found with you, but then I realized it was you yourself who posted it on Stack Overflow!

Thank you so much for doing that. I had already tried a million things and your solution worked:

https://stackoverflow.com/a/62434070

@TiraelSedai
Copy link

TiraelSedai commented Jul 16, 2021

I also stumbled on the bug, NSwagStudio 13.11.3.0.

The worst part is not it's non-nullable, but that NSwag doesn't think they can be null and throws ArgumentNullException instead of omitting the property.

For the class

    public record CommandEventModel
    {
        /// <summary>
        /// Screenshot, if it's available.
        /// </summary>
        public IFormFile? Screenshot { get; init; }
        [BindRequired]
        public byte[] Command { get; init; } = Array.Empty<byte>();
    }

I have swagger

...
        "requestBody": {
          "content": {
            "multipart/form-data": {
              "schema": {
                "required": [
                  "Command"
                ],
                "type": "object",
                "properties": {
                  "Screenshot": {
                    "type": "string",
                    "description": "Screenshot, if it's available.",
                    "format": "binary"
                  },
                  "Command": {
                    "type": "string",
                    "description": "The command in binary format.",
                    "format": "byte"
                  },
                }
              },
              "encoding": {
                "Screenshot": {
                  "style": "form"
                },
                "Command": {
                  "style": "form"
                },
              }
            }
          }
        },

But the NSwagStudio generates this (notice that screenshot is expected never to be null but command can be null; however swagger schema expects the opposite):

        public async System.Threading.Tasks.Task ApiCommandsAsync(..., string? api_version, FileParameter screenshot, byte[]? command, System.Threading.CancellationToken cancellationToken)
        {
                    ...
                    if (screenshot == null)
                        throw new System.ArgumentNullException("screenshot");
                    else
                    {
                        var content_screenshot_ = new System.Net.Http.StreamContent(screenshot.Data);
                        if (!string.IsNullOrEmpty(screenshot.ContentType))
                            content_screenshot_.Headers.ContentType = System.Net.Http.Headers.MediaTypeHeaderValue.Parse(screenshot.ContentType);
                        content_.Add(content_screenshot_, "Screenshot", screenshot.FileName ?? "Screenshot");
                    }
                    if (command == null)
                        throw new System.ArgumentNullException("command");
                    else
                    {
                        content_.Add(new System.Net.Http.StringContent(ConvertToString(command, System.Globalization.CultureInfo.InvariantCulture)), "Command");
                    }

Which completely aborts the call if screenshot is null, instead it should have been

                    if (screenshot != null)
                    {
                        var content_screenshot_ = new System.Net.Http.StreamContent(screenshot.Data);
                        if (!string.IsNullOrEmpty(screenshot.ContentType))
                            content_screenshot_.Headers.ContentType = System.Net.Http.Headers.MediaTypeHeaderValue.Parse(screenshot.ContentType);
                        content_.Add(content_screenshot_, "Screenshot", screenshot.FileName ?? "Screenshot");
                    }

@BertusVanZyl
Copy link

I have come across this issue as well.
All my API calls return this c# class:

public class StandardResponse<T>
    {
        public T Payload { get; set; }
        public bool Success { get; set; }
        public string ErrorMessage { get; set; }
        public int ErrorCode { get; set; }
        public string APICallGuid { get; set; }
    }

If there is some kind of error, then Success, is false, and ErrorMessage and ErrorCode is populated. Payload is not populated, because there was an error. Since I upgraded all my swagger related nuget packages (because of moving to core 3.0.0), the c# client generated by nswag chokes on every single error coming back from the API, because the class generated has Required = Newtonsoft.Json.Required.DisallowNull set on the Payload property.

@BertusVanZyl I have the exact same issue with nullable generic payload property after upgrading to OpenApi 3.0.0. Did you find any workaround for this? I've tried

[JsonProperty(Required = Required.AllowNull)]
public T Payload { get; set; }

but it doesn't seem to affect the swagger schema file that swashbuckle generates for me.

At the moment the only workaround I've found is to find and replace
[Newtonsoft.Json.JsonProperty("payload", Required = Newtonsoft.Json.Required.DisallowNull
with
[Newtonsoft.Json.JsonProperty("payload", Required = Newtonsoft.Json.Required.AllowNull

in my generated CSharp client.

I'd prefer to find an option in swashbuckle (or even .net core intercept routing) to edit the schema so it goes from this:

 "properties": {
          "payload": {
            "$ref": "#/components/schemas/MyResultClass"
          },

to this:

 "properties": {
          "payload": {
             "nullable": true,
            "$ref": "#/components/schemas/MyResultClass"
          },

But I haven't found a way to do that - @RicoSuter do you know of any way round this?

I went into the middleware somewhere, and checked if it was null. If it was null, I just wrote "new object()" into the property. At least the it no longer chokes on nulls.

@AdamDiament
Copy link

I went into the middleware somewhere, and checked if it was null. If it was null, I just wrote "new object()" into the property. At least the it no longer chokes on nulls.

@BertusVanZyl see my fix/ workaround posted here https://stackoverflow.com/a/62434070

@tsinghammer
Copy link

I get that via the OpenApi specification nullable fields can be declared by nullable: true.
What seems to be a bit inconsistent to me is that if a field is not declared as nullable: true an Generate Nullable Reference Type is selected, NSwag generates:

[Newtonsoft.Json.JsonProperty("revenue", Required = Newtonsoft.Json.Required.DisallowNull, NullValueHandling = Newtonsoft.Json.NullValueHandling.Ignore)] public string? Revenue { get; set; } = default!;

That means although the value can be null from a C# perspective, it can never be if it is populated from a JSON.
Shouldn't the nullable reference declaration depend on the nullable definition in OpenApi, too?

@henryvalentine
Copy link

please how do I configure NJsonSchema to produce nullable option for types (from a .Net Class) should those element not be available on a json data to be validated?

I need something like this for all types within the schema:

"oneOf": [
{
"type": "null"
},
{
"$ref": "#/definitions/AddressType"
}
]

OR

"Hobbies": {
"type": [
"array",
"null"
]

OR

"FavoriteSport": {
"type": [
"null",
"string"
]

where any of the above and more is needed to be the case.

Right now the nullable type option is provided for some types while it is not produced for some and this makes my json data validation to fail if those type are not present in the data

@stijnherreman
Copy link
Contributor

I wonder if https://github.com/dotnet/csharplang/blob/main/proposals/required-members.md could be used in the schema generator, in addition to the current methods for marking a field/property as required.
It was recently mentioned in the related discussion in dotnet/csharplang that the feature has "a very good chance" to make it into C# 11.

@tsinghammer
Copy link

What do you think of providing a setting for this behavior, like in this PR?
#1563

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