Globalization invariant mode: Require explicit LCID when constructing SqlMetaData

[edwardneal]

Description

SqlClient doesn't currently support globalization invariant mode, and we throw an exception whenever someone tries to use it in the main SqlConnection entry point. This PR closes a similar entry point when instantiating the SqlMetaData type.

I've explicitly been specific when closing entry points, in order to leave the door open to future support for this mode. The main reason why we can't support it was the requirement for LCID/codepage mapping, but this was removed as a secondary effect of #4212.

The result of this PR is that if globalization invariant mode is enabled, only four SqlMetaData constructors are permitted for columns of type Text / NText / Varchar / NVarchar / Char / NChar:

• SqlMetaData(string name, SqlDbType dbType, long maxLength, byte precision, byte scale, long locale, SqlCompareOptions compareOptions, Type userDefinedType)
• SqlMetaData(string name, SqlDbType dbType, long maxLength, byte precision, byte scale, long localeId, SqlCompareOptions compareOptions, Type userDefinedType, bool useServerDefault, bool isUniqueKey, SortOrder columnSortOrder, int sortOrdinal)
• SqlMetaData(string name, SqlDbType dbType, long maxLength, long locale, SqlCompareOptions compareOptions, bool useServerDefault, bool isUniqueKey, SortOrder columnSortOrder, int sortOrdinal)
• SqlMetaData(string name, SqlDbType dbType, long maxLength, long locale, SqlCompareOptions compareOptions)

These are permitted because they force the user to specify an LCID in the locale or the localeId parameter.

Issues

Stands alone, but also lays the groundwork for #3742.

Testing

Unit tests have been added alongside the existing ones (which are, in turn, in the wrong assembly - to be cleaned up in a follow-up PR.)

[azure-pipelines]

Azure Pipelines:
There may be pipelines that require an authorized user to comment /azp run to run.

[paulmedynski]

/azp run

[azure-pipelines]

Azure Pipelines:
Successfully started running 3 pipeline(s).

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 64.90%. Comparing base (fdebcd2) to head (3942f7b).
⚠️ Report is 22 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #4457      +/-   ##
==========================================
- Coverage   65.83%   64.90%   -0.93%     
==========================================
  Files         287      287              
  Lines       43763    66970   +23207     
==========================================
+ Hits        28812    43469   +14657     
- Misses      14951    23501    +8550     

Flag	Coverage Δ
CI-SqlClient	?
PR-SqlClient-Project	64.90% <100.00%> (?)

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[paulmedynski]

I think this looks fine. I'll defer to @cheenamalhotra and @mdaigle to remove the Approval Needed label.

[mdaigle]

GlobalizationInvariantMode isn't supported yet, so setting it already throws an exception at connection open. So this shouldn't be considered a breaking API change.

At the same time, now that you've removed the technical roadblocks 🥳 , it would be helpful to see a proposed end state for GlobalizationInvariantMode support. What will the final state of the public API be? How will customers enable and interact with this mode e2e?

The reason I ask is that the metadata class you're touching here has 15 constructors (!!) and already seems unwieldy to deal with. Understanding how the changes here fit into a larger user experience will guide my review.

[edwardneal]

Thanks @mdaigle. I'm working to a few rules of thumb:

1. Enabling globalization invariant mode shouldn't require any code changes in 95% of client code.
2. In the remaining 5% of cases, the reasons should be intuitive. In this case, someone is setting up a column schema and specifying a varchar column without also specifying a locale.
3. Globalization invariant mode should be considered a strict subset of the normal operating mode - if the ICU libraries aren't enabled, a feature should be switched off completely rather than operate in a different way.
4. When the library isn't working with user data, its internal operations should be operating using the invariant culture anyway.

This should hopefully mean that whichever release eventually enables support for globalization invariant mode can present some simple guidelines in the documentation:

• If building the structure of the C# equivalent of a user-defined table type via SqlMetaData, the collation of varchar columns will no longer be inherited from the current culture. Specify this using the constructors which request a locale or a localeId parameter.
• When mapping column names to ordinals, SqlDataReader will use the invariant culture's case sensitivity rules, not that of the database locale. Exactly the same principle applies when SqlParameterCollection is mapping a parameter name to an ordinal.
• LocalDB error messages will be presented using the invariant culture.

The current tests will provide code coverage of the exception in the current environment. In the long term, prior to public enablement, I'd like to take advantage of the fact that GIM will be a strict subset of the normal operating mode: all unit/functional/manual tests would be run in globalization invariant mode by default, with a much smaller number of tests which are guaranteed to require locale awareness run in the current operating mode.

[cheenamalhotra]

The changes look good to me for bringing in globalization-invariant mode support.

However, my only concern is whether it could break an existing customer?

If yes, I would say this should be included in the 8.0.0 milestone instead, which kicks off in September after 7.1 GA release, not too far.

[edwardneal]

It's been noted in the past that SqlClient doesn't support globalization invariant mode, so my initial view is that any customers using it in this situation are already in unsupported territory.

With that said, throwing a NotSupportedException would break a customer who decided to use SqlMetaData and SqlDataRecord as an in-memory collection of rows which are never used as a SqlParameter value. I could remove that break by throwing an InvalidOperationException instead of a NotSupportedException - it just wouldn't be quite as semantically clear. SqlConnection.Open throws a NotSupportedException when globalization invariant mode is enabled, but that code path will be removed in the fullness of time so I don't think we'd want to treat that as precedent.

We could decide that customers impacted are already using SqlClient in an unsupported manner and merge as-is; I could change the exception type and avoid the break at the slight expense of clarity; or we could put the PR on hold until September and take the break as part of v8. I'm slightly inclined towards the second option, but this isn't central to any 7.1 functionality - I'd be happy to wait.