Thanks guys for all the effort in digging into the issue here. From a pragmatic point of view I guess #2 would be my preferred option and then think about what we should do with that save thing :) Maybe a strategy pattern similar to what we have for TX would be the way to go.
The documentation does not put a focus on it, but Data is in no way exclusively dependent on EntityRepository resp. AbstractEntityRepository - those are just convenience constructs which I have seen in almost any team I've worked popping up when it comes to JPA usage. With delegates [1] you can actually build your own convenience base repositories they way they should be ;) Also other features like criteria support have been moved to their own interfaces and can be pulled in on demand. Hope that helps, Thomas [1] http://deltaspike.apache.org/documentation/data.html#QueryDelegates On Thu, Aug 13, 2015 at 6:21 PM, Marvin Toll <marvint...@gtcgroup.com> wrote: > 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 > >
