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

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();
             }

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> T createNewObject(Class<T> entityClass) {
-        T obj = null;
+        T obj;
         try {
             obj = entityClass.newInstance();
         } catch (java.lang.InstantiationException | IllegalAccessException e) {
-            //do nothing
+            obj = null;
         }
         return obj;
     }

elide-core/src/main/java/com/yahoo/elide/core/RequestScope.java

@@ -272,7 +272,7 @@ public Optional<FilterExpression> getFilterExpressionByType(String type) {
     public Optional<FilterExpression> getLoadFilterExpression(Class<?> loadClass) {
         Optional<FilterExpression> permissionFilter;
         permissionFilter = getPermissionExecutor().getReadPermissionFilter(loadClass);
-        Optional<FilterExpression> globalFilterExpressionOptional = null;
+        Optional<FilterExpression> globalFilterExpressionOptional;
         if (globalFilterExpression == null) {
             String typeName = dictionary.getJsonAliasFor(loadClass);
             globalFilterExpressionOptional =  getFilterExpressionByType(typeName);

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<Resource> data) {
     }
 
     public Data<Resource> getData() {
-        if (data == null) {
-            return null;
-        }
         return data;
     }
 
@@ -84,12 +83,11 @@ public void addIncluded(Resource resource) {
 
     @Override
     public int hashCode() {
+        Collection<Resource> 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<Resource> resources = data.get();
-        if ((resources == null || other.getData().get() == null) && resources != other.getData().get()) {
+        Collection<Resource> resources = Optional.ofNullable(data).map(Data::get).orElse(Collections.emptySet());
+        Collection<Resource> 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();

elide-core/src/main/java/com/yahoo/elide/parsers/state/RelationshipTerminalState.java

@@ -55,13 +55,10 @@ public Supplier<Pair<Integer, JsonNode>> handleGet(StateContext state) {
         Optional<MultivaluedMap<String, String>> queryParams = requestScope.getQueryParams();
 
         Map<String, Relationship> 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<Resource> data = relationship.getData();

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<Resource> 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<Resource> 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<Resource> 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<Resource> 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<Resource> 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<Resource> 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<Resource> 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<Resource> 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<Parent> pRec1 = new PersistentResource<>(parent1, null, userScope.getUUIDFor(parent1), userScope);
+        PersistentResource<Parent> 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());
     }
 }