Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Optional (id) field cannot be verified #80

Closed
GoogleCodeExporter opened this issue Mar 29, 2015 · 5 comments
Closed

Optional (id) field cannot be verified #80

GoogleCodeExporter opened this issue Mar 29, 2015 · 5 comments

Comments

@GoogleCodeExporter
Copy link

In my application we store objects in a DB.
Therefore I want to use the (primary key) ID field as the identity of the 
objects. However, until the object is persisted the ID is not set and I rely on 
the java object-identity instead.
The classes equals and hashcode methods are displayed below.

The problem is that EqualsVerifier throws an exception and suggests to 
'consider suppressing Warning.IDENTICAL_COPY'. After all, by design, two new 
objects with identical fields are not equal (for my application this is fine 
and I can live with this duplication).

However, following up on this suggestion is not helpful. Since two identical 
copies with IDs other than 0 are equal. So I get the message:
java.lang.AssertionError: Unnecessary suppression: IDENTICAL_COPY. Two 
identical copies are equal.

Finally EqualsVerifier.forExamples(..) without an id=0 case does not resolve 
this issue, I still see that equals is invoked with a case where id=0.



    @Override
    public int hashCode() {
        final int prime = 31;
        int result = 1;
        if (id == 0L) {
            return super.hashCode();
        }
        result = prime * result + (int)(id ^ (id >>> 32));
        return result;
    }

    @Override
    public boolean equals(Object obj) {
        if (this == obj) {
            return true;
        }
        if (obj == null) {
            return false;
        }
        if (getClass() != obj.getClass()) {
            return false;
        }
        VersionedEntity other = (VersionedEntity)obj;
        if (id == 0L && other.id == 0L) {
            return super.equals(obj);
        }
        if (id != other.id) {
            return false;
        }
        return true;
    }

Original issue reported on code.google.com by [email protected] on 6 May 2013 at 12:48

@GoogleCodeExporter
Copy link
Author

When I added the Identical Copy check, I didn't think of this use case. I did 
some testing, and it does look like it's valid, in the sense that it follows 
all the rules of the contract. So I consider the fact that your class doesn't 
pass as a bug.

That said, I'm not sure yet how best to solve this:
* just remove the "unnecessary suppression" check?
* add a new modifier specifically for this use case?

There are pros and cons to both solutions, and maybe there's a third one I 
haven't thought of yet.

To be continued!

Original comment by [email protected] on 9 May 2013 at 8:32

  • Changed state: Accepted
  • Added labels: ****
  • Removed labels: ****

@GoogleCodeExporter
Copy link
Author

Original comment by [email protected] on 20 May 2013 at 11:47

  • Added labels: ****
  • Removed labels: ****

Attachments:

@GoogleCodeExporter
Copy link
Author

Hi Jan,

First EqualsVerifier is detecting non-equal identical objects. This is an 
assumption from EV about what my business logic should look like, but will 
mostly be right.
If I disable the warning EV warns about equal identical objects. Again an 
assumption (based on disabling the first warning) what my business logic looks 
like, that will mostly be correct.

Since my case doesn't comply with 'mostly' I would like to prevent EV from 
making assumptions about what is equality in my model.
Therefore, *an option to forgo these checks would have my preference*. Since 
both detections are probably valid in most cases.

That is one vote for a new modifier.

(As a side-note: with default equals/hashcode identical objects are not equal, 
but I don't get the warning, how is this possible?)


Original comment by [email protected] on 23 May 2013 at 7:58

  • Added labels: ****
  • Removed labels: ****

@GoogleCodeExporter
Copy link
Author

Hi Tim,

I think I agree with you. My vision for EqualsVerifier is that it's an 
opinionated piece of software that helps its users write correct equals methods 
that are as simple as possible. Sometimes, "simple" isn't correct, and 
sometimes opinions simply differ. In those situations, warnings can be 
suppressed, but only to a point. I think the first assumption is still valid. 
Like you, I think the assumption made when suppressing the IDENTICAL_COPY 
warning, is also still valid. Hence, a new modifier seems to be the only 
solution. So I will add this modifier to the next version.

Regarding your side-note, it's really simple, really: EV checks whether 
equals/hashCode are inherited directly from Object. If so, it simply skips some 
checks :). I did this because it makes the EV code a lot simpler, and Object's 
equals and hashCode methods are correct anyway.

Original comment by [email protected] on 26 May 2013 at 2:24

  • Added labels: ****
  • Removed labels: ****

@GoogleCodeExporter
Copy link
Author

I've added the modifier IDENTICAL_COPY_FOR_VERSIONED_ENTITY in version 1.3.1, 
which is just released.

Original comment by [email protected] on 9 Jun 2013 at 3:23

  • Changed state: Fixed
  • Added labels: ****
  • Removed labels: ****

jqno pushed a commit that referenced this issue Aug 23, 2016
Fix up small spelling mistake in ad code.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

1 participant