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.
>>>>>
>>>>>
>>>>>
>>>>>
>>>>>
--
Tom Ware
Principal Software Engineer
Oracle Canada Inc.
Direct: (613) 783-4598
Email: tom.ware_at_oracle.com