From bcacaf347315e066c72a6da0ba97754962876a65 Mon Sep 17 00:00:00 2001 From: William Cekan Date: Mon, 27 Jan 2020 12:58:04 -0600 Subject: [PATCH] JsonApiDocument hashCode and equals were inconsistent (#1163) * Fix JsonApiDocument hashcode and equals * Handle empty ConstraintViolations * Remove unnecessary initializations * Verify serialization does not change data. * write null object test * Treat data=null as empty collection * Use Optional.map.else to handle resource data * Fix line length --- .../src/main/java/com/yahoo/elide/Elide.java | 2 +- .../elide/core/DataStoreTransaction.java | 4 +- .../com/yahoo/elide/core/RequestScope.java | 2 +- .../elide/jsonapi/models/JsonApiDocument.java | 25 ++--- .../state/RelationshipTerminalState.java | 7 +- .../com/yahoo/elide/jsonapi/JsonApiTest.java | 105 ++++++++++++++++++ 6 files changed, 121 insertions(+), 24 deletions(-) diff --git a/elide-core/src/main/java/com/yahoo/elide/Elide.java b/elide-core/src/main/java/com/yahoo/elide/Elide.java index 3678cc64b2..43d53dea6e 100644 --- a/elide-core/src/main/java/com/yahoo/elide/Elide.java +++ b/elide-core/src/main/java/com/yahoo/elide/Elide.java @@ -313,7 +313,7 @@ protected ElideResponse handleRequest(boolean isReadOnly, Object opaqueUser, } catch (ConstraintViolationException e) { log.debug("Constraint violation exception caught", e); String message = "Constraint violation"; - if (!e.getConstraintViolations().isEmpty()) { + if (e.getConstraintViolations() != null && !e.getConstraintViolations().isEmpty()) { // Return error for the first constraint violation message = e.getConstraintViolations().iterator().next().getMessage(); } diff --git a/elide-core/src/main/java/com/yahoo/elide/core/DataStoreTransaction.java b/elide-core/src/main/java/com/yahoo/elide/core/DataStoreTransaction.java index 11e46a776a..9286603661 100644 --- a/elide-core/src/main/java/com/yahoo/elide/core/DataStoreTransaction.java +++ b/elide-core/src/main/java/com/yahoo/elide/core/DataStoreTransaction.java @@ -102,11 +102,11 @@ default void preCommit() { * @return a new instance of type T */ default T createNewObject(Class entityClass) { - T obj = null; + T obj; try { obj = entityClass.newInstance(); } catch (java.lang.InstantiationException | IllegalAccessException e) { - //do nothing + obj = null; } return obj; } diff --git a/elide-core/src/main/java/com/yahoo/elide/core/RequestScope.java b/elide-core/src/main/java/com/yahoo/elide/core/RequestScope.java index b0b7a40fb2..08fe19b20f 100644 --- a/elide-core/src/main/java/com/yahoo/elide/core/RequestScope.java +++ b/elide-core/src/main/java/com/yahoo/elide/core/RequestScope.java @@ -272,7 +272,7 @@ public Optional getFilterExpressionByType(String type) { public Optional getLoadFilterExpression(Class loadClass) { Optional permissionFilter; permissionFilter = getPermissionExecutor().getReadPermissionFilter(loadClass); - Optional globalFilterExpressionOptional = null; + Optional globalFilterExpressionOptional; if (globalFilterExpression == null) { String typeName = dictionary.getJsonAliasFor(loadClass); globalFilterExpressionOptional = getFilterExpressionByType(typeName); diff --git a/elide-core/src/main/java/com/yahoo/elide/jsonapi/models/JsonApiDocument.java b/elide-core/src/main/java/com/yahoo/elide/jsonapi/models/JsonApiDocument.java index 4171c32683..681d81ff0d 100644 --- a/elide-core/src/main/java/com/yahoo/elide/jsonapi/models/JsonApiDocument.java +++ b/elide-core/src/main/java/com/yahoo/elide/jsonapi/models/JsonApiDocument.java @@ -14,10 +14,12 @@ import java.util.ArrayList; import java.util.Collection; +import java.util.Collections; import java.util.LinkedHashMap; import java.util.LinkedHashSet; import java.util.List; import java.util.Map; +import java.util.Optional; /** * JSON API Document. @@ -46,9 +48,6 @@ public void setData(Data data) { } public Data getData() { - if (data == null) { - return null; - } return data; } @@ -84,12 +83,11 @@ public void addIncluded(Resource resource) { @Override public int hashCode() { + Collection resources = data == null ? null : data.get(); return new HashCodeBuilder(37, 79) - .append(data) - .append(meta) - .append(includedRecs) + .append(resources == null ? 0 : resources.stream().mapToInt(Object::hashCode).sum()) .append(links) - .append(included) + .append(included == null ? 0 : included.stream().mapToInt(Object::hashCode).sum()) .build(); } @@ -99,16 +97,13 @@ public boolean equals(Object obj) { return false; } JsonApiDocument other = (JsonApiDocument) obj; - Collection resources = data.get(); - if ((resources == null || other.getData().get() == null) && resources != other.getData().get()) { + Collection resources = Optional.ofNullable(data).map(Data::get).orElse(Collections.emptySet()); + Collection otherResources = + Optional.ofNullable(other.data).map(Data::get).orElse(Collections.emptySet()); + + if (resources.size() != otherResources.size() || !resources.stream().allMatch(otherResources::contains)) { return false; } - if (resources != null) { - if (resources.size() != other.getData().get().size() - || !resources.stream().allMatch(other.getData().get()::contains)) { - return false; - } - } // TODO: Verify links and meta? if (other.getIncluded() == null) { return included.isEmpty(); diff --git a/elide-core/src/main/java/com/yahoo/elide/parsers/state/RelationshipTerminalState.java b/elide-core/src/main/java/com/yahoo/elide/parsers/state/RelationshipTerminalState.java index 39b3e6ab71..70c1d8c216 100644 --- a/elide-core/src/main/java/com/yahoo/elide/parsers/state/RelationshipTerminalState.java +++ b/elide-core/src/main/java/com/yahoo/elide/parsers/state/RelationshipTerminalState.java @@ -55,13 +55,10 @@ public Supplier> handleGet(StateContext state) { Optional> queryParams = requestScope.getQueryParams(); Map relationships = record.toResourceWithSortingAndPagination().getRelationships(); - Relationship relationship = null; if (relationships != null) { - relationship = relationships.get(relationshipName); - } + Relationship relationship = relationships.get(relationshipName); - // Handle valid relationship - if (relationship != null) { + // Handle valid relationship // Set data Data data = relationship.getData(); diff --git a/elide-core/src/test/java/com/yahoo/elide/jsonapi/JsonApiTest.java b/elide-core/src/test/java/com/yahoo/elide/jsonapi/JsonApiTest.java index b029e6a9ab..323ce405fd 100644 --- a/elide-core/src/test/java/com/yahoo/elide/jsonapi/JsonApiTest.java +++ b/elide-core/src/test/java/com/yahoo/elide/jsonapi/JsonApiTest.java @@ -26,15 +26,19 @@ import com.yahoo.elide.security.User; import com.fasterxml.jackson.core.JsonProcessingException; +import com.google.common.collect.Lists; import com.google.common.collect.Sets; + import example.Child; import example.Parent; import example.TestCheckMappings; + import org.junit.jupiter.api.BeforeAll; import org.junit.jupiter.api.Test; import org.mockito.Answers; import java.io.IOException; +import java.io.UncheckedIOException; import java.util.ArrayList; import java.util.Collections; import java.util.HashSet; @@ -88,8 +92,12 @@ public void writeSingleNoAttributesNoRel() throws JsonProcessingException { String expected = "{\"data\":{\"type\":\"parent\",\"id\":\"123\",\"attributes\":{\"firstName\":null},\"relationships\":{\"children\":{\"data\":[]},\"spouses\":{\"data\":[]}}}}"; + Data data = jsonApiDocument.getData(); String doc = mapper.writeJsonApiDocument(jsonApiDocument); + assertEquals(data, jsonApiDocument.getData()); + assertEquals(expected, doc); + checkEquality(jsonApiDocument); } @Test @@ -108,8 +116,12 @@ public void writeSingle() throws JsonProcessingException { String expected = "{\"data\":{\"type\":\"parent\",\"id\":\"123\",\"attributes\":{\"firstName\":\"bob\"},\"relationships\":{\"children\":{\"data\":[{\"type\":\"child\",\"id\":\"2\"}]},\"spouses\":{\"data\":[]}}}}"; + Data data = jsonApiDocument.getData(); String doc = mapper.writeJsonApiDocument(jsonApiDocument); + assertEquals(data, jsonApiDocument.getData()); + assertEquals(expected, doc); + checkEquality(jsonApiDocument); } @Test @@ -131,8 +143,12 @@ public void writeSingleIncluded() throws JsonProcessingException { String expected = "{\"data\":{\"type\":\"parent\",\"id\":\"123\",\"attributes\":{\"firstName\":\"bob\"},\"relationships\":{\"children\":{\"data\":[{\"type\":\"child\",\"id\":\"2\"}]},\"spouses\":{\"data\":[]}}},\"included\":[{\"type\":\"child\",\"id\":\"2\",\"attributes\":{\"name\":null},\"relationships\":{\"friends\":{\"data\":[]},\"parents\":{\"data\":[{\"type\":\"parent\",\"id\":\"123\"}]}}}]}"; + Data data = jsonApiDocument.getData(); String doc = mapper.writeJsonApiDocument(jsonApiDocument); + assertEquals(data, jsonApiDocument.getData()); + assertEquals(expected, doc); + checkEquality(jsonApiDocument); } @Test @@ -153,8 +169,12 @@ public void writeList() throws JsonProcessingException { String expected = "{\"data\":[{\"type\":\"parent\",\"id\":\"123\",\"attributes\":{\"firstName\":\"bob\"},\"relationships\":{\"children\":{\"data\":[{\"type\":\"child\",\"id\":\"2\"}]},\"spouses\":{\"data\":[]}}}]}"; + Data data = jsonApiDocument.getData(); String doc = mapper.writeJsonApiDocument(jsonApiDocument); + assertEquals(data, jsonApiDocument.getData()); + assertEquals(expected, doc); + checkEquality(jsonApiDocument); } @Test @@ -178,8 +198,12 @@ public void writeListIncluded() throws JsonProcessingException { String expected = "{\"data\":[{\"type\":\"parent\",\"id\":\"123\",\"attributes\":{\"firstName\":\"bob\"},\"relationships\":{\"children\":{\"data\":[{\"type\":\"child\",\"id\":\"2\"}]},\"spouses\":{\"data\":[]}}}],\"included\":[{\"type\":\"child\",\"id\":\"2\",\"attributes\":{\"name\":null},\"relationships\":{\"friends\":{\"data\":[]},\"parents\":{\"data\":[{\"type\":\"parent\",\"id\":\"123\"}]}}}]}"; + Data data = jsonApiDocument.getData(); String doc = mapper.writeJsonApiDocument(jsonApiDocument); + assertEquals(data, jsonApiDocument.getData()); + assertEquals(expected, doc); + checkEquality(jsonApiDocument); } @Test @@ -191,8 +215,12 @@ public void writeEmptyList() throws JsonProcessingException { JsonApiDocument jsonApiDocument = new JsonApiDocument(); jsonApiDocument.setData(empty); + Data data = jsonApiDocument.getData(); String doc = mapper.writeJsonApiDocument(jsonApiDocument); + assertEquals(data, jsonApiDocument.getData()); + assertEquals(expected, doc); + checkEquality(jsonApiDocument); } @Test @@ -204,8 +232,27 @@ public void writeEmptyObject() throws JsonProcessingException { JsonApiDocument jsonApiDocument = new JsonApiDocument(); jsonApiDocument.setData(empty); + Data data = jsonApiDocument.getData(); String doc = mapper.writeJsonApiDocument(jsonApiDocument); + assertEquals(data, jsonApiDocument.getData()); + + assertEquals(expected, doc); + checkEquality(jsonApiDocument); + } + + @Test + public void writeNullObject() throws JsonProcessingException { + String expected = "{\"data\":null}"; + + JsonApiDocument jsonApiDocument = new JsonApiDocument(); + jsonApiDocument.setData(null); + + assertNull(jsonApiDocument.getData()); + String doc = mapper.writeJsonApiDocument(jsonApiDocument); + assertNull(jsonApiDocument.getData()); + assertEquals(expected, doc); + checkEquality(jsonApiDocument); } @Test @@ -224,6 +271,7 @@ public void readSingle() throws IOException { assertEquals("bob", attributes.get("firstName")); assertEquals("child", relations.get("children").getData().getSingleValue().getType()); assertEquals("2", relations.get("children").getData().getSingleValue().getId()); + checkEquality(jsonApiDocument); } @Test @@ -244,6 +292,7 @@ public void readSingleWithMeta() throws IOException { assertEquals(attributes.get("firstName"), "bob"); assertEquals(relations.get("children").getData().getSingleValue().getType(), "child"); assertEquals(relations.get("children").getData().getSingleValue().getId(), "2"); + checkEquality(jsonApiDocument); } @Test @@ -267,6 +316,7 @@ public void readSingleIncluded() throws Exception { assertEquals("child", includedChild.getType()); assertEquals("2", includedChild.getId()); assertEquals("123", parent.getId()); + checkEquality(jsonApiDocument); } @Test @@ -285,6 +335,7 @@ public void readList() throws IOException { assertEquals("bob", attributes.get("firstName")); assertEquals("2", data.getRelationships().get("children").getData().getSingleValue().getId()); assertNull(included); + checkEquality(jsonApiDocument); } @Test @@ -306,6 +357,7 @@ public void readListIncluded() throws IOException { assertEquals("child", includedChild.getType()); assertEquals("2", includedChild.getId()); assertEquals("123", parent.getId()); + checkEquality(jsonApiDocument); } @Test @@ -326,5 +378,58 @@ public void readListWithMeta() throws IOException { assertEquals(attributes.get("firstName"), "bob"); assertEquals(data.getRelationships().get("children").getData().getSingleValue().getId(), "2"); assertNull(included); + checkEquality(jsonApiDocument); + } + + @Test + public void compareNullAndEmpty() throws JsonProcessingException { + Data empty = new Data<>((Resource) null); + + JsonApiDocument jsonApiEmpty = new JsonApiDocument(); + jsonApiEmpty.setData(empty); + + JsonApiDocument jsonApiNull = new JsonApiDocument(); + jsonApiNull.setData(null); + + assertEquals(jsonApiEmpty, jsonApiNull); + assertEquals(jsonApiEmpty.hashCode(), jsonApiNull.hashCode()); + } + + @Test + public void compareOrder() throws JsonProcessingException { + Parent parent1 = new Parent(); + parent1.setId(123L); + Parent parent2 = new Parent(); + parent2.setId(456L); + + PersistentResource pRec1 = new PersistentResource<>(parent1, null, userScope.getUUIDFor(parent1), userScope); + PersistentResource pRec2 = new PersistentResource<>(parent2, null, userScope.getUUIDFor(parent2), userScope); + + JsonApiDocument jsonApiDocument1 = new JsonApiDocument(); + jsonApiDocument1.setData(new Data<>(Lists.newArrayList(pRec1.toResource(), pRec2.toResource()))); + + JsonApiDocument jsonApiDocument2 = new JsonApiDocument(); + jsonApiDocument2.setData(new Data<>(Lists.newArrayList(pRec2.toResource(), pRec1.toResource()))); + + assertEquals(jsonApiDocument1, jsonApiDocument2); + assertEquals(jsonApiDocument1.hashCode(), jsonApiDocument2.hashCode()); + + jsonApiDocument1.getData().sort((a, b) -> Integer.compare(a.hashCode(), b.hashCode())); + jsonApiDocument2.getData().sort((a, b) -> Integer.compare(b.hashCode(), a.hashCode())); + + assertEquals(jsonApiDocument1, jsonApiDocument2); + assertEquals(jsonApiDocument1.hashCode(), jsonApiDocument2.hashCode()); + } + + private void checkEquality(JsonApiDocument doc1) { + JsonApiDocument doc2; + try { + String json = mapper.writeJsonApiDocument(doc1); + doc2 = mapper.readJsonApiDocument(json); + } catch (IOException e) { + throw new UncheckedIOException(e); + } + assertEquals(doc1, doc2); + assertEquals(doc1.hashCode(), doc2.hashCode()); } }