persistence@glassfish.java.net

Re: code reivew for issue 1355

From: jielin <Jie.Leng_at_Sun.COM>
Date: Mon, 18 Dec 2006 15:47:25 -0800

Hi, Michael,

I made the change.
I added two unit tests in the past to test update embedded field and
test unknown enumerated class constant. Would you please also review the
change?

Thanks.

Jielin

Michael Bouschen wrote:

> Hi Jielin,
>
> looks good! Just one comment about class DotNode.java, method
> checkNavigation:
> For the invalidNavigation exception I propose to take line and column
> information from the specified node parameter (so node.getLine(),
> node.getColumn()). The other exception invalidCollectionNavigation in
> method checkNavigation is fine.
>
> Regards Michael
>
>> Hi, Micahel,
>>
>> Attached is the change based on your comments. Would you please
>> review them?
>>
>> Thanks.
>>
>> Jielin
>>
>> Michael Bouschen wrote:
>>
>>> Hi Jielin,
>>>
>>> please find my remarks below. Most of the remarks are about taking
>>> line and column information from a different node than the current one.
>>>
>>> EJBQLException.java/EJBQLExceptionResource.java:
>>> - I propose to add line and column information for error message
>>> 8018 invalidMultipleUseOfSameParameter. Method defineParameterType
>>> of ParseTreeContext could take line and column as additional
>>> parameters and pass it to the error message. The method is called
>>> from ParameterNode.
>>> - The same appies for error message 8019
>>> multipleVariableDeclaration. Methods registerSchema and
>>> registerJoinVariable of ParseTreeContext could get additional line
>>> and column parameters.
>>>
>>> EJBQLExceptionResource.java:
>>> - The error message of 8032 does not use the attribute name. I
>>> propose to change the text to: Invalid access of attribute [{0}] in
>>> SET clause target [{1}], only state fields ...
>>> - The error message of entry 8033 is broken.
>>> - In the definition of field contents there is a typo in the second
>>> entry from the bottom: it should have the number 8037 instead of 8034.
>>> - The error message of 8038 should start with an 'a': "line {1},
>>> column {2}: a problem ...
>>>
>>> DotNode.java:
>>> Using getLine() and getColumn in the class DotNode returns the
>>> position of the dot itself. In some cases we can give a better error
>>> message when using the position of the erroneous syntax element
>>> mentioned in the error message. E.g. if the error message says
>>> unknown attribute we should use the position of the attribute
>>> instead of the position of the dot:
>>> - Method validate, exception unknownAttribute: right.getLine(),
>>> right.getColumn()
>>> - Method validate, exception resolveEnumConstant: right.getLine(),
>>> right.getColumn()
>>> - Method validate, exception aliasResolutionException:
>>> leftMost.getLine(), leftMost.getColumn()
>>> - Method checkNavigation, exception invalidCollectionNavigation
>>> right.getLine(), right.getColumn()
>>>
>>> EqualsAssignmentNode.java:
>>> - Method validateTarget
>>> I propose to use the line and column information of the attribute
>>> node for the invalidSetClauseTarget exception. You could intoduce a
>>> new local variable attributeNode and then use
>>> attributeNode.getLine() and attributeNode.getColumn() for the error
>>> message:
>>> Node attributeNode = (AttributeNode)node.getRight();
>>> String attribute = attributeNode.getAttributeName();
>>> ...
>>> - Method validateNavigation
>>> Same as above: use attributeNode.getLine() and
>>> attributeNode.getColumn()
>>> Node attributeNode = (AttributeNode)qualifier.getRight();
>>> String attribute = attributeNode.getAttributeName();
>>> ...
>>>
>>> GroupByNode.java:
>>> - Method validate: use line and column information of the select
>>> expression (selectExpr.getLine() and selectExpr.getColumn()).
>>>
>>> HavingNode.java:
>>> - Method validate: use line and column information of the having
>>> expression (having.getLine() and having.getColumn()).
>>>
>>> InNode.java:
>>> - Method validate: use line and column information of the node in
>>> the IN expression list (node.getLine() and node.getColumn()).
>>>
>>> ModNode.java:
>>> - Method validate: use line and column information of the mode
>>> argument nodes (left.getLine() and left.getColumn(),
>>> denominator.getLine() and denominator.getColumn()).
>>>
>>> OrderByItemNode.java:
>>> - Method validate: use line and column information of the order by
>>> item node (orderByItem.getLine() and orderByItem.getColumn()).
>>>
>>> Regards Michael
>>>
>>>> Hi, Michael,
>>>>
>>>> Attched is the changes for issue 1355. The issue is about adding
>>>> line and column information when there is any error in a Java
>>>> Persistence query results in a EJBQLException.
>>>>
>>>> Basically, there are two type of error handling for jpql in the
>>>> current code: one is thrown from
>>>> EJBQLParser.handleANTLRException(); the other is all validation
>>>> methods in the Node subclasses. Line and column information are
>>>> added in these two category. If the same error method is used in
>>>> other place and don't have line and column information, a new
>>>> method is added with same same having suffix 2.
>>>>
>>>> Would you please review the changes?
>>>>
>>>> Thanks.
>>>>
>>>> Jielin
>>>
>>>
>>>