fix: csharp generate not null properties

[zaytsevand]

Description

• Update the CSharp generator to render nullable properties (CSharp v8+ style) properly
• Test cases are added/adjusted to demonstrate the functionality

Related issue(s)
Fixes #941

[sonarqubecloud]

Kudos, SonarCloud Quality Gate passed!   

0 Bugs
0 Vulnerabilities
0 Security Hotspots
0 Code Smells

No Coverage information
0.0% Duplication

[jonaslagoni]

I have a bit of an issue with this change, and I am not sure how to proceed 🤔 Hope you can help @zaytsevand.

First, should the type of the model (model.type) contain the conditional type ? 🤔 Or is there cases where you would like to only have non-conditional type? For example for method arguments or other cases?

Is optional (non-required properties) the same as nullable types i.e. for inputs such as type: ['string', 'null']?

[zaytsevand]

I have a bit of an issue with this change, and I am not sure how to proceed 🤔 Hope you can help @zaytsevand.

First, should the type of the model (model.type) contain the conditional type ? 🤔 Or is there cases where you would like to only have non-conditional type? For example for method arguments or other cases?

Is optional (non-required properties) the same as nullable types i.e. for inputs such as type: ['string', 'null']?

Generally, for ? we have two cases:

• nullable value types, like bool?, which is just a shortcut for the Nullable<bool> generic type. If that were the only case, model.type would have been a perfect solution since full type specification should definitely contain ? in that case
• nullable reference types. That's a pickle... Depending on a project configuration and language version, reference types could be explicitly set to allow/disallow nulls. So we have things like object? type, which makes sense only if our project uses ? syntax to guard against null references. Otherwise, just a plain object would be equivalent to an optional property.

Currently, all new projects created for dotnet v6 (release date November 8, 2021) and c# v8+ are, by default, configured to allow nullable reference types. So I guess if we're not striving to support "legacy" projects, we could go for ? embedded in model.type.
If I'm not mistaken, Microsoft aims to push nullable references across its codebase.
But at the same time if((object)x is null) is still a perfectly valid C# (it shouldn't pass a static code check with NRT enabled).

[zaytsevand]

@jonaslagoni I checked TypeMapping, and it seems that at the time when type is determined, we can access Options (which is good, we can introduce a C#-specific option that would determine supported language and framework versions). However, we still need access to .required field for a property.

upd: figured that out. I will re-submit with a new PR later on.

[jonaslagoni]

Currently, all new projects created for dotnet v6 (release date November 8, 2021) and c# v8+ are, by default, configured to allow nullable reference types. So I guess if we're not striving to support "legacy" projects, we could go for ? embedded in model.type.

If it's easy to support legacy projects by a simple change in syntax, I would say we go for that, unless we explicitly wants to add something like a csharpVersion/dotnetVersion to switch between the syntaxes. Not sure if that would make sense, or just introduce too much complexity 🤔

@jonaslagoni I checked TypeMapping, and it seems that at the time when type is determined, we can access Options (which is good, we can introduce a C#-specific option that would determine supported language and framework versions). However, we still need access to .required field for a property.

In #947, it is actually being redesigned to give you access to whether a type is being rendered for a property through partOfProperty so you can have access to whether the property is required or not.

My question is, if the type for the property is normally string and we all of a sudden change it to be optional string?, would there be cases where you would much rather have access to string and not string? in a method signature or something similar? 🤔

[zaytsevand]

@jonaslagoni I don't think that it would be an issue if we implement the addition of '?' in TypeMapping. If somebody will need a plain string type instead of string? it's just a matter of replacing the standard TypeMapping with a custom one.
From a language standpoint, type is only fully qualified when we determine its nullability. So I don't see any problem with returning string?.

[zaytsevand]

After some code examination: when creating a model for a property, we've no outer context, but it's the outer context where we have that array of all required properties for an outer model. So a simple fix would be to embed a list of all required properties in ConstrainContext, but it will result in errors when rendering nested models since the global context will not be changed.
I can't see how I can fix the issue, unfortunately.

[jonaslagoni]

Let me push forward #947 as it will give you access to whether a property is required or not when constraining the type.

I don't think you would need access to the entire object model, do you? 🤔

[zaytsevand]

@jonaslagoni 💯 correct: the whole model is not needed for it

[zaytsevand]

@jonaslagoni could you please take another look at this one? #947 is not here yet, but rendering is broken in the next branch when auto-implemented properties are used, making it useless for C# generation, unfortunately.