NSwag icon indicating copy to clipboard operation
NSwag copied to clipboard

Non-nullable properties are marked as optional in generated TypeScript

Open Kordonme opened this issue 3 years ago • 26 comments

I have the YAML below. Two properties are explicit nullable: true (completedBy, completedTime) while the others are implicit nullable: false. When generating a TypeScript client I get the following interface:

export interface ITask {
    plantId?: string;
    hoursInterval?: number;
    deadline?: Date;
    completedBy?: string | null;
    completedTime?: Date | null;
    taskId?: string;
}

I would expect all properties to not have the ? as they are not nullable. Except of course completedBy and completedTime.

openapi: 3.0.1
info:
  title: Non-nullable demo
  description: Non-nullable properties are treated as nullable in generated TypeScript
  version: v1
servers:
  - url: /v1
    description: Api version 1.0
paths:
  '/tasks':
    get:
      summary: ''
      tags: []
      operationId: getTasks
      responses:
        '200':
          description: OK
          content:
            application/json:
              schema:
                $ref: '#/components/schemas/Tasks'
components:
  schemas:
    Tasks:
      description: A list of tasks.
      type: array
      items:
        $ref: '#/components/schemas/Task'
      x-tags:
        - Tasks
      title: Tasks
    Task:
      title: Task
      type: object
      description: A task.
      properties:
        plantId:
          type: string
        hoursInterval:
          type: integer
        deadline:
          type: string
          format: date-time
        completedBy:
          nullable: true
          type: string
        completedTime:
          nullable: true
          type: string
          format: date-time
        taskId:
          type: string

Kordonme avatar Oct 16 '20 08:10 Kordonme

You need to make them all required in the spec or you turn off the generation of ? (GenerateOptionalProperties?)

RicoSuter avatar Nov 12 '20 23:11 RicoSuter

@RicoSuter Is there any way to have the nullability of the properties in the spec determine the optionality of the properties in the generated types? We're also running into this issue and would prefer not to have to annotate every non-nullable property with [Required] when we're already marking required properties by making them non-nullable.

jgilchrist avatar Nov 12 '20 23:11 jgilchrist

@RicoSuter If I turn of the generation of ? they are all ! which is also not intended.

I think @jgilchrist have some great points regarding the issue.

Kordonme avatar Nov 16 '20 08:11 Kordonme

Any updates on this?

It feels like the behaviour around [Required] changed at some point.

If I have this class in C#:

public class Test
{
  [Required]
  public int? SomeNullableInt { get; set; }
}

I'd expect "SomeNullableInt" to be generated as a (required) nullable field in TypeScript. But the nullable bit is just thrown away.

I.e. I'd expect the following property in TypeScript:

someNullableInt: number | undefined;

Rather than:

someNullableInt?: number | undefined;

Because the property itself is not optional, it's required but its value can be null.

Using the [JsonProperty(Required = Required.AllowNull)] does produce a better result but it adds the "!" modifier to the TypeScript property as noted by previous commenters.

Sharparam avatar Feb 18 '21 14:02 Sharparam

@Kordonme @Sharparam @RicoSuter Just wanted to add that we found that switching back to AddSwaggerDocument from AddOpenApiDocument fixed this for us, we can get the correct nullability based entirely on ? annotations in C#.

jgilchrist avatar Feb 18 '21 17:02 jgilchrist

Switching back from OpenAPI 3.0 to Swagger 2.0 should not be your solution... did you check out all the spec generator configs and client generator configs regarding how null is handled (there are many)?

RicoSuter avatar Feb 18 '21 22:02 RicoSuter

@Sharparam I'm not sure what you mean by they generate ! but I'm guessing maybe you are generating classes?

I am using interfaces for a typescript Angular client and when I use [JsonProperty(Required = Required.AllowNull)] I get the following which sounds like what you want:

 export interface PipelineRunData {
     pipelineRun_OrderId: string | null;
 }

I am using the "markOptionalProperties": true

I normally prefer to get lastname?: string | null; generated, but sometimes I want to require a null and this accomplishes that.

Is this maybe an interface vs. class issue?

simeyla avatar Feb 21 '21 22:02 simeyla

I'm not sure what you mean by they generate ! but I'm guessing maybe you are generating classes?

It's the classes that get the ! modifier yes, as interfaces don't support it.

The problem is that if a nullable property is marked as [Required] the TypeScript that is geneated discards the nullability property and treats it as if it's a required non-nullable property. Which makes no sense.

Sharparam avatar Feb 24 '21 16:02 Sharparam

The problem I've got is that I created a new endpoint so we can release an openApi spec (v3) alongside our swagger (v2).

Both using the same config, produces the following difference in required props in our models:

Using Swagger (v2):

"Potato": {
      "type": "object",
      "required": [
        "PotatoId"
      ],
      "properties": {
        "PotatoId": {
          "type": "integer",
          "format": "int64"
      }
}

Using OpenApi (v3):

"Potato": {
      "type": "object",
      "additionalProperties": false,
      "properties": {
        "PotatoId": {
          "type": "integer",
          "format": "int64"
      }
}

We'll revert to v2 until we find a way to "expose" all the fields as required without manually going and decorating them one by one.

Can't find this behaviour under: https://swagger.io/blog/news/whats-new-in-openapi-3-0/

andreujuanc avatar Feb 26 '21 10:02 andreujuanc

I think the reason for this is that in Swagger 2.0 you only have "required" (which is used for required and nullability) and in OAI 3 you have "required" and "nullable". So with Swagger 2.0 we need to use "required" to find out about "nullability" and thus non-nullable fields must be marked with "required" as by default missing "required" is defined as "nullable" in the code generators". In OAI 3 you have both fields and the default is optional (not-required) and not-nullable and that's why you have the change - the code gens correctly generate optional but not-nullable fields with your OAI 3 output... (nullability is quite the PITA)

until we find a way to "expose" all the fields as required without manually going and decorating them one by one.

For the v3 document registration you can register a schema processor and change the required flag on all properties automatically.

https://github.com/RicoSuter/NJsonSchema/wiki/Schema-Processors

RicoSuter avatar Mar 01 '21 08:03 RicoSuter

Thanks for the hint @RicoSuter, we had indeed tried many combinations of the different configs and weren't able to find one that had the desired effect on the required field. Registering a custom schema processor did the trick and we were able to switch back to OpenAPI 3.0.

jgilchrist avatar Mar 01 '21 15:03 jgilchrist

@RicoSuter I'm trying to understand your explanation but that still doesn't make sense. If my C# Web API defines some model with value type property (TaskId: int) ....this property never can be null so I expect that in generated schema, it will be required and not-nullable. Treating all properties as not-required by default makes absolutely no sense for all .NET value types....

Liwoj avatar Mar 15 '21 14:03 Liwoj

@jgilchrist Any chance you can share how did you implemented the schema processor and the way to register it ?

Liwoj avatar Mar 15 '21 15:03 Liwoj

@Liwoj Our schema processor looks like this:

public class MarkAsRequiredIfNonNullableSchemaProcessor : ISchemaProcessor
{
    public void Process(SchemaProcessorContext context)
    {
        foreach (var (propName, prop) in context.Schema.Properties)
        {
            if (!prop.IsNullable(SchemaType.OpenApi3))
            {
                prop.IsRequired = true;
            }
        }
    }
}

And registering it looks like this:

options.SchemaProcessors.Add(new MarkAsRequiredIfNonNullableSchemaProcessor());

I agree that the default behaviour seems strange here.

jgilchrist avatar Mar 15 '21 15:03 jgilchrist

@RicoSuter It there any way to use above ISchemaProcessor implementation with MSBuild/command line tool (nswag configuration file) ? Thx!

Liwoj avatar Mar 15 '21 16:03 Liwoj

@Liwoj A thumbs up on #2948 - seems like the feature you are requesting?

Kordonme avatar Mar 20 '21 23:03 Kordonme

EDIT yep, You are right. This is exactly what I need!

Liwoj avatar Mar 21 '21 15:03 Liwoj

@Liwoj did you find a solution?

hisuwh avatar May 21 '21 15:05 hisuwh

@hisuwh Unfortunately not - I'm sticking with Swagger 2.0 for now

Liwoj avatar May 21 '21 16:05 Liwoj

@Liwoj actually it seems to be working for me. I followed this guide for generating on build: https://blog.sanderaernouts.com/autogenerate-typescript-api-client-with-nswag

hisuwh avatar May 21 '21 16:05 hisuwh

@hisuwh It is not very clear what you mean by "working for me". I have no problem setting up schema/client generation pipeline (although I'm using classic ASP.NET + NET 4.8).

Liwoj avatar May 21 '21 16:05 Liwoj

@RicoSuter It there any way to use above ISchemaProcessor implementation with MSBuild/command line tool (nswag configuration file) ? Thx!

I'm referring to this comment. I'm using this schema processor and msbuild and I get the correct output

hisuwh avatar May 21 '21 17:05 hisuwh

@hisuwh Ok, so are you adding this ISchemaProcessor somewhere in the Startup of your API ? I guess so. I doubt this will work for classic ASP.NET as the generator is using reflection

Liwoj avatar May 21 '21 17:05 Liwoj

Exactly yh

hisuwh avatar May 25 '21 07:05 hisuwh

@Liwoj Our schema processor looks like this:

public class MarkAsRequiredIfNonNullableSchemaProcessor : ISchemaProcessor
{
    public void Process(SchemaProcessorContext context)
    {
        foreach (var (propName, prop) in context.Schema.Properties)
        {
            if (!prop.IsNullable(SchemaType.OpenApi3))
            {
                prop.IsRequired = true;
            }
        }
    }
}

And registering it looks like this:

options.SchemaProcessors.Add(new MarkAsRequiredIfNonNullableSchemaProcessor());

I agree that the default behaviour seems strange here.

Any plans on having this included as a setting or implemented in some other way? I understand that changing this to default behavior would be quite a breaking change.

It would make it so much easier to generate clients with the npm package with this behavior. And having the correct nullability on typescript clients is a real super power!

Rinsen avatar Apr 05 '22 21:04 Rinsen

Thank you for great suggestion!

@Liwoj Our schema processor looks like this:

public class MarkAsRequiredIfNonNullableSchemaProcessor : ISchemaProcessor
{
    public void Process(SchemaProcessorContext context)
    {
        foreach (var (propName, prop) in context.Schema.Properties)
        {
            if (!prop.IsNullable(SchemaType.OpenApi3))
            {
                prop.IsRequired = true;
            }
        }
    }
}

And registering it looks like this:

options.SchemaProcessors.Add(new MarkAsRequiredIfNonNullableSchemaProcessor());

I agree that the default behaviour seems strange here.

If anyone has problems with the current workaround not functioning correctly for schemas involving inheritance, this seems to be fixed by looping over Schema.ActualProperties instead of Schema.Properties

kev-andrews avatar Aug 17 '22 22:08 kev-andrews

For NSwag 14 / NJsonSchema 11, you will have to override options.SchemaSettings.SchemaProcessors instead.

kescherCode avatar Oct 12 '23 08:10 kescherCode

Hi, I have a problem with generating nullable property if it is defined as reference. Here an example of openapi:

{
  "openapi": "3.0.1",
  "components": {
    "schemas": {
      "AmountType": {
        "type": "number",
        "format": "double"
      },
      "TestObject": {
        "type": "object",
        "properties": {
          "amountClass": {
            "$ref": "#/components/schemas/AmountType"
          },
          "amountValue": {
	        "type": "number",
	        "format": "double"
          }
        }
      }
    }
  }
}

and it generates C# code with

CSharpGeneratorSettings =
{
  GenerateOptionalPropertiesAsNullable = true
}
    [System.CodeDom.Compiler.GeneratedCode("NJsonSchema", "14.0.0.0 (NJsonSchema v11.0.0.0 (Newtonsoft.Json v13.0.0.0))")]
    public partial class TestObject
    {
        [Newtonsoft.Json.JsonProperty("amountClass", Required = Newtonsoft.Json.Required.Default, NullValueHandling = Newtonsoft.Json.NullValueHandling.Ignore)]
        public double AmountClass { get; set; }

        [Newtonsoft.Json.JsonProperty("amountValue", Required = Newtonsoft.Json.Required.Default, NullValueHandling = Newtonsoft.Json.NullValueHandling.Ignore)]
        public double? AmountValue { get; set; }

    }

I can't understand why AmountClass is non-nullable like AmountValue?

PS! For Swagger 2.0 AmountClass is generated as double? like expected.

levik-buka avatar Jan 05 '24 19:01 levik-buka