-
Notifications
You must be signed in to change notification settings - Fork 4.6k
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
[Uri] Uri.EscapeUriString should be deprecated #31387
Comments
@MihaZupan can you please look into this one to validate the conclusions above? |
From the linked thread (emphasis mine):
I believe this is a valid use-case. I am not aware of a direct/better alternative for such a scenario, so I don't think the method can be deprecated. The documentation for it could, however, be updated to note its intended usage - if there are other valid usage scenarios for it I would love to know. |
@MihaZupan so what is a concrete example, in which case |
The two methods have different use-cases. For example, if you are escaping a query key or value, you should use string uri = "https://foo.bar/?someKey=" + Uri.EscapeDataString(someValue); But, if you are trying to escape a whole uri, you should not use string userProvidedUri = "http://foo.bar/a b";
string escapeData = Uri.EscapeDataString(userProvidedUri);
// http%3A%2F%2Ffoo.bar%2Fa%20b
string escapeUri = Uri.EscapeUriString(userProvidedUri);
// http://foo.bar/a%20b That said, I am having trouble finding a uri example where using |
That's exactly the point.
If you need to escape a whole URI, what kind of escaping you need to do depends on what are you escaping for. I suspect (and it might have been mentioned in the original post), that the whole URI might had a need to be escaped for certain browsers which did not support spaces or, for example, instant messengers, which would confuse a space in plain text as a URI's end. But this escaping is highly specific to a concrete browser/IM/other software use case. And What's a real problem though, is that its presence makes it confusing to pick from https://docs.microsoft.com/en-us/dotnet/api/system.uri.escapeuristring It even has this line "Use the EscapeUriString method to prepare an unescaped URI string to be a parameter to the Uri constructor." And with your example it is clear, that it is outdated, as |
To further illustrate the problem, just look at this: https://github.com/search?q=EscapeUriString&type=Code I struggled to find any code example there, in which |
Triage: We should likely make it part of Roslyn Analyzers. @MihaZupan can you please summarize our approach and recommendation. |
@karelz I think Roslyn Analyzers is insufficient, as they are rarely used. It should at very least be marked with Here's another link, that might convince you: https://github.com/search?q=org%3Amicrosoft+EscapeUriString&type=Code I see one in Microsoft/Docker.DotNet |
@lostmsu we have plans to add default Roslyn Analyzers. The rule would be default if 99% cases are incorrectly used. If it is more than that, we may have to make the rule off by default. |
@aik-jahoda please add the Obsolete attribute, @MihaZupan will help with writing the right docs later. |
Obsoletion statustracking comment to see status of all necessary steps |
Moving to 6.0 -- we won't backport it to 5.0 this late in the game. |
Just found a corner case where it differs: Console.WriteLine(new Uri("http://my/%20").AbsoluteUri); // Output: http://my/%20
Console.WriteLine(new Uri(Uri.EscapeUriString("http://my/%20")).AbsoluteUri); // Output: http://my/%2520
Console.WriteLine(new Uri("http://my/%23").AbsoluteUri); // Output: http://my/%23
Console.WriteLine(new Uri(Uri.EscapeUriString("http://my/%23")).AbsoluteUri); // Output: http://my/%2523 |
@karelz I'm moving this to 7.0 (along with some other obsoletions). Feel free to pull this back into 6.0 if you were still planning to do this for RC1. |
@jeffhandley given that this is missing just docs, I think it makes sense to have it in 6.0. I expect that docs will happen post RC1 fork ... |
Ah; cool. I overlooked that aspect. Thanks. |
The documentation effort seems to be complete here, I linked the relevant PRs where changes happened in #31387 (comment). Closing.
The output may differ, but the |
Please, see the relevant StackOveflow thread.
From my understanding of the issue, there is no valid and/or safe use case for
EscapeUriString
. Either one wants to escape a URI component, in which case they should callEscapeDataString
, or they construct URI incorrectly in the first place.The text was updated successfully, but these errors were encountered: