Fix Prefix Matching Logic in PrefixContainer #58068

sami-daniel · 2024-09-25T09:58:41Z

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

…if the prefix matches exactly with keys

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.

…x if the Prefix is equal to the keys

sami-daniel · 2024-09-25T10:05:22Z

@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

A small JIT-trick for better machine code.

src/Mvc/Mvc.Core/src/ModelBinding/PrefixContainer.cs

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 <[email protected]>

sami-daniel · 2024-09-25T23:31:08Z

A small JIT-trick for better machine code.

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

gfoidl · 2024-09-26T11:46:52Z

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 · 2024-09-26T12:26:43Z

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!

adityamandaleeka · 2024-10-04T23:39:52Z

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 · 2024-10-04T23:40:17Z

/azp run

azure-pipelines · 2024-10-04T23:40:37Z

Azure Pipelines successfully started running 3 pipeline(s).

adityamandaleeka · 2024-10-05T00:46:45Z

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

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

FatTigerWang · 2024-10-10T05:20:02Z

/azp run

azure-pipelines · 2024-10-10T05:20:08Z

Commenter does not have sufficient privileges for PR 58068 in repo dotnet/aspnetcore

wtgodbe · 2024-10-10T16:45:52Z

/azp run

azure-pipelines · 2024-10-10T16:46:18Z

Azure Pipelines successfully started running 3 pipeline(s).

adityamandaleeka · 2024-10-18T01:13:47Z

Not sure if there's something making our CI more flaky than normal...

/azp run

sami-daniel · 2024-12-03T22:12:00Z

Hello again :).
Would it be possible to run the pipelines gain to see if the behavior is the same? I will make some changes if necessary

wtgodbe · 2024-12-03T22:23:25Z

/azp run

azure-pipelines · 2024-12-03T22:23:44Z

Azure Pipelines successfully started running 3 pipeline(s).

captainsafia · 2024-12-16T22:26:28Z

/azp run

azure-pipelines · 2024-12-16T22:26:49Z

Azure Pipelines successfully started running 3 pipeline(s).

captainsafia

Thanks for opening this PR!

It looks like the test failures that were introduced here are legitimate failures. Specifically, the failures in Mvc.FunctionalTests can be debugged to investigate what regression was introduced here.

Copilot

Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.

dotnet-policy-service · 2024-12-26T17:51:07Z

Looks like this PR hasn't been active for some time and the codebase could have been changed in the meantime.
To make sure no conflicting changes have occurred, please rerun validation before merging. You can do this by leaving an /azp run comment here (requires commit rights), or by simply closing and reopening.

dotnet-policy-service · 2025-01-05T19:23:30Z

Hi @sami-daniel.
It seems you haven't touched this PR for the last two weeks. To avoid accumulating old PRs, we're marking it as stale. As a result, it will be closed if no further activity occurs within 4 days of this comment. You can learn more about our Issue Management Policies here.

sami-daniel · 2025-06-25T04:39:19Z

Moved to #62459 because I lost the branch where this PR was attached.

Sami Daniel Santos Silva and others added 3 commits September 25, 2024 05:00

Refactor PrefixContainerTest to include a new test case for checking …

3da6a69

…if the prefix matches exactly with keys

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 requested a review from a team as a code owner September 25, 2024 09:58

ghost added the area-mvc Includes: MVC, Actions and Controllers, Localization, CORS, most templates label Sep 25, 2024

dotnet-policy-service bot added the community-contribution Indicates that the PR has been added by a community member label Sep 25, 2024

mkArtakMSFT assigned adityamandaleeka Sep 25, 2024

gfoidl reviewed Sep 25, 2024

View reviewed changes

src/Mvc/Mvc.Core/src/ModelBinding/PrefixContainer.cs Outdated Show resolved Hide resolved

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 closed this Oct 3, 2024

sami-daniel reopened this Oct 3, 2024

dotnet-policy-service bot added this to the 10.0-preview1 milestone Oct 3, 2024

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

dotnet-policy-service bot added the pending-ci-rerun When assigned to a PR indicates that the CI checks should be rerun label Oct 17, 2024

sami-daniel mentioned this pull request Dec 3, 2024

HttpGet [FromQuery] Model binding fails in case of propertity with the same name #57637

Open

1 task

Merge branch 'dotnet:main' into main

b021b26

captainsafia removed the pending-ci-rerun When assigned to a PR indicates that the CI checks should be rerun label Dec 16, 2024

captainsafia reviewed Dec 17, 2024

View reviewed changes

captainsafia requested a review from Copilot December 17, 2024 21:28

Copilot AI reviewed Dec 17, 2024

View reviewed changes

mkArtakMSFT added the pr: pending author input For automation. Specifically separate from Needs: Author Feedback label Dec 19, 2024

dotnet-policy-service bot added the pending-ci-rerun When assigned to a PR indicates that the CI checks should be rerun label Dec 26, 2024

dotnet-policy-service bot added the stale Indicates a stale issue. These issues will be closed automatically soon. label Jan 5, 2025

dotnet-policy-service bot removed this from the 10.0-preview1 milestone Jan 9, 2025

dotnet-policy-service bot closed this Jan 9, 2025

sami-daniel mentioned this pull request Jun 25, 2025

Fix Create Property Model Name for Complex Objects #62459

Open

Fix Prefix Matching Logic in PrefixContainer #58068

Fix Prefix Matching Logic in PrefixContainer #58068

Uh oh!

Conversation

sami-daniel commented Sep 25, 2024

Fix Prefix Matching Logic in PrefixContainer

Description

Uh oh!

sami-daniel commented Sep 25, 2024

Uh oh!

gfoidl left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

sami-daniel commented Sep 25, 2024

Uh oh!

gfoidl commented Sep 26, 2024

Uh oh!

sami-daniel commented Sep 26, 2024

Uh oh!

adityamandaleeka commented Oct 4, 2024

Uh oh!

adityamandaleeka commented Oct 4, 2024

Uh oh!

azure-pipelines bot commented Oct 4, 2024

Uh oh!

adityamandaleeka commented Oct 5, 2024

Uh oh!

FatTigerWang commented Oct 10, 2024

Uh oh!

azure-pipelines bot commented Oct 10, 2024

Uh oh!

wtgodbe commented Oct 10, 2024

Uh oh!

azure-pipelines bot commented Oct 10, 2024

Uh oh!

adityamandaleeka commented Oct 18, 2024

Uh oh!

sami-daniel commented Dec 3, 2024

Uh oh!

wtgodbe commented Dec 3, 2024

Uh oh!

azure-pipelines bot commented Dec 3, 2024

Uh oh!

captainsafia commented Dec 16, 2024

Uh oh!

azure-pipelines bot commented Dec 16, 2024

Uh oh!

captainsafia left a comment

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Uh oh!

dotnet-policy-service bot commented Dec 26, 2024

Uh oh!

dotnet-policy-service bot commented Jan 5, 2025

Uh oh!

sami-daniel commented Jun 25, 2025

Uh oh!

Uh oh!