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.

Sign up for GitHub

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

Jump to bottom

Do not fail if parenthesis are not in expected state #2828

Open
wants to merge 3 commits into
base: release-7.x
Choose a base branch
from
Open

Do not fail if parenthesis are not in expected state #2828

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
3a84b10
Do not fail if parenthesis are not in expected state
ificator Dec 11, 2023
e234c2b
Keep old behavior when KeyAsSegment is not enabled
ificator Dec 17, 2023
86aa997
Still block empty identifier for non-key-as-segment scenarios.
ificator Dec 17, 2023
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
84 changes: 59 additions & 25 deletions src/Microsoft.OData.Core/UriParser/Parsers/ODataPathParser.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -74,36 +74,61 @@ internal ODataPathParser(ODataUriParserConfiguration configuration)
/// anything and simply provides the raw text of both the identifier and parenthetical expression.
/// </summary>
/// <remarks>Internal only so it can be called from tests. Should not be used outside <see cref="ODataPathParser"/>.</remarks>
/// <param name="enableKeyAsSegment">Whether key-as-segment is enabled.</param>
/// <param name="segmentText">The segment text.</param>
/// <param name="identifier">The identifier that was found.</param>
/// <param name="parenthesisExpression">The query portion that was found. Will be null after the call if no query portion was present.</param>
internal static void ExtractSegmentIdentifierAndParenthesisExpression(string segmentText, out string identifier, out string parenthesisExpression)
internal static void ExtractSegmentIdentifierAndParenthesisExpression(
bool enableKeyAsSegment,
string segmentText,
out string identifier,
out string parenthesisExpression)
{
Debug.Assert(segmentText != null, "segment != null");

// We allow a single trailing '/', which results in an empty segment.
// However System.Uri removes it, so any empty segment we see is a 404 error.
if (segmentText.Length == 0)
{
throw ExceptionUtil.ResourceNotFoundError(ODataErrorStrings.RequestUriProcessor_EmptySegmentInRequestUrl);
}

int parenthesisStart = segmentText.IndexOf('(');
if (parenthesisStart < 0)

bool containsOpenParenthesis = parenthesisStart >= 0;
bool endsWithCloseParenthesis = segmentText[segmentText.Length - 1] == ')';

if (containsOpenParenthesis != endsWithCloseParenthesis && !enableKeyAsSegment)
{
identifier = segmentText;
parenthesisExpression = null;
// If the parenthesis are not in the expected state fail with a syntax error. We do not do this for key-as-segment
// because in that case the segment text is potentially still a valid key value.
throw ExceptionUtil.CreateSyntaxError();
Comment on lines +101 to +105
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this condition consistent with existing behaviour?

In the current behaviour, if the segment does not contain and open parenthesis but ends with closing parenthesis, e.g. folder), the condition would pass and the identifier would be set to the segmentText.

However, in your change, if the segment does not contain an open parenthesis but ends with closing parenthesis, the condition would fail when enableKeyAsSegment is false and a segment like folder) would throw an error.

}
else
else if (containsOpenParenthesis && endsWithCloseParenthesis)
{
if (segmentText[segmentText.Length - 1] != ')')
{
throw ExceptionUtil.CreateSyntaxError();
}

// split the string to grab the identifier and remove the parentheses
// Split the string to grab the identifier and remove the parentheses.
identifier = segmentText.Substring(0, parenthesisStart);
parenthesisExpression = segmentText.Substring(parenthesisStart + 1, segmentText.Length - identifier.Length - 2);
}
else
{
identifier = segmentText;
parenthesisExpression = null;
}

// We allow a single trailing '/', which results in an empty segment.
// However System.Uri removes it, so any empty segment we see is a 404 error.
if (identifier.Length == 0)
{
throw ExceptionUtil.ResourceNotFoundError(ODataErrorStrings.RequestUriProcessor_EmptySegmentInRequestUrl);
if (enableKeyAsSegment)
{
// We already know that the segment itself is non-zero and so if the identifier is empty and we've enabled
// key-as-segment we'll treat the entire segment as the identifier.
identifier = segmentText;
parenthesisExpression = null;
}
else
{
throw ExceptionUtil.ResourceNotFoundError(ODataErrorStrings.RequestUriProcessor_EmptySegmentInRequestUrl);
}
}
}

Expand Down Expand Up @@ -835,9 +860,12 @@ private void CreateFirstSegment(string segmentText)
return;
}

string identifier;
string parenthesisExpression;
ExtractSegmentIdentifierAndParenthesisExpression(segmentText, out identifier, out parenthesisExpression);
bool enableKeyAsSegment = this.configuration.UrlKeyDelimiter.EnableKeyAsSegment;
ODataPathParser.ExtractSegmentIdentifierAndParenthesisExpression(
enableKeyAsSegment,
segmentText,
out string identifier,
out string parenthesisExpression);
Debug.Assert(identifier != null, "identifier != null");

// Look for well-known system resource points.
Expand Down Expand Up @@ -923,9 +951,12 @@ private void CreateFirstSegment(string segmentText)
/// <returns>boolean value.</returns>
private bool BindSegmentBeforeEscapeFunction(string segmentText)
{
string identifier;
string parenthesisExpression;
ExtractSegmentIdentifierAndParenthesisExpression(segmentText, out identifier, out parenthesisExpression);
bool enableKeyAsSegment = this.configuration.UrlKeyDelimiter.EnableKeyAsSegment;
ODataPathParser.ExtractSegmentIdentifierAndParenthesisExpression(
enableKeyAsSegment,
segmentText,
out string identifier,
out string parenthesisExpression);

if (this.parsedSegments.Count == 0)
{
Expand Down Expand Up @@ -966,7 +997,7 @@ private bool BindSegmentBeforeEscapeFunction(string segmentText)
}

// For KeyAsSegment, try to handle as key segment
if (this.configuration.UrlKeyDelimiter.EnableKeyAsSegment && this.TryHandleAsKeySegment(segmentText))
if (enableKeyAsSegment && this.TryHandleAsKeySegment(segmentText))
{
return true;
}
Expand Down Expand Up @@ -1178,9 +1209,12 @@ private void CreateNextSegment(string text)
return;
}

string identifier;
string parenthesisExpression;
ExtractSegmentIdentifierAndParenthesisExpression(text, out identifier, out parenthesisExpression);
bool enableKeyAsSegment = this.configuration.UrlKeyDelimiter.EnableKeyAsSegment;
ODataPathParser.ExtractSegmentIdentifierAndParenthesisExpression(
enableKeyAsSegment,
text,
out string identifier,
out string parenthesisExpression);
/*
* For Non-KeyAsSegment, try to handle it as a key property value, unless it was preceded by an escape - marker segment('$').
* For KeyAsSegment, the following precedence rules should be supported[ODATA - 799]:
Expand Down Expand Up @@ -1247,7 +1281,7 @@ private void CreateNextSegment(string text)
}

// For KeyAsSegment, try to handle as key segment
if (this.configuration.UrlKeyDelimiter.EnableKeyAsSegment && this.TryHandleAsKeySegment(text))
if (enableKeyAsSegment && this.TryHandleAsKeySegment(text))
{
return;
}
Expand Down
4 changes: 2 additions & 2 deletions ...Tests/Microsoft.OData.Core.Tests/ScenarioTests/UriParser/ParameterAliasFunctionalTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -419,11 +419,11 @@ public void ParseFilter_AliasInsideInOperator_OperandsAsExpressions()
public void ParseFilter_AliasInFilterPathSegment_AliasAsFirstSegment()
{
Action parse = () => ParseUriAndVerify(
new Uri("http://gobbledygook/$filter(@p1)&@p1=true"),
new Uri("http://gobbledygook/$filter(@p1)?@p1=true"),
(oDataPath, filterClause, orderByClause, selectExpandClause, aliasNodes) =>
{
});
parse.Throws<ODataException>(Strings.RequestUriProcessor_SyntaxError);
parse.Throws<ODataUnrecognizedPathException>(Strings.RequestUriProcessor_FilterOnRoot);
}

[Fact]
Expand Down
2 changes: 1 addition & 1 deletion test/FunctionalTests/Microsoft.OData.Core.Tests/UriParser/ODataUriParserTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1359,7 +1359,7 @@ public void ParseEscapeFunctionWithInheritance(string escapeFunctionUri, string
[InlineData("/entitySetEscaped::/32:/NonComposableParameter", typeof(ODataUnrecognizedPathException))]
[InlineData("/entitySetEscaped('32')/NS.SpecialDrive::/ComposableParameter::/nestedNonComposableParameter:", typeof(ODataUnrecognizedPathException))]
[InlineData("/entitySetEscaped(32)/:NS.SpecialDrive:/ComposableParameter::/nestedNonComposableParameter:", typeof(ODataException))]
[InlineData("/entitySetEscaped('32')::/NS.SpecialDrive:/ComposableParameter::/nestedNonComposableParameter:", typeof(ODataException))]
[InlineData("/entitySetEscaped('32')::/NS.SpecialDrive:/ComposableParameter::/nestedNonComposableParameter:", typeof(ODataUnrecognizedPathException))]
public void ParseInvalidEscapeURIShouldThrow(string escapeFunctionUri, Type exceptionType)
{
// Arrange
Expand Down
76 changes: 61 additions & 15 deletions test/FunctionalTests/Microsoft.OData.Core.Tests/UriParser/Parsers/ODataPathParserTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,30 +36,72 @@ public void CallbackReturnsBatchReferenceSegment()

[Fact]
public void RequestUriProcessorExtractSegmentIdentifierTest()
{
ExtractSegmentIdentifierAndParenthesisExpression("blah", "blah", null);
ExtractSegmentIdentifierAndParenthesisExpression("multiple words", "multiple words", null);
ExtractSegmentIdentifierAndParenthesisExpression("multiple(123)", "multiple", "123");
ExtractSegmentIdentifierAndParenthesisExpression("multiple(123;321)", "multiple", "123;321");
ExtractSegmentIdentifierAndParenthesisExpression("set()", "set", string.Empty);
}

[Fact]
public void RequestUriProcessorExtractSegmentIdentifierErrorTest()
{
string actualIdentifier;
string queryPortion;

Action emptyString = () => ODataPathParser.ExtractSegmentIdentifierAndParenthesisExpression(string.Empty, out actualIdentifier, out queryPortion);
// Success cases
ExtractSegmentIdentifierAndParenthesisExpression(enableKeyAsSegment: false, "blah", "blah", null);
ExtractSegmentIdentifierAndParenthesisExpression(enableKeyAsSegment: false, "multiple words", "multiple words", null);
ExtractSegmentIdentifierAndParenthesisExpression(enableKeyAsSegment: false, "multiple(123)", "multiple", "123");
ExtractSegmentIdentifierAndParenthesisExpression(enableKeyAsSegment: false, "multiple(123;321)", "multiple", "123;321");
ExtractSegmentIdentifierAndParenthesisExpression(enableKeyAsSegment: false, "set()", "set", string.Empty);
Copy link
Contributor

Choose a reason for hiding this comment

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

Could you add a test case for a scenario that does not contain an ( but ends with a ), like blah), I expect the result would be blah) for the identifier and null for the parenthesis expression.


// Failure cases
Action emptyString =
() =>
ODataPathParser.ExtractSegmentIdentifierAndParenthesisExpression(
enableKeyAsSegment: false,
string.Empty,
out actualIdentifier,
out queryPortion);
emptyString.Throws<ODataUnrecognizedPathException>(ErrorStrings.RequestUriProcessor_EmptySegmentInRequestUrl);

Action noIdentifier = () => ODataPathParser.ExtractSegmentIdentifierAndParenthesisExpression("()", out actualIdentifier, out queryPortion);
Action noIdentifier =
() =>
ODataPathParser.ExtractSegmentIdentifierAndParenthesisExpression(
enableKeyAsSegment: false,
"()",
out actualIdentifier,
out queryPortion);
noIdentifier.Throws<ODataUnrecognizedPathException>(ErrorStrings.RequestUriProcessor_EmptySegmentInRequestUrl);

Action noEndParen = () => ODataPathParser.ExtractSegmentIdentifierAndParenthesisExpression("foo(", out actualIdentifier, out queryPortion);
Action noEndParen =
() =>
ODataPathParser.ExtractSegmentIdentifierAndParenthesisExpression(
enableKeyAsSegment: false,
"foo(",
out actualIdentifier,
out queryPortion);
noEndParen.Throws<ODataException>(ErrorStrings.RequestUriProcessor_SyntaxError);
}

[Fact]
public void RequestUriProcessorExtractSegmentIdentifierTest_KeyAsSegment()
{
string actualIdentifier;
string queryPortion;

// Success cases
ExtractSegmentIdentifierAndParenthesisExpression(enableKeyAsSegment: true, "blah", "blah", null);
ExtractSegmentIdentifierAndParenthesisExpression(enableKeyAsSegment: true, "multiple words", "multiple words", null);
ExtractSegmentIdentifierAndParenthesisExpression(enableKeyAsSegment: true, "multiple(123)", "multiple", "123");
ExtractSegmentIdentifierAndParenthesisExpression(enableKeyAsSegment: true, "multiple(123;321)", "multiple", "123;321");
ExtractSegmentIdentifierAndParenthesisExpression(enableKeyAsSegment: true, "set()", "set", string.Empty);
ExtractSegmentIdentifierAndParenthesisExpression(enableKeyAsSegment: true, "()", "()", null);
ExtractSegmentIdentifierAndParenthesisExpression(enableKeyAsSegment: true, "foo(", "foo(", null);
Copy link
Contributor

Choose a reason for hiding this comment

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

Could you add a test case for a scenario that does not contain an ( but ends with a ), like blah).

Also, is it a good idea to support different syntaxes depending on whether key as segment is enabled?


// Failure cases
Action emptyString =
() =>
ODataPathParser.ExtractSegmentIdentifierAndParenthesisExpression(
enableKeyAsSegment: false,
string.Empty,
out actualIdentifier,
out queryPortion);
emptyString.Throws<ODataUnrecognizedPathException>(ErrorStrings.RequestUriProcessor_EmptySegmentInRequestUrl);
}

#region $ref cases
[Fact]
public void EntityReferenceFollowingEntityCollectionShouldWork()
Expand Down Expand Up @@ -811,11 +853,15 @@ public void ParseBoundFunctionWithTypeDefinitionAsParameterAndReturnTypeShouldWo
Assert.Equal("abc", node.Value);
}

private static void ExtractSegmentIdentifierAndParenthesisExpression(string segment, string expectedIdentifier, string expectedQueryPortion)
private static void ExtractSegmentIdentifierAndParenthesisExpression(
bool enableKeyAsSegment,
string segment,
string expectedIdentifier,
string expectedQueryPortion)
{
string actualIdentifier;
string queryPortion;
ODataPathParser.ExtractSegmentIdentifierAndParenthesisExpression(segment, out actualIdentifier, out queryPortion);
ODataPathParser.ExtractSegmentIdentifierAndParenthesisExpression(enableKeyAsSegment, segment, out actualIdentifier, out queryPortion);
Assert.Equal(expectedIdentifier, actualIdentifier);
Assert.Equal(expectedQueryPortion, queryPortion);
}
Expand Down
8 changes: 4 additions & 4 deletions test/FunctionalTests/Microsoft.OData.Core.Tests/UriParser/Parsers/UriTemplateParserTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -377,7 +377,7 @@ public void ErrorParameterTemplateInputShouldThrow()
new {Input = "}{" , Error = Strings.ExpressionLexer_InvalidCharacter("}", 6, "onCat=}{")},
new {Input = "{{}" , Error = Strings.ExpressionLexer_UnbalancedBracketExpression}, // Thrown by ODataPathParser::TryBindingParametersAndMatchingOperation
new {Input = "{}}" , Error = Strings.ExpressionLexer_InvalidCharacter("}", 8, "onCat={}}")},
new {Input = "{#}" , Error = Strings.RequestUriProcessor_SyntaxError},
new {Input = "{#}" , Error = Strings.RequestUriProcessor_ResourceNotFound("Fully.Qualified.Namespace.HasHat(onCat={")},
};

foreach (var errorCase in errorCases)
Expand All @@ -388,7 +388,7 @@ public void ErrorParameterTemplateInputShouldThrow()
};

Action action = () => uriParser.ParsePath();
action.Throws<ODataException>(errorCase.Error);
action.ThrowsAny<ODataException>(errorCase.Error);
Copy link
Contributor

Choose a reason for hiding this comment

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

What's the difference between Throws and ThrowsAny?

}
}

Expand All @@ -403,7 +403,7 @@ public void ErrorKeyTemplateInputShouldThrow()
new {Input = "}{" , Error = Strings.ExpressionLexer_InvalidCharacter("}", 1, "(}{)")},
new {Input = "{{}" , Error = Strings.ExpressionLexer_UnbalancedBracketExpression}, // Thrown by ODataPathParser::TryBindKeyFromParentheses
new {Input = "{}}" , Error = Strings.ExpressionLexer_InvalidCharacter("}", 3, "({}})")},
new {Input = "{#}" , Error = Strings.RequestUriProcessor_SyntaxError},
new {Input = "{#}" , Error = Strings.RequestUriProcessor_ResourceNotFound("People({")},
};

foreach (var errorCase in errorCases)
Expand All @@ -414,7 +414,7 @@ public void ErrorKeyTemplateInputShouldThrow()
};

Action action = () => uriParser.ParsePath();
action.Throws<ODataException>(errorCase.Error);
action.ThrowsAny<ODataException>(errorCase.Error);
}
}
#endregion
Expand Down