Mike, Tom, Marina,
The modified rules are in the javadoc of the following method which is
going to be used to decide access-type of an embeddable class. Please
review it and get back soon. AccessType is defined like this:
enum AccessType {FIELD, PROPERTY, UNDEFINED, MIXED};
/**
* This method is responsible for determining the access-type for an
* Embeddable class represented by emDescr that is passed to
* this method. This method should *not* be called more than once as
this is
* quite expensive. Now the rules:
*
* Rule 1: In the *absence* of metadata in embeddable class,
access-type of
* an embeddable is determined by the access-type of the enclosing
entity.
*
* Rule 2: In the presence of metadata in embeddable class,
access-type of
* an embeddable is determined using that metadata. This allows sharing
* of the embeddable in entities with conflicting access-types.
*
* Rule 3: It is an error to use a *metadata-less* embeddable class in
* entities with conflicting access-types as that might result in
* different database mapping for the same embeddable class.
*
* Rule 4: It is an error if metadata-complete == false, and
* metadata is present *both* in annotations and XML, and
* access-type as determined by each of them is *not* same.
*
* Rule 5: It is an error if *both* fields and properties of an
embeddable
* class are annotated and metadata-complete == false.
*
* @param emDesc descriptor for the Embeddable class
*/
private AccessType determineAccessTypeOfEmbedded(
MetadataDescriptor emDesc) {
final AccessType entityAccessType =
m_descriptor.usesPropertyAccess() ?
PROPERTY : FIELD;
// access-type of embeddable defaults to that of enclosing entity's
AccessType accessType = UNDEFINED;
boolean metadataComplete =emDesc.ignoreAnnotations();
AccessType accessTypeUsingAnnottaion =
computeAccessTypeFromAnnotation(emDesc);
AccessType accessTypeUsingXML =
computeAccessTypeFromXML(emDesc);
if (metadataComplete) {
// metadata-complete is true, then use XML access-type if
defined,
// else use enclosing entity's access-type.
accessTypeUsingXML = accessTypeUsingXML != UNDEFINED ?
accessTypeUsingXML : entityAccessType;
} else {// metadata-complete is false
if (accessTypeUsingAnnottaion == UNDEFINED
&& accessTypeUsingAnnottaion == UNDEFINED) {
// metadata is absent, so we use enclosing entity's
access-type
accessType = entityAccessType;
} else if (accessTypeUsingXML == UNDEFINED
&& accessTypeUsingAnnottaion != UNDEFINED) {
// annottaion is present in embeddable class
accessType = accessTypeUsingAnnottaion;
} else if (accessTypeUsingAnnottaion == UNDEFINED
&& accessTypeUsingXML != UNDEFINED) {
// access is defined using XML for embeddable class
accessType = accessTypeUsingXML;
} else if (accessTypeUsingAnnottaion == accessTypeUsingXML) {
// annotation is present as well as access is defined
using XML
// and they are same. So use it.
accessType = accessTypeUsingAnnottaion;
} else {
// annotation is present as well as access is defined
using XML
// and they are different. So report an exception.
m_validator.throwIncorrectOverridingOfAccessType(
emDesc.getJavaClass(),
accessTypeUsingAnnottaion.toString(),
accessTypeUsingXML.toString());
}
}
if (accessType == MIXED) {
m_validator.throwBothFieldsAndPropertiesAnnotatedException(emDesc.getJavaClass());
}
return accessType;
}
Thanks,
Sahoo
Marina Vatkina wrote:
> Sahoo, Team,
>
> We discussed with Tom the short term requirements to fix this issue, and
> agreed that for now we should not change the behavior supported by the
> current code. Tom, please correct me if I miss anything.
>
> TopLink is able to support sharing an Embeddable between 2 entities with
> different access type, because it creates a copy of the model for each
> owner. Which means that the best current approach to fix this bug (we can
> continue philosophical discussions about the features) should be:
>
> Rule 1: If an Embeddable, does not define annotations, its access type
> is determined by the access type of the owning entity.
>
> Rule 3: It's an error for a verifier mode to detect a conflict access
> type (non-portable application) either between an entity and an
> embeddable,
> or between two entities sharing the same embeddable class.
>
> Sahoo, I'll leave it for you to determine how the rules above affect
> rule #2.
>
> regards,
> -marina
>
> Sanjeeb Kumar Sahoo wrote:
>> 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
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>
>>>
>>>
>>>
>>
>