[codegen][validation] Add support for 'null' type

[sebastien-rosset]

Add support for the 'null' type, and retain existing behavior for OAS 3.0.x documents that don't use the 'null' type.

In OAS 3.1, the 'null' type is supported, and some existing 3.0.x documents use the 'null' type, even though it's not actually supported by the OAS 3.0 spec.

As an example, a schema may be defined as:

type: 'null'

This is useful in the following scenario when a property may be null or some referenced complex type:

Order:
  type: object
  properties:
    billTo:
      oneOf:
      - type: 'null'
      - $ref: "#/definitions/Address"

In 3.0.x, the above snippet could be written differently by using "nullable: true", but the nullable attribute in being deprecated in OAS 3.1. See #5180 for issues related to the 'nullable' attribute.

When a OAS document does not explicitly specify a 'null' value, the generated code should not accept a null value. In practice, some language generators such as go-experimental are lenient and allow null values. Other generators such as the python-experimental module perform a strict validation of the payload (e.g. server response). If the response is null but the OAS schema does not allow null values, the client raises an exception such as the one below:

Exception when calling API: Invalid type for variable 'parent'.
Required value type is MoOptionalBaseMo and passed type was NoneType at
['received_data']['results'][0]['parent']

PR checklist

• Read the contribution guidelines.
• If contributing template-only or documentation-only changes which will change sample output, build the project before.
• Run the shell script(s) under ./bin/ (or Windows batch scripts under.\bin\windows) to update Petstore samples related to your fix. This is important, as CI jobs will verify all generator outputs of your HEAD commit, and these must match the expectations made by your contribution. You only need to run ./bin/{LANG}-petstore.sh, ./bin/openapi3/{LANG}-petstore.sh if updating the code or mustache templates for a language ({LANG}) (e.g. php, ruby, python, etc).
• File the PR against the correct branch: master, 4.3.x, 5.0.x. Default: master.
• Copy the technical committee to review the pull request if your PR is targeting a particular programming language.

[sebastien-rosset]

Note to reviewer: this was discussed with @wing328:

If I remember correctly, that refers to InlineModelResolver (https://github.com/OpenAPITools/openapi-generator/blob/master/modules/openapi-generator/src/main/java/org/openapitools/codegen/InlineModelResolver.java) not supporting allOf so the warning notifies the users to use $ref instead of defining the schema inline in allOf . We should have improved the InlineModelResolver to better handle inline schema allOf so I think it's ok to remove the warning.

[jimschubert]

I'm not aware of the discussion, but I don't know if it's related to InlineModelResolver. This method is only called from here:

openapi-generator/modules/openapi-generator/src/main/java/org/openapitools/codegen/DefaultCodegen.java

Line 1725 in 0693a83

return toAllOfName(names, cs);

getSchemaType will call ModelUtils.getInterfaces on a composed schema, then iterate all the schemas and get the first defined schema as the "type" of the composed schema. I don't think that behavior is correct because it assumes or suggests that composed schemas have some concept of "interfaces".

InlineModelResolver has other issues, in that it tries to do a best-guess of models defined inline and resolve them to Schemas defined at the top of the document. This goes against requirements in the spec which claim that inline models should not be considered for composed schemas. At a certain point, we have no information whether our model was initially inline.

[sebastien-rosset]

There is also a non-obvious point: the 'allOf' name is constructed from the first element in the "allOf" list. If two separate authors want to model class inheritance, one OAS author may think it's better to put the "parent" class first, and another author may think it's better to put the "parent" last.

[jimschubert]

Just a heads up that I don't think this is intended to always indicate polymorphism in the specification. An allOf ref with discriminator is always supposed to indicate polymporphism, but the opposite isn't clear. The case that you've defined here, and that I'm assuming was intended to match the refedWithoutDiscriminator.size() == 1 condition below is a "legacy" implementation from when we'd began implementing allOf support but it wasn't fully supported by the generator.

Details:

• discriminator document starts with "Adds support for polymorphism."
• Composition and Inheritance (Polymorphism) states: "To support polymorphism, the OpenAPI Specification adds the discriminator field. When used, the discriminator will be the name of the property that decides which schema definition validates the structure of the model."

It doesn't explain this well enough, but from these two it seems like they're defining subtype polymorphism since other types of polymorphism are either irrelevant or don't make much sense in this usage.

More discussion (we could turn it into an issue as well):

There are additional details of OpenAPI Spec which aren't explained very well with allOf. For example, the discriminator for allOf can be in the $ref schema and not in the schema containing the allOf definition. In this case, the discriminator will use the schema name of the schema with the allOf and $ref. Example:

Pet:
  …
  discriminator:
    propertyName: type
  properties:
    name: type
    type: String
Cat:
  allOf:
    - $ref: #/components/schemas/Pet

Even without the discriminator, I don't think the LOGGER.warn message is totally accurate. For example, the json-schema combining doc shows this example:

{
  "definitions": {
    "address": {
      "type": "object",
      "properties": {
        "street_address": { "type": "string" },
        "city":           { "type": "string" },
        "state":          { "type": "string" }
      },
      "required": ["street_address", "city", "state"]
    }
  },

  "allOf": [
    { "$ref": "#/definitions/address" },
    { "properties": {
        "type": { "enum": [ "residential", "business" ] }
      }
    }
  ]
}

Unfortunately, this doesn't define whether a codegen should model:

Address:
  + StreetAddress: String
  + City: String
  + State: String

TypedAddress <Address>:
    Enum: [ Residential, Business ]

or:

Address:
  + StreetAddress: String
  + City: String
  + State: String
    Enum: [ Residential, Business ]

In this issue, there's no clarification: OAI/OpenAPI-Specification#1467

In fact, if you consider from a REST input modeling perspective, it might make sense that all allOf without discriminator are unique types with properties of the $ref.

I wonder if it may make sense for us to support a vendor extension as a hint whether the user wants to use composition or inheritance for these one-off allOf without a discriminator. Something like:

• x-codegen-prefer-polymorphism: true. When no discriminator is defined, we'll try to create a parent and child relationship if the target language/generator supports it
• x-codegen-prefer-polymorphism: false. When no discriminator is defined, we'll merge all properties into one "super" class.

The latter might be challenging to support the spec requirement that these are validated independently like in the case where two refs have the same property with different validation characteristics. This isn't a case we handle now.

[sebastien-rosset]

Thank you for the quick feedback. Let me digest this and get back to you tomorrow.

[sebastien-rosset]

Unfortunately, this doesn't define whether a codegen should model:

Address:
  + StreetAddress: String
  + City: String
  + State: String

TypedAddress <Address>:
    Enum: [ Residential, Business ]

or:

Address:
  + StreetAddress: String
  + City: String
  + State: String
    Enum: [ Residential, Business ]

Above did you mean? 1) the generated code; 2) the serialization format of the JSON document; or 3) both? If (1), I agree it's not defined in the OAS spec, but OAS does not mandate how code generators should be implemented. If (2), isn't https://tools.ietf.org/html/draft-handrews-json-schema-02 section 9.2.1.1 mandating that it must be the second case?

[sebastien-rosset]

• x-codegen-prefer-polymorphism: true. When no discriminator is defined, we'll try to create a parent and child relationship if the target language/generator supports it
• x-codegen-prefer-polymorphism: false. When no discriminator is defined, we'll merge all properties into one "super" class.

The latter might be challenging to support the spec requirement that these are validated independently like in the case where two refs have the same property with different validation characteristics. This isn't a case we handle now.

I see there are related discussions about defined "subclassOf", but it does not seem to make much progress.

I agree with all your points, they will have to be addressed in separate PRs. For example, I had an issue with inlined schema exactly as you mentioned, and I ended up doing a workaround by using a $ref.

[jimschubert]

Oh. I wasn't suggesting any additional changes here. I just wanted to comment on the allOf change because of the ambiguity, and hoped it'd generate discussion. There's a lot to be done for allOf support, and I'm hoping we'll make great progress in the 5.0 release.

Regarding your comment:

If (2), isn't https://tools.ietf.org/html/draft-handrews-json-schema-02 section 9.2.1.1 mandating that it must be the second case?

The issue is that JSON Schema focuses on validation only and not on structure. This is further complicated because OpenAPI Specification disassociates it's allOf from JSON Schema:

allOf - Inline or referenced schema MUST be of a Schema Object and not a standard JSON Schema.

I think the only sane way to support it cleanly is by having good defaults and vendor extensions to modify each model's behavior. We may also want to consider a configuration option at generation to define a "global" default.

[jimschubert]

This looks great. I have a small question…

I know that type: 'null' is the way to define something with the word null in YAML. Are there any concerns that the new check might fail if someone defines their JSON spec as { "type": "'null'" }, like if they misread or misunderstand the OAS 3.1 text?

[jimschubert]

LGTM

[sebastien-rosset]

This looks great. I have a small question…

I know that type: 'null' is the way to define something with the word null in YAML. Are there any concerns that the new check might fail if someone defines their JSON spec as { "type": "'null'" }, like if they misread or misunderstand the OAS 3.1 text?

I can add one more validation to check that values of the "type" attribute are one of 'null', 'boolean', 'object', 'array', 'number', 'string', or 'integer'

Update: I have added a validation rule for the 'type' attribute.