[Multiple languages] properties are generated as nullable/optional, despite required/nullable usage in OAS schema

[bkoelman]

Kiota ignores the usage of required/nullable in OAS schemas. C# properties are always generated as nullable. This results in unneeded noisy squiggles, as shown here.

NSwag does a better job, by performing the following translation (using the /GenerateNullableReferenceTypes:true switch):

• OAS non-required + nullable: C# nullable
• OAS required + nullable: C# nullable
• OAS non-required + non-nullable: C# non-nullable, unless combined with the /GenerateOptionalPropertiesAsNullable:true switch
• OAS required + non-nullable: C# non-nullable

Additionally, NSwag adds [Newtonsoft.Json.JsonProperty(Required = ..., , NullValueHandling = ...)] on the generated properties, which results in client-side serialization errors (thus avoiding a server roundtrip on invalid input).

Example component schema

"exampleComponent": {
  "required": [
    "requiredNonNullableReferenceType",
    "requiredNullableReferenceType",
    "requiredNullableValueType",
    "requiredValueType"
  ],
  "type": "object",
  "properties": {
    "nonNullableReferenceType": {
      "type": "string"
    },
    "requiredNonNullableReferenceType": {
      "type": "string"
    },
    "nullableReferenceType": {
      "type": "string",
      "nullable": true
    },
    "requiredNullableReferenceType": {
      "type": "string",
      "nullable": true
    },
    "valueType": {
      "type": "integer",
      "format": "int32"
    },
    "requiredValueType": {
      "type": "integer",
      "format": "int32"
    },
    "nullableValueType": {
      "type": "integer",
      "format": "int32",
      "nullable": true
    },
    "requiredNullableValueType": {
      "type": "integer",
      "format": "int32",
      "nullable": true
    }
  },
  "additionalProperties": false
}

The motivation for this behavior in Kiota is provided here and here. I find it unfortunate that correct server implementations have to suffer from this.

Please upvote this issue if you'd like Kiota to respect the required/nullable usage in your OAS.

[markm77]

Interesting that you should write about this. I'm currently moving away from AutoRest and have been evaluating Kiota and NSwag and this is the issue that is pushing me to NSwag (despite NSwag's lack of updates and catalogue of open issues). I recently watched the .NET video on Kiota and there are many things to like but unfortunately this issue is the blocker.

Basically the consequence of the current behaviour is that I would need to create a second set of models with corrected nullability which makes no sense. Or make copious manual edits to the auto-generated models.

PS: my preference would be for a "strict nullability" option which produces non-nullables for the case of "OAS required + non-nullable". Nullables for the case of "OAS non-required + non-nullable" are fine and even desirable (as null simply means property absent from serialised object).

[baywet]

Thanks @bkoelman for creating this focused issue and bringing everyone to the discussion table.
This could represent a breaking change depending on how we design the change, I've put it to the v3 milestone for the time being, but let's keep the conversation going.

I'd like to explore a solution where we wouldn't need an additional switch on the CLI. Not only this keeps things simple to anybody using kiota, this also reduces the number of cases to implement, test for and support.

I like the solution proposed by @markm77 where, if I understood well, and assuming we don't have any switch, we'd only make as non-nullable properties/return types that are required and non-nullable in the description. This is a very specific case to implement, with fewer ramifications, and potentially non-breaking. Any thoughts on that?

Assuming we go down that route, what would you expect during serialization/deserialization? if an object has a non-nullable property, the default value would be serialized when not set, potentially leading to side effects. If a payload doesn't contain the value for a non-nullable property, the default value would be set, potentially also leading to side effects?

[baywet]

Hi everyone,
We've talked in depth about the nullable aspect of this during our planning meeting. I'm going to capture the notes here to help fuel the discussion.

on adding a switch

We're pushing back against that aspect, not only for the simplicity as I outlined, but also for the maintainability.

Here is a truth table considering the different variables, which already gives us 8 different permutations. Multiply this by the number of languages, we're effectively in the 50+ permutations to implement, test, and maintain, this would represent too much and we believe that's one of the reason why some other generators out there are struggling.

required	nullable	flag
0	0	0
1	0	0
0	1	0
1	1	0
0	0	1
1	0	1
0	1	1
1	1	1

on the breaking change aspect

While depending on the project setup for a dotnet developer, this change might or might not become a breaking change, it's clearly one for other languages like Java for instance. And we strive to only break people on major revisions, so this would have to be scheduled accordingly.

Here is an example, let's assume the foo property is boolean and non nullable today in the description.
Today in Java the model would look like this.

class Model {
  Boolean foo;
}

and would change to that

class Model {
  boolean foo;
}

The change of capitalization of the type means we've effectively switched from a pointer type (true|false|null) to a value type (true|false) and the code depending on that property would need to change to some extent.

We could alleviate some of that by adding backward compatible code and making use of the ecb switch

class Model {
  @Deprecated
  Boolean foo;
  boolean getFoo() {
    if (foo == null) {
      throw;
   }
   return foo.Value;
  }
//...
}

But this gets ugly really quick.

on the serialization writer and the parsenode interfaces

This effectively means that all primitive methods in this interfaces need to be duplicated. Which would tax the implementers.

public interface ParseNode  {
	bool? GetBooleanValue();
        bool GetNonNullBooleanValue(); // this would need to be added
}

instead of doing that we could add static helpers in abstractions to reduce the burden on code generation and implementers

public static ParseNodeNonNullHelpers {
     public static bool GetNonNullBooleanValue(Func<bool?> getter) {
          if (getter() is bool result) return result;
          throw new InvalidOperationException("the value was null when it wasn't expected");
          // this is helpful since it helps with the validation of the incoming and outgoing payloads
    }
}

on microsoft graph descriptions

While investigating this we discovered the Microsoft Graph OpenAPI descriptions are inaccurate around that topic. They are being generated from CSDl by OpenAPI.net.OData, a library our team also owns. I've created two issues over there to help solve this.
microsoft/OpenAPI.NET.OData#466
microsoft/OpenAPI.NET.OData#467

We still need to think further about the implications of required. But feedback on all these notes and the previous reply is most appreciated.

[bkoelman]

I like the solution proposed by @markm77 where, if I understood well, and assuming we don't have any switch, we'd only make as non-nullable properties/return types that are required and non-nullable in the description. This is a very specific case to implement, with fewer ramifications, and potentially non-breaking. Any thoughts on that?

Sounds good to me. This matches NSwag with /GenerateNullableReferenceTypes:true /GenerateOptionalPropertiesAsNullable:true, which is what I've seen being recommended in blog posts. I guess the second switch was introduced later because (some) developers didn't like the initial puristic approach.

Assuming we go down that route, what would you expect during serialization/deserialization? if an object has a non-nullable property, the default value would be serialized when not set, potentially leading to side effects. If a payload doesn't contain the value for a non-nullable property, the default value would be set, potentially also leading to side effects?

This is where the Newtonsoft attributes come in. They drive how to (de)serialize, potentially throwing when expectations aren't met (for example, serializing a non-nullable reference-type property that is null). Kiota may need to introduce something similar, being taken into account during (de)serialization. In NSwag, a non-nullable ref/value-type property that contains its default value isn't serialized because of [Newtonsoft.Json.JsonProperty(Required = ..., , NullValueHandling = ...)] usage. Kiota can provide a superior experience when --backing-store is used, but that's an opt-in feature.

[markm77]

Thanks for the detailed replies @baywet .

Assuming we go down that route, what would you expect during serialization/deserialization? if an object has a non-nullable property, the default value would be serialized when not set, potentially leading to side effects. If a payload doesn't contain the value for a non-nullable property, the default value would be set, potentially also leading to side effects?

Considering just the proposed change case of OAS required + non-nullable, my preference would be:

• [de-serialisation] if a property is not present, a de-serialisation error results due to it being declared "required" to the de-serialiser (a received null would ideally also trigger an error)
• [serialisation] the case of the property not being set is blocked by it being required in code (e.g. through the constructor enforcing this or by some other means such as the new C# required keyword). In no case should null or a default/unset value be serialised.