[C#] Optional/Required properties

[aleksei-saharov]

Hi, all

I have issue with client C# code generator on http://editor.swagger.io

YAML:

Entity:
    type: object
    required:
      - Values
      - Unit
      - Start
    properties:
      Values:
        description: Array of values.
        type: array
        items:
          type: number
          format: double
      Unit:
        $ref: '#/definitions/MyEnum'
      Start:
        description: Start.
        type: number
        format: double
MyEnum:
    type: string
    enum: [
      val1,
      val2,
      val3]

http://editor.swagger.io generate following code:
C# (unnecessary parts removed):

    public partial class Entity
    {
        protected Entity() { }
        /// <summary>
        /// Initializes a new instance of the <see cref="Entity" /> class.
        /// </summary>
        /// <param name="Values">Array of values. (required).</param>
        /// <param name="Unit">Unit (required).</param>
        /// <param name="Start">Start. (required).</param>
        public Entity(List<double?> Values = null, MyEnum Unit = null, double? Start = null)
        {
            // to ensure "Values" is required (not null)
            if (Values == null)
            {
                throw new InvalidDataException("Values is a required property for Entity and cannot be null");
            }
            else
            {
                this.Values = Values;
            }
            // to ensure "Unit" is required (not null)
            if (Unit == null)
            {
                throw new InvalidDataException("Unit is a required property for Entity and cannot be null");
            }
            else
            {
                this.Unit = Unit;
            }
            // to ensure "Start" is required (not null)
            if (Start == null)
            {
                throw new InvalidDataException("Start is a required property for Entity and cannot be null");
            }
            else
            {
                this.Start = Start;
            }
        }
        
        /// <summary>
        /// Array of values.
        /// </summary>
        /// <value>Array of values.</value>
        [DataMember(Name="Values", EmitDefaultValue=false)]
        public List<double?> Values { get; set; }
        /// <summary>
        /// Gets or Sets Unit
        /// </summary>
        [DataMember(Name="Unit", EmitDefaultValue=false)]
        public MyEnum Unit { get; set; }
        /// <summary>
        /// Start.
        /// </summary>
        /// <value>Start.</value>
        [DataMember(Name="Start", EmitDefaultValue=false)]
        public double? Start { get; set; }
}

Problems:

1. MyEnum set as nonoptional. Everithing is good with property declaration but in constructor default value is null (not compiled).
   In C# there is operator default. Generator can work in following way:
   public Entity(List<double?> Values = default(List<double?>), MyEnum Unit = default(MyEnum), double? Start = default(double?))
   It will be compiled.
   a) P.S.: If enum not set as required (should be optional) code generator do not generate "?" near property declaration. Enum property has non nullable type even if property should be optional. But constructor validation for NULL are not generated (it is correct).

2. Start is required but in property declaration it has double? type (optional). Constructor use validation and exception, but it will be better to:
   a) declare Start as double and constructor parameter too;
   b) add RequiredAttribute to property declaration

3. Values:
   a) should follow 2.b mentioned.
   b) Validation in constructor should be because there is no possibilities to make List<> not null
   c) How configure List items oplionality (no ways I see)?

4. Overall with following 2.a: If property is require but declared as optional and validation added only in constructor there will be possibility to set it NULL using property setter. Not good 👎 If fix 2.a. this problem will disappear.

[jimschubert]

I'll comment on each of your points. They're somewhat related, but it'd probably be clearer if I maintain the same structure.

1. Addressed in PR [csharp] Use default rather than null in ctor #4145

2. a) I have an explanation for the behavior of Nullables on required fields here. Perhaps this requires official documentation on the wiki?

   b) A RequiredAttribute on the property declaration would be redundant with the IValidatable implementation. See this sample. The IValidatable method is easier to implement than lots of attributes, which is probably just a limitation of using Mustache for templating.

3. a) I don't disagree that List<double?> should be List<double> in this case. I wonder if there are use cases I'm unaware of here.

   b) I don't understand your comment here. List<T> can be null here, and the validation is correct. The required property in swagger spec Determines whether this parameter is mandatory. There's currently nothing defined in the swagger specification that defines a requirement of non-empty array parameters for mandatory fields. There's a allowEmptyValue but this is defined as:

   Sets the ability to pass empty-valued parameters. This is valid only for either query or formData parameters and allows you to send a parameter with a name only or an empty value. Default value is false.

   This would be an excellent feature request against https://github.com/OAI/OpenAPI-Specification

   c) I believe your question here is related to my comment in 3.b just above?

4. Your model should be generated with a Validate method. I'm not sure why a client setting something to null would be a problem, when this validation method should be invoked by your API automatically (at least if you're using ASP.NET MVC or Web API). The end result is the same, but preventing a client from temporarily nulling out a property is prescribing coding techniques that may not be followed by everyone. As the code is currently, we're defining the contract of a required value for all the required properties for object construction. We could perform validation in the ApiClient prior to transmission, but we've had exceptions in the ApiClient cause confusion (when validation occurs client side prior to transmission versus server side after transmission). The validations in the constructor are more about de-serialization validations than client usage validations.

   I wouldn't be opposed to changing the setters on required properties to private (to merge de-serialization and client usage)... but then it wouldn't make much sense when one could mutate a Collection-based property but not change a primitive property. Then, if a property is a reference type, we'd have no control over the object's mutation. A limitation like this would really need to be all or nothing.

[aleksei-saharov]

Looks good:)
I understood your point.

1. OK. I hope I will work good
   a) Please, see again. You missed it.

2. a) OK, I will try default, but we have default 'default value' for double. It is 0. May be it will be good to follow: property is required => parameter is non-optional and looks as double Start = default(double)?
   b) OK. I will see.

3. a) I need List<double> for use cases
   b) I tried to tell that I like constructor validation for list. Nothing wrong
   c) I saw allowEmptyValue it will be good if we can use it as:

properties:
    MyList:
        type: array
        allowEmptyValue: false
        items:
          type: integer

And it will mean List<int>

4. Hmmm, OK. Give client more possibilities to break our server and try to increase server validation level. Sounds intresting:) As I understood client validation can be wider than servervalidation and it is correct.

Overall: I still intresting in 1.a, 2.a, 3.c (follow 3.a).

[jimschubert]

@aleksei-saharov

Sorry for the late follow up. I've been sick.

1.a: I think the entirety of your first question is addressed in that #4145 PR. Can you build that branch and verify? If enums are broken for required true/false, that's a bug that would need to be fixed.

2.a: There's a slight disconnect between my original answer here and your follow up question.

I know you touched on this, but I wanted to be more clear for anyone else following along. To put my answer a different way, the required attribute of a swagger definition doesn't mean a value is required it means a property is required. It's a structural constraint. See Schema Object in the specification and JSON Schema Validation:

An object instance is valid against this keyword if its property set contains all elements in this keyword's array value.

You're equating this with the RequiredAttribute from C#, which defines a value as required. It's true that a decimal type has a known default value, but this isn't necessarily the case in all languages and the swagger specification is generic enough for use across languages. If the C# generator interpreted the required field of a swagger definition schema as "non-optional", it would be broken according to the spec.

If you truly want to provide a default value for your schema object's properties, specify the default attribute. For example default=0. This would give you a double? value = 0 for non-required double properties with defaulted values or double value = 0 for required properties.

3.c: Unfortunately, my suggestion of allowEmptyValue was that it might be a good feature request for OpenAPI. I don't know how versioning is handled on the specification, so I don't know if it's something that would make it into OpenAPI 2.0. There are a couple issues opened there (OAI/OpenAPI-Specification#611 and OAI/OpenAPI-Specification#229). Going off the second issue from OpenAPI, maybe we could look into supporting x-nullable as a vendor extension? I don't know what the proper approach is for requesting or implementing vendor extensions in generators. Maybe @wing328 could provide a suggestion here?

3.a: When I said I wonder if there are use cases I'm unaware of for List<double?>, I meant more along the lines of [ null ] as requested in OAI/OpenAPI-Specification#229.

Also, I understand it's a slight performance hit, but why not cast the List<double?>?

$ csharp
Mono C# Shell, type "help;" for help

Enter statements below.
csharp> var numbers = new List<double?>()
csharp> numbers.Add(1)
csharp> numbers.Add(2)
csharp> numbers.ToString()
"System.Collections.Generic.List`1[System.Nullable`1[System.Double]]"
csharp> numbers.Cast<double>().ToList()
{ 1, 2 }
csharp> numbers.Cast<double>().ToList().ToString()
"System.Collections.Generic.List`1[System.Double]"

Here you can quickly convert the list to your expected List<double> using Cast<T>.

Your response to number four above concerns me. You should always validate when you cross a physical boundary in a system for numerous reasons: data corruption, man-in-the-middle attacks, client-server logic mismatch, sanity, etc. For in-memory C# code, you actually have no control over visibility. Someone using your client can use reflection to get and set private members, so making the setters public and allowing the client to do what they need isn't opening your server up to problems. If you're concerned about someone DoS'ing your API, you should research best practices and configure IIS or nginx (or whatever you're using to host and/or proxy your API) to account for this security concern.

[aleksei-saharov]

Ok. I will try to see branch but it is important to have stable swaggerhub web-client.
For SwaggerHub:

1. default instead null still not work
2. We prefer to use allowEmptyValue if it will be supported for list items. Cast<T> is good but code generator will use our clients who do not know about not null list items. In class constructor now there is no validations of list items (I did not see even list items count validation). We interested in code-generator which will create code without the need to modify the client. NOTE: Of course I tell about api-library, test-library contains empty tests and customer should fill them by himself.

[aleksei-saharov]

@jimschubert go back to your link
Scroll up. You will see comments:

        /// <param name="Date">Date (required).</param>
...
        /// <param name="Password">Password (required).</param>

But if we back to Validate method. There is strange behavior:
Password is check for null here
But Date NO!
Date and Password check for null in constructor together, but why Password has special advantages in Validation method. Only because it check for maxLength?

Overall:
3. Validation method partially support check for null. I think it is bug!

[aleksei-saharov]

4. I check default=0. It is not working for properties and arrays:
   YAML:

Parameter:
   type: object
   required:
     - Value
     - ArrayValue
   properties:
     Value:
       type: integer
       minimum: 3
       maximum: 5
       default: 3
     ArrayValue:
       type: array
       minItems: 3
       maxItems: 5
       items:
         type: integer
         default: 3

C# constructor:

        public Parameter(int? Value = null, List<int?> ArrayValue = null)
        {
            // to ensure "Value" is required (not null)
            if (Value == null)
            {
                throw new InvalidDataException("Value is a required property for Parameter and cannot be null");
            }
            else
            {
                this.Value = Value;
            }
            // to ensure "ArrayValue" is required (not null)
            if (ArrayValue == null)
            {
                throw new InvalidDataException("ArrayValue is a required property for Parameter and cannot be null");
            }
            else
            {
                this.ArrayValue = ArrayValue;
            }
        }

Nothing about default value.