[GitHub] [lucene-solr] madrob commented on a change in pull request #1728: SOLR-14596: equals/hashCode for common SolrRequest classes

[GitBox]

madrob commented on a change in pull request #1728:
URL: https://github.com/apache/lucene-solr/pull/1728#discussion_r468716670



##########
File path: solr/solrj/src/java/org/apache/solr/client/solrj/SolrRequest.java
##########
@@ -244,6 +247,8 @@ public String getBasePath() {
   public void addHeader(String key, String value) {
     if (headers == null) {
       headers = new HashMap<>();
+      final HashMap<String, String> asdf = new HashMap<>();

Review comment:
       what?

##########
File path: 
solr/solrj/src/test/org/apache/solr/client/solrj/request/TestUpdateRequest.java
##########
@@ -17,52 +17,164 @@
 package org.apache.solr.client.solrj.request;
 
 import java.util.Arrays;
+import java.util.List;
 
+import com.google.common.collect.Lists;
 import org.apache.solr.common.SolrInputDocument;
 import org.junit.Before;
 import org.junit.Rule;
 import org.junit.Test;
 import org.junit.rules.ExpectedException;
 
+import static org.apache.solr.SolrTestCaseJ4.adoc;
+import static org.junit.Assert.assertEquals;
+import static org.junit.Assert.assertNotEquals;
+
 public class TestUpdateRequest {
 
   @Rule
   public ExpectedException exception = ExpectedException.none();
 
   @Before
   public void expectException() {
-    exception.expect(NullPointerException.class);
-    exception.expectMessage("Cannot add a null SolrInputDocument");
   }
 
   @Test
   public void testCannotAddNullSolrInputDocument() {
+    exception.expect(NullPointerException.class);
+    exception.expectMessage("Cannot add a null SolrInputDocument");
+
     UpdateRequest req = new UpdateRequest();
     req.add((SolrInputDocument) null);
   }
 
   @Test
   public void testCannotAddNullDocumentWithOverwrite() {
+    exception.expect(NullPointerException.class);
+    exception.expectMessage("Cannot add a null SolrInputDocument");
+
     UpdateRequest req = new UpdateRequest();
     req.add(null, true);
   }
 
   @Test
   public void testCannotAddNullDocumentWithCommitWithin() {
+    exception.expect(NullPointerException.class);
+    exception.expectMessage("Cannot add a null SolrInputDocument");
+
     UpdateRequest req = new UpdateRequest();
     req.add(null, 1);
   }
 
   @Test
   public void testCannotAddNullDocumentWithParameters() {
+    exception.expect(NullPointerException.class);
+    exception.expectMessage("Cannot add a null SolrInputDocument");
+
     UpdateRequest req = new UpdateRequest();
     req.add(null, 1, true);
   }
 
   @Test
   public void testCannotAddNullDocumentAsPartOfList() {
+    exception.expect(NullPointerException.class);
+    exception.expectMessage("Cannot add a null SolrInputDocument");
+
     UpdateRequest req = new UpdateRequest();
     req.add(Arrays.asList(new SolrInputDocument(), new SolrInputDocument(), 
null));
   }
 
+  @Test
+  public void testEqualsMethod() {
+    final SolrInputDocument doc1 = new SolrInputDocument("id", "1", "value_s", 
"foo");
+    final SolrInputDocument doc2 = new SolrInputDocument("id", "2", "value_s", 
"bar");
+    final SolrInputDocument doc3 = new SolrInputDocument("id", "3", "value_s", 
"baz");
+/*

Review comment:
       left over from other testing?

##########
File path: 
solr/solrj/src/test/org/apache/solr/client/solrj/request/TestUpdateRequest.java
##########
@@ -17,52 +17,164 @@
 package org.apache.solr.client.solrj.request;
 
 import java.util.Arrays;
+import java.util.List;
 
+import com.google.common.collect.Lists;
 import org.apache.solr.common.SolrInputDocument;
 import org.junit.Before;
 import org.junit.Rule;
 import org.junit.Test;
 import org.junit.rules.ExpectedException;
 
+import static org.apache.solr.SolrTestCaseJ4.adoc;
+import static org.junit.Assert.assertEquals;
+import static org.junit.Assert.assertNotEquals;
+
 public class TestUpdateRequest {
 
   @Rule
   public ExpectedException exception = ExpectedException.none();
 
   @Before
   public void expectException() {
-    exception.expect(NullPointerException.class);

Review comment:
       remove the whole method?

##########
File path: solr/solrj/src/java/org/apache/solr/client/solrj/ResponseParser.java
##########
@@ -49,4 +52,31 @@ public String getVersion()
   {
     return "2.2";
   }
+
+  @Override
+  public int hashCode() {
+    return new HashCodeBuilder()
+        .append(getWriterType())
+        .append(getContentType())
+        .append(getVersion())
+        .toHashCode();
+  }
+
+  @Override
+  public boolean equals(Object rhs) {
+    if (rhs == null || getClass() != rhs.getClass()) {
+      return false;
+    } else if (this == rhs) {
+      return true;
+    } else if (hashCode() != rhs.hashCode()) {
+      return false;
+    }
+
+    final ResponseParser rhsCast = (ResponseParser) rhs;

Review comment:
       I think I prefer Objects.hash, but I'm not sure why? Definitely willing 
to be convinced the other way if there's a reason or a difference or even if 
they're equivalent and there is already inertia here.




----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscr...@lucene.apache.org
For additional commands, e-mail: issues-h...@lucene.apache.org