Am in strong agreement with Harald's statement: "I never really liked the fact that save() blurs the distinction between persist() and merge(), and by its very name it encourages users to always call save() after changing an entity state which is usually unnecessary and sometimes even wrong - so far, I've seen each and every new user of DeltaSpike Data doing that."
Having used near-native JPA2 for about 3.5 years I'm having an awkward time mentally mapping the "Data" module absraction to native JPA. Said another way, I become a bit confused trying to remember how native JPA2 works and how the DeltaSpike abstraction works. However, this potential criticism is made without me having adequate time/experience with DeltaSpike Data... and may simply be my own transition discomfort. A wild idea... would you consider a less intrusive abstraction for a Data2 module? One that does not try to mirror Spring (or the Repository Pattern) but rather seeks to extend the JPA2 API? _Marvin -----Original Message----- From: Harald Wellmann [mailto:hwellmann...@gmail.com] Sent: Wednesday, August 12, 2015 5:46 PM To: dev@deltaspike.apache.org Subject: Issues with EntityRepository.save() This is a quick summary of the issues around EntityRepository.save() that seem to exist since 1.4.2. I'll create test cases and JIRA issues as time permits, but I think these notes may be helpful at this stage to find out whether or not incompatibilities experienced by other users have the same root cause. According to Javadoc [1], this is what save() does: "Persist (new entity) or merge the given entity. The distinction on calling either method is done based on the primary key field being null or not. If this results in wrong behavior for a specific case, consider using the EntityManagerDelegate which offers both persist and merge." As far as I can see, the description accurately describes the behaviour in 1.4.1. The behaviour was changed in 1.4.2 in an incompatible way without adapting the documentation. Since 1.4.2, if the primary key is not null, DeltaSpike also runs a database query to check whether an entity with the given key exists. If so, it calls merge(), otherwise persist(). So there are now quite a few cases when save() calls persist() where it would have called merge() in 1.4.1. Some use cases: Use case 1: // assume separate transactions. foo = save(foo); remove(foo); foo = save(foo); Result in 1.4.1: foo is persistent. The second save() is a merge(). Result in 1.4.2: Exception. The second save() is a persist(), since the key no longer exists. But then Hibernate complains it cannot persist a detached entity. Use case 2: Assume a one-to-many association Blog -> Comment, both with auto-generated identities of type long. public Blog createBlog(List<Comment> comments) { Blog = new Blog(); blog.setComments(comments); blog = blogRepository.save(blog); blog.getComments().add(new Comment()); } Now someone calls createBlog(Arrays.asList(c1, c2)). In 1.4.1 this call succeeds. In 1.4.2, there is an UnsupportedOperationException since blog.getComments() is immutable. In 1.4.1, the identity member is 0. 0 is not null, so save() performs a merge(). merge() creates a new blog instance with a new mutable comments list (at least with Hibernate (PersistentBag)). In 1.4.2, there is no existing blog entity with identity 0, so save() calls persist() and returns the original blog instance. Its comments member is the result of Arrays.asList() which is immutable. Sometimes it is useful to question the things you think you know, like "an entity with an identity value of 0 or null cannot be persistent". I searched the JPA 2.1 spec and did not find a word about zero identities. In fact, Eclipselink has an option to *enable* zero identities (eclipselink.allow-zero-id). The exception from Hibernate is also misleading. Just because an entity passed to persist() has a non-null and non-zero key, it does not have to be detached, it may well be a new transient entity. I never really liked the fact that save() blurs the distinction between persist() and merge(), and by its very name it encourages users to always call save() after changing an entity state which is usually unnecessary and sometimes even wrong - so far, I've seen each and every new user of DeltaSpike Data doing that. At the moment, I don't see a way to specify and implement save() in a way that is logically consistent *and* portable across JPA providers. (I'll be happy to be proved false.) So for the next release we should do one of the following: 1) Adapt Javadoc to the current behaviour and document the incompatibilities. 2) Leave Javadoc unchanged and restore compatibility. 3) Specify a new expected behaviour and adapt both Javadoc and implementation. 4) Add a big fat warning to save() Javadoc and move persist() and merge() from EntityManagerDelegate to EntityRepository. [1] http://deltaspike.apache.org/javadoc/1.4.2/org/apache/deltaspike/data/api/EntityRepository.html#save-E- Regards, Harald
