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

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

Open
wants to merge 1 commit into
base: release-7.x
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
21 changes: 20 additions & 1 deletion src/Microsoft.OData.Core/UriParser/ExpressionLexer.cs
Original file line number Diff line number Diff line change
Expand Up @@ -83,6 +83,11 @@ internal class ExpressionLexer
/// <summary>Whether the lexer is being used to parse function parameters. If true, will allow/recognize parameter aliases and typed nulls.</summary>
private readonly bool parsingFunctionParameters;

/// <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;
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

how can customer config this?


/// <summary>Lexer ignores whitespace</summary>
private bool ignoreWhitespace;

Expand All @@ -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)
Copy link
Member

Choose a reason for hiding this comment

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

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?

Copy link
Contributor

Choose a reason for hiding this comment

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

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

: this(expression, moveToFirstToken, useSemicolonDelimiter, parsingFunctionParameters, true)
Copy link
Contributor

Choose a reason for hiding this comment

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

For better legibility:

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

{
}

/// <summary>Initializes a new <see cref="ExpressionLexer"/>.</summary>
/// <param name="expression">Expression to parse.</param>
/// <param name="moveToFirstToken">If true, this constructor will call NextToken() to move to the first token.</param>
/// <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>
/// <param name="applySemanticsToIdentifiers">
/// 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)
/// </param>
internal ExpressionLexer(string expression, bool moveToFirstToken, bool useSemicolonDelimiter, bool parsingFunctionParameters, bool applySemanticsToIdentifiers)
{
Debug.Assert(expression != null, "expression != null");

Expand All @@ -116,6 +134,7 @@ internal ExpressionLexer(string expression, bool moveToFirstToken, bool useSemic
this.TextLen = this.Text.Length;
this.useSemicolonDelimiter = useSemicolonDelimiter;
this.parsingFunctionParameters = parsingFunctionParameters;
this.applySemanticsToIdentifiers = applySemanticsToIdentifiers;
this.parsingDoubleQuotedString = false;

this.SetTextPos(0);
Expand Down Expand Up @@ -810,7 +829,7 @@ private void HandleTypePrefixedLiterals()

this.HandleQuotedValues();
}
else
else if (this.applySemanticsToIdentifiers)
{
// Handle keywords.
// Get built in type literal prefix.
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -81,7 +81,7 @@ public 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;
Copy link
Member

Choose a reason for hiding this comment

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

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?

Copy link
Contributor Author

@corranrogue9 corranrogue9 Jun 22, 2022

Choose a reason for hiding this comment

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

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).

Copy link
Contributor

Choose a reason for hiding this comment

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

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;


this.enableCaseInsensitiveBuiltinIdentifier = enableCaseInsensitiveBuiltinIdentifier;

Expand Down
Loading