Skip to content

[core] Provide User feedback on confusing use of oneOf #4695

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Dec 7, 2019
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -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")
Expand Down Expand Up @@ -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);
}
}
}
});
}
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are we deleting this because it is unused?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah. This would fail compilation of it was still expected.


// TODO revise the logic below to set dicriminator, xml attributes
if (supportsInheritance || supportsMixins) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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);
Expand Down Expand Up @@ -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)));
Expand Down