New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Include dots in parseIdentifier for annotation parsing #1123

Merged
merged 2 commits into from Jun 21, 2018

Conversation

Projects
None yet
2 participants
@biaol-odata
Member

biaol-odata commented Mar 21, 2018

Issues

This pull request fixes an issue for annotation parsing in ODL 7.4.2.

Description

Annotation such as "@NS.myAnnotation" are not parsed correctly as Identifier token because lexer stops at the dot char during the traversing. This can be fixed by allowing to include dot chars for paring string token starting with "@", which could be either an parameter alias (w/o dots) or an annotation.

Checklist (Uncheck if it is not completed)

  • Test cases added
  • Build and test with one-click build and test script passed

Additional work necessary

If documentation update is needed, please add "Docs Needed" label to the issue and provide details about the required document change in the issue.

@@ -1201,14 +1203,15 @@ private void AdvanceThroughBalancedExpression(char startingCharacter, char endin
/// <summary>Parses an identifier by advancing the current character.</summary>
private void ParseIdentifier()
/// <param name="includingDots">Optional flag for whether to include dots as part of the identifier.</param>
private void ParseIdentifier(bool includingDots = false)

This comment has been minimized.

@mikepizzo

mikepizzo Mar 22, 2018

Member

bool includingDots = false [](start = 37, length = 26)

We pass this as true if the leading character is an "@", otherwise we pass this as false. Rather than making this a parameter, should we just set includingDots = this.ch==UriQueryConstants.AnnotationPrefix within the function? #ByDesign

This comment has been minimized.

@biaol-odata

biaol-odata Mar 22, 2018

Member

It would be equivalent to the current logic, but explicitly passing in as bool from caller is causing less ripple, cleaner therefore less risky and more flexible in case this method is needed later in other place. Therefore I prefer explicitly passing in a bool value unless you have strong opinion on this.


In reply to: 176544495 [](ancestors = 176544495)

this.ParseIdentifier();
// Include dots for the case of annotation.
this.ParseIdentifier(true /*includingDots*/);

This comment has been minimized.

@mikepizzo

mikepizzo Mar 22, 2018

Member

true /includingDots/ [](start = 45, length = 22)

So we allow dots for either the parameter case or the annotation case. Dots aren't really allowed in parameter names, but for the purpose of parsing the expression it is probably fine to include them as part of the identifier (i.e., there's not a valid case where a dot would be a valid separator between a parameter and something else, so including it in the identifier is reasonable) #ByDesign

This comment has been minimized.

@biaol-odata

biaol-odata Mar 22, 2018

Member

Yes, should be OK to include dots, which is further used to differentiate parameter alias and annotation per OData protocol. Please let me know if I miss anything in your comment.


In reply to: 176545588 [](ancestors = 176545588)

@mikepizzo

This comment has been minimized.

Member

mikepizzo commented Mar 22, 2018

Can we add some additional tests for the UriParser, to parse different expressions containing combinations of annotations and parameter markers? i.e.; with this change, the lexer will support the parser parsing @my.annotation eq 5 in a filter expression; that should either throw a reasonable exception or parse to an annotation node as the left operand; it should not successfully parse as a parameter as the left operand. #Resolved

@biaol-odata

This comment has been minimized.

Member

biaol-odata commented Mar 22, 2018

Sure. Please see two test cases added in ODataUriParserTests in the commit above. #Resolved

@mikepizzo mikepizzo merged commit fd59035 into OData:master Jun 21, 2018

2 checks passed

continuous-integration/vsts The build trigger has fired.
Details
license/cla All CLA requirements met.
Details

biaol-odata added a commit to biaol-odata/odata.net that referenced this pull request Jun 26, 2018

Include dots in parseIdentifier for annotation parsing (OData#1123)
* Include dots in parseIdentifier so that annotation token can be parsed correctly.

* Add UriParser test cases per CR comment.

biaol-odata added a commit that referenced this pull request Jun 26, 2018

Include dots in parseIdentifier for annotation parsing (#1123)
* Include dots in parseIdentifier so that annotation token can be parsed correctly.

* Add UriParser test cases per CR comment.

@biaol-odata biaol-odata self-assigned this Jun 27, 2018

@biaol-odata biaol-odata deleted the biaol-odata:lexerParseIdentifier branch Jun 27, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment