Skip to content

Commit 879088e

Browse files
committed
fix(uri): Uri equality and hashing are canonical (#1995, SR-AUD-142)
Rule-14 sweep. operator== compared the raw input string and GetHashCode hashed it, so HTTP://EXAMPLE.COM:80/Path and http://example.com/Path were unequal with different hashes. Both members now feed from one canonical key, which is how .NET keeps them consistent -- it renders both from UriComponents.HttpRequestUrl = Scheme | Host | Port | Path | Query. The design record missed the most surprising half. Section 14.1 proposes only folding and the default port; the reference ALSO excludes the fragment and the user-info, in a comment of its own -- "Fragment AND UserInfo (for non-mailto URIs) are ignored" (Uri.cs:1833). A trap measurement found, not the record: defaultPortForScheme matches lower-case names only, so this port parses HTTP://example.com/ with port -1 and the lower-case form with 80. Comparing stored numbers would have left them unequal even after folding the scheme, defeating the repair on its own motivating input. The key resolves the default from the folded scheme, for comparison only; the parse is untouched. Not reproduced and stated: .NET compares UNC/DOS paths case-insensitively and hashes file: URIs likewise. This port models neither, so equality is narrower than .NET's -- never equal where .NET is unequal, the safe direction. UriBuilder is deliberately NOT delegated, though section 14.1 and .NET both say it should be: delegating reintroduces the throw #2004 measured and removed, so the two landed decisions conflict and the reference cannot settle it for this port. Filed as #2391. Six mutations, all caught. M2/M3/M6 were invalid as first written -- their anchors spelled \x01 as an escape rather than the literal text -- and were re-run rather than counted. Three pre-existing pins handled: two inverted (both existed to make this change visible) and one rewritten, with the obsolete name removed rather than left as a disabled stub. Downstream measured: 0 sites in both -- the single grep hit is a comment saying cna deliberately avoids System::Uri. Gate: 17,528 run, 17,528 passed, 0 failed, 0 skipped across 38 executables (+6 on 17,522; SharpRuntimeTests_Uri 294 -> 300). Module graph 41/93.
1 parent 81a666f commit 879088e

7 files changed

Lines changed: 289 additions & 32 deletions

File tree

CLAUDE.md

Lines changed: 1 addition & 1 deletion
Large diffs are not rendered by default.
Lines changed: 116 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,116 @@
1+
<!-- SPDX-License-Identifier: MIT -->
2+
<!-- Copyright (c) Robert Vokac and contributors -->
3+
4+
# Migration — `Uri` equality and hashing are canonical (ticket #1995, SR-AUD-142)
5+
6+
*2026-08-19.* `System::Uri::operator==` and `GetHashCode` compare and hash a **canonical
7+
identity** instead of the raw input string.
8+
9+
**Equality semantics change**, so the behaviour of any container keyed by `Uri` changes. Nothing
10+
that was equal becomes unequal — the change is a widening. Landed under **SA-5**; no signature,
11+
layout, vtable, mangled-symbol or `noexcept` change, and **no rendered string moves**.
12+
13+
---
14+
15+
## 1. What was wrong
16+
17+
`operator==` compared `absoluteUri_` verbatim and `GetHashCode` hashed it. Measured:
18+
`HTTP://EXAMPLE.COM:80/Path` and `http://example.com/Path` were **unequal, with different
19+
hashes**, although they name the same resource.
20+
21+
## 2. What .NET compares — and the half the design record missed
22+
23+
Both members feed from the **same** component set, which is why they cannot disagree:
24+
25+
```csharp
26+
UriComponents components = UriComponents.HttpRequestUrl; // Uri.cs:1835, 1539
27+
if (_syntax.InFact(UriSyntaxFlags.MailToLikeUri)) components |= UriComponents.UserInfo;
28+
string selfUrl = ... GetParts(components, UriFormat.SafeUnescaped);
29+
```
30+
31+
and `HttpRequestUrl = Scheme | Host | Port | Path | Query` (`UriEnumTypes.cs:43`).
32+
33+
**The design record proposed only "case-fold scheme and host, treat an explicit default port as
34+
absent".** The reference *also* excludes the **fragment** and the **user-info**, and says so in a
35+
comment of its own — *"Fragment AND UserInfo (for non-mailto URIs) are ignored"* (`Uri.cs:1833`).
36+
So:
37+
38+
```cpp
39+
Uri("http://example.com/p#one") == Uri("http://example.com/p#two") // true
40+
Uri("http://user@example.com/p") == Uri("http://example.com/p") // true
41+
```
42+
43+
Neither follows from §14.1's wording. Both are now asserted.
44+
45+
## 3. A trap measurement found, not the record
46+
47+
`defaultPortForScheme` matches **lower-case** scheme names only, so this port parses
48+
`HTTP://example.com/` with port **−1** and `http://example.com/` with port **80**. Comparing the
49+
*stored* numbers would therefore have left those two **unequal even after folding the scheme** —
50+
defeating the repair on exactly the input it exists for.
51+
52+
The identity key resolves the default from the **folded** scheme. The parse itself is untouched:
53+
`getPortProperty()` still returns −1 there, and the pre-existing pin asserting that still passes.
54+
55+
.NET has no such trap because it lower-cases the scheme *while parsing* and then resolves the
56+
default, so its `Port != other.Port` (`Uri.cs:1822`) already compares resolved numbers.
57+
58+
## 4. Why fold at comparison rather than at parse
59+
60+
This port deliberately performs **no parse-time canonicalisation** — an explicit exclusion of the
61+
review plan (§15). Folding at comparison reaches .NET's answer without touching `AbsoluteUri`,
62+
`OriginalString`, `getSchemeProperty()` or `getHostProperty()`, all of which still return the raw
63+
input. A test asserts that.
64+
65+
## 5. What is *not* reproduced, stated rather than discovered later
66+
67+
.NET compares the **path** case-insensitively for UNC and DOS paths
68+
(`IsUncOrDosPath ? OrdinalIgnoreCase : Ordinal`, `Uri.cs:1849`) and hashes a `file:` URI
69+
case-insensitively (`Uri.cs:1548-1551`). This port models **neither** `IsUncOrDosPath` nor
70+
`IsFile`, so the path is always compared case-sensitively.
71+
72+
That is a **narrower** equality than .NET's — it never calls equal two URIs .NET would call
73+
unequal — which is the safe direction. A test pins it.
74+
75+
## 6. `UriBuilder` is deliberately not changed, and that is a conflict, not an omission
76+
77+
§14.1 also says *"`UriBuilder` delegates to `Uri`"*, and .NET does exactly that
78+
(`GetHashCode() => Uri.GetHashCode()`, `UriBuilder.cs:279`). **Delegating would reintroduce a
79+
defect this repository already measured and removed.** .NET's `Uri` property builds the Uri and
80+
can **throw**; ticket #2004 moved `UriBuilder::GetHashCode` off that route precisely because of
81+
it, listing four routes through *ordinary setters* where it throws.
82+
83+
So the two landed decisions conflict, and the reference does not settle it *for this port*: .NET
84+
is consistent because **both** its members go through `Uri` and throw together; this port is
85+
consistent because **neither** does. Delegating one alone restores the inconsistency #2004
86+
removed.
87+
88+
That is **ticket #2391**, not something to resolve silently here. The gap #1995 widens — a builder
89+
identity that is raw text while `Uri`'s is canonical — is pinned by
90+
`Decl1995_BuilderHashNoLongerMatchesTheBuiltUrisHash`.
91+
92+
## 7. Evidence
93+
94+
Six mutations, **all caught**: no case folding; the fragment included; the user-info included;
95+
the default port not resolved; the hash reverting to `absoluteUri_`; the query dropped.
96+
97+
M2, M3 and M6 were **invalid as first written** — their anchors spelled `\x01` as an escape
98+
rather than as the literal backslash text in the source — and were re-run rather than counted.
99+
100+
Two pre-existing pins were **inverted rather than deleted**, because both existed to make this
101+
change visible: `DocumentedContract_CaseDifferingUrisAreNotEqualYet` said *"this test must be
102+
updated when identity changes"*, and `UriIdentityItselfIsUnchangedByThisTicket` was written by
103+
#2004 so a reader would not mistake **that** ticket for an identity change. A third,
104+
`HashIsValueIdenticalWhereTheOldRouteSucceeded`, asserted a #2004 compatibility claim this ticket
105+
invalidates, and was rewritten to assert the new relationship and name #2391.
106+
107+
Gate: **17,528 run, 17,528 passed, 0 failed, 0 skipped** across 38 executables — `+6` on 17,522
108+
(`SharpRuntimeTests_Uri` 294 → 300: seven cases added, two pins inverted in place, and one
109+
obsolete pin removed rather than left disabled). No other executable moved. Module graph unchanged
110+
at 41/93.
111+
112+
## 8. Downstream, measured
113+
114+
`System::Uri` appears in **zero** places in `cna` and **zero** in `mobile-eggbert`. The one grep
115+
hit is a **comment** in `cna/modules/media/src/Xna/Song.cpp` explaining that it deliberately does
116+
*not* use `System::Uri`. Neither repository was modified.

modules/uri/include/System/Uri.hpp

Lines changed: 10 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -59,6 +59,16 @@ namespace System {
5959
/** Parses @p uriString into component fields. Called by all constructors. */
6060
void parse(const std::string& uriString);
6161

62+
/**
63+
* @brief Builds the canonical string that `operator==` compares and `GetHashCode` hashes.
64+
*
65+
* Ticket #1995 / SR-AUD-142. Both identity members feed from this one function, so they
66+
* cannot disagree -- the same guarantee .NET gets by feeding both from
67+
* `GetParts(UriComponents.HttpRequestUrl, ...)`. See the definition in `Uri.cpp` for what
68+
* is included, what is deliberately excluded, and what is not reproduced.
69+
*/
70+
[[nodiscard]] std::string identityKey() const;
71+
6272
public:
6373
/**
6474
* @brief Constructs a Uri from an absolute or relative URI string.

modules/uri/src/System/Uri.cpp

Lines changed: 70 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -642,15 +642,83 @@ bool Uri::getIsLoopbackProperty() const {
642642

643643
std::string Uri::ToString() const { return absoluteUri_; }
644644

645+
// ---------------------------------------------------------------------------
646+
// Ticket #1995 / SR-AUD-142 — URI identity.
647+
//
648+
// operator== compared absoluteUri_ verbatim and GetHashCode hashed it, so
649+
// "HTTP://EXAMPLE.COM:80/Path" and "http://example.com/Path" were UNEQUAL with different hashes,
650+
// although they name the same resource.
651+
//
652+
// .NET compares and hashes the SAME component set, which is why the two agree by construction:
653+
//
654+
// UriComponents components = UriComponents.HttpRequestUrl; // Uri.cs:1835, 1539
655+
// if (_syntax.InFact(UriSyntaxFlags.MailToLikeUri)) components |= UriComponents.UserInfo;
656+
// string selfUrl = ... GetParts(components, UriFormat.SafeUnescaped);
657+
//
658+
// and HttpRequestUrl is `Scheme | Host | Port | Path | Query` (UriEnumTypes.cs:43).
659+
//
660+
// THE DESIGN RECORD MISSED THE MOST SURPRISING HALF OF THAT. Section 14.1 proposes only
661+
// "case-fold scheme and host, treat an explicit default port as absent"; the reference ALSO
662+
// EXCLUDES THE FRAGMENT AND THE USER-INFO, and says so in a comment of its own -- "Fragment AND
663+
// UserInfo (for non-mailto URIs) are ignored" (Uri.cs:1833). So `http://a/b#x == http://a/b#y`
664+
// and `http://u@a/b == http://a/b`, which no reading of the record would have predicted.
665+
//
666+
// Scheme and host are folded HERE rather than at parse time because this port deliberately does
667+
// no parse-time canonicalisation (review plan section 15); .NET lower-cases them while parsing,
668+
// so by the time its equality runs they are already canonical. Folding at comparison reaches the
669+
// same answer without touching AbsoluteUri or OriginalString -- no rendered string moves.
670+
//
671+
// NOT REPRODUCED, and measured rather than assumed: .NET compares the PATH case-insensitively for
672+
// UNC and DOS paths (`IsUncOrDosPath ? OrdinalIgnoreCase : Ordinal`, Uri.cs:1849) and hashes a
673+
// file: URI case-insensitively (Uri.cs:1548-1551). This port models neither IsUncOrDosPath nor
674+
// IsFile, so the path is always compared case-sensitively. That is a narrower equality than
675+
// .NET's -- it never calls equal two URIs .NET would call unequal -- and a test pins it.
676+
std::string Uri::identityKey() const {
677+
// A relative reference has no components to canonicalise: .NET compares OriginalString
678+
// (Uri.cs:1752-1753).
679+
if (!isAbsoluteUri_) return "\x01relative\x01" + absoluteUri_;
680+
681+
const auto fold = [](std::string v) {
682+
for (char& c : v)
683+
if (c >= 'A' && c <= 'Z') c = static_cast<char>(c - 'A' + 'a');
684+
return v;
685+
};
686+
687+
const std::string foldedScheme = fold(scheme_);
688+
689+
// THE DEFAULT PORT MUST BE RESOLVED FROM THE FOLDED SCHEME, and that is a trap measurement
690+
// found rather than the design record: defaultPortForScheme matches lower-case names only, so
691+
// this port parses "HTTP://example.com/" with port -1 and "http://example.com/" with port 80.
692+
// Comparing the stored numbers would therefore have left those two UNEQUAL even after
693+
// case-folding the scheme -- defeating the repair on exactly the input it exists for.
694+
// (`UriTests.DocumentedContract_MixedCaseSchemeHasNoDefaultPort` pins that parse behaviour,
695+
// which is unchanged: this resolves for COMPARISON only.)
696+
//
697+
// .NET has no such trap because it lower-cases the scheme while parsing and then resolves the
698+
// default, so its `Port != other.Port` (Uri.cs:1822) is already comparing resolved numbers.
699+
const intcs resolvedPort = (port_ == -1) ? defaultPortForScheme(foldedScheme) : port_;
700+
701+
return foldedScheme + "\x01" + fold(host_) + "\x01" + std::to_string(resolvedPort) + "\x01"
702+
+ path_ + "\x01" + query_;
703+
}
704+
645705
intcs Uri::GetHashCode() const {
646-
return static_cast<intcs>(std::hash<std::string>{}(absoluteUri_));
706+
// Hashes exactly what operator== compares, so the two cannot disagree -- which is the same
707+
// guarantee .NET gets by feeding both from GetParts(HttpRequestUrl, ...).
708+
return static_cast<intcs>(std::hash<std::string>{}(identityKey()));
647709
}
648710

649711
// ---------------------------------------------------------------------------
650712
// Operators
651713
// ---------------------------------------------------------------------------
652714

653-
bool Uri::operator==(const Uri& other) const { return absoluteUri_ == other.absoluteUri_; }
715+
bool Uri::operator==(const Uri& other) const {
716+
// An absolute and a relative reference are never equal: .NET returns false as soon as
717+
// IsAbsoluteUri differs (Uri.cs:1749-1750). The key encodes that, but stating it here keeps
718+
// the intent visible.
719+
if (isAbsoluteUri_ != other.isAbsoluteUri_) return false;
720+
return identityKey() == other.identityKey();
721+
}
654722
bool Uri::operator!=(const Uri& other) const { return !(*this == other); }
655723

656724
// ---------------------------------------------------------------------------

modules/uri/tests/System/UriBuilderTests.cpp

Lines changed: 29 additions & 22 deletions
Original file line numberDiff line numberDiff line change
@@ -2,6 +2,7 @@
22
// Copyright (c) Robert Vokac and contributors
33
// Portions based on .NET runtime API (MIT License, Copyright .NET Foundation and Contributors)
44
#include <gtest/gtest.h>
5+
#include <functional>
56
#include "System/UriBuilder.hpp"
67
#include "System/ArgumentException.hpp"
78
#include "System/ArgumentOutOfRangeException.hpp"
@@ -310,19 +311,26 @@ TEST(UriBuilderTest, HashIsObtainableWhereverEqualsSucceeds_OtherUnparseableRend
310311
}
311312
}
312313

313-
TEST(UriBuilderTest, HashIsValueIdenticalWhereTheOldRouteSucceeded) {
314-
// The compatibility claim, asserted rather than argued: for every builder whose
315-
// rendering parses, hashing the string and hashing the built Uri give the same number.
316-
UriBuilder shapes[6];
317-
shapes[0].setHostProperty("example.com");
318-
shapes[1].setHostProperty("example.com"); shapes[1].setPortProperty(8080);
319-
shapes[2].setHostProperty("example.com"); shapes[2].setUserNameProperty("u");
320-
shapes[2].setPasswordProperty("p");
321-
shapes[3].setHostProperty("example.com"); shapes[3].setQueryProperty("a=1");
322-
shapes[4].setHostProperty("example.com"); shapes[4].setFragmentProperty("f");
323-
shapes[5].setHostProperty("[::1]"); shapes[5].setPortProperty(443);
324-
for (const UriBuilder& b : shapes)
325-
EXPECT_EQ(b.GetHashCode(), b.getUriProperty().GetHashCode()) << b.ToString();
314+
TEST(UriBuilderTest, Decl1995_BuilderHashNoLongerMatchesTheBuiltUrisHash) {
315+
// REWRITTEN by #1995. This asserted #2004's compatibility claim -- that for every builder
316+
// whose rendering parses, hashing the string and hashing the built Uri give the same number.
317+
// That claim rested on Uri::GetHashCode hashing absoluteUri_ verbatim, which #1995 changed:
318+
// Uri now hashes a canonical identity key (folded scheme and host, resolved port, no fragment
319+
// or user-info), so the two numbers legitimately differ.
320+
//
321+
// The builder's own hash is UNCHANGED and still hashes the rendered string, because #2004
322+
// measured four ordinary setter routes where building a Uri THROWS -- see this file's
323+
// GetHashCode doc-comment. Making the builder delegate to Uri, which is what .NET does
324+
// (`GetHashCode() => Uri.GetHashCode()`, UriBuilder.cs:279), would reintroduce that throw.
325+
// That conflict is ticket #2391, not something to resolve silently here.
326+
UriBuilder b;
327+
b.setHostProperty("EXAMPLE.COM");
328+
b.setPortProperty(80);
329+
EXPECT_EQ(b.GetHashCode(), static_cast<SharpRuntime::intcs>(
330+
std::hash<std::string>{}(b.ToString())))
331+
<< "the builder still hashes its rendered string";
332+
EXPECT_NE(b.GetHashCode(), b.getUriProperty().GetHashCode())
333+
<< "and the built Uri now hashes its canonical identity instead";
326334
}
327335

328336
TEST(UriBuilderTest, EqualsAndGetHashCodeAreTotalOnTheSameSet) {
@@ -379,21 +387,20 @@ TEST(UriBuilderTest, DeliberatelyUnequalPairsStayUnequal_PinsTheGatedIdentityCha
379387
// and only they have to be updated when #1995 lands.
380388
}
381389

382-
TEST(UriBuilderTest, UriIdentityItselfIsUnchangedByThisTicket) {
383-
// Uri's own operator== and GetHashCode were already paired (both read absoluteUri_) and
384-
// this ticket does not touch them. Pinned so a later reader does not mistake #2004 for
385-
// an identity change.
390+
TEST(UriBuilderTest, Fix1995_UriIdentityIsNowCanonical) {
391+
// INVERTED by #1995. This was written by #2004 to say "this ticket does not change Uri
392+
// identity", so a later reader would not mistake #2004 for an identity change. #1995 IS that
393+
// change, so the pin's subject moves rather than the pin being deleted.
386394
System::Uri a("http://example.com/p");
387395
System::Uri b("http://example.com/p");
388396
System::Uri caseDiff("HTTP://EXAMPLE.COM/p");
389397
System::Uri explicitDefaultPort("http://example.com:80/p");
390-
// Equal Uris hash equally -- the contract direction, kept. The two matching inequalities for
391-
// the unequal pairs were removed by #2284: a collision between unequal values is legal
392-
// (docs/HashAssertionContractRule.md R2), so the operator== assertions carry the pin.
393398
EXPECT_TRUE(a == b);
394399
EXPECT_EQ(a.GetHashCode(), b.GetHashCode());
395-
EXPECT_FALSE(a == caseDiff);
396-
EXPECT_FALSE(a == explicitDefaultPort);
400+
EXPECT_TRUE(a == caseDiff) << "#1995: scheme and host are folded for comparison";
401+
EXPECT_TRUE(a == explicitDefaultPort) << "#1995: an explicit default port is the default";
402+
EXPECT_EQ(a.GetHashCode(), caseDiff.GetHashCode());
403+
EXPECT_EQ(a.GetHashCode(), explicitDefaultPort.GetHashCode());
397404
}
398405

399406
TEST(UriBuilderTest, Layout_SizeOfUriBuilderIsPinned) {

0 commit comments

Comments
 (0)