Skip to content

Commit 22c6c0c

Browse files
jimschubertjimschubert
authored
[core] Extracting recommendations to validation framework (#4979)
* [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 * openapi.generator.rule.anti-patterns.uri-unexpected-body=false * Move recommendation/validations to oas package and add javadoc comments * Refactor and test recommendation validations * Refactor validation function signatures to return explicit state rather than boolean * Add operation recommendation for GET/HEAD w/body
1 parent f06ac9d commit 22c6c0c

File tree

20 files changed

+1090
-76
lines changed

20 files changed

+1090
-76
lines changed

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

Lines changed: 16 additions & 30 deletions
Original file line numberDiff line numberDiff line change
@@ -22,14 +22,14 @@
2222

2323
import io.swagger.parser.OpenAPIParser;
2424
import io.swagger.v3.oas.models.OpenAPI;
25-
import io.swagger.v3.oas.models.media.ComposedSchema;
26-
import io.swagger.v3.oas.models.media.Schema;
2725
import io.swagger.v3.parser.core.models.SwaggerParseResult;
28-
import org.openapitools.codegen.utils.ModelUtils;
26+
import org.apache.commons.lang3.text.WordUtils;
27+
import org.openapitools.codegen.validation.ValidationResult;
28+
import org.openapitools.codegen.validations.oas.OpenApiEvaluator;
29+
import org.openapitools.codegen.validations.oas.RuleConfiguration;
2930

3031
import java.util.HashSet;
3132
import java.util.List;
32-
import java.util.Map;
3333
import java.util.Set;
3434

3535
@Command(name = "validate", description = "Validate specification")
@@ -54,42 +54,28 @@ public void run() {
5454
StringBuilder sb = new StringBuilder();
5555
OpenAPI specification = result.getOpenAPI();
5656

57-
if (Boolean.TRUE.equals(recommend)) {
58-
if (specification != null) {
59-
// Add information about unused models to the warnings set.
60-
List<String> unusedModels = ModelUtils.getUnusedSchemas(specification);
61-
if (unusedModels != null) {
62-
unusedModels.forEach(name -> warnings.add("Unused model: " + name));
63-
}
64-
65-
// check for loosely defined oneOf extension requirements.
66-
// This is a recommendation because the 3.0.x spec is not clear enough on usage of oneOf.
67-
// 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'.
68-
Map<String, Schema> schemas = ModelUtils.getSchemas(specification);
69-
schemas.forEach((key, schema) -> {
70-
if (schema instanceof ComposedSchema) {
71-
final ComposedSchema composed = (ComposedSchema) schema;
72-
if (composed.getOneOf() != null && composed.getOneOf().size() > 0) {
73-
if (composed.getProperties() != null && composed.getProperties().size() >= 1 && composed.getProperties().get("discriminator") == null) {
74-
warnings.add("Schema (oneOf) should not contain properties: " + key);
75-
}
76-
}
77-
}
78-
});
79-
}
80-
}
57+
RuleConfiguration ruleConfiguration = new RuleConfiguration();
58+
ruleConfiguration.setEnableRecommendations(recommend != null ? recommend : false);
59+
60+
OpenApiEvaluator evaluator = new OpenApiEvaluator(ruleConfiguration);
61+
ValidationResult validationResult = evaluator.validate(specification);
62+
63+
// TODO: We could also provide description here along with getMessage. getMessage is either a "generic" message or specific (e.g. Model 'Cat' has issues).
64+
// This would require that we parse the messageList coming from swagger-parser into a better structure.
65+
validationResult.getWarnings().forEach(invalid -> warnings.add(invalid.getMessage()));
66+
validationResult.getErrors().forEach(invalid -> errors.add(invalid.getMessage()));
8167

8268
if (!errors.isEmpty()) {
8369
sb.append("Errors:").append(System.lineSeparator());
8470
errors.forEach(msg ->
85-
sb.append("\t-").append(msg).append(System.lineSeparator())
71+
sb.append("\t- ").append(WordUtils.wrap(msg, 90).replace(System.lineSeparator(), System.lineSeparator() + "\t ")).append(System.lineSeparator())
8672
);
8773
}
8874

8975
if (!warnings.isEmpty()) {
9076
sb.append("Warnings: ").append(System.lineSeparator());
9177
warnings.forEach(msg ->
92-
sb.append("\t-").append(msg).append(System.lineSeparator())
78+
sb.append("\t- ").append(WordUtils.wrap(msg, 90).replace(System.lineSeparator(), System.lineSeparator() + "\t ")).append(System.lineSeparator())
9379
);
9480
}
9581

‎modules/openapi-generator-core/src/main/java/org/openapitools/codegen/validation/GenericValidator.java

Lines changed: 4 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -23,9 +23,8 @@
2323
*
2424
* @param <TInput> The type of object being evaluated.
2525
*/
26-
@SuppressWarnings({"WeakerAccess"})
2726
public class GenericValidator<TInput> implements Validator<TInput> {
28-
private List<ValidationRule> rules;
27+
protected List<ValidationRule> rules;
2928

3029
/**
3130
* Constructs a new instance of {@link GenericValidator}.
@@ -48,11 +47,11 @@ public ValidationResult validate(TInput input) {
4847
ValidationResult result = new ValidationResult();
4948
if (rules != null) {
5049
rules.forEach(it -> {
51-
boolean passes = it.evaluate(input);
52-
if (passes) {
50+
ValidationRule.Result attempt = it.evaluate(input);
51+
if (attempt.passed()) {
5352
result.addResult(Validated.valid(it));
5453
} else {
55-
result.addResult(Validated.invalid(it, it.getFailureMessage()));
54+
result.addResult(Validated.invalid(it, it.getFailureMessage(), attempt.getDetails()));
5655
}
5756
});
5857
}

‎modules/openapi-generator-core/src/main/java/org/openapitools/codegen/validation/Invalid.java

Lines changed: 19 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -23,6 +23,7 @@
2323
public final class Invalid extends Validated {
2424
private String message;
2525
private ValidationRule rule;
26+
private String details;
2627

2728
/**
2829
* Constructs a new {@link Invalid} instance.
@@ -35,13 +36,29 @@ public final class Invalid extends Validated {
3536
this.message = message;
3637
}
3738

39+
/**
40+
* Constructs a new {@link Invalid} instance.
41+
*
42+
* @param rule The rule which was evaluated and resulted in this state.
43+
* @param message The message to be displayed for this invalid state.
44+
* @param details Additional contextual details related to the invalid state.
45+
*/
46+
public Invalid(ValidationRule rule, String message, String details) {
47+
this(rule, message);
48+
this.details = details;
49+
}
50+
51+
public String getDetails() {
52+
return details;
53+
}
54+
3855
@Override
39-
String getMessage() {
56+
public String getMessage() {
4057
return message;
4158
}
4259

4360
@Override
44-
ValidationRule getRule() {
61+
public ValidationRule getRule() {
4562
return rule;
4663
}
4764

‎modules/openapi-generator-core/src/main/java/org/openapitools/codegen/validation/Validated.java

Lines changed: 12 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -54,6 +54,18 @@ boolean isValid() {
5454
public static Validated invalid(ValidationRule rule, String message) {
5555
return new Invalid(rule, message);
5656
}
57+
/**
58+
* Creates an instance of an {@link Invalid} validation state.
59+
*
60+
* @param rule The rule which was evaluated.
61+
* @param message The message to display to a user.
62+
* @param details Additional contextual details related to the invalid state.
63+
*
64+
* @return A {@link Validated} instance representing an invalid state according to the rule.
65+
*/
66+
public static Validated invalid(ValidationRule rule, String message, String details) {
67+
return new Invalid(rule, message, details);
68+
}
5769

5870
/**
5971
* Creates an instance of an {@link Valid} validation state.

‎modules/openapi-generator-core/src/main/java/org/openapitools/codegen/validation/ValidationResult.java

Lines changed: 8 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -24,7 +24,6 @@
2424
/**
2525
* Encapsulates details about the result of a validation test.
2626
*/
27-
@SuppressWarnings("WeakerAccess")
2827
public final class ValidationResult {
2928
private final List<Validated> validations;
3029

@@ -96,9 +95,16 @@ public List<Invalid> getWarnings(){
9695
public void addResult(Validated validated) {
9796
synchronized (validations) {
9897
ValidationRule rule = validated.getRule();
99-
if (rule != null && !rule.equals(ValidationRule.empty())) {
98+
if (rule != null && !rule.equals(ValidationRule.empty()) && !validations.contains(validated)) {
10099
validations.add(validated);
101100
}
102101
}
103102
}
103+
104+
public ValidationResult consume(ValidationResult other) {
105+
synchronized (validations) {
106+
validations.addAll(other.validations);
107+
}
108+
return this;
109+
}
104110
}

‎modules/openapi-generator-core/src/main/java/org/openapitools/codegen/validation/ValidationRule.java

Lines changed: 75 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -26,7 +26,7 @@ public class ValidationRule {
2626
private Severity severity;
2727
private String description;
2828
private String failureMessage;
29-
private Function<Object, Boolean> test;
29+
private Function<Object, Result> test;
3030

3131
/**
3232
* Constructs a new instance of {@link ValidationRule}
@@ -37,7 +37,7 @@ public class ValidationRule {
3737
* @param test The test condition to be applied as a part of this rule, when this function returns <code>true</code>,
3838
* the evaluated instance will be considered "valid" according to this rule.
3939
*/
40-
ValidationRule(Severity severity, String description, String failureMessage, Function<Object, Boolean> test) {
40+
ValidationRule(Severity severity, String description, String failureMessage, Function<Object, Result> test) {
4141
this.severity = severity;
4242
this.description = description;
4343
this.failureMessage = failureMessage;
@@ -60,7 +60,7 @@ public String getFailureMessage() {
6060
*
6161
* @return <code>true</code> if the object state is valid according to this rule, otherwise <code>false</code>.
6262
*/
63-
public boolean evaluate(Object input) {
63+
public Result evaluate(Object input) {
6464
return test.apply(input);
6565
}
6666

@@ -90,7 +90,7 @@ public String getDescription() {
9090
* @return An "empty" rule.
9191
*/
9292
static ValidationRule empty() {
93-
return new ValidationRule(Severity.ERROR, "empty", "failure message", (i) -> false);
93+
return new ValidationRule(Severity.ERROR, "empty", "failure message", (i) -> Fail.empty() );
9494
}
9595

9696
/**
@@ -106,8 +106,8 @@ static ValidationRule empty() {
106106
* @return A new instance of a {@link ValidationRule}
107107
*/
108108
@SuppressWarnings("unchecked")
109-
public static <T> ValidationRule create(Severity severity, String description, String failureMessage, Function<T, Boolean> fn) {
110-
return new ValidationRule(severity, description, failureMessage, (Function<Object, Boolean>) fn);
109+
public static <T> ValidationRule create(Severity severity, String description, String failureMessage, Function<T, Result> fn) {
110+
return new ValidationRule(severity, description, failureMessage, (Function<Object, Result>) fn);
111111
}
112112

113113
/**
@@ -121,8 +121,8 @@ public static <T> ValidationRule create(Severity severity, String description, S
121121
* @return A new instance of a {@link ValidationRule}
122122
*/
123123
@SuppressWarnings("unchecked")
124-
public static <T> ValidationRule error(String failureMessage, Function<T, Boolean> fn) {
125-
return new ValidationRule(Severity.ERROR, null, failureMessage, (Function<Object, Boolean>) fn);
124+
public static <T> ValidationRule error(String failureMessage, Function<T, Result> fn) {
125+
return new ValidationRule(Severity.ERROR, null, failureMessage, (Function<Object, Result>) fn);
126126
}
127127

128128
/**
@@ -137,8 +137,8 @@ public static <T> ValidationRule error(String failureMessage, Function<T, Boolea
137137
* @return A new instance of a {@link ValidationRule}
138138
*/
139139
@SuppressWarnings("unchecked")
140-
public static <T> ValidationRule warn(String description, String failureMessage, Function<T, Boolean> fn) {
141-
return new ValidationRule(Severity.WARNING, description, failureMessage, (Function<Object, Boolean>) fn);
140+
public static <T> ValidationRule warn(String description, String failureMessage, Function<T, Result> fn) {
141+
return new ValidationRule(Severity.WARNING, description, failureMessage, (Function<Object, Result>) fn);
142142
}
143143

144144
@Override
@@ -149,4 +149,69 @@ public String toString() {
149149
", failureMessage='" + failureMessage + '\'' +
150150
'}';
151151
}
152+
153+
public static abstract class Result {
154+
protected String details = null;
155+
protected Throwable throwable = null;
156+
157+
public String getDetails() {
158+
return details;
159+
}
160+
161+
public void setDetails(String details) {
162+
assert this.details == null;
163+
this.details = details;
164+
}
165+
166+
public abstract boolean passed();
167+
public final boolean failed() { return !passed(); }
168+
169+
public Throwable getThrowable() {
170+
return throwable;
171+
}
172+
173+
public boolean thrown() { return this.throwable == null; }
174+
}
175+
176+
public static final class Pass extends Result {
177+
public static Result empty() { return new Pass(); }
178+
179+
public Pass() {
180+
super();
181+
}
182+
183+
public Pass(String details) {
184+
this();
185+
this.details = details;
186+
}
187+
188+
@Override
189+
public boolean passed() {
190+
return true;
191+
}
192+
}
193+
194+
public static final class Fail extends Result {
195+
public static Result empty() { return new Fail(); }
196+
197+
public Fail() {
198+
super();
199+
}
200+
201+
public Fail(String details) {
202+
this();
203+
this.details = details;
204+
}
205+
206+
public Fail(String details, Throwable throwable) {
207+
this();
208+
this.throwable = throwable;
209+
this.details = details;
210+
}
211+
212+
@Override
213+
public boolean passed() {
214+
return false;
215+
}
216+
}
152217
}

‎modules/openapi-generator-core/src/test/java/org/openapitools/codegen/validation/GenericValidatorTest.java

Lines changed: 16 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -25,7 +25,7 @@
2525
import java.util.Optional;
2626

2727
public class GenericValidatorTest {
28-
class Person {
28+
static class Person {
2929
private int age;
3030
private String name;
3131

@@ -35,33 +35,33 @@ class Person {
3535
}
3636
}
3737

38-
private static boolean isValidAge(Person person) {
39-
return person.age > 0;
38+
private static ValidationRule.Result checkAge(Person person) {
39+
return person.age > 0 ? ValidationRule.Pass.empty() : ValidationRule.Fail.empty();
4040
}
4141

42-
private static boolean isAdult(Person person) {
43-
return person.age > 18;
42+
private static ValidationRule.Result checkAdult(Person person) {
43+
return person.age > 18 ? ValidationRule.Pass.empty() : ValidationRule.Fail.empty();
4444
}
4545

46-
private static boolean isNameSet(Person person) {
47-
return person.name != null && person.name.length() > 0;
46+
private static ValidationRule.Result checkName(Person person) {
47+
return (person.name != null && person.name.length() > 0) ? ValidationRule.Pass.empty() : ValidationRule.Fail.empty();
4848
}
4949

50-
private static boolean isNameValid(Person person) {
50+
private static ValidationRule.Result checkNamePattern(Person person) {
5151
String pattern = "^[A-Z][a-z]*$";
52-
return person.name.matches(pattern);
52+
return person.name.matches(pattern) ? ValidationRule.Pass.empty() : ValidationRule.Fail.empty();
5353
}
5454

55-
private static boolean isNameNormalLength(Person person) {
56-
return person.name.length() < 10;
55+
private static ValidationRule.Result checkNameNormalLength(Person person) {
56+
return person.name.length() < 10? ValidationRule.Pass.empty() : ValidationRule.Fail.empty();
5757
}
5858

5959
private List<ValidationRule> validationRules = Arrays.asList(
60-
ValidationRule.error("Age must be positive and more than zero", GenericValidatorTest::isValidAge),
61-
ValidationRule.error("Only adults (18 years old and older)", GenericValidatorTest::isAdult),
62-
ValidationRule.error("Name isn't set!", GenericValidatorTest::isNameSet),
63-
ValidationRule.error("Name isn't formatted correct", GenericValidatorTest::isNameValid),
64-
ValidationRule.warn("Name too long?", "Name may be too long.", GenericValidatorTest::isNameNormalLength)
60+
ValidationRule.error("Age must be positive and more than zero", GenericValidatorTest::checkAge),
61+
ValidationRule.error("Only adults (18 years old and older)", GenericValidatorTest::checkAdult),
62+
ValidationRule.error("Name isn't set!", GenericValidatorTest::checkName),
63+
ValidationRule.error("Name isn't formatted correct", GenericValidatorTest::checkNamePattern),
64+
ValidationRule.warn("Name too long?", "Name may be too long.", GenericValidatorTest::checkNameNormalLength)
6565
);
6666

6767
@Test

0 commit comments

Comments
 (0)