Updated ODataUriParser to allow select and expand property names that… #2437

corranrogue9 · 2022-06-20T22:35:12Z

… conflict with reserved keywords

Issues

This pull request fixes #2410.

Description

The SelectExpandParser used by the ODataUriParser takes a two-pass approach to parsing the select and expand options. The first pass is intended to be syntactic only, while the second pass applies semantics. For code re-use, The syntactic pass leverages the ExpressionLexer. However, ExpressionLexer applies some semantics while parsing when a perceived "identifier" token is reached. I have added a flag in ExpressionLexer to optionally disable these semantics, and updated the SelectExpandParser to disable this behavior for its lexer.

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.

… conflict with reserved keywords

pull-request-quantifier-deprecated · 2022-06-20T22:35:37Z

This PR has 293 quantified lines of changes. In general, a change size of upto 200 lines is ideal for the best PR experience!

Quantification details

Label      : Large
Size       : +291 -2
Percentile : 69.3%

Total files changed: 3

Change summary by file extension:
.cs : +291 -2

Change counts above are quantified counts, based on the PullRequestQuantifier customizations.

Why proper sizing of changes matters

Optimal pull request sizes drive a better predictable PR flow as they strike a
balance between between PR complexity and PR review overhead. PRs within the
optimal size (typical small, or medium sized PRs) mean:

Fast and predictable releases to production:
- Optimal size changes are more likely to be reviewed faster with fewer
  iterations.
- Similarity in low PR complexity drives similar review times.
Review quality is likely higher as complexity is lower:
- Bugs are more likely to be detected.
- Code inconsistencies are more likely to be detetcted.
Knowledge sharing is improved within the participants:
- Small portions can be assimilated better.
Better engineering practices are exercised:
- Solving big problems by dividing them in well contained, smaller problems.
- Exercising separation of concerns within the code changes.

What can I do to optimize my changes

Use the PullRequestQuantifier to quantify your PR accurately
- Create a context profile for your repo using the context generator
- Exclude files that are not necessary to be reviewed or do not increase the review complexity. Example: Autogenerated code, docs, project IDE setting files, binaries, etc. Check out the Excluded section from your prquantifier.yaml context profile.
- Understand your typical change complexity, drive towards the desired complexity by adjusting the label mapping in your prquantifier.yaml context profile.
- Only use the labels that matter to you, see context specification to customize your prquantifier.yaml context profile.
Change your engineering behaviors
- For PRs that fall outside of the desired spectrum, review the details and check if:
  - Your PR could be split in smaller, self-contained PRs instead
  - Your PR only solves one particular issue. (For example, don't refactor and code new features in the same PR).

How to interpret the change counts in git diff output

One line was added: +1 -0
One line was deleted: +0 -1
One line was modified: +1 -1 (git diff doesn't know about modified, it will
interpret that line like one addition plus one deletion)
Change percentiles: Change characteristics (addition, deletion, modification)
of this PR in relation to all other PRs within the repository.

Was this comment helpful? 👍 :ok_hand: :thumbsdown: (Email)
Customize PullRequestQuantifier for this repository.

xuzhg · 2022-06-21T18:05:56Z

src/Microsoft.OData.Core/UriParser/Parsers/SelectExpandParser.cs

@@ -81,7 +81,7 @@ internal sealed class SelectExpandParser

            // Sets up our lexer. We don't turn useSemicolonDelimiter on since the parsing code for expand options,
            // which is the only thing that needs it, is in a different class that uses it's own lexer.
-            this.lexer = clauseToParse != null ? new ExpressionLexer(clauseToParse, false /*moveToFirstToken*/, false /*useSemicolonDelimiter*/) : null;
+            this.lexer = clauseToParse != null ? new ExpressionLexer(clauseToParse, false, false, false, false) : null;


false

so, only in select and expand, it's false, in others it's true?

We don't allow: $filter=true eq true, if one of the 'true' is for the property?

There's a lot going on here. For $filter=true eq true, that's the same as just saying $filter=true or not providing the filter at all. We interpret the "true" literal as a boolean expression, which as far as I can tell is in accordance with the standard. If there is a property named "true", the above is still accurate, but the client can disambiguate that they are referring to the property by using $it, as in $filter=$it/true eq true. The reality is that we also have a bug in that case as well. I talked with Mike yesterday and we decided that we would address the $filter issue separately, since it implies a larger change (specifically, ExpressionLexer applies semantics, but many query options require a syntax-only pass first; this syntax-only pass using ExpressionLexer has issues across all of the query option implementations).

mikepizzo · 2022-07-01T20:02:07Z

test/FunctionalTests/Microsoft.OData.Core.Tests/UriParser/ODataUriParserTests.cs

+            var parser = new ODataUriParser(model, new Uri("/foos?$select=null", UriKind.Relative));
+            var odataException = Assert.Throws<ODataException>(() => parser.ParseUri());
+
+            Assert.Equal("Could not find a property named 'null' on type 'foobar.foo'.", odataException.Message);


Should have another test where the type is defined as open -- in which case, no error should be returned.

mikepizzo · 2022-07-01T20:03:51Z

src/Microsoft.OData.Core/UriParser/ExpressionLexer.cs

@@ -108,6 +113,19 @@ internal ExpressionLexer(string expression, bool moveToFirstToken, bool useSemic
        /// <param name="useSemicolonDelimiter">If true, the lexer will tokenize based on semicolons as well.</param>
        /// <param name="parsingFunctionParameters">Whether the lexer is being used to parse function parameters. If true, will allow/recognize parameter aliases and typed nulls.</param>
        internal ExpressionLexer(string expression, bool moveToFirstToken, bool useSemicolonDelimiter, bool parsingFunctionParameters)


This is internal -- how many places do we call this existing method from? Would it be easier to add the parameter to the existing overload and explicitly pass "true", rather than define a new overload?

Something unrelated --
I have in the past considered making the ExpressionLexer public and customizable.

KenitoInc · 2022-07-04T11:32:00Z

src/Microsoft.OData.Core/UriParser/ExpressionLexer.cs

+        /// <summary>
+        /// Whether or not to interpret the semantics of a token that appears to be an identifier (i.e. consider if it is a keyword instead of an identifier)
+        /// </summary>
+        private readonly bool applySemanticsToIdentifiers;


Where are we setting this?
I think we should have this in the ODataUriParserSettings.

how can customer config this?

habbes

Looks good to me. Left minor suggestions to improve legibility.

habbes · 2022-07-25T13:35:06Z

src/Microsoft.OData.Core/UriParser/ExpressionLexer.cs

@@ -108,6 +113,19 @@ internal ExpressionLexer(string expression, bool moveToFirstToken, bool useSemic
        /// <param name="useSemicolonDelimiter">If true, the lexer will tokenize based on semicolons as well.</param>
        /// <param name="parsingFunctionParameters">Whether the lexer is being used to parse function parameters. If true, will allow/recognize parameter aliases and typed nulls.</param>
        internal ExpressionLexer(string expression, bool moveToFirstToken, bool useSemicolonDelimiter, bool parsingFunctionParameters)
+            : this(expression, moveToFirstToken, useSemicolonDelimiter, parsingFunctionParameters, true)


For better legibility:

Suggested change

: this(expression, moveToFirstToken, useSemicolonDelimiter, parsingFunctionParameters, true)

: this(expression, moveToFirstToken, useSemicolonDelimiter, parsingFunctionParameters, applySemanticsToIdentifiers: true)

habbes · 2022-07-25T13:43:23Z

src/Microsoft.OData.Core/UriParser/Parsers/SelectExpandParser.cs

@@ -81,7 +81,7 @@ internal sealed class SelectExpandParser

            // Sets up our lexer. We don't turn useSemicolonDelimiter on since the parsing code for expand options,
            // which is the only thing that needs it, is in a different class that uses it's own lexer.
-            this.lexer = clauseToParse != null ? new ExpressionLexer(clauseToParse, false /*moveToFirstToken*/, false /*useSemicolonDelimiter*/) : null;
+            this.lexer = clauseToParse != null ? new ExpressionLexer(clauseToParse, false, false, false, false) : null;


Hard to tell what all these false params are for, suggestion to use named parameters in such cases:

Suggested change

this.lexer = clauseToParse != null ? new ExpressionLexer(clauseToParse, false, false, false, false) : null;

this.lexer = clauseToParse != null ? new ExpressionLexer(clauseToParse, moveToFirstToken: false, useSemicolonDelimiter: false, parsingFunctionParameters: false, applySemanticsToIdentifiers: false) : null;

habbes · 2022-07-25T13:48:26Z

test/FunctionalTests/Microsoft.OData.Core.Tests/UriParser/ODataUriParserTests.cs

+        }
+
+        /// <summary>
+        /// Selects a property named "null" when such a property doesn't exist on the entity


I think this comment should specify that this returns an error. On first reading, I assumed it actually does allow you to $select a property name "null" without any issues, even when it doesn't exist.

habbes · 2022-07-25T13:49:17Z

test/FunctionalTests/Microsoft.OData.Core.Tests/UriParser/ODataUriParserTests.cs

+        }
+
+        /// <summary>
+        /// Expands a property named "null" when such a property doesn't exist on the entity


I think comment should mention that this results in an exception.

xuzhg · 2022-07-25T20:56:48Z

Others look good to me

Updated ODataUriParser to allow select and expand property names that…

af5f87d

… conflict with reserved keywords

pull-request-quantifier-deprecated bot added the Large label Jun 20, 2022

xuzhg reviewed Jun 21, 2022

View reviewed changes

mikepizzo reviewed Jul 1, 2022

View reviewed changes

KenitoInc reviewed Jul 4, 2022

View reviewed changes

habbes reviewed Jul 25, 2022

View reviewed changes

xuzhg approved these changes Jul 25, 2022

View reviewed changes

habbes mentioned this pull request Jan 31, 2024

Property named "true" in $select throws exception #2410

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Updated ODataUriParser to allow select and expand property names that… #2437

Updated ODataUriParser to allow select and expand property names that… #2437

corranrogue9 commented Jun 20, 2022

pull-request-quantifier-deprecated bot commented Jun 20, 2022

What can I do to optimize my changes

How to interpret the change counts in git diff output

xuzhg Jun 21, 2022

corranrogue9 Jun 22, 2022 •

edited

Loading

mikepizzo Jul 1, 2022

mikepizzo Jul 1, 2022

KenitoInc Jul 4, 2022

KenitoInc Jul 4, 2022

xuzhg Jul 25, 2022

habbes left a comment

habbes Jul 25, 2022

habbes Jul 25, 2022

habbes Jul 25, 2022

habbes Jul 25, 2022

xuzhg commented Jul 25, 2022

	: this(expression, moveToFirstToken, useSemicolonDelimiter, parsingFunctionParameters, true)
	: this(expression, moveToFirstToken, useSemicolonDelimiter, parsingFunctionParameters, applySemanticsToIdentifiers: true)

	this.lexer = clauseToParse != null ? new ExpressionLexer(clauseToParse, false, false, false, false) : null;
	this.lexer = clauseToParse != null ? new ExpressionLexer(clauseToParse, moveToFirstToken: false, useSemicolonDelimiter: false, parsingFunctionParameters: false, applySemanticsToIdentifiers: false) : null;

Updated ODataUriParser to allow select and expand property names that… #2437

Are you sure you want to change the base?

Updated ODataUriParser to allow select and expand property names that… #2437

Conversation

corranrogue9 commented Jun 20, 2022

Issues

Description

Checklist (Uncheck if it is not completed)

Additional work necessary

pull-request-quantifier-deprecated bot commented Jun 20, 2022

What can I do to optimize my changes

How to interpret the change counts in git diff output

Choose a reason for hiding this comment

corranrogue9 Jun 22, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

habbes left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

xuzhg commented Jul 25, 2022

corranrogue9 Jun 22, 2022 •

edited

Loading