persistence@glassfish.java.net

RE: Fix for issue 1021

From: Gordon Yorke <gordon.yorke_at_oracle.com>
Date: Fri, 29 Sep 2006 15:48:11 -0400

I am also surprised that you were confused. Issue 1021 is an insert order issue that spawned a discussion on validation of cascaded lifecycle operations. There was a bug (gf issue 1035) that applied to the validation discussion. Issue 1021 is a user found issue; it really shouldn't be downgraded.

Yes the specification is very clear, all EM api should throw an exception, but that is not what issue 1021 is about, nor is that what issue 1035 is about.
--Gordon

-----Original Message-----
From: Marina.Vatkina_at_Sun.COM [mailto:Marina.Vatkina_at_Sun.COM]On Behalf Of
Marina Vatkina
Sent: Friday, September 29, 2006 3:34 PM
To: persistence_at_glassfish.dev.java.net
Subject: Re: Fix for issue 1021


Gordon,

When I said "Let's downgrade this to p4", I meant the issue that we are
discussing in the subject line - 1021, but I explicitly said in the same
email "we should then fix 1035", so I'm surprised there is any confusion.

The spec is clear that all EM api should throw an exception if a non-
entity is passed as an argument.

Regards,
-marina

Gordon Yorke wrote:
> Marina,
> I have been quite consistent in my emails. Markus's comments in gf issue 1021 was that it was a validation issue because TopLink was attempting to persist non-entities. I disagree and issue 1021 needs to continue to be a bug that must be addressed. The importance of issue 1021 can be discussed but it is not a validation issue.
> Downgrading 1035 was your suggestion "Let's downgrade this to p4 until we have a real user complain that things are falling apart because we allow this, or until the spec is clarified", as 1035 deals with preventing persisting non-entities.
> --Gordon
>
> -----Original Message-----
> From: Marina.Vatkina_at_Sun.COM [mailto:Marina.Vatkina_at_Sun.COM]On Behalf Of
> Marina Vatkina
> Sent: Thursday, September 28, 2006 3:59 PM
> To: persistence_at_glassfish.dev.java.net
> Subject: Re: Fix for issue 1021
>
>
> Now I'm totally confused...
>
> 1. Isn't the dependency management that causes the order of executed sql,
> part of another issue?
>
> 2. If cascade-persist doesn't work for non-entities, and the spec is not
> expected to allow this, why should we spend time *fixing* it, while it's
> much clearer to throw another exception? If you disagree with an
> IllegalArgumentException, then it can be an UnsupportedOptionException.
>
> 3. Issue 1035 is closed currently as 'wontfix', so why should it be
> downgraded instead of being reopen? What else do you like to discuss
> about it?
>
> thanks,
> -marina
>
>
> Gordon Yorke wrote:
>
>>Right now issue 1021 is reporting an exception case. We should fix that problem directly. Issue 1035 can be downgraded and left for discussion as issue 1035 covers validation of non-entities.
>>--Gordon
>>
>>-----Original Message-----
>>From: Marina.Vatkina_at_Sun.COM [mailto:Marina.Vatkina_at_Sun.COM]On Behalf Of
>>Marina Vatkina
>>Sent: Thursday, September 28, 2006 2:44 PM
>>To: persistence_at_glassfish.dev.java.net
>>Subject: Re: Fix for issue 1021
>>
>>
>>Hello Gordon,
>>
>>Let's compromise ;). Let's downgrade this to p4 until we have a real
>>user complain that things are falling apart because we allow this, or
>>until the spec is clarified.
>>
>>On the other hand, we should then fix 1035. Will you agree?
>>
>>thanks,
>>-marina
>>
>>Gordon Yorke wrote:
>>
>>
>>>Yes Marina,
>>> As I have been mentioning we should be cascading without error.
>>>--Gordon
>>>
>>>-----Original Message-----
>>>From: Marina.Vatkina_at_Sun.COM [mailto:Marina.Vatkina_at_Sun.COM]On Behalf Of
>>>Marina Vatkina
>>>Sent: Thursday, September 28, 2006 1:47 PM
>>>To: persistence_at_glassfish.dev.java.net
>>>Subject: Re: Fix for issue 1021
>>>
>>>
>>>Gordon,
>>>
>>>The current spec does not define the rules on cascading to non-entities.
>>>Should we not cascade then?
>>>
>>>thanks,
>>>-marina
>>>
>>>Gordon Yorke wrote:
>>>
>>>
>>>
>>>>Although the specification specifically states that non-entities can not be passed as arguments to the EntityManager or Query interfaces the specification does not define what should happed if an operation is cascaded to a non-entity. If this becomes clarified then validation will be required, but it is not required now and would remove functionality from users. GF issue 1021 can not be fixed through adding validation.
>>>>--Gordon
>>>>
>>>>-----Original Message-----
>>>>From: Marina.Vatkina_at_Sun.COM [mailto:Marina.Vatkina_at_Sun.COM]On Behalf Of
>>>>Marina Vatkina
>>>>Sent: Wednesday, September 27, 2006 7:11 PM
>>>>To: persistence_at_glassfish.dev.java.net
>>>>Subject: Re: Fix for issue 1021
>>>>
>>>>
>>>>Hi Markus,
>>>>
>>>>I checked with Linda, and she confirmed that the current spec requires
>>>>non-entities to cause an exception on EM operations as well as cascade
>>>>operations, as from the spec perspective, cascade means applying EM
>>>>api logic to the related instance. She plans to clarify this.
>>>>
>>>>She is also interested in real user cases when the opposite behavior
>>>>is needed, so if you know any, please let her know.
>>>>
>>>>thanks,
>>>>-marina
>>>>
>>>>Markus Fuchs wrote On 09/26/06 09:18,:
>>>>
>>>>
>>>>
>>>>
>>>>>Hi Gordon,
>>>>>
>>>>>Reading section 2.1.9.3 I only find that non-entity super classes can't
>>>>>be persistent:
>>>>>
>>>>>*The non-entity superclass serves for inheritance of behavior only. The
>>>>>state of a non-entity superclass is
>>>>>not persistent. Any state inherited from non-entity super classes is
>>>>>non-persistent in an inheriting entity
>>>>>class. This non-persistent state is not managed by the
>>>>>EntityManager[12]. Any annotations on such
>>>>>super classes are ignored.
>>>>>
>>>>>*This doesn't say anything about non-entity subclasses of entity
>>>>>classes. Further on, the next sentence:
>>>>>
>>>>>*Non-entity classes cannot be passed as arguments to methods of the
>>>>>EntityManager or Query
>>>>>interfaces and cannot bear mapping information.
>>>>>
>>>>>*Says that non-entity classes can't be persisted, etc. Even if not
>>>>>explicitly stated, it would only make sense to apply the above rule to
>>>>>cascaded operations. The situation described in 1021 shows an example,
>>>>>where cascading the persist operation to a non-entity subclass doesn't
>>>>>work in the current implementation.*
>>>>>
>>>>>*Thanks,
>>>>>
>>>>>-- markus.*
>>>>>*
>>>>>Gordon Yorke wrote:
>>>>>
>>>>>
>>>>>
>>>>>
>>>>>
>>>>>>Hello Markus,
>>>>>> I am a little confused here. There was already a discussion about
>>>>>>this behaviour and I believe the conclusion was to allow users to
>>>>>>reference non-entity inheritance heirarchy members in their object
>>>>>>model. TopLink would persist the state from the parent Entity object.
>>>>>>(discussion thread had subject : "How to prevent stack overflow" and
>>>>>>was in reference to gf bug 939).
>>>>>> Contrary to the comments in the bug there is nothing the
>>>>>>specification that prevents this behaviour and the specification even
>>>>>>covers this case in section 2.1.9.3
>>>>>> If we are not validating EntityManager and Query apis then that
>>>>>>needs to be added but this bug is not a validation issue.
>>>>>>--Gordon
>>>>>>
>>>>>> -----Original Message-----
>>>>>> *From:* Markus.Fuchs_at_Sun.COM [mailto:Markus.Fuchs_at_Sun.COM]*On
>>>>>> Behalf Of *Markus Fuchs
>>>>>> *Sent:* Monday, September 25, 2006 8:08 PM
>>>>>> *To:* persistence_at_glassfish.dev.java.net
>>>>>> *Subject:* Fix for issue 1021
>>>>>>
>>>>>> Hi TopLink team,
>>>>>>
>>>>>> I'm trying to fix issue 1021: VALIDATION: Disallow persisting
>>>>>> non-entity subclasses
>>>>>>
>>>>>> Can I assume that there's a one-to-one relationship btw. entity
>>>>>> classes
>>>>>> and descriptors? I.e. is the follow statement true:
>>>>>>
>>>>>> Class is annotated as @Entity <=> There's a ClassDescriptor for
>>>>>> that class.
>>>>>>
>>>>>> Based on this assumption, the attached change throws a
>>>>>> RuntimeException in
>>>>>> AbstractSession.getDescriptor(Class theClass), if the descriptor
>>>>>> for a class
>>>>>> is not found. Adding it there will also change TopLink behavior
>>>>>> for the
>>>>>> non-JPA case. The exception message must still be localized, of
>>>>>> course.
>>>>>> Is there any reason, why TopLink returns the superclass
>>>>>> descriptor, if no
>>>>>> descriptor for the a class can be found?
>>>>>>
>>>>>> Another place to add this validation would be in EntityManagerImpl,
>>>>>> similar to how em.contains() is called for some JPA operations. But
>>>>>> that would not cover the cases were the operation is cascaded to an
>>>>>> instance, as the internal operation is called directly and not/
>>>>>> /through
>>>>>> the EM interface.
>>>>>>
>>>>>> I tested the validation by trying to persist a non-entity class or a
>>>>>> mapped superclass instance.
>>>>>>
>>>>>> What do you think?
>>>>>>
>>>>>> Thanks!
>>>>>>
>>>>>> -- markus.
>>>>>>