[normalizer] bug fixes (isNullTypeSchema, handling of primitive types with oneOf) (#19781)

wing328 · web-flow · commit ad6c2dd2b776 · 2024-10-05T13:57:11.000+08:00
* better handling of primivitype type with oneOf

* fix null check, add tests

* add check for properties
diff --git a/modules/openapi-generator/src/main/java/org/openapitools/codegen/OpenAPINormalizer.java b/modules/openapi-generator/src/main/java/org/openapitools/codegen/OpenAPINormalizer.java
@@ -642,22 +642,29 @@ private Schema normalizeAllOfWithProperties(Schema schema, Set<Schema> visitedSc
     }
 
     private Schema normalizeOneOf(Schema schema, Set<Schema> visitedSchemas) {
-        for (int i = 0; i < schema.getOneOf().size(); i++) {
-            // normalize oneOf sub schemas one by one
-            Object item = schema.getOneOf().get(i);
+        // simplify first as the schema may no longer be a oneOf after processing the rule below
+        schema = processSimplifyOneOf(schema);
 
-            if (item == null) {
-                continue;
-            }
-            if (!(item instanceof Schema)) {
-                throw new RuntimeException("Error! oneOf schema is not of the type Schema: " + item);
-            }
+        // if it's still a oneOf, loop through the sub-schemas
+        if (schema.getOneOf() != null) {
+            for (int i = 0; i < schema.getOneOf().size(); i++) {
+                // normalize oneOf sub schemas one by one
+                Object item = schema.getOneOf().get(i);
 
-            // update sub-schema with the updated schema
-            schema.getOneOf().set(i, normalizeSchema((Schema) item, visitedSchemas));
+                if (item == null) {
+                    continue;
+                }
+                if (!(item instanceof Schema)) {
+                    throw new RuntimeException("Error! oneOf schema is not of the type Schema: " + item);
+                }
+
+                // update sub-schema with the updated schema
+                schema.getOneOf().set(i, normalizeSchema((Schema) item, visitedSchemas));
+            }
+        } else {
+            // normalize it as it's no longer an oneOf
+            schema = normalizeSchema(schema, visitedSchemas);
         }
-        // process rules here
-        schema = processSimplifyOneOf(schema);
 
         return schema;
     }
@@ -683,7 +690,7 @@ private Schema normalizeAnyOf(Schema schema, Set<Schema> visitedSchemas) {
         schema = processSimplifyAnyOf(schema);
 
         // last rule to process as the schema may become String schema (not "anyOf") after the completion
-        return processSimplifyAnyOfStringAndEnumString(schema);
+        return normalizeSchema(processSimplifyAnyOfStringAndEnumString(schema), visitedSchemas);
     }
 
     private Schema normalizeComplexComposedSchema(Schema schema, Set<Schema> visitedSchemas) {
@@ -694,7 +701,7 @@ private Schema normalizeComplexComposedSchema(Schema schema, Set<Schema> visited
 
         processRemoveAnyOfOneOfAndKeepPropertiesOnly(schema);
 
-        return schema;
+        return normalizeSchema(schema, visitedSchemas);
     }
 
     // ===================== a list of rules =====================
@@ -893,21 +900,40 @@ private Schema processSimplifyAnyOfStringAndEnumString(Schema schema) {
     }
 
     /**
-     * Check if the schema is of type 'null'
+     * Check if the schema is of type 'null' or schema itself is pointing to null
      * <p>
      * Return true if the schema's type is 'null' or not specified
      *
      * @param schema Schema
+     * @param openAPI OpenAPI
+     *
+     * @return true if schema is null type
      */
-    public boolean isNullTypeSchema(Schema schema) {
+    public boolean isNullTypeSchema(OpenAPI openAPI, Schema schema) {
         if (schema == null) {
             return true;
         }
 
+        // dereference the schema
+        schema = ModelUtils.getReferencedSchema(openAPI, schema);
+
+        // allOf/anyOf/oneOf
         if (ModelUtils.hasAllOf(schema) || ModelUtils.hasOneOf(schema) || ModelUtils.hasAnyOf(schema)) {
             return false;
         }
 
+        // schema with properties
+        if (schema.getProperties() != null) {
+            return false;
+        }
+
+        // convert referenced enum of null only to `nullable:true`
+        if (schema.getEnum() != null && schema.getEnum().size() == 1) {
+            if ("null".equals(String.valueOf(schema.getEnum().get(0)))) {
+                return true;
+            }
+        }
+
         if (schema.getTypes() != null && !schema.getTypes().isEmpty()) {
             // 3.1 spec
             if (schema.getTypes().size() == 1) { // 1 type only
@@ -933,14 +959,6 @@ public boolean isNullTypeSchema(Schema schema) {
             }
         }
 
-        // convert referenced enum of null only to `nullable:true`
-        Schema referencedSchema = ModelUtils.getReferencedSchema(openAPI, schema);
-        if (referencedSchema.getEnum() != null && referencedSchema.getEnum().size() == 1) {
-            if ("null".equals(String.valueOf(referencedSchema.getEnum().get(0)))) {
-                return true;
-            }
-        }
-
         return false;
     }
 
@@ -986,7 +1004,7 @@ private Schema processSimplifyOneOf(Schema schema) {
                 }
             }
 
-            if (oneOfSchemas.removeIf(oneOf -> isNullTypeSchema(oneOf))) {
+            if (oneOfSchemas.removeIf(oneOf -> isNullTypeSchema(openAPI, oneOf))) {
                 schema.setNullable(true);
 
                 // if only one element left, simplify to just the element (schema)
@@ -997,6 +1015,11 @@ private Schema processSimplifyOneOf(Schema schema) {
                     return (Schema) oneOfSchemas.get(0);
                 }
             }
+
+            if (ModelUtils.isIntegerSchema(schema) || ModelUtils.isNumberSchema(schema) || ModelUtils.isStringSchema(schema)) {
+                // TODO convert oneOf const to enum
+                schema.setOneOf(null);
+            }
         }
 
         return schema;
@@ -1117,7 +1140,7 @@ private Schema processSimplifyAnyOf(Schema schema) {
                 }
             }
 
-            if (anyOfSchemas.removeIf(anyOf -> isNullTypeSchema(anyOf))) {
+            if (anyOfSchemas.removeIf(anyOf -> isNullTypeSchema(openAPI, anyOf))) {
                 schema.setNullable(true);
             }
 
diff --git a/modules/openapi-generator/src/test/java/org/openapitools/codegen/OpenAPINormalizerTest.java b/modules/openapi-generator/src/test/java/org/openapitools/codegen/OpenAPINormalizerTest.java
@@ -113,7 +113,7 @@ public void isNullTypeSchemaTest() {
         Map<String, String> options = new HashMap<>();
         OpenAPINormalizer openAPINormalizer = new OpenAPINormalizer(openAPI, options);
         Schema schema = openAPI.getComponents().getSchemas().get("AnyOfStringArrayOfString");
-        assertFalse(openAPINormalizer.isNullTypeSchema(schema));
+        assertFalse(openAPINormalizer.isNullTypeSchema(openAPI, schema));
     }
 
     @Test
@@ -705,6 +705,12 @@ public void testOpenAPINormalizerSimplifyOneOfAnyOf31Spec() {
         Schema schema13 = openAPI.getComponents().getSchemas().get("OneOfAnyType");
         assertEquals(schema13.getOneOf().size(), 6);
 
+        Schema schema15 = openAPI.getComponents().getSchemas().get("TypeIntegerWithOneOf");
+        assertEquals(schema15.getOneOf().size(), 3);
+
+        Schema schema17 = openAPI.getComponents().getSchemas().get("OneOfNullAndRef3");
+        assertEquals(schema17.getOneOf().size(), 2);
+
         Map<String, String> options = new HashMap<>();
         options.put("SIMPLIFY_ONEOF_ANYOF", "true");
         OpenAPINormalizer openAPINormalizer = new OpenAPINormalizer(openAPI, options);
@@ -742,5 +748,14 @@ public void testOpenAPINormalizerSimplifyOneOfAnyOf31Spec() {
         Schema schema14 = openAPI.getComponents().getSchemas().get("OneOfAnyType");
         assertEquals(schema14.getOneOf(), null);
         assertEquals(schema14.getType(), null);
+
+        Schema schema16 = openAPI.getComponents().getSchemas().get("TypeIntegerWithOneOf");
+        // oneOf should have been removed as the schema is essentially a primitive type
+        assertEquals(schema16.getOneOf(), null);
+
+        Schema schema18 = openAPI.getComponents().getSchemas().get("OneOfNullAndRef3");
+        // original oneOf removed and simplified to just $ref (oneOf sub-schema) instead
+        assertEquals(schema18.getOneOf(), null);
+        assertEquals(schema18.get$ref(), "#/components/schemas/Parent");
     }
 }
diff --git a/modules/openapi-generator/src/test/resources/3_1/simplifyOneOfAnyOf_test.yaml b/modules/openapi-generator/src/test/resources/3_1/simplifyOneOfAnyOf_test.yaml
@@ -112,3 +112,29 @@ components:
         - type: integer
         - type: array
           items: {}
+    TypeIntegerWithOneOf:
+      type: integer
+      oneOf:
+        - title: ITEM A
+          description: This permission is for item A.
+          const: 1
+        - title: ITEM B
+          description: This permission is for item B.
+          const: 2
+        - title: ITEM C
+          description: This permission is for item C.
+          const: 4
+      format: int32
+    # need to repeat the issue when it only occurs with the 3rd, 4th, 5th, etc schemas with oneOf(type: null, $ref)
+    OneOfNullAndRef:
+      oneOf:
+        - $ref: '#/components/schemas/Parent'
+        - type: "null"
+    OneOfNullAndRef2:
+      oneOf:
+        - $ref: '#/components/schemas/Parent'
+        - type: "null"
+    OneOfNullAndRef3:
+      oneOf:
+        - $ref: '#/components/schemas/Parent'
+        - type: "null"
