-
Notifications
You must be signed in to change notification settings - Fork 349
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
Do not fail if parenthesis are not in expected state #2828
base: release-7.x
Are you sure you want to change the base?
Conversation
This PR has Quantification details
Why proper sizing of changes matters
Optimal pull request sizes drive a better predictable PR flow as they strike a
What can I do to optimize my changes
How to interpret the change counts in git diff output
Was this comment helpful? 👍 :ok_hand: :thumbsdown: (Email) |
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(); |
There was a problem hiding this comment.
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.
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); |
There was a problem hiding this comment.
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.
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); |
There was a problem hiding this comment.
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?
@@ -388,7 +388,7 @@ public void ErrorParameterTemplateInputShouldThrow() | |||
}; | |||
|
|||
Action action = () => uriParser.ParsePath(); | |||
action.Throws<ODataException>(errorCase.Error); | |||
action.ThrowsAny<ODataException>(errorCase.Error); |
There was a problem hiding this comment.
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
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Confirm whether this change throws an exception when a segment that does not contain an opening parenthesis ends with a closing parenthesis, and whether that would be a regression.
Issues
This pull request fixes #2827
Description
Modify
ExtractSegmentIdentifierAndParenthesisExpression
so that it doesn't throw when the parenthesis are in an unexpected state for key-as-parameter.Checklist (Uncheck if it is not completed)