Skip to content
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

[release/6.0] Fix calling NLS with Cultures has alternative sort names #62155

Merged
merged 4 commits into from
Dec 15, 2021

Conversation

github-actions[bot]
Copy link
Contributor

@github-actions github-actions bot commented Nov 29, 2021

Fixes #61977
Backport of #61992 to release/6.0

/cc @tarekgh

Customer Impact

This issue is a regression from .NET 5.0 when getting the supported codepages for cultures. This regression is causing failures in the SQL clients which upgraded to .NET 6.0. Here are some links demonstrating such failures:
dotnet/SqlClient#1319 (comment)
dotnet/SqlClient#1396 (comment)
dotnet/efcore#26812 (comment)

The regression happens only with a small set of cultures having alternative sorts (more details about that are below) and only happens on Windows.

Details:
In .NET we depend on the ICU library for globalization. There are some cultural properties which are not supported by ICU. Example of that culture's ANSI codepage. For such properties, we used to hardcode the values in .NET 5.0. In .NET 6.0 when running on Windows OS, we started to call Windows to get a better-updated value.
Usually, cultures support one collation behavior. Some cultures support more than one collation behavior though. For example, the Spanish culture es-ES supports alternative traditional collation es-ES_tradnl. Internally we have two names for the cultures. These two names will have the same value in the usual cases. But the names will differ only with the cultures that have alternative sorts. One name is the normal culture name like es-ES_tradnl. The second name which we'll use when calling ICU like es-ES@collation=tradnl.
The bug we have in .NET is when calling Windows to get the properties like the ANSI codepage, we mistakenly used the name we use to call ICU instead of using the normal culture name. That causes the call to fail, and we return a zero value for the codepage. The problem happens only with the cultures which have alternative collation. Here is the list of such cultures:

    de-DE_phoneb
    es-ES_tradnl
    hu-HU_technl
    ka-GE_modern
    ja-JP_radstr
    x-iv_mathan
    zh-CN_phoneb
    zh-CN_stroke
    zh-HK_radstr
    zh-MO_radstr
    zh-MO_stroke
    zh-SG_phoneb
    zh-SG_stroke
    zh-TW_pronun
    zh-TW_radstr

The fix is simple that is using the correct name when calling Windows.

Testing

The change is merged to main branch and passed all regression tests. Added a specific test to catch the reported problem. Also, manually checked all culture names to ensure the fix will work fine with all cultures.

Risk

The risk is low as the change is scoped to Windows and the change is using the correct culture names which work fine with Windows. I cannot see this change can cause any problem.

@ghost
Copy link

ghost commented Nov 29, 2021

Tagging subscribers to this area: @tarekgh, @safern
See info in area-owners.md if you want to be subscribed.

Issue Details

Backport of #61992 to release/6.0

/cc @tarekgh

Customer Impact

Testing

Risk

Author: github-actions[bot]
Assignees: -
Labels:

area-System.Globalization

Milestone: -

@tarekgh tarekgh added this to the .NET 6.0 milestone Nov 30, 2021
@tarekgh tarekgh added the Servicing-consider Issue for next servicing release review label Nov 30, 2021
@tarekgh
Copy link
Member

tarekgh commented Nov 30, 2021

CC @ericstj @danmoseley for awareness.

CC @safern as he was the one reviewed the original change.

@karelz karelz modified the milestones: .NET 6.0, 6.0.x Nov 30, 2021
@danmoseley
Copy link
Member

@tarekgh thanks for the ping, I agree this mails sense for you to submit to tactics.

@ericstj ericstj added the NO-MERGE The PR is not ready for merge yet (see discussion for detailed reasons) label Nov 30, 2021
@ericstj
Copy link
Member

ericstj commented Nov 30, 2021

Marking as NO MERGE until branch is open.

@tarekgh tarekgh added Servicing-approved Approved for servicing release and removed Servicing-consider Issue for next servicing release review labels Dec 2, 2021
@tarekgh
Copy link
Member

tarekgh commented Dec 2, 2021

Approved by @SteveMCarroll

@safern safern removed the NO-MERGE The PR is not ready for merge yet (see discussion for detailed reasons) label Dec 15, 2021
@safern safern merged commit a4f5b78 into release/6.0 Dec 15, 2021
@safern safern deleted the backport/pr-61992-to-release/6.0 branch December 15, 2021 18:41
@ghost ghost locked as resolved and limited conversation to collaborators Jan 14, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants