[Scala] Default case class Option types to None for non-required fields

[gmarz]

PR checklist

• Read the contribution guidelines.
• Ran the shell script under ./bin/ to update Petstore sample so that CIs can verify the change. (For instance, only need to run ./bin/{LANG}-petstore.sh and ./bin/security/{LANG}-petstore.sh if updating the {LANG} (e.g. php, ruby, python, etc) code generator or {LANG} client's mustache templates). Windows batch files can be found in .\bin\windows\.
• Filed the PR against the correct branch: 3.0.0 branch for changes related to OpenAPI spec 3.0. Default: master.
• Copied the technical committee to review the pull request if your PR is targeting a particular programming langauge.

Description of the PR

Similar to #6538, this defaults Option types to None for non-required fields in the model case classes.

/cc @clasnake @jimschubert @ramzimaalej @wing328

[jimschubert]

It looks like this wasn't added by this PR, but I haven't seen it elsewhere yet.

swagger-async-httpclient hasn't been updated in 3 years. Is there any reason we're not using an async http client that is current?

[gmarz]

Hmm, not sure where this came from. Just a side effect of running the petstore sample scripts.

[wing328]

The Petstore security test samples (Scala) were not updated before.

[jimschubert]

Sorry, I should have explicitly mentioned that my second question was a general aside. It makes sense that the script added it, and the sample just hadn't been generated in a while.

[jimschubert]

So, there are three different states for properties in objects here: swagger spec's required = false means that objects don't require the property in the object. These should be defaulted to None. Then, there are the Scala semantics around avoiding nulls using Option[A]. It looks like this PR considers all nullable types as spec's optional? I think defaulting in the serializer config would account for all states.

How does this account for the state where a property is required, but nullable? Or would you consider this a non-issue?

I ask mainly because one of the sample types now has all constructor parameters with defaulted None args, which I don't think is valid in Swagger 2.0 specs.

[wing328]

@gmarz did you have a chance to review the questions by @jimschubert ?

[gmarz]

How does this account for the state where a property is required, but nullable?

Can you give an example of such a case? I think this is a non-issue wrt to the Scala client. If a property is required, then it shouldn't be of type Option, IMO.

[jimschubert]

@gmarz "required" only pertains to the structure of a transport object, not the validation of data in that object.

See http://json-schema.org/latest/json-schema-validation.html#rfc.section.6.17.

The value of this keyword MUST be an array. Elements of this array, if any, MUST be strings, and MUST be unique.

An object instance is valid against this keyword if every item in the array is the name of a property in the instance.

Omitting this keyword has the same behavior as an empty array.

This means when Swagger Specification defines required properties, it is saying that a document passed between systems must have the property.

For instance,

{
    "title": "Written Article",
    "type": "object",
    "properties": {
        "id": {
            "title": "Article Identifier",
            "type": "number",
            "readOnly": true
        },
        "title": {
            "title": "Article Title",
            "type": "string"
        },
        "authorId": {
            "type": "integer"
        },
        "imgData": {
            "title": "Article Illustration (thumbnail)",
            "type": "string",
            "media": {
                "binaryEncoding": "base64",
                "type": "image/png"
            }
        }
    },
    "required" : ["id", "title", "authorId"]
}

Valid JSON objects, according to spec:

{ "id": 1, "title": "Some title", "authorId": 2 }
{ "id": 1, "title": null, "authorId": 2 }

An invalid JSON object:

{ "id": 1, "authorId": 2 }

Here, title is a required property per the spec, but it is a nullable reference type (a String). In Scala, we avoid nulls, so this would be Option[String] as there are unfortunately no semantics in Swagger 2.0 Specifications to require non-null data within a property like this (there's min/max length, but those only pertain to string content and not nullability of a property's data).

I hope that clarifies.

I've considered adding support for an x-nullable vendor extension so we can support this somewhat in Swagger 2.0 documents. Open API Specification 3.0 addresses this with the nullable schema object property (see https://github.com/OAI/OpenAPI-Specification/blob/3.0.0/versions/3.0.0.md#schemaNullable).

[gmarz]

@jimschubert yes, thank you. That makes perfect sense.

Taking a step back though, this PR actually only changes the behavior to defaulting to None for non-required fields ({{^required}}] = None{{/required}}) and doesn't change how required, but nullable properties should be handled.

I ask mainly because one of the sample types now has all constructor parameters with defaulted None args, which I don't think is valid in Swagger 2.0 specs.

Can you point out where in the spec it says this is invalid? Shouldn't an empty body {} be allowed?

[jimschubert]

I looked and can't seem to find what I remembered seeing. There was a passage in JSON Schema spec which made it sound like a body required at least one key/value, but you're right { } is a valid body.

I did notice that we don't seem to account for allowEmptyValue = false, which is a similar concern.

I'll revisit when I'm not on mobile and see if I can find the place in the spec that I'm thinking of.

Again, you're right that the default to None here seems to be focused and outside of the concerns I stated above. I may need to open a new issue if I find the spec area that concerned me.

Thanks for following up on my comment!

[jimschubert]

Sorry I finally got around to pulling the code and checking it out. It looks good to me!

[wing328]

The Travis CI job actually passed: https://travis-ci.org/swagger-api/swagger-codegen/builds/300009630?utm_source=github_status&utm_medium=notification

Merging this into master.

Thanks @jimschubert for reviewing.