Merge pull request #405 from paulfrmbstn/issue/404

Fixing #404: Non-deterministic validation result when useDefaults are used with combined schema dependencies

Author: erosb

core/src/main/java/org/everit/json/schema/CombinedSchema.java

@@ -7,6 +7,7 @@
 import java.util.Collection;
 import java.util.List;
 import java.util.Objects;
+import java.util.stream.Collectors;
 
 /**
  * Validator for {@code allOf}, {@code oneOf}, {@code anyOf} schemas.
@@ -167,7 +168,20 @@ public CombinedSchema(Builder builder) {
         super(builder);
         this.synthetic = builder.synthetic;
         this.criterion = requireNonNull(builder.criterion, "criterion cannot be null");
-        this.subschemas = requireNonNull(builder.subschemas, "subschemas cannot be null");
+        this.subschemas = sortByCombinedFirst(requireNonNull(builder.subschemas, "subschemas cannot be null"));
+    }
+
+    private static int compareBySchemaType(Schema lschema, Schema rschema) {
+        return lschema instanceof CombinedSchema ?
+                (rschema instanceof CombinedSchema ? 0 : -1) :
+                (rschema instanceof CombinedSchema ? 1 : 0);
+    }
+
+    // ensure subschemas of type CombinedSchema are always visited first
+    private static Collection<Schema> sortByCombinedFirst(Collection<Schema> schemas) {
+        return schemas.stream()
+                .sorted(CombinedSchema::compareBySchemaType)
+                .collect(Collectors.toCollection(ArrayList::new));
     }
 
     public ValidationCriterion getCriterion() {

core/src/test/java/org/everit/json/schema/CombinedSchemaTest.java

@@ -34,6 +34,53 @@ public class CombinedSchemaTest {
             NumberSchema.builder().multipleOf(10).build(),
             NumberSchema.builder().multipleOf(3).build());
 
+    @Test
+    public void subschemasSort() {
+        CombinedSchema subcombined1 = CombinedSchema
+                .allOf(asList(BooleanSchema.INSTANCE))
+                .build();
+
+        CombinedSchema subcombined2 = CombinedSchema
+                .allOf(asList(NullSchema.INSTANCE))
+                .build();
+
+        BooleanSchema booleanSchema1 = BooleanSchema.INSTANCE;
+        BooleanSchema booleanSchema2 = BooleanSchema.INSTANCE;
+
+        EmptySchema emptySchema1 = EmptySchema.INSTANCE;
+        EmptySchema emptySchema2 = EmptySchema.INSTANCE;
+
+        NullSchema nullSchema1 = NullSchema.INSTANCE;
+        NullSchema nullSchema2 = NullSchema.INSTANCE;
+
+        // subschemas of type CombinedSchema should bubble to the top,
+        // all other schema types should remain in same order
+        CombinedSchema subject = CombinedSchema.builder()
+                .criterion(CombinedSchema.ALL_CRITERION)
+                .subschema(nullSchema1)
+                .subschema(emptySchema1)
+                .subschema(booleanSchema1)
+                .subschema(subcombined1)
+                .subschema(booleanSchema2)
+                .subschema(emptySchema2)
+                .subschema(nullSchema2)
+                .subschema(subcombined2)
+                .isSynthetic(true)
+                .build();
+
+        Object[] subschemas = subject.getSubschemas().toArray();
+
+        assertEquals(8, subschemas.length);
+        assertEquals(subcombined1, subschemas[0]);
+        assertEquals(subcombined2, subschemas[1]);
+        assertEquals(nullSchema1, subschemas[2]);
+        assertEquals(emptySchema1, subschemas[3]);
+        assertEquals(booleanSchema1, subschemas[4]);
+        assertEquals(booleanSchema2, subschemas[5]);
+        assertEquals(emptySchema2, subschemas[6]);
+        assertEquals(nullSchema2, subschemas[7]);
+    }
+
     @Test
     public void allCriterionFailure() {
         assertThrows(ValidationException.class, () -> {

core/src/test/java/org/everit/json/schema/DefaultAndRequiredTest.java

@@ -0,0 +1,43 @@
+package org.everit.json.schema;
+
+import org.everit.json.schema.loader.SchemaLoader;
+import org.json.JSONObject;
+import org.junit.jupiter.api.Test;
+
+import static org.junit.jupiter.api.Assertions.*;
+
+public class DefaultAndRequiredTest {
+
+    private static final ResourceLoader LOADER = ResourceLoader.DEFAULT;
+
+    @Test
+    public void defaultAndRequired() {
+        // tests consistent validation when useDefaults
+        // is enabled and schema contains dependency on property with
+        // default value; see issue #404
+        int expectedExceptions = 20;
+        int actualExceptions = 0;
+
+        for (int i = 0; i < expectedExceptions; i++) {
+            JSONObject jsonSchema = LOADER.readObj("default-and-required-schema.json");
+            JSONObject jsonSpecification = LOADER.readObj("default-and-required.json");
+            Schema schema = SchemaLoader
+                    .builder()
+                    .useDefaults(true)
+                    .schemaJson(jsonSchema)
+                    .build()
+                    .load()
+                    .build();
+
+            try {
+                schema.validate(jsonSpecification);
+            } catch (ValidationException e) {
+                assertEquals(1, e.getCausingExceptions().size());
+                assertTrue(e.getMessage().contains("only 1 subschema matches out of 2"));
+                actualExceptions += 1;
+            }
+        }
+
+        assertEquals(expectedExceptions, actualExceptions);
+    }
+}

core/src/test/resources/org/everit/jsonvalidator/default-and-required-schema.json

@@ -0,0 +1,37 @@
+{
+  "$schema": "http://json-schema.org/draft-07/schema#",
+  "title": "bugtest",
+  "description": "everit bug",
+  "type": "object",
+  "additionalProperties": false,
+  "dependencies": {},
+  "properties": {
+    "arrayPropEnabled": {
+      "type": "boolean"
+    },
+    "arrayProp": {
+      "type": "array",
+      "items": {
+        "type": "number"
+      },
+      "default": [0,1,2,3]
+    }
+  },
+  "allOf": [
+    {
+      "if": {
+        "properties": {
+          "arrayPropEnabled": true
+        }
+      },
+      "then": {
+        "dependencies": {
+          "arrayPropEnabled": [
+            "arrayProp"
+          ]
+        }
+      }
+    }
+  ]
+}
+

core/src/test/resources/org/everit/jsonvalidator/default-and-required.json

@@ -0,0 +1,3 @@
+{
+    "arrayPropEnabled": true
+}

tests/vanilla/src/main/resources/org/everit/json/schema/issues/issue126/subject-valid.json

@@ -1 +1 @@
-{prop: null}
+{"prop": null}