From 0851cc37ee0437251708fe372db9a3148fcd9bb3 Mon Sep 17 00:00:00 2001 From: Tobias Marstaller Date: Fri, 17 Jul 2020 17:51:15 +0200 Subject: [PATCH 1/7] Issue #302: do not use ReferenceSchema#referredSchema for equals+hashCode --- .../everit/json/schema/ReferenceSchema.java | 3 +- .../json/schema/HashCodeRecursionTest.java | 29 +++++++++++++++++++ .../json/schema/ReferenceSchemaTest.java | 2 +- .../org/everit/jsonvalidator/cyclic.json | 28 ++++++++++++++++++ 4 files changed, 59 insertions(+), 3 deletions(-) create mode 100644 core/src/test/java/org/everit/json/schema/HashCodeRecursionTest.java create mode 100644 core/src/test/resources/org/everit/jsonvalidator/cyclic.json diff --git a/core/src/main/java/org/everit/json/schema/ReferenceSchema.java b/core/src/main/java/org/everit/json/schema/ReferenceSchema.java index 8dae39d9f..3ee81187f 100644 --- a/core/src/main/java/org/everit/json/schema/ReferenceSchema.java +++ b/core/src/main/java/org/everit/json/schema/ReferenceSchema.java @@ -145,7 +145,6 @@ public boolean equals(Object o) { return that.canEqual(this) && Objects.equals(refValue, that.refValue) && Objects.equals(unprocessedProperties, that.unprocessedProperties) && - Objects.equals(referredSchema, that.referredSchema) && Objects.equals(title, that.title) && Objects.equals(description, that.description) && super.equals(that); @@ -156,7 +155,7 @@ public boolean equals(Object o) { @Override public int hashCode() { - return Objects.hash(super.hashCode(), referredSchema, refValue, unprocessedProperties, title, description); + return Objects.hash(super.hashCode(), refValue, unprocessedProperties, title, description); } @Override diff --git a/core/src/test/java/org/everit/json/schema/HashCodeRecursionTest.java b/core/src/test/java/org/everit/json/schema/HashCodeRecursionTest.java new file mode 100644 index 000000000..f702f366f --- /dev/null +++ b/core/src/test/java/org/everit/json/schema/HashCodeRecursionTest.java @@ -0,0 +1,29 @@ +package org.everit.json.schema; + +import org.everit.json.schema.loader.SchemaLoader; +import org.json.JSONObject; +import org.json.JSONTokener; +import org.junit.Test; + +import java.io.IOException; +import java.io.InputStream; + +public class HashCodeRecursionTest +{ + @Test + public void hashCode_should_not_produce_stackoverflow_on_cyclic_schema() throws IOException + { + JSONObject schemaJson; + try (InputStream inStream = getClass().getResourceAsStream("/org/everit/jsonvalidator/cyclic.json")) { + schemaJson = new JSONObject(new JSONTokener(inStream)); + } + + Schema schema = new SchemaLoader.SchemaLoaderBuilder() + .schemaJson(schemaJson) + .build() + .load() + .build(); + + schema.hashCode(); + } +} diff --git a/core/src/test/java/org/everit/json/schema/ReferenceSchemaTest.java b/core/src/test/java/org/everit/json/schema/ReferenceSchemaTest.java index 68456fd11..08b63454b 100644 --- a/core/src/test/java/org/everit/json/schema/ReferenceSchemaTest.java +++ b/core/src/test/java/org/everit/json/schema/ReferenceSchemaTest.java @@ -76,7 +76,7 @@ public void definesPropertyThrowsExc_IfNoReferredSchemaIsSet() { public void equalsVerifier() { EqualsVerifier.forClass(ReferenceSchema.class) .withRedefinedSuperclass() - .withIgnoredFields("schemaLocation", "location") + .withIgnoredFields("schemaLocation", "location", "referredSchema") //there are specifically some non final fields for loading of recursive schemas .suppress(Warning.NONFINAL_FIELDS) .suppress(Warning.STRICT_INHERITANCE) diff --git a/core/src/test/resources/org/everit/jsonvalidator/cyclic.json b/core/src/test/resources/org/everit/jsonvalidator/cyclic.json new file mode 100644 index 000000000..a8b1f2f09 --- /dev/null +++ b/core/src/test/resources/org/everit/jsonvalidator/cyclic.json @@ -0,0 +1,28 @@ +{ + "$schema": "http://json-schema.org/draft-06/schema#", + "type": "object", + "title": "Foo Schema", + "allOf": [ + { + "$ref": "#/definitions/Foo" + } + ], + "definitions": { + "Bar": { + "type": "object", + "properties": { + "foo": { + "$ref": "#/definitions/Foo" + } + } + }, + "Foo": { + "type": "object", + "properties": { + "bar": { + "$ref": "#/definitions/Bar" + } + } + } + } +} From 14ae9d2e76ce2fbf1a3f84724957b1ebd28a3481 Mon Sep 17 00:00:00 2001 From: Tobias Marstaller Date: Fri, 17 Jul 2020 17:54:19 +0200 Subject: [PATCH 2/7] #302: fix unit test name naming --- .java-version | 1 + .../test/java/org/everit/json/schema/HashCodeRecursionTest.java | 2 +- 2 files changed, 2 insertions(+), 1 deletion(-) create mode 100644 .java-version diff --git a/.java-version b/.java-version new file mode 100644 index 000000000..625934097 --- /dev/null +++ b/.java-version @@ -0,0 +1 @@ +1.8 diff --git a/core/src/test/java/org/everit/json/schema/HashCodeRecursionTest.java b/core/src/test/java/org/everit/json/schema/HashCodeRecursionTest.java index f702f366f..a9e1c6a86 100644 --- a/core/src/test/java/org/everit/json/schema/HashCodeRecursionTest.java +++ b/core/src/test/java/org/everit/json/schema/HashCodeRecursionTest.java @@ -11,7 +11,7 @@ public class HashCodeRecursionTest { @Test - public void hashCode_should_not_produce_stackoverflow_on_cyclic_schema() throws IOException + public void hashCodeShouldNotProduceStackoverflowOnCyclicSchema() throws IOException { JSONObject schemaJson; try (InputStream inStream = getClass().getResourceAsStream("/org/everit/jsonvalidator/cyclic.json")) { From 2834a61a767513ed9b19de042783ce37f0c6e773 Mon Sep 17 00:00:00 2001 From: Tobias Marstaller Date: Fri, 17 Jul 2020 17:55:20 +0200 Subject: [PATCH 3/7] #302: remove accidentially added jenv config --- .java-version | 1 - 1 file changed, 1 deletion(-) delete mode 100644 .java-version diff --git a/.java-version b/.java-version deleted file mode 100644 index 625934097..000000000 --- a/.java-version +++ /dev/null @@ -1 +0,0 @@ -1.8 From 844d515b6282c89c330cb411f20a573777df4380 Mon Sep 17 00:00:00 2001 From: Tobias Marstaller Date: Fri, 24 Jul 2020 12:50:48 +0200 Subject: [PATCH 4/7] Issue #302: include ReferenceSchema#referredSchema in equals --- .../everit/json/schema/ReferenceSchema.java | 1 + .../json/schema/HashCodeRecursionTest.java | 19 ++++++++++++++++--- .../json/schema/ReferenceSchemaTest.java | 3 ++- 3 files changed, 19 insertions(+), 4 deletions(-) diff --git a/core/src/main/java/org/everit/json/schema/ReferenceSchema.java b/core/src/main/java/org/everit/json/schema/ReferenceSchema.java index 3ee81187f..40f165343 100644 --- a/core/src/main/java/org/everit/json/schema/ReferenceSchema.java +++ b/core/src/main/java/org/everit/json/schema/ReferenceSchema.java @@ -147,6 +147,7 @@ public boolean equals(Object o) { Objects.equals(unprocessedProperties, that.unprocessedProperties) && Objects.equals(title, that.title) && Objects.equals(description, that.description) && + Objects.equals(referredSchema, that.referredSchema) && super.equals(that); } else { return false; diff --git a/core/src/test/java/org/everit/json/schema/HashCodeRecursionTest.java b/core/src/test/java/org/everit/json/schema/HashCodeRecursionTest.java index a9e1c6a86..946bc4c48 100644 --- a/core/src/test/java/org/everit/json/schema/HashCodeRecursionTest.java +++ b/core/src/test/java/org/everit/json/schema/HashCodeRecursionTest.java @@ -8,22 +8,35 @@ import java.io.IOException; import java.io.InputStream; +import static org.junit.Assert.assertTrue; + public class HashCodeRecursionTest { @Test public void hashCodeShouldNotProduceStackoverflowOnCyclicSchema() throws IOException + { + loadSelfCyclic().hashCode(); + } + + @Test + public void equalsShouldNotProduceStackoverflowOnCyclicSchema() throws IOException + { + Schema cyclic = loadSelfCyclic(); + Schema cyclicCopy = loadSelfCyclic(); + cyclic.equals(cyclicCopy); + } + + private Schema loadSelfCyclic() throws IOException { JSONObject schemaJson; try (InputStream inStream = getClass().getResourceAsStream("/org/everit/jsonvalidator/cyclic.json")) { schemaJson = new JSONObject(new JSONTokener(inStream)); } - Schema schema = new SchemaLoader.SchemaLoaderBuilder() + return new SchemaLoader.SchemaLoaderBuilder() .schemaJson(schemaJson) .build() .load() .build(); - - schema.hashCode(); } } diff --git a/core/src/test/java/org/everit/json/schema/ReferenceSchemaTest.java b/core/src/test/java/org/everit/json/schema/ReferenceSchemaTest.java index 08b63454b..6137c341c 100644 --- a/core/src/test/java/org/everit/json/schema/ReferenceSchemaTest.java +++ b/core/src/test/java/org/everit/json/schema/ReferenceSchemaTest.java @@ -76,10 +76,11 @@ public void definesPropertyThrowsExc_IfNoReferredSchemaIsSet() { public void equalsVerifier() { EqualsVerifier.forClass(ReferenceSchema.class) .withRedefinedSuperclass() - .withIgnoredFields("schemaLocation", "location", "referredSchema") + .withIgnoredFields("schemaLocation", "location") //there are specifically some non final fields for loading of recursive schemas .suppress(Warning.NONFINAL_FIELDS) .suppress(Warning.STRICT_INHERITANCE) + .suppress(Warning.STRICT_HASHCODE) .verify(); } From fe1a51c5000f1ac45acf0165cb2d31af9519e3d1 Mon Sep 17 00:00:00 2001 From: Tobias Marstaller Date: Fri, 24 Jul 2020 14:23:57 +0200 Subject: [PATCH 5/7] Issue #302: improve stability of HashCodeRecursionTest --- core/src/test/resources/org/everit/jsonvalidator/cyclic.json | 1 - 1 file changed, 1 deletion(-) diff --git a/core/src/test/resources/org/everit/jsonvalidator/cyclic.json b/core/src/test/resources/org/everit/jsonvalidator/cyclic.json index a8b1f2f09..2946ae342 100644 --- a/core/src/test/resources/org/everit/jsonvalidator/cyclic.json +++ b/core/src/test/resources/org/everit/jsonvalidator/cyclic.json @@ -1,6 +1,5 @@ { "$schema": "http://json-schema.org/draft-06/schema#", - "type": "object", "title": "Foo Schema", "allOf": [ { From 13438c94bfb348955d2c8b869815b6b203062202 Mon Sep 17 00:00:00 2001 From: Tobias Marstaller Date: Fri, 24 Jul 2020 15:58:16 +0200 Subject: [PATCH 6/7] Issue #302: DRAFT: support ReferenceSchema#equals with cyclic schemas --- .../everit/json/schema/ReferenceSchema.java | 12 +++- .../schema/internal/EqualsCycleBreaker.java | 57 +++++++++++++++++++ .../json/schema/HashCodeRecursionTest.java | 9 +-- .../json/schema/ReferenceSchemaTest.java | 16 +++--- 4 files changed, 78 insertions(+), 16 deletions(-) create mode 100644 core/src/main/java/org/everit/json/schema/internal/EqualsCycleBreaker.java diff --git a/core/src/main/java/org/everit/json/schema/ReferenceSchema.java b/core/src/main/java/org/everit/json/schema/ReferenceSchema.java index 40f165343..6e75f0660 100644 --- a/core/src/main/java/org/everit/json/schema/ReferenceSchema.java +++ b/core/src/main/java/org/everit/json/schema/ReferenceSchema.java @@ -1,11 +1,13 @@ package org.everit.json.schema; -import static java.util.Objects.requireNonNull; +import org.everit.json.schema.internal.EqualsCycleBreaker; import java.util.HashMap; import java.util.Map; import java.util.Objects; +import static java.util.Objects.requireNonNull; + /** * This class is used by {@link org.everit.json.schema.loader.SchemaLoader} to resolve JSON pointers * during the construction of the schema. This class has been made mutable to permit the loading of @@ -147,13 +149,17 @@ public boolean equals(Object o) { Objects.equals(unprocessedProperties, that.unprocessedProperties) && Objects.equals(title, that.title) && Objects.equals(description, that.description) && - Objects.equals(referredSchema, that.referredSchema) && - super.equals(that); + super.equals(that) && + EqualsCycleBreaker.equalsWithoutCycle(this, that, true, ReferenceSchema::equalsPossiblyCyclic); } else { return false; } } + private boolean equalsPossiblyCyclic(ReferenceSchema that) { + return Objects.equals(referredSchema, that.referredSchema); + } + @Override public int hashCode() { return Objects.hash(super.hashCode(), refValue, unprocessedProperties, title, description); diff --git a/core/src/main/java/org/everit/json/schema/internal/EqualsCycleBreaker.java b/core/src/main/java/org/everit/json/schema/internal/EqualsCycleBreaker.java new file mode 100644 index 000000000..d40eac8ce --- /dev/null +++ b/core/src/main/java/org/everit/json/schema/internal/EqualsCycleBreaker.java @@ -0,0 +1,57 @@ +package org.everit.json.schema.internal; + +import java.util.HashSet; +import java.util.Set; +import java.util.WeakHashMap; +import java.util.function.BiFunction; + +public final class EqualsCycleBreaker +{ + private EqualsCycleBreaker() + { + throw new UnsupportedOperationException(); + } + + /** + * For each invocation of a.equals(b) that uses this class, + */ + private static final ThreadLocal> ongoingEqualityChecks = ThreadLocal.withInitial(WeakHashMap::new); + + public static boolean equalsWithoutCycle(T self, T other, boolean equalsOnCycle, BiFunction equalityFunction) { + Set localOngoingEqualityChecks = ongoingEqualityChecks.get() + .computeIfAbsent(new Identity<>(self), (_k) -> new HashSet<>()); + if (localOngoingEqualityChecks.add(other)) { + try { + return equalityFunction.apply(self, other); + } + finally { + localOngoingEqualityChecks.remove(other); + if (localOngoingEqualityChecks.isEmpty()) { + ongoingEqualityChecks.remove(); + } + } + } else { + return equalsOnCycle; + } + } + + private static class Identity { + private final E e; + + public Identity(E e) + { + this.e = e; + } + + public int hashCode() { + return System.identityHashCode(e); + } + + public boolean equals(Object o) { + if (!(o instanceof Identity)) { + return false; + } + return ((Identity) o).e == e; + } + } +} diff --git a/core/src/test/java/org/everit/json/schema/HashCodeRecursionTest.java b/core/src/test/java/org/everit/json/schema/HashCodeRecursionTest.java index 946bc4c48..9aec041d2 100644 --- a/core/src/test/java/org/everit/json/schema/HashCodeRecursionTest.java +++ b/core/src/test/java/org/everit/json/schema/HashCodeRecursionTest.java @@ -8,7 +8,7 @@ import java.io.IOException; import java.io.InputStream; -import static org.junit.Assert.assertTrue; +import static org.junit.Assert.assertEquals; public class HashCodeRecursionTest { @@ -21,9 +21,10 @@ public void hashCodeShouldNotProduceStackoverflowOnCyclicSchema() throws IOExcep @Test public void equalsShouldNotProduceStackoverflowOnCyclicSchema() throws IOException { - Schema cyclic = loadSelfCyclic(); - Schema cyclicCopy = loadSelfCyclic(); - cyclic.equals(cyclicCopy); + CombinedSchema cyclic = (CombinedSchema) loadSelfCyclic(); + CombinedSchema cyclicCopy = (CombinedSchema) loadSelfCyclic(); + + assertEquals(cyclic, cyclicCopy); } private Schema loadSelfCyclic() throws IOException diff --git a/core/src/test/java/org/everit/json/schema/ReferenceSchemaTest.java b/core/src/test/java/org/everit/json/schema/ReferenceSchemaTest.java index 6137c341c..caf8cc5d9 100644 --- a/core/src/test/java/org/everit/json/schema/ReferenceSchemaTest.java +++ b/core/src/test/java/org/everit/json/schema/ReferenceSchemaTest.java @@ -15,21 +15,19 @@ */ package org.everit.json.schema; -import static java.util.Collections.emptyMap; -import static org.everit.json.schema.TestSupport.buildWithLocation; -import static org.junit.Assert.assertEquals; -import static org.junit.Assert.assertTrue; - +import com.google.common.collect.ImmutableMap; +import nl.jqno.equalsverifier.EqualsVerifier; +import nl.jqno.equalsverifier.Warning; import org.everit.json.schema.ReferenceSchema.Builder; import org.everit.json.schema.loader.SchemaLoader; import org.json.JSONObject; import org.junit.Assert; import org.junit.Test; -import com.google.common.collect.ImmutableMap; - -import nl.jqno.equalsverifier.EqualsVerifier; -import nl.jqno.equalsverifier.Warning; +import static java.util.Collections.emptyMap; +import static org.everit.json.schema.TestSupport.buildWithLocation; +import static org.junit.Assert.assertEquals; +import static org.junit.Assert.assertTrue; public class ReferenceSchemaTest { From 631d08d0da57013c43af77cb371d2882db1da4b3 Mon Sep 17 00:00:00 2001 From: Tobias Marstaller Date: Fri, 24 Jul 2020 16:19:37 +0200 Subject: [PATCH 7/7] Issue #302: javadocs --- .../schema/internal/EqualsCycleBreaker.java | 69 ++++++++++++++++++- 1 file changed, 68 insertions(+), 1 deletion(-) diff --git a/core/src/main/java/org/everit/json/schema/internal/EqualsCycleBreaker.java b/core/src/main/java/org/everit/json/schema/internal/EqualsCycleBreaker.java index d40eac8ce..6a6fa153c 100644 --- a/core/src/main/java/org/everit/json/schema/internal/EqualsCycleBreaker.java +++ b/core/src/main/java/org/everit/json/schema/internal/EqualsCycleBreaker.java @@ -13,10 +13,77 @@ private EqualsCycleBreaker() } /** - * For each invocation of a.equals(b) that uses this class, + * ThreadLocal because this class doesn't bother with stack overflows across multiple threads, if that + * is even possible. + *
+ * A weak map so that this consumes less memory. + *
+ * For each ongoing equality check via {@link #equalsWithoutCycle(Object, Object, boolean, BiFunction)}, + * maps the this pointer of the equals invocation to all of the objects it is + * being compared against. Each mapping is removed when `equals` returns. + *
+ * This way, when {@link Object#equals(Object)} is called with the same parameters (this and the other reference) + * a second time before the first invocation has returned (= cyclic!), it can be detected and handled. */ private static final ThreadLocal> ongoingEqualityChecks = ThreadLocal.withInitial(WeakHashMap::new); + /** + * Use to break cycles in equality checks. For example: + * + * 
+     *     class A {
+     *         B b;
+     *
+     *         public boolean equals(Object o) {
+     *             if (!(o instanceof A)) {
+     *                 return false;
+     *             }
+     *
+     *             return this.b.equals(((A) o).b);
+     *         }
+     *     }
+     *     class B {
+     *         int i;
+     *         A a;
+     *
+     *         public boolean equals(Object o) {
+     *             if (!(o instanceof B)) {
+     *                 return false;
+     *             }
+     *
+     *             B that = (B) o;
+     *             if (i != that.i) {
+     *                 return false;
+     *             }
+     *
+     *             return EqualsCycleBreaker.equalsWithoutCycle(this, that, true, B::equalsPossiblyCyclic);
+     *         }
+     *
+     *         private boolean equalsPossiblyCyclic(B that) {
+     *             return this.a.equals(that.a);
+     *         }
+     *     }
+     *
+ * + * If you now construct a cyclic object tree and call equals on it, it will not explode with a stack overflow: + * 
+     *     A a = new A();
+     *     B b = new B();
+     *     b.i = 10;
+     *     b.a = a;
+     *     a.b = b;
+     *
+     *     b.equals(b); // returns true
+     *
+ * + * @param self The receiver of an invocation to {@link Object#equals(Object)}. E.g. in a.equals(b), this + * parameter is a. + * @param other The parameter of an invocation to {@link Object#equals(Object)}. E.g. in a.equals(b), this + * parameter is b. + * @param equalsOnCycle What this method should return when it detects a cycle + * @param equalityFunction The part of the equality check that can cause cyclic invocations / stack overflows. + * @return If this method is called in a cycle, returns equalsOnCycle. Otherwise defers to equalityFunction. + */ public static boolean equalsWithoutCycle(T self, T other, boolean equalsOnCycle, BiFunction equalityFunction) { Set localOngoingEqualityChecks = ongoingEqualityChecks.get() .computeIfAbsent(new Identity<>(self), (_k) -> new HashSet<>());