-
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -35,7 +35,7 @@ public class ODataUriResolver | |
/// <remarks> | ||
/// 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 commentThe reason will be displayed to describe this comment to others. Learn more. I thought this was already Could you add a test that makes this to false. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @WanjohiSammy The default was For the 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 commentThe reason will be displayed to describe this comment to others. Learn more. Something like this will suffice |
||
|
||
/// <summary> | ||
/// Gets and sets the optional-$-sign-prefix for OData system query option. | ||
|
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?