Hi Jielin,
the changes look good!
Just a heads up: Wonseok plans to change some Date values in the test
data of entity-persistence-tests (see mail thread with subject "Some
tests in JUnitEJBQLDateTimeTestSuite failed on Oracle"). Is your change
affected by this?
Regards Michael
> 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
>>>>
>>>>
>>>>