<Harald> 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.) </Harald?
Had another idea just before drifting off to sleep last night... perhaps this dreamy though is useful for consideration in the light of day? What if--- DeltaSpike DataH[impl only] (the current DeltaSpike Data) was optimized for Hibernate? And a new DeltaSpike DataE[impl only] was optimized for EclipseLink? Said another way, an objective of trying to abstract both of these provides, given the large volume of custom extensions required for a still too immature JPA specification, and perhaps more importantly maintain an adequate Test Suite(s), may be more challenging/limiting than first imagined? _Marvin -----Original Message----- From: Thomas Hug [mailto:thomas....@gmail.com] Sent: Friday, August 14, 2015 5:00 AM To: dev@deltaspike.apache.org; marvint...@gtcgroup.com Subject: Re: Issues with EntityRepository.save() 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 the 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 abstraction 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 > >