Re: Issues with EntityRepository.save()
imo the next step is to add an optional check for the @Version property. for other special cases it should be enough to document the customization with QueryInvocationContext. regards, gerhard 2015-08-22 1:46 GMT+02:00 Marvin Toll : > First an FYI... the Ford Motor Company Intellectual Property attorney > group approved our (ten specifically named Software Engineers) > participation with the Apache DeltaSpike community yesterday. So, we can > now use the word "Ford" in our informal communications. :-) > > This does not represent a formal endorsement of DS nor does it give > license for the use/misuse of the "Ford" brand in any formal communications. > > In any case, we are continuing to develop use cases using the "save" > method in combination with EclipseLink (only). We may have found a > 'broken" scenario today but plan to verify this on Monday with fresh eyes > to see whether this is truly a "bug"... and worth pursing as a > representative case of "unexpected" behavior using the "save" abstraction. > > Our strategy includes, running a scenario with native JPA (EclipseLink) > and then the same scenario with DS Data... and looking for different > outcomes. > > Bottom line? I agree with John that this discussion needs attention. > > More to come, > Marvin > > -Original Message- > From: John D. Ament [mailto:johndam...@apache.org] > Sent: Friday, August 21, 2015 5:22 PM > To: dev@deltaspike.apache.org; Marvin Toll > Subject: Re: Issues with EntityRepository.save() > > So the discussions on this died down, but I think we should still plan to > do something regarding the issue. > > I'd like to propose that we do #1 and the back half of #4. > > We may want to add some additional comments. For example, recommend that > the @Id column is always nullable, which should clarify some of the > behavior. We may even want to try to add a log message around it. > > I'm going to try to add nullable IDs to the project I'm working on right > now to see if it works the way I expect. If so would be the right way to > go IMHO for users in general. > > Another issue I noticed with EntityRepository is that I can extend it with > EntityRepository where String is not the @Id column. There > are absolutely no warning about this, and you wouldn't notice the issue > until you try calling findBy(PK). May want to add a note about this as > well, even if its only in dev mode. > > John > > > >> > 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.) > > >> > > > >
RE: Issues with EntityRepository.save()
First an FYI... the Ford Motor Company Intellectual Property attorney group approved our (ten specifically named Software Engineers) participation with the Apache DeltaSpike community yesterday. So, we can now use the word "Ford" in our informal communications. :-) This does not represent a formal endorsement of DS nor does it give license for the use/misuse of the "Ford" brand in any formal communications. In any case, we are continuing to develop use cases using the "save" method in combination with EclipseLink (only). We may have found a 'broken" scenario today but plan to verify this on Monday with fresh eyes to see whether this is truly a "bug"... and worth pursing as a representative case of "unexpected" behavior using the "save" abstraction. Our strategy includes, running a scenario with native JPA (EclipseLink) and then the same scenario with DS Data... and looking for different outcomes. Bottom line? I agree with John that this discussion needs attention. More to come, Marvin -Original Message- From: John D. Ament [mailto:johndam...@apache.org] Sent: Friday, August 21, 2015 5:22 PM To: dev@deltaspike.apache.org; Marvin Toll Subject: Re: Issues with EntityRepository.save() So the discussions on this died down, but I think we should still plan to do something regarding the issue. I'd like to propose that we do #1 and the back half of #4. We may want to add some additional comments. For example, recommend that the @Id column is always nullable, which should clarify some of the behavior. We may even want to try to add a log message around it. I'm going to try to add nullable IDs to the project I'm working on right now to see if it works the way I expect. If so would be the right way to go IMHO for users in general. Another issue I noticed with EntityRepository is that I can extend it with EntityRepository where String is not the @Id column. There are absolutely no warning about this, and you wouldn't notice the issue until you try calling findBy(PK). May want to add a note about this as well, even if its only in dev mode. John > >> > 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.) > >> >
Re: Issues with EntityRepository.save()
So the discussions on this died down, but I think we should still plan to do something regarding the issue. I'd like to propose that we do #1 and the back half of #4. We may want to add some additional comments. For example, recommend that the @Id column is always nullable, which should clarify some of the behavior. We may even want to try to add a log message around it. I'm going to try to add nullable IDs to the project I'm working on right now to see if it works the way I expect. If so would be the right way to go IMHO for users in general. Another issue I noticed with EntityRepository is that I can extend it with EntityRepository where String is not the @Id column. There are absolutely no warning about this, and you wouldn't notice the issue until you try calling findBy(PK). May want to add a note about this as well, even if its only in dev mode. John On Mon, Aug 17, 2015 at 2:43 PM Gerhard Petracek wrote: > i just thought about how we could call such a strategy and saw that we > don't need it imo. > QueryInvocationContext is already a spi -> just provide a specialized > version of > org.apache.deltaspike.data.impl.handler.CdiQueryInvocationContext (or a > global alternative) or use a decorator and override #isNew. > > regards, > gerhard > > > > 2015-08-14 16:12 GMT+02:00 Gerhard Petracek : > > > +1 for a strategy - that also allows to do a faster check against the > > @Version property (if available). > > -1 for any split (we already separated the data-module from the > jpa-module > > due to its complexity). > > > > regards, > > gerhard > > > > > > > > 2015-08-14 15:33 GMT+02:00 Marvin Toll : > > > >> Sounds good... perhaps obviously I'm a DeltaSpike newbie and negotiating > >> my way through the learning curve. > >> > >> -Original Message----- > >> From: Karl Kildén [mailto:karl.kil...@gmail.com] > >> Sent: Friday, August 14, 2015 8:00 AM > >> To: dev@deltaspike.apache.org; marvint...@gtcgroup.com > >> Subject: Re: Issues with EntityRepository.save() > >> > >> 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 > wrote: > >> > >> > 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.) > >> > >> > > >> > 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 > >> > > >> > >> > >> > > >
Re: Issues with EntityRepository.save()
i just thought about how we could call such a strategy and saw that we don't need it imo. QueryInvocationContext is already a spi -> just provide a specialized version of org.apache.deltaspike.data.impl.handler.CdiQueryInvocationContext (or a global alternative) or use a decorator and override #isNew. regards, gerhard 2015-08-14 16:12 GMT+02:00 Gerhard Petracek : > +1 for a strategy - that also allows to do a faster check against the > @Version property (if available). > -1 for any split (we already separated the data-module from the jpa-module > due to its complexity). > > regards, > gerhard > > > > 2015-08-14 15:33 GMT+02:00 Marvin Toll : > >> Sounds good... perhaps obviously I'm a DeltaSpike newbie and negotiating >> my way through the learning curve. >> >> -Original Message- >> From: Karl Kildén [mailto:karl.kil...@gmail.com] >> Sent: Friday, August 14, 2015 8:00 AM >> To: dev@deltaspike.apache.org; marvint...@gtcgroup.com >> Subject: Re: Issues with EntityRepository.save() >> >> 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 wrote: >> >> > 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.) >> > > > >> > 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 >> > >> >> >> >
Re: Issues with EntityRepository.save()
+1 for a strategy - that also allows to do a faster check against the @Version property (if available). -1 for any split (we already separated the data-module from the jpa-module due to its complexity). regards, gerhard 2015-08-14 15:33 GMT+02:00 Marvin Toll : > Sounds good... perhaps obviously I'm a DeltaSpike newbie and negotiating > my way through the learning curve. > > -Original Message- > From: Karl Kildén [mailto:karl.kil...@gmail.com] > Sent: Friday, August 14, 2015 8:00 AM > To: dev@deltaspike.apache.org; marvint...@gtcgroup.com > Subject: Re: Issues with EntityRepository.save() > > 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 wrote: > > > 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.) > > > > > 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 > > > > >
RE: Issues with EntityRepository.save()
Sounds good... perhaps obviously I'm a DeltaSpike newbie and negotiating my way through the learning curve. -Original Message- From: Karl Kildén [mailto:karl.kil...@gmail.com] Sent: Friday, August 14, 2015 8:00 AM To: dev@deltaspike.apache.org; marvint...@gtcgroup.com Subject: Re: Issues with EntityRepository.save() 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 wrote: > 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.) > > 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 >
Re: Issues with EntityRepository.save()
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 wrote: > 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.) > > 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 > 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, De
RE: Issues with EntityRepository.save()
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.)
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
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 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
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 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
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 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
> pas
RE: Issues with EntityRepository.save()
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 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
