dotnet / SqlClient

Microsoft.Data.SqlClient provides database connectivity to SQL Server for .NET applications.
MIT License
857 stars 287 forks source link

Consider error diagnostic when SqlClient used when invariant globalization enabled. #2043

Open mitchdenny opened 1 year ago

mitchdenny commented 1 year ago

Is your feature request related to a problem? Please describe.

In the ASP.NET repo we have updated our API, Worker, and gRPC templates to use invariant globalization. Initially we enabled invariant globalization for those project templates regardless of whether the publishing target was native AOT or not but received feedback that this created friction for SQL client users. As a result we are opting out of invariant globalization non-native-AOT usage.

For native AOT users (where invariant globalization is the default) we believe that there is an opportunity for the SQL client team to raise a diagnostic alert to let them know that they should disable invariant globalization if their project happens to have it enabled.

Describe the solution you'd like

A extend/create Roslyn analyzer which warns if the project in which SqlClient is invoked from has Invariant Globalization enabled that a diagnostic error is raised. This would shift detection of this issue left for the developer instead of crashing at runtime.

Describe alternatives you've considered

We considered disabling invariant globalization for AOT scenarios, but for API scenarios we believe invariant globalization is a better initial choice. Given this it doesn't really make sense for us to have an analyzer for AOT usage in the ASP.NET repo, epecially since SqlClient's lack of support for invariant globalization isn't an ASP.NET specific issue (we understand that there are good reasons for SqlClient to depend on globalization support).

Additional context

Here is the original PR where we enabled invariant globalization: https://github.com/dotnet/aspnetcore/pull/47066 Here is a customer issue reporting the usability problem to us: https://github.com/dotnet/aspnetcore/issues/48186 Here is out PR opting out of invariant globalization for non-AOT scenarios: https://github.com/dotnet/aspnetcore/pull/48238

JRahnama commented 1 year ago

@mitchdenny thanks for the suggestion. We will discuss this internally and will update you later.

mungojam commented 11 months ago

Will SqlClient ever support invariant globalization? The new AWS lambda runtime doesn't have icu, so needs invariant globalization and this then means SqlClient can't be used there, without jumping through some hoops to bundle it with the deployed app

David-Engel commented 11 months ago

Will SqlClient ever support invariant globalization? The new AWS lambda runtime doesn't have icu, so needs invariant globalization and this then means SqlClient can't be used there

Unlikely. It would take a lot of effort and testing to ensure every part of the code behaves correctly in globalization invariant mode when processing all different types of data. Currently, I don't see that rising in priority over other things any time soon.

This issue (the analyzer check), however, seems relatively easy. We are going to look into it and see what it would entail.

H-Yeo commented 10 months ago

Also relates to: this topic #1913