omitting stacktraces for internal exceptions

The purpose of this change is to avoid stacktrace generation for most `ValidationException` instances, since it can come with significant performance improvements in some usecases, see #318 . An `InternalValidationException` is introduced which is a subclass of `ValidationException` but it omits the stacktrace generation by overriding `fillInStackTrace()` to be no-op. This is now thrown everywhere internally. At the end of the validation process the `DefaultValidator` catches the possibly thrown `InternalValidationException` and copies it into a `ValidationInstance` and throws it, so from the caller point of view an exception with a proper stacktrace is thrown. This "public" `ValidationException` is necessary to avoid any potential BC breaks. Tests updated. A good number of unittests and all of the integration tests verify that only root exceptions (and no causing exceptions) have non-empty stacktraces. There is some code duplication between the unit and integ test codebase, this is to be cleaned up a bit in the future (needs a separate testutil module which can be depended on by both `core` and `tests/vanilla`).
everit-org · Sep 12, 2019 · 41dcbd3 · 41dcbd3
1 parent 3c2fb90
commit 41dcbd3
Show file tree

Hide file tree

Showing 13 changed files with 133 additions and 23 deletions.
diff --git a/core/pom.xml b/core/pom.xml
@@ -69,7 +69,7 @@
                         <Export-Package>
                             org.everit.json.schema;version=${project.version},
                             org.everit.json.schema.loader;version=${project.version},
-                            org.everit.json.schema.regexp;version=${project.version}
+                            org.everit.json.schema.regexp;version=${project.version},
                             org.everit.json.schema.event;version=${project.version}
                         </Export-Package>
                     </instructions>

diff --git a/core/src/main/java/org/everit/json/schema/ConditionalSchemaValidatingVisitor.java b/core/src/main/java/org/everit/json/schema/ConditionalSchemaValidatingVisitor.java
@@ -52,7 +52,7 @@ void visitThenSchema(Schema thenSchema) {
         if (ifSchemaException == null) {
             ValidationException thenSchemaException = owner.getFailureOfSchema(thenSchema, subject);
             if (thenSchemaException != null) {
-                ValidationException failure = new ValidationException(conditionalSchema,
+                ValidationException failure = new InternalValidationException(conditionalSchema,
                         new StringBuilder(new StringBuilder("#")),
                         "input is invalid against the \"then\" schema",
                         asList(thenSchemaException),
@@ -72,7 +72,7 @@ void visitElseSchema(Schema elseSchema) {
         if (ifSchemaException != null) {
             ValidationException elseSchemaException = owner.getFailureOfSchema(elseSchema, subject);
             if (elseSchemaException != null) {
-                ValidationException failure = new ValidationException(conditionalSchema,
+                ValidationException failure = new InternalValidationException(conditionalSchema,
                         new StringBuilder(new StringBuilder("#")),
                         "input is invalid against both the \"if\" and \"else\" schema",
                         asList(ifSchemaException, elseSchemaException),

diff --git a/core/src/main/java/org/everit/json/schema/InternalValidationException.java b/core/src/main/java/org/everit/json/schema/InternalValidationException.java
@@ -0,0 +1,30 @@
+package org.everit.json.schema;
+
+import java.util.List;
+
+class InternalValidationException extends ValidationException {
+
+    InternalValidationException(Schema violatedSchema, Class<?> expectedType, Object actualValue) {
+        super(violatedSchema, expectedType, actualValue);
+    }
+
+    InternalValidationException(Schema violatedSchema, Class<?> expectedType, Object actualValue, String keyword,
+            String schemaLocation) {
+        super(violatedSchema, expectedType, actualValue, keyword, schemaLocation);
+    }
+
+    InternalValidationException(Schema violatedSchema, String message, String keyword, String schemaLocation) {
+        super(violatedSchema, message, keyword, schemaLocation);
+    }
+
+    InternalValidationException(Schema violatedSchema, StringBuilder pointerToViolation, String message,
+            List<ValidationException> causingExceptions, String keyword, String schemaLocation) {
+        super(violatedSchema, pointerToViolation, message, causingExceptions, keyword, schemaLocation);
+    }
+
+    @Override
+    public synchronized Throwable fillInStackTrace() {
+        return this;
+    }
+
+}
diff --git a/core/src/main/java/org/everit/json/schema/ValidatingVisitor.java b/core/src/main/java/org/everit/json/schema/ValidatingVisitor.java
@@ -164,7 +164,7 @@ void visitCombinedSchema(CombinedSchema combinedSchema) {
         try {
             criterion.validate(subschemas.size(), matchingCount);
         } catch (ValidationException e) {
-            failureReporter.failure(new ValidationException(combinedSchema,
+            failureReporter.failure(new InternalValidationException(combinedSchema,
                     new StringBuilder(e.getPointerToViolation()),
                     e.getMessage(),
                     failures,

diff --git a/core/src/main/java/org/everit/json/schema/ValidationException.java b/core/src/main/java/org/everit/json/schema/ValidationException.java
@@ -20,6 +20,8 @@
 public class ValidationException extends RuntimeException {
     private static final long serialVersionUID = 6192047123024651924L;
 
+
+
     private static int getViolationCount(List<ValidationException> causes) {
         int causeCount = causes.stream().mapToInt(ValidationException::getViolationCount).sum();
         return Math.max(1, causeCount);
@@ -67,7 +69,7 @@ public static void throwFor(Schema rootFailingSchema,
     }
 
     static ValidationException createWrappingException(Schema rootFailingSchema, List<ValidationException> failures) {
-        return new ValidationException(rootFailingSchema,
+        return new InternalValidationException(rootFailingSchema,
                 new StringBuilder("#"),
                 getViolationCount(failures) + " schema violations found",
                 new ArrayList<>(failures),
@@ -393,7 +395,7 @@ public ValidationException prepend(String fragment, Schema violatedSchema) {
         List<ValidationException> prependedCausingExceptions = causingExceptions.stream()
                 .map(exc -> exc.prepend(escapedFragment))
                 .collect(Collectors.toList());
-        return new ValidationException(newPointer, violatedSchema, super.getMessage(),
+        return new InternalValidationException(violatedSchema, newPointer, super.getMessage(),
                 prependedCausingExceptions, this.keyword, this.schemaLocation);
     }
 
@@ -479,4 +481,9 @@ public String getSchemaLocation() {
         result = 31 * result + (keyword == null ? 0 : keyword.hashCode());
         return result;
     }
+
+    ValidationException copy() {
+        return new ValidationException(pointerToViolation, violatedSchema, super.getMessage(), causingExceptions,
+                keyword, schemaLocation);
+    }
 }
diff --git a/core/src/main/java/org/everit/json/schema/ValidationFailureReporter.java b/core/src/main/java/org/everit/json/schema/ValidationFailureReporter.java
@@ -18,11 +18,11 @@ abstract class ValidationFailureReporter {
     }
 
     void failure(String message, String keyword) {
-        failure(new ValidationException(schema, message, keyword, schema.getSchemaLocation()));
+        failure(new InternalValidationException(schema, message, keyword, schema.getSchemaLocation()));
     }
 
     void failure(Class<?> expectedType, Object actualValue) {
-        failure(new ValidationException(schema, expectedType, actualValue, "type", schema.getSchemaLocation()));
+        failure(new InternalValidationException(schema, expectedType, actualValue, "type", schema.getSchemaLocation()));
     }
 
     abstract void failure(ValidationException exc);

diff --git a/core/src/main/java/org/everit/json/schema/Validator.java b/core/src/main/java/org/everit/json/schema/Validator.java
@@ -66,8 +66,12 @@ class DefaultValidator implements Validator {
         ValidationFailureReporter failureReporter = createFailureReporter(schema);
         ReadWriteValidator readWriteValidator = ReadWriteValidator.createForContext(readWriteContext, failureReporter);
         ValidatingVisitor visitor = new ValidatingVisitor(input, failureReporter, readWriteValidator, validationListener);
-        visitor.visit(schema);
-        visitor.failIfErrorFound();
+        try {
+            visitor.visit(schema);
+            visitor.failIfErrorFound();
+        } catch (InternalValidationException e) {
+            throw e.copy();
+        }
     }
 
     private ValidationFailureReporter createFailureReporter(Schema schema) {

diff --git a/core/src/test/java/org/everit/json/schema/ConditionalSchemaEventsTest.java b/core/src/test/java/org/everit/json/schema/ConditionalSchemaEventsTest.java
@@ -60,7 +60,7 @@ public void ifMatch_thenMismatch() {
         validateInstance(instance);
 
         verify(listener).ifSchemaMatch(new ConditionalSchemaMatchEvent(schema, instance, IF));
-        ValidationException failure = new ValidationException(MIN_LENGTH_STRING_SCHEMA,
+        ValidationException failure = new InternalValidationException(MIN_LENGTH_STRING_SCHEMA,
                 "expected minLength: 6, actual: 3", "minLength",
                 "#/then");
         verify(listener).thenSchemaMismatch(new ConditionalSchemaMismatchEvent(schema, instance, THEN, failure));
@@ -72,8 +72,8 @@ public void ifMismatch_elseMatch() {
         String instance = "boo";
         validateInstance(instance);
 
-        ValidationException failure = new ValidationException(PATTERN_STRING_SCHEMA, "string [boo] does not match pattern f.*o",
-                "pattern", "#/if");
+        ValidationException failure = new InternalValidationException(PATTERN_STRING_SCHEMA,
+                "string [boo] does not match pattern f.*o", "pattern", "#/if");
         verify(listener).ifSchemaMismatch(new ConditionalSchemaMismatchEvent(schema, instance, IF, failure));
         verify(listener).elseSchemaMatch(new ConditionalSchemaMatchEvent(schema, instance, ELSE));
         verifyNoMoreInteractions(listener);
@@ -84,11 +84,11 @@ public void ifMismatch_elseMismatch() {
         String instance = "booooooooooooo";
         validateInstance(instance);
 
-        ValidationException ifFailure = new ValidationException(PATTERN_STRING_SCHEMA,
+        ValidationException ifFailure = new InternalValidationException(PATTERN_STRING_SCHEMA,
                 "string [booooooooooooo] does not match pattern f.*o",
                 "pattern", "#/if");
         verify(listener).ifSchemaMismatch(new ConditionalSchemaMismatchEvent(schema, instance, IF, ifFailure));
-        ValidationException elseFailure = new ValidationException(MAX_LENGTH_STRING_SCHEMA,
+        ValidationException elseFailure = new InternalValidationException(MAX_LENGTH_STRING_SCHEMA,
                 "expected maxLength: 4, actual: 14",
                 "maxLength", "#/else");
         verify(listener).elseSchemaMismatch(new ConditionalSchemaMismatchEvent(schema, instance, ELSE, elseFailure));

diff --git a/core/src/test/java/org/everit/json/schema/InternalValidationExceptionTest.java b/core/src/test/java/org/everit/json/schema/InternalValidationExceptionTest.java
@@ -0,0 +1,17 @@
+package org.everit.json.schema;
+
+import static org.junit.Assert.assertEquals;
+
+import org.junit.Test;
+
+public class InternalValidationExceptionTest {
+
+    @Test
+    public void stackTraceShouldBeEmpty() {
+        try {
+            throw new InternalValidationException(BooleanSchema.INSTANCE, "message", "keyword", "#");
+        } catch (ValidationException e) {
+            assertEquals(0, e.getStackTrace().length);
+        }
+    }
+}
diff --git a/core/src/test/java/org/everit/json/schema/TestSupport.java b/core/src/test/java/org/everit/json/schema/TestSupport.java
@@ -17,13 +17,15 @@
 
 import static org.hamcrest.CoreMatchers.containsString;
 import static org.junit.Assert.assertEquals;
+import static org.junit.Assert.assertNotEquals;
 import static org.junit.Assert.assertSame;
 import static org.junit.Assert.assertThat;
 import static org.junit.Assert.fail;
 
 import java.io.ByteArrayInputStream;
 import java.io.InputStream;
 import java.util.List;
+import java.util.Optional;
 
 import org.everit.json.schema.loader.SchemaLoader;
 
@@ -163,6 +165,7 @@ public static void expectFailure(final Schema failingSchema,
             test(failingSchema, expectedPointer, input);
         } catch (ValidationException e) {
             assertSame(expectedViolatedSchema, e.getViolatedSchema());
+            verifyStacktraces(e);
         }
     }
 
@@ -185,9 +188,28 @@ public static void expectFailure(final Failure failure) {
             if (failure.expectedMessageFragment != null) {
                 assertThat(e.getMessage(), containsString(failure.expectedMessageFragment));
             }
+            verifyStacktraces(e);
         }
     }
 
+    private static void verifyStacktraces(ValidationException e) {
+        assertNotEquals(0, e.getStackTrace().length);
+        assertEmptyCauseStackTraces(e).ifPresent(nonempty -> {
+            throw new AssertionError("non-empty stacktrace: " + nonempty);
+        });
+    }
+
+    private static Optional<ValidationException> assertEmptyCauseStackTraces(ValidationException e) {
+        return e.getCausingExceptions().stream().filter(exc -> exc.getStackTrace().length > 0)
+                .findFirst()
+                .map(Optional::of)
+                .orElseGet(() -> e.getCausingExceptions().stream()
+                        .map(TestSupport::assertEmptyCauseStackTraces)
+                        .filter(Optional::isPresent)
+                        .findFirst()
+                        .orElse(Optional.empty()));
+    }
+
     public static final InputStream asStream(final String string) {
         return new ByteArrayInputStream(string.getBytes());
     }

diff --git a/core/src/test/java/org/everit/json/schema/ValidatingVisitorTest.java b/core/src/test/java/org/everit/json/schema/ValidatingVisitorTest.java
@@ -133,7 +133,7 @@ public void triggersCombinedSchemaEvents() {
 
         new ValidatingVisitor(instance, reporter, ReadWriteValidator.NONE, listener).visit(combinedSchema);
 
-        ValidationException exc = new ValidationException(stringSchema, String.class, instance);
+        ValidationException exc = new InternalValidationException(stringSchema, String.class, instance);
         verify(listener).combinedSchemaMismatch(new CombinedSchemaMismatchEvent(combinedSchema, stringSchema, instance, exc));
         verify(listener).combinedSchemaMatch(new CombinedSchemaMatchEvent(combinedSchema, emptySchema, instance));
         verify(listener).combinedSchemaMatch(new CombinedSchemaMatchEvent(combinedSchema, objectSchema, instance));

diff --git a/core/src/test/java/org/everit/json/schema/ValidationExceptionTest.java b/core/src/test/java/org/everit/json/schema/ValidationExceptionTest.java
@@ -15,6 +15,7 @@
  */
 package org.everit.json.schema;
 
+import static java.util.Collections.emptyList;
 import static org.everit.json.schema.JSONMatcher.sameJsonAs;
 import static org.hamcrest.MatcherAssert.assertThat;
 import static org.junit.Assert.assertEquals;
@@ -40,7 +41,7 @@ public void constructorNullSchema() {
     private ValidationException createDummyException(final String pointer) {
         return new ValidationException(BooleanSchema.INSTANCE,
                 new StringBuilder(pointer),
-                "stuff went wrong", Collections.emptyList());
+                "stuff went wrong", emptyList());
     }
 
     @Test
@@ -112,7 +113,7 @@ public void prependWithCausingExceptions() {
 
     private ValidationException subjectWithCauses(final ValidationException... causes) {
         if (causes.length == 0) {
-            return new ValidationException("");
+            return new ValidationException(rootSchema, "", emptyList());
         }
         try {
             ValidationException.throwFor(rootSchema, Arrays.asList(causes));
@@ -169,7 +170,7 @@ public void throwForMultipleFailures() {
 
     @Test
     public void throwForNoFailure() {
-        ValidationException.throwFor(rootSchema, Collections.emptyList());
+        ValidationException.throwFor(rootSchema, emptyList());
     }
 
     @Test
@@ -195,7 +196,7 @@ public void toStringWithCauses() {
     public void testToJSON() {
         ValidationException subject =
                 new ValidationException(BooleanSchema.INSTANCE, new StringBuilder("#/a/b"),
-                        "exception message", Collections.emptyList(), "type", null);
+                        "exception message", emptyList(), "type", null);
         JSONObject expected = loader.readObj("exception-to-json.json");
         JSONObject actual = subject.toJSON();
         assertThat(actual, sameJsonAs(expected));
@@ -205,7 +206,7 @@ public void testToJSON() {
     public void testToJSONWithSchemaLocation() {
         ValidationException subject =
                 new ValidationException(BooleanSchema.INSTANCE, new StringBuilder("#/a/b"),
-                        "exception message", Collections.emptyList(), "type", "#/schema/location");
+                        "exception message", emptyList(), "type", "#/schema/location");
         JSONObject expected = loader.readObj("exception-to-json-with-schema-location.json");
         JSONObject actual = subject.toJSON();
         assertThat(actual, sameJsonAs(expected));
@@ -215,7 +216,7 @@ public void testToJSONWithSchemaLocation() {
     public void toJSONNullPointerToViolation() {
         ValidationException subject =
                 new ValidationException(BooleanSchema.INSTANCE, null,
-                        "exception message", Collections.emptyList(), "type", null);
+                        "exception message", emptyList(), "type", null);
         JSONObject actual = subject.toJSON();
         assertEquals(JSONObject.NULL, actual.get("pointerToViolation"));
     }
@@ -226,7 +227,7 @@ public void toJSONWithCauses() {
                 new ValidationException(NullSchema.INSTANCE,
                         new StringBuilder("#/a/0"),
                         "cause msg",
-                        Collections.emptyList(),
+                        emptyList(),
                         "type",
                         null);
         ValidationException subject =
@@ -237,4 +238,11 @@ public void toJSONWithCauses() {
         assertThat(actual, sameJsonAs(expected));
     }
 
+    @Test
+    public void testCopy() {
+        ValidationException subject = subjectWithCauses(subjectWithCauses());
+        ValidationException copy = subject.copy();
+        assertEquals(subject, copy);
+    }
+
 }
diff --git a/tests/vanilla/src/main/java/org/everit/json/schema/TestCase.java b/tests/vanilla/src/main/java/org/everit/json/schema/TestCase.java
@@ -1,11 +1,14 @@
 package org.everit.json.schema;
 
+import static org.junit.Assert.assertNotEquals;
+
 import java.io.IOException;
 import java.io.InputStream;
 import java.io.InputStreamReader;
 import java.io.UncheckedIOException;
 import java.util.ArrayList;
 import java.util.List;
+import java.util.Optional;
 import java.util.Set;
 import java.util.regex.Pattern;
 
@@ -89,9 +92,28 @@ private void testWithValidator(Validator validator, Schema schema) {
             if (expectedToBeValid) {
                 throw new AssertionError("false failure for " + inputDescription, e);
             }
+            verifyStacktraces(e);
         }
     }
 
+    private static void verifyStacktraces(ValidationException e) {
+        assertNotEquals(0, e.getStackTrace().length);
+        assertEmptyCauseStackTraces(e).ifPresent(nonempty -> {
+            throw new AssertionError("non-empty stacktrace: " + nonempty);
+        });
+    }
+
+    private static Optional<ValidationException> assertEmptyCauseStackTraces(ValidationException e) {
+        return e.getCausingExceptions().stream().filter(exc -> exc.getStackTrace().length > 0)
+                .findFirst()
+                .map(Optional::of)
+                .orElseGet(() -> e.getCausingExceptions().stream()
+                        .map(TestCase::assertEmptyCauseStackTraces)
+                        .filter(Optional::isPresent)
+                        .findFirst()
+                        .orElse(Optional.empty()));
+    }
+
     public void loadSchema(SchemaLoader.SchemaLoaderBuilder loaderBuilder) {
         try {
             SchemaLoader loader = loaderBuilder.schemaJson(schemaJson).build();