Skip to content
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

[Feature] IN operator #1165

Merged
merged 11 commits into from
Jun 22, 2018
Merged

Conversation

AlanWong-MS
Copy link
Contributor

@AlanWong-MS AlanWong-MS commented May 15, 2018

Issues

#1147

Description

Implementation of the IN operator as part of the OData 4.01 spec (see ABNF).

This PR includes the IN operator feature, which includes a new class InNode, modifications to the expression lexer to recognize the "in" keyword, and unit and functional test cases.

Exclusions of implementation:

  • The ABNF indicates that brackets are supported for enclosing the lists; however, this particular feature is currently excluded from implementation due to some breaking changes in code. For now, lists are supported only with parentheses.

Checklist (Uncheck if it is not completed)

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

…e to support feature. Adding corresponding tests.
@@ -285,7 +285,7 @@ public virtual bool Read()
}

// We might see end of array here
if (this.characterBuffer[this.tokenStartIndex] == ']')
if (this.characterBuffer[this.tokenStartIndex] == ']' || this.characterBuffer[this.tokenStartIndex] == ')')
{
Copy link
Member

@mikepizzo mikepizzo Jun 5, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will this also allow me to terminate a JSON array with a closing parens? i.e., does "names": ["bob","sue") now parse as valid? #Closed

Copy link
Contributor Author

@AlanWong-MS AlanWong-MS Jun 5, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great question. The answer is no and I don't have an existing test case for this but I've tested it. I have a test case for the vice-versa scenario (since we currently don't allow bracketed collections in IN expression):

public void FilterWithInOperationWithMismatchedClosureCollection()
{
    Action parse = () => ParseFilter("ID in (1,2,3]", HardCodedTestModel.TestModel, HardCodedTestModel.GetPersonType());
    parse.ShouldThrow<ODataException>().WithMessage(ODataErrorStrings.ExpressionLexer_UnbalancedBracketExpression);
}

I added the test case that you described with the closing parens and will update the PR after we discuss the other items that you have. No product code changes needed as the parser already takes care of this case. #Closed

@mikepizzo
Copy link
Member

    public virtual bool Read()

Let's talk through how/where JsonReader gets called for this case; it would be nice not to have to add parens parsing to the JSON reader, as it is not a JSON concept.


Refers to: src/Microsoft.OData.Core/Json/JsonReader.cs:220 in becc74f. [](commit_id = becc74f, deletion_comment = False)

this.left = left;
this.right = right;

if (this.left.GetEdmTypeReference().Definition != this.right.ItemType.Definition)
Copy link
Member

@mikepizzo mikepizzo Jun 5, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this.left.GetEdmTypeReference().Definition != this.right.ItemType.Definition [](start = 16, length = 76)

We should support the type of the right node collection being comparable to the type of the left node (including being derived from the type of the left node). See EdmLibraryExtensions.IsAssignableFrom. #Closed

Copy link
Contributor Author

@AlanWong-MS AlanWong-MS Jun 5, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the catch, I think I had forgotten to jot down the point regarding derived types during code review. Regarding supporting comparable types, is there a function or API that you're specifically referring to or is what I currently have in the code of comparing the definitions sufficient? #Closed

… logic and instead, passing in a bracket-based collection and keeping the parens-based collection as the literal text
@AlanWong-MS AlanWong-MS merged commit a870695 into OData:master Jun 22, 2018
biaol-odata pushed a commit to biaol-odata/odata.net that referenced this pull request Jun 26, 2018
* Implementing IN operator. Define new InNode and CollectionConstantNode to support feature. Adding corresponding tests.
* The ABNF indicates that brackets are supported for enclosing the lists; however, this particular feature is currently excluded from implementation due to some breaking changes in code. For now, lists are supported only with parentheses.
biaol-odata pushed a commit that referenced this pull request Jun 26, 2018
* Implementing IN operator. Define new InNode and CollectionConstantNode to support feature. Adding corresponding tests.
* The ABNF indicates that brackets are supported for enclosing the lists; however, this particular feature is currently excluded from implementation due to some breaking changes in code. For now, lists are supported only with parentheses.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants