Hi Markus,
A more descriptive exception would be a good idea.
-Tom
Markus Fuchs wrote:
>Hi Tom,
>
>I agree that setting the non-nullable relationship to null is a user
>error. But we should throw a more description exception at commit-time,
>and not rely on the SQLException. Shall I work on that?
>
>Thanks,
>
>-- markus.
>
>Tom Ware wrote:
>
>
>
>>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.
>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>
>>>
>>>
>>>
--
Tom Ware
Principal Software Engineer
Oracle Canada Inc.
Direct: (613) 783-4598
Email: tom.ware_at_oracle.com