Fix winget after a call to winget settings export (#2767)

microsoft · Dec 15, 2022 · 8a805cc · 8a805cc
1 parent 23f9942
commit 8a805cc
Show file tree

Hide file tree

Showing 15 changed files with 138 additions and 82 deletions.
diff --git a/src/AppInstallerCLICore/Command.cpp b/src/AppInstallerCLICore/Command.cpp
@@ -30,8 +30,9 @@ namespace AppInstaller::CLI
         std::string_view parent,
         Command::Visibility visibility,
         Settings::ExperimentalFeature::Feature feature,
-        Settings::TogglePolicy::Policy groupPolicy) :
-        m_name(name), m_aliases(std::move(aliases)), m_visibility(visibility), m_feature(feature), m_groupPolicy(groupPolicy)
+        Settings::TogglePolicy::Policy groupPolicy,
+        CommandOutputFlags outputFlags) :
+        m_name(name), m_aliases(std::move(aliases)), m_visibility(visibility), m_feature(feature), m_groupPolicy(groupPolicy), m_outputFlags(outputFlags)
     {
         if (!parent.empty())
         {
@@ -933,7 +934,8 @@ namespace AppInstaller::CLI
     {
         try
         {
-            if (!Settings::User().GetWarnings().empty())
+            if (!Settings::User().GetWarnings().empty() &&
+                !WI_IsFlagSet(command->GetOutputFlags(), CommandOutputFlags::IgnoreSettingsWarnings))
             {
                 context.Reporter.Warn() << Resource::String::SettingsWarnings << std::endl;
             }

diff --git a/src/AppInstallerCLICore/Command.h b/src/AppInstallerCLICore/Command.h
@@ -44,6 +44,15 @@ namespace AppInstaller::CLI
         std::vector<Utility::LocIndString> m_params;
     };
 
+    // Flags to control the behavior of the command output.
+    enum class CommandOutputFlags : int
+    {
+        None = 0x0,
+        IgnoreSettingsWarnings = 0x1,
+    };
+
+    DEFINE_ENUM_FLAG_OPERATORS(CommandOutputFlags);
+
     struct Command
     {
         // Controls the visibility of the field.
@@ -57,17 +66,27 @@ namespace AppInstaller::CLI
 
         Command(std::string_view name, std::string_view parent) :
             Command(name, {}, parent) {}
-        Command(std::string_view name,std::vector<std::string_view> aliases, std::string_view parent) :
+        Command(std::string_view name, std::vector<std::string_view> aliases, std::string_view parent) :
             Command(name, aliases, parent, Settings::ExperimentalFeature::Feature::None) {}
-        Command(std::string_view name,std::vector<std::string_view> aliases, std::string_view parent, Command::Visibility visibility) :
+        Command(std::string_view name, std::string_view parent, CommandOutputFlags outputFlags) :
+            Command(name, {}, parent, Command::Visibility::Show, Settings::ExperimentalFeature::Feature::None, Settings::TogglePolicy::Policy::None, outputFlags) {}
+        Command(std::string_view name, std::vector<std::string_view> aliases, std::string_view parent, Command::Visibility visibility) :
             Command(name, aliases, parent, visibility, Settings::ExperimentalFeature::Feature::None) {}
-        Command(std::string_view name,std::vector<std::string_view> aliases, std::string_view parent, Settings::ExperimentalFeature::Feature feature) :
+        Command(std::string_view name, std::vector<std::string_view> aliases, std::string_view parent, Settings::ExperimentalFeature::Feature feature) :
             Command(name, aliases, parent, Command::Visibility::Show, feature) {}
-        Command(std::string_view name,std::vector<std::string_view> aliases, std::string_view parent, Settings::TogglePolicy::Policy groupPolicy) :
-            Command(name, aliases, parent, Command::Visibility::Show, Settings::ExperimentalFeature::Feature::None, groupPolicy) {}
-        Command(std::string_view name,std::vector<std::string_view> aliases, std::string_view parent, Command::Visibility visibility, Settings::ExperimentalFeature::Feature feature) :
-            Command(name, aliases, parent, visibility, feature, Settings::TogglePolicy::Policy::None) {}
-        Command(std::string_view name,std::vector<std::string_view> aliases, std::string_view parent, Command::Visibility visibility, Settings::ExperimentalFeature::Feature feature, Settings::TogglePolicy::Policy groupPolicy);
+        Command(std::string_view name, std::vector<std::string_view> aliases, std::string_view parent, Settings::TogglePolicy::Policy groupPolicy) :
+            Command(name, aliases, parent, Command::Visibility::Show, Settings::ExperimentalFeature::Feature::None, groupPolicy, CommandOutputFlags::None) {}
+        Command(std::string_view name, std::vector<std::string_view> aliases, std::string_view parent, Command::Visibility visibility, Settings::ExperimentalFeature::Feature feature) :
+            Command(name, aliases, parent, visibility, feature, Settings::TogglePolicy::Policy::None, CommandOutputFlags::None) {}
+
+        Command(std::string_view name,
+            std::vector<std::string_view> aliases,
+            std::string_view parent,
+            Command::Visibility visibility,
+            Settings::ExperimentalFeature::Feature feature,
+            Settings::TogglePolicy::Policy groupPolicy,
+            CommandOutputFlags outputFlags);
+
         virtual ~Command() = default;
 
         Command(const Command&) = default;
@@ -85,6 +104,7 @@ namespace AppInstaller::CLI
         Command::Visibility GetVisibility() const;
         Settings::ExperimentalFeature::Feature Feature() const { return m_feature; }
         Settings::TogglePolicy::Policy GroupPolicy() const { return m_groupPolicy; }
+        CommandOutputFlags GetOutputFlags() const { return m_outputFlags; }
 
         virtual std::vector<std::unique_ptr<Command>> GetCommands() const { return {}; }
         virtual std::vector<Argument> GetArguments() const { return {}; }
@@ -118,6 +138,7 @@ namespace AppInstaller::CLI
         Command::Visibility m_visibility;
         Settings::ExperimentalFeature::Feature m_feature;
         Settings::TogglePolicy::Policy m_groupPolicy;
+        CommandOutputFlags m_outputFlags;
     };
 
     template <typename Container>

diff --git a/src/AppInstallerCLICore/Commands/RootCommand.cpp b/src/AppInstallerCLICore/Commands/RootCommand.cpp
@@ -27,6 +27,8 @@ using namespace AppInstaller::Utility::literals;
 
 namespace AppInstaller::CLI
 {
+    using namespace Settings;
+
     namespace
     {
         void OutputGroupPolicySourceList(Execution::Context& context, const std::vector<Settings::SourceFromPolicy>& sources, Resource::StringId header)
@@ -193,7 +195,7 @@ namespace AppInstaller::CLI
             };
 
             info << std::endl << Resource::String::Logs << ": "_liv << Runtime::GetPathTo(Runtime::PathName::DefaultLogLocationForDisplay).u8string() << std::endl;
-            info << std::endl << Resource::String::UserSettings << ": "_liv << Runtime::GetPathTo(Runtime::PathName::UserSettingsFileLocationForDisplay).u8string() << std::endl;
+            info << std::endl << Resource::String::UserSettings << ": "_liv << UserSettings::SettingsFilePath(true).u8string() << std::endl;
 
             info << std::endl;
 

diff --git a/src/AppInstallerCLICore/Commands/SettingsCommand.h b/src/AppInstallerCLICore/Commands/SettingsCommand.h
@@ -24,7 +24,7 @@ namespace AppInstaller::CLI
 
     struct SettingsExportCommand final : public Command
     {
-        SettingsExportCommand(std::string_view parent) : Command("export", parent) {}
+        SettingsExportCommand(std::string_view parent) : Command("export", parent, CommandOutputFlags::IgnoreSettingsWarnings) {}
 
         Resource::LocString ShortDescription() const override;
         Resource::LocString LongDescription() const override;

diff --git a/src/AppInstallerCLICore/Commands/SourceCommand.h b/src/AppInstallerCLICore/Commands/SourceCommand.h
@@ -106,7 +106,7 @@ namespace AppInstaller::CLI
 
     struct SourceExportCommand final : public Command
     {
-        SourceExportCommand(std::string_view parent) : Command("export", parent) {}
+        SourceExportCommand(std::string_view parent) : Command("export", parent, CommandOutputFlags::IgnoreSettingsWarnings) {}
 
         std::vector<Argument> GetArguments() const override;
 

diff --git a/src/AppInstallerCLICore/Workflows/SettingsFlow.cpp b/src/AppInstallerCLICore/Workflows/SettingsFlow.cpp
@@ -20,7 +20,7 @@ namespace AppInstaller::CLI::Workflow
             {
                 root["$schema"] = "https://aka.ms/winget-settings-export.schema.json";
                 root["adminSettings"] = Json::ValueType::objectValue;
-                root["userSettingsFile"] = Runtime::GetPathTo(Runtime::PathName::UserSettingsFileLocation).u8string();
+                root["userSettingsFile"] = UserSettings::SettingsFilePath().u8string();
             }
 
             void AddAdminSetting(AdminSetting setting)

diff --git a/src/AppInstallerCLIPackage/Shared/Strings/en-us/winget.resw b/src/AppInstallerCLIPackage/Shared/Strings/en-us/winget.resw
@@ -1541,4 +1541,7 @@ Please specify one of them using the --source option to proceed.</value>
   <data name="UserSettings" xml:space="preserve">
     <value>User Settings</value>
   </data>
+  <data name="SettingsWarningUsingDefault" xml:space="preserve">
+    <value>Settings file couldn't load. Using default values.</value>
+  </data>
 </root>
diff --git a/src/AppInstallerCLITests/UserSettings.cpp b/src/AppInstallerCLITests/UserSettings.cpp
@@ -419,12 +419,16 @@ TEST_CASE("SettingsPortablePackageUserRoot", "[settings]")
     SECTION("Relative path")
     {
         DeleteUserSettingsFiles();
-        std::string_view json = R"({ "installBehavior": { "portablePackageUserRoot": %LOCALAPPDATA%/Portable/Root } })";
+        std::string_view json = R"({ "installBehavior": { "portablePackageUserRoot": "%LOCALAPPDATA%/Portable/Root" } })";
         SetSetting(Stream::PrimaryUserSettings, json);
         UserSettingsTest userSettingTest;
 
         REQUIRE(userSettingTest.Get<Setting::PortablePackageUserRoot>().empty());
-        REQUIRE(userSettingTest.GetWarnings().size() == 1);
+
+        auto warnings = userSettingTest.GetWarnings();
+        REQUIRE(warnings.size() == 1);
+        REQUIRE(warnings[0].Message == AppInstaller::StringResource::String::SettingsWarningInvalidFieldValue);
+        REQUIRE(warnings[0].Path == ".installBehavior.portablePackageUserRoot");
     }
     SECTION("Valid path")
     {
@@ -443,12 +447,16 @@ TEST_CASE("SettingsPortablePackageMachineRoot", "[settings]")
     SECTION("Relative path")
     {
         DeleteUserSettingsFiles();
-        std::string_view json = R"({ "installBehavior": { "portablePackageMachineRoot": %LOCALAPPDATA%/Portable/Root } })";
+        std::string_view json = R"({ "installBehavior": { "portablePackageMachineRoot": "%LOCALAPPDATA%/Portable/Root" } })";
         SetSetting(Stream::PrimaryUserSettings, json);
         UserSettingsTest userSettingTest;
 
         REQUIRE(userSettingTest.Get<Setting::PortablePackageMachineRoot>().empty());
-        REQUIRE(userSettingTest.GetWarnings().size() == 1);
+
+        auto warnings = userSettingTest.GetWarnings();
+        REQUIRE(warnings.size() == 1);
+        REQUIRE(warnings[0].Message == AppInstaller::StringResource::String::SettingsWarningInvalidFieldValue);
+        REQUIRE(warnings[0].Path == ".installBehavior.portablePackageMachineRoot");
     }
     SECTION("Valid path")
     {

diff --git a/src/AppInstallerCommonCore/Filesystem.cpp b/src/AppInstallerCommonCore/Filesystem.cpp
@@ -201,4 +201,41 @@ namespace AppInstaller::Filesystem
             return path;
         }
     }
+
+    void ReplaceCommonPathPrefix(std::filesystem::path& source, const std::filesystem::path& prefix, std::string_view replacement)
+    {
+        auto prefixItr = prefix.begin();
+        auto sourceItr = source.begin();
+
+        while (prefixItr != prefix.end() && sourceItr != source.end())
+        {
+            if (*prefixItr != *sourceItr)
+            {
+                break;
+            }
+
+            ++prefixItr;
+            ++sourceItr;
+        }
+
+        // Only replace source if we found all of prefix
+        if (prefixItr == prefix.end())
+        {
+            std::filesystem::path temp{ replacement };
+
+            for (; sourceItr != source.end(); ++sourceItr)
+            {
+                temp /= *sourceItr;
+            }
+
+            source = std::move(temp);
+        }
+    }
+
+    std::filesystem::path GetKnownFolderPath(const KNOWNFOLDERID& id)
+    {
+        wil::unique_cotaskmem_string knownFolder = nullptr;
+        THROW_IF_FAILED(SHGetKnownFolderPath(id, KF_FLAG_NO_ALIAS | KF_FLAG_DONT_VERIFY | KF_FLAG_NO_PACKAGE_REDIRECTION, NULL, &knownFolder));
+        return knownFolder.get();
+    }
 }
diff --git a/src/AppInstallerCommonCore/Public/AppInstallerRuntime.h b/src/AppInstallerCommonCore/Public/AppInstallerRuntime.h
@@ -62,10 +62,6 @@ namespace AppInstaller::Runtime
         PortableLinksUserLocation,
         // The location where symlinks to portable packages are stored under machine scope.
         PortableLinksMachineLocation,
-        // The location of the user settings json file.
-        UserSettingsFileLocation,
-        // The location of the user settings json file, anonymized using environment variables.
-        UserSettingsFileLocationForDisplay,
     };
 
     // The principal that an ACE applies to.

diff --git a/src/AppInstallerCommonCore/Public/winget/Filesystem.h b/src/AppInstallerCommonCore/Public/winget/Filesystem.h
@@ -2,6 +2,7 @@
 // Licensed under the MIT License.
 #pragma once
 #include <filesystem>
+#include <shtypes.h>
 
 namespace AppInstaller::Filesystem
 {
@@ -35,4 +36,10 @@ namespace AppInstaller::Filesystem
 
     // Get expanded file system path.
     std::filesystem::path GetExpandedPath(const std::string& path);
+
+    // If `source` begins with all of `prefix`, replace that with `replacement`.
+    void ReplaceCommonPathPrefix(std::filesystem::path& source, const std::filesystem::path& prefix, std::string_view replacement);
+
+    // Gets the path of a known folder.
+    std::filesystem::path GetKnownFolderPath(const KNOWNFOLDERID& id);
 }
diff --git a/src/AppInstallerCommonCore/Public/winget/Resources.h b/src/AppInstallerCommonCore/Public/winget/Resources.h
@@ -43,6 +43,7 @@ namespace AppInstaller
             WINGET_DEFINE_RESOURCE_STRINGID(SettingsWarningInvalidValueFromPolicy);
             WINGET_DEFINE_RESOURCE_STRINGID(SettingsWarningLoadedBackupSettings);
             WINGET_DEFINE_RESOURCE_STRINGID(SettingsWarningParseError);
+            WINGET_DEFINE_RESOURCE_STRINGID(SettingsWarningUsingDefault);
         };
     }
 

diff --git a/src/AppInstallerCommonCore/Public/winget/UserSettings.h b/src/AppInstallerCommonCore/Public/winget/UserSettings.h
@@ -206,7 +206,7 @@ namespace AppInstaller::Settings
 
         static UserSettings const& Instance(const std::optional<std::string>& content = std::nullopt);
 
-        static std::filesystem::path SettingsFilePath();
+        static std::filesystem::path SettingsFilePath(bool forDisplay = false);
 
         UserSettings(const UserSettings&) = delete;
         UserSettings& operator=(const UserSettings&) = delete;

diff --git a/src/AppInstallerCommonCore/Runtime.cpp b/src/AppInstallerCommonCore/Runtime.cpp
@@ -16,6 +16,7 @@ namespace AppInstaller::Runtime
 {
     using namespace Utility;
     using namespace Settings;
+    using namespace Filesystem;
 
     namespace
     {
@@ -96,13 +97,6 @@ namespace AppInstaller::Runtime
         static std::map<PathName, PathDetails> s_Path_TestHook_Overrides;
 #endif
 
-        std::filesystem::path GetKnownFolderPath(const KNOWNFOLDERID& id)
-        {
-            wil::unique_cotaskmem_string knownFolder = nullptr;
-            THROW_IF_FAILED(SHGetKnownFolderPath(id, KF_FLAG_NO_ALIAS | KF_FLAG_DONT_VERIFY | KF_FLAG_NO_PACKAGE_REDIRECTION, NULL, &knownFolder));
-            return knownFolder.get();
-        }
-
         // Gets the user's temp path
         std::filesystem::path GetPathToUserTemp()
         {
@@ -166,37 +160,6 @@ namespace AppInstaller::Runtime
             return result;
         }
 
-        // If `source` begins with all of `prefix`, replace that with `replacement`.
-        void ReplaceCommonPathPrefix(std::filesystem::path& source, const std::filesystem::path& prefix, std::string_view replacement)
-        {
-            auto prefixItr = prefix.begin();
-            auto sourceItr = source.begin();
-
-            while (prefixItr != prefix.end() && sourceItr != source.end())
-            {
-                if (*prefixItr != *sourceItr)
-                {
-                    break;
-                }
-
-                ++prefixItr;
-                ++sourceItr;
-            }
-
-            // Only replace source if we found all of prefix
-            if (prefixItr == prefix.end())
-            {
-                std::filesystem::path temp{ replacement };
-
-                for (; sourceItr != source.end(); ++sourceItr)
-                {
-                    temp /= *sourceItr;
-                }
-
-                source = std::move(temp);
-            }
-        }
-
         DWORD AccessPermissionsFrom(ACEPermissions permissions)
         {
             DWORD result = 0;
@@ -547,13 +510,6 @@ namespace AppInstaller::Runtime
         case PathName::PortableLinksMachineLocation:
             result = GetPathDetailsCommon(path);
             break;
-        case PathName::UserSettingsFileLocation:
-            result.Path = UserSettings::SettingsFilePath();
-            break;
-        case PathName::UserSettingsFileLocationForDisplay:
-            result.Path = UserSettings::SettingsFilePath();
-            ReplaceCommonPathPrefix(result.Path, GetKnownFolderPath(FOLDERID_LocalAppData), "%LOCALAPPDATA%");
-            break;
         default:
             THROW_HR(E_UNEXPECTED);
         }
@@ -624,13 +580,6 @@ namespace AppInstaller::Runtime
         case PathName::PortableLinksMachineLocation:
             result = GetPathDetailsCommon(path);
             break;
-        case PathName::UserSettingsFileLocation:
-            result.Path = UserSettings::SettingsFilePath();
-            break;
-        case PathName::UserSettingsFileLocationForDisplay:
-            result.Path = UserSettings::SettingsFilePath();
-            ReplaceCommonPathPrefix(result.Path, GetKnownFolderPath(FOLDERID_LocalAppData), "%LOCALAPPDATA%");
-            break;
         default:
             THROW_HR(E_UNEXPECTED);
         }