Fix loading of fragments that update multiple profiles (#11598)

The "updates" key is an alternative "guid" key for fragment profiles.
But SettingsLoader::_appendProfile stores and deduplicates profiles according
to their "guid" only. We need to modify the function to optionally store
profiles by their "updates" key as well, otherwise multiple fragment
profiles without "guid" might collide as they produce the same default GUID.

## PR Checklist
* [x] Closes #11597
* [x] I work here
* [ ] Tests added/passed
* [ ] Schema updated.
* [ ] I've discussed this with core contributors already. If not checked, I'm ready to accept this work might be rejected in favor of a different grand plan. Issue number where discussion took place: #xxx

## Validation Steps Performed
* Unit tests pass ✔️
* Issue #11597 doesn't reproduce anymore ✔️

src/cascadia/LocalTests_SettingsModel/DeserializationTests.cpp

@@ -7,7 +7,9 @@
 #include "../TerminalSettingsModel/CascadiaSettings.h"
 #include "JsonTestClass.h"
 #include "TestUtils.h"
+
 #include <defaults.h>
+#include <userDefaults.h>
 
 using namespace Microsoft::Console;
 using namespace WEX::Logging;
@@ -70,6 +72,7 @@ namespace SettingsModelLocalTests
         TEST_METHOD(TestCloneInheritanceTree);
         TEST_METHOD(TestValidDefaults);
         TEST_METHOD(TestInheritedCommand);
+        TEST_METHOD(LoadFragmentsWithMultipleUpdates);
 
     private:
         static winrt::com_ptr<implementation::CascadiaSettings> createSettings(const std::string_view& userJSON)
@@ -1979,4 +1982,34 @@ namespace SettingsModelLocalTests
             VERIFY_IS_NULL(actualKeyChord);
         }
     }
+
+    // This test ensures GH#11597 doesn't regress.
+    void DeserializationTests::LoadFragmentsWithMultipleUpdates()
+    {
+        static constexpr std::wstring_view fragmentSource{ L"fragment" };
+        static constexpr std::string_view fragmentJson{ R"({
+            "profiles": [
+                {
+                    "updates": "{61c54bbd-c2c6-5271-96e7-009a87ff44bf}",
+                    "cursorShape": "filledBox"
+                },
+                {
+                    "updates": "{0caa0dad-35be-5f56-a8ff-afceeeaa6101}",
+                    "cursorShape": "filledBox"
+                },
+                {
+                    "guid": "{6239a42c-0000-49a3-80bd-e8fdd045185c}",
+                    "commandline": "cmd.exe"
+                }
+            ]
+        })" };
+
+        implementation::SettingsLoader loader{ std::string_view{}, DefaultJson };
+        loader.MergeInboxIntoUserSettings();
+        loader.MergeFragmentIntoUserSettings(winrt::hstring{ fragmentSource }, fragmentJson);
+        loader.FinalizeLayering();
+
+        VERIFY_IS_FALSE(loader.duplicateProfile);
+        VERIFY_ARE_EQUAL(3u, loader.userSettings.profiles.size());
+    }
 }

src/cascadia/TerminalSettingsModel/CascadiaSettings.h

@@ -57,6 +57,7 @@ namespace winrt::Microsoft::Terminal::Settings::Model::implementation
         void ApplyRuntimeInitialSettings();
         void MergeInboxIntoUserSettings();
         void FindFragmentsAndMergeIntoUserSettings();
+        void MergeFragmentIntoUserSettings(const winrt::hstring& source, const std::string_view& content);
         void FinalizeLayering();
         bool DisableDeletedProfiles();
 
@@ -82,7 +83,7 @@ namespace winrt::Microsoft::Terminal::Settings::Model::implementation
         void _parseFragment(const winrt::hstring& source, const std::string_view& content, ParsedSettings& settings);
         static JsonSettings _parseJson(const std::string_view& content);
         static winrt::com_ptr<implementation::Profile> _parseProfile(const OriginTag origin, const winrt::hstring& source, const Json::Value& profileJson);
-        void _appendProfile(winrt::com_ptr<implementation::Profile>&& profile, ParsedSettings& settings);
+        void _appendProfile(winrt::com_ptr<Profile>&& profile, const winrt::guid& guid, ParsedSettings& settings);
         static void _addParentProfile(const winrt::com_ptr<implementation::Profile>& profile, ParsedSettings& settings);
         void _executeGenerator(const IDynamicProfileGenerator& generator);
 

src/cascadia/TerminalSettingsModel/CascadiaSettingsSerialization.cpp

@@ -207,26 +207,6 @@ void SettingsLoader::FindFragmentsAndMergeIntoUserSettings()
                 {
                     const auto content = ReadUTF8File(fragmentExt.path());
                     _parseFragment(source, content, fragmentSettings);
-
-                    for (const auto& fragmentProfile : fragmentSettings.profiles)
-                    {
-                        if (const auto updates = fragmentProfile->Updates(); updates != winrt::guid{})
-                        {
-                            if (const auto it = userSettings.profilesByGuid.find(updates); it != userSettings.profilesByGuid.end())
-                            {
-                                it->second->InsertParent(0, fragmentProfile);
-                            }
-                        }
-                        else
-                        {
-                            _addParentProfile(fragmentProfile, userSettings);
-                        }
-                    }
-
-                    for (const auto& kv : fragmentSettings.globals->ColorSchemes())
-                    {
-                        userSettings.globals->AddColorScheme(kv.Value());
-                    }
                 }
                 CATCH_LOG();
             }
@@ -289,6 +269,15 @@ void SettingsLoader::FindFragmentsAndMergeIntoUserSettings()
     }
 }
 
+// See FindFragmentsAndMergeIntoUserSettings.
+// This function does the same, but for a single given JSON blob and source
+// and at the time of writing is used for unit tests only.
+void SettingsLoader::MergeFragmentIntoUserSettings(const winrt::hstring& source, const std::string_view& content)
+{
+    ParsedSettings fragmentSettings;
+    _parseFragment(source, content, fragmentSettings);
+}
+
 // Call this method before passing SettingsLoader to the CascadiaSettings constructor.
 // It layers all remaining objects onto each other (those that aren't covered
 // by MergeInboxIntoUserSettings/FindFragmentsAndMergeIntoUserSettings).
@@ -475,7 +464,7 @@ void SettingsLoader::_parse(const OriginTag origin, const winrt::hstring& source
             // GH#9962: Discard Guid-less, Name-less profiles.
             if (profile->HasGuid())
             {
-                _appendProfile(std::move(profile), settings);
+                _appendProfile(std::move(profile), profile->Guid(), settings);
             }
         }
     }
@@ -517,14 +506,37 @@ void SettingsLoader::_parseFragment(const winrt::hstring& source, const std::str
                 auto profile = _parseProfile(OriginTag::Fragment, source, profileJson);
                 // GH#9962: Discard Guid-less, Name-less profiles, but...
                 // allow ones with an Updates field, as those are special for fragments.
-                if (profile->HasGuid() || profile->Updates() != winrt::guid{})
+                // We need to make sure to only call Guid() if HasGuid() is true,
+                // as Guid() will dynamically generate a return value otherwise.
+                const auto guid = profile->HasGuid() ? profile->Guid() : profile->Updates();
+                if (guid != winrt::guid{})
                 {
-                    _appendProfile(std::move(profile), settings);
+                    _appendProfile(std::move(profile), guid, settings);
                 }
             }
             CATCH_LOG()
         }
     }
+
+    for (const auto& fragmentProfile : settings.profiles)
+    {
+        if (const auto updates = fragmentProfile->Updates(); updates != winrt::guid{})
+        {
+            if (const auto it = userSettings.profilesByGuid.find(updates); it != userSettings.profilesByGuid.end())
+            {
+                it->second->InsertParent(0, fragmentProfile);
+            }
+        }
+        else
+        {
+            _addParentProfile(fragmentProfile, userSettings);
+        }
+    }
+
+    for (const auto& kv : settings.globals->ColorSchemes())
+    {
+        userSettings.globals->AddColorScheme(kv.Value());
+    }
 }
 
 SettingsLoader::JsonSettings SettingsLoader::_parseJson(const std::string_view& content)
@@ -564,11 +576,11 @@ winrt::com_ptr<Profile> SettingsLoader::_parseProfile(const OriginTag origin, co
 
 // Adds a profile to the ParsedSettings instance. Takes ownership of the profile.
 // It ensures no duplicate GUIDs are added to the ParsedSettings instance.
-void SettingsLoader::_appendProfile(winrt::com_ptr<Profile>&& profile, ParsedSettings& settings)
+void SettingsLoader::_appendProfile(winrt::com_ptr<Profile>&& profile, const winrt::guid& guid, ParsedSettings& settings)
 {
     // FYI: The static_cast ensures we don't move the profile into
     // `profilesByGuid`, even though we still need it later for `profiles`.
-    if (settings.profilesByGuid.emplace(profile->Guid(), static_cast<const winrt::com_ptr<Profile>&>(profile)).second)
+    if (settings.profilesByGuid.emplace(guid, static_cast<const winrt::com_ptr<Profile>&>(profile)).second)
     {
         settings.profiles.emplace_back(profile);
     }