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

Fix Prefix Matching Logic in PrefixContainer #58068

Open
wants to merge 4 commits into
base: main
Choose a base branch
from
Open

Fix Prefix Matching Logic in PrefixContainer #58068

wants to merge 4 commits into from
+48 −23

Conversation

sami-daniel
Copy link

Fix Prefix Matching Logic in PrefixContainer

Description

This PR modifies the BinarySearch method in PrefixContainer to ensure that an exact match of a prefix does not incorrectly qualify as a valid prefix.

Now the ContainsPrefix method no longer gives a false positive if it has a prefix with the same name as the Action method parameter.

Fixes #57637

sami-daniel and others added 3 commits September 25, 2024 05:00
@sami-daniel
Refactor PrefixContainerTest to include a new test case for checking …
3da6a69 
…if the prefix matches exactly with keys
Fix BinarySearch logic
34a42e4 
Updated the BinarySearch method in PrefixContainer to ensure that an exact match of a prefix does not incorrectly qualify as a valid prefix. Enhanced comments to clarify the flow and decision-making process when checking for delimiters. This change improves the accuracy of prefix matching for hierarchical key structures.
Fix the name of the test that checks the Return False of ContaisPrefi…
324652b 
…x if the Prefix is ​​equal to the keys
@sami-daniel sami-daniel requested a review from a team as a code owner September 25, 2024 09:58
@dotnet-issue-labeler dotnet-issue-labeler bot added the area-mvc Includes: MVC, Actions and Controllers, Localization, CORS, most templates label Sep 25, 2024
@dotnet-policy-service dotnet-policy-service bot added the community-contribution Indicates that the PR has been added by a community member label Sep 25, 2024
@sami-daniel
Copy link
Author

@sami-daniel please read the following Contributor License Agreement(CLA). If you agree with the CLA, please reply with the following information.

@dotnet-policy-service agree [company="{your company}"]

Options:

  • (default - no company specified) I have sole ownership of intellectual property rights to my Submissions and I am not making Submissions in the course of work for my employer.
@dotnet-policy-service agree
  • (when company given) I am making Submissions in the course of work for my employer (or my employer has intellectual property rights in my Submissions by contract or applicable law). I have permission from my employer to make Submissions and enter into this Agreement on behalf of my employer. By signing below, the defined term “You” includes me and my employer.
@dotnet-policy-service agree company="Microsoft"

Contributor License Agreement

@dotnet-policy-service agree

gfoidl
gfoidl reviewed Sep 25, 2024
View reviewed changes
Copy link
Member

@gfoidl gfoidl left a comment

Choose a reason for hiding this comment

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

A small JIT-trick for better machine code.

src/Mvc/Mvc.Core/src/ModelBinding/PrefixContainer.cs Outdated Show resolved Hide resolved
@sami-daniel @gfoidl
Optimize Boundary Check in BinarySearch Method
eb68105 
This commit enhances the performance of the BinarySearch method by eliminating unnecessary boundary checks. The condition if ((uint)candidate.Length > (uint)prefix.Length) ensures that the index access candidate[prefix.Length] is safe, and the JIT compiler can now safely elide the boundary check, improving execution efficiency.

Co-authored-by: Günther Foidl <gue@korporal.at>
@sami-daniel
Copy link
Author

A small JIT-trick for better machine code.

I didn't know that, but it makes sense, thanks!

@gfoidl
Copy link
Member

gfoidl commented Sep 26, 2024

I didn't know that

The nice thing on contributing: you help to improve the product and get the opportunity to learn something new. A win - win situation 😃.

Thanks for the PR 👍🏻

@sami-daniel
Copy link
Author

I didn't know that

The nice thing on contributing: you help to improve the product and get the opportunity to learn something new. A win - win situation 😃.

Thanks for the PR 👍🏻

Yessss, C# and .NET have been my focus of study since the beginning of my interest in technology and until now I don't even know half of its power. This is my first contribution and I already loved it. Thanks!

@dotnet-policy-service dotnet-policy-service bot added the pending-ci-rerun When assigned to a PR indicates that the CI checks should be rerun label Oct 3, 2024
@sami-daniel sami-daniel closed this Oct 3, 2024
@sami-daniel sami-daniel reopened this Oct 3, 2024
@dotnet-policy-service dotnet-policy-service bot added this to the 10.0-preview1 milestone Oct 3, 2024
@adityamandaleeka
Copy link
Member

Yessss, C# and .NET have been my focus of study since the beginning of my interest in technology and until now I don't even know half of its power. This is my first contribution and I already loved it. Thanks!

Love it, welcome @sami-daniel!

@adityamandaleeka
Copy link
Member

/azp run

@dotnet-policy-service dotnet-policy-service bot removed the pending-ci-rerun When assigned to a PR indicates that the CI checks should be rerun label Oct 4, 2024
@azure-pipelines Azure Pipelines
Copy link

Azure Pipelines successfully started running 3 pipeline(s).

@adityamandaleeka
Copy link
Member

Ubuntu failure is unrelated; it failed during node installation. cc: @dotnet/aspnet-build

##[error]Request timeout: /dist/index.json

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@gfoidl gfoidl gfoidl left review comments

At least 1 approving review is required to merge this pull request.

Assignees

@adityamandaleeka adityamandaleeka

Labels
area-mvc Includes: MVC, Actions and Controllers, Localization, CORS, most templates community-contribution Indicates that the PR has been added by a community member
Projects
None yet
Milestone
10.0-preview1
Development

Successfully merging this pull request may close these issues.

HttpGet [FromQuery] Model binding fails in case of propertity with the same name
3 participants
@sami-daniel @gfoidl @adityamandaleeka