Skip to content

Conversation

lilinus
Copy link
Contributor

@lilinus lilinus commented Jun 18, 2025

Fixes #110480

@Copilot Copilot AI review requested due to automatic review settings June 18, 2025 15:05
Copy link
Contributor

@Copilot 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 fixes a URI edge case by adjusting the offset calculations during URI parsing and adding a test case to validate the behavior when leading whitespace and Unicode characters are present.

  • Updated the offset assignments for scheme, user, and host in the URI parsing logic.
  • Added a new functional test to verify URI construction under edge-case conditions in Windows and non-Windows environments.

Reviewed Changes

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

File Description
src/libraries/System.Private.Uri/tests/FunctionalTests/UriTests.cs Added a new test verifying the behavior of URI parsing with leading whitespace and Unicode.
src/libraries/System.Private.Uri/src/System/Uri.cs Updated offset fields in the URI parsing method to address the edge-case bug.
Comments suppressed due to low confidence (2)

src/libraries/System.Private.Uri/tests/FunctionalTests/UriTests.cs:805

  • [nitpick] Consider adding an inline comment explaining the intention behind the chosen URI string format and handling of leading whitespace/unicode characters for clarity in future maintenance.
            string uriString = PlatformDetection.IsWindows ? "\tC:\\\u005C" : "\t/\u005C";

src/libraries/System.Private.Uri/src/System/Uri.cs:3301

  • [nitpick] It might be beneficial to document why the offset fields for User and Host are set to the current string length immediately after setting the Scheme offset, to aid future maintainers in understanding the edge-case handling.
                    _info.Offset.Scheme = 0;

@dotnet-policy-service dotnet-policy-service bot added the community-contribution Indicates that the PR has been added by a community member label Jun 18, 2025
Copy link
Contributor

Tagging subscribers to this area: @dotnet/ncl
See info in area-owners.md if you want to be subscribed.

Copy link
Member

@MihaZupan MihaZupan left a comment

Choose a reason for hiding this comment

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

Test failures are related

@lilinus lilinus requested a review from MihaZupan June 24, 2025 14:26
Copy link
Member

@MihaZupan MihaZupan left a comment

Choose a reason for hiding this comment

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

Thank you

@MihaZupan MihaZupan added this to the 10.0.0 milestone Jun 25, 2025
@MihaZupan MihaZupan merged commit 347ac70 into dotnet:main Jun 26, 2025
80 of 86 checks passed
@github-actions github-actions bot locked and limited conversation to collaborators Jul 26, 2025
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

area-System.Net community-contribution Indicates that the PR has been added by a community member

Projects

None yet

Development

Successfully merging this pull request may close these issues.

System.Uri.ToString() throws despite Uri.TryCreate succeeding

2 participants