Skip to content

Enables code analysis rules about culture sensitive string operations #26656

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 5 commits into from
Oct 15, 2020
Merged

Enables code analysis rules about culture sensitive string operations #26656

merged 5 commits into from
Oct 15, 2020

Conversation

@pranavkm
Copy link
Contributor

@pranavkm pranavkm commented Oct 7, 2020 •
edited
Loading

  • Use StringComparison in StartsWith, EndsWith, Compare operations
  • Use CultureInfo.InvariantCulture when parsing and formatting non-culture sensitive strings, and in tests.
  • Use CultureInfo.CurrentCulture when printing user-visible error messages \ logs

@pranavkm pranavkm force-pushed the prkrishn/string-stuff branch 2 times, most recently from 4bfea38 to 176aeb2 Compare October 7, 2020 16:05
@Pilchie Pilchie added area-blazor Includes: Blazor, Razor Components area-commandlinetools Includes: Command line tools, dotnet-dev-certs, dotnet-user-jwts, and OpenAPI area-dataprotection Includes: DataProtection area-mvc Includes: MVC, Actions and Controllers, Localization, CORS, most templates area-auth Includes: Authn, Authz, OAuth, OIDC, Bearer area-servers area-signalr Includes: SignalR clients and servers labels Oct 7, 2020
@pranavkm pranavkm force-pushed the prkrishn/string-stuff branch from 176aeb2 to 80f22df Compare October 12, 2020 16:21
@pranavkm pranavkm force-pushed the prkrishn/string-stuff branch from 80f22df to c5d9ea5 Compare October 12, 2020 16:27
@pranavkm pranavkm added this to the 6.0.0-alpha1 milestone Oct 12, 2020
@pranavkm pranavkm changed the title Enable string comparison FxCop rules Enables code analysis rules about culture sensitive string operations Oct 12, 2020
@pranavkm pranavkm marked this pull request as ready for review October 12, 2020 17:01
@pranavkm pranavkm requested a review from a team October 12, 2020 17:01
@SteveSandersonMS
Copy link
Member

SteveSandersonMS commented Oct 13, 2020

@pranavkm Do you have a suggestion about how this PR should be reviewed? It's probably too big for any one person to read and reason about the correctness of each change here. Should each affected product area team be responsible for reviewing their own files?

@pranavkm
Copy link
Contributor Author

pranavkm commented Oct 13, 2020

@SteveSandersonMS a lot of the file changes are in our tests which require less scrutiny. Product changes (filtering by files that have /src/ in their path) is a much smaller subset and it's likely just about anybody could review it.

jkotalik
jkotalik reviewed Oct 14, 2020
View reviewed changes
jkotalik
jkotalik reviewed Oct 14, 2020
View reviewed changes
jkotalik
jkotalik reviewed Oct 14, 2020
View reviewed changes
jkotalik
jkotalik reviewed Oct 14, 2020
View reviewed changes
jkotalik
jkotalik reviewed Oct 14, 2020
View reviewed changes
src/SignalR/clients/csharp/Http.Connections.Client/src/HttpConnection.cs
@@ -440,7 +440,7 @@ private async Task<NegotiationResponse> NegotiateAsync(Uri url, HttpClient httpC
// Get a connection ID from the server
Log.EstablishingConnection(logger, url);
var urlBuilder = new UriBuilder(url);
if (!urlBuilder.Path.EndsWith("/"))
if (!urlBuilder.Path.EndsWith("/", StringComparison.Ordinal))
Copy link
Contributor

@jkotalik jkotalik Oct 14, 2020

Choose a reason for hiding this comment

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

Can this be changed to a character instead of string?

Copy link
Contributor Author

@pranavkm pranavkm Oct 15, 2020

Choose a reason for hiding this comment

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

This one cross-compiles to ns2.0 which does not have the char overload.

jkotalik
jkotalik approved these changes Oct 14, 2020
View reviewed changes
Copy link
Contributor

@jkotalik jkotalik left a comment •
edited
Loading

Choose a reason for hiding this comment

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

u owe me BIG time 😄 .

Few questions about CurrentCulture vs InvariantCulture but besides that LGTM.

@pranavkm pranavkm merged commit 8a81194 into master Oct 15, 2020
@pranavkm pranavkm deleted the prkrishn/string-stuff branch October 15, 2020 16:14
@Pilchie Pilchie mentioned this pull request Oct 15, 2020
Closed
@dahlbyk dahlbyk mentioned this pull request Oct 20, 2020
Merged
@amcasey amcasey added area-hosting Includes Hosting area-middleware Includes: URL rewrite, redirect, response cache/compression, session, and other general middlewares and removed area-runtime labels Jun 1, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@BrennanConroy BrennanConroy Awaiting requested review from BrennanConroy

@halter73 halter73 Awaiting requested review from halter73

@javiercn javiercn Awaiting requested review from javiercn

@SteveSandersonMS SteveSandersonMS Awaiting requested review from SteveSandersonMS

@Tratcher Tratcher Awaiting requested review from Tratcher

1 more reviewer

@jkotalik jkotalik jkotalik approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

area-auth Includes: Authn, Authz, OAuth, OIDC, Bearer area-blazor Includes: Blazor, Razor Components area-commandlinetools Includes: Command line tools, dotnet-dev-certs, dotnet-user-jwts, and OpenAPI area-dataprotection Includes: DataProtection area-hosting Includes Hosting area-middleware Includes: URL rewrite, redirect, response cache/compression, session, and other general middlewares area-mvc Includes: MVC, Actions and Controllers, Localization, CORS, most templates area-signalr Includes: SignalR clients and servers

Projects

None yet

Milestone

6.0-preview1

Development

Successfully merging this pull request may close these issues.

6 participants

@pranavkm @SteveSandersonMS @jkotalik @Pilchie @amcasey