fix(mocks-http): auto-prepend / to partial endpoint paths (#5838)#5874
fix(mocks-http): auto-prepend / to partial endpoint paths (#5838)#5874
/ to partial endpoint paths (#5838)#5874Conversation
`Mock.HttpClient(...).Handler.OnPost("my_endpoint")` previously failed to
match because the request URI's `PathAndQuery` is `/my_endpoint`. Normalize
in `RequestMatcher.Path()` and `PathStartsWith()` so partial and rooted
forms are equivalent. Absolute URLs (containing `://`) and regex patterns
are left untouched.
Closes #5838
There was a problem hiding this comment.
Code Review
Overall: Solid, well-scoped fix. The implementation is clean and the test coverage is good. A few things worth flagging:
Coupling between and the absolute-URI fallback in
The NormalizePath helper skips absolute URLs (via Contains("://")), and separately, MatchesPath has a fallback that checks the full requestUri.ToString() when the path-only check fails:
// MatchesPath
if (_exactPath != null && !string.Equals(path, _exactPath, ...))
{
var absUri = requestUri?.ToString() ?? "";
if (!string.Equals(absUri, _exactPath, ...))
return false;
}These two mechanisms are co-conspirators — the :// guard in NormalizePath ensures absolute URLs reach MatchesPath unmodified so the fallback can do its job — but this contract is entirely implicit and not co-located. A future developer could update one without realising the other depends on it. A short comment like:
// Absolute URLs are left as-is; MatchesPath has a full-URI fallback for them.
if (value.Contains("://", StringComparison.Ordinal)) return value;…would make the invariant explicit.
has no absolute-URI fallback (pre-existing, but worth noting)
_exactPath has the double-check against the raw URI string; _pathPrefix does not. This means PathStartsWith("https://my.domain/api") silently never matches, even though the symmetric Path("https://my.domain/foo") does work. This PR doesn't change that, but since you're touching this area it's worth a TODO comment or a follow-up issue to make both matchers consistent.
test name is slightly misleading
PathMatches uses a completely separate field (_pathPattern) and was never wired through NormalizePath, so the test is really just confirming that existing regex behaviour was not accidentally broken by the refactor. That's useful, but the test name implies the normalization was considered and rejected for regex — a comment like // PathMatches stores a compiled Regex, not a plain string, so NormalizePath is not involved would convey that intent.
now shows the normalized path
_exactPath and _pathPrefix are stored after normalization, so Describe() (used in failure messages) will render /my_endpoint even when the user wrote my_endpoint. That's arguably better UX (shows what's actually matched), but it's a subtle behaviour change worth being aware of. No action needed, just noting it.
The fix itself is correct, the NormalizePath helper is a clean abstraction, and the four regression tests cover the important boundaries. The main concern is the undocumented contract between NormalizePath and the absolute-URI fallback in MatchesPath — a one-line comment would significantly reduce future maintenance risk.
There was a problem hiding this comment.
Code Review
Overall: Solid, well-scoped fix. The implementation is clean and the test coverage is good. A few things worth flagging.
Coupling between NormalizePath and the absolute-URI fallback in MatchesPath
The NormalizePath helper skips absolute URLs (via Contains("://")), and separately, MatchesPath has a fallback that checks the full requestUri.ToString() when the path-only check fails. These two mechanisms are co-conspirators: the "://" guard ensures absolute URLs reach MatchesPath unmodified so the fallback can do its job, but this contract is entirely implicit and not co-located. A future developer could update one without realising the other depends on it.
A short comment on that branch in NormalizePath would make the invariant explicit:
// Absolute URLs are left as-is; MatchesPath has a full-URI fallback for them.
if (value.Contains("://", StringComparison.Ordinal)) return value;
PathStartsWith has no absolute-URI fallback (pre-existing, but worth noting)
_exactPath has the double-check against the raw URI string in MatchesPath; _pathPrefix does not. This means PathStartsWith("https://my.domain/api") silently never matches, even though the symmetric Path("https://my.domain/foo") does work. This PR does not introduce the asymmetry, but since you are touching this area it is a good opportunity for a TODO comment or a follow-up issue to make both matchers consistent.
MatchesByPath_RegexNotNormalized test name is slightly misleading
PathMatches uses a completely separate field (_pathPattern) and was never wired through NormalizePath, so the test is really just confirming that existing regex behaviour was not accidentally broken by the refactor. That is useful, but the test name implies the normalization was considered and rejected for regex. A comment explaining that PathMatches stores a compiled Regex so NormalizePath is not involved would convey that intent more clearly.
Describe() now shows the normalized path
_exactPath and _pathPrefix are stored after normalization, so Describe() (used in failure messages) will render "/my_endpoint" even when the user wrote "my_endpoint". That is arguably better UX since it shows what is actually being matched, but it is a subtle behaviour change worth being aware of. No action needed, just noting it.
The fix itself is correct, the NormalizePath helper is a clean abstraction, and the four regression tests cover the important boundaries. The main concern is the undocumented contract between NormalizePath and the absolute-URI fallback in MatchesPath: a one-line comment there would significantly reduce future maintenance risk.
Not up to standards ⛔🔴 Issues
|
| Category | Results |
|---|---|
| CodeStyle | 2 minor |
NEW Get contextual insights on your PRs based on Codacy's metrics, along with PR and Jira context, without leaving GitHub. Enable AI reviewer
TIP This summary will be updated as you push new changes.
There was a problem hiding this comment.
Follow-up Review (commit d98a339)
The refactor commit makes two changes to NormalizePath:
- Merges the null/rooted guard into a single expression — clean, no concern.
- Replaces
value.Contains("://", StringComparison.Ordinal)withUri.TryCreate(value, UriKind.Absolute, out _)— this is a genuine improvement. Using the BCL's URI parser is more semantically correct than a string probe; it handles edge cases likeurn:schemes, avoids potential false positives on path segments that coincidentally contain://, and makes the intent explicit.
Outstanding from the previous review
Undocumented coupling between NormalizePath and MatchesPath's absolute-URI fallback — still not addressed. The invariant (absolute URIs must reach MatchesPath unnormalized so the requestUri.ToString() fallback can fire) is still implicit. A one-line comment on the Uri.TryCreate branch in NormalizePath would pay it off.
PathStartsWith has no absolute-URI fallback — pre-existing asymmetry, still not addressed. Calling PathStartsWith("https://my.domain/api") silently never matches because path is PathAndQuery (rooted) but there's no fallback to compare against requestUri.ToString(), unlike _exactPath. A TODO comment or a follow-up issue is sufficient.
MatchesByPath_RegexNotNormalized test name — still slightly misleading (see prior review). No action needed if the intent is just regression-guarding existing behaviour, but a brief inline comment would clarify.
New observation on Uri.TryCreate
One subtle edge case: Uri.TryCreate("localhost:8080/api", UriKind.Absolute, out _) returns true on .NET (the BCL treats localhost as a custom URI scheme). So a caller who writes OnGet("localhost:8080/api") will get the value stored as-is and silently never match — whereas the old Contains("://") check would at least have prepended / (also broken, but differently). This is an unlikely usage, not a blocker, but worth a comment or a note in the XML doc that path is expected to be a rooted path or a full http(s):// URL.
Overall the refactor is correct and the core bug fix is solid. The two outstanding actionable items from the first review (coupling comment, PathStartsWith asymmetry note) remain open.
Summary
RequestMatcher.Path()andPathStartsWith()soOnPost("my_endpoint")matches the same requests asOnPost("/my_endpoint").://) and regex patterns (PathMatches), preserving existing behaviour.MockHttpHandlerTests.Why
Per #5838, the mismatch is silent and easy to overlook: requests resolve to
RequestUri.PathAndQuery(always rooted with/), but the configured path is stored verbatim, so a partial endpoint never matches and falls through to the default 404 (orHttpRequestExceptionwhenThrowOnUnmatched).Test plan
dotnet test --project TUnit.Mocks.Http.Tests --framework net10.0— 45/45 passing (4 new).Closes #5838