11

When the business layer creates a new entity, which logically represents an instance of an existing entity that should be updated (say they share the same business key), is this method of merging bad practice?

public User add(User user){

    User existingUser = getUserDao().findByBusinessKey(user.getBusinessKey(), false);
    user.setId(existingUser.getId());

    user = getUserDao().merge(user);

    return user;
}

I ask because setting the ID explicitly on the detached entity feels pretty strange to me, but even though the equals and hashcode method of the User entity are appropriately implemented, setting the ID here is the only way to ensure the merge takes place.

Is there a better practice?

Are there specific drawbacks to this method that would bite me later on?

Thanks for taking a look!

D Parsin
  • 731
  • 2
  • 7
  • 17
  • 1
    One thing that my code snippet didn't property demonstrate was setting getUserDao().merge(user)'s result to the original user reference... updating the snippet to show that. – D Parsin Jan 25 '11 at 06:33
  • Why have an ID (autogenerated) and and a business key in the first place? Why not use the Busines key as ID? This assumes that it never changes of course.. – bert Jan 25 '11 at 07:10
  • It's always been the recommendation of the hibernate team from what I've read - and it seems to work out well in most situations, except for the one I posed :s http://community.jboss.org/wiki/EqualsandHashCode – D Parsin Jan 26 '11 at 01:48

2 Answers2

3

That code will work, but setting the ID explicitly on the detached entity should not be necessary. A typical Hibernate app have a 'save' method that handles two cases:

  1. The user wanted to create a new User, so the app creates a User object with 'null' as the ID.
  2. The user queried for a list of users, and is selecting one for edit. In this case the app does a query and propagates the object to the 'save' method. The object will have an ID and the code will apply new values to it.

Looks like something in your code isn't doing the second case in the typical way. If the 'user' object comes from some prior Hibernate query (triggered by the user clicking 'edit user' or something like that), then it will already have an ID. Thus, only the merge(user) call is needed.

I usually do something like this:

if (user.getId() == null)
  em.persist(user);
else
  user = em.merge(user);

Then I add code to handle optimistic locking issues (another session updated the object) and unique constraint issues (another session tried to persist something with the same business key).

Frameworks such as Seam can make this even simpler because they propagate the Hibernate session between the controller bean methods. So even the 'merge' is not needed.

Joshua Davis
  • 3,499
  • 1
  • 26
  • 29
  • Is it equivalent to `merge` unconditionally? Other than returning a copy of the object, `merge` can do everything `persist` does. – Peter Davis Jun 26 '11 at 06:34
  • IIRC, merge is a superset of persist. I hardly use merge any more since I started using Seam. – Joshua Davis Jul 12 '11 at 19:35
  • @JoshuaDavis I am confused since I used to use merge() as a 'saveOrUpdate' and it worked!!! Recently, it broke on upgrading hibernate and I switched to persist() which works as SAVE OR UPDATE as seen on line 110 here https://github.com/deanhiller/webpieces/blob/master/webserver/webpiecesServerBuilder/templateProject/WEBPIECESxAPPNAME/src/main/java/webpiecesxxxxxpackage/web/crud/CrudUserController.java. (damn, I really liked the saveOrUpdate). the platform passes in UserDbo and I don't care if it was edited or brand new. I just need to call a saveOrUpdate method. – Dean Hiller May 29 '20 at 18:38
  • @DeanHiller Yes. I think what you're seeing is the "works by maverick" problem. 'em.persist()' has a specific behavior defined by JPA. If the implementation happens to also do something that is not explicitly disallowed by the specification, then it works by accident. I prefer to be explicit about create vs update and I want the ORM to tell me if I'm not doing what I thought I was doing. – Joshua Davis Jul 02 '20 at 15:45
0

If your entity is a detached entity the only thing u really need to do is to invoke entityManager.merge(user). You dont need to exec any finder method. If your entity is not detached but rather new (it does not have id specified) you should find appropriate entity in the database prior performing any modification operations on that entity and merge it afterwards. i.e:

User user = userDao.findBySomething(Criteria c);

//stuff that modifies user 

user = userDao.merge(user);
Marcin Michalski
  • 1,266
  • 13
  • 17
  • I guess that my problem is the 2nd case that you mention. Unless I explicitly set the id on the new instance of the entity, the merge is not successful. This makes sense if the only way the entityManager knows to merge is using the identifier, but I had thought that it was supposed to understand .equals(). If not, and I continue by setting the ID, will I run into any other trouble? – D Parsin Jan 25 '11 at 01:44
  • 1
    Your equals() and hashCode() methods should *never* assume that ID is set, they should always use the business key. The problem is that somehow the detached instance doesn't have an ID (making it *transient*, not detached). See my answer above. – Joshua Davis Jan 26 '11 at 16:53