Make it more robust

OpenAPITools · Sep 20, 2024 · e478171 · e478171
1 parent 2d112da
commit e478171
Show file tree

Hide file tree

Showing 5 changed files with 164 additions and 64 deletions.
diff --git a/modules/openapi-generator/src/main/java/org/openapitools/codegen/DefaultCodegen.java b/modules/openapi-generator/src/main/java/org/openapitools/codegen/DefaultCodegen.java
@@ -3270,11 +3270,11 @@ private CodegenProperty discriminatorFound(String composedSchemaName, Schema sc,
      * @param sc             The Schema that may contain the discriminator
      * @param visitedSchemas An array list of visited schemas
      */
-    private DiscriminatorAndReferenceSchema recursiveGetDiscriminator(Schema sc, ArrayList<Schema> visitedSchemas) {
+    private Discriminator recursiveGetDiscriminator(Schema sc, ArrayList<Schema> visitedSchemas) {
         Schema refSchema = ModelUtils.getReferencedSchema(openAPI, sc);
-        Discriminator foundRefDisc = refSchema.getDiscriminator();
-        if (foundRefDisc != null) {
-            return new DiscriminatorAndReferenceSchema(foundRefDisc, refSchema);
+        Discriminator foundDisc = refSchema.getDiscriminator();
+        if (foundDisc != null) {
+            return foundDisc;
         }
 
         if (this.getLegacyDiscriminatorBehavior()) {
@@ -3288,15 +3288,17 @@ private DiscriminatorAndReferenceSchema recursiveGetDiscriminator(Schema sc, Arr
         }
         visitedSchemas.add(refSchema);
 
+        Discriminator disc = new Discriminator();
         if (ModelUtils.isComposedSchema(refSchema)) {
             Schema composedSchema = refSchema;
             if (composedSchema.getAllOf() != null) {
                 // If our discriminator is in one of the allOf schemas break when we find it
                 for (Object allOf : composedSchema.getAllOf()) {
-                    DiscriminatorAndReferenceSchema foundDisc =
-                        recursiveGetDiscriminator((Schema) allOf, visitedSchemas);
+                    foundDisc = recursiveGetDiscriminator((Schema) allOf, visitedSchemas);
                     if (foundDisc != null) {
-                       return foundDisc;
+                        disc.setPropertyName(foundDisc.getPropertyName());
+                        disc.setMapping(foundDisc.getMapping());
+                        return disc;
                     }
                 }
             }
@@ -3305,7 +3307,6 @@ private DiscriminatorAndReferenceSchema recursiveGetDiscriminator(Schema sc, Arr
                 Integer hasDiscriminatorCnt = 0;
                 Integer hasNullTypeCnt = 0;
                 Set<String> discriminatorsPropNames = new HashSet<>();
-                DiscriminatorAndReferenceSchema foundDisc = null;
                 for (Object oneOf : composedSchema.getOneOf()) {
                     if (ModelUtils.isNullType((Schema) oneOf)) {
                         // The null type does not have a discriminator. Skip.
@@ -3314,7 +3315,7 @@ private DiscriminatorAndReferenceSchema recursiveGetDiscriminator(Schema sc, Arr
                     }
                     foundDisc = recursiveGetDiscriminator((Schema) oneOf, visitedSchemas);
                     if (foundDisc != null) {
-                        discriminatorsPropNames.add(foundDisc.discriminator.getPropertyName());
+                        discriminatorsPropNames.add(foundDisc.getPropertyName());
                         hasDiscriminatorCnt++;
                     }
                 }
@@ -3323,7 +3324,9 @@ private DiscriminatorAndReferenceSchema recursiveGetDiscriminator(Schema sc, Arr
                             "oneOf schemas must have the same property name, but found " + String.join(", ", discriminatorsPropNames));
                 }
                 if (foundDisc != null && (hasDiscriminatorCnt + hasNullTypeCnt) == composedSchema.getOneOf().size() && discriminatorsPropNames.size() == 1) {
-                    return foundDisc;
+                    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.
@@ -3333,7 +3336,6 @@ private DiscriminatorAndReferenceSchema recursiveGetDiscriminator(Schema sc, Arr
                 Integer hasDiscriminatorCnt = 0;
                 Integer hasNullTypeCnt = 0;
                 Set<String> discriminatorsPropNames = new HashSet<>();
-                DiscriminatorAndReferenceSchema foundDisc = null;
                 for (Object anyOf : composedSchema.getAnyOf()) {
                     if (ModelUtils.isNullType((Schema) anyOf)) {
                         // The null type does not have a discriminator. Skip.
@@ -3342,7 +3344,7 @@ private DiscriminatorAndReferenceSchema recursiveGetDiscriminator(Schema sc, Arr
                     }
                     foundDisc = recursiveGetDiscriminator((Schema) anyOf, visitedSchemas);
                     if (foundDisc != null) {
-                        discriminatorsPropNames.add(foundDisc.discriminator.getPropertyName());
+                        discriminatorsPropNames.add(foundDisc.getPropertyName());
                         hasDiscriminatorCnt++;
                     }
                 }
@@ -3351,7 +3353,9 @@ private DiscriminatorAndReferenceSchema recursiveGetDiscriminator(Schema sc, Arr
                             "anyOf schemas must have the same property name, but found " + String.join(", ", discriminatorsPropNames));
                 }
                 if (foundDisc != null && (hasDiscriminatorCnt + hasNullTypeCnt) == composedSchema.getAnyOf().size() && discriminatorsPropNames.size() == 1) {
-                    return foundDisc;
+                    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.
@@ -3360,16 +3364,6 @@ private DiscriminatorAndReferenceSchema recursiveGetDiscriminator(Schema sc, Arr
         return null;
     }
 
-    private static class DiscriminatorAndReferenceSchema{
-        private final Discriminator discriminator;
-        private final Schema referenceSchema;
-
-      private DiscriminatorAndReferenceSchema(Discriminator discriminator, Schema referenceSchema) {
-        this.discriminator = discriminator;
-        this.referenceSchema = referenceSchema;
-      }
-    }
-
     /**
      * This function is only used for composed schemas which have a discriminator
      * Process oneOf and anyOf models in a composed schema and adds them into
@@ -3503,12 +3497,10 @@ protected List<MappedModel> getAllOfDescendants(String thisSchemaName) {
     }
 
     protected CodegenDiscriminator createDiscriminator(String schemaName, Schema schema) {
-        DiscriminatorAndReferenceSchema discriminatorAndReferenceSchema =
-            recursiveGetDiscriminator(schema, new ArrayList<Schema>());
-        if (discriminatorAndReferenceSchema == null) {
+        Discriminator sourceDiscriminator = recursiveGetDiscriminator(schema, new ArrayList<Schema>());
+        if (sourceDiscriminator == null) {
             return null;
         }
-        Discriminator sourceDiscriminator = discriminatorAndReferenceSchema.discriminator;
         CodegenDiscriminator discriminator = new CodegenDiscriminator();
         String discriminatorPropertyName = sourceDiscriminator.getPropertyName();
         discriminator.setPropertyName(toVarName(discriminatorPropertyName));
@@ -3531,21 +3523,6 @@ protected CodegenDiscriminator createDiscriminator(String schemaName, Schema sch
                         .orElseGet(() -> typeMapping.get("string"));
         discriminator.setPropertyType(propertyType);
 
-        // check to see if the discriminator property is an enum string
-        // If it is not in the schema it should be in the reference schema of sourceDiscriminator.
-        for (Schema s : List.of(schema, discriminatorAndReferenceSchema.referenceSchema)) {
-            Optional<Schema> discriminatorSchema = Optional.ofNullable(s.getProperties()).map(p -> (Schema) p.get(discriminatorPropertyName));
-            if (discriminatorSchema.isPresent()) {
-                Optional<Schema> refSchema = Optional.ofNullable(discriminatorSchema.get().get$ref())
-                        .map(ModelUtils::getSimpleRef)
-                        .map(n -> ModelUtils.getSchema(openAPI, n));
-                Schema correctSchema = refSchema.orElse(discriminatorSchema.get());
-                if (correctSchema instanceof StringSchema && correctSchema.getEnum() != null && !correctSchema.getEnum().isEmpty()) {
-                    discriminator.setIsEnum(true);
-                }
-            }
-        }
-
         discriminator.setMapping(sourceDiscriminator.getMapping());
         List<MappedModel> uniqueDescendants = new ArrayList<>();
         if (sourceDiscriminator.getMapping() != null && !sourceDiscriminator.getMapping().isEmpty()) {
@@ -3597,6 +3574,32 @@ protected CodegenDiscriminator createDiscriminator(String schemaName, Schema sch
         }
         discriminator.getMappedModels().addAll(uniqueDescendants);
 
+        // check current schema, all parents and descendants to see if the discriminator property is an enum string
+        List<Schema> schemasToCheckForEnumDiscriminator = new ArrayList<>();
+        schemasToCheckForEnumDiscriminator.add(schema);
+        if (ModelUtils.isComposedSchema(schema)) {
+            ModelUtils.getAllParentsName(schema, openAPI.getComponents().getSchemas(), true).stream()
+                    .map(n -> ModelUtils.getSchema(openAPI, n))
+                    .forEach(schemasToCheckForEnumDiscriminator::add);
+        }
+        uniqueDescendants.stream().map(MappedModel::getModelName)
+                .map(n->ModelUtils.getSchema(openAPI, n))
+                .forEach(schemasToCheckForEnumDiscriminator::add);
+        for (Schema schemaToCheck : schemasToCheckForEnumDiscriminator) {
+            if (schemaToCheck == null) {
+                continue;
+            }
+            boolean hasDiscriminatorEnum = Optional.ofNullable(schemaToCheck.getProperties())
+                    .map(p -> (Schema) p.get(discriminatorPropertyName))
+                    .map(s -> ModelUtils.getReferencedSchema(openAPI, s))
+                    .filter(s -> s instanceof StringSchema && s.getEnum() != null && !s.getEnum().isEmpty())
+                    .isPresent();
+            if (hasDiscriminatorEnum) {
+                discriminator.setIsEnum(true);
+                break;
+            }
+        }
+
         return discriminator;
     }
 

diff --git a/modules/openapi-generator/src/main/java/org/openapitools/codegen/utils/ModelUtils.java b/modules/openapi-generator/src/main/java/org/openapitools/codegen/utils/ModelUtils.java
@@ -1595,6 +1595,11 @@ public static String getParentName(Schema composedSchema, Map<String, Schema> al
      * @return the name of the parent model
      */
     public static List<String> getAllParentsName(Schema composedSchema, Map<String, Schema> allSchemas, boolean includeAncestors) {
+        return getAllParentsName(composedSchema, allSchemas, includeAncestors, new HashSet<>());
+    }
+
+    public static List<String> getAllParentsName(
+            Schema composedSchema, Map<String, Schema> allSchemas, boolean includeAncestors, Set<String> seenNames) {
         List<Schema> interfaces = getInterfaces(composedSchema);
         List<String> names = new ArrayList<String>();
 
@@ -1603,6 +1608,10 @@ public static List<String> getAllParentsName(Schema composedSchema, Map<String,
                 // get the actual schema
                 if (StringUtils.isNotEmpty(schema.get$ref())) {
                     String parentName = getSimpleRef(schema.get$ref());
+                    if (seenNames.contains(parentName)) {
+                        continue;
+                    }
+                    seenNames.add(parentName);
                     Schema s = allSchemas.get(parentName);
                     if (s == null) {
                         LOGGER.error("Failed to obtain schema from {}", parentName);
@@ -1611,7 +1620,7 @@ public static List<String> getAllParentsName(Schema composedSchema, Map<String,
                         // discriminator.propertyName is used or x-parent is used
                         names.add(parentName);
                         if (includeAncestors && isComposedSchema(s)) {
-                            names.addAll(getAllParentsName(s, allSchemas, true));
+                            names.addAll(getAllParentsName(s, allSchemas, true, seenNames));
                         }
                     } else {
                         // not a parent since discriminator.propertyName is not set

diff --git a/...nerator/src/test/java/org/openapitools/codegen/csharpnetcore/CSharpClientCodegenTest.java b/...nerator/src/test/java/org/openapitools/codegen/csharpnetcore/CSharpClientCodegenTest.java
@@ -159,20 +159,26 @@ public void testEnumDiscriminatorDefaultValueIsNotString() throws IOException {
         Map<String, File> files = defaultGenerator.generate().stream()
                 .collect(Collectors.toMap(File::getPath, Function.identity()));
 
-        Map<String, String> expectedMapping = Map.of(
-                "Catty", "Cat",
-                "Lizzy", "Lizard",
-                "Dog", "Dog"
+        Map<String, String> expectedContents = Map.of(
+                "Cat", "PetTypeEnum petType = PetTypeEnum.Catty",
+                "Dog", "PetTypeEnum petType = PetTypeEnum.Dog",
+                "Gecko", "PetTypeEnum petType = PetTypeEnum.Gecko",
+                "Chameleon", "PetTypeEnum petType = PetTypeEnum.Camo",
+                "MiniVan", "CarType carType = CarType.MiniVan",
+                "CargoVan", "CarType carType = CarType.CargoVan",
+                "SUV", "CarType carType = CarType.SUV",
+                "Truck", "CarType carType = CarType.Truck"
+
         );
-        for (Map.Entry<String, String> e : expectedMapping.entrySet()) {
-            String modelName = e.getValue();
-            String enumValue = e.getKey();
+        for (Map.Entry<String, String> e : expectedContents.entrySet()) {
+            String modelName = e.getKey();
+            String expectedContent = e.getValue();
             File file = files.get(Paths
                     .get(output.getAbsolutePath(), "src", "Org.OpenAPITools", "Model", modelName + ".cs")
                     .toString()
             );
             assertNotNull(file, "Could not find file for model: " + modelName);
-            assertFileContains(file.toPath(), "AnimalType petType = AnimalType." + enumValue);
+            assertFileContains(file.toPath(), expectedContent);
         }
     }
 }
diff --git a/.../openapi-generator/src/test/java/org/openapitools/codegen/java/JavaClientCodegenTest.java b/.../openapi-generator/src/test/java/org/openapitools/codegen/java/JavaClientCodegenTest.java
@@ -2297,10 +2297,20 @@ public void testAllOfWithSinglePrimitiveTypeRef() {
         Map<String, File> files = new DefaultGenerator().opts(new ClientOptInput().openAPI(openAPI).config(codegen))
             .generate().stream().collect(Collectors.toMap(File::getName, Function.identity()));
 
-        File fooEntityFile = files.get("Cat.java");
-        assertNotNull(fooEntityFile);
-        assertThat(fooEntityFile).content()
-            .doesNotContain("this.petType = this.getClass().getSimpleName();");
+        List<String> entities = List.of(
+                "Cat",
+                "Dog",
+                "Gecko",
+                "Chameleon",
+                "MiniVan",
+                "CargoVan",
+                "SUV",
+                "Truck");
+        for (String entity : entities) {
+            File entityFile = files.get(entity + ".java");
+            assertNotNull(entityFile);
+            assertThat(entityFile).content().doesNotContain("Type = this.getClass().getSimpleName();");
+        }
     }
 
     @Test

diff --git a/modules/openapi-generator/src/test/resources/3_0/enum_discriminator_inheritance.yaml b/modules/openapi-generator/src/test/resources/3_0/enum_discriminator_inheritance.yaml
@@ -17,22 +17,22 @@ components:
     Animal:
       type: object
       properties:
+        # Inline enum type
         petType:
-          $ref: '#/components/schemas/AnimalType'
+          type: string
+          enum:
+            - Dog
+            - Catty
+            - Gecko
+            - Camo
       discriminator:
         propertyName: petType
-        # Mapping with implicit (Dog), explicit ref (Catty -> Cat), and explicit schema name (Lizzy -> Lizard)
+        # Mapping with implicit (Dog, Gecko), explicit ref (Catty -> Cat), and explicit schema name (Camo -> Chameleon)
         mapping:
           Catty: '#/components/schemas/Cat'
-          Lizzy: 'Lizard'
+          Camo: 'Chameleon'
       required:
         - petType
-    AnimalType:
-      type: string
-      enum:
-        - Dog
-        - Catty
-        - Lizzy
     Cat:
       type: object
       allOf:
@@ -48,9 +48,81 @@ components:
         bark:
           type: string
     Lizard:
+      oneOf:
+        - $ref: '#/components/schemas/Gecko'
+        - $ref: '#/components/schemas/Chameleon'
+    Gecko:
       type: object
       allOf:
         - $ref: '#/components/schemas/Animal'
       properties:
         lovesRocks:
           type: string
+    Chameleon:
+      type: object
+      allOf:
+        - $ref: '#/components/schemas/Animal'
+      properties:
+        currentColor:
+          type: string
+    # Car inheritance tree: Car -> Truck -> SUV
+    #                       Car -> Van -> MiniVan
+    #                       Car -> Van -> CargoVan
+    Car:
+      type: object
+      required:
+        - carType
+      # Discriminator carType not defined in Car properties, but in child properties
+      discriminator:
+        propertyName: carType
+    CarType:
+      type: string
+      enum:
+        - Truck
+        - SUV
+        - Sedan
+        - MiniVan
+        - CargoVan
+    Truck:
+      type: object
+      allOf:
+        - $ref: '#/components/schemas/Car'
+      properties:
+        carType:
+          $ref: '#/components/schemas/CarType'
+      required:
+        - carType
+    SUV:
+      type: object
+      # SUV gets its discriminator property from Truck
+      allOf:
+        - $ref: '#/components/schemas/Truck'
+    Sedan:
+      type: object
+      allOf:
+        - $ref: '#/components/schemas/Car'
+      properties:
+        carType:
+          $ref: '#/components/schemas/CarType'
+    Van:
+      oneOf:
+        - $ref: '#/components/schemas/MiniVan'
+        - $ref: '#/components/schemas/CargoVan'
+    MiniVan:
+      type: object
+      allOf:
+        - $ref: '#/components/schemas/Car'
+      properties:
+        carType:
+          $ref: '#/components/schemas/CarType'
+      required:
+        - carType
+    CargoVan:
+      type: object
+      allOf:
+        - $ref: '#/components/schemas/Car'
+      properties:
+        carType:
+          $ref: '#/components/schemas/CarType'
+      required:
+        - carType