Hi Tom,
We have had lots of discussion on this and I have spent enough time on
this issue; it's beginning to bother me.
Any way since I did not hear any objection in last 10 days, I went ahead
and implemented the proposal. As I mentioned in an earlier email, the
changes make an application behave I am attaching the changes here.
Please review them and get back soon.
I have run entity-persistence-tests and did not see any regression. I
have tested using 5-6 unit test cases for various related code paths and
they all passed.
Thanks,
Sahoo
Tom Ware wrote:
> Hi Sahoo, Marina,
>
> I believe we should probably have a disscussion about this issue
> prior to checking in any changes. The fact that there has been no
> further discussion about that is probably in part due to the fact that
> a couple of the key Oracle people have been on vacation over the last
> week or so.
>
> Can we discuss next week?
>
> -Tom
>
> Marina Vatkina wrote:
>
>> Hi Sahoo,
>>
>> As there had been no objections in the last week, I suggest
>> to check in your changes.
>>
>> thanks,
>> -marina
>>
>> Sanjeeb Kumar Sahoo wrote On 08/11/06 11:57,:
>>
>>
>>> How about these rules for *our implementation*:
>>>
>>> Rule 1. Annotation processor *always* uses access-type of enclosing
>>> entity to decide access-type of embeddable *irrespective* of existence
>>> of annotations in embeddable class. Since this is a rule for annotation
>>> processor, it does *not* come into picture when embeddable has
>>> metadata-complete = true in mapping XML file. In future, we can always
>>> have our own TopLink specific annotation to override this rule, or when
>>> then spec officially changes the rule, we can change our
>>> implementation.
>>>
>>> Rule 2: Allow mapping XML to provide a conflicting access-type for an
>>> embeddable only with *metadata-complete = true*. It is an *error* if
>>> XML
>>> provides a conflicting access-type for an embeddable with
>>> metadata-complete = false.
>>> Allowing XML to override access-type when metadata-complete = false
>>> requires elaborate rules which users may find difficult to understand,
>>> so why have it?
>>>
>>> Rule 3: It is an *error* to use an embeddable in entity classes with
>>> conflicting access-types in a persistence unit when the embeddable does
>>> not have metadata-complete = true. A ValidationException will be thrown
>>> for this case.
>>>
>>> I think, these rules keeps the behavior spec compliant as far as the
>>> current wordings in the spec go and yet we are able to provide the
>>> extra
>>> feature of sharing of an embeddable in entity classes with conflicting
>>> access type. Only difference is that, as of now user must use mapping
>>> XML to use this extra feature.
>>>
>>> The fix I sent for code review was already doing #1 and #3. I need
>>> to do
>>> #2 and send the code for review again. I hope not to see many
>>> objections
>>> to these rules.
>>>
>>> I am deliberately not mentioning behavior of verifier. Let's keep
>>> verifier out of this discussion.
>>>
>>> Thanks,
>>> Sahoo
>>>
>>> Marina Vatkina wrote:
>>>
>>>
>>>> Mike,
>>>>
>>>> This is not what the spec says in section "2.1.5 Embeddable Classes":
>>>>
>>>> "The access type for an embedded object is
>>>> determined by the access type of the entity in which it is embedded."
>>>>
>>>> There is nothing else in the spec that states otherwise (I searched
>>>> for embeddable), so the only option to support a mixed access type
>>>> correctly will be to introduce a product-specific annotation which
>>>> will make it absolutely clear that a developer uses a product-specific
>>>> feature.
>>>>
>>>> thanks,
>>>> -marina
>>>>
>>>> Mike Keith wrote On 08/09/06 12:23,:
>>>>
>>>>
>>>>
>>>>> Right. The combination of access types is undefined,
>>>>> but an embeddable and an entity are two separate
>>>>> classes and putting annotations on an embedded object has nothing
>>>>> to do with combining access types with the entity. They are
>>>>> completely separate objects and do not share, merge or combine
>>>>> persistent state.
>>>>>
>>>>> We actually did have technical reasons for not supporting
>>>>> different access types in inheritance hierarchies, and this
>>>>> was the reason why the limitation exists. The confusion factor
>>>>> went up significantly when we tried to define the rules for how
>>>>> field and property mappings were defaulted in an entity
>>>>> hierarchy with and without mapped superclasses.
>>>>>
>>>>> I agree that in the next release we will need to clarify.
>>>>> I also do not want to alienate features that make perfect sense
>>>>> and are essentially just miswordings of the spec. Non-portability
>>>>> is one thing, making them appear wrong is another.
>>>>>
>>>>> -Mike
>>>>>
>>>>>
>>>>>
>>>>>
>>>>>> -----Original Message-----
>>>>>> From: Linda.Demichiel_at_Sun.COM [mailto:Linda.Demichiel_at_Sun.COM]On
>>>>>> Behalf
>>>>>> Of Linda DeMichiel
>>>>>> Sent: Wednesday, August 09, 2006 2:23 PM
>>>>>> To: Mike Keith
>>>>>> Cc: Sanjeeb Kumar Sahoo; persistence_at_glassfish.dev.java.net
>>>>>> Subject: Re: code review for issue #831
>>>>>>
>>>>>>
>>>>>>
>>>>>> The spec is very consistent in treating the combination of
>>>>>> access types as undefined. The fact that it *could* work
>>>>>> does not make it any less undefined, or the spec any more
>>>>>> or less 'incorrect'. We didn't have any technical reasons
>>>>>> for not supporting different access types in mapped superclasses
>>>>>> either.
>>>>>>
>>>>>> At any rate, I do think we should look at clarifying this
>>>>>> in a future release, systematically with the other cases
>>>>>> for which more flexibility makes sense.
>>>>>>
>>>>>> Linda
>>>>>>
>>>>>>
>>>>>> Mike Keith wrote:
>>>>>>
>>>>>>
>>>>>>
>>>>>>
>>>>>>> The section in 2.1.1 applies to entity/class hierarchies and not
>>>>>>> really to embeddables. There is no technical reason to disallow
>>>>>>> an embeddable having a different access type from its owning entity
>>>>>>> since there is no conflict between it and the owning class
>>>>>>> (like there is in entity hierarchies where fields/properties are
>>>>>>> inherited). The problem was in the wording of that one line that
>>>>>>> discusses how the access type should be defined for an embeddable
>>>>>>> in the absence of metadata (since there is no PK and hence no
>>>>>>> requirement for there to be any metadata at all).
>>>>>>>
>>>>>>> Sahoo once asked me whether something like the following code
>>>>>>> would work:
>>>>>>>
>>>>>>> @Entity class Building {
>>>>>>> @Id int id;
>>>>>>> @Embedded Address address;
>>>>>>> }
>>>>>>> @Entity class Person {
>>>>>>> @Id int getId() { ... }
>>>>>>> @Embedded public Address getAddress() { ... }
>>>>>>> ...
>>>>>>> }
>>>>>>> @Embeddable class Address {
>>>>>>> String street;
>>>>>>> String city;
>>>>>>> String getStreet() { ... };
>>>>>>> String getCity() { ... };
>>>>>>> ...
>>>>>>> }
>>>>>>>
>>>>>>> The answer that I gave was that it would work fine and that
>>>>>>> to map the column names, etc, @AttributeOverride annotations
>>>>>>> should be used on the entities.
>>>>>>>
>>>>>>> The example should be able to work just as well, though, if the
>>>>>>> address were annotated, as in:
>>>>>>>
>>>>>>> @Embeddable class Address {
>>>>>>> @Column(name="STR")
>>>>>>> String street;
>>>>>>> String city;
>>>>>>> ...
>>>>>>> }
>>>>>>>
>>>>>>> This would simply mean that the embedded object is just being
>>>>>>> accessed using field access, which neither concerns nor affects
>>>>>>> the owning entity. In reality I can't even think of a really good
>>>>>>> use case for overriding the access type of an embedded
>>>>>>>
>>>>>> object to match
>>>>>>
>>>>>>
>>>>>>
>>>>>>> the entity that happens to be pointing to it.
>>>>>>>
>>>>>>> The *only* reason why the above example might not be permitted is
>>>>>>> because of the one incorrectly worded line in the spec that
>>>>>>>
>>>>>> I pointed
>>>>>>
>>>>>>
>>>>>>
>>>>>>> out. As it is there does not need to be any vendor-specific
>>>>>>>
>>>>>> annotations,
>>>>>>
>>>>>>
>>>>>>
>>>>>>> since just annotating the class is the obvious way to do this.
>>>>>>>
>>>>>>> -Mike
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>>> -----Original Message-----
>>>>>>>> From: Linda.Demichiel_at_Sun.COM
>>>>>>>> [mailto:Linda.Demichiel_at_Sun.COM]On Behalf
>>>>>>>> Of Linda DeMichiel
>>>>>>>> Sent: Wednesday, August 09, 2006 12:13 AM
>>>>>>>> To: Sanjeeb Kumar Sahoo
>>>>>>>> Cc: Mike Keith; persistence_at_glassfish.dev.java.net
>>>>>>>> Subject: Re: code review for issue #831
>>>>>>>>
>>>>>>>>
>>>>>>>> Sahoo,
>>>>>>>>
>>>>>>>> I agree with your assessment in your earlier email.
>>>>>>>> As stated in section 2.1.5, the access type for an embedded
>>>>>>>> object is determined by the access type of the entity in which
>>>>>>>> it is embedded, and as stated in section 2.1.1, the behavior
>>>>>>>> is unspecified if mapping annotations are applied to both
>>>>>>>> persistent fields and properties or if the XML descriptor
>>>>>>>> specifies use of different access types within a class hierarchy.
>>>>>>>> Specifying a different access type in an embeddable class than
>>>>>>>> in the entity which embeds it is undefined, and the verifier
>>>>>>>> should flag this case. Defined behavior exists only for the
>>>>>>>> case where a single access type applies for an entire entity
>>>>>>>> hierarchy.
>>>>>>>>
>>>>>>>> In the next rev of the spec, we can look at whether or not
>>>>>>>> we should define annotations on embeddables that imply a
>>>>>>>> different access type to be 'overriding' (and if so, whether
>>>>>>>> for the embedded class as a whole or for a specific field
>>>>>>>> or property). If we do so, we should do this more generally
>>>>>>>> (i.e., also handle the cases of subclasses defining different
>>>>>>>> access types as well).
>>>>>>>>
>>>>>>>> In the mean time, if it is important to support a vendor-specific
>>>>>>>> mixing of access types, we should use a product-specific
>>>>>>>> annotation
>>>>>>>> for that purpose.
>>>>>>>>
>>>>>>>> -Linda
>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>> Sanjeeb Kumar Sahoo wrote:
>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>>> Mike,
>>>>>>>>>
>>>>>>>>> If that was the intent, then why should an application be
>>>>>>>>>
>>>>>>>>>
>>>>>>>> non-portable
>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>>> if it uses different access-type in the embeddable? Or are we
>>>>>>>>> also
>>>>>>>>> missing a line like this in that same section of the spec:
>>>>>>>>>
>>>>>>>>>
>>>>>>>> /a portable
>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>>> app should not use access-type that is different from the
>>>>>>>>> enclosing
>>>>>>>>> entity class./
>>>>>>>>>
>>>>>>>>> More over, were similar lines missing for MappedSuperclass?
>>>>>>>>>
>>>>>>>>>
>>>>>>>> How about
>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>>> mixed access-type in an entity hierarchy?
>>>>>>>>>
>>>>>>>>> Thanks,
>>>>>>>>> Sahoo
>>>>>>>>> Mike Keith wrote:
>>>>>>>>>
>>>>>>>>>
>>>>>>>>>
>>>>>>>>>
>>>>>>>>>
>>>>>>>>>> // /When there is no metadata specified for an embedded
>>>>>>>>>>
>>>>>>>>>>
>>>>>> object, the
>>>>>>
>>>>>>
>>>>>>
>>>>>>>>>> access type for an embedded object *is* determined by the
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>> access type
>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>>>> of the entity in which it is embedded./
>>>>>>>>>>
>>>>>>>>>> -----Original Message-----
>>>>>>>>>> *From:* Mike Keith [mailto:michael.keith_at_oracle.com]
>>>>>>>>>> *Sent:* Tuesday, August 08, 2006 1:45 PM
>>>>>>>>>> *To:* Sanjeeb Kumar Sahoo; persistence_at_glassfish.dev.java.net;
>>>>>>>>>> Linda DeMichiel
>>>>>>>>>> *Subject:* RE: code review for issue #831
>>>>>>>>>>
>>>>>>>>>> The intent was never to disallow a vendor form doing
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>> so on their
>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>>>> own. Our wording in the first section might have
>>>>>>>>>> been better stated as:
>>>>>>>>>>
>>>>>>>>>> -----Original Message-----
>>>>>>>>>> *From:* Sanjeeb.Sahoo_at_Sun.COM
>>>>>>>>>> [mailto:Sanjeeb.Sahoo_at_Sun.COM]*On Behalf Of
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>> *Sanjeeb Kumar Sahoo
>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>>>> *Sent:* Tuesday, August 08, 2006 1:27 PM
>>>>>>>>>> *To:* persistence_at_glassfish.dev.java.net; Mike
>>>>>>>>>>
>>>>>> Keith; Linda
>>>>>>
>>>>>>
>>>>>>
>>>>>>>>>> DeMichiel
>>>>>>>>>> *Subject:* Re: code review for issue #831
>>>>>>>>>>
>>>>>>>>>> Hi Gordon,
>>>>>>>>>>
>>>>>>>>>> Thanks for reviewing. Are there any other comments?
>>>>>>>>>>
>>>>>>>>>> I totally agree with you that an embeddable should
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>> be allowed
>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>>>> to have its own access-type and that would have given more
>>>>>>>>>> options to the user, but unfortunately, the expert group
>>>>>>>>>> ignored this point when it was raised. More over,
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>> the spec is
>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>>>> *very very* confusing on this subject. In 2.1.5, it says:
>>>>>>>>>> /The access type for an embedded object *is*
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>> determined by the
>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>>>> access type of the entity in which it is embedded./
>>>>>>>>>> (Note, it says *is determined by* as opposed to *can be
>>>>>>>>>> determined*.). Yet, *orm_1_0.xsd* allows an
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>> access-type to be
>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>>>> specified for an embeddable, there by *contradicting* the
>>>>>>>>>> previous rule. Moreover, only non-portable
>>>>>>>>>>
>>>>>> applications can
>>>>>>
>>>>>>
>>>>>>
>>>>>>>>>> have different access-type for an embeddable as
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>> the spec says
>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>>>> the following in section #10.1.5.2:
>>>>>>>>>> /Portable applications must not specify the access
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>> attribute
>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>>>> if mapping annotations have been applied
>>>>>>>>>> to the fields or properties of the embeddable class or the
>>>>>>>>>> entity with which it is associated and the value
>>>>>>>>>> differs from the access type defined by means of
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>> annotations.
>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>>>> Portable applications must not use more than one
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>> access type
>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>>>> within an entity hierarchy.
>>>>>>>>>> /
>>>>>>>>>> So, until it is officially clarified in the spec,
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>> I think, we
>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>>>> *must not* use annotations in embeddable class to
>>>>>>>>>>
>>>>>>>>>>
>>>>>> determine
>>>>>>
>>>>>>
>>>>>>
>>>>>>>>>> access-type. I am also not in favor of allowing
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>> access-type of
>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>>>> an embeddable to be overridden in orm.xml unless
>>>>>>>>>>
>>>>>> we support
>>>>>>
>>>>>>
>>>>>>
>>>>>>>>>> the same extension for MappedSuperclass. Are you
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>> proposing to
>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>>>> support this for MappedSuperclass as well? Looking at the
>>>>>>>>>> code, I think, supporting this for
>>>>>>>>>>
>>>>>> MappedSuperclass will be
>>>>>>
>>>>>>
>>>>>>
>>>>>>>>>> tougher than that for Embeddable.
>>>>>>>>>>
>>>>>>>>>> Thanks,
>>>>>>>>>> Sahoo
>>>>>>>>>>
>>>>>>>>>> Gordon Yorke wrote:
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>>> Hello Sahoo,
>>>>>>>>>>> I have reviewed your code and have a suggestion. The
>>>>>>>>>>> Access Type of the Embeddable should still be determined
>>>>>>>>>>> based on
>>>>>>>>>>> annotations or XML (XML can define access type directly). This
>>>>>>>>>>> allows users to specify the access type of the
>>>>>>>>>>>
>>>>>> Embeddable. If the
>>>>>>
>>>>>>
>>>>>>
>>>>>>>>>>> access type is not available from the Embeddable then use
>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>> the access
>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>>>>> type of the parent. The specification states that the
>>>>>>>>>>>
>>>>>>>>>>>
>>>>>> access type
>>>>>>
>>>>>>
>>>>>>
>>>>>>>>>>> can be 'determined' from that of the parent but is not
>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>> mandated to be
>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>>>>> that of the parent( Mike, is that not the intention of the
>>>>>>>>>>> specification?) This would give users much more flexibility
>>>>>>>>>>> for
>>>>>>>>>>> sharing Embeddables.
>>>>>>>>>>> Also based on how TopLink treats Embeddable
>>>>>>>>>>>
>>>>>>>>>>>
>>>>>> mappings we
>>>>>>
>>>>>>
>>>>>>
>>>>>>>>>>> could, in the future, support a different access type for
>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>> each target
>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>>>>> of an Embeddable mapping if the AccesType of the
>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>> Embeddable was not
>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>>>>> supplied by the user.
>>>>>>>>>>> --Gordon
>>>>>>>>>>>
>>>>>>>>>>> -----Original Message-----
>>>>>>>>>>> From: Sanjeeb.Sahoo_at_Sun.COM
>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>> [mailto:Sanjeeb.Sahoo_at_Sun.COM]On
>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>>>>> Behalf Of
>>>>>>>>>>> Sanjeeb Kumar Sahoo
>>>>>>>>>>> Sent: Monday, August 07, 2006 9:00 AM
>>>>>>>>>>> To: Tom Ware
>>>>>>>>>>> Cc: persistence_at_glassfish.dev.java.net
>>>>>>>>>>> Subject: code review for issue #831
>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>> Hi Tom,
>>>>>>>>>>>
>>>>>>>>>>> Attached here with the suggested fix for
>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>> https://glassfish.dev.java.net/issues/show_bug.cgi?id=831 .
>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>>>>> The jar file contains modified sources in new folder,
>>>>>>>>>>> original sources
>>>>>>>>>>> in orig folder and differences in diffs folder.
>>>>>>>>>>> Please review them. I have update issue tracker with the
>>>>>>>>>>> details.
>>>>>>>>>>>
>>>>>>>>>>> Tests run:
>>>>>>>>>>> entity-persistence-tests(all of them passed)
>>>>>>>>>>> JPA TCK v1.0: 3 tests failed but they fail on a clean
>>>>>>>>>>> workspace as well
>>>>>>>>>>> (Yes, I have run those 3 tests in a clean
>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>> workspace and they
>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>>>>> failed).
>>>>>>>>>>> So, my changes are not causing those failures.
>>>>>>>>>>>
>>>>>>>>>>> Appreciate a quick reply.
>>>>>>>>>>>
>>>>>>>>>>> Thanks,
>>>>>>>>>>> Sahoo
>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>>
>>>>
>>>>
>>>>
>