-
Notifications
You must be signed in to change notification settings - Fork 4.9k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Implement UriCreationOptions #59173
Implement UriCreationOptions #59173
Conversation
Includes DangerousDisablePathAndQueryCanonicalization
Note regarding the This serves as a reminder for when your PR is modifying a ref *.cs file and adding/modifying public APIs, to please make sure the API implementation in the src *.cs file is documented with triple slash comments, so the PR reviewers can sign off that change. |
Tagging subscribers to this area: @dotnet/ncl Issue DetailsImplements a minimal subset of #59099: Not yet implemented: public struct UriCreationOptions
{
public UriKind UriKind { readonly get; set; }
public bool AllowImplicitFilePaths { readonly get; set; }
}
|
if (DisablePathAndQueryCanonicalization && (components & (UriComponents.Path | UriComponents.Query)) != 0) | ||
{ | ||
throw new InvalidOperationException(SR.net_uri_GetComponentsCalledWhenCanonicalizationDisabled); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Offline discussion: I'm concerned that there's no way for the consumer to know if DisablePathAndQueryCanonicalization is set and avoid this exception. This method can also throw IOE for relative Uris, but there you can check IsAbsoluteUri first.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This method can also throw IOE for relative Uris, but there you can check IsAbsoluteUri first.
In the current version this is unreachable for relative Uris (since UriKind
hasn't been exposed yet), but I will keep it in mind when adding the rest of the API. It's not critical as it would always throw anyway, but the change wasn't intentional.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we should consider adding a way to expose this, something like IsPathAndQueryCanonicalizationDisabled.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just to be super-clear: We can track that for 7.0, we do not consider it blocking attempt for 6.0 backport.
@geoffkizer please let us know if you disagree.
Test failure unrelated: #58356 |
public bool DangerousDisablePathAndQueryCanonicalization | ||
{ | ||
readonly get => _disablePathAndQueryCanonicalization; | ||
set => _disablePathAndQueryCanonicalization = value; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you please explain what readonly get
does, and link me to associated docs? Thank you
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/backport to release/6.0-rc2 |
Started backporting to release/6.0-rc2: https://github.com/dotnet/runtime/actions/runs/1246572044 |
Fixes #52628
Fixes #58057
Implements a minimal subset of #59099:
The
Uri
constructor overloads and theDangerousDisablePathAndQueryCanonicalization
property ofUriCreationOptions
.Not yet implemented: