User feedback on confusing use of oneOf (#4695)

OAS 3.x specification isn't entirely clear on oneOf support. JSON Schema
defines oneOf in such a way that the Schema is only valid if it
validates against exactly one of the referenced Schemas. This suggests
that a Schema defined with oneOf can't include additional "dynamic"
properties. OpenAPI extends on this by adding the necessary
discriminator object, which allows tooling to decide the intended
Schema.

As tooling, openapi-generator may support loose or confusing definitions
in the Specification to better support our user's use cases. In this
case, we may warn that while this usage is technically valid the two
target specifications are unclear about the actual constraints regarding
oneOf.

Author: jimschubert

modules/openapi-generator-cli/src/main/java/org/openapitools/codegen/cmd/Validate.java

@@ -22,11 +22,14 @@
 
 import io.swagger.parser.OpenAPIParser;
 import io.swagger.v3.oas.models.OpenAPI;
+import io.swagger.v3.oas.models.media.ComposedSchema;
+import io.swagger.v3.oas.models.media.Schema;
 import io.swagger.v3.parser.core.models.SwaggerParseResult;
 import org.openapitools.codegen.utils.ModelUtils;
 
 import java.util.HashSet;
 import java.util.List;
+import java.util.Map;
 import java.util.Set;
 
 @Command(name = "validate", description = "Validate specification")
@@ -58,6 +61,21 @@ public void run() {
                 if (unusedModels != null) {
                     unusedModels.forEach(name -> warnings.add("Unused model: " + name));
                 }
+
+                // check for loosely defined oneOf extension requirements.
+                // This is a recommendation because the 3.0.x spec is not clear enough on usage of oneOf.
+                // see https://json-schema.org/draft/2019-09/json-schema-core.html#rfc.section.9.2.1.3 and the OAS section on 'Composition and Inheritance'.
+                Map<String, Schema> schemas = ModelUtils.getSchemas(specification);
+                schemas.forEach((key, schema) -> {
+                    if (schema instanceof ComposedSchema) {
+                        final ComposedSchema composed = (ComposedSchema) schema;
+                        if (composed.getOneOf() != null && composed.getOneOf().size() > 0) {
+                            if (composed.getProperties() != null && composed.getProperties().size() >= 1 && composed.getProperties().get("discriminator") == null) {
+                                warnings.add("Schema (oneOf) should not contain properties: " + key);
+                            }
+                        }
+                    }
+                });
             }
         }
 

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

@@ -1789,21 +1789,18 @@ public CodegenModel fromModel(String name, Schema schema) {
             List<String> allRequired = new ArrayList<String>();
 
             // if schema has properties outside of allOf/oneOf/anyOf also add them to m
-            if (schema.getProperties() != null) {
-                addVars(m, unaliasPropertySchema(schema.getProperties()), schema.getRequired(), null, null);
+            if (composed.getProperties() != null && !composed.getProperties().isEmpty()) {
+                if (composed.getOneOf() != null && !composed.getOneOf().isEmpty()) {
+                    LOGGER.warn("'oneOf' is intended to include only the additional optional OAS extension discriminator object. " +
+                            "For more details, see https://json-schema.org/draft/2019-09/json-schema-core.html#rfc.section.9.2.1.3 and the OAS section on 'Composition and Inheritance'.");
+                }
+                addVars(m, unaliasPropertySchema(composed.getProperties()), composed.getRequired(), null, null);
             }
 
-            // uncomment this when https://github.com/swagger-api/swagger-parser/issues/1252 is resolved
-            // if schema has additionalproperties outside of allOf/oneOf/anyOf also add it to m
-            // if (schema.getAdditionalProperties() != null) {
-            //     addAdditionPropertiesToCodeGenModel(m, schema);
-            // }
-
             // parent model
             final String parentName = ModelUtils.getParentName(composed, allDefinitions);
             final List<String> allParents = ModelUtils.getAllParentsName(composed, allDefinitions);
             final Schema parent = StringUtils.isBlank(parentName) || allDefinitions == null ? null : allDefinitions.get(parentName);
-            final boolean hasParent = StringUtils.isNotBlank(parentName);
 
             // TODO revise the logic below to set dicriminator, xml attributes
             if (supportsInheritance || supportsMixins) {

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

@@ -35,6 +35,7 @@
 
 import java.util.ArrayList;
 import java.util.Collections;
+import java.util.HashMap;
 import java.util.List;
 import java.util.Map;
 import java.util.Map.Entry;
@@ -117,7 +118,18 @@ public static List<String> getAllUsedSchemas(OpenAPI openAPI) {
      * @return schemas a list of unused schemas
      */
     public static List<String> getUnusedSchemas(OpenAPI openAPI) {
-        Map<String, List<String>> childrenMap = getChildrenMap(openAPI);
+        final  Map<String, List<String>> childrenMap;
+        Map<String, List<String>> tmpChildrenMap;
+        try {
+            tmpChildrenMap = getChildrenMap(openAPI);
+        } catch (NullPointerException npe) {
+            // in rare cases, such as a spec document with only one top-level oneOf schema and multiple referenced schemas,
+            // the stream used in getChildrenMap will raise an NPE. Rather than modify getChildrenMap which is used by getAllUsedSchemas,
+            // we'll catch here as a workaround for this edge case.
+            tmpChildrenMap = new HashMap<>();
+        }
+
+        childrenMap = tmpChildrenMap;
         List<String> unusedSchemas = new ArrayList<String>();
 
         Map<String, Schema> schemas = getSchemas(openAPI);
@@ -875,6 +887,7 @@ public static Header getHeader(OpenAPI openAPI, String name) {
     public static Map<String, List<String>> getChildrenMap(OpenAPI openAPI) {
         Map<String, Schema> allSchemas = getSchemas(openAPI);
 
+        // FIXME: The collect here will throw NPE if a spec document has only a single oneOf hierarchy.
         Map<String, List<Entry<String, Schema>>> groupedByParent = allSchemas.entrySet().stream()
             .filter(entry -> isComposedSchema(entry.getValue()))
             .collect(Collectors.groupingBy(entry -> getParentName((ComposedSchema) entry.getValue(), allSchemas)));