[REQ] [csharp] Support optional not-nullable properties

[SDP190]

Is your feature request related to a problem? Please describe.

Problem:
Currently, for properties that are not marked as nullable, if the property is missing from json payload, csharp generator will throw an error when deserializing, even though the property is not 'required', (thus its not included in the 'required' json schema spec field). This is a bug and needs to be handled.

In json spec world, 'required' and 'nullable' (-or type: 'null') are two separate indicators, as such even if a property is not marked as 'nullable' and therefore is not allowed to be listed in the json with a null value, still since that property is not in the 'required' json-spec field, the property is allowed to be omitted from the json at all.

Many web api's out there will handle differently a missing property, then a property with a null value. For example, in an update operation scenario, if the property is ommitted from the json body, then the update will ignore that property, vs a null value would be intercepted by the API as an instruction to actually set that property to null.

All good with json and dynamically typed languages like javascript, however with the nature of typed languages like c#, we have a problem cause the type will always have that property, and since the property is "Optional" it must return a nullable type, and as such how do we distinguish between if it was not set at all and property was just initialized to null and so should not be included in json payload, or maybe it was actually intentionally set to null and as such should be included in json.

Am wondering whats the official current recommendation to work with that, since am sure many who use this library have had this problem.

Describe the solution you'd like

I am thinking of 2 sln's here:

1. Optional properties should be of type Optional<TPropertyType> ( - remember: TPropertyType could still independently be of a nullable type as well, based on json-spec - ) where the Optional object has a IsSet property, and based on that JsonConverter (both at read/write) should work with that property. (On write serialize property only if IsSet=true, if field is required then throw error when IsSet==false, on read, do validation as mentioned and then set the Optional<TPropertyType>.IsSet based on whether that property was included in the json).

2. For each optional property make the property of type nullable (for instance, if its an int type, do int?), however, besides we should also generate another corresponding property for example if we have an optional property named MiddleInitial, we then add an additional property to that model: [NotSerializable] bool MiddleInitialIsSet {get;set;}. and as with the previous solution, JsonConverter should be configured to honor this property.

Hi @devhl-labs , your thoughts here?
Have any better alternative plans on how to handle this?

Describe alternatives you've considered

Additional context

[devhl-labs]

You raise a good point. For awareness, the null checks are there only because STJ does not have missing members support like Newtonsoft. If you need an immediate solution, you can replace the converters in your host with your own converter. I will think about this.

[mwilby]

Just to suggest an alternative approach.
There is a report of a bug in the Rust generator #16283. I am not sure its an actual problem in rust, but it does describe this issue quite well.
The solution used here is a fairly common pattern in Rust to use an Option of an Option for nullable optional variables, i.e. Option<Option<My_Type>>. It seems like you could use the same pattern.
Just modify the template to check the nullable field and set the variable accordingly. The relevant part of the Rust model template is:

    pub {{{name}}}: {{#isNullable}}Option<{{/isNullable}}{{^required}}Option<{{/required}}{{#isEnum}}{{#isArray}}{{#uniqueItems}}std::collections::HashSet<{{/uniqueItems}}{{^uniqueItems}}Vec<{{/uniqueItems}}{{/isArray}}{{{enumName}}}{{#isArray}}>{{/isArray}}{{/isEnum}}{{^isEnum}}{{#isModel}}Box<{{{dataType}}}>{{/isModel}}{{^isModel}}{{{dataType}}}{{/isModel}}{{/isEnum}}{{#isNullable}}>{{/isNullable}}{{^required}}>{{/required}},

which would translate into something like

    public {{{name}}}: {{#isNullable}}Optional<{{/isNullable}}{{^required}}Optional<{{/required}}do some funky type stuff{{#isNullable}}>{{/isNullable}}{{^required}}>{{/required}},

The hard part is adding the serialization/deserialization code, which you get for free in Rust, via serde. But this code would be generic, until you get to the actual contained object and all of the changes are just a template modification.

Note. This assumes the csharp converter sets the isNullable correctly and this may not be done if the property field is defined as a reference.

[SDP190]

@mwilby , its seems unclear what's your intent here with the double "Option" types, a single "Option" could solve this just the same, c# natively supports nullable types, so just a single Option wrapper would be used to indicate whether the value was explicitly set or wasn't

[mwilby]

I guess it's not as elegant when you have alternative options, but the intent is based on the following: If p is set to Optional<Optional<T>>, then if p.HasValue is false, this parameter was not set and doesn't exist. If it's true, the p.Value is an Optional<T> object. If its HasValue is false, then the value p represents is null. If HasValueis true the p represents p.Value.Value. Obviously there are many other ways of doing this, but this defines a general pattern that can be used for any object. Because it represents a general pattern, it means the code for various types a serialisation can be standardized. It might even be possible to put most of the code into a support library. As I said it's just a possible pattern that might be useful in building a general solution.

…

On Mon, 11 Sept 2023, 17:01 SDP190, ***@***.***> wrote: @mwilby <https://github.com/mwilby> , its seems unclear what's your intent here with the double "Option" types, a single "Option" could solve this just the same. — Reply to this email directly, view it on GitHub <#16520 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ACCN7XP2457D3AMNMFJQPCTXZ4RUHANCNFSM6AAAAAA4MLAXA4> . You are receiving this because you were mentioned.Message ID: ***@***.***>

[SDP190]

@mwilby ,

Still, as I said in c# we don't need a custom wrapper to support nullable, nullable types are supported out of the box. for example for optional nullable of type int we would use Optional<int?>. This is how it works in c#. If optional.IsSet = true, you then check optional.Value which returns a type of int? which itself can be null. as such, it wouldn't make sense to create an additional wrapper to support nullable which we can have with simple c#.

Please study "c# nullable" to get the idea.

[devhl-labs]

I believe that only works if nullable reference types are enabled.

[SDP190]

@devhl-labs

If nullable-reference type is not enabled then you would just use regular reference type which were always allowed to be null anyway.
You would then only need to use nullable-types (thus add the additional "?" symbol) for value type properties. as far as I recall I've already seen logic in the current codebase which differentiates between value types and reference types.

See example usage of #vendorExtensions.x-is-value-type here

[devhl-labs]

Of course, thanks.

[SDP190]

@mwilby ,

To make my case even stronger, in c#, int? is just syntactic sugar for Nullable<int>, so Optional<int?> is exactly the same as Optional<Nullable<int>>, this is basically the same pattern as Optional<Optional<T>>. but instead of using a custom type it uses the built-in type tailored and optimized specially for Nullable use case.

[alecjames]

I'm following up on Issue 16520 and would like to discuss a related concern that might be relevant. I've encountered a problem in C# server code when dealing with query parameters that are not marked as required. Specifically, an exception is thrown when such a query parameter is absent in a request.

I use Stoplight Studio for creating my OpenAPI documentation. I've noticed that Stoplight Studio does not automatically mark a query parameter as nullable when it's not marked as required. To address this, I manually added the nullable: true attribute to the query parameters in my OpenAPI document. However, even with this manual adjustment, the code generated by the OpenAPI Generator throws exceptions if the query parameter is missing.

As a workaround, I modified the generated server code to define the optional query parameters as nullable (Type? instead of Type in C#). This change resolved the issue, suggesting a potential gap in how the OpenAPI Generator interprets the OpenAPI document regarding nullable query parameters.

Given this experience, I wonder if the proposed solutions or discussions in Issue 16520 should also consider the handling of query parameters, particularly in scenarios where tools like Stoplight Studio and OpenAPI Generator are used together. Any insights or suggestions on this would be greatly appreciated.

[devhl-labs]

@alecjames Please open a new issue with steps to reproduce the problem including yaml and the generate command.