Fix validated part uri equality - #131806
Fix validated part uri equality #131806alinpahontu2912 merged 9 commits into
Conversation
PR dotnet#118574 added object.Equals/GetHashCode overrides to ValidatedPartUri (which derives from System.Uri) to support case-insensitive part-name matching. This broke the inherited Uri equality contract: Equals became asymmetric against a plain System.Uri of the same value, and GetHashCode became inconsistent with System.Uri.GetHashCode(). Consumers mixing ValidatedPartUri with plain System.Uri in HashSet<Uri>/Dictionary<Uri,_> (e.g. the Open XML SDK) got incorrect lookup results. - Revert the Equals(object?)/GetHashCode() overrides, restoring the original CA1067 suppression. ValidatedPartUri once again inherits Uri's value-equality, matching plain System.Uri semantics. - Add PackUriHelper.ValidatedPartUriEqualityComparer, an internal IEqualityComparer<ValidatedPartUri> providing case-insensitive equality/hashing, and use it explicitly in ZipPackage's content-type override dictionary. Without this, removing GetHashCode() would leave that Dictionary<ValidatedPartUri,_> with a hash/equals mismatch (case-insensitive Equals via IEquatable<T>, case-sensitive hash via inherited Uri.GetHashCode()), silently dropping parts whose zip entry name casing differs from their [Content_Types].xml Override PartName. - Add regression tests covering the Uri equality contract and the content-type override case-insensitivity behavior. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: f1ae4251-6f11-4001-afb3-7cfdea3b64b5
…pare PR dotnet#118574 changed Compare() to use StringComparison.OrdinalIgnoreCase instead of string.CompareOrdinal. This is redundant: both operands are NormalizedPartUriString, which is already case-folded via ToUpperInvariant() before the comparison runs, so Ordinal and OrdinalIgnoreCase comparisons produce identical results for realistic part-name characters. Reverting to string.CompareOrdinal restores the original code, minimizes the diff, and removes any residual risk from divergent Unicode case-folding tables between ToUpperInvariant and OrdinalIgnoreCase for non-ASCII edge cases. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: f1ae4251-6f11-4001-afb3-7cfdea3b64b5
|
Azure Pipelines: Successfully started running 3 pipeline(s). 13 pipeline(s) were filtered out due to trigger conditions. There may be pipelines that require an authorized user to comment /azp run to run. |
|
Tagging subscribers to this area: @dotnet/area-system-io |
There was a problem hiding this comment.
Pull request overview
This PR restores PackUriHelper.ValidatedPartUri’s compatibility with System.Uri’s object.Equals/GetHashCode contract (so it can safely interoperate with plain Uri instances in hash-based collections), while keeping case-insensitive part-name semantics where needed via an explicit internal comparer.
Changes:
- Remove
ValidatedPartUri’sEquals(object?)/GetHashCode()overrides to re-align with baseSystem.Uriequality/hashing behavior. - Introduce
PackUriHelper.ValidatedPartUriEqualityComparerand use it for ZipPackage’s override content-type dictionary to preserve case-insensitive override lookups without changingUri’s base contract. - Add regression tests covering (1) the
Uriequality/hash contract and (2) case-insensitive content-type override resolution.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| src/libraries/System.IO.Packaging/tests/PartPieceTests.cs | Adds regression tests for ValidatedPartUri equality/hash compatibility and for case-insensitive override content-type lookup. |
| src/libraries/System.IO.Packaging/src/System/IO/Packaging/ZipPackage.cs | Updates the override dictionary to use the new ValidatedPartUriEqualityComparer. |
| src/libraries/System.IO.Packaging/src/System/IO/Packaging/PackUriHelper.cs | Removes Equals(object?)/GetHashCode() overrides from ValidatedPartUri and adds the internal equality comparer type. |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated no new comments.
Suppressed comments (4)
src/libraries/System.IO.Packaging/src/System/IO/Packaging/ZipPackage.cs:949
- EnsureOverrideDictionary() now uses ValidatedPartUriEqualityComparer, but the comment still says comparison is done using PartUriComparer. This is misleading when debugging override lookups and should be updated to reflect the actual comparer in use.
// The part Uris are stored in the Override Dictionary in their original form , but they are compared
// in a normalized manner using the PartUriComparer
src/libraries/System.IO.Packaging/tests/PartPieceTests.cs:377
- The symmetry assertion can pass even if both directions return false. Since this test is specifically about the equality contract for two URIs that should be value-equal, it should assert both directions are true (not just equal).
// object.Equals must be symmetric.
Assert.Equal(a.Equals(b), b.Equals(a));
src/libraries/System.IO.Packaging/src/System/IO/Packaging/PackUriHelper.cs:607
- The CA1067 suppression comment is self-contradictory ("Override Equals...; not overriding...") and doesn’t explain the real intent (preserving System.Uri’s equality/hash contract). This is easy to misread during future maintenance.
#pragma warning disable CA1067 // Override Equals because it implements IEquatable<T>; not overriding to avoid possible regressions in code that's working
internal sealed class ValidatedPartUri : Uri, IComparable<ValidatedPartUri>, IEquatable<ValidatedPartUri>
#pragma warning restore CA1067
src/libraries/System.IO.Packaging/src/System/IO/Packaging/PackUriHelper.cs:908
- ValidatedPartUri.NormalizedPartUriString is already case-folded to upper invariant (see GetNormalizedPartUriString), so using OrdinalIgnoreCase hashing here is unnecessary and slightly more expensive/misleading. Using ordinal hashing better matches the comparer’s actual semantics (ordinal compare on normalized strings).
public int GetHashCode(ValidatedPartUri obj)
=> StringComparer.OrdinalIgnoreCase.GetHashCode(obj.NormalizedPartUriString);
|
Azure Pipelines: Successfully started running 3 pipeline(s). 13 pipeline(s) were filtered out due to trigger conditions. There may be pipelines that require an authorized user to comment /azp run to run. |
rzikm
left a comment
There was a problem hiding this comment.
Some copilot suggestions seem relevant, otherwise LGTM if tests pass
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated no new comments.
Suppressed comments (1)
src/libraries/System.IO.Packaging/src/System/IO/Packaging/PackUriHelper.cs:907
- ValidatedPartUri.NormalizedPartUriString is already case-folded to upper-invariant (see GetNormalizedPartUriString), so using StringComparer.OrdinalIgnoreCase here does extra work without changing behavior. Using StringComparer.Ordinal keeps equality/hash consistency while reducing hashing cost.
public int GetHashCode(ValidatedPartUri obj)
=> StringComparer.OrdinalIgnoreCase.GetHashCode(obj.NormalizedPartUriString);
MihaZupan
left a comment
There was a problem hiding this comment.
So besides the hash code inconsistency, it seems that this type is also breaking invariants in other ways?
E.g. a.Equals(b) may not mean that a.Equals((object)b)
or a.Compare(b) == 0 may not mean that a == b?
These seem like pre-existing issues though
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated no new comments.
Suppressed comments (1)
src/libraries/System.IO.Packaging/src/System/IO/Packaging/PackUriHelper.cs:605
- The CA1067 suppression comment is contradictory (it says "Override Equals" while the code intentionally does not override object.Equals/GetHashCode). This makes the rationale unclear for future maintainers.
#pragma warning disable CA1067 // Override Equals because it implements IEquatable<T>; not overriding to avoid possible regressions in code that's working
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated no new comments.
Suppressed comments (1)
src/libraries/System.IO.Packaging/src/System/IO/Packaging/PackUriHelper.cs:907
ValidatedPartUri.NormalizedPartUriStringis always upper-cased (seeGetNormalizedPartUriString()), so usingStringComparer.OrdinalIgnoreCasehere is redundant and adds extra work on every hash computation. UsingStringComparer.Ordinalis sufficient and keeps the comparer consistent with the normalization strategy.
public int GetHashCode(ValidatedPartUri obj)
=> StringComparer.OrdinalIgnoreCase.GetHashCode(obj.NormalizedPartUriString);
….com/alinpahontu2912/runtime into fix/validatedparturi-equality-118574
Fixes dotnet#129927 PR dotnet#118574 added object.Equals/GetHashCode overrides to ValidatedPartUri (which derives from System.Uri) to support case-insensitive part-name matching. This broke the inherited Uri equality contract: Equals became asymmetric against a plain System.Uri of the same value, and GetHashCode became inconsistent with System.Uri.GetHashCode(). Consumers mixing ValidatedPartUri with plain System.Uri in HashSet/Dictionary<Uri,_> (e.g. the Open XML SDK) got incorrect lookup results. Revert the Equals(object?)/GetHashCode() overrides, restoring the original CA1067 suppression. ValidatedPartUri once again inherits Uri's value-equality, matching plain System.Uri semantics. Keep the case-insensitive Compare()/IComparable/ IEquatable logic, which is what makes Package.cs's SortedList<ValidatedPartUri, PackagePart> lookups case-insensitive (fixing dotnet#112783 for that path) and is unaffected by this change. Add PackUriHelper.ValidatedPartUriEqualityComparer, an internal IEqualityComparer providing case-insensitive equality/hashing, and use it explicitly in ZipPackage's content-type override dictionary. Without this, removing GetHashCode() would leave that Dictionary<ValidatedPartUri,_> with a hash/equals mismatch (case-insensitive Equals via IEquatable, case-sensitive hash via inherited Uri.GetHashCode()), silently dropping parts whose zip entry name casing differs from their [Content_Types].xml Override PartName. Add regression tests covering the Uri equality contract and the content-type override case-insensitivity behavior. --------- Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com> Copilot-Session: f1ae4251-6f11-4001-afb3-7cfdea3b64b5
Fixes #129927
PR #118574 added object.Equals/GetHashCode overrides to ValidatedPartUri (which derives from System.Uri) to support case-insensitive part-name matching. This broke the inherited Uri equality contract: Equals became asymmetric against a plain System.Uri of the same value, and GetHashCode became inconsistent with System.Uri.GetHashCode(). Consumers mixing ValidatedPartUri with plain System.Uri in HashSet/Dictionary<Uri,_> (e.g. the Open XML SDK) got incorrect lookup results.
Revert the Equals(object?)/GetHashCode() overrides, restoring the original CA1067 suppression. ValidatedPartUri once again inherits Uri's value-equality, matching plain System.Uri semantics.
Keep the case-insensitive Compare()/IComparable/ IEquatable logic, which is what makes Package.cs's SortedList<ValidatedPartUri, PackagePart> lookups case-insensitive (fixing #112783 for that path) and is unaffected by this change.
Add PackUriHelper.ValidatedPartUriEqualityComparer, an internal IEqualityComparer providing case-insensitive equality/hashing, and use it explicitly in ZipPackage's content-type override dictionary. Without this, removing GetHashCode() would leave that Dictionary<ValidatedPartUri,_> with a hash/equals mismatch (case-insensitive Equals via IEquatable, case-sensitive hash via inherited Uri.GetHashCode()), silently dropping parts whose zip entry name casing differs from their [Content_Types].xml Override PartName.
Add regression tests covering the Uri equality contract and the content-type override case-insensitivity behavior.