Marvin, What you are suggesting is not required imo. Some strategy configuration like suggested from Thomas would give the same benefits.
On 14 August 2015 at 13:39, Marvin Toll <marvint...@gtcgroup.com> wrote: > <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 > > > > > >
