From 88856720c4cff2e2a069cca990ec14029e021b60 Mon Sep 17 00:00:00 2001 From: Jim Schubert Date: Sat, 11 Jan 2020 23:36:34 -0500 Subject: [PATCH 1/6] [core] Extracting recommendations to validation framework This is work to extract recommendation logic out of the CLI validate command into a shared evaluation instance which can be used elsewhere (such as Gradle, or the Online tool). For now, these validations are in addition to those provided by swagger-parser and are only the following recommendations: * Apache/Nginx warning that header values with underscore are dropped by default * Unused models/schemas * Use of properties with oneOf, which is ambiguous in OpenAPI Specification I've allowed for disabling recommendations via System properties, since this is something that has been requested a few times by users. System properties in this commit include: * openapi.generator.rule.recommendations=false - Allows for disabling recommendations completely. This wouldn't include all warnings and errors, only those we deem to be suggestions * openapi.generator.rule.apache-nginx-underscore=false - Allows for disabling the Apache/Nginx warning when header names have underscore - This is a legacy CGI configuration, and doesn't affect all web servers * openapi.generator.rule.oneof-properties-ambiguity=false - We support this functionality, but the specification may not intend for it - This is more to reduce noise * openapi.generator.rule.unused-schemas=false - We will warn when a schema is not referenced outside of Components, which users have requested to be able to turn off --- .../openapitools/codegen/cmd/Validate.java | 47 +++++----- .../codegen/validation/GenericValidator.java | 2 +- .../codegen/validation/Invalid.java | 4 +- .../codegen/validation/ValidationResult.java | 9 +- .../codegen/validations/OpenApiEvaluator.java | 91 +++++++++++++++++++ .../OpenApiParameterValidations.java | 36 ++++++++ .../validations/OpenApiSchemaValidations.java | 52 +++++++++++ .../OpenApiSecuritySchemeValidations.java | 29 ++++++ .../validations/RuleConfiguration.java | 47 ++++++++++ .../validations/ValidationConstants.java | 6 ++ 10 files changed, 293 insertions(+), 30 deletions(-) create mode 100644 modules/openapi-generator/src/main/java/org/openapitools/codegen/validations/OpenApiEvaluator.java create mode 100644 modules/openapi-generator/src/main/java/org/openapitools/codegen/validations/OpenApiParameterValidations.java create mode 100644 modules/openapi-generator/src/main/java/org/openapitools/codegen/validations/OpenApiSchemaValidations.java create mode 100644 modules/openapi-generator/src/main/java/org/openapitools/codegen/validations/OpenApiSecuritySchemeValidations.java create mode 100644 modules/openapi-generator/src/main/java/org/openapitools/codegen/validations/RuleConfiguration.java create mode 100644 modules/openapi-generator/src/main/java/org/openapitools/codegen/validations/ValidationConstants.java diff --git a/modules/openapi-generator-cli/src/main/java/org/openapitools/codegen/cmd/Validate.java b/modules/openapi-generator-cli/src/main/java/org/openapitools/codegen/cmd/Validate.java index 153475f5032a..237eded35786 100644 --- a/modules/openapi-generator-cli/src/main/java/org/openapitools/codegen/cmd/Validate.java +++ b/modules/openapi-generator-cli/src/main/java/org/openapitools/codegen/cmd/Validate.java @@ -21,11 +21,20 @@ import io.airlift.airline.Option; import io.swagger.parser.OpenAPIParser; +import io.swagger.v3.oas.models.Components; import io.swagger.v3.oas.models.OpenAPI; +import io.swagger.v3.oas.models.Paths; import io.swagger.v3.oas.models.media.ComposedSchema; import io.swagger.v3.oas.models.media.Schema; +import io.swagger.v3.oas.models.security.SecurityScheme; import io.swagger.v3.parser.core.models.SwaggerParseResult; +import org.apache.commons.lang3.text.WordUtils; import org.openapitools.codegen.utils.ModelUtils; +import org.openapitools.codegen.validation.ValidationResult; +import org.openapitools.codegen.validations.OpenApiEvaluator; +import org.openapitools.codegen.validations.OpenApiParameterValidations; +import org.openapitools.codegen.validations.OpenApiSecuritySchemeValidations; +import org.openapitools.codegen.validations.RuleConfiguration; import java.util.HashSet; import java.util.List; @@ -54,42 +63,28 @@ public void run() { StringBuilder sb = new StringBuilder(); OpenAPI specification = result.getOpenAPI(); - if (Boolean.TRUE.equals(recommend)) { - if (specification != null) { - // Add information about unused models to the warnings set. - List unusedModels = ModelUtils.getUnusedSchemas(specification); - 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 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); - } - } - } - }); - } - } + RuleConfiguration ruleConfiguration = new RuleConfiguration(); + ruleConfiguration.setEnableRecommendations(recommend != null ? recommend : true); + + OpenApiEvaluator evaluator = new OpenApiEvaluator(ruleConfiguration); + ValidationResult validationResult = evaluator.validate(specification); + + // TODO: We could also provide description here along with getMessage. getMessage is either a "generic" message or specific (e.g. Model 'Cat' has issues). + // This would require that we parse the messageList coming from swagger-parser into a better structure. + validationResult.getWarnings().forEach(invalid -> warnings.add(invalid.getMessage())); + validationResult.getErrors().forEach(invalid -> errors.add(invalid.getMessage())); if (!errors.isEmpty()) { sb.append("Errors:").append(System.lineSeparator()); errors.forEach(msg -> - sb.append("\t-").append(msg).append(System.lineSeparator()) + sb.append("\t- ").append(WordUtils.wrap(msg, 90).replace(System.lineSeparator(), System.lineSeparator() + "\t ")).append(System.lineSeparator()) ); } if (!warnings.isEmpty()) { sb.append("Warnings: ").append(System.lineSeparator()); warnings.forEach(msg -> - sb.append("\t-").append(msg).append(System.lineSeparator()) + sb.append("\t- ").append(WordUtils.wrap(msg, 90).replace(System.lineSeparator(), System.lineSeparator() + "\t ")).append(System.lineSeparator()) ); } diff --git a/modules/openapi-generator-core/src/main/java/org/openapitools/codegen/validation/GenericValidator.java b/modules/openapi-generator-core/src/main/java/org/openapitools/codegen/validation/GenericValidator.java index c095bc5abd54..3e52ecbdba7d 100644 --- a/modules/openapi-generator-core/src/main/java/org/openapitools/codegen/validation/GenericValidator.java +++ b/modules/openapi-generator-core/src/main/java/org/openapitools/codegen/validation/GenericValidator.java @@ -25,7 +25,7 @@ */ @SuppressWarnings({"WeakerAccess"}) public class GenericValidator implements Validator { - private List rules; + protected List rules; /** * Constructs a new instance of {@link GenericValidator}. diff --git a/modules/openapi-generator-core/src/main/java/org/openapitools/codegen/validation/Invalid.java b/modules/openapi-generator-core/src/main/java/org/openapitools/codegen/validation/Invalid.java index ad59a6285369..67c8e6ff642f 100644 --- a/modules/openapi-generator-core/src/main/java/org/openapitools/codegen/validation/Invalid.java +++ b/modules/openapi-generator-core/src/main/java/org/openapitools/codegen/validation/Invalid.java @@ -36,12 +36,12 @@ public final class Invalid extends Validated { } @Override - String getMessage() { + public String getMessage() { return message; } @Override - ValidationRule getRule() { + public ValidationRule getRule() { return rule; } diff --git a/modules/openapi-generator-core/src/main/java/org/openapitools/codegen/validation/ValidationResult.java b/modules/openapi-generator-core/src/main/java/org/openapitools/codegen/validation/ValidationResult.java index 374c35445441..0b3f451d4d2c 100644 --- a/modules/openapi-generator-core/src/main/java/org/openapitools/codegen/validation/ValidationResult.java +++ b/modules/openapi-generator-core/src/main/java/org/openapitools/codegen/validation/ValidationResult.java @@ -96,9 +96,16 @@ public List getWarnings(){ public void addResult(Validated validated) { synchronized (validations) { ValidationRule rule = validated.getRule(); - if (rule != null && !rule.equals(ValidationRule.empty())) { + if (rule != null && !rule.equals(ValidationRule.empty()) && !validations.contains(validated)) { validations.add(validated); } } } + + public ValidationResult consume(ValidationResult other) { + synchronized (validations) { + validations.addAll(other.validations); + } + return this; + } } diff --git a/modules/openapi-generator/src/main/java/org/openapitools/codegen/validations/OpenApiEvaluator.java b/modules/openapi-generator/src/main/java/org/openapitools/codegen/validations/OpenApiEvaluator.java new file mode 100644 index 000000000000..6dd51540e91e --- /dev/null +++ b/modules/openapi-generator/src/main/java/org/openapitools/codegen/validations/OpenApiEvaluator.java @@ -0,0 +1,91 @@ +package org.openapitools.codegen.validations; + +import io.swagger.v3.oas.models.Components; +import io.swagger.v3.oas.models.OpenAPI; +import io.swagger.v3.oas.models.Paths; +import io.swagger.v3.oas.models.media.Schema; +import io.swagger.v3.oas.models.parameters.Parameter; +import io.swagger.v3.oas.models.security.SecurityScheme; +import org.openapitools.codegen.utils.ModelUtils; +import org.openapitools.codegen.validation.*; + +import java.util.ArrayList; +import java.util.List; +import java.util.Map; +import java.util.stream.Stream; + +public class OpenApiEvaluator implements Validator { + private RuleConfiguration ruleConfiguration; + + public OpenApiEvaluator(RuleConfiguration ruleConfiguration) { + this.ruleConfiguration = ruleConfiguration; + } + + /** + * Validates input, resulting in a instance of {@link ValidationResult} which provides details on all validations performed (success, error, warning). + * + * @param specification The {@link OpenAPI} object instance to be validated. + * @return A {@link ValidationResult} which details the success, error, and warning validation results. + */ + @Override + public ValidationResult validate(OpenAPI specification) { + ValidationResult validationResult = new ValidationResult(); + if (specification == null) return validationResult; + + OpenApiParameterValidations parameterValidations = new OpenApiParameterValidations(ruleConfiguration); + OpenApiSecuritySchemeValidations securitySchemeValidations = new OpenApiSecuritySchemeValidations(ruleConfiguration); + OpenApiSchemaValidations schemaValidations = new OpenApiSchemaValidations(ruleConfiguration); + + if (ruleConfiguration.isEnableUnusedSchemasSuggestion()) { + ValidationRule unusedSchema = ValidationRule.create(Severity.WARNING, "Unused schema", "A schema was determined to be unused.", s -> true); + ModelUtils.getUnusedSchemas(specification).forEach(schemaName -> validationResult.addResult(Validated.invalid(unusedSchema, "Unused model: " + schemaName))); + } + + Map schemas = ModelUtils.getSchemas(specification); + schemas.forEach((key, schema) -> validationResult.consume(schemaValidations.validate(schema))); + + List parameters = new ArrayList<>(50); + + Paths paths = specification.getPaths(); + if (paths != null) { + paths.forEach((key, pathItem) -> { + // parameters defined "globally" + List pathParameters = pathItem.getParameters(); + if (pathParameters != null) parameters.addAll(pathItem.getParameters()); + + // parameters on each operation method + Stream.of( + pathItem.getGet(), + pathItem.getDelete(), + pathItem.getHead(), + pathItem.getOptions(), + pathItem.getPatch(), + pathItem.getPost(), + pathItem.getPut(), + pathItem.getTrace()).forEach(op -> { + if (op != null && op.getParameters() != null) parameters.addAll(op.getParameters()); + }); + }); + } + + + Components components = specification.getComponents(); + if (components != null) { + Map securitySchemes = components.getSecuritySchemes(); + if (securitySchemes != null && !securitySchemes.isEmpty()) { + securitySchemes.values().forEach(securityScheme -> validationResult.consume(securitySchemeValidations.validate(securityScheme))); + } + + if (components.getParameters() != null) { + parameters.addAll(components.getParameters().values()); + } + } + + parameters.forEach(parameter -> { + validationResult.consume(parameterValidations.validate(parameter)); + }); + + + return validationResult; + } +} diff --git a/modules/openapi-generator/src/main/java/org/openapitools/codegen/validations/OpenApiParameterValidations.java b/modules/openapi-generator/src/main/java/org/openapitools/codegen/validations/OpenApiParameterValidations.java new file mode 100644 index 000000000000..a61e1c0f895f --- /dev/null +++ b/modules/openapi-generator/src/main/java/org/openapitools/codegen/validations/OpenApiParameterValidations.java @@ -0,0 +1,36 @@ +package org.openapitools.codegen.validations; + +import io.swagger.v3.oas.models.parameters.HeaderParameter; +import io.swagger.v3.oas.models.parameters.Parameter; +import org.apache.commons.lang3.StringUtils; +import org.openapitools.codegen.validation.GenericValidator; +import org.openapitools.codegen.validation.ValidationRule; + +import java.util.ArrayList; + +public class OpenApiParameterValidations extends GenericValidator { + OpenApiParameterValidations(RuleConfiguration ruleConfiguration) { + super(new ArrayList<>()); + if (ruleConfiguration.isEnableRecommendations()) { + if (ruleConfiguration.isEnableApacheNginxUnderscoreSuggestion()) { + rules.add(ValidationRule.warn( + ValidationConstants.ApacheNginxUnderscoreDescription, + ValidationConstants.ApacheNginxUnderscoreFailureMessage, + OpenApiParameterValidations::isApacheNginxHeaderSuggestion + )); + } + } + } + + private static boolean isApacheNginxHeaderSuggestion(Parameter parameter) { + boolean result = false; + if (parameter instanceof HeaderParameter) { + HeaderParameter headerParameter = (HeaderParameter) parameter; + String headerName = headerParameter.getName(); + if (StringUtils.isNotEmpty(headerName) && StringUtils.contains(headerName, '_')) { + result = true; + } + } + return result; + } +} diff --git a/modules/openapi-generator/src/main/java/org/openapitools/codegen/validations/OpenApiSchemaValidations.java b/modules/openapi-generator/src/main/java/org/openapitools/codegen/validations/OpenApiSchemaValidations.java new file mode 100644 index 000000000000..4cde0ba3e069 --- /dev/null +++ b/modules/openapi-generator/src/main/java/org/openapitools/codegen/validations/OpenApiSchemaValidations.java @@ -0,0 +1,52 @@ +package org.openapitools.codegen.validations; + +import io.swagger.v3.oas.models.media.ComposedSchema; +import io.swagger.v3.oas.models.media.Schema; +import org.openapitools.codegen.validation.GenericValidator; +import org.openapitools.codegen.validation.ValidationRule; + +import java.util.ArrayList; + +public class OpenApiSchemaValidations extends GenericValidator { + OpenApiSchemaValidations(RuleConfiguration ruleConfiguration) { + super(new ArrayList<>()); + if (ruleConfiguration.isEnableRecommendations()) { + if (ruleConfiguration.isEnableOneOfWithPropertiesSuggestion()) { + rules.add(ValidationRule.warn( + "Schema defines properties alongside oneOf.", + "Schemas defining properties and oneOf are not clearly defined in the OpenAPI Specification. While our tooling supports this, it may cause issues with other tools.", + OpenApiSchemaValidations::hasOneOfWithProperties + )); + } + } + } + + /** + * JSON Schema defines oneOf as a validation property which can be applied to any schema. + *

+ * OpenAPI Specification is a variant of JSON Schema for which oneOf is defined as: + * "Inline or referenced schema MUST be of a Schema Object and not a standard JSON Schema." + *

+ * 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 + * properties on the schema defining oneOf relationships may not be intentional in the OpenAPI Specification. + * + * @param schema An input schema, regardless of the type of schema + * @return true if the schema has oneOf defined along with properties other than discriminator. + */ + private static boolean hasOneOfWithProperties(Schema schema) { + boolean result = false; + if (schema instanceof ComposedSchema) { + final ComposedSchema composed = (ComposedSchema) schema; + // 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'. + if (composed.getOneOf() != null && composed.getOneOf().size() > 0) { + if (composed.getProperties() != null && composed.getProperties().size() >= 1 && composed.getProperties().get("discriminator") == null) { + result = true; + } + } + } + return result; + } +} diff --git a/modules/openapi-generator/src/main/java/org/openapitools/codegen/validations/OpenApiSecuritySchemeValidations.java b/modules/openapi-generator/src/main/java/org/openapitools/codegen/validations/OpenApiSecuritySchemeValidations.java new file mode 100644 index 000000000000..b03c6ce633a1 --- /dev/null +++ b/modules/openapi-generator/src/main/java/org/openapitools/codegen/validations/OpenApiSecuritySchemeValidations.java @@ -0,0 +1,29 @@ +package org.openapitools.codegen.validations; + +import io.swagger.v3.oas.models.security.SecurityScheme; +import org.apache.commons.lang3.StringUtils; +import org.openapitools.codegen.validation.GenericValidator; +import org.openapitools.codegen.validation.ValidationRule; + +import java.util.ArrayList; + +public class OpenApiSecuritySchemeValidations extends GenericValidator { + OpenApiSecuritySchemeValidations(RuleConfiguration ruleConfiguration) { + super(new ArrayList<>()); + if (ruleConfiguration.isEnableRecommendations()) { + if (ruleConfiguration.isEnableApacheNginxUnderscoreSuggestion()) { + rules.add(ValidationRule.warn( + ValidationConstants.ApacheNginxUnderscoreDescription, + ValidationConstants.ApacheNginxUnderscoreFailureMessage, + OpenApiSecuritySchemeValidations::isApacheNginxHeaderSuggestion + )); + } + } + } + + private static boolean isApacheNginxHeaderSuggestion(SecurityScheme securityScheme) { + return securityScheme != null && + SecurityScheme.Type.APIKEY.equals(securityScheme.getType()) && + StringUtils.contains(securityScheme.getName(), '_'); + } +} diff --git a/modules/openapi-generator/src/main/java/org/openapitools/codegen/validations/RuleConfiguration.java b/modules/openapi-generator/src/main/java/org/openapitools/codegen/validations/RuleConfiguration.java new file mode 100644 index 000000000000..87692b7bc951 --- /dev/null +++ b/modules/openapi-generator/src/main/java/org/openapitools/codegen/validations/RuleConfiguration.java @@ -0,0 +1,47 @@ +package org.openapitools.codegen.validations; + +public class RuleConfiguration { + private static String propertyPrefix = "openapi.generator.rule"; + private static boolean defaultedBoolean(String key, boolean defaultValue) { + String property = System.getProperty(key); + if (property == null) return defaultValue; + return Boolean.parseBoolean(property); + } + + private boolean enableRecommendations = defaultedBoolean(propertyPrefix + ".recommendations", true); + private boolean enableApacheNginxUnderscoreSuggestion = defaultedBoolean(propertyPrefix + ".apache-nginx-underscore", true); + private boolean enableOneOfWithPropertiesSuggestion = defaultedBoolean(propertyPrefix + ".oneof-properties-ambiguity", true); + private boolean enableUnusedSchemasSuggestion = defaultedBoolean(propertyPrefix + ".unused-schemas", true); + + public boolean isEnableApacheNginxUnderscoreSuggestion() { + return enableApacheNginxUnderscoreSuggestion; + } + + public boolean isEnableOneOfWithPropertiesSuggestion() { + return enableOneOfWithPropertiesSuggestion; + } + + public boolean isEnableRecommendations() { + return enableRecommendations; + } + + public boolean isEnableUnusedSchemasSuggestion() { + return enableUnusedSchemasSuggestion; + } + + public void setEnableApacheNginxUnderscoreSuggestion(boolean enableApacheNginxUnderscoreSuggestion) { + this.enableApacheNginxUnderscoreSuggestion = enableApacheNginxUnderscoreSuggestion; + } + + public void setEnableOneOfWithPropertiesSuggestion(boolean enableOneOfWithPropertiesSuggestion) { + this.enableOneOfWithPropertiesSuggestion = enableOneOfWithPropertiesSuggestion; + } + + public void setEnableRecommendations(boolean enableRecommendations) { + this.enableRecommendations = enableRecommendations; + } + + public void setEnableUnusedSchemasSuggestion(boolean enableUnusedSchemasSuggestion) { + this.enableUnusedSchemasSuggestion = enableUnusedSchemasSuggestion; + } +} diff --git a/modules/openapi-generator/src/main/java/org/openapitools/codegen/validations/ValidationConstants.java b/modules/openapi-generator/src/main/java/org/openapitools/codegen/validations/ValidationConstants.java new file mode 100644 index 000000000000..69fef91a5435 --- /dev/null +++ b/modules/openapi-generator/src/main/java/org/openapitools/codegen/validations/ValidationConstants.java @@ -0,0 +1,6 @@ +package org.openapitools.codegen.validations; + +final class ValidationConstants { + static String ApacheNginxUnderscoreDescription = "Apache and Nginx may fail on headers keys with underscore!"; + static String ApacheNginxUnderscoreFailureMessage = "Apache and Nginx webservers may fail due to legacy CGI constraints enabled by default in which header keys with underscore are disallowed. See https://stackoverflow.com/a/22856867/151445."; +} From 3c1ef7550609f1278823143c51fda1f5ed8f796f Mon Sep 17 00:00:00 2001 From: Jim Schubert Date: Sun, 12 Jan 2020 10:17:45 -0500 Subject: [PATCH 2/6] Move to oas package and add javadoc comments --- .../openapitools/codegen/cmd/Validate.java | 15 +-- .../validations/RuleConfiguration.java | 47 ------- .../{ => oas}/OpenApiEvaluator.java | 14 ++- .../OpenApiParameterValidations.java | 16 ++- .../{ => oas}/OpenApiSchemaValidations.java | 9 +- .../OpenApiSecuritySchemeValidations.java | 18 ++- .../validations/oas/RuleConfiguration.java | 115 ++++++++++++++++++ .../{ => oas}/ValidationConstants.java | 2 +- 8 files changed, 162 insertions(+), 74 deletions(-) delete mode 100644 modules/openapi-generator/src/main/java/org/openapitools/codegen/validations/RuleConfiguration.java rename modules/openapi-generator/src/main/java/org/openapitools/codegen/validations/{ => oas}/OpenApiEvaluator.java (91%) rename modules/openapi-generator/src/main/java/org/openapitools/codegen/validations/{ => oas}/OpenApiParameterValidations.java (65%) rename modules/openapi-generator/src/main/java/org/openapitools/codegen/validations/{ => oas}/OpenApiSchemaValidations.java (89%) rename modules/openapi-generator/src/main/java/org/openapitools/codegen/validations/{ => oas}/OpenApiSecuritySchemeValidations.java (56%) create mode 100644 modules/openapi-generator/src/main/java/org/openapitools/codegen/validations/oas/RuleConfiguration.java rename modules/openapi-generator/src/main/java/org/openapitools/codegen/validations/{ => oas}/ValidationConstants.java (88%) diff --git a/modules/openapi-generator-cli/src/main/java/org/openapitools/codegen/cmd/Validate.java b/modules/openapi-generator-cli/src/main/java/org/openapitools/codegen/cmd/Validate.java index 237eded35786..a790464c781e 100644 --- a/modules/openapi-generator-cli/src/main/java/org/openapitools/codegen/cmd/Validate.java +++ b/modules/openapi-generator-cli/src/main/java/org/openapitools/codegen/cmd/Validate.java @@ -21,24 +21,15 @@ import io.airlift.airline.Option; import io.swagger.parser.OpenAPIParser; -import io.swagger.v3.oas.models.Components; import io.swagger.v3.oas.models.OpenAPI; -import io.swagger.v3.oas.models.Paths; -import io.swagger.v3.oas.models.media.ComposedSchema; -import io.swagger.v3.oas.models.media.Schema; -import io.swagger.v3.oas.models.security.SecurityScheme; import io.swagger.v3.parser.core.models.SwaggerParseResult; import org.apache.commons.lang3.text.WordUtils; -import org.openapitools.codegen.utils.ModelUtils; import org.openapitools.codegen.validation.ValidationResult; -import org.openapitools.codegen.validations.OpenApiEvaluator; -import org.openapitools.codegen.validations.OpenApiParameterValidations; -import org.openapitools.codegen.validations.OpenApiSecuritySchemeValidations; -import org.openapitools.codegen.validations.RuleConfiguration; +import org.openapitools.codegen.validations.oas.OpenApiEvaluator; +import org.openapitools.codegen.validations.oas.RuleConfiguration; import java.util.HashSet; import java.util.List; -import java.util.Map; import java.util.Set; @Command(name = "validate", description = "Validate specification") @@ -64,7 +55,7 @@ public void run() { OpenAPI specification = result.getOpenAPI(); RuleConfiguration ruleConfiguration = new RuleConfiguration(); - ruleConfiguration.setEnableRecommendations(recommend != null ? recommend : true); + ruleConfiguration.setEnableRecommendations(recommend != null ? recommend : false); OpenApiEvaluator evaluator = new OpenApiEvaluator(ruleConfiguration); ValidationResult validationResult = evaluator.validate(specification); diff --git a/modules/openapi-generator/src/main/java/org/openapitools/codegen/validations/RuleConfiguration.java b/modules/openapi-generator/src/main/java/org/openapitools/codegen/validations/RuleConfiguration.java deleted file mode 100644 index 87692b7bc951..000000000000 --- a/modules/openapi-generator/src/main/java/org/openapitools/codegen/validations/RuleConfiguration.java +++ /dev/null @@ -1,47 +0,0 @@ -package org.openapitools.codegen.validations; - -public class RuleConfiguration { - private static String propertyPrefix = "openapi.generator.rule"; - private static boolean defaultedBoolean(String key, boolean defaultValue) { - String property = System.getProperty(key); - if (property == null) return defaultValue; - return Boolean.parseBoolean(property); - } - - private boolean enableRecommendations = defaultedBoolean(propertyPrefix + ".recommendations", true); - private boolean enableApacheNginxUnderscoreSuggestion = defaultedBoolean(propertyPrefix + ".apache-nginx-underscore", true); - private boolean enableOneOfWithPropertiesSuggestion = defaultedBoolean(propertyPrefix + ".oneof-properties-ambiguity", true); - private boolean enableUnusedSchemasSuggestion = defaultedBoolean(propertyPrefix + ".unused-schemas", true); - - public boolean isEnableApacheNginxUnderscoreSuggestion() { - return enableApacheNginxUnderscoreSuggestion; - } - - public boolean isEnableOneOfWithPropertiesSuggestion() { - return enableOneOfWithPropertiesSuggestion; - } - - public boolean isEnableRecommendations() { - return enableRecommendations; - } - - public boolean isEnableUnusedSchemasSuggestion() { - return enableUnusedSchemasSuggestion; - } - - public void setEnableApacheNginxUnderscoreSuggestion(boolean enableApacheNginxUnderscoreSuggestion) { - this.enableApacheNginxUnderscoreSuggestion = enableApacheNginxUnderscoreSuggestion; - } - - public void setEnableOneOfWithPropertiesSuggestion(boolean enableOneOfWithPropertiesSuggestion) { - this.enableOneOfWithPropertiesSuggestion = enableOneOfWithPropertiesSuggestion; - } - - public void setEnableRecommendations(boolean enableRecommendations) { - this.enableRecommendations = enableRecommendations; - } - - public void setEnableUnusedSchemasSuggestion(boolean enableUnusedSchemasSuggestion) { - this.enableUnusedSchemasSuggestion = enableUnusedSchemasSuggestion; - } -} diff --git a/modules/openapi-generator/src/main/java/org/openapitools/codegen/validations/OpenApiEvaluator.java b/modules/openapi-generator/src/main/java/org/openapitools/codegen/validations/oas/OpenApiEvaluator.java similarity index 91% rename from modules/openapi-generator/src/main/java/org/openapitools/codegen/validations/OpenApiEvaluator.java rename to modules/openapi-generator/src/main/java/org/openapitools/codegen/validations/oas/OpenApiEvaluator.java index 6dd51540e91e..f72d06e88510 100644 --- a/modules/openapi-generator/src/main/java/org/openapitools/codegen/validations/OpenApiEvaluator.java +++ b/modules/openapi-generator/src/main/java/org/openapitools/codegen/validations/oas/OpenApiEvaluator.java @@ -1,4 +1,4 @@ -package org.openapitools.codegen.validations; +package org.openapitools.codegen.validations.oas; import io.swagger.v3.oas.models.Components; import io.swagger.v3.oas.models.OpenAPI; @@ -14,9 +14,17 @@ import java.util.Map; import java.util.stream.Stream; +/** + * A validator which evaluates an OpenAPI 3.x specification document + */ public class OpenApiEvaluator implements Validator { private RuleConfiguration ruleConfiguration; + /** + * Constructs a new instance of {@link OpenApiEvaluator} with applied rules. + * + * @param ruleConfiguration The set of rules to be applied to evaluation. + */ public OpenApiEvaluator(RuleConfiguration ruleConfiguration) { this.ruleConfiguration = ruleConfiguration; } @@ -36,7 +44,7 @@ public ValidationResult validate(OpenAPI specification) { OpenApiSecuritySchemeValidations securitySchemeValidations = new OpenApiSecuritySchemeValidations(ruleConfiguration); OpenApiSchemaValidations schemaValidations = new OpenApiSchemaValidations(ruleConfiguration); - if (ruleConfiguration.isEnableUnusedSchemasSuggestion()) { + if (ruleConfiguration.isEnableUnusedSchemasRecommendation()) { ValidationRule unusedSchema = ValidationRule.create(Severity.WARNING, "Unused schema", "A schema was determined to be unused.", s -> true); ModelUtils.getUnusedSchemas(specification).forEach(schemaName -> validationResult.addResult(Validated.invalid(unusedSchema, "Unused model: " + schemaName))); } @@ -68,7 +76,6 @@ public ValidationResult validate(OpenAPI specification) { }); } - Components components = specification.getComponents(); if (components != null) { Map securitySchemes = components.getSecuritySchemes(); @@ -85,7 +92,6 @@ public ValidationResult validate(OpenAPI specification) { validationResult.consume(parameterValidations.validate(parameter)); }); - return validationResult; } } diff --git a/modules/openapi-generator/src/main/java/org/openapitools/codegen/validations/OpenApiParameterValidations.java b/modules/openapi-generator/src/main/java/org/openapitools/codegen/validations/oas/OpenApiParameterValidations.java similarity index 65% rename from modules/openapi-generator/src/main/java/org/openapitools/codegen/validations/OpenApiParameterValidations.java rename to modules/openapi-generator/src/main/java/org/openapitools/codegen/validations/oas/OpenApiParameterValidations.java index a61e1c0f895f..909f55da8745 100644 --- a/modules/openapi-generator/src/main/java/org/openapitools/codegen/validations/OpenApiParameterValidations.java +++ b/modules/openapi-generator/src/main/java/org/openapitools/codegen/validations/oas/OpenApiParameterValidations.java @@ -1,4 +1,4 @@ -package org.openapitools.codegen.validations; +package org.openapitools.codegen.validations.oas; import io.swagger.v3.oas.models.parameters.HeaderParameter; import io.swagger.v3.oas.models.parameters.Parameter; @@ -8,11 +8,14 @@ import java.util.ArrayList; -public class OpenApiParameterValidations extends GenericValidator { +/** + * A standalone instance for evaluating rules and recommendations related to OAS {@link Parameter} + */ +class OpenApiParameterValidations extends GenericValidator { OpenApiParameterValidations(RuleConfiguration ruleConfiguration) { super(new ArrayList<>()); if (ruleConfiguration.isEnableRecommendations()) { - if (ruleConfiguration.isEnableApacheNginxUnderscoreSuggestion()) { + if (ruleConfiguration.isEnableApacheNginxUnderscoreRecommendation()) { rules.add(ValidationRule.warn( ValidationConstants.ApacheNginxUnderscoreDescription, ValidationConstants.ApacheNginxUnderscoreFailureMessage, @@ -22,6 +25,13 @@ public class OpenApiParameterValidations extends GenericValidator { } } + /** + * Apache and Nginx default to legacy CGI behavior in which header with underscore are ignored. Raise this for awareness to the user. + * + * @param parameter Any spec doc parameter. The method will handle {@link HeaderParameter} evaluation. + * + * @return true if the header has an underscore (e.g. 'api_key') + */ private static boolean isApacheNginxHeaderSuggestion(Parameter parameter) { boolean result = false; if (parameter instanceof HeaderParameter) { diff --git a/modules/openapi-generator/src/main/java/org/openapitools/codegen/validations/OpenApiSchemaValidations.java b/modules/openapi-generator/src/main/java/org/openapitools/codegen/validations/oas/OpenApiSchemaValidations.java similarity index 89% rename from modules/openapi-generator/src/main/java/org/openapitools/codegen/validations/OpenApiSchemaValidations.java rename to modules/openapi-generator/src/main/java/org/openapitools/codegen/validations/oas/OpenApiSchemaValidations.java index 4cde0ba3e069..6678aefb58cd 100644 --- a/modules/openapi-generator/src/main/java/org/openapitools/codegen/validations/OpenApiSchemaValidations.java +++ b/modules/openapi-generator/src/main/java/org/openapitools/codegen/validations/oas/OpenApiSchemaValidations.java @@ -1,4 +1,4 @@ -package org.openapitools.codegen.validations; +package org.openapitools.codegen.validations.oas; import io.swagger.v3.oas.models.media.ComposedSchema; import io.swagger.v3.oas.models.media.Schema; @@ -7,11 +7,14 @@ import java.util.ArrayList; -public class OpenApiSchemaValidations extends GenericValidator { +/** + * A standalone instance for evaluating rules and recommendations related to OAS {@link Schema} + */ +class OpenApiSchemaValidations extends GenericValidator { OpenApiSchemaValidations(RuleConfiguration ruleConfiguration) { super(new ArrayList<>()); if (ruleConfiguration.isEnableRecommendations()) { - if (ruleConfiguration.isEnableOneOfWithPropertiesSuggestion()) { + if (ruleConfiguration.isEnableOneOfWithPropertiesRecommendation()) { rules.add(ValidationRule.warn( "Schema defines properties alongside oneOf.", "Schemas defining properties and oneOf are not clearly defined in the OpenAPI Specification. While our tooling supports this, it may cause issues with other tools.", diff --git a/modules/openapi-generator/src/main/java/org/openapitools/codegen/validations/OpenApiSecuritySchemeValidations.java b/modules/openapi-generator/src/main/java/org/openapitools/codegen/validations/oas/OpenApiSecuritySchemeValidations.java similarity index 56% rename from modules/openapi-generator/src/main/java/org/openapitools/codegen/validations/OpenApiSecuritySchemeValidations.java rename to modules/openapi-generator/src/main/java/org/openapitools/codegen/validations/oas/OpenApiSecuritySchemeValidations.java index b03c6ce633a1..65798433b654 100644 --- a/modules/openapi-generator/src/main/java/org/openapitools/codegen/validations/OpenApiSecuritySchemeValidations.java +++ b/modules/openapi-generator/src/main/java/org/openapitools/codegen/validations/oas/OpenApiSecuritySchemeValidations.java @@ -1,4 +1,4 @@ -package org.openapitools.codegen.validations; +package org.openapitools.codegen.validations.oas; import io.swagger.v3.oas.models.security.SecurityScheme; import org.apache.commons.lang3.StringUtils; @@ -7,11 +7,14 @@ import java.util.ArrayList; -public class OpenApiSecuritySchemeValidations extends GenericValidator { +/** + * A standalone instance for evaluating rules and recommendations related to OAS {@link SecurityScheme} + */ +class OpenApiSecuritySchemeValidations extends GenericValidator { OpenApiSecuritySchemeValidations(RuleConfiguration ruleConfiguration) { super(new ArrayList<>()); if (ruleConfiguration.isEnableRecommendations()) { - if (ruleConfiguration.isEnableApacheNginxUnderscoreSuggestion()) { + if (ruleConfiguration.isEnableApacheNginxUnderscoreRecommendation()) { rules.add(ValidationRule.warn( ValidationConstants.ApacheNginxUnderscoreDescription, ValidationConstants.ApacheNginxUnderscoreFailureMessage, @@ -21,9 +24,16 @@ public class OpenApiSecuritySchemeValidations extends GenericValidatortrue if the header has an underscore (e.g. 'api_key') + */ private static boolean isApacheNginxHeaderSuggestion(SecurityScheme securityScheme) { return securityScheme != null && - SecurityScheme.Type.APIKEY.equals(securityScheme.getType()) && + securityScheme.getIn() == SecurityScheme.In.HEADER && StringUtils.contains(securityScheme.getName(), '_'); } } diff --git a/modules/openapi-generator/src/main/java/org/openapitools/codegen/validations/oas/RuleConfiguration.java b/modules/openapi-generator/src/main/java/org/openapitools/codegen/validations/oas/RuleConfiguration.java new file mode 100644 index 000000000000..5403a7550ddb --- /dev/null +++ b/modules/openapi-generator/src/main/java/org/openapitools/codegen/validations/oas/RuleConfiguration.java @@ -0,0 +1,115 @@ +package org.openapitools.codegen.validations.oas; + +/** + * Allows for configuration of validation rules which will be applied to a specification. + */ +@SuppressWarnings({"WeakerAccess", "unused"}) +public class RuleConfiguration { + private static String propertyPrefix = "openapi.generator.rule"; + private boolean enableRecommendations = defaultedBoolean(propertyPrefix + ".recommendations", true); + private boolean enableApacheNginxUnderscoreRecommendation = defaultedBoolean(propertyPrefix + ".apache-nginx-underscore", true); + private boolean enableOneOfWithPropertiesRecommendation = defaultedBoolean(propertyPrefix + ".oneof-properties-ambiguity", true); + private boolean enableUnusedSchemasRecommendation = defaultedBoolean(propertyPrefix + ".unused-schemas", true); + + @SuppressWarnings("SameParameterValue") + private static boolean defaultedBoolean(String key, boolean defaultValue) { + String property = System.getProperty(key); + if (property == null) return defaultValue; + return Boolean.parseBoolean(property); + } + + /** + * Gets whether we will raise awareness that header parameters with underscore may be ignored in Apache or Nginx by default. + * For more details, see https://stackoverflow.com/a/22856867/151445. + * + * @return true if enabled, false if disabled + */ + public boolean isEnableApacheNginxUnderscoreRecommendation() { + return enableApacheNginxUnderscoreRecommendation; + } + + /** + * Enable or Disable the recommendation check for Apache/Nginx potentially ignoring header with underscore by default. + * + * For more details, see {@link RuleConfiguration#isEnableApacheNginxUnderscoreRecommendation()} + * + * @param enableApacheNginxUnderscoreRecommendation true to enable, false to disable + */ + public void setEnableApacheNginxUnderscoreRecommendation(boolean enableApacheNginxUnderscoreRecommendation) { + this.enableApacheNginxUnderscoreRecommendation = enableApacheNginxUnderscoreRecommendation; + } + + /** + * Gets whether the recommendation check for oneOf with sibling properties exists. + * + * JSON Schema defines oneOf as a validation property which can be applied to any schema. + *

+ * OpenAPI Specification is a variant of JSON Schema for which oneOf is defined as: + * "Inline or referenced schema MUST be of a Schema Object and not a standard JSON Schema." + *

+ * 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 + * properties on the schema defining oneOf relationships may not be intentional in the OpenAPI Specification. + * + * @return true if enabled, false if disabled + */ + public boolean isEnableOneOfWithPropertiesRecommendation() { + return enableOneOfWithPropertiesRecommendation; + } + + /** + * Enable or Disable the recommendation check for schemas containing properties and oneOf definitions. + * + * For more details, see {@link RuleConfiguration#isEnableOneOfWithPropertiesRecommendation()} + * + * @param enableOneOfWithPropertiesRecommendation true to enable, false to disable + */ + public void setEnableOneOfWithPropertiesRecommendation(boolean enableOneOfWithPropertiesRecommendation) { + this.enableOneOfWithPropertiesRecommendation = enableOneOfWithPropertiesRecommendation; + } + + /** + * Get whether recommendations are enabled. + * + * @return true if enabled, false if disabled + */ + public boolean isEnableRecommendations() { + return enableRecommendations; + } + + /** + * Enable or Disable recommendations. Recommendations are either informational or warning level type validations + * which are raised to communicate issues to the user which they may not be aware of, or for which support in the + * tooling/spec may not be clearly defined. + * + * @param enableRecommendations true to enable, false to disable + */ + public void setEnableRecommendations(boolean enableRecommendations) { + this.enableRecommendations = enableRecommendations; + } + + /** + * Gets whether the recommendation to check for unused schemas is enabled. + * + * While the tooling may or may not support generation of models representing unused schemas, we take the stance that + * a schema which is defined but not referenced in an operation or by some schema bound to an operation may be a good + * indicator of a programming error. We surface this information to the user in case the orphaned schema(s) are not + * intentional. + * + * @return true if enabled, false if disabled + */ + public boolean isEnableUnusedSchemasRecommendation() { + return enableUnusedSchemasRecommendation; + } + + /** + * Enable or Disable the recommendation check for unused schemas. + * + * For more details, see {@link RuleConfiguration#isEnableUnusedSchemasRecommendation()} + * + * @param enableUnusedSchemasRecommendation true to enable, false to disable + */ + public void setEnableUnusedSchemasRecommendation(boolean enableUnusedSchemasRecommendation) { + this.enableUnusedSchemasRecommendation = enableUnusedSchemasRecommendation; + } +} diff --git a/modules/openapi-generator/src/main/java/org/openapitools/codegen/validations/ValidationConstants.java b/modules/openapi-generator/src/main/java/org/openapitools/codegen/validations/oas/ValidationConstants.java similarity index 88% rename from modules/openapi-generator/src/main/java/org/openapitools/codegen/validations/ValidationConstants.java rename to modules/openapi-generator/src/main/java/org/openapitools/codegen/validations/oas/ValidationConstants.java index 69fef91a5435..9571f1bedcdf 100644 --- a/modules/openapi-generator/src/main/java/org/openapitools/codegen/validations/ValidationConstants.java +++ b/modules/openapi-generator/src/main/java/org/openapitools/codegen/validations/oas/ValidationConstants.java @@ -1,4 +1,4 @@ -package org.openapitools.codegen.validations; +package org.openapitools.codegen.validations.oas; final class ValidationConstants { static String ApacheNginxUnderscoreDescription = "Apache and Nginx may fail on headers keys with underscore!"; From 018ace788a4290cd5459c990430011903b7d5c27 Mon Sep 17 00:00:00 2001 From: Jim Schubert Date: Sun, 12 Jan 2020 11:23:18 -0500 Subject: [PATCH 3/6] Refactor and test recommendation validations --- .../oas/OpenApiParameterValidations.java | 23 ++-- .../oas/OpenApiSchemaValidations.java | 11 +- .../oas/OpenApiSecuritySchemeValidations.java | 12 +- .../oas/OpenApiParameterValidationsTest.java | 93 +++++++++++++ .../oas/OpenApiSchemaValidationsTest.java | 128 ++++++++++++++++++ .../OpenApiSecuritySchemeValidationsTest.java | 87 ++++++++++++ 6 files changed, 332 insertions(+), 22 deletions(-) create mode 100644 modules/openapi-generator/src/test/java/org/openapitools/codegen/validations/oas/OpenApiParameterValidationsTest.java create mode 100644 modules/openapi-generator/src/test/java/org/openapitools/codegen/validations/oas/OpenApiSchemaValidationsTest.java create mode 100644 modules/openapi-generator/src/test/java/org/openapitools/codegen/validations/oas/OpenApiSecuritySchemeValidationsTest.java diff --git a/modules/openapi-generator/src/main/java/org/openapitools/codegen/validations/oas/OpenApiParameterValidations.java b/modules/openapi-generator/src/main/java/org/openapitools/codegen/validations/oas/OpenApiParameterValidations.java index 909f55da8745..fafeeb8cf331 100644 --- a/modules/openapi-generator/src/main/java/org/openapitools/codegen/validations/oas/OpenApiParameterValidations.java +++ b/modules/openapi-generator/src/main/java/org/openapitools/codegen/validations/oas/OpenApiParameterValidations.java @@ -19,7 +19,7 @@ class OpenApiParameterValidations extends GenericValidator { rules.add(ValidationRule.warn( ValidationConstants.ApacheNginxUnderscoreDescription, ValidationConstants.ApacheNginxUnderscoreFailureMessage, - OpenApiParameterValidations::isApacheNginxHeaderSuggestion + OpenApiParameterValidations::passesApacheNginxHeaderCheck )); } } @@ -30,17 +30,18 @@ class OpenApiParameterValidations extends GenericValidator { * * @param parameter Any spec doc parameter. The method will handle {@link HeaderParameter} evaluation. * - * @return true if the header has an underscore (e.g. 'api_key') + * @return true if the check succeeds (header does not have an underscore, e.g. 'api_key') */ - private static boolean isApacheNginxHeaderSuggestion(Parameter parameter) { - boolean result = false; - if (parameter instanceof HeaderParameter) { - HeaderParameter headerParameter = (HeaderParameter) parameter; - String headerName = headerParameter.getName(); - if (StringUtils.isNotEmpty(headerName) && StringUtils.contains(headerName, '_')) { - result = true; - } + private static boolean passesApacheNginxHeaderCheck(Parameter parameter) { + if (parameter == null || !parameter.getIn().equals("header")) return true; + + boolean valid = true; + + String headerName = parameter.getName(); + if (StringUtils.isNotEmpty(headerName) && StringUtils.contains(headerName, '_')) { + valid = false; } - return result; + + return valid; } } diff --git a/modules/openapi-generator/src/main/java/org/openapitools/codegen/validations/oas/OpenApiSchemaValidations.java b/modules/openapi-generator/src/main/java/org/openapitools/codegen/validations/oas/OpenApiSchemaValidations.java index 6678aefb58cd..9d45fb0c05aa 100644 --- a/modules/openapi-generator/src/main/java/org/openapitools/codegen/validations/oas/OpenApiSchemaValidations.java +++ b/modules/openapi-generator/src/main/java/org/openapitools/codegen/validations/oas/OpenApiSchemaValidations.java @@ -18,7 +18,7 @@ class OpenApiSchemaValidations extends GenericValidator { rules.add(ValidationRule.warn( "Schema defines properties alongside oneOf.", "Schemas defining properties and oneOf are not clearly defined in the OpenAPI Specification. While our tooling supports this, it may cause issues with other tools.", - OpenApiSchemaValidations::hasOneOfWithProperties + OpenApiSchemaValidations::validOneOfWithProperties )); } } @@ -37,8 +37,8 @@ class OpenApiSchemaValidations extends GenericValidator { * @param schema An input schema, regardless of the type of schema * @return true if the schema has oneOf defined along with properties other than discriminator. */ - private static boolean hasOneOfWithProperties(Schema schema) { - boolean result = false; + private static boolean validOneOfWithProperties(Schema schema) { + boolean valid = true; if (schema instanceof ComposedSchema) { final ComposedSchema composed = (ComposedSchema) schema; // check for loosely defined oneOf extension requirements. @@ -46,10 +46,11 @@ private static boolean hasOneOfWithProperties(Schema schema) { // 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'. if (composed.getOneOf() != null && composed.getOneOf().size() > 0) { if (composed.getProperties() != null && composed.getProperties().size() >= 1 && composed.getProperties().get("discriminator") == null) { - result = true; + // not necessarily "invalid" here, but we trigger the recommendation which requires the method to return false. + valid = false; } } } - return result; + return valid; } } diff --git a/modules/openapi-generator/src/main/java/org/openapitools/codegen/validations/oas/OpenApiSecuritySchemeValidations.java b/modules/openapi-generator/src/main/java/org/openapitools/codegen/validations/oas/OpenApiSecuritySchemeValidations.java index 65798433b654..20e56c6df602 100644 --- a/modules/openapi-generator/src/main/java/org/openapitools/codegen/validations/oas/OpenApiSecuritySchemeValidations.java +++ b/modules/openapi-generator/src/main/java/org/openapitools/codegen/validations/oas/OpenApiSecuritySchemeValidations.java @@ -18,7 +18,7 @@ class OpenApiSecuritySchemeValidations extends GenericValidator rules.add(ValidationRule.warn( ValidationConstants.ApacheNginxUnderscoreDescription, ValidationConstants.ApacheNginxUnderscoreFailureMessage, - OpenApiSecuritySchemeValidations::isApacheNginxHeaderSuggestion + OpenApiSecuritySchemeValidations::passesApacheNginxHeaderCheck )); } } @@ -29,11 +29,11 @@ class OpenApiSecuritySchemeValidations extends GenericValidator * * @param securityScheme Security schemes are often used as header parameters (e.g. APIKEY). * - * @return true if the header has an underscore (e.g. 'api_key') + * @return true if the check succeeds (header does not have an underscore, e.g. 'api_key') */ - private static boolean isApacheNginxHeaderSuggestion(SecurityScheme securityScheme) { - return securityScheme != null && - securityScheme.getIn() == SecurityScheme.In.HEADER && - StringUtils.contains(securityScheme.getName(), '_'); + private static boolean passesApacheNginxHeaderCheck(SecurityScheme securityScheme) { + if (securityScheme == null || securityScheme.getIn() != SecurityScheme.In.HEADER) return true; + + return !StringUtils.contains(securityScheme.getName(), '_'); } } diff --git a/modules/openapi-generator/src/test/java/org/openapitools/codegen/validations/oas/OpenApiParameterValidationsTest.java b/modules/openapi-generator/src/test/java/org/openapitools/codegen/validations/oas/OpenApiParameterValidationsTest.java new file mode 100644 index 000000000000..0b5c7807f9c4 --- /dev/null +++ b/modules/openapi-generator/src/test/java/org/openapitools/codegen/validations/oas/OpenApiParameterValidationsTest.java @@ -0,0 +1,93 @@ +package org.openapitools.codegen.validations.oas; + +import io.swagger.v3.oas.models.parameters.Parameter; +import org.openapitools.codegen.validation.Invalid; +import org.openapitools.codegen.validation.ValidationResult; +import org.testng.Assert; +import org.testng.annotations.DataProvider; +import org.testng.annotations.Test; + +import java.util.List; +import java.util.stream.Collectors; + +public class OpenApiParameterValidationsTest { + + @Test(dataProvider = "apacheNginxRecommendationExpectations", description = "disable apache nginx via turning off recommendations") + public void testApacheNginxWithDisabledRecommendations(String in, String key, boolean matches) { + RuleConfiguration config = new RuleConfiguration(); + config.setEnableRecommendations(false); + OpenApiParameterValidations validator = new OpenApiParameterValidations(config); + + Parameter parameter = new Parameter(); + parameter.setIn(in); + parameter.setName(key); + + ValidationResult result = validator.validate(parameter); + Assert.assertNotNull(result.getWarnings()); + + List warnings = result.getWarnings().stream() + .filter(invalid -> ValidationConstants.ApacheNginxUnderscoreFailureMessage.equals(invalid.getMessage())) + .collect(Collectors.toList()); + + Assert.assertNotNull(warnings); + Assert.assertEquals(warnings.size(), 0, "Expected recommendations to be disabled completely."); + } + + @Test(dataProvider = "apacheNginxRecommendationExpectations", description = "disable apache nginx via turning off recommendations") + public void testApacheNginxWithDisabledRule(String in, String key, boolean matches) { + RuleConfiguration config = new RuleConfiguration(); + config.setEnableApacheNginxUnderscoreRecommendation(false); + OpenApiParameterValidations validator = new OpenApiParameterValidations(config); + + Parameter parameter = new Parameter(); + parameter.setIn(in); + parameter.setName(key); + + ValidationResult result = validator.validate(parameter); + Assert.assertNotNull(result.getWarnings()); + + List warnings = result.getWarnings().stream() + .filter(invalid -> ValidationConstants.ApacheNginxUnderscoreFailureMessage.equals(invalid.getMessage())) + .collect(Collectors.toList()); + + Assert.assertNotNull(warnings); + Assert.assertEquals(warnings.size(), 0, "Expected rule to be disabled."); + } + + @Test(dataProvider = "apacheNginxRecommendationExpectations", description = "default apache nginx recommendation") + public void testDefaultRecommendationApacheNginx(String in, String key, boolean matches) { + RuleConfiguration config = new RuleConfiguration(); + config.setEnableRecommendations(true); + OpenApiParameterValidations validator = new OpenApiParameterValidations(config); + + Parameter parameter = new Parameter(); + parameter.setIn(in); + parameter.setName(key); + + ValidationResult result = validator.validate(parameter); + Assert.assertNotNull(result.getWarnings()); + + List warnings = result.getWarnings().stream() + .filter(invalid -> ValidationConstants.ApacheNginxUnderscoreFailureMessage.equals(invalid.getMessage())) + .collect(Collectors.toList()); + + Assert.assertNotNull(warnings); + if (matches) { + Assert.assertEquals(warnings.size(), 1, "Expected " + key + " to match recommendation."); + } else { + Assert.assertEquals(warnings.size(), 0, "Expected " + key + " not to match recommendation."); + } + } + + @DataProvider(name = "apacheNginxRecommendationExpectations") + public Object[][] apacheNginxRecommendationExpectations() { + return new Object[][]{ + {"header", "api_key", true}, + {"header", "apikey", false}, + {"cookie", "api_key", false}, + {"cookie", "apikey", false}, + {"query", "api_key", false}, + {"query", "apikey", false} + }; + } +} \ No newline at end of file diff --git a/modules/openapi-generator/src/test/java/org/openapitools/codegen/validations/oas/OpenApiSchemaValidationsTest.java b/modules/openapi-generator/src/test/java/org/openapitools/codegen/validations/oas/OpenApiSchemaValidationsTest.java new file mode 100644 index 000000000000..2f8ad8ce5c98 --- /dev/null +++ b/modules/openapi-generator/src/test/java/org/openapitools/codegen/validations/oas/OpenApiSchemaValidationsTest.java @@ -0,0 +1,128 @@ +package org.openapitools.codegen.validations.oas; + +import io.swagger.v3.oas.models.media.*; +import org.openapitools.codegen.validation.Invalid; +import org.openapitools.codegen.validation.ValidationResult; +import org.testng.Assert; +import org.testng.annotations.DataProvider; +import org.testng.annotations.Test; + +import java.util.Arrays; +import java.util.List; +import java.util.stream.Collectors; + +public class OpenApiSchemaValidationsTest { + @Test(dataProvider = "apacheNginxRecommendationExpectations", description = "default oneOf with sibling properties recommendation") + public void testDefaultRecommendationOneOfWithSiblingProperties(Schema schema, boolean matches) { + RuleConfiguration config = new RuleConfiguration(); + config.setEnableRecommendations(true); + OpenApiSchemaValidations validator = new OpenApiSchemaValidations(config); + + ValidationResult result = validator.validate(schema); + Assert.assertNotNull(result.getWarnings()); + + List warnings = result.getWarnings().stream() + .filter(invalid -> "Schema defines properties alongside oneOf." .equals(invalid.getRule().getDescription())) + .collect(Collectors.toList()); + + Assert.assertNotNull(warnings); + if (matches) { + Assert.assertEquals(warnings.size(), 1, "Expected to match recommendation."); + } else { + Assert.assertEquals(warnings.size(), 0, "Expected not to match recommendation."); + } + } + + @Test(dataProvider = "apacheNginxRecommendationExpectations", description = "disable oneOf with sibling properties recommendation via turning off recommendations") + public void testOneOfWithSiblingPropertiesDisabledRecommendations(Schema schema, boolean matches) { + RuleConfiguration config = new RuleConfiguration(); + config.setEnableRecommendations(false); + OpenApiSchemaValidations validator = new OpenApiSchemaValidations(config); + + ValidationResult result = validator.validate(schema); + Assert.assertNotNull(result.getWarnings()); + + List warnings = result.getWarnings().stream() + .filter(invalid -> "Schema defines properties alongside oneOf." .equals(invalid.getRule().getDescription())) + .collect(Collectors.toList()); + + Assert.assertNotNull(warnings); + Assert.assertEquals(warnings.size(), 0, "Expected recommendations to be disabled completely."); + } + + @Test(dataProvider = "apacheNginxRecommendationExpectations", description = "disable oneOf with sibling properties recommendation via turning off rule") + public void testOneOfWithSiblingPropertiesDisabledRule(Schema schema, boolean matches) { + RuleConfiguration config = new RuleConfiguration(); + config.setEnableOneOfWithPropertiesRecommendation(false); + OpenApiSchemaValidations validator = new OpenApiSchemaValidations(config); + + ValidationResult result = validator.validate(schema); + Assert.assertNotNull(result.getWarnings()); + + List warnings = result.getWarnings().stream() + .filter(invalid -> "Schema defines properties alongside oneOf." .equals(invalid.getRule().getDescription())) + .collect(Collectors.toList()); + + Assert.assertNotNull(warnings); + Assert.assertEquals(warnings.size(), 0, "Expected rule to be disabled."); + } + + @DataProvider(name = "apacheNginxRecommendationExpectations") + public Object[][] apacheNginxRecommendationExpectations() { + return new Object[][]{ + {getOneOfSample(true), true}, + {getOneOfSample(false), false}, + {getAllOfSample(true), false}, + {getAllOfSample(false), false}, + {getAnyOfSample(true), false}, + {getAnyOfSample(false), false}, + {new StringSchema(), false}, + {new MapSchema(), false}, + {new ArraySchema(), false}, + {new ObjectSchema(), false} + }; + } + + private ComposedSchema getOneOfSample(boolean withProperties) { + ComposedSchema schema = new ComposedSchema().oneOf(Arrays.asList( + new StringSchema(), + new IntegerSchema().format("int64")) + ); + + if (withProperties) { + schema.addProperties("other", new ArraySchema() + .items(new Schema().$ref("#/definitions/Other"))); + } + + return schema; + } + + private ComposedSchema getAllOfSample(boolean withProperties) { + // This doesn't matter if it's realistic; it's a structural check + ComposedSchema schema = new ComposedSchema().allOf(Arrays.asList( + new StringSchema(), + new IntegerSchema().format("int64")) + ); + + if (withProperties) { + schema.addProperties("other", new ArraySchema() + .items(new Schema().$ref("#/definitions/Other"))); + } + + return schema; + } + + private ComposedSchema getAnyOfSample(boolean withProperties) { + ComposedSchema schema = new ComposedSchema().anyOf(Arrays.asList( + new StringSchema(), + new IntegerSchema().format("int64")) + ); + + if (withProperties) { + schema.addProperties("other", new ArraySchema() + .items(new Schema().$ref("#/definitions/Other"))); + } + + return schema; + } +} \ No newline at end of file diff --git a/modules/openapi-generator/src/test/java/org/openapitools/codegen/validations/oas/OpenApiSecuritySchemeValidationsTest.java b/modules/openapi-generator/src/test/java/org/openapitools/codegen/validations/oas/OpenApiSecuritySchemeValidationsTest.java new file mode 100644 index 000000000000..aae32dd30126 --- /dev/null +++ b/modules/openapi-generator/src/test/java/org/openapitools/codegen/validations/oas/OpenApiSecuritySchemeValidationsTest.java @@ -0,0 +1,87 @@ +package org.openapitools.codegen.validations.oas; + +import io.swagger.v3.oas.models.security.SecurityScheme; +import org.openapitools.codegen.validation.Invalid; +import org.openapitools.codegen.validation.ValidationResult; +import org.testng.Assert; +import org.testng.annotations.DataProvider; +import org.testng.annotations.Test; + +import java.util.List; +import java.util.stream.Collectors; + +public class OpenApiSecuritySchemeValidationsTest { + + @Test(dataProvider = "apacheNginxRecommendationExpectations", description = "disable apache nginx via turning off recommendations") + public void testApacheNginxWithDisabledRecommendations(SecurityScheme.In in, String key, boolean matches) { + RuleConfiguration config = new RuleConfiguration(); + config.setEnableRecommendations(false); + OpenApiSecuritySchemeValidations validator = new OpenApiSecuritySchemeValidations(config); + + SecurityScheme securityScheme = new SecurityScheme().in(in).name(key); + + ValidationResult result = validator.validate(securityScheme); + Assert.assertNotNull(result.getWarnings()); + + List warnings = result.getWarnings().stream() + .filter(invalid -> ValidationConstants.ApacheNginxUnderscoreFailureMessage.equals(invalid.getMessage())) + .collect(Collectors.toList()); + + Assert.assertNotNull(warnings); + Assert.assertEquals(warnings.size(), 0, "Expected recommendations to be disabled completely."); + } + + @Test(dataProvider = "apacheNginxRecommendationExpectations", description = "disable apache nginx via turning off rule") + public void testApacheNginxWithDisabledRule(SecurityScheme.In in, String key, boolean matches) { + RuleConfiguration config = new RuleConfiguration(); + config.setEnableApacheNginxUnderscoreRecommendation(false); + OpenApiSecuritySchemeValidations validator = new OpenApiSecuritySchemeValidations(config); + + SecurityScheme securityScheme = new SecurityScheme().in(in).name(key); + + ValidationResult result = validator.validate(securityScheme); + Assert.assertNotNull(result.getWarnings()); + + List warnings = result.getWarnings().stream() + .filter(invalid -> ValidationConstants.ApacheNginxUnderscoreFailureMessage.equals(invalid.getMessage())) + .collect(Collectors.toList()); + + Assert.assertNotNull(warnings); + Assert.assertEquals(warnings.size(), 0, "Expected rule to be disabled."); + } + + @Test(dataProvider = "apacheNginxRecommendationExpectations", description = "default apache nginx recommendation") + public void testDefaultRecommendationApacheNginx(SecurityScheme.In in, String key, boolean matches) { + RuleConfiguration config = new RuleConfiguration(); + config.setEnableRecommendations(true); + OpenApiSecuritySchemeValidations validator = new OpenApiSecuritySchemeValidations(config); + + SecurityScheme securityScheme = new SecurityScheme().in(in).name(key); + + ValidationResult result = validator.validate(securityScheme); + Assert.assertNotNull(result.getWarnings()); + + List warnings = result.getWarnings().stream() + .filter(invalid -> ValidationConstants.ApacheNginxUnderscoreFailureMessage.equals(invalid.getMessage())) + .collect(Collectors.toList()); + + Assert.assertNotNull(warnings); + if (matches) { + Assert.assertEquals(warnings.size(), 1, "Expected " + key + " to match recommendation."); + } else { + Assert.assertEquals(warnings.size(), 0, "Expected " + key + " not to match recommendation."); + } + } + + @DataProvider(name = "apacheNginxRecommendationExpectations") + public Object[][] apacheNginxRecommendationExpectations() { + return new Object[][]{ + {SecurityScheme.In.HEADER, "api_key", true}, + {SecurityScheme.In.HEADER, "apikey", false}, + {SecurityScheme.In.COOKIE, "api_key", false}, + {SecurityScheme.In.COOKIE, "apikey", false}, + {SecurityScheme.In.QUERY, "api_key", false}, + {SecurityScheme.In.COOKIE, "apikey", false} + }; + } +} \ No newline at end of file From 2a5191fe7d14ebbff8c3c6de71730b4072aacf96 Mon Sep 17 00:00:00 2001 From: Jim Schubert Date: Sun, 12 Jan 2020 15:25:29 -0500 Subject: [PATCH 4/6] Refactor validation function signatures to return explicit state rather than boolean --- .../codegen/validation/GenericValidator.java | 7 +- .../codegen/validation/Invalid.java | 17 ++++ .../codegen/validation/Validated.java | 12 +++ .../codegen/validation/ValidationResult.java | 1 - .../codegen/validation/ValidationRule.java | 85 ++++++++++++++++--- .../validation/GenericValidatorTest.java | 32 +++---- .../validation/ValidationRuleTest.java | 22 ++--- .../validations/oas/OpenApiEvaluator.java | 6 +- .../oas/OpenApiParameterValidations.java | 17 ++-- .../oas/OpenApiSchemaValidations.java | 13 +-- .../oas/OpenApiSecuritySchemeValidations.java | 16 +++- 11 files changed, 164 insertions(+), 64 deletions(-) diff --git a/modules/openapi-generator-core/src/main/java/org/openapitools/codegen/validation/GenericValidator.java b/modules/openapi-generator-core/src/main/java/org/openapitools/codegen/validation/GenericValidator.java index 3e52ecbdba7d..00e4e00e463d 100644 --- a/modules/openapi-generator-core/src/main/java/org/openapitools/codegen/validation/GenericValidator.java +++ b/modules/openapi-generator-core/src/main/java/org/openapitools/codegen/validation/GenericValidator.java @@ -23,7 +23,6 @@ * * @param The type of object being evaluated. */ -@SuppressWarnings({"WeakerAccess"}) public class GenericValidator implements Validator { protected List rules; @@ -48,11 +47,11 @@ public ValidationResult validate(TInput input) { ValidationResult result = new ValidationResult(); if (rules != null) { rules.forEach(it -> { - boolean passes = it.evaluate(input); - if (passes) { + ValidationRule.Result attempt = it.evaluate(input); + if (attempt.passed()) { result.addResult(Validated.valid(it)); } else { - result.addResult(Validated.invalid(it, it.getFailureMessage())); + result.addResult(Validated.invalid(it, it.getFailureMessage(), attempt.getDetails())); } }); } diff --git a/modules/openapi-generator-core/src/main/java/org/openapitools/codegen/validation/Invalid.java b/modules/openapi-generator-core/src/main/java/org/openapitools/codegen/validation/Invalid.java index 67c8e6ff642f..131e7a73465d 100644 --- a/modules/openapi-generator-core/src/main/java/org/openapitools/codegen/validation/Invalid.java +++ b/modules/openapi-generator-core/src/main/java/org/openapitools/codegen/validation/Invalid.java @@ -23,6 +23,7 @@ public final class Invalid extends Validated { private String message; private ValidationRule rule; + private String details; /** * Constructs a new {@link Invalid} instance. @@ -35,6 +36,22 @@ public final class Invalid extends Validated { this.message = message; } + /** + * Constructs a new {@link Invalid} instance. + * + * @param rule The rule which was evaluated and resulted in this state. + * @param message The message to be displayed for this invalid state. + * @param details Additional contextual details related to the invalid state. + */ + public Invalid(ValidationRule rule, String message, String details) { + this(rule, message); + this.details = details; + } + + public String getDetails() { + return details; + } + @Override public String getMessage() { return message; diff --git a/modules/openapi-generator-core/src/main/java/org/openapitools/codegen/validation/Validated.java b/modules/openapi-generator-core/src/main/java/org/openapitools/codegen/validation/Validated.java index 2df3b9c3ae3c..05f9df833e4b 100644 --- a/modules/openapi-generator-core/src/main/java/org/openapitools/codegen/validation/Validated.java +++ b/modules/openapi-generator-core/src/main/java/org/openapitools/codegen/validation/Validated.java @@ -54,6 +54,18 @@ boolean isValid() { public static Validated invalid(ValidationRule rule, String message) { return new Invalid(rule, message); } + /** + * Creates an instance of an {@link Invalid} validation state. + * + * @param rule The rule which was evaluated. + * @param message The message to display to a user. + * @param details Additional contextual details related to the invalid state. + * + * @return A {@link Validated} instance representing an invalid state according to the rule. + */ + public static Validated invalid(ValidationRule rule, String message, String details) { + return new Invalid(rule, message, details); + } /** * Creates an instance of an {@link Valid} validation state. diff --git a/modules/openapi-generator-core/src/main/java/org/openapitools/codegen/validation/ValidationResult.java b/modules/openapi-generator-core/src/main/java/org/openapitools/codegen/validation/ValidationResult.java index 0b3f451d4d2c..628a11a225c0 100644 --- a/modules/openapi-generator-core/src/main/java/org/openapitools/codegen/validation/ValidationResult.java +++ b/modules/openapi-generator-core/src/main/java/org/openapitools/codegen/validation/ValidationResult.java @@ -24,7 +24,6 @@ /** * Encapsulates details about the result of a validation test. */ -@SuppressWarnings("WeakerAccess") public final class ValidationResult { private final List validations; diff --git a/modules/openapi-generator-core/src/main/java/org/openapitools/codegen/validation/ValidationRule.java b/modules/openapi-generator-core/src/main/java/org/openapitools/codegen/validation/ValidationRule.java index e220e0482cf6..8d5818b2887b 100644 --- a/modules/openapi-generator-core/src/main/java/org/openapitools/codegen/validation/ValidationRule.java +++ b/modules/openapi-generator-core/src/main/java/org/openapitools/codegen/validation/ValidationRule.java @@ -26,7 +26,7 @@ public class ValidationRule { private Severity severity; private String description; private String failureMessage; - private Function test; + private Function test; /** * Constructs a new instance of {@link ValidationRule} @@ -37,7 +37,7 @@ public class ValidationRule { * @param test The test condition to be applied as a part of this rule, when this function returns true, * the evaluated instance will be considered "valid" according to this rule. */ - ValidationRule(Severity severity, String description, String failureMessage, Function test) { + ValidationRule(Severity severity, String description, String failureMessage, Function test) { this.severity = severity; this.description = description; this.failureMessage = failureMessage; @@ -60,7 +60,7 @@ public String getFailureMessage() { * * @return true if the object state is valid according to this rule, otherwise false. */ - public boolean evaluate(Object input) { + public Result evaluate(Object input) { return test.apply(input); } @@ -90,7 +90,7 @@ public String getDescription() { * @return An "empty" rule. */ static ValidationRule empty() { - return new ValidationRule(Severity.ERROR, "empty", "failure message", (i) -> false); + return new ValidationRule(Severity.ERROR, "empty", "failure message", (i) -> Fail.empty() ); } /** @@ -106,8 +106,8 @@ static ValidationRule empty() { * @return A new instance of a {@link ValidationRule} */ @SuppressWarnings("unchecked") - public static ValidationRule create(Severity severity, String description, String failureMessage, Function fn) { - return new ValidationRule(severity, description, failureMessage, (Function) fn); + public static ValidationRule create(Severity severity, String description, String failureMessage, Function fn) { + return new ValidationRule(severity, description, failureMessage, (Function) fn); } /** @@ -121,8 +121,8 @@ public static ValidationRule create(Severity severity, String description, S * @return A new instance of a {@link ValidationRule} */ @SuppressWarnings("unchecked") - public static ValidationRule error(String failureMessage, Function fn) { - return new ValidationRule(Severity.ERROR, null, failureMessage, (Function) fn); + public static ValidationRule error(String failureMessage, Function fn) { + return new ValidationRule(Severity.ERROR, null, failureMessage, (Function) fn); } /** @@ -137,8 +137,8 @@ public static ValidationRule error(String failureMessage, Function ValidationRule warn(String description, String failureMessage, Function fn) { - return new ValidationRule(Severity.WARNING, description, failureMessage, (Function) fn); + public static ValidationRule warn(String description, String failureMessage, Function fn) { + return new ValidationRule(Severity.WARNING, description, failureMessage, (Function) fn); } @Override @@ -149,4 +149,69 @@ public String toString() { ", failureMessage='" + failureMessage + '\'' + '}'; } + + public static abstract class Result { + protected String details = null; + protected Throwable throwable = null; + + public String getDetails() { + return details; + } + + public void setDetails(String details) { + assert this.details == null; + this.details = details; + } + + public abstract boolean passed(); + public final boolean failed() { return !passed(); } + + public Throwable getThrowable() { + return throwable; + } + + public boolean thrown() { return this.throwable == null; } + } + + public static final class Pass extends Result { + public static Result empty() { return new Pass(); } + + public Pass() { + super(); + } + + public Pass(String details) { + this(); + this.details = details; + } + + @Override + public boolean passed() { + return true; + } + } + + public static final class Fail extends Result { + public static Result empty() { return new Fail(); } + + public Fail() { + super(); + } + + public Fail(String details) { + this(); + this.details = details; + } + + public Fail(String details, Throwable throwable) { + this(); + this.throwable = throwable; + this.details = details; + } + + @Override + public boolean passed() { + return false; + } + } } diff --git a/modules/openapi-generator-core/src/test/java/org/openapitools/codegen/validation/GenericValidatorTest.java b/modules/openapi-generator-core/src/test/java/org/openapitools/codegen/validation/GenericValidatorTest.java index d37dc827c845..3fa8874ff19c 100644 --- a/modules/openapi-generator-core/src/test/java/org/openapitools/codegen/validation/GenericValidatorTest.java +++ b/modules/openapi-generator-core/src/test/java/org/openapitools/codegen/validation/GenericValidatorTest.java @@ -25,7 +25,7 @@ import java.util.Optional; public class GenericValidatorTest { - class Person { + static class Person { private int age; private String name; @@ -35,33 +35,33 @@ class Person { } } - private static boolean isValidAge(Person person) { - return person.age > 0; + private static ValidationRule.Result checkAge(Person person) { + return person.age > 0 ? ValidationRule.Pass.empty() : ValidationRule.Fail.empty(); } - private static boolean isAdult(Person person) { - return person.age > 18; + private static ValidationRule.Result checkAdult(Person person) { + return person.age > 18 ? ValidationRule.Pass.empty() : ValidationRule.Fail.empty(); } - private static boolean isNameSet(Person person) { - return person.name != null && person.name.length() > 0; + private static ValidationRule.Result checkName(Person person) { + return (person.name != null && person.name.length() > 0) ? ValidationRule.Pass.empty() : ValidationRule.Fail.empty(); } - private static boolean isNameValid(Person person) { + private static ValidationRule.Result checkNamePattern(Person person) { String pattern = "^[A-Z][a-z]*$"; - return person.name.matches(pattern); + return person.name.matches(pattern) ? ValidationRule.Pass.empty() : ValidationRule.Fail.empty(); } - private static boolean isNameNormalLength(Person person) { - return person.name.length() < 10; + private static ValidationRule.Result checkNameNormalLength(Person person) { + return person.name.length() < 10? ValidationRule.Pass.empty() : ValidationRule.Fail.empty(); } private List validationRules = Arrays.asList( - ValidationRule.error("Age must be positive and more than zero", GenericValidatorTest::isValidAge), - ValidationRule.error("Only adults (18 years old and older)", GenericValidatorTest::isAdult), - ValidationRule.error("Name isn't set!", GenericValidatorTest::isNameSet), - ValidationRule.error("Name isn't formatted correct", GenericValidatorTest::isNameValid), - ValidationRule.warn("Name too long?", "Name may be too long.", GenericValidatorTest::isNameNormalLength) + ValidationRule.error("Age must be positive and more than zero", GenericValidatorTest::checkAge), + ValidationRule.error("Only adults (18 years old and older)", GenericValidatorTest::checkAdult), + ValidationRule.error("Name isn't set!", GenericValidatorTest::checkName), + ValidationRule.error("Name isn't formatted correct", GenericValidatorTest::checkNamePattern), + ValidationRule.warn("Name too long?", "Name may be too long.", GenericValidatorTest::checkNameNormalLength) ); @Test diff --git a/modules/openapi-generator-core/src/test/java/org/openapitools/codegen/validation/ValidationRuleTest.java b/modules/openapi-generator-core/src/test/java/org/openapitools/codegen/validation/ValidationRuleTest.java index ce0fa165ce7a..b626085c8b3a 100644 --- a/modules/openapi-generator-core/src/test/java/org/openapitools/codegen/validation/ValidationRuleTest.java +++ b/modules/openapi-generator-core/src/test/java/org/openapitools/codegen/validation/ValidationRuleTest.java @@ -33,13 +33,13 @@ public String getName() { } } - private static boolean checkName(Sample input) { - return input.getName() != null && input.getName().length() > 7; + private static ValidationRule.Result checkName(Sample input) { + return (input.getName() != null && input.getName().length() > 7) ? ValidationRule.Pass.empty() : ValidationRule.Fail.empty(); } - private static boolean checkPattern(Sample input) { + private static ValidationRule.Result checkPattern(Sample input) { String pattern = "^[A-Z][a-z]*$"; - return input.getName() != null && input.getName().matches(pattern); + return (input.getName() != null && input.getName().matches(pattern)) ? ValidationRule.Pass.empty() : ValidationRule.Fail.empty(); } @Test @@ -49,10 +49,10 @@ public void createMethodUsingMethodReference(){ Sample seven = new Sample("1234567"); Sample eight = new Sample("12345678"); ValidationRule result = ValidationRule.error("test", ValidationRuleTest::checkName); - assertFalse(result.evaluate(nil)); - assertFalse(result.evaluate(six)); - assertFalse(result.evaluate(seven)); - assertTrue(result.evaluate(eight)); + assertFalse(result.evaluate(nil).passed()); + assertFalse(result.evaluate(six).passed()); + assertFalse(result.evaluate(seven).passed()); + assertTrue(result.evaluate(eight).passed()); } @Test @@ -61,8 +61,8 @@ public void createMethodUsingLambda(){ Sample lowercase = new Sample("jim"); Sample titlecase = new Sample("Jim"); ValidationRule result = ValidationRule.error("test", i -> checkPattern((Sample)i)); - assertFalse(result.evaluate(nil)); - assertFalse(result.evaluate(lowercase)); - assertTrue(result.evaluate(titlecase)); + assertFalse(result.evaluate(nil).passed()); + assertFalse(result.evaluate(lowercase).passed()); + assertTrue(result.evaluate(titlecase).passed()); } } diff --git a/modules/openapi-generator/src/main/java/org/openapitools/codegen/validations/oas/OpenApiEvaluator.java b/modules/openapi-generator/src/main/java/org/openapitools/codegen/validations/oas/OpenApiEvaluator.java index f72d06e88510..46dbd8c7b47c 100644 --- a/modules/openapi-generator/src/main/java/org/openapitools/codegen/validations/oas/OpenApiEvaluator.java +++ b/modules/openapi-generator/src/main/java/org/openapitools/codegen/validations/oas/OpenApiEvaluator.java @@ -45,7 +45,7 @@ public ValidationResult validate(OpenAPI specification) { OpenApiSchemaValidations schemaValidations = new OpenApiSchemaValidations(ruleConfiguration); if (ruleConfiguration.isEnableUnusedSchemasRecommendation()) { - ValidationRule unusedSchema = ValidationRule.create(Severity.WARNING, "Unused schema", "A schema was determined to be unused.", s -> true); + ValidationRule unusedSchema = ValidationRule.create(Severity.WARNING, "Unused schema", "A schema was determined to be unused.", s -> ValidationRule.Pass.empty()); ModelUtils.getUnusedSchemas(specification).forEach(schemaName -> validationResult.addResult(Validated.invalid(unusedSchema, "Unused model: " + schemaName))); } @@ -88,9 +88,7 @@ public ValidationResult validate(OpenAPI specification) { } } - parameters.forEach(parameter -> { - validationResult.consume(parameterValidations.validate(parameter)); - }); + parameters.forEach(parameter -> validationResult.consume(parameterValidations.validate(parameter))); return validationResult; } diff --git a/modules/openapi-generator/src/main/java/org/openapitools/codegen/validations/oas/OpenApiParameterValidations.java b/modules/openapi-generator/src/main/java/org/openapitools/codegen/validations/oas/OpenApiParameterValidations.java index fafeeb8cf331..cf9316ea2d16 100644 --- a/modules/openapi-generator/src/main/java/org/openapitools/codegen/validations/oas/OpenApiParameterValidations.java +++ b/modules/openapi-generator/src/main/java/org/openapitools/codegen/validations/oas/OpenApiParameterValidations.java @@ -7,6 +7,7 @@ import org.openapitools.codegen.validation.ValidationRule; import java.util.ArrayList; +import java.util.Locale; /** * A standalone instance for evaluating rules and recommendations related to OAS {@link Parameter} @@ -19,7 +20,7 @@ class OpenApiParameterValidations extends GenericValidator { rules.add(ValidationRule.warn( ValidationConstants.ApacheNginxUnderscoreDescription, ValidationConstants.ApacheNginxUnderscoreFailureMessage, - OpenApiParameterValidations::passesApacheNginxHeaderCheck + OpenApiParameterValidations::apacheNginxHeaderCheck )); } } @@ -30,18 +31,18 @@ class OpenApiParameterValidations extends GenericValidator { * * @param parameter Any spec doc parameter. The method will handle {@link HeaderParameter} evaluation. * - * @return true if the check succeeds (header does not have an underscore, e.g. 'api_key') + * @return {@link ValidationRule.Pass} if the check succeeds, otherwise {@link ValidationRule.Fail} with details "[key] contains an underscore." */ - private static boolean passesApacheNginxHeaderCheck(Parameter parameter) { - if (parameter == null || !parameter.getIn().equals("header")) return true; - - boolean valid = true; + private static ValidationRule.Result apacheNginxHeaderCheck(Parameter parameter) { + if (parameter == null || !parameter.getIn().equals("header")) return ValidationRule.Pass.empty(); + ValidationRule.Result result = ValidationRule.Pass.empty(); String headerName = parameter.getName(); if (StringUtils.isNotEmpty(headerName) && StringUtils.contains(headerName, '_')) { - valid = false; + result = new ValidationRule.Fail(); + result.setDetails(String.format(Locale.ROOT, "%s contains an underscore.", headerName)); } - return valid; + return result; } } diff --git a/modules/openapi-generator/src/main/java/org/openapitools/codegen/validations/oas/OpenApiSchemaValidations.java b/modules/openapi-generator/src/main/java/org/openapitools/codegen/validations/oas/OpenApiSchemaValidations.java index 9d45fb0c05aa..bfb829a8010f 100644 --- a/modules/openapi-generator/src/main/java/org/openapitools/codegen/validations/oas/OpenApiSchemaValidations.java +++ b/modules/openapi-generator/src/main/java/org/openapitools/codegen/validations/oas/OpenApiSchemaValidations.java @@ -18,7 +18,7 @@ class OpenApiSchemaValidations extends GenericValidator { rules.add(ValidationRule.warn( "Schema defines properties alongside oneOf.", "Schemas defining properties and oneOf are not clearly defined in the OpenAPI Specification. While our tooling supports this, it may cause issues with other tools.", - OpenApiSchemaValidations::validOneOfWithProperties + OpenApiSchemaValidations::checkOneOfWithProperties )); } } @@ -35,10 +35,11 @@ class OpenApiSchemaValidations extends GenericValidator { * properties on the schema defining oneOf relationships may not be intentional in the OpenAPI Specification. * * @param schema An input schema, regardless of the type of schema - * @return true if the schema has oneOf defined along with properties other than discriminator. + * @return {@link ValidationRule.Pass} if the check succeeds, otherwise {@link ValidationRule.Fail} */ - private static boolean validOneOfWithProperties(Schema schema) { - boolean valid = true; + private static ValidationRule.Result checkOneOfWithProperties(Schema schema) { + ValidationRule.Result result = ValidationRule.Pass.empty(); + if (schema instanceof ComposedSchema) { final ComposedSchema composed = (ComposedSchema) schema; // check for loosely defined oneOf extension requirements. @@ -47,10 +48,10 @@ private static boolean validOneOfWithProperties(Schema schema) { if (composed.getOneOf() != null && composed.getOneOf().size() > 0) { if (composed.getProperties() != null && composed.getProperties().size() >= 1 && composed.getProperties().get("discriminator") == null) { // not necessarily "invalid" here, but we trigger the recommendation which requires the method to return false. - valid = false; + result = ValidationRule.Fail.empty(); } } } - return valid; + return result; } } diff --git a/modules/openapi-generator/src/main/java/org/openapitools/codegen/validations/oas/OpenApiSecuritySchemeValidations.java b/modules/openapi-generator/src/main/java/org/openapitools/codegen/validations/oas/OpenApiSecuritySchemeValidations.java index 20e56c6df602..93a906f4a6f3 100644 --- a/modules/openapi-generator/src/main/java/org/openapitools/codegen/validations/oas/OpenApiSecuritySchemeValidations.java +++ b/modules/openapi-generator/src/main/java/org/openapitools/codegen/validations/oas/OpenApiSecuritySchemeValidations.java @@ -6,6 +6,7 @@ import org.openapitools.codegen.validation.ValidationRule; import java.util.ArrayList; +import java.util.Locale; /** * A standalone instance for evaluating rules and recommendations related to OAS {@link SecurityScheme} @@ -18,7 +19,7 @@ class OpenApiSecuritySchemeValidations extends GenericValidator rules.add(ValidationRule.warn( ValidationConstants.ApacheNginxUnderscoreDescription, ValidationConstants.ApacheNginxUnderscoreFailureMessage, - OpenApiSecuritySchemeValidations::passesApacheNginxHeaderCheck + OpenApiSecuritySchemeValidations::apacheNginxHeaderCheck )); } } @@ -31,9 +32,16 @@ class OpenApiSecuritySchemeValidations extends GenericValidator * * @return true if the check succeeds (header does not have an underscore, e.g. 'api_key') */ - private static boolean passesApacheNginxHeaderCheck(SecurityScheme securityScheme) { - if (securityScheme == null || securityScheme.getIn() != SecurityScheme.In.HEADER) return true; + private static ValidationRule.Result apacheNginxHeaderCheck(SecurityScheme securityScheme) { + if (securityScheme == null || securityScheme.getIn() != SecurityScheme.In.HEADER) return ValidationRule.Pass.empty(); + ValidationRule.Result result = ValidationRule.Pass.empty(); - return !StringUtils.contains(securityScheme.getName(), '_'); + String key = securityScheme.getName(); + if (StringUtils.contains(key, '_')) { + result = new ValidationRule.Fail(); + result.setDetails(String.format(Locale.ROOT, "%s contains an underscore.", key)); + } + + return result; } } From 5d4ab5070244b1e8b8882a6d793ce740b8efc348 Mon Sep 17 00:00:00 2001 From: Jim Schubert Date: Sun, 12 Jan 2020 21:54:57 -0500 Subject: [PATCH 5/6] Add operation recommendation for GET/HEAD w/body Recommendation can be disabled via system property: -Dopenapi.generator.rule.anti-patterns.uri-unexpected-body=false --- .../validations/oas/OpenApiEvaluator.java | 23 ++-- .../oas/OpenApiOperationValidations.java | 75 +++++++++++ .../oas/OpenApiParameterValidations.java | 1 - .../oas/OpenApiSecuritySchemeValidations.java | 4 +- .../validations/oas/OperationWrapper.java | 42 +++++++ .../validations/oas/RuleConfiguration.java | 32 ++++- .../oas/OpenApiOperationValidationsTest.java | 116 ++++++++++++++++++ 7 files changed, 273 insertions(+), 20 deletions(-) create mode 100644 modules/openapi-generator/src/main/java/org/openapitools/codegen/validations/oas/OpenApiOperationValidations.java create mode 100644 modules/openapi-generator/src/main/java/org/openapitools/codegen/validations/oas/OperationWrapper.java create mode 100644 modules/openapi-generator/src/test/java/org/openapitools/codegen/validations/oas/OpenApiOperationValidationsTest.java diff --git a/modules/openapi-generator/src/main/java/org/openapitools/codegen/validations/oas/OpenApiEvaluator.java b/modules/openapi-generator/src/main/java/org/openapitools/codegen/validations/oas/OpenApiEvaluator.java index 46dbd8c7b47c..e28a69b4386b 100644 --- a/modules/openapi-generator/src/main/java/org/openapitools/codegen/validations/oas/OpenApiEvaluator.java +++ b/modules/openapi-generator/src/main/java/org/openapitools/codegen/validations/oas/OpenApiEvaluator.java @@ -12,7 +12,6 @@ import java.util.ArrayList; import java.util.List; import java.util.Map; -import java.util.stream.Stream; /** * A validator which evaluates an OpenAPI 3.x specification document @@ -43,6 +42,7 @@ public ValidationResult validate(OpenAPI specification) { OpenApiParameterValidations parameterValidations = new OpenApiParameterValidations(ruleConfiguration); OpenApiSecuritySchemeValidations securitySchemeValidations = new OpenApiSecuritySchemeValidations(ruleConfiguration); OpenApiSchemaValidations schemaValidations = new OpenApiSchemaValidations(ruleConfiguration); + OpenApiOperationValidations operationValidations = new OpenApiOperationValidations(ruleConfiguration); if (ruleConfiguration.isEnableUnusedSchemasRecommendation()) { ValidationRule unusedSchema = ValidationRule.create(Severity.WARNING, "Unused schema", "A schema was determined to be unused.", s -> ValidationRule.Pass.empty()); @@ -61,17 +61,16 @@ public ValidationResult validate(OpenAPI specification) { List pathParameters = pathItem.getParameters(); if (pathParameters != null) parameters.addAll(pathItem.getParameters()); - // parameters on each operation method - Stream.of( - pathItem.getGet(), - pathItem.getDelete(), - pathItem.getHead(), - pathItem.getOptions(), - pathItem.getPatch(), - pathItem.getPost(), - pathItem.getPut(), - pathItem.getTrace()).forEach(op -> { - if (op != null && op.getParameters() != null) parameters.addAll(op.getParameters()); + pathItem.readOperationsMap().forEach((httpMethod, op) -> { + if (op != null) { + // parameters on each operation method + if (op.getParameters() != null) { + parameters.addAll(op.getParameters()); + } + + OperationWrapper wrapper = new OperationWrapper(op, httpMethod); + validationResult.consume(operationValidations.validate(wrapper)); + } }); }); } diff --git a/modules/openapi-generator/src/main/java/org/openapitools/codegen/validations/oas/OpenApiOperationValidations.java b/modules/openapi-generator/src/main/java/org/openapitools/codegen/validations/oas/OpenApiOperationValidations.java new file mode 100644 index 000000000000..9b1de83e5242 --- /dev/null +++ b/modules/openapi-generator/src/main/java/org/openapitools/codegen/validations/oas/OpenApiOperationValidations.java @@ -0,0 +1,75 @@ +package org.openapitools.codegen.validations.oas; + +import io.swagger.v3.oas.models.PathItem; +import io.swagger.v3.oas.models.parameters.RequestBody; +import org.apache.commons.lang3.StringUtils; +import org.openapitools.codegen.validation.GenericValidator; +import org.openapitools.codegen.validation.ValidationRule; + +import java.util.ArrayList; +import java.util.Locale; + +/** + * A standalone instance for evaluating rule and recommendations related to OAS {@link io.swagger.v3.oas.models.Operation} + */ +class OpenApiOperationValidations extends GenericValidator { + OpenApiOperationValidations(RuleConfiguration ruleConfiguration) { + super(new ArrayList<>()); + if (ruleConfiguration.isEnableRecommendations()) { + if (ruleConfiguration.isEnableApiRequestUriWithBodyRecommendation()) { + rules.add(ValidationRule.warn( + "API GET/HEAD defined with request body", + "While technically allowed, GET/HEAD with request body may indicate programming error, and is considered an anti-pattern.", + OpenApiOperationValidations::checkAntipatternGetOrHeadWithBody + )); + } + } + } + + /** + * Determines whether a GET or HEAD operation is configured to expect a body. + *

+ * RFC7231 describes this behavior as: + *

+ * A payload within a GET request message has no defined semantics; + * sending a payload body on a GET request might cause some existing + * implementations to reject the request. + *

+ * See https://tools.ietf.org/html/rfc7231#section-4.3.1 + *

+ * Because there are no defined semantics, and because some client and server implementations + * may silently ignore the entire body (see https://xhr.spec.whatwg.org/#the-send()-method) or + * throw an error (see https://fetch.spec.whatwg.org/#ref-for-dfn-throw%E2%91%A1%E2%91%A1), + * we maintain that the existence of a body for this operation is most likely programmer error and raise awareness. + * + * @param wrapper Wraps an operation with accompanying HTTP Method + * @return {@link ValidationRule.Pass} if the check succeeds, otherwise {@link ValidationRule.Fail} + */ + private static ValidationRule.Result checkAntipatternGetOrHeadWithBody(OperationWrapper wrapper) { + if (wrapper == null) { + return ValidationRule.Pass.empty(); + } + + ValidationRule.Result result = ValidationRule.Pass.empty(); + + if (wrapper.getHttpMethod() == PathItem.HttpMethod.GET || wrapper.getHttpMethod() == PathItem.HttpMethod.HEAD) { + RequestBody body = wrapper.getOperation().getRequestBody(); + + if (body != null) { + if (StringUtils.isNotEmpty(body.get$ref()) || (body.getContent() != null && body.getContent().size() > 0)) { + result = new ValidationRule.Fail(); + result.setDetails(String.format( + Locale.ROOT, + "%s %s contains a request body and is considered an anti-pattern.", + wrapper.getHttpMethod().name(), + wrapper.getOperation().getOperationId()) + ); + } + } + + } + + return result; + } + +} diff --git a/modules/openapi-generator/src/main/java/org/openapitools/codegen/validations/oas/OpenApiParameterValidations.java b/modules/openapi-generator/src/main/java/org/openapitools/codegen/validations/oas/OpenApiParameterValidations.java index cf9316ea2d16..0e3585585a03 100644 --- a/modules/openapi-generator/src/main/java/org/openapitools/codegen/validations/oas/OpenApiParameterValidations.java +++ b/modules/openapi-generator/src/main/java/org/openapitools/codegen/validations/oas/OpenApiParameterValidations.java @@ -30,7 +30,6 @@ class OpenApiParameterValidations extends GenericValidator { * Apache and Nginx default to legacy CGI behavior in which header with underscore are ignored. Raise this for awareness to the user. * * @param parameter Any spec doc parameter. The method will handle {@link HeaderParameter} evaluation. - * * @return {@link ValidationRule.Pass} if the check succeeds, otherwise {@link ValidationRule.Fail} with details "[key] contains an underscore." */ private static ValidationRule.Result apacheNginxHeaderCheck(Parameter parameter) { diff --git a/modules/openapi-generator/src/main/java/org/openapitools/codegen/validations/oas/OpenApiSecuritySchemeValidations.java b/modules/openapi-generator/src/main/java/org/openapitools/codegen/validations/oas/OpenApiSecuritySchemeValidations.java index 93a906f4a6f3..53bc3f5224e4 100644 --- a/modules/openapi-generator/src/main/java/org/openapitools/codegen/validations/oas/OpenApiSecuritySchemeValidations.java +++ b/modules/openapi-generator/src/main/java/org/openapitools/codegen/validations/oas/OpenApiSecuritySchemeValidations.java @@ -29,11 +29,11 @@ class OpenApiSecuritySchemeValidations extends GenericValidator * Apache and Nginx default to legacy CGI behavior in which header with underscore are ignored. Raise this for awareness to the user. * * @param securityScheme Security schemes are often used as header parameters (e.g. APIKEY). - * * @return true if the check succeeds (header does not have an underscore, e.g. 'api_key') */ private static ValidationRule.Result apacheNginxHeaderCheck(SecurityScheme securityScheme) { - if (securityScheme == null || securityScheme.getIn() != SecurityScheme.In.HEADER) return ValidationRule.Pass.empty(); + if (securityScheme == null || securityScheme.getIn() != SecurityScheme.In.HEADER) + return ValidationRule.Pass.empty(); ValidationRule.Result result = ValidationRule.Pass.empty(); String key = securityScheme.getName(); diff --git a/modules/openapi-generator/src/main/java/org/openapitools/codegen/validations/oas/OperationWrapper.java b/modules/openapi-generator/src/main/java/org/openapitools/codegen/validations/oas/OperationWrapper.java new file mode 100644 index 000000000000..c2e3b249fdac --- /dev/null +++ b/modules/openapi-generator/src/main/java/org/openapitools/codegen/validations/oas/OperationWrapper.java @@ -0,0 +1,42 @@ +package org.openapitools.codegen.validations.oas; + +import io.swagger.v3.oas.models.Operation; +import io.swagger.v3.oas.models.PathItem; + +/** + * Encapsulates an operation with its HTTP Method. In OAS, the {@link PathItem} structure contains more than what we'd + * want to evaluate for operation-only checks. + */ +public class OperationWrapper { + private Operation operation; + private PathItem.HttpMethod httpMethod; + + /** + * Constructs a new instance of {@link OperationWrapper} + * + * @param operation The operation instances to wrap + * @param httpMethod The http method to wrap + */ + OperationWrapper(Operation operation, PathItem.HttpMethod httpMethod) { + this.operation = operation; + this.httpMethod = httpMethod; + } + + /** + * Gets the operation associated with the http method + * + * @return An operation instance + */ + public Operation getOperation() { + return operation; + } + + /** + * Gets the http method associated with the operation + * + * @return The http method + */ + public PathItem.HttpMethod getHttpMethod() { + return httpMethod; + } +} diff --git a/modules/openapi-generator/src/main/java/org/openapitools/codegen/validations/oas/RuleConfiguration.java b/modules/openapi-generator/src/main/java/org/openapitools/codegen/validations/oas/RuleConfiguration.java index 5403a7550ddb..c8238ef590e4 100644 --- a/modules/openapi-generator/src/main/java/org/openapitools/codegen/validations/oas/RuleConfiguration.java +++ b/modules/openapi-generator/src/main/java/org/openapitools/codegen/validations/oas/RuleConfiguration.java @@ -11,6 +11,8 @@ public class RuleConfiguration { private boolean enableOneOfWithPropertiesRecommendation = defaultedBoolean(propertyPrefix + ".oneof-properties-ambiguity", true); private boolean enableUnusedSchemasRecommendation = defaultedBoolean(propertyPrefix + ".unused-schemas", true); + private boolean enableApiRequestUriWithBodyRecommendation = defaultedBoolean(propertyPrefix + ".anti-patterns.uri-unexpected-body", true); + @SuppressWarnings("SameParameterValue") private static boolean defaultedBoolean(String key, boolean defaultValue) { String property = System.getProperty(key); @@ -30,7 +32,7 @@ public boolean isEnableApacheNginxUnderscoreRecommendation() { /** * Enable or Disable the recommendation check for Apache/Nginx potentially ignoring header with underscore by default. - * + *

* For more details, see {@link RuleConfiguration#isEnableApacheNginxUnderscoreRecommendation()} * * @param enableApacheNginxUnderscoreRecommendation true to enable, false to disable @@ -40,8 +42,28 @@ public void setEnableApacheNginxUnderscoreRecommendation(boolean enableApacheNgi } /** - * Gets whether the recommendation check for oneOf with sibling properties exists. + * Gets whether we will raise awareness a GET or HEAD operation is defined with body. + * + * @return true if enabled, false if disabled + */ + public boolean isEnableApiRequestUriWithBodyRecommendation() { + return enableApiRequestUriWithBodyRecommendation; + } + + /** + * Enable or Disable the recommendation check for GET or HEAD operations with bodies. + *

+ * For more details, see {@link RuleConfiguration#isEnableApiRequestUriWithBodyRecommendation()} * + * @param enableApiRequestUriWithBodyRecommendation true to enable, false to disable + */ + public void setEnableApiRequestUriWithBodyRecommendation(boolean enableApiRequestUriWithBodyRecommendation) { + this.enableApiRequestUriWithBodyRecommendation = enableApiRequestUriWithBodyRecommendation; + } + + /** + * Gets whether the recommendation check for oneOf with sibling properties exists. + *

* JSON Schema defines oneOf as a validation property which can be applied to any schema. *

* OpenAPI Specification is a variant of JSON Schema for which oneOf is defined as: @@ -59,7 +81,7 @@ public boolean isEnableOneOfWithPropertiesRecommendation() { /** * Enable or Disable the recommendation check for schemas containing properties and oneOf definitions. - * + *

* For more details, see {@link RuleConfiguration#isEnableOneOfWithPropertiesRecommendation()} * * @param enableOneOfWithPropertiesRecommendation true to enable, false to disable @@ -90,7 +112,7 @@ public void setEnableRecommendations(boolean enableRecommendations) { /** * Gets whether the recommendation to check for unused schemas is enabled. - * + *

* While the tooling may or may not support generation of models representing unused schemas, we take the stance that * a schema which is defined but not referenced in an operation or by some schema bound to an operation may be a good * indicator of a programming error. We surface this information to the user in case the orphaned schema(s) are not @@ -104,7 +126,7 @@ public boolean isEnableUnusedSchemasRecommendation() { /** * Enable or Disable the recommendation check for unused schemas. - * + *

* For more details, see {@link RuleConfiguration#isEnableUnusedSchemasRecommendation()} * * @param enableUnusedSchemasRecommendation true to enable, false to disable diff --git a/modules/openapi-generator/src/test/java/org/openapitools/codegen/validations/oas/OpenApiOperationValidationsTest.java b/modules/openapi-generator/src/test/java/org/openapitools/codegen/validations/oas/OpenApiOperationValidationsTest.java new file mode 100644 index 000000000000..16cb66df26eb --- /dev/null +++ b/modules/openapi-generator/src/test/java/org/openapitools/codegen/validations/oas/OpenApiOperationValidationsTest.java @@ -0,0 +1,116 @@ +package org.openapitools.codegen.validations.oas; + +import io.swagger.v3.oas.models.Operation; +import io.swagger.v3.oas.models.PathItem; +import io.swagger.v3.oas.models.media.Content; +import io.swagger.v3.oas.models.media.MediaType; +import io.swagger.v3.oas.models.parameters.RequestBody; +import org.apache.commons.lang3.StringUtils; +import org.openapitools.codegen.validation.Invalid; +import org.openapitools.codegen.validation.ValidationResult; +import org.testng.Assert; +import org.testng.annotations.DataProvider; +import org.testng.annotations.Test; + +import java.util.List; +import java.util.stream.Collectors; + +public class OpenApiOperationValidationsTest { + @DataProvider(name = "getOrHeadWithBodyExpectations") + public Object[][] getOrHeadWithBodyExpectations() { + return new Object[][]{ + /* method */ /* operationId */ /* ref */ /* content */ /* triggers warning */ + {PathItem.HttpMethod.GET, "opWithRerf", "#/components/schemas/Animal", null, true}, + {PathItem.HttpMethod.GET, "opWithRerf", null, new Content().addMediaType("a", new MediaType()), true}, + {PathItem.HttpMethod.GET, "opWithoutRerf", null, null, false}, + {PathItem.HttpMethod.HEAD, "opWithRerf", "#/components/schemas/Animal", null, true}, + {PathItem.HttpMethod.HEAD, "opWithRerf", null, new Content().addMediaType("a", new MediaType()), true}, + {PathItem.HttpMethod.HEAD, "opWithoutRerf", null, null, false}, + {PathItem.HttpMethod.POST, "opWithRerf", "#/components/schemas/Animal", null, false}, + {PathItem.HttpMethod.POST, "opWithRerf", null, new Content().addMediaType("a", new MediaType()), false}, + {PathItem.HttpMethod.POST, "opWithoutRerf", null, null, false} + }; + } + + @Test(dataProvider = "getOrHeadWithBodyExpectations") + public void testGetOrHeadWithBody(PathItem.HttpMethod method, String operationId, String ref, Content content, boolean shouldTriggerFailure) { + RuleConfiguration config = new RuleConfiguration(); + config.setEnableRecommendations(true); + OpenApiOperationValidations validator = new OpenApiOperationValidations(config); + + Operation op = new Operation().operationId(operationId); + RequestBody body = new RequestBody(); + if (StringUtils.isNotEmpty(ref) || content != null) { + body.$ref(ref); + body.content(content); + + op.setRequestBody(body); + } + + ValidationResult result = validator.validate(new OperationWrapper(op, method)); + Assert.assertNotNull(result.getWarnings()); + + List warnings = result.getWarnings().stream() + .filter(invalid -> "API GET/HEAD defined with request body".equals(invalid.getRule().getDescription())) + .collect(Collectors.toList()); + + Assert.assertNotNull(warnings); + if (shouldTriggerFailure) { + Assert.assertEquals(warnings.size(), 1, "Expected warnings to include recommendation."); + } else { + Assert.assertEquals(warnings.size(), 0, "Expected warnings not to include recommendation."); + } + } + + @Test(dataProvider = "getOrHeadWithBodyExpectations") + public void testGetOrHeadWithBodyWithDisabledRecommendations(PathItem.HttpMethod method, String operationId, String ref, Content content, boolean shouldTriggerFailure) { + RuleConfiguration config = new RuleConfiguration(); + config.setEnableRecommendations(false); + OpenApiOperationValidations validator = new OpenApiOperationValidations(config); + + Operation op = new Operation().operationId(operationId); + RequestBody body = new RequestBody(); + if (StringUtils.isNotEmpty(ref) || content != null) { + body.$ref(ref); + body.content(content); + + op.setRequestBody(body); + } + + ValidationResult result = validator.validate(new OperationWrapper(op, method)); + Assert.assertNotNull(result.getWarnings()); + + List warnings = result.getWarnings().stream() + .filter(invalid -> "API GET/HEAD defined with request body".equals(invalid.getRule().getDescription())) + .collect(Collectors.toList()); + + Assert.assertNotNull(warnings); + Assert.assertEquals(warnings.size(), 0, "Expected warnings not to include recommendation."); + } + + @Test(dataProvider = "getOrHeadWithBodyExpectations") + public void testGetOrHeadWithBodyWithDisabledRule(PathItem.HttpMethod method, String operationId, String ref, Content content, boolean shouldTriggerFailure) { + RuleConfiguration config = new RuleConfiguration(); + config.setEnableApiRequestUriWithBodyRecommendation(false); + OpenApiOperationValidations validator = new OpenApiOperationValidations(config); + + Operation op = new Operation().operationId(operationId); + RequestBody body = new RequestBody(); + if (StringUtils.isNotEmpty(ref) || content != null) { + body.$ref(ref); + body.content(content); + + op.setRequestBody(body); + } + + ValidationResult result = validator.validate(new OperationWrapper(op, method)); + Assert.assertNotNull(result.getWarnings()); + + List warnings = result.getWarnings().stream() + .filter(invalid -> "API GET/HEAD defined with request body".equals(invalid.getRule().getDescription())) + .collect(Collectors.toList()); + + Assert.assertNotNull(warnings); + Assert.assertEquals(warnings.size(), 0, "Expected warnings not to include recommendation."); + } +} \ No newline at end of file From 129d0e1486cbc13c76bb6ccf56df296a1e491ffd Mon Sep 17 00:00:00 2001 From: Jim Schubert Date: Sun, 12 Jan 2020 21:59:59 -0500 Subject: [PATCH 6/6] Update operationId values in test --- .../oas/OpenApiOperationValidationsTest.java | 20 +++++++++---------- .../oas/OpenApiParameterValidationsTest.java | 2 +- 2 files changed, 11 insertions(+), 11 deletions(-) diff --git a/modules/openapi-generator/src/test/java/org/openapitools/codegen/validations/oas/OpenApiOperationValidationsTest.java b/modules/openapi-generator/src/test/java/org/openapitools/codegen/validations/oas/OpenApiOperationValidationsTest.java index 16cb66df26eb..c4d911563eca 100644 --- a/modules/openapi-generator/src/test/java/org/openapitools/codegen/validations/oas/OpenApiOperationValidationsTest.java +++ b/modules/openapi-generator/src/test/java/org/openapitools/codegen/validations/oas/OpenApiOperationValidationsTest.java @@ -20,15 +20,15 @@ public class OpenApiOperationValidationsTest { public Object[][] getOrHeadWithBodyExpectations() { return new Object[][]{ /* method */ /* operationId */ /* ref */ /* content */ /* triggers warning */ - {PathItem.HttpMethod.GET, "opWithRerf", "#/components/schemas/Animal", null, true}, - {PathItem.HttpMethod.GET, "opWithRerf", null, new Content().addMediaType("a", new MediaType()), true}, - {PathItem.HttpMethod.GET, "opWithoutRerf", null, null, false}, - {PathItem.HttpMethod.HEAD, "opWithRerf", "#/components/schemas/Animal", null, true}, - {PathItem.HttpMethod.HEAD, "opWithRerf", null, new Content().addMediaType("a", new MediaType()), true}, - {PathItem.HttpMethod.HEAD, "opWithoutRerf", null, null, false}, - {PathItem.HttpMethod.POST, "opWithRerf", "#/components/schemas/Animal", null, false}, - {PathItem.HttpMethod.POST, "opWithRerf", null, new Content().addMediaType("a", new MediaType()), false}, - {PathItem.HttpMethod.POST, "opWithoutRerf", null, null, false} + {PathItem.HttpMethod.GET, "opWithRef", "#/components/schemas/Animal", null, true}, + {PathItem.HttpMethod.GET, "opWithContent", null, new Content().addMediaType("a", new MediaType()), true}, + {PathItem.HttpMethod.GET, "opWithoutRefOrContent", null, null, false}, + {PathItem.HttpMethod.HEAD, "opWithRef", "#/components/schemas/Animal", null, true}, + {PathItem.HttpMethod.HEAD, "opWithContent", null, new Content().addMediaType("a", new MediaType()), true}, + {PathItem.HttpMethod.HEAD, "opWithoutRefOrContent", null, null, false}, + {PathItem.HttpMethod.POST, "opWithRef", "#/components/schemas/Animal", null, false}, + {PathItem.HttpMethod.POST, "opWithContent", null, new Content().addMediaType("a", new MediaType()), false}, + {PathItem.HttpMethod.POST, "opWithoutRefOrContent", null, null, false} }; } @@ -113,4 +113,4 @@ public void testGetOrHeadWithBodyWithDisabledRule(PathItem.HttpMethod method, St Assert.assertNotNull(warnings); Assert.assertEquals(warnings.size(), 0, "Expected warnings not to include recommendation."); } -} \ No newline at end of file +} diff --git a/modules/openapi-generator/src/test/java/org/openapitools/codegen/validations/oas/OpenApiParameterValidationsTest.java b/modules/openapi-generator/src/test/java/org/openapitools/codegen/validations/oas/OpenApiParameterValidationsTest.java index 0b5c7807f9c4..a242a355e3a9 100644 --- a/modules/openapi-generator/src/test/java/org/openapitools/codegen/validations/oas/OpenApiParameterValidationsTest.java +++ b/modules/openapi-generator/src/test/java/org/openapitools/codegen/validations/oas/OpenApiParameterValidationsTest.java @@ -90,4 +90,4 @@ public Object[][] apacheNginxRecommendationExpectations() { {"query", "apikey", false} }; } -} \ No newline at end of file +}