-
-
Notifications
You must be signed in to change notification settings - Fork 6.5k
Commit
This commit does not belong to any branch on this repository, and may belong to a fork outside of the repository.
Add operation recommendation for GET/HEAD w/body
Recommendation can be disabled via system property: -Dopenapi.generator.rule.anti-patterns.uri-unexpected-body=false
- Loading branch information
1 parent
2a5191f
commit 5d4ab50
Showing
7 changed files
with
273 additions
and
20 deletions.
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
75 changes: 75 additions & 0 deletions
75
...r/src/main/java/org/openapitools/codegen/validations/oas/OpenApiOperationValidations.java
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -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<OperationWrapper> { | ||
| 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. | ||
| * <p> | ||
| * RFC7231 describes this behavior as: | ||
| * <p> | ||
| * 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. | ||
| * <p> | ||
| * See https://tools.ietf.org/html/rfc7231#section-4.3.1 | ||
| * <p> | ||
| * 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; | ||
| } | ||
|
|
||
| } |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
42 changes: 42 additions & 0 deletions
42
...pi-generator/src/main/java/org/openapitools/codegen/validations/oas/OperationWrapper.java
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -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; | ||
| } | ||
| } |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
116 changes: 116 additions & 0 deletions
116
...c/test/java/org/openapitools/codegen/validations/oas/OpenApiOperationValidationsTest.java
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -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<Invalid> 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<Invalid> 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<Invalid> 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."); | ||
| } | ||
| } |