I have looked a bit more at this bug and think there is an argument that
this is not, in fact, a bug.
The argument that the update should not occur is based on the part of
the specification that says:
"Note that it is the application that bears responsibility for
maintaining the consistency of runtime relatinships -- for example, for
ensuring that the "one" and "many" sides of a bidirectional relationship
are consistent with one another when the application updates the
relationship."
This does not appear to me to say that on a delete the user must null
out the relationship on the object to be deleted or that when the
application does that, the persistence provider must handle it.
It seems to me that setting a not-nullable relationship to null is an
error. (whether or not you are about to delete). When an object is
deleted it's relationships go away with it. Maintaining the consistency
of the relationships as stated above refers to ensuring that other
objects that refer to the object that is being deleted no longer refer
to it prior to the delete.
I believe that we should throw an exception when a not-nullable
relationship is updated to null. The current exception is a
SQLException. We can either continue to do that, or we could throw a
more descriptive exception at commit-time if our metadata tells us what
we are attempting to null out is not nullable.
-Tom
Tom Ware wrote:
>Hi Markus,
>
> Sorry for taking so long to get back to you about your changes for
>issue 699. Here are some comments:
>
>1. Removing all the change sets of deleted objects is a bit risky at
>this point (commitToDatabase). I realize that occurs in the
>"performDeletesFirst" scenario, but when users do that they realize they
>are altering the default behavior and exposing themselves to risk.
>2. It might be a better idea to look at the code where we do the
>update. Perhaps we can avoid doing the update for objects in the
>deleted objects list.
>3. I am trying to figure out if there may be a spec issue here. i.e. Is
>it possible for the persistence provider to know enough about the
>underlying schema to know whether or not it can avoid an update that has
>been asked for by the user. Are there some times when the foreign key
>goes both ways in a bidirectional relationship where we just can't know
>if we can avoid that delete. I'll have to think about that one some
>more and get back to you if/when I come up with an answer.
>
>-Tom
>
>Markus Fuchs wrote:
>
>
>
>>Hi Tom,
>>
>>Tom Ware wrote:
>>
>>
>>
>>
>>
>>>Hi Markus,
>>>
>>> Comments inline
>>>
>>>Markus Fuchs wrote:
>>>
>>>
>>>
>>>
>>>
>>>>Hi Tom!
>>>>
>>>>[Including the persistence alias in the discussion]
>>>>
>>>>I hope you had a great vacation!
>>>>
>>>>While investigating issue 879 I noticed, that the class
>>>>OneToManyMapping doesn't have to implement method
>>>>hasInverseConstraintDependency().
>>>>
>>>>The dependencies corresponding to a OneToMany/ManyToOne relationship
>>>>are always added by the owning side, which is represented by the class
>>>>OneToOneMapping in TopLink, correct?
>>>>
>>>>So, the dependencies are added twice, once from the owning, and once
>>>>
>>>>
>>>>from the non-owning side. It should be possible to remove this method,
>>>
>>>
>>>>letting OneToManyMapping fall back to the default implementation
>>>>returning 'false'. Would you agree?
>>>>
>>>>
>>>>
>>>>
>>>>
>>>>
>>>If we make this change, how will the commit order calculation logic
>>>work with we are processing a OneToMany? With the code as it is, it
>>>looks to me like if we have OneToManyMapping return false from this
>>>method, in recordMappingDependencies() we will get end up adding a
>>>dependancy that is in the incorrect order (because we will get into
>>>the "if" rather than the "else")
>>>
>>>
>>>
>>>
>>>
>>I understand the handling of the two cases in recordMappingDependencies()
>>as follows:
>>
>>(1) (ForeignReferenceMapping)mapping).hasConstraintDependency() == true
>>
>>This is for the owning side. The referenced class is the owned class
>>and is added with all its subclasses to the owner's list relatedNodes.
>>
>>(2) ((ForeignReferenceMapping)mapping).hasInverseConstraintDependency()
>>== true
>>
>>This is for the non-owning side. The current class is the owned class and
>>is added with all its subclasses to the owner's list of relatedNodes.
>>
>>
>>recordMappingDependencies() uses an exclusive if ("if" ... "else if")
>>construct. So if the two methods hasConstraintDependency() and
>>hasInverseConstraintDependency() are implemented consistently for each
>>mapping, there is no problem of adding "reversed" dependencies.
>>
>>The implementation of hasInverseConstraintDependency() for
>>OneToManyMapping is *correct*, and shouldn't be changed. Rather, one
>>could consider changing the implementation of
>>recordMappingDependencies() to evaluate only the owning side (case 1)
>>for calculating the dependencies. But it's not a big deal anyway. I
>>withdraw my proposal to change OneToManyMapping.
>>
>>
>>
>>
>>
>>>>Could you please also comment on my evaluation of issue 879? The
>>>>change in class ManyToManyMapping could be the fix for this issue.
>>>>
>>>>
>>>>
>>>>
>>>>
>>>>
>>>I'll add comments to the bug shortly.
>>>
>>>
>>>
>>>
>>>
>>>>For issue 699, I changed UnitOfWorkImpl to remove the ObjectChangeSets
>>>>for all objects that are to be deleted before updates/inserts are
>>>>processed in any case. I got the impression that the updates for
>>>>deleted objects are executed in order to prevent foreign key (unique
>>>>key) violations. Is this correct?
>>>>
>>>>There's no database constraint forcing to nullify foreign keys for for
>>>>OneToMany relationships, before other updates happen. The same is true
>>>>for join table entires in case of ManyToMany relationships, which
>>>>don't need to be removed. The only case where nulling out the foreign
>>>>key is important is for OneToOne relationships, which might have a
>>>>unique constraint on the foreign key in the database. But as
>>>>dependencies within the same class can't be determinated anyway,
>>>>removing those updates should not make a difference. Please see the
>>>>attached UnitOfWorkImpl class for my change.
>>>>
>>>>
>>>>
>>>>
>>>>
>>>>
>>>I am concerned about the change to UnitOfWorkImpl and will need to
>>>take a closer look at the issue. Here is my concern: It looks like
>>>the way we are solving this issue is to go through the change sets as
>>>usual and build a change set as normal and them make a second pass to
>>>"fix" the change set. Ideally, I think we would build a change set
>>>that we could use correctly like from the beginning. I'll take a look
>>>at this issue in some more depth and get back to you.
>>>
>>>
>>>
>>>
>>>
>>Thanks, I'll wait for your update!
>>
>>-- markus.
>>
>>
>>
>>
>>
>>>>I ran entity-persistence-tests w/o regressions with my changes. What
>>>>do you think?
>>>>
>>>>Thanks very much,
>>>>
>>>>-- markus.
>>>>
>>>>
>>>>Tom Ware wrote:
>>>>
>>>>
>>>>
>>>>
>>>>
>>>>
>>>>
>>>>>Hi Markus,
>>>>>
>>>>>I was on vacation, but it looks like you got your answer. Feel
>>>>>free to contact me with questions.
>>>>>
>>>>>-Tom
>>>>>
>>>>>Markus Fuchs wrote:
>>>>>
>>>>>
>>>>>
>>>>>
>>>>>
>>>>>
>>>>>
>>>>>>Hi Tom,
>>>>>>
>>>>>>is it ok if I look into this?
>>>>>>
>>>>>>Thanks!
>>>>>>
>>>>>>-- markus.
>>>>>>
>>>>>>
>>>>>>
>>>>>>
>>>>>>
>>>>>>
>>>>>>
>
>
>