-
Notifications
You must be signed in to change notification settings - Fork 357
Default EnableCaseInsensitive to true when initializing a new instance of ODataUriResolver #3265
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
Default EnableCaseInsensitive to true when initializing a new instance of ODataUriResolver #3265
Conversation
…e of ODataUriResolver
f31e27d
to
7ff15ce
Compare
/// <remarks> | ||
/// All extensions should look at this property and keep case sensitive behavior consistent. | ||
/// </remarks> | ||
public virtual bool EnableCaseInsensitive { get; set; } |
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.
From your notes in the PR, it says that the case insensitive for the query options key name no matter whether it prefixes with '$', Is it right?
Here, if we default to 'true', it means all the Uri parts are case insensitive by default?
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.
@xuzhg That statement is directly from the standard: OData V4.01 states that services MUST support case-insensitive system query option names specified with or without the $ prefix.
With this change, the URI parts are case-insensitive by default - which is what the UnqualifiedODataUriResolver
does in ASP.NET Core OData here:
https://github.com/OData/AspNetCoreOData/blob/1368836752963d88e920d9508d94807eb995894b/src/Microsoft.AspNetCore.OData/ODataOptions.cs#L326
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.
Hi @gathogojr,
Could you clarify why this MR changes the handling of property names? The standard specifies that query parameter names (like $expand) should be case-insensitive, but not their values. For example, $exand and $EXPand should be treated as equivalent, but $expand=shoe and $expand=Shoe should not.
This has been a pain point for me regarding case insensitivity—especially since C# class properties are also resolved in a case-insensitive manner, though I haven’t found a clear justification for it. (Related PR where I struggled to write tests)
Could you explain why property names should be handled case-insensitively?
/// All extensions should look at this property and keep case sensitive behavior consistent. | ||
/// </remarks> | ||
public virtual bool EnableCaseInsensitive { get; set; } | ||
public virtual bool EnableCaseInsensitive { get; set; } = true; |
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.
I thought this was already true
by default.
Could you add a test that makes this to false.
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.
@WanjohiSammy The default was false
, as you can see from the following tests:
For the UnqualifiedODataUriResolver
in ASP.NET Core OData here
https://github.com/OData/AspNetCoreOData/blob/1368836752963d88e920d9508d94807eb995894b/src/Microsoft.AspNetCore.OData/ODataOptions.cs#L326, we set it to true
to support case insensitive behaviour when resolving OData URIs.
Regarding your ask, do you need me to add a test like the following:
[Fact]
public void TestEnableCaseInsensitiveBuiltinIdentifierSetToFalse()
{
Assert.False((new ODataUriResolver { EnableCaseInsensitive = false }).EnableCaseInsensitive);
}
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.
Something like this will suffice
Issues
This pull request fixes #3066.
Description
This pull request addresses #3066 by aligning the default behavior of
ODataUriResolver
with the OData V4.01 specification.According to the OData V4.01 specification, services MUST support case-insensitive system query option names, whether or not they are prefixed with
$
.ASP.NET Core OData already complies with this requirement by injecting a case-insensitive
UnqualifiedODataUriResolver
(derived fromODataUriResolver
) by default:https://github.com/OData/AspNetCoreOData/blob/1368836752963d88e920d9508d94807eb995894b/src/Microsoft.AspNetCore.OData/ODataOptions.cs#L326-L329
To ensure consistent behavior across the OData ecosystem, this PR proposes setting
EnableCaseInsensitive = true
by default when initializing a new instance ofODataUriResolver
in ODL:odata.net/src/Microsoft.OData.Core/UriParser/Resolver/ODataUriResolver.cs
Line 24 in 95233fc
With this change, both of the following requests will be parsed successfully and equivalently:
This change improves spec compliance and developer experience by reducing case-sensitivity issues in query options.
Checklist (Uncheck if it is not completed)
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.
Repository notes
Team members can start a CI build by adding a comment with the text
/AzurePipelines run
to a PR. A bot may respond indicating that there is no pipeline associated with the pull request. This can be ignored if the build is triggered.Team members should not trigger a build this way for pull requests coming from forked repositories. They should instead trigger the build manually by setting the "branch" to
refs/pull/{prId}/merge
where{prId}
is the ID of the PR.