From e269712e7d085c1b95646eb08868106dc321b3b9 Mon Sep 17 00:00:00 2001 From: pvdb Date: Thu, 9 Jan 2025 12:07:18 +0100 Subject: [PATCH] code review (incl renaming and doc updates) --- .../guide/validator/core/ApiFunctions.java | 39 ++++++++++-------- .../validator/core/util/SchemaValidator.java | 40 ++++++++----------- ...le-ExamplesShouldValidateAgainstSchema.drl | 10 ++--- .../rules/oas/Rule-ReadOnlyProperties.drl | 2 +- ...sOfRequiredShouldBeDefinedAsProperties.drl | 4 +- 5 files changed, 47 insertions(+), 48 deletions(-) diff --git a/core/src/main/java/io/github/belgif/rest/guide/validator/core/ApiFunctions.java b/core/src/main/java/io/github/belgif/rest/guide/validator/core/ApiFunctions.java index aa003b5..7810b3f 100644 --- a/core/src/main/java/io/github/belgif/rest/guide/validator/core/ApiFunctions.java +++ b/core/src/main/java/io/github/belgif/rest/guide/validator/core/ApiFunctions.java @@ -112,31 +112,35 @@ private static Set getSubSchemas(SchemaDefinition schemaDefini return subSchemas; } - private static Set getRecursiveSubSchemas(SchemaDefinition schemaDefinition, Parser.ParserResult result, boolean includeTopLevelSchemas) { + /** + * @return all subschemas of given schema, including recursively referenced ones + */ + private static Set getAllSubSchemas(SchemaDefinition schemaDefinition, Parser.ParserResult result, boolean includeTopLevelSchemas) { Set subSchemas = getSubSchemas(schemaDefinition, result, includeTopLevelSchemas); Set output = new HashSet<>(); for (SchemaDefinition schema : subSchemas) { - output.addAll(getRecursiveSubSchemas(schema, result, includeTopLevelSchemas)); + output.addAll(getAllSubSchemas(schema, result, includeTopLevelSchemas)); } output.addAll(subSchemas); return output; } /** - * Returns all the properties (schemas) in a complex schema + * Returns all the properties (with their schemas) at top-level of a possibly composite schema. + * Properties only reachable via discriminator are not added, unless valueNode parameter is used. * * @param result Result from validationparser * @param schema Schema of which the properties have to be returned - * @param exampleNode JsonNode of an example that can be used to also collect all properties based on a discriminator value + * @param valueNode optional JSON value compliant with the schema */ - public static Map getRecursiveProperties(Schema schema, Parser.ParserResult result, JsonNode exampleNode) { + public static Map getAllProperties(Schema schema, Parser.ParserResult result, JsonNode valueNode) { Map properties = new HashMap<>(); SchemaDefinition schemaDef = recursiveResolve(schema, result); Set definitions = new HashSet<>(); definitions.add(schemaDef); - definitions.addAll(getRecursiveSubSchemas(schemaDef, result, true)); - if (exampleNode != null) { - definitions.addAll(findDiscriminatorSchemas(definitions, result, exampleNode)); + definitions.addAll(getAllSubSchemas(schemaDef, result, true)); + if (valueNode != null) { + definitions.addAll(findDiscriminatorSchemas(definitions, result, valueNode)); } definitions.forEach(schemaDefinition -> { if (schemaDefinition.getModel().getProperties() != null) { @@ -146,19 +150,20 @@ public static Map getRecursiveProperties(Schema schema, Parser.P return properties; } - private static Set findDiscriminatorSchemas(Set schemaDefinitions, Parser.ParserResult result, JsonNode exampleNode) { + private static Set findDiscriminatorSchemas(Set schemaDefinitions, Parser.ParserResult result, JsonNode valueNode) { Set schemaDefinitionsWithDiscriminators = schemaDefinitions.stream() - .filter(def -> def.getModel().getDiscriminator() != null && exampleNode.has(def.getModel().getDiscriminator().getPropertyName())) + .filter(def -> def.getModel().getDiscriminator() != null && valueNode.has(def.getModel().getDiscriminator().getPropertyName())) .collect(Collectors.toSet()); Set schemaDefinitionMappings = new HashSet<>(); for (SchemaDefinition schemaDefinition : schemaDefinitionsWithDiscriminators) { String discriminator = schemaDefinition.getModel().getDiscriminator().getPropertyName(); - String discriminatorValue = exampleNode.get(discriminator).asText(); + String discriminatorValue = valueNode.get(discriminator).asText(); String mapping; if (schemaDefinition.getModel().getDiscriminator().getMapping() != null) { mapping = schemaDefinition.getModel().getDiscriminator().getMapping().get(discriminatorValue); } else { + // TODO: if mapping present, but property value not found in discriminator mapping, also fallback to this mapping = "#/components/schemas/" + discriminatorValue; } @@ -166,7 +171,7 @@ private static Set findDiscriminatorSchemas(Set resolvedDef = result.resolve(mapping, schemaDefinition.getOpenApiFile()); SchemaDefinition schemaDef = (SchemaDefinition) resolvedDef; schemaDefinitionMappings.add(schemaDef); - schemaDefinitionMappings.addAll(ApiFunctions.getRecursiveSubSchemas(schemaDef, result, true)); + schemaDefinitionMappings.addAll(ApiFunctions.getAllSubSchemas(schemaDef, result, true)); // TODO: move this to caller? } catch (RuntimeException e) { log.error("Cannot find schema for discriminator {} : {}", discriminator, discriminatorValue); } @@ -174,13 +179,13 @@ private static Set findDiscriminatorSchemas(Set getRequiredValues(SchemaDefinition schemaDefinition, Parser.ParserResult result) { - Set requiredValues = new HashSet<>(); + public static Set getRequiredProperties(SchemaDefinition schemaDefinition, Parser.ParserResult result) { + Set requiredProperties = new HashSet<>(); if (schemaDefinition.getModel() != null && schemaDefinition.getModel().getRequired() != null) { - requiredValues.addAll(schemaDefinition.getModel().getRequired()); + requiredProperties.addAll(schemaDefinition.getModel().getRequired()); } - getRecursiveSubSchemas(schemaDefinition, result, false).forEach(schema -> requiredValues.addAll(getRequiredValues(schema, result))); - return requiredValues; + getAllSubSchemas(schemaDefinition, result, false).forEach(schema -> requiredProperties.addAll(getRequiredProperties(schema, result))); + return requiredProperties; } public static boolean isPropertyRequiredAndReadOnly(SchemaDefinition schemaDefinition, String propertyName, Parser.ParserResult result) { diff --git a/core/src/main/java/io/github/belgif/rest/guide/validator/core/util/SchemaValidator.java b/core/src/main/java/io/github/belgif/rest/guide/validator/core/util/SchemaValidator.java index 833ba1a..6dbe76c 100644 --- a/core/src/main/java/io/github/belgif/rest/guide/validator/core/util/SchemaValidator.java +++ b/core/src/main/java/io/github/belgif/rest/guide/validator/core/util/SchemaValidator.java @@ -36,27 +36,31 @@ public class SchemaValidator { private SchemaValidator() { } - public static List getNonDefinedProperties(ExampleDefinition exampleDefinition) { + public static List getUndefinedProperties(ExampleDefinition exampleDefinition) { SchemaDefinition schemaDefinition = getSchemaDefinition(exampleDefinition); ExampleDefinition example = (ExampleDefinition) exampleDefinition.getResult().resolve(exampleDefinition.getModel()); JsonNode exampleNode = getExampleNode(example); - if (schemaDefinition.getModel().getAdditionalPropertiesBoolean() != null) { //Other example validator will point out issues when additionalProperties is specifically set to false or true + if (schemaDefinition.getModel().getAdditionalPropertiesBoolean() != null) { + // Properties are only considered as undefined when additionalProperties are not explicitly allowed in the schema + // if additionalProperties is false, example schema validation will mark any undefined properties as invalid, so can be ignored here + + // TODO: composite with additional properties? What if in nested object only? - move to fromObjectNode? return new ArrayList<>(); } - return getNonDefinedProperties(exampleNode, schemaDefinition); + return getUndefinedProperties(exampleNode, schemaDefinition); } - private static List getNonDefinedProperties(JsonNode exampleNode, SchemaDefinition schemaDefinition) { + private static List getUndefinedProperties(JsonNode exampleNode, SchemaDefinition schemaDefinition) { List missingProperties = new ArrayList<>(); - missingProperties.addAll(getNonDefinedPropertiesFromArrayNode(exampleNode, schemaDefinition)); - missingProperties.addAll(getNonDefinedPropertiesFromObjectNode(exampleNode, schemaDefinition)); + missingProperties.addAll(getUndefinedPropertiesFromArrayNode(exampleNode, schemaDefinition)); + missingProperties.addAll(getUndefinedPropertiesFromObjectNode(exampleNode, schemaDefinition)); return missingProperties; } - private static List getNonDefinedPropertiesFromObjectNode(JsonNode exampleNode, SchemaDefinition schemaDefinition) { + private static List getUndefinedPropertiesFromObjectNode(JsonNode exampleNode, SchemaDefinition schemaDefinition) { List missingProperties = new ArrayList<>(); - Map definedProperties = ApiFunctions.getRecursiveProperties(schemaDefinition.getModel(), schemaDefinition.getResult(), exampleNode); + Map definedProperties = ApiFunctions.getAllProperties(schemaDefinition.getModel(), schemaDefinition.getResult(), exampleNode); Iterator exampleFieldNames = exampleNode.fieldNames(); while (exampleFieldNames.hasNext()) { @@ -65,33 +69,23 @@ private static List getNonDefinedPropertiesFromObjectNode(JsonNode examp missingProperties.add(exampleFieldName + " not found in: #" + schemaDefinition.getPrintableJsonPointer()); } else { SchemaDefinition def = (SchemaDefinition) schemaDefinition.getResult().resolve(definedProperties.get(exampleFieldName)); - if (subSchemaShouldBeInspectedFurther(def)) { - missingProperties.addAll(getNonDefinedProperties(exampleNode.get(exampleFieldName), def)); - } + missingProperties.addAll(getUndefinedProperties(exampleNode.get(exampleFieldName), def)); } } return missingProperties; } - private static List getNonDefinedPropertiesFromArrayNode(JsonNode exampleNode, SchemaDefinition schemaDefinition) { + private static List getUndefinedPropertiesFromArrayNode(JsonNode exampleNode, SchemaDefinition schemaDefinition) { List missingProperties = new ArrayList<>(); - if (schemaDefinition.getModel().getType() != null && schemaDefinition.getModel().getType().equals(Schema.SchemaType.ARRAY)) { + if (schemaDefinition.getModel().getType() != null && schemaDefinition.getModel().getType().equals(Schema.SchemaType.ARRAY)) { // TODO: What if schema is an allOf of an array and a description? SchemaDefinition arrayItemSchemaDefinition = (SchemaDefinition) schemaDefinition.getResult().resolve(schemaDefinition.getModel().getItems()); - if (subSchemaShouldBeInspectedFurther(arrayItemSchemaDefinition)) { - for (JsonNode arrayItem : exampleNode) { - missingProperties.addAll(getNonDefinedProperties(arrayItem, arrayItemSchemaDefinition)); - } + for (JsonNode arrayItem : exampleNode) { //TODO: should we test exampleNode.isArray here (returns properties if it's on object)? + missingProperties.addAll(getUndefinedProperties(arrayItem, arrayItemSchemaDefinition)); } } return missingProperties; } - private static boolean subSchemaShouldBeInspectedFurther(SchemaDefinition schemaDefinition) { - Schema schema = schemaDefinition.getModel(); - return (schema.getType() != null && (schema.getType().equals(Schema.SchemaType.OBJECT) || schema.getType().equals(Schema.SchemaType.ARRAY))) || - schema.getAllOf() != null || schema.getAnyOf() != null || schema.getOneOf() != null; - } - public static String getExampleViolations(ExampleDefinition exampleDefinition) { try { SchemaDefinition schemaDefinition = getSchemaDefinition(exampleDefinition); diff --git a/rules/src/main/resources/io/github/belgif/rest/guide/validator/rules/oas/Rule-ExamplesShouldValidateAgainstSchema.drl b/rules/src/main/resources/io/github/belgif/rest/guide/validator/rules/oas/Rule-ExamplesShouldValidateAgainstSchema.drl index 9dd70a6..7246bc6 100644 --- a/rules/src/main/resources/io/github/belgif/rest/guide/validator/rules/oas/Rule-ExamplesShouldValidateAgainstSchema.drl +++ b/rules/src/main/resources/io/github/belgif/rest/guide/validator/rules/oas/Rule-ExamplesShouldValidateAgainstSchema.drl @@ -17,9 +17,9 @@ function void violationExamplesShouldValidateAgainstSchema(ViolationReport oas, "Example does not validate against schema", violation, example); } -function void violationExamplesShouldNotDefineNonExistingProperties(ViolationReport oas, ExampleDefinition example, String violation) { - oas.addViolation("[exam-prop]", - "Property used in example not present in schema: ", violation, example, ViolationLevel.RECOMMENDED); +function void violationExamplesShouldNotHaveUndefinedProperties(ViolationReport oas, ExampleDefinition example, String violation) { + oas.addViolation("[oas-exampl]", + "Example shouldn't contain undefined property", violation, example, ViolationLevel.RECOMMENDED); } rule "Example should be valid against schema" @@ -33,7 +33,7 @@ end rule "Example should not contain undefined properties" when $example: ExampleDefinition(definitionType.equals(DefinitionType.INLINE)) - $violation: String() from SchemaValidator.getNonDefinedProperties($example) + $violation: String() from SchemaValidator.getUndefinedProperties($example) then - violationExamplesShouldNotDefineNonExistingProperties(oas, $example, $violation); + violationExamplesShouldNotHaveUndefinedProperties(oas, $example, $violation); end \ No newline at end of file diff --git a/rules/src/main/resources/io/github/belgif/rest/guide/validator/rules/oas/Rule-ReadOnlyProperties.drl b/rules/src/main/resources/io/github/belgif/rest/guide/validator/rules/oas/Rule-ReadOnlyProperties.drl index c59f35a..091edb4 100644 --- a/rules/src/main/resources/io/github/belgif/rest/guide/validator/rules/oas/Rule-ReadOnlyProperties.drl +++ b/rules/src/main/resources/io/github/belgif/rest/guide/validator/rules/oas/Rule-ReadOnlyProperties.drl @@ -20,7 +20,7 @@ function void violationReadOnlyProperties(ViolationReport oas, SchemaDefinition rule "ReadOnly Properties" when $schema: SchemaDefinition(model.getRef() == null) - Map($property: /entrySet) from ApiFunctions.getRecursiveProperties($schema.getModel(), parserResult, null) + Map($property: /entrySet) from ApiFunctions.getAllProperties($schema.getModel(), parserResult, null) $propertyKey: String() from $property.getKey() eval ( ApiFunctions.isPropertyRequiredAndReadOnly($schema, $propertyKey, parserResult) ) then diff --git a/rules/src/main/resources/io/github/belgif/rest/guide/validator/rules/oas/Rule-ValuesOfRequiredShouldBeDefinedAsProperties.drl b/rules/src/main/resources/io/github/belgif/rest/guide/validator/rules/oas/Rule-ValuesOfRequiredShouldBeDefinedAsProperties.drl index 24d0b8f..a03ede6 100644 --- a/rules/src/main/resources/io/github/belgif/rest/guide/validator/rules/oas/Rule-ValuesOfRequiredShouldBeDefinedAsProperties.drl +++ b/rules/src/main/resources/io/github/belgif/rest/guide/validator/rules/oas/Rule-ValuesOfRequiredShouldBeDefinedAsProperties.drl @@ -20,8 +20,8 @@ function void violationRequiredProperties(ViolationReport oas, SchemaDefinition rule "Values of required should be defined as properties" when $schema: SchemaDefinition(isHighLevelSchema()) - $required: String() from ApiFunctions.getRequiredValues($schema, parserResult) - not Map( containsKey($required) ) from ApiFunctions.getRecursiveProperties($schema.getModel(), parserResult, null) + $required: String() from ApiFunctions.getRequiredProperties($schema, parserResult) + not Map( containsKey($required) ) from ApiFunctions.getAllProperties($schema.getModel(), parserResult, null) then violationRequiredProperties(oas, $schema, $required); end \ No newline at end of file