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
>>>>>>>>>
>>>>>>>>>
>>>>>>>>>
>>>>>>>>>
>>>>>>>>
>>>>>>>>
>>
>>
>>
>