persistence@glassfish.java.net

Re: code review for issue 743

From: Michael Bouschen <Michael.Bouschen_at_Sun.COM>
Date: Thu, 24 Aug 2006 17:51:12 +0200

Hi Jielin,

looks good! Two minor remarks:
- The javadoc in TypeHelper.isEmbeddable talks about "embedded entity
class". I propose to skip the word entity.
- I propose to rename method EJBQLException.notSupportJoinArgument to
EJBQLException.unsupportedJoinArgument (and the same name change for the
corresponding constant).

Regards Michael

> Hi, Michael/Tom,
>
> Here is the change based on previous comments. I added
> isEmbeddedAttribute(Object ownerClass, String attribute) in TypeHelper
> and use this API in JoinDeclNode and FetchJoinNode for validation.
>
> Please review the change.
>
> Thanks.
>
> Jielin
>
> Tom Ware wrote:
>
>> Hi,
>>
>> Sahoo is right. Embeddable is called Aggregate in TopLink.
>> isAggregateMapping() should provide this information.
>>
>> -Tom
>>
>> Sanjeeb Kumar Sahoo wrote:
>>
>>> Hi Michael,
>>>
>>> Let me try answering your question. Someone like Tom can override...
>>>
>>> public boolean isEmbeddedAttribute(Object ownerClass, String
>>> attribute) {
>>> ClassDescriptor desc = session.getDescriptor((Class)type);
>>> return
>>> desc.getMappingForAttributeName(attribute).isAggregateMapping();
>>> }
>>>
>>> I don't think you need isEmbeddable() anymore. A validation exception
>>> gets reported when an embedded attribute's type is not an embeddable.
>>>
>>> Thanks,
>>> Sahoo
>>> Michael Bouschen wrote:
>>>
>>>
>>>> Hi Jielin,
>>>>
>>>> I'm wondering whether we have to distinguish @Embeddable and
>>>> @Embedded here. I think an entity class might have an @Embeddable
>>>> annotation. So the TypeHelper method taking a type representation
>>>> should be called isEmbeddable:
>>>> public boolean isEmbeddable(Object type);
>>>>
>>>> I think we need another TypeHelper method that checks whether a
>>>> field or property is annotated with the @Embedded annotation, so
>>>> something like:
>>>> public Object isEmbeddedAttribute(Object ownerClass, String attribute);
>>>> The method is called in the validate method of JoinDeclNode and
>>>> FetchJoinNode. The path property of the two nodes is a DotNode. The
>>>> type of the left child is the ownerClas, the name of the right child
>>>> (an AttributeNode) is the attribute parameter for the call.
>>>>
>>>> I'm not sure how the @Embedded annotation is represented in the
>>>> TopLink mapping classes, so I do not know how to implement method
>>>> isEmbeddedAttribute. Tom, do you know?
>>>>
>>>> Regards Michael
>>>>
>>>>
>>>>> Hi, Michael/Tom,
>>>>>
>>>>> Issue 743 Join of embedded entities should be disallowed is fix in
>>>>> this jar. Also the unit test for this issue is included. Would you
>>>>> please review the code?
>>>>>
>>>>> The changes are:
>>>>>
>>>>> Added isEmbeddedEntity() API.
>>>>> entity-persistence/src/java/oracle/toplink/essentials/internal/parsing/TypeHelperImpl.java
>>>>>
>>>>> entity-persistence/src/java/oracle/toplink/essentials/internal/parsing/TypeHelper.java
>>>>>
>>>>>
>>>>> Added code for check if the entity is embedded or not
>>>>> entity-persistence/src/java/oracle/toplink/essentials/internal/parsing/JoinDecNode.java
>>>>>
>>>>> entity-persistence/src/java/oracle/toplink/essentials/internal/parsing/FetchJoinNode.java
>>>>>
>>>>>
>>>>> Added EJBQL error message:
>>>>> entity-persistence/src/java/oracle/toplink/essentials/exceptions/EJBQLException.java
>>>>>
>>>>> entity-persistence/src/java/oracle/toplink/essentials/exceptions/i18n/EJBQLExceptionResource.java
>>>>>
>>>>>
>>>>> Added unit test
>>>>> entity-persistence-tests/src/java/oracle/toplink/essentials/testing/tests/ejb/ejbqltesting/JUnitEJBQLValidationTestSuite.java
>>>>>
>>>>>
>>>>> Thanks.
>>>>>
>>>>> Jielin
>>>>>
>>>>
>>
>