@EqualsAndHashCode for domain objects with identity

[Jeff Schnitzer]

For JPA (ie Hibernate) domain objects, it's usual to have an identity
field like this:

class Person
{
@Id Long id;
@Column(nullable=false) String name;
}

For nearly all objects of this type, the appropriate equals()
implementation is: "if the classes are the same and the ids are the
same". hashCode() should just defer to the id.

How about including this intelligence in lombok? Some possibilities:

1) Just "do the right thing" when it finds a field annotated with
javax.persistence.Id.

2) Specify the identity key explicitly, ie @EqualsAndHashCode
(identity="id")

3) Add an include parameter that would work with exclude similar to
the way that ant filesets work, ie @EqualsAndHashCode(include={"id"}).

I'm fond of both #1 and #3.

Incidentally, compound primary keys are encapuslated in a class, so
there is still a single id object (which itself must implement equals
() and hashCode()).

Jeff

[Reinier Zwitserloot]

Tricky. I'm all for doing the right thing automatically - in fact, in
the trunk, Roel, before he went on vacation, added code to 'do the
right thing' in the face of @NotNull/@NonNull annotations on fields
with @Getter/@Setter. A null check is added, and the annotation is
copied to the parameter (setters) and return type (getters). The
problem with doing the right thing automatically is that it becomes
rather difficult for folks to figure out what the heck's going on. I'm
convinced we need some sort of tool to print the result of lombok's
transformations, and preferably a nice plugin for eclipse (and the
other IDEs) that adds a toggle button in the IDE chrome.

I'm definitely not a fan of solution #2, especially considering that
if you really need equality/hashCode keyed off of just 1 field,
rolling them yourself is a lot easier. The include idea in #3 mirrors
the current exclude concept, so it's okay. Doing the right thing is
even better if we can get past the magic factor. So, let's see:

1. Is there ANY situation you can think of where just keying off of
all fields annotated with JPA @Id, if there is at least 1 field tagged
with @Id, is NOT the correct action to take? I can't really think of
any. If the "@Id Long id" field is on the class in the first place,
then any other notion of equality is right out the window - 2 objects
with the same everything wouldn't be equal if they didn't have equal
ids. So, you'd HAVE to explicitly exclude the id, either by marking it
transient, or by excluding it, which lombok can pick up on and
disregard that field for the "@Id fields win out for equals and
hashCode" rule.

2. As lombok needs to take a wild guess at typing resolutions, is
there any other annotation named 'Id' that's famous*? That would make
it more likely that lombok would do the wrong thing, which is very bad
if lombok does this silently without any feedback.


If there's absolutely no way you can possibly imagine an equals
working any other way (condition #1) and there's no other commonly
used @Id, please add an issue to key off of only fields annotated with
@javax.persistence.Id if there's at least 1 field present with that
annotation.

What happens if you mark 2 fields with @Id, anyway? I thought that's
how compound IDs were handled. Eh, I never use compound IDs, I don't
have experience with that particular case.


*) Lombok runs before all types have been resolved. Lombok has its own
type resolver but as it doesn't know about your class or source path,
it takes a wild stab at it by scanning for other types in the same
file, and import statements. It gets confused if you use a lot of *
imports, though. I doubt you'll ever get into trouble because of this,
but if there's another common annotation also named @Id this will blow
up (and do so silently due to the 'lombok silently does the right
thing' spiel) if you use a lot of star imports.

[Jeff Schnitzer]

I doublechecked the spec, since I haven't used composites in a while.
There are two ways composite keys are handled. Option one (drawn from
the spec):

@IdClass(com.acme.EmployeePK.class)
@Entity
public class Employee {
@Id String empName;
@Id Date birthDay;
...
}

Option two, which precludes any @Id annotations:

@Entity
public class Employee {
@EmbeddedId
protected EmployeePK empPK;
...
}

Looks like they did add a facility for multiple @Id annotations. So
it looks like the hashCode and equals implementations should use
either an aggregate of all fields with @Id, or the @EmbeddedId (which
must have its own valid hashCode and equals).

I'm quite certain that making lombok automagically implement hc and
equals this way is The Right Thing, but I know what you mean about the
magic. Enough of these rules and it'll get confusing. I'd say do it
though! I'll create the bug now.

FWIW, I think the only time this might be problematic is if someone is
(ab)using the @Id annotation for purposes other than JPA. They
shouldn't do that :-)

Jeff

[Jeff Schnitzer]

Oh, there is one more issue which I'm not sure how you would want to
handle: @Id and @EmbeddedId annotations can come from superclasses.

It might also be a good idea for this behavior to only apply when a
class is annotated with @Entity.

It might also be a good idea to have the @EqualsAndHashCode(include=
{"xxx"}) option as well since it's still possible for (masochistic)
developers to define the ids and relationships in XML. And it's a
useful construct in general.

Jeff

On Aug 17, 5:43 pm, Reinier Zwitserloot <reini...@gmail.com> wrote:

[Reinier Zwitserloot]

So,the conclusion is:

If the class is @Entity, *AND* there are fields annotated with either
@Id or @EmbeddedId, use only those fields for equals and hashCode and
ignore the rest.

If the class is not @Entity, OR there are 0 fields annoted with @Id or
@EmbeddedId, do the usual thing.

Any fields that are excluded (via exclude="" or via transient) are
ejected early in this decision process, so if all @Id/@EmbeddedId
marked fields aren't in play, do the normal thing.

And, add include="" to EqAHC. Make it a write-time error if you try to
use both include and exclude.


Is that what you have in mind? If so, we're on the same page :)

[Jeff Schnitzer]

Sounds perfect. Thanks!

Jeff

[Moandji Ezana]

On Tue, Aug 18, 2009 at 2:26 AM, Jeff Schnitzer <jeff-...@infohazard.org> wrote:

For nearly all objects of this type, the appropriate equals()
implementation is:  "if the classes are the same and the ids are the
same".  hashCode() should just defer to the id.

Are you sure? In the Hibernate/JPA book, they warn against this. Problems arise with newly-created objects.

If you need to compare New instances to a Managed one, the id of the New instance won't have been set.

If you put a New instance into a Set, when it becomes Managed (ie. the @Id is filled in) its hashcode will have changed.

Also, if you have unicity constraints, you can use those to discriminate, rather than the id.

Maybe this view is outdated, but it's what I took away from the book.

Moandji

[Reinier Zwitserloot]

These all sound like rather valid concerns to me. They make the case
that the equals method should compare on everything except the Id
classes!

This is trending way too much towards the voodoo magic side of things,
but is there a reliable way to test if the id has not been set yet?
Would it be a good idea if lombok compared ids if they have both been
set, but compare everything except id when either (or both) of the
involved objects don't have anything set?

And, what should be done with hashCode? I don't think there's a way
around changing the returned hashCode pre- and post- insertion (when
the id gets set). The only reliable way to go there is to compare
everything except the @Id fields, but if hashCode works like that,
then equals() would have to work the same way as well.

Ugh. Anyone got a good idea?

On Aug 18, 1:36 pm, Moandji Ezana <mwa...@gmail.com> wrote:
> On Tue, Aug 18, 2009 at 2:26 AM, Jeff Schnitzer

> <jeff-goo...@infohazard.org>wrote:

[Jeff Schnitzer]

I checked with the book (section 9.2.3).

In fact, there are certain circumstances when using identity as
equality will cause problems - particularly when doing cascading
saves. If you don't do this (ie, if you persist the objects before
adding collections), it's not an issue... but I'm willing to concede
that this is probably not a good default behavior for lombok.

Using all non-id fields as identity is even worse. When you change
any field (and not a lot of apps can get away with perfectly static
data) you break equality.

The book suggests that you define equality by a natural business key.
This is rather disingenuous advice, because plenty of objects don't
have natural business keys. In my experience, probably about 1/3rd of
the classes I've modeled.

So it looks like the way to implement equality is "it depends" and
sometimes the answer is "there is no good answer". Probably lombok
should punt this decision to the architect and just offer the
@EqualsAndHashCode(include={"blah"}) facility.

Honestly I'm surprised Hibernate has problems with cascading saves
when equality == identity. There are ways to solve that problem. I
will talk to the Powers That Be.

Jeff

[Moandji Ezana]

On Tue, Aug 18, 2009 at 9:46 PM, Jeff Schnitzer <jeff-...@infohazard.org> wrote:

When you change
any field (and not a lot of apps can get away with perfectly static
data) you break equality.

Isn't using mutable fields in equals/hashcode discouraged, anyway?

Another usecase is for unit tests. Since you can't have a setId() (another thing Lombok would have to take into account), equality in unit tests should probably rely on "real" equals().

Theoretically, with Hibernate/JPA plain identity should work for objects that don't have a "natural business key", but I've been having some weird behaviour with Javassist proxies...

Moandji

[Reinier Zwitserloot]

Using mutables in equals/hashCode is not discouraged at all. equals is
what it is; if I have a Circle object with a mutable radius, then a
circle with radius 5 is not equal to a circle of radius 10. End of
story.

What IS discouraged is using mutable objects (in the sense that their
equals/hashCode values can change over the lifetime of that object) as
keys in maps.

On Aug 18, 10:25 pm, Moandji Ezana <mwa...@gmail.com> wrote:
> On Tue, Aug 18, 2009 at 9:46 PM, Jeff Schnitzer

> <jeff-goo...@infohazard.org>wrote:

[Moandji Ezana]

On Tue, Aug 18, 2009 at 11:39 PM, Reinier Zwitserloot <rein...@gmail.com> wrote:

What IS discouraged is using mutable objects (in the sense that their
equals/hashCode values can change over the lifetime of that object) as
keys in maps.

Ah, right. I've kind of taken that case as a general rule, which might be overkill.

Moandji

[Reinier Zwitserloot]

I'm going to put all work on @Entity and @Id scanning on the
backburner. Until hibernate/JPA themselves sort out if they'd like
equality to mean 'refers to the same db row' or to mean 'has the same
data', we're stuck. JPA works by proxying the class, right? If so,
isn't this JPA's job? Once they figure out what equality is supposed
to mean, they can just proxy a proper equals and hashCode
implementation in there. Hibernate surely knows more about the
specifics than lombok ever can.

I will, however, add 'include=', which should also allow you to roll-
your-own support here. I gather for JPA it would pretty much boil down
to:

@EqualsAndHashCode(include="id")


On Aug 19, 12:08 am, Moandji Ezana <mwa...@gmail.com> wrote:

[Jeff Schnitzer]

On Aug 18, 3:13 pm, Reinier Zwitserloot <reini...@gmail.com> wrote:
> I'm going to put all work on @Entity and @Id scanning on the
> backburner. Until hibernate/JPA themselves sort out if they'd like
> equality to mean 'refers to the same db row' or to mean 'has the same
> data', we're stuck. JPA works by proxying the class, right? If so,
> isn't this JPA's job? Once they figure out what equality is supposed
> to mean, they can just proxy a proper equals and hashCode
> implementation in there. Hibernate surely knows more about the
> specifics than lombok ever can.

Actually, JPA doesn't usually proxy entity classes - only in a few
very special cases. We're still on our own for equals() and
hashCode(). But I don't think lombok can automatically figure out the
proper implementation of these methods, so yeah, best to table this.

> I will, however, add 'include=', which should also allow you to roll-
> your-own support here. I gather for JPA it would pretty much boil down
> to:
>
> @EqualsAndHashCode(include="id")

That will suffice for my, and I think most other people's,
purposes. But don't forget it needs to be able to accept an array!
:-)

Jeff

[Peter Becker]

I have actually run into these issues. The ID seems a good key at first,
but it is quite easy to put a new object into some hashmap before the
save() call, in which case it will magically disappear. I would consider
using the ID field in any hashCode() implementation as a problem waiting
to happen. By implication the same applies to equals(..).

Peter

[Peter Becker]

I don't think there is a solution. This is one spot where the leakiness
of the whole ORM idea turns into rain. Combine that with the ill-defined
notion of object identity in Java and the only conclusion is that you
better not try unless you have a well-established notion of identity on
the business side. That is actually common for core business objects, so
often avoiding the problem is pretty easy. Sometimes it is not.

Peter

[Peter Becker]

I tend to discourage equals/hashCode with mutables since you usually
don't know how objects are used. Unless you can guarantee that objects
of a class are never put into a hash structure, you are just waiting for
trouble. It's not just the maps, but also the HashSet (which admittedly
is a map underneath, but not semantically).

Peter