1

There is well known problem with implementing equals() (and hashCode(), i will speak about equals() only) for persistance object with database managed id. New object is not stored in database, therefore does not have database identity, therefore its "id" field is null (or 0 if it is primitive type).

If equals look at id, it will consider all new objects equal, and once it gets id, hash code change so if it already was in hash sensitive collection, it will not be found.

One solution is using business key, but sometimes everything except surrogate id is mutable. Another solution is to generate (one more, or use it as databes id too) surrogate id when object is created.

Approach I haven't seen mentioned is, use id in equals and make equals (and hashCode()) fail (throw IllegalStateException) when id is null. (and document this behavior) This way it still can't be in hash collections, but it cannot be accidentaly put there. And for puting it in collections when without id, some wrapper can be used. Is it good/bad idea? Does it have hiddent problems?

As kan pointed out, if child objects should be put in Set property and persisted with their parent, inability to put objects in Set before they are persisted is big problem (and TreeSet does not help, because it uses equals(), even if it does not use hashCode()). I mostly use list for child entities, so it does not need to manifest, but it definitely is problem.

Alpedar
  • 1,314
  • 1
  • 8
  • 12

2 Answers2

1

I always use the auto-generated id and have never had a problem. You could enforce an object when instantiated to also be persisted using your service layer/factory.

I think the likelihood of any other field (composing a biz key) changing is much more probable than using a non-persisted object in a hashmap and then persisting at the same time causing the look up to fail.

This problem, imho, is somewhat over-analysed. The auto-generated id is often the only test I want to do for equality, nothing else makes sense in a lot of cases. I take the approach that if a non-persisted object is being used/compared the problem is in the business logic and not the underlying equals/hashcode methods

To specifically answer the illegalstateexception idea, throwing an exception when objects are not equal and/or have not been persisted seems rather dramatic.

Community
  • 1
  • 1
NimChimpsky
  • 46,453
  • 60
  • 198
  • 311
  • My idea is, that I would throw not because they are not equal, but because user is not supposed to ask just now whether they are. This way the problem in biz. log. is pointed out. – Alpedar Oct 25 '11 at 08:46
  • it may be pointed out, but its not fixed. The user will receive an error message. The only 100% safe to do is to generate an id for your object on object creation ( http://onjava.com/pub/a/onjava/2006/09/13/dont-let-hibernate-steal-your-identity.html?page=1 ). But as I said I use db id all the time, make sure you doc your code and you'll be fine. – NimChimpsky Oct 25 '11 at 08:51
  • I want developer to get that error message and make him change that code. I will use my throwing equals in current project for classes that are persisted before any meaningfull activity. But it definitely is not generic solution. Autogenerated id on creation is probubly THE generic solution. – Alpedar Oct 25 '11 at 09:06
  • Unfortunately, it is not _THE generic solution_. Read a discussion below my answer. – kan Oct 25 '11 at 10:37
-1

I use the next code. It covers most cases and it could be used for all cases which I think could occur while using ORM.

public class VersionedEntity
{
        private static final long serialVersionUID=1L;
        private Long id;
        private long version;
        @Transient
        private int hashCode;
...
        public void setId(final Long id)
        {
            if(this.id != null && !this.id.equals(id))
                throw new IllegalArgumentException(this+" has an ID already, cannot change it to "+id);
            this.id = id;
        }
        @Override
        public String toString() {
            return getClass().getName()+'#'+getId();
        }

        public boolean equals(final Object o)
        {
            if (this==o) return true;
            if (!(o instanceof VersionedEntity))
                return false;
            final VersionedEntity entity=(VersionedEntity) o;
            final Long id1 = entity.getId();
            final Long id2 = getId();
            if(id1==null && id2==null)
                return super.equals(o);
            return id1 != null
                   && id2 != null
                   && id2.equals(id1);

        }

        public int hashCode()
        {
            if(hashCode == 0)
            {
                hashCode = id != null ? id.hashCode() : super.hashCode();
                if(hashCode == 0)
                    hashCode = 42;
            }
            return hashCode;
        }
}

You has to able to use collections before assigning the id. If you want create an object with a Set property which contains pack of other objects, you have to add them as elements to the Set.

kan
  • 28,279
  • 7
  • 71
  • 101
  • how does this enforce that id is set when object is created ? This just shows that when two objects have null id the super class's method is called. And the same for hashcode. I don't understand why you think this is better. – NimChimpsky Oct 25 '11 at 08:35
  • I can create a new object and use it in a collection. The id can be null when put in collection, I can then setid() afterwards. It does not solve the problem at all. And two new objects with null id will just call super class's method. – NimChimpsky Oct 25 '11 at 08:43
  • Just read the code carefully, it will work properly in this case. Do I miss something? – kan Oct 25 '11 at 08:46
  • What case ! 1. I create a new object the id is null, I put it in collection. then my code changes id elsewhere. Lookup from hashmap will fail. 2. New objects with null id will evaluate to equal or not equal ? And you are assigning a value of 42 to hashcode, this is a joke surely ? – NimChimpsky Oct 25 '11 at 08:47
  • 1. **hashCode is stored**, `equals` compares by the id, so it works. 2. If the object the same (`super.equals == true`) then true, if two different objects, then false. 42. Kind of, just small optimization. – kan Oct 25 '11 at 08:54
  • No no no ! Yes equals compares by id, But the id can change at any point in your code and we still have no idea what super.equals does. hashCode() method is called, the hashcode variable is irrelevant. Two new objects with null id's, are they equal ? – NimChimpsky Oct 25 '11 at 08:57
  • If you change the id in the object which is in a set and somewhere else, but the object the same, it will be `equals`. `hashCode` doesn't change after an id is assigned, because it's stored. Two different objects with null id's are NOT equal. It passes all three cases in summary section of http://community.jboss.org/wiki/EqualsAndHashCode – kan Oct 25 '11 at 09:02
  • new object with null id is put into collection, the super hashcode method is called. While in collection setid() is called, this changes the hashcode returned (a problem). And it does not solve the problem that was asked. And 42 is not an optimization, its a magic number, and its bad. – NimChimpsky Oct 25 '11 at 09:21
  • You are right, the `super.equals` is not necessary, it's just `java.lang.Object.equals`, it does `this == obj` which I have already in the first line of the method. Anyway, could you describe a test case when my code will not work properly? I could imagine one situation, but I believe it should not happen in Hibernate. – kan Oct 25 '11 at 09:21
  • _this changes the hashcode returned (a problem)_ - NOOO!! HASHCODE IS STORED!!! It will not change after assigning an ID! – kan Oct 25 '11 at 09:22
  • OK, what if super.hashcode() returns zero ? Then every new object will have a hashcode of 42, totally removing the point of hashcode. And not solving the problem at all. This is whole question is about new objects with null id's. your code just assigns 42 as a hashcode for new objects. – NimChimpsky Oct 25 '11 at 09:25
  • PLEASE, PLEASE, read the code carefully! It calls java.lang.Object.hashCode()! If you think there is a JVM which returns zero hash code for all objects, I'm in trouble. Could you give me a test case which fails for my code? – kan Oct 25 '11 at 09:29
  • OK, I thought you were inheriting from one of your objects. But in that case your 42 is irrelevant. If you have two instances of an object with the same id, that are used in a collection before setting the id. They will return different hashcodes, but be equal. The inital call to hascode when new object is put into collection will set hashcode to Object.hashcode and store it. Subsequently you set id, so that two objects are equal. Your hashcode() method will however return two different hashcodes for equal objects. – NimChimpsky Oct 25 '11 at 09:33
  • In one hibernate session there are no two instances with the same ID. At least it says it on the tin _Hibernate guarantee a scope of Java object identity in this cache. Only one object instance representing a particular database row exists in the cache_ http://www.lalitbhatt.com/Hibernate+Persistent+Context+and+Session – kan Oct 25 '11 at 09:39
  • Of course, the mixing of objects between different hibernate sessions breaks my code, but it breaks some other things also. I just avoid it. – kan Oct 25 '11 at 09:43
  • That is irrelevant entities can exist outside the session. And that is the whole point of the question, specifying id to new objects which are not yet persisted. I think the code just over complicates things with no real benefit. – NimChimpsky Oct 25 '11 at 09:44
  • In fact it breaks one of the key rules that if an object is equal it must also return the same hashcode(). http://stackoverflow.com/questions/27581/overriding-equals-and-hashcode-in-java/27609#27609 – NimChimpsky Oct 25 '11 at 09:51
  • I just avoid situations if an transient object was persisted and then detached. Before detach, I'll drop out all transients and use persistent instead. It's more flexible, your solution with auto-generated id doesn't allow create a disconnected object graph on the client side (which doesn't have a database) then transfer it via wire to the server to persist it into database. It's not _THE generic solution_ at all. My solution incorporates yours, you could `setId` by autogenerated id before using sets/etc, my code falls back to your approach then. – kan Oct 25 '11 at 10:11
  • I agree auto id is not perfect. But your code breaks one of they key rules - hashcode can be different when objects are equal. – NimChimpsky Oct 25 '11 at 10:15
  • Technically it doesn't. ;) Transient and persisted object is not exactly the same. Moreover, I think it's possible to solve by small amendment (just make them not equal), however I don't bother about it, since proper use of hibernate avoids the situation. Anyway, my solution is correct with some reasonable precautions, it should not be downvoted. – kan Oct 25 '11 at 10:24
  • Technically it doesn't ? What ? IT DOES. Put two new objects with null id into a collection their hashcode will be different, then setid to the same for each object and they are equal but with different hashcodes. Whether they are persisted/transient is irrelevant you have broken the equals/hashcode contract It doesn't solve the problem, it just changes it. I didn't downvote. – NimChimpsky Oct 25 '11 at 10:27
  • I mean it could be amended. If you create two objects and set the same id (first of all, it must not happen in hibernate!), it could remember that state and return `!equals` for them. It follows hibernate's logic for transient/persistent entities. – kan Oct 25 '11 at 10:34
  • It must not happen in hibernate ? But we are talking about new objects that are not yet persisted, that is the point of the question. If you are guaranting/assuming that the objects are already in a session, then the auto id is much better – NimChimpsky Oct 25 '11 at 10:39
  • 1
    let us [continue this discussion in chat](http://chat.stackoverflow.com/rooms/4501/discussion-between-kan-and-nimchimpsky) – kan Oct 25 '11 at 10:40