You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@lucene.apache.org by GitBox <gi...@apache.org> on 2020/08/08 01:59:10 UTC
[GitHub] [lucene-solr] gerlowskija opened a new pull request #1728: SOLR-14596: equals/hashCode for common SolrRequest classes
gerlowskija opened a new pull request #1728:
URL: https://github.com/apache/lucene-solr/pull/1728
# Description
Currently, many SolrRequest classes (e.g. UpdateRequest) don't implement `equals()` or `hashCode()`. This isn't a problem for Solr's normal operation, but it can be a barrier in unit testing SolrJ client code. `equals()` implementations would make it much easier to assert that client code is building the request that developers think it's building.
# Solution
This takes the first baby step towards remedying this by adding `equals` and `hashCode` implementations for a few SolrRequest implementations. Specifically the base SolrRequest, UpdateRequest, and CollectionAdminRequest and all its subclasses.
In an effort to simplify, I've standardized on using EqualsBuilder/HashCodeBuilder for these classes. It relies on apache-commons, but since that's already a dependency of SolrJ I don't think this is a problem. It results in much simpler code than writing the required logic by hand.
# Tests
None yet but some forthcoming.
# Checklist
Please review the following and check all that apply:
- [X] I have reviewed the guidelines for [How to Contribute](https://wiki.apache.org/solr/HowToContribute) and my code conforms to the standards described there to the best of my ability.
- [X] I have created a Jira issue and added the issue ID to my pull request title.
- [X] I have given Solr maintainers [access](https://help.github.com/en/articles/allowing-changes-to-a-pull-request-branch-created-from-a-fork) to contribute to my PR branch. (optional but recommended)
- [X] I have developed this patch against the `master` branch.
- [ ] I have run `ant precommit` and the appropriate test suite.
- [ ] I have added tests for my changes.
- [X] I have added documentation for the [Ref Guide](https://github.com/apache/lucene-solr/tree/master/solr/solr-ref-guide) (for Solr changes only).
----------------------------------------------------------------
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
[GitHub] [lucene-solr] ErickErickson commented on a change in pull request #1728: SOLR-14596: equals/hashCode for common SolrRequest classes
Posted by GitBox <gi...@apache.org>.
ErickErickson commented on a change in pull request #1728:
URL: https://github.com/apache/lucene-solr/pull/1728#discussion_r467457703
##########
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'm curious about the comparison between Objects.hash(bunch of params) and the commons builder. I'm not advocating you change to Objects.hash, rather wondering whether there's a particular advantage to using one over the other.
----------------------------------------------------------------
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
[GitHub] [lucene-solr] gerlowskija commented on a change in pull request #1728: SOLR-14596: equals/hashCode for common SolrRequest classes
Posted by GitBox <gi...@apache.org>.
gerlowskija commented on a change in pull request #1728:
URL: https://github.com/apache/lucene-solr/pull/1728#discussion_r471012967
##########
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 didn't make an intentional choice against Objects.hash - I'm just more familiar with HashCodeBuilder, so that's what I used.
Anyone have a concrete reason to prefer one vs the other? If not, I'll do a little digging to see if one has advantages and report back.
Also relevant to this discussion is that @dsmiley made an argument in the [jira ticket](https://issues.apache.org/jira/browse/SOLR-14596) that hashCode impls aren't worth the maintenance burden and should be skipped here. Since you guys showed some interest in the PR, either of you want to cast a vote there to settle things out more conclusively? (I don't agree personally with his argument, but am happy to go with a clear consensus.)
----------------------------------------------------------------
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
[GitHub] [lucene-solr] gerlowskija commented on pull request #1728: SOLR-14596: equals/hashCode for common SolrRequest classes
Posted by GitBox <gi...@apache.org>.
gerlowskija commented on pull request #1728:
URL: https://github.com/apache/lucene-solr/pull/1728#issuecomment-674422652
> Were these autogenerated? By which IDE?
They weren't, no. But in many cases they could be. I skipped autogeneration because in a lot of cases I needed to dig into the instance variables in question to understand (1) whether they were relevant for equality, and (2) whether the instance variable types had adequate hashCode/equals methods themselves (e.g. SolrResponse.equals relies on ResponseParser.equals which didn't exist).
But I've got nothing against autogenerated methods if they're double-checked, and I might go that route in other parts of SolrJ. 🤷
----------------------------------------------------------------
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
[GitHub] [lucene-solr] gerlowskija commented on a change in pull request #1728: SOLR-14596: equals/hashCode for common SolrRequest classes
Posted by GitBox <gi...@apache.org>.
gerlowskija commented on a change in pull request #1728:
URL: https://github.com/apache/lucene-solr/pull/1728#discussion_r471013282
##########
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:
Yes. My bad - the tests are a bit WIP/experimental at this point. I ran out of weekend-time partially into starting them. I made a comment about this in the jira, but should've mentioned it here where it'd come more easily to the attention of reviewers.
----------------------------------------------------------------
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
[GitHub] [lucene-solr] madrob commented on a change in pull request #1728: SOLR-14596: equals/hashCode for common SolrRequest classes
Posted by GitBox <gi...@apache.org>.
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org