[codegen][Python-experimental] Discriminator NPE fix, handle 'null' type, #4906 enhancements (#5809)

* Adds addComposedMappedModels and testComposedSchemaOneOfDiscriminatorMap

* Requires that discriminators be required properties

* Strengthens discriminaotr validation, adds better error messages, adds schema oneof samples

* Adds oneOf and anyOf invalidDiscriminator tests

* Runs ensure up to date

* Updates incorrect addOneOfInterfaceModel invocation

* Runs ensure-up-to-date

* Fix NPE when at least one extension is defined but not x-discriminator-value

* Adds addComposedMappedModels and testComposedSchemaOneOfDiscriminatorMap

* Requires that discriminators be required properties

* Strengthens discriminaotr validation, adds better error messages, adds schema oneof samples

* Adds oneOf and anyOf invalidDiscriminator tests

* Updates incorrect addOneOfInterfaceModel invocation

* Runs ensure-up-to-date

* Adds updates from Sebastien Rosset

* Removes newlines

* Add documentation and new getValidDiscriminatorMappings function

* Add documentation and new getValidDiscriminatorMappings function

* Add documentation and new getValidDiscriminatorMappings function

* Add documentation and new getValidDiscriminatorMappings function

* Add documentation and new getValidDiscriminatorMappings function

* Add documentation and new getValidDiscriminatorMappings function

* throw exception if discriminator mappingName argument is null

* handle scenario when composed schema has 'null' type

* remove extraneous characters in comments

* Uses df.isString

* Traverse discriminators to resolve discriminator mapping

* Fixes tests be correctly setting df.isString

* Remove unused method

* Updates discriminatorExplicitMappingVerbose description per PR feedback

* Adds description of how mappedModels is populated

* Adds the suggestion exception raising when a MappedModel mappingName is null

* Actually resolves merge conflicts

* Adds addComposedMappedModels and testComposedSchemaOneOfDiscriminatorMap

* Requires that discriminators be required properties

* Strengthens discriminaotr validation, adds better error messages, adds schema oneof samples

* Adds oneOf and anyOf invalidDiscriminator tests

* Updates incorrect addOneOfInterfaceModel invocation

* Runs ensure-up-to-date

* Adds updates from Sebastien Rosset

* Removes newlines

* Uses df.isString

* Fixes tests be correctly setting df.isString

* Updates discriminatorExplicitMappingVerbose description per PR feedback

* Adds description of how mappedModels is populated

* Adds the suggestion exception raising when a MappedModel mappingName is null

* Actually resolves merge conflicts

* Switches two methods to package private because they are needed for testing

* Allow nulls in MappedModel.getMappingName

* Remove exception when mappingName is null value

* Remove exception when mappingName is null value

* resolve merge conflicts

* resolve merge conflicts

* Execute scripts in the bin directory

* Fix CI issues and address PR review comments: better documentation and fix white space issues.

* Fix CI issues and address PR review comments: better documentation and fix white space issues.

* run sample scripts

* resolve merge conflicts

* fix end-of-line issue

* resolve merge conflicts

* resolve merge issues

* Handle case when discriminator is not specified in input data

* minor changes and add code comments

* Refactor get_discriminator code

* Add unit test with missing discriminator property

* improve get_discriminator function

* Run sample scripts

* add unit tests for recursive get_discriminator_class

* fix unit test issues

* fix formatting issues

* fix formatting issues

* fix formatting issues

* fix index out of range exception

* fix formatting issues

* fix formatting issues

* fix formatting issues. Finally figured out how to check formatting in local workspace

Co-authored-by: Justin Black <[email protected]>
Co-authored-by: William Cheng <[email protected]>

Author: 3 people

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

@@ -22,6 +22,9 @@
 
 import java.util.*;
 
+/**
+ * CodegenModel represents a schema object in a OpenAPI document.
+ */
 @JsonIgnoreProperties({"parentModel", "interfaceModels"})
 public class CodegenModel implements IJsonSchemaValidationProperties {
     // The parent model name from the schemas. The parent is determined by inspecting the allOf, anyOf and
@@ -220,6 +223,45 @@ public void setDescription(String description) {
         this.description = description;
     }
 
+    /**
+     * Returns the discriminator for this schema object, or null if no discriminator has been specified.
+     * 
+     * The list of all possible schema discriminator mapping values is obtained
+     * from explicit discriminator mapping values in the OpenAPI document, and from
+     * inherited discriminators through oneOf, allOf, anyOf.
+     * For example, a discriminator may be defined in a 'Pet' schema as shown below.
+     * The Dog and Cat schemas inherit the discriminator through the allOf reference.
+     * In the 'Pet' schema, the supported discriminator mapping values for the
+     * 'objectType' properties are 'Dog' and 'Cat'.
+     * The allowed discriminator mapping value for the Dog schema is 'Dog'.
+     * The allowed discriminator mapping value for the Cat schema is 'Dog'.
+     * 
+     * Pet:
+     *   type: object
+     *   discriminator:
+     *     propertyName: objectType
+     *   required:
+     *     - objectType
+     *   properties:
+     *     objectType:
+     *     type: string
+     * Dog:
+     *   allOf:
+     *   - $ref: '#/components/schemas/Pet'
+     *   - type: object
+     *     properties:
+     *       p1:
+     *         type: string
+     * Cat:
+     *   allOf:
+     *   - $ref: '#/components/schemas/Pet'
+     *   - type: object
+     *     properties:
+     *       p2:
+     *         type: string
+     * 
+     * @return the discriminator.
+     */
     public CodegenDiscriminator getDiscriminator() {
         return discriminator;
     }
@@ -228,6 +270,14 @@ public void setDiscriminator(CodegenDiscriminator discriminator) {
         this.discriminator = discriminator;
     }
 
+    /**
+     * Returns the name of the discriminator property for this schema in the OpenAPI document.
+     * In the OpenAPI document, the discriminator may be specified in the local schema or
+     * it may be inherited, such as through a 'allOf' schema which references another schema
+     * that has a discriminator, recursively.
+     * 
+     * @return the name of the discriminator property.
+     */
     public String getDiscriminatorName() {
         return discriminator == null ? null : discriminator.getPropertyName();
     }

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

@@ -2553,36 +2553,60 @@ private Discriminator recursiveGetDiscriminator(Schema sc, OpenAPI openAPI) {
             if (composedSchema.getOneOf() != null && composedSchema.getOneOf().size() != 0) {
                 // All oneOf definitions must contain the discriminator
                 Integer hasDiscriminatorCnt = 0;
+                Integer hasNullTypeCnt = 0;
                 Set<String> discriminatorsPropNames = new HashSet<>();
-                for (Schema oneOf : composedSchema.getOneOf()) {
+                for  (Schema oneOf: composedSchema.getOneOf()) {
+                    if (ModelUtils.isNullType(oneOf)) {
+                        // The null type does not have a discriminator. Skip.
+                        hasNullTypeCnt++;
+                        continue;
+                    }
                     foundDisc = recursiveGetDiscriminator(oneOf, openAPI);
                     if (foundDisc != null) {
                         discriminatorsPropNames.add(foundDisc.getPropertyName());
                         hasDiscriminatorCnt++;
                     }
                 }
-                if (hasDiscriminatorCnt == composedSchema.getOneOf().size() && discriminatorsPropNames.size() == 1) {
+                if (discriminatorsPropNames.size() > 1) {
+                    throw new RuntimeException("The oneOf schemas have conflicting discriminator property names. " +
+                        "oneOf schemas must have the same property name, but found " + String.join(", ", discriminatorsPropNames));
+                }
+                if ((hasDiscriminatorCnt + hasNullTypeCnt) == composedSchema.getOneOf().size() && discriminatorsPropNames.size() == 1) {
                     disc.setPropertyName(foundDisc.getPropertyName());
                     disc.setMapping(foundDisc.getMapping());
                     return disc;
                 }
+                // If the scenario when oneOf has two children and one of them is the 'null' type,
+                // there is no need for a discriminator.
             }
             if (composedSchema.getAnyOf() != null && composedSchema.getAnyOf().size() != 0) {
                 // All anyOf definitions must contain the discriminator because a min of one must be selected
                 Integer hasDiscriminatorCnt = 0;
+                Integer hasNullTypeCnt = 0;
                 Set<String> discriminatorsPropNames = new HashSet<>();
-                for (Schema anyOf : composedSchema.getAnyOf()) {
+                for  (Schema anyOf: composedSchema.getAnyOf()) {
+                    if (ModelUtils.isNullType(anyOf)) {
+                        // The null type does not have a discriminator. Skip.
+                        hasNullTypeCnt++;
+                        continue;
+                    }
                     foundDisc = recursiveGetDiscriminator(anyOf, openAPI);
                     if (foundDisc != null) {
                         discriminatorsPropNames.add(foundDisc.getPropertyName());
                         hasDiscriminatorCnt++;
                     }
                 }
-                if (hasDiscriminatorCnt == composedSchema.getAnyOf().size() && discriminatorsPropNames.size() == 1) {
+                if (discriminatorsPropNames.size() > 1) {
+                    throw new RuntimeException("The anyOf schemas have conflicting discriminator property names. " +
+                        "anyOf schemas must have the same property name, but found " + String.join(", ", discriminatorsPropNames));
+                }
+                if ((hasDiscriminatorCnt + hasNullTypeCnt) == composedSchema.getAnyOf().size() && discriminatorsPropNames.size() == 1) {
                     disc.setPropertyName(foundDisc.getPropertyName());
                     disc.setMapping(foundDisc.getMapping());
                     return disc;
                 }
+                // If the scenario when anyOf has two children and one of them is the 'null' type,
+                // there is no need for a discriminator.
             }
         }
         return null;
@@ -2611,7 +2635,10 @@ protected List<MappedModel> getOneOfAnyOfDescendants(String composedSchemaName,
             if (schemaList == null) {
                 continue;
             }
-            for (Schema sc : schemaList) {
+            for  (Schema sc: schemaList) {
+                if (ModelUtils.isNullType(sc)) {
+                    continue;
+                }
                 String ref = sc.get$ref();
                 if (ref == null) {
                     // for schemas with no ref, it is not possible to build the discriminator map

modules/openapi-generator/src/main/java/org/openapitools/codegen/validations/oas/OpenApiSchemaValidations.java

@@ -61,7 +61,7 @@ class OpenApiSchemaValidations extends GenericValidator<SchemaWrapper> {
      * <p>
      * Where the only examples of oneOf in OpenAPI Specification are used to define either/or type structures rather than validations.
      * Because of this ambiguity in the spec about what is non-standard about oneOf support, we'll warn as a recommendation that
-ne      * properties on the schema defining oneOf relationships may not be intentional in the OpenAPI Specification.
+     * properties on the schema defining oneOf relationships may not be intentional in the OpenAPI Specification.
      *
      * @param schemaWrapper An input schema, regardless of the type of schema
      * @return {@link ValidationRule.Pass} if the check succeeds, otherwise {@link ValidationRule.Fail}

modules/openapi-generator/src/main/resources/python/python-experimental/model_templates/method_discriminator.mustache

@@ -51,6 +51,4 @@ class {{unescapedDescription}}(ModelComposed):
               {{{.}}},
 {{/oneOf}}
           ],
-        }{{#discriminator}}
-
-{{> python-experimental/model_templates/method_discriminator }}{{/discriminator}}
+        }

modules/openapi-generator/src/main/resources/python/python-experimental/model_templates/model_composed.mustache

@@ -26,6 +26,4 @@ class {{unescapedDescription}}(ModelNormal):
 
     _composed_schemas = {}
 
-{{> python-experimental/model_templates/method_init_normal}}{{#discriminator}}
-
-{{> python-experimental/model_templates/method_discriminator }}{{/discriminator}}
+{{> python-experimental/model_templates/method_init_normal}}

modules/openapi-generator/src/main/resources/python/python-experimental/model_templates/model_normal.mustache

@@ -66,31 +66,78 @@ class OpenApiModel(object):
         # pick a new schema/class to instantiate because a discriminator
         # propertyName value was passed in
 
+        # Build a list containing all oneOf and anyOf descendants.
+        oneof_anyof_classes = None
+        if cls._composed_schemas is not None:
+            oneof_anyof_classes = (
+                cls._composed_schemas.get('oneOf', ()) +
+                cls._composed_schemas.get('anyOf', ()))
+        if (oneof_anyof_classes and none_type in oneof_anyof_classes and
+                len(args) == 1 and args[0] is None):
+            # The input data is the 'null' value AND one of the oneOf/anyOf children
+            # is the 'null' type (which is introduced in OAS schema >= 3.1).
+            return None
+
         visited_composed_classes = kwargs.get('_visited_composed_classes', ())
         if (
             cls.discriminator is None or
             cls in visited_composed_classes
         ):
-            # we don't have a discriminator
-            # or we have already visited this class before and are sure that we
-            # want to instantiate it this time
+            # This openapi schema (cls) does not have a discriminator
+            # Or we have already visited this class before and are sure that we
+            # want to instantiate it this time.
+            #
+            # If we are making an instance of a composed schema Descendent
+            # which allOf includes Ancestor, then Ancestor contains
+            # a discriminator that includes Descendent.
+            # So if we make an instance of Descendent, we have to make an
+            # instance of Ancestor to hold the allOf properties.
+            # This code detects that use case and makes the instance of Ancestor
+            # For example:
+            # When making an instance of Dog, _visited_composed_classes = (Dog,)
+            # then we make an instance of Animal to include in dog._composed_instances
+            # so when we are here, cls is Animal
+            # cls.discriminator != None
+            # cls not in _visited_composed_classes
+            # new_cls = Dog
+            # but we know we know that we already have Dog
+            # because it is in visited_composed_classes
+            # so make Animal here
             return super(OpenApiModel, cls).__new__(cls)
 
-        oneof_anyof_classes = []
-        oneof_anyof_classes.extend(cls._composed_schemas.get('oneOf', ()))
-        oneof_anyof_classes.extend(cls._composed_schemas.get('anyOf', ()))
-        new_cls = cls.get_discriminator_class(kwargs)
+        # Get the name and value of the discriminator property.
+        # The discriminator name is obtained from the discriminator meta-data
+        # and the discriminator value is obtained from the input data.
+        discr_propertyname_py = list(cls.discriminator.keys())[0]
+        discr_propertyname_js = cls.attribute_map[discr_propertyname_py]
+        if discr_propertyname_js in kwargs:
+            discr_value = kwargs[discr_propertyname_js]
+        elif discr_propertyname_py in kwargs:
+            discr_value = kwargs[discr_propertyname_py]
+        else:
+            # The input data does not contain the discriminator property.
+            path_to_item = kwargs.get('_path_to_item', ())
+            raise ApiValueError(
+                "Cannot deserialize input data due to missing discriminator. "
+                "The discriminator property '%s' is missing at path: %s" %
+                (discr_propertyname_js, path_to_item)
+            )
+
+        # Implementation note: the last argument to get_discriminator_class
+        # is a list of visited classes. get_discriminator_class may recursively
+        # call itself and update the list of visited classes, and the initial
+        # value must be an empty list. Hence not using 'visited_composed_classes'
+        new_cls = get_discriminator_class(
+                    cls, discr_propertyname_py, discr_value, [])
         if new_cls is None:
-            disc_prop_name_py = list(cls.discriminator.keys())[0]
-            disc_prop_name_js = cls.attribute_map[disc_prop_name_py]
             path_to_item = kwargs.get('_path_to_item', ())
             disc_prop_value = kwargs.get(
-                disc_prop_name_js, kwargs.get(disc_prop_name_py))
+                discr_propertyname_js, kwargs.get(discr_propertyname_py))
             raise ApiValueError(
                 "Cannot deserialize input data due to invalid discriminator "
                 "value. The OpenAPI document has no mapping for discriminator "
                 "property '%s'='%s' at path: %s" %
-                (disc_prop_name_js, disc_prop_value, path_to_item)
+                (discr_propertyname_js, disc_prop_value, path_to_item)
             )
 
         if new_cls in visited_composed_classes:
@@ -101,7 +148,7 @@ class OpenApiModel(object):
         kwargs['_visited_composed_classes'] = visited_composed_classes + (cls,)
 
         if cls._composed_schemas.get('allOf') and oneof_anyof_child:
-            # validate that we can make self because when we make the
+            # Validate that we can make self because when we make the
             # new_cls it will not include the allOf validations in self
             self_inst = super(OpenApiModel, cls).__new__(cls)
             self_inst.__init__(*args, **kwargs)
@@ -131,7 +178,29 @@ class ModelNormal(OpenApiModel):
 
 class ModelComposed(OpenApiModel):
     """the parent class of models whose type == object in their
-    swagger/openapi and have oneOf/allOf/anyOf"""
+    swagger/openapi and have oneOf/allOf/anyOf
+
+    When one sets a property we use var_name_to_model_instances to store the value in
+    the correct class instances + run any type checking + validation code.
+    When one gets a property we use var_name_to_model_instances to get the value
+    from the correct class instances.
+    This allows multiple composed schemas to contain the same property with additive
+    constraints on the value.
+
+    _composed_schemas (dict) stores the anyOf/allOf/oneOf classes
+    key (str): allOf/oneOf/anyOf
+    value (list): the classes in the XOf definition.
+        Note: none_type can be included when the openapi document version >= 3.1.0
+    _composed_instances (list): stores a list of instances of the composed schemas
+    defined in _composed_schemas. When properties are accessed in the self instance,
+    they are returned from the self._data_store or the data stores in the instances
+    in self._composed_schemas
+    _var_name_to_model_instances (dict): maps between a variable name on self and
+    the composed instances (self included) which contain that data
+    key (str): property name
+    value (list): list of class instances, self or instances in _composed_instances
+    which contain the value that the key is referring to.
+    """
 
 {{> python-experimental/model_templates/methods_setattr_getattr_composed }}
 
@@ -624,6 +693,56 @@ def deserialize_primitive(data, klass, path_to_item):
         )
 
 
+def get_discriminator_class(model_class,
+                            discr_name,
+                            discr_value, cls_visited):
+    """Returns the child class specified by the discriminator.
+
+    Args:
+        model_class (OpenApiModel): the model class.
+        discr_name (string): the name of the discriminator property.
+        discr_value (any): the discriminator value.
+        cls_visited (list): list of model classes that have been visited.
+            Used to determine the discriminator class without
+            visiting circular references indefinitely.
+
+    Returns:
+        used_model_class (class/None): the chosen child class that will be used
+            to deserialize the data, for example dog.Dog.
+            If a class is not found, None is returned.
+    """
+
+    if model_class in cls_visited:
+        # The class has already been visited and no suitable class was found.
+        return None
+    cls_visited.append(model_class)
+    used_model_class = None
+    if discr_name in model_class.discriminator:
+        class_name_to_discr_class = model_class.discriminator[discr_name]
+        used_model_class = class_name_to_discr_class.get(discr_value)
+    if used_model_class is None:
+        # We didn't find a discriminated class in class_name_to_discr_class.
+        # The discriminator mapping may exist in a descendant (anyOf, oneOf)
+        # or ancestor (allOf).
+        # Ancestor example: in the "Dog -> Mammal -> Chordate -> Animal"
+        #   hierarchy, the discriminator mappings may be defined at any level
+        #   in the hieararchy.
+        # Descendant example: a schema is oneOf[Plant, Mammal], and each
+        #   oneOf child may itself be an allOf with some arbitrary hierarchy,
+        #   and a graph traversal is required to find the discriminator.
+        composed_children = model_class._composed_schemas.get('oneOf', ()) + \
+            model_class._composed_schemas.get('anyOf', ()) + \
+            model_class._composed_schemas.get('allOf', ())
+        for cls in composed_children:
+            # Check if the schema has inherited discriminators.
+            if cls.discriminator is not None:
+                used_model_class = get_discriminator_class(
+                                    cls, discr_name, discr_value, cls_visited)
+                if used_model_class is not None:
+                    return used_model_class
+    return used_model_class
+
+
 def deserialize_model(model_data, model_class, path_to_item, check_type,
                       configuration, from_server):
     """Deserializes model_data to model instance.