Use OrdinalIgnoreCase for all Source and Package source mapping implementation #4264

erdembayar · 2021-09-14T18:49:15Z

Bug

Fixes: NuGet/Home#11182
Fixes: NuGet/Home#11241

Regression? Last working version:

Description

@kartheekp-ms noticed we have inconsistent use of string comparison approach, after discussion we decided to use OrdinalIgnoreCase only for "Source and package source mapping", after my current changes I don't see any breaking or big difference after this change.

nuget.config used for my manual testing:

<?xml version="1.0" encoding="utf-8"?>
<configuration>
  <packageSources>
    <clear />
    <add key="encyclopaedia" value="https://api.nuget.org/v3/index.json" />
    <add key="Encyclopaedia"  value="C:\Users\xxxx\1111" />
    <add key="encyclopædia" value="C:\Users\xxxx\2222" />
    <add key="Encyclopædia" value="https://pkgs.dev.azure.com/azure-public/vside/_packaging/vssdk/nuget/v3/index.json" />
  </packageSources>
</configuration>

I didn't do changes in following places due to breaking/unwanted change:

In Apex tests I moved create nuget.config part before solution create action.

PR Checklist

…ource.

erdembayar · 2021-09-14T18:51:44Z

src/NuGet.Clients/NuGet.PackageManagement.UI/Xamls/PackageManagerControl.xaml.cs

@@ -754,7 +754,7 @@ private ValueTask PopulateSourceRepoListAsync(IReadOnlyCollection<PackageSourceM
            {
                SelectedSource = PackageSources
                    // if the old active source still exists. Keep it as the active source.
-                    .FirstOrDefault(i => StringComparer.CurrentCultureIgnoreCase.Equals(i.SourceName, selectedSourceName))


This one decides current active source.

erdembayar · 2021-09-14T22:24:32Z

src/NuGet.Clients/NuGetConsole.Host.PowerShell/PowerShellHost.cs

@@ -810,7 +810,7 @@ public string[] GetAvailableProjects()
                    safeAndDisplayName.Add(displayName);
                }
                // Sort with respect to the DisplayName
-                var sortedDisplayNames = safeAndDisplayName.OrderBy(i => i.Item2, StringComparer.CurrentCultureIgnoreCase).ToArray();


I'm reverting this one, because it's working project names, not related to source.

erdembayar · 2021-09-14T22:25:25Z

src/NuGet.Clients/NuGetConsole.Host.PowerShell/PowerShellHost.cs

@@ -708,7 +708,7 @@ private void UpdateActiveSource(string activePackageSource)
            else
            {
                var s = _packageSources.FirstOrDefault(
-                    p => StringComparer.CurrentCultureIgnoreCase.Equals(p, activePackageSource));


This one let user to choose between encyclopaedia and encyclopædia sources.
UI rendering not working correctly so created follow up issue NuGet/Home#11235

erdembayar · 2021-09-14T23:54:02Z

src/NuGet.Core/NuGet.Configuration/PackageSource/PackageSource.cs

@@ -145,7 +145,7 @@ public bool Equals(PackageSource other)
                return false;
            }

-            return Name.Equals(other.Name, StringComparison.CurrentCultureIgnoreCase) &&


After this change search is done on both encyclopaedia and encyclopædia sources as separate sources.

Were they treated the same before this change?

It looks it was not, so I'll revert changes here.

Reverting because it's already doing Source compare with StringComparison.OrdinalIgnoreCase. Here actually Source is more important than Name.

erdembayar · 2021-09-15T01:05:04Z

src/NuGet.Core/NuGet.PackageManagement/Utility/UriHelper.cs

@@ -107,7 +107,7 @@ public static bool IsHttpSource(string source, PackageSourceProvider packageSour
                }

                var packageSource = packageSourceProvider.LoadPackageSources()
-                    .FirstOrDefault(p => p.Name.Equals(source, StringComparison.CurrentCultureIgnoreCase));


This IsHttpSource method not used anywhere in our code, maybe we can remove it later.

erdembayar · 2021-09-15T02:53:00Z

src/NuGet.Core/NuGet.Configuration/Settings/ConfigurationDefaults.cs

@@ -64,7 +64,7 @@ public IEnumerable<PackageSource> DefaultPackageSources
                        // In a SettingValue representing a package source, the Key represents the name of the package source and the Value its source
                        _defaultPackageSources.Add(new PackageSource(source.GetValueAsPath(),
                            source.Key,
-                            isEnabled: !disabledPackageSources.Any(p => p.Key.Equals(source.Key, StringComparison.CurrentCultureIgnoreCase)),


It's about adding default sources from NuGetDefaults.config which exist on C:\Program Files (x86)\NuGet path on Windows. https://docs.microsoft.com/en-us/nuget/consume-packages/configuring-nuget-behavior
Looks not popular feature, not sure how relevant now days.
Maybe we deprecate this one in the future?

erdembayar · 2021-09-15T16:58:09Z

src/NuGet.Core/NuGet.Configuration/PackageSource/PackageSourceProvider.cs

@@ -158,8 +158,8 @@ private static void AddDefaultPackageSources(List<PackageSource> loadedPackageSo

            foreach (var packageSource in defaultPackageSources)
            {
-                var sourceMatching = loadedPackageSources.Any(p => p.Source.Equals(packageSource.Source, StringComparison.CurrentCultureIgnoreCase));


Related to https://github.com/NuGet/NuGet.Client/pull/4264/files#r708797119 on PMUI.
Looks not really relevant anymore.

erdembayar · 2021-09-15T17:18:17Z

src/NuGet.Core/NuGet.Configuration/PackageSource/PackageSourceProvider.cs

@@ -302,8 +302,8 @@ private PackageSource GetPackageSource(string key, Dictionary<string, IndexedPac

                foreach (var packageSource in _configurationDefaultSources)
                {
-                    var isSourceMatch = loadedPackageSources.Any(p => p.Source.Equals(packageSource.Source, StringComparison.CurrentCultureIgnoreCase));


Related to https://github.com/NuGet/NuGet.Client/pull/4264/files#r708797119 for nuget.exe.
Triggered for nuget sources Add -Name "MyServer" -Source \\myserver\packages (https://docs.microsoft.com/en-us/nuget/reference/cli-reference/cli-ref-sources).
Looks not really relevant anymore.

erdembayar · 2021-09-15T18:43:40Z

...NuGet.Clients/NuGet.VisualStudio.Internal.Contracts/ContextInfos/PackageSourceContextInfo.cs

@@ -48,7 +48,7 @@ public bool Equals(PackageSourceContextInfo other)
                return false;
            }

-            return Name.Equals(other.Name, StringComparison.CurrentCultureIgnoreCase) &&


After this change search both encyclopaedia and encyclopædia sources as separate sources.

This one also looks irrelevant change, reverting because it's already doing Source compare with StringComparison.OrdinalIgnoreCase. Here actually Source is more important than Name.

erdembayar · 2021-09-15T23:34:18Z

src/NuGet.Clients/NuGet.PackageManagement.UI/Options/PackageSourcesOptionsControl.cs

@@ -416,7 +416,7 @@ private TryUpdateSourceResults TryUpdateSource()
                return TryUpdateSourceResults.NotUpdated;
            }

-            if (selectedPackageSource.Name.Equals(name, StringComparison.CurrentCultureIgnoreCase) && selectedPackageSource.Source.Equals(source, StringComparison.OrdinalIgnoreCase))


I decided not to make this change in this file, revert the changes. It brings some unwanted side effects, if update existing source encyclopaedia with Encyclopædia on PMUI then save it creates duplicate entrees in nuget.config file.
Problem is when saving it check existing using packagesource key but due existing key entry following code it assumes above 2 are different hence creates another entry instead of updating it. Related to bdb3d6f#r709649975

erdembayar · 2021-09-15T23:34:24Z

src/NuGet.Clients/NuGet.PackageManagement.UI/Options/PackageSourcesOptionsControl.cs

@@ -425,7 +425,7 @@ private TryUpdateSourceResults TryUpdateSource()

            // check to see if name has already been added
            // also make sure it's not the same as the aggregate source ('All')
-            bool hasName = sourcesList.Any(ps => ps != selectedPackageSource && string.Equals(name, ps.Name, StringComparison.CurrentCultureIgnoreCase));


I decided not to make this change in this file, revert the changes. It brings some unwanted side effects, if update existing source encyclopaedia with Encyclopædia on PMUI then save it creates duplicate entrees in nuget.config file.
Problem is when saving it check existing using packagesource key but due existing key entry following code it assumes above 2 are different hence creates another entry instead of updating it. Related to bdb3d6f#r709649975

erdembayar · 2021-09-15T23:44:04Z

src/NuGet.Core/NuGet.Configuration/Settings/Items/SourceItem.cs

@@ -77,8 +77,8 @@ public override bool Equals(object other)
                return true;
            }

-            return string.Equals(Key, source.Key, StringComparison.Ordinal) &&


I decided to revert this change. For below input:

<packageSources> <clear /> <add key="encyclopaedia" value="C:\Users\eryondon\Desktop\NamespaceTestPackages\PublicRepository" /> <add key="Encyclopaedia" value="https://api.nuget.org/v3/index.json" /> </packageSources>

Before this change:

After this change:

Please note source url is quite different and looks breaking change experience for end user.

This reverts commit bdb3d6f.

erdembayar · 2021-09-20T16:23:18Z

test/NuGet.Tests.Apex/NuGet.Tests.Apex/NuGetEndToEndTests/NuGetUITestCase.cs


            // Create nuget.config with Package namespace filtering rules before project is created.
            CommonUtility.CreateConfigurationFile(Path.Combine(solutionDirectory, "NuGet.config"), $@"<?xml version=""1.0"" encoding=""utf-8""?>
 <configuration>
    <packageSources>
    <!--To inherit the global NuGet package sources remove the <clear/> line below -->
    <clear />
-    <add key=""ExternalRepository"" value=""{externalRepositoryPath}"" />
-    <add key=""PrivateRepository"" value=""{privateRepositoryPath}"" />
+    <add key=""encyclopaedia"" value=""{externalRepositoryPath}"" />


Test "encyclopaedia" and "encyclopædia" sources used together doesn't crash VS PMUI setting.

erdembayar · 2021-09-28T00:04:36Z

@NuGet/nuget-client
Please review.

nkolev92

The fact that there's so many culture comparison gives me a pause about the impact this change could have.

nkolev92 · 2021-09-28T01:16:11Z

src/NuGet.Core/NuGet.Configuration/PackageSource/PackageSource.cs

@@ -145,7 +145,7 @@ public bool Equals(PackageSource other)
                return false;
            }

-            return Name.Equals(other.Name, StringComparison.CurrentCultureIgnoreCase) &&


Were they treated the same before this change?

erdembayar · 2021-09-29T22:03:52Z

src/NuGet.Clients/NuGet.PackageManagement.UI/ViewModels/LoadingStatusViewModel.cs

@@ -22,7 +23,7 @@ public void UpdateModel(IItemLoaderState loaderState)
            PackageSearchStatus = Convert(loaderState.LoadingStatus);
            ItemsFound = loaderState.ItemsCount;

-            var convertedList = new System.Collections.SortedList();


Make VS stop crashing NuGet/Home#11241

Replace CurrentCultureIgnoreCase with OrdinalIgnoreCase for package s…

4389cca

…ource.

erdembayar commented Sep 14, 2021

View reviewed changes

erdembayar commented Sep 15, 2021

View reviewed changes

erdembayar added 3 commits September 15, 2021 11:48

Revert unwanted changes.

527758d

Add unit tests.

92bfe16

Add back

bdb3d6f

erdembayar commented Sep 15, 2021

View reviewed changes

Revert "Add back"

50e2e87

This reverts commit bdb3d6f.

erdembayar force-pushed the dev-eryondon-11182 branch from 063a4f4 to 50e2e87 Compare September 16, 2021 00:07

erdembayar marked this pull request as ready for review September 16, 2021 00:20

erdembayar requested a review from a team as a code owner September 16, 2021 00:20

jeffkl approved these changes Sep 16, 2021

View reviewed changes

kartheekp-ms mentioned this pull request Sep 17, 2021

[Bug]: VS crashes when package sources dropdown in PM UI has multiple package sources whose names are equal when compared using Culture Ignore Case NuGet/Home#11241

Closed

erdembayar added 2 commits September 20, 2021 09:18

Move nuget.config before solution creation.

ca27685

Address VS crash with encyclopaedia and encyclopædia sources.

6803b0e

erdembayar force-pushed the dev-eryondon-11182 branch from e974543 to 6803b0e Compare September 20, 2021 16:19

erdembayar commented Sep 20, 2021

View reviewed changes

erdembayar requested review from kartheekp-ms and nkolev92 September 20, 2021 20:58

nkolev92 reviewed Sep 28, 2021

View reviewed changes

erdembayar added 2 commits September 27, 2021 19:32

Revert unneeded change

b251a2c

2

c5e441d

nkolev92 approved these changes Sep 28, 2021

View reviewed changes

erdembayar commented Sep 29, 2021

View reviewed changes

Merge branch 'dev' into dev-eryondon-11182

29583e2

erdembayar force-pushed the dev-eryondon-11182 branch 2 times, most recently from 1ab4aed to 29583e2 Compare October 1, 2021 02:12

Merge branch 'dev' into dev-eryondon-11182

a9225cc

erdembayar merged commit 6cf7a6e into dev Oct 1, 2021

erdembayar deleted the dev-eryondon-11182 branch October 1, 2021 17:30

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use OrdinalIgnoreCase for all Source and Package source mapping implementation #4264

Use OrdinalIgnoreCase for all Source and Package source mapping implementation #4264

erdembayar commented Sep 14, 2021 •

edited

Loading

erdembayar Sep 14, 2021 •

edited

Loading

erdembayar Sep 14, 2021

erdembayar Sep 14, 2021 •

edited

Loading

erdembayar Sep 14, 2021 •

edited

Loading

nkolev92 Sep 28, 2021

erdembayar Sep 28, 2021

erdembayar Sep 28, 2021

erdembayar Sep 15, 2021 •

edited

Loading

erdembayar Sep 15, 2021 •

edited

Loading

erdembayar Sep 15, 2021 •

edited

Loading

erdembayar Sep 15, 2021 •

edited

Loading

erdembayar Sep 15, 2021 •

edited

Loading

erdembayar Sep 28, 2021

erdembayar Sep 15, 2021 •

edited

Loading

erdembayar Sep 15, 2021 •

edited

Loading

erdembayar Sep 15, 2021 •

edited

Loading

erdembayar Sep 20, 2021

erdembayar commented Sep 28, 2021

nkolev92 left a comment

nkolev92 Sep 28, 2021

erdembayar Sep 29, 2021

Use OrdinalIgnoreCase for all Source and Package source mapping implementation #4264

Use OrdinalIgnoreCase for all Source and Package source mapping implementation #4264

Conversation

erdembayar commented Sep 14, 2021 • edited Loading

Bug

Description

PR Checklist

erdembayar Sep 14, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

erdembayar Sep 14, 2021 • edited Loading

Choose a reason for hiding this comment

erdembayar Sep 14, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

erdembayar Sep 15, 2021 • edited Loading

Choose a reason for hiding this comment

erdembayar Sep 15, 2021 • edited Loading

Choose a reason for hiding this comment

erdembayar Sep 15, 2021 • edited Loading

Choose a reason for hiding this comment

erdembayar Sep 15, 2021 • edited Loading

Choose a reason for hiding this comment

erdembayar Sep 15, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

erdembayar Sep 15, 2021 • edited Loading

Choose a reason for hiding this comment

erdembayar Sep 15, 2021 • edited Loading

Choose a reason for hiding this comment

erdembayar Sep 15, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

erdembayar commented Sep 28, 2021

nkolev92 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

erdembayar commented Sep 14, 2021 •

edited

Loading

erdembayar Sep 14, 2021 •

edited

Loading

erdembayar Sep 14, 2021 •

edited

Loading

erdembayar Sep 14, 2021 •

edited

Loading

erdembayar Sep 15, 2021 •

edited

Loading

erdembayar Sep 15, 2021 •

edited

Loading

erdembayar Sep 15, 2021 •

edited

Loading

erdembayar Sep 15, 2021 •

edited

Loading

erdembayar Sep 15, 2021 •

edited

Loading

erdembayar Sep 15, 2021 •

edited

Loading

erdembayar Sep 15, 2021 •

edited

Loading

erdembayar Sep 15, 2021 •

edited

Loading