Solr
  1. Solr
  2. SOLR-5837

Add missing equals implementation for SolrDocument, SolrInputDocument and SolrInputField.

    Details

    • Type: Improvement Improvement
    • Status: Resolved
    • Priority: Major Major
    • Resolution: Won't Fix
    • Affects Version/s: None
    • Fix Version/s: None
    • Component/s: None
    • Labels:
      None

      Description

      While working on SOLR-5265 I tried comparing objects of SolrDocument, SolrInputDocument and SolrInputField. These classes did not Override the equals implementation.

      The issue will Override equals and hashCode methods to the 3 classes.

      1. SOLR-5837.patch
        4 kB
        Varun Thacker
      2. SOLR-5837.patch
        7 kB
        Varun Thacker

        Activity

        Hide
        Varun Thacker added a comment -

        Patch Overrides equals() and hashCode() methods of SolrDocument, SolrInputDocument and SolrInputField.

        Show
        Varun Thacker added a comment - Patch Overrides equals() and hashCode() methods of SolrDocument, SolrInputDocument and SolrInputField.
        Hide
        Mark Miller added a comment -

        Thanks Varun! We should add a couple tests for this as well.

        Show
        Mark Miller added a comment - Thanks Varun! We should add a couple tests for this as well.
        Hide
        Yonik Seeley added a comment -

        I think it's probably OK to add these methods for testing purposes, but we should not guarantee equals/hashcode of these classes (think about things like streaming based Reader fields, etc).

        Show
        Yonik Seeley added a comment - I think it's probably OK to add these methods for testing purposes, but we should not guarantee equals/hashcode of these classes (think about things like streaming based Reader fields, etc).
        Hide
        Varun Thacker added a comment -

        New patch with tests.

        I didn't know whether to create a separate class for all 3 equality tests, so currently I have added it to DocumentBuilderTest

        Show
        Varun Thacker added a comment - New patch with tests. I didn't know whether to create a separate class for all 3 equality tests, so currently I have added it to DocumentBuilderTest
        Hide
        ASF subversion and git services added a comment -

        Commit 1575886 from Noble Paul in branch 'dev/trunk'
        [ https://svn.apache.org/r1575886 ]

        SOLR-5837 Added .equals method for SolrDocument, SolrInputDocument and SolrInputField

        Show
        ASF subversion and git services added a comment - Commit 1575886 from Noble Paul in branch 'dev/trunk' [ https://svn.apache.org/r1575886 ] SOLR-5837 Added .equals method for SolrDocument, SolrInputDocument and SolrInputField
        Hide
        ASF subversion and git services added a comment -

        Commit 1575891 from Noble Paul in branch 'dev/branches/branch_4x'
        [ https://svn.apache.org/r1575891 ]

        SOLR-5837 Added .equals method for SolrDocument, SolrInputDocument and SolrInputField

        Show
        ASF subversion and git services added a comment - Commit 1575891 from Noble Paul in branch 'dev/branches/branch_4x' [ https://svn.apache.org/r1575891 ] SOLR-5837 Added .equals method for SolrDocument, SolrInputDocument and SolrInputField
        Hide
        Mark Miller added a comment -

        Yonik's comment still needs to be addressed - the doc for the new hashcode / equals methods needs to explain its limitations and that they are just for testing purposes.

        Show
        Mark Miller added a comment - Yonik's comment still needs to be addressed - the doc for the new hashcode / equals methods needs to explain its limitations and that they are just for testing purposes.
        Hide
        Mark Miller added a comment -

        The following also should be addressed:

        + } catch (IOException e) { 
        + //TODO fail test? 
        + } 
        
        Show
        Mark Miller added a comment - The following also should be addressed: + } catch (IOException e) { + //TODO fail test? + }
        Hide
        ASF subversion and git services added a comment -

        Commit 1576004 from Mark Miller in branch 'dev/trunk'
        [ https://svn.apache.org/r1576004 ]

        SOLR-5837: Clean up issue: Add hashCode/equals to SolrDocument, SolrInputDocument and SolrInputField for testing purposes.

        Show
        ASF subversion and git services added a comment - Commit 1576004 from Mark Miller in branch 'dev/trunk' [ https://svn.apache.org/r1576004 ] SOLR-5837 : Clean up issue: Add hashCode/equals to SolrDocument, SolrInputDocument and SolrInputField for testing purposes.
        Hide
        ASF subversion and git services added a comment -

        Commit 1576005 from Mark Miller in branch 'dev/branches/branch_4x'
        [ https://svn.apache.org/r1576005 ]

        SOLR-5837: Clean up issue: Add hashCode/equals to SolrDocument, SolrInputDocument and SolrInputField for testing purposes.

        Show
        ASF subversion and git services added a comment - Commit 1576005 from Mark Miller in branch 'dev/branches/branch_4x' [ https://svn.apache.org/r1576005 ] SOLR-5837 : Clean up issue: Add hashCode/equals to SolrDocument, SolrInputDocument and SolrInputField for testing purposes.
        Hide
        Varun Thacker added a comment -

        [~hakeber] Looks like you cleaned up code from both this issue and SOLR-5265. Thanks
        One small nit - BIN_FILE_LOCATION in TestJavaBinCodec is not static

        Show
        Varun Thacker added a comment - [~hakeber] Looks like you cleaned up code from both this issue and SOLR-5265 . Thanks One small nit - BIN_FILE_LOCATION in TestJavaBinCodec is not static
        Hide
        Noble Paul added a comment -

        The equals and hashCode() will fail only when you are streaming a document . Otherwise it is perfectly OK to use them

        Show
        Noble Paul added a comment - The equals and hashCode() will fail only when you are streaming a document . Otherwise it is perfectly OK to use them
        Hide
        Mark Miller added a comment -

        Exactly - you point out why they should not be used other than for tests.

        We don't want code that works except when it screws you.

        Show
        Mark Miller added a comment - Exactly - you point out why they should not be used other than for tests. We don't want code that works except when it screws you.
        Hide
        Mark Miller added a comment -

        I'm going to reopen this. It still bugs me - I think perhaps we should do with this with test code instead. It's what I was originally thinking - even the javadoc warnings don't make me feel warm and fuzzy about this. I was chatting with Robert about it this morning, and that confirmed my feelings that adding hashCode/equals to these classes just for tests is the wrong move.

        Show
        Mark Miller added a comment - I'm going to reopen this. It still bugs me - I think perhaps we should do with this with test code instead. It's what I was originally thinking - even the javadoc warnings don't make me feel warm and fuzzy about this. I was chatting with Robert about it this morning, and that confirmed my feelings that adding hashCode/equals to these classes just for tests is the wrong move.
        Hide
        Robert Muir added a comment -

        My recommendation in a case like this: if you want to use it in testing, would be to convert the logic to some assertEquals type helper methods (e.g. in a test base class like SolrTestCaseJ4).

        Show
        Robert Muir added a comment - My recommendation in a case like this: if you want to use it in testing, would be to convert the logic to some assertEquals type helper methods (e.g. in a test base class like SolrTestCaseJ4).
        Hide
        Varun Thacker added a comment -

        So I guess we should revert this then and I will work on a new assert* method and provide a new patch on SOLR-5265?

        Show
        Varun Thacker added a comment - So I guess we should revert this then and I will work on a new assert* method and provide a new patch on SOLR-5265 ?
        Hide
        Noble Paul added a comment -

        Yes, remove the equals and hashCode from the respective classes and add this to the test classes

        Show
        Noble Paul added a comment - Yes, remove the equals and hashCode from the respective classes and add this to the test classes
        Hide
        ASF subversion and git services added a comment -

        Commit 1577491 from Noble Paul in branch 'dev/trunk'
        [ https://svn.apache.org/r1577491 ]

        SOLR-5837 remove .equals from Solr doc objects

        Show
        ASF subversion and git services added a comment - Commit 1577491 from Noble Paul in branch 'dev/trunk' [ https://svn.apache.org/r1577491 ] SOLR-5837 remove .equals from Solr doc objects
        Hide
        ASF subversion and git services added a comment -

        Commit 1577500 from Noble Paul in branch 'dev/branches/branch_4x'
        [ https://svn.apache.org/r1577500 ]

        SOLR-5837 remove .equals from Solr doc objects

        Show
        ASF subversion and git services added a comment - Commit 1577500 from Noble Paul in branch 'dev/branches/branch_4x' [ https://svn.apache.org/r1577500 ] SOLR-5837 remove .equals from Solr doc objects
        Hide
        Noble Paul added a comment -

        The equals methods were added to testcases . So ,this is not required

        Show
        Noble Paul added a comment - The equals methods were added to testcases . So ,this is not required
        Hide
        Mark Miller added a comment -

        If you look at the previous commits, you will see that this also had a CHANGES entry introduced under Other that needs to be removed.

        Show
        Mark Miller added a comment - If you look at the previous commits, you will see that this also had a CHANGES entry introduced under Other that needs to be removed.

          People

          • Assignee:
            Noble Paul
            Reporter:
            Varun Thacker
          • Votes:
            0 Vote for this issue
            Watchers:
            6 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development