Skip to content

Conversation

@martincostello
Copy link
Contributor

Fix issue where request header matching was based on the order of registrations, rather than the number of headers, which could cause a response intended for a less-specific HTTP request to be returned for a more-specific HTTP registration due to the first match winning. The ordering was also non-deterministic as the LINQ sort on the object's identity (hash code) isn't stable if all the priorities are the same.

For example, if you had an HTTP request for an unauthenticated resource with an Accept header that should return 401 and then another that returns 200 when Authorization is added, the 401 response could sometimes be selected if it appeared first in the enumerated candidates because the 401 request contains a subset of the required headers (i.e. Accept with the expected value), so it still matched.

Fixed by sorting the matches to enumerate from most-to-least count of request headers so more-specific registrations are tried before less-specific ones.

The issue can be worked around by using priorities, but this change makes the default behaviour more intuitive.

Also migrates from .sln to .slnx.

Migrate from `.sln` to `.slnx`.
Fix issue where header matching was based on the order of registrations, rather than the number of headers, which could cause a response intended for a less-specific HTTP request to be returned for a more-specific HTTP registration due to the first match winning.
Copilot AI review requested due to automatic review settings July 3, 2025 10:40
@martincostello martincostello requested a review from a team as a code owner July 3, 2025 10:40
Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR ensures HTTP request header matching favors registrations with more headers (more specific) before those with fewer, fixing nondeterministic first-match behavior, and updates solution format.

  • Sorts mappings by header count after priority to match more-specific requests first
  • Adds a test to verify correct header-matching precedence
  • Migrates from .sln to .slnx solution file

Reviewed Changes

Copilot reviewed 5 out of 5 changed files in this pull request and generated 1 comment.

Show a summary per file
File Description
tests/HttpClientInterception.Tests/Bundles/header-matching.json New bundle defining authorized and unauthorized request scenarios
tests/HttpClientInterception.Tests/Bundles/BundleExtensionsTests.cs Added test Can_Intercept_Http_Requests_With_Correct_Precedence_For_Http_Request_Headers
src/HttpClientInterception/HttpClientInterceptorOptions.cs Updated BuildResponseAsync to sort by number of request headers
HttpClientInterception.slnx Added new .slnx solution manifest
HttpClientInterception.sln Removed obsolete .sln file
Comments suppressed due to low confidence (1)

tests/HttpClientInterception.Tests/Bundles/BundleExtensionsTests.cs:631

  • The test currently asserts only the response body. It should also verify that the Content-Type header from the bundle is applied correctly (e.g. via firstResponse.Content.Headers.ContentType).
        first.ShouldBe("unauthorized");

@codecov
Copy link

codecov bot commented Jul 3, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 99.33%. Comparing base (5e92c65) to head (755c006).
Report is 1 commits behind head on main.

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #1061   +/-   ##
=======================================
  Coverage   99.32%   99.33%           
=======================================
  Files          15       15           
  Lines         886      896   +10     
  Branches      204      205    +1     
=======================================
+ Hits          880      890   +10     
  Misses          4        4           
  Partials        2        2           
Flag Coverage Δ
linux 99.33% <100.00%> (+<0.01%) ⬆️
macos 99.33% <100.00%> (+<0.01%) ⬆️
windows 99.33% <100.00%> (+<0.01%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

Copy link
Member

@slang25 slang25 left a comment

Choose a reason for hiding this comment

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

That makes sense, having more sensible defaults makes things more usable.
Thanks for the slnx migration too 🥳

@slang25 slang25 merged commit 24cf5c3 into justeattakeaway:main Jul 3, 2025
15 checks passed
@martincostello martincostello deleted the fix-request-header-matching branch July 3, 2025 11:05
martincostello added a commit to martincostello/dotnet-bumper that referenced this pull request Jul 3, 2025
Should be redundant with ingestion of fix from justeattakeaway/httpclient-interception#1061.
costellobot bot pushed a commit to martincostello/dotnet-bumper that referenced this pull request Jul 3, 2025
* Bump dependency JustEat.HttpClientInterception to 5.1.2

| datasource | package                        | from  | to    |
| ---------- | ------------------------------ | ----- | ----- |
| nuget      | JustEat.HttpClientInterception | 5.1.1 | 5.1.2 |


Signed-off-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>

* Remove explicit priorities

Should be redundant with ingestion of fix from justeattakeaway/httpclient-interception#1061.

---------

Signed-off-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>
Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>
Co-authored-by: Martin Costello <[email protected]>
@Demetrios-loutsios
Copy link
Contributor

Thanks for the slnx migration too 🥳

How very Martin of you! 😅 🙇

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants