From 0959ce811c251e68e16a48bd2ce0491e36a01581 Mon Sep 17 00:00:00 2001 From: John McPherson Date: Wed, 6 May 2020 16:13:50 -0700 Subject: [PATCH 1/4] Add default source functionality and tests --- .../Commands/SourceCommand.cpp | 3 +- .../Workflows/SourceFlow.cpp | 6 - .../Workflows/SourceFlow.h | 6 - .../Workflows/WorkflowBase.cpp | 2 +- src/AppInstallerCLITests/Sources.cpp | 56 ++++++- .../Public/AppInstallerRepositorySource.h | 6 +- .../RepositorySource.cpp | 138 ++++++++++++------ 7 files changed, 145 insertions(+), 72 deletions(-) diff --git a/src/AppInstallerCLICore/Commands/SourceCommand.cpp b/src/AppInstallerCLICore/Commands/SourceCommand.cpp index fdebd1bba1..4bcfb99c23 100644 --- a/src/AppInstallerCLICore/Commands/SourceCommand.cpp +++ b/src/AppInstallerCLICore/Commands/SourceCommand.cpp @@ -165,8 +165,7 @@ namespace AppInstaller::CLI { context << Workflow::QueryUserForSourceReset << - Workflow::ResetAllSources << - Workflow::AddDefaultSources; + Workflow::ResetAllSources; } } } diff --git a/src/AppInstallerCLICore/Workflows/SourceFlow.cpp b/src/AppInstallerCLICore/Workflows/SourceFlow.cpp index b9b2aeee6e..5d71e700b6 100644 --- a/src/AppInstallerCLICore/Workflows/SourceFlow.cpp +++ b/src/AppInstallerCLICore/Workflows/SourceFlow.cpp @@ -211,10 +211,4 @@ namespace AppInstaller::CLI::Workflow Repository::DropSource({}); context.Reporter.Info() << " Done." << std::endl; } - - void AddDefaultSources(Execution::Context& context) - { - context.Reporter.Info() << "Adding default sources ..." << std::endl; - context.Reporter.ExecuteWithProgress(Repository::AddDefaultSources); - } } diff --git a/src/AppInstallerCLICore/Workflows/SourceFlow.h b/src/AppInstallerCLICore/Workflows/SourceFlow.h index 3c165d7d11..058f9adcb1 100644 --- a/src/AppInstallerCLICore/Workflows/SourceFlow.h +++ b/src/AppInstallerCLICore/Workflows/SourceFlow.h @@ -64,10 +64,4 @@ namespace AppInstaller::CLI::Workflow // Inputs: None // Outputs: None void ResetAllSources(Execution::Context& context); - - // Removes the sources in SourceList. - // Required Args: None - // Inputs: None - // Outputs: None - void AddDefaultSources(Execution::Context& context); } diff --git a/src/AppInstallerCLICore/Workflows/WorkflowBase.cpp b/src/AppInstallerCLICore/Workflows/WorkflowBase.cpp index cc69c6e31a..a618c04f3c 100644 --- a/src/AppInstallerCLICore/Workflows/WorkflowBase.cpp +++ b/src/AppInstallerCLICore/Workflows/WorkflowBase.cpp @@ -94,7 +94,7 @@ namespace AppInstaller::CLI::Workflow else { // Even if a name was given, there are no sources - context.Reporter.Error() << "No sources defined; add one with 'source add'" << std::endl; + context.Reporter.Error() << "No sources defined; add one with 'source add' or reset to defaults with 'source reset'" << std::endl; AICLI_TERMINATE_CONTEXT(APPINSTALLER_CLI_ERROR_NO_SOURCES_DEFINED); } } diff --git a/src/AppInstallerCLITests/Sources.cpp b/src/AppInstallerCLITests/Sources.cpp index d16633d647..07baa978d0 100644 --- a/src/AppInstallerCLITests/Sources.cpp +++ b/src/AppInstallerCLITests/Sources.cpp @@ -46,11 +46,13 @@ constexpr std::string_view s_ThreeSources = R"( Arg: testArg Data: testData LastUpdate: 0 + IsDefault: 1 - Name: testName2 Type: testType2 Arg: testArg2 Data: testData2 LastUpdate: 1 + IsDefault: 0 - Name: testName3 Type: testType3 Arg: testArg3 @@ -143,7 +145,8 @@ TEST_CASE("RepoSources_UserSettingDoesNotExist", "[sources]") RemoveSetting(s_RepositorySettings_UserSources); std::vector sources = GetSources(); - REQUIRE(sources.empty()); + // The default source is added when no source exists + REQUIRE(sources.size() == 1); } TEST_CASE("RepoSources_EmptySourcesList", "[sources]") @@ -185,6 +188,7 @@ TEST_CASE("RepoSources_ThreeSources", "[sources]") REQUIRE(sources[i].Arg == "testArg"s + suffix[i]); REQUIRE(sources[i].Data == "testData"s + suffix[i]); REQUIRE(sources[i].LastUpdateTime == ConvertUnixEpochToSystemClock(i)); + REQUIRE(sources[i].IsDefault == (i == 0)); } } @@ -204,7 +208,7 @@ TEST_CASE("RepoSources_MissingField", "[sources]") TEST_CASE("RepoSources_AddSource", "[sources]") { - RemoveSetting(s_RepositorySettings_UserSources); + SetSetting(s_RepositorySettings_UserSources, s_EmptySources); TestHook_ClearSourceFactoryOverrides(); std::string name = "thisIsTheName"; @@ -234,7 +238,7 @@ TEST_CASE("RepoSources_AddSource", "[sources]") TEST_CASE("RepoSources_AddMultipleSources", "[sources]") { - RemoveSetting(s_RepositorySettings_UserSources); + SetSetting(s_RepositorySettings_UserSources, s_EmptySources); std::string name = "thisIsTheName"; std::string type = "thisIsTheType"; @@ -283,7 +287,7 @@ TEST_CASE("RepoSources_UpdateSource", "[sources]") { using namespace std::chrono_literals; - RemoveSetting(s_RepositorySettings_UserSources); + SetSetting(s_RepositorySettings_UserSources, s_EmptySources); TestHook_ClearSourceFactoryOverrides(); std::string name = "thisIsTheName"; @@ -333,7 +337,7 @@ TEST_CASE("RepoSources_UpdateSourceRetries", "[sources]") { using namespace std::chrono_literals; - RemoveSetting(s_RepositorySettings_UserSources); + SetSetting(s_RepositorySettings_UserSources, s_EmptySources); TestHook_ClearSourceFactoryOverrides(); std::string name = "thisIsTheName"; @@ -368,7 +372,7 @@ TEST_CASE("RepoSources_UpdateSourceRetries", "[sources]") TEST_CASE("RepoSources_RemoveSource", "[sources]") { - RemoveSetting(s_RepositorySettings_UserSources); + SetSetting(s_RepositorySettings_UserSources, s_EmptySources); TestHook_ClearSourceFactoryOverrides(); std::string name = "thisIsTheName"; @@ -399,7 +403,6 @@ TEST_CASE("RepoSources_UpdateOnOpen", "[sources]") { using namespace std::chrono_literals; - RemoveSetting(s_RepositorySettings_UserSources); TestHook_ClearSourceFactoryOverrides(); std::string name = "testName"; @@ -429,3 +432,42 @@ TEST_CASE("RepoSources_UpdateOnOpen", "[sources]") REQUIRE(sources[0].Data == data); REQUIRE(sources[0].LastUpdateTime == ConvertUnixEpochToSystemClock(0)); } + +TEST_CASE("RepoSources_DropSourceByName", "[sources]") +{ + SetSetting(s_RepositorySettings_UserSources, s_ThreeSources); + + std::vector sources = GetSources(); + REQUIRE(sources.size() == 3); + + DropSource("testName"); + + sources = GetSources(); + REQUIRE(sources.size() == 2); + + const char* suffix[2] = { "2", "3" }; + + for (size_t i = 0; i < 2; ++i) + { + INFO("Source #" << i); + REQUIRE(sources[i].Name == "testName"s + suffix[i]); + REQUIRE(sources[i].Type == "testType"s + suffix[i]); + REQUIRE(sources[i].Arg == "testArg"s + suffix[i]); + REQUIRE(sources[i].Data == "testData"s + suffix[i]); + REQUIRE(sources[i].LastUpdateTime == ConvertUnixEpochToSystemClock(i + 1)); + REQUIRE(!sources[i].IsDefault); + } +} + +TEST_CASE("RepoSources_DropAllSources", "[sources]") +{ + SetSetting(s_RepositorySettings_UserSources, s_ThreeSources); + + std::vector sources = GetSources(); + REQUIRE(sources.size() == 3); + + DropSource({}); + + sources = GetSources(); + REQUIRE(sources.size() == 1); +} diff --git a/src/AppInstallerRepositoryCore/Public/AppInstallerRepositorySource.h b/src/AppInstallerRepositoryCore/Public/AppInstallerRepositorySource.h index f1d4a5a5ec..0ec2e1b3d3 100644 --- a/src/AppInstallerRepositoryCore/Public/AppInstallerRepositorySource.h +++ b/src/AppInstallerRepositoryCore/Public/AppInstallerRepositorySource.h @@ -31,6 +31,9 @@ namespace AppInstaller::Repository // The last time that this source was updated. std::chrono::system_clock::time_point LastUpdateTime; + + // This source is a default source; added for the user by the tool. + bool IsDefault = false; }; // Interface for interacting with a source from outside of the repository lib. @@ -54,9 +57,6 @@ namespace AppInstaller::Repository // Adds a new source for the user. void AddSource(std::string name, std::string type, std::string arg, IProgressCallback& progress); - // Adds the default sources. - void AddDefaultSources(IProgressCallback& progress); - // Opens an existing source. // Passing an empty string as the name of the source will return a source that aggregates all others. std::shared_ptr OpenSource(std::string_view name, IProgressCallback& progress); diff --git a/src/AppInstallerRepositoryCore/RepositorySource.cpp b/src/AppInstallerRepositoryCore/RepositorySource.cpp index e89c72babd..697b8f0833 100644 --- a/src/AppInstallerRepositoryCore/RepositorySource.cpp +++ b/src/AppInstallerRepositoryCore/RepositorySource.cpp @@ -18,18 +18,25 @@ namespace AppInstaller::Repository constexpr std::string_view s_SourcesYaml_Source_Arg = "Arg"sv; constexpr std::string_view s_SourcesYaml_Source_Data = "Data"sv; constexpr std::string_view s_SourcesYaml_Source_LastUpdate = "LastUpdate"sv; + constexpr std::string_view s_SourcesYaml_Source_IsDefault = "IsDefault"sv; + + constexpr std::string_view s_Source_WingetCommunityDefault_Name = "winget-community"sv; + constexpr std::string_view s_Source_WingetCommunityDefault_Arg = "https://winget.azureedge.net/cache"sv; namespace { // Attempts to read a single scalar value from the node. template - bool TryReadScalar(std::string_view settingName, const std::string& settingValue, const YAML::Node& sourceNode, std::string_view name, Value& value) + bool TryReadScalar(std::string_view settingName, const std::string& settingValue, const YAML::Node& sourceNode, std::string_view name, Value& value, bool required = true) { YAML::Node valueNode = sourceNode[std::string{ name }]; if (!valueNode || !valueNode.IsScalar()) { - AICLI_LOG(Repo, Error, << "Setting '" << settingName << "' did not contain the expected format (" << name << " is invalid within a source):\n" << settingValue); + if (required) + { + AICLI_LOG(Repo, Error, << "Setting '" << settingName << "' did not contain the expected format (" << name << " is invalid within a source):\n" << settingValue); + } return false; } @@ -88,6 +95,16 @@ namespace AppInstaller::Repository int64_t lastUpdateInEpoch{}; if (!TryReadScalar(settingName, settingValue, source, s_SourcesYaml_Source_LastUpdate, lastUpdateInEpoch)) { return false; } details.LastUpdateTime = Utility::ConvertUnixEpochToSystemClock(lastUpdateInEpoch); + int32_t isDefaultNumber{}; + if (TryReadScalar(settingName, settingValue, source, s_SourcesYaml_Source_IsDefault, isDefaultNumber, false)) + { + details.IsDefault = (isDefaultNumber != 0); + } + else + { + // If older than defaults, assume it is not one. + details.IsDefault = false; + } result.emplace_back(std::move(details)); } @@ -126,16 +143,6 @@ namespace AppInstaller::Repository return TryGetSourcesFromSetting(settingName).value_or(std::vector{}); } - // If there is no setting value at all, adds the default sources. - void AddDefaultSourcesIfNeeded(IProgressCallback& progress) - { - auto sourcesStream = Runtime::GetSettingStream(s_RepositorySettings_UserSources); - if (!sourcesStream) - { - AddDefaultSources(progress); - } - } - // Make up for the lack of string_view support in YAML CPP. YAML::Emitter& operator<<(YAML::Emitter& out, std::string_view sv) { @@ -158,6 +165,7 @@ namespace AppInstaller::Repository out << YAML::Key << s_SourcesYaml_Source_Arg << YAML::Value << details.Arg; out << YAML::Key << s_SourcesYaml_Source_Data << YAML::Value << details.Data; out << YAML::Key << s_SourcesYaml_Source_LastUpdate << YAML::Value << Utility::ConvertSystemClockToUnixEpoch(details.LastUpdateTime); + out << YAML::Key << s_SourcesYaml_Source_IsDefault << YAML::Value << (details.IsDefault ? 1 : 0); out << YAML::EndMap; } @@ -264,10 +272,77 @@ namespace AppInstaller::Repository return false; } + + SourceDetails PrepareSourceDetailsForAdd(std::string name, std::string type, std::string arg, bool isDefault) + { + THROW_HR_IF(E_INVALIDARG, name.empty()); + + AICLI_LOG(Repo, Info, << "Adding source: Name[" << name << "], Type[" << type << "], Arg[" << arg << "]"); + + // Check all sources for the given name. + std::vector currentSources = GetSources(); + + auto itr = FindSourceByName(currentSources, name); + THROW_HR_IF(APPINSTALLER_CLI_ERROR_SOURCE_NAME_ALREADY_EXISTS, itr != currentSources.end()); + + SourceDetails details; + details.Name = std::move(name); + details.Type = std::move(type); + details.Arg = std::move(arg); + details.LastUpdateTime = Utility::ConvertUnixEpochToSystemClock(0); + details.IsDefault = isDefault; + + return details; + } + + void AddDetailsToSetting(const SourceDetails& details, std::string_view setting) + { + AICLI_LOG(Repo, Info, << "Source created with extra data: " << details.Data); + + std::vector currentSources = GetSourcesFromSetting(setting); + currentSources.emplace_back(details); + + SetSourcesToSetting(setting, currentSources); + } + + void AddSourceInternal(std::string name, std::string type, std::string arg, bool isDefault, IProgressCallback& progress) + { + SourceDetails details = PrepareSourceDetailsForAdd(name, type, arg, isDefault); + + UpdateSourceFromDetails(details, progress); + + AddDetailsToSetting(details, s_RepositorySettings_UserSources); + } + + void AddUninitializedSourceInternal(std::string name, std::string type, std::string arg, bool isDefault) + { + SourceDetails details = PrepareSourceDetailsForAdd(name, type, arg, isDefault); + AddDetailsToSetting(details, s_RepositorySettings_UserSources); + } + + // If there is no setting value at all, adds the default sources. + void AddDefaultSourcesIfNeeded() + { + auto sourcesStream = Runtime::GetSettingStream(s_RepositorySettings_UserSources); + if (!sourcesStream) + { + // We have to set an initial, empty list of sources or the add will create an infinite loop. + SetSourcesToSetting(s_RepositorySettings_UserSources, std::vector{}); + + AddUninitializedSourceInternal( + std::string(s_Source_WingetCommunityDefault_Name), + std::string(Microsoft::PreIndexedPackageSourceFactory::Type()), + std::string(s_Source_WingetCommunityDefault_Arg), + true); + } + } } + // TODO: If we merge sources from multiple settings in the future, the other functions + // in this file all need to be enlightened with how to write to othe appropriate location. std::vector GetSources() { + AddDefaultSourcesIfNeeded(); return GetSourcesFromSetting(s_RepositorySettings_UserSources); } @@ -289,42 +364,11 @@ namespace AppInstaller::Repository void AddSource(std::string name, std::string type, std::string arg, IProgressCallback& progress) { - THROW_HR_IF(E_INVALIDARG, name.empty()); - - AICLI_LOG(Repo, Info, << "Adding source: Name[" << name << "], Type[" << type << "], Arg[" << arg << "]"); - - // Check all sources for the given name. - std::vector currentSources = GetSources(); - - auto itr = FindSourceByName(currentSources, name); - THROW_HR_IF(APPINSTALLER_CLI_ERROR_SOURCE_NAME_ALREADY_EXISTS, itr != currentSources.end()); - - SourceDetails details; - details.Name = std::move(name); - details.Type = std::move(type); - details.Arg = std::move(arg); - details.LastUpdateTime = Utility::ConvertUnixEpochToSystemClock(0); - - UpdateSourceFromDetails(details, progress); - - AICLI_LOG(Repo, Info, << "Source created with extra data: " << details.Data); - - currentSources = GetSourcesFromSetting(s_RepositorySettings_UserSources); - currentSources.emplace_back(details); - - SetSourcesToSetting(s_RepositorySettings_UserSources, currentSources); - } - - void AddDefaultSources(IProgressCallback& progress) - { - UNREFERENCED_PARAMETER(progress); - // TODO: Create list of default sources + AddSourceInternal(name, type, arg, false, progress); } std::shared_ptr OpenSource(std::string_view name, IProgressCallback& progress) { - AddDefaultSourcesIfNeeded(progress); - std::vector currentSources = GetSources(); if (name.empty()) @@ -367,7 +411,7 @@ namespace AppInstaller::Repository { THROW_HR_IF(E_INVALIDARG, name.empty()); - std::vector currentSources = GetSourcesFromSetting(s_RepositorySettings_UserSources); + std::vector currentSources = GetSources(); auto itr = FindSourceByName(currentSources, name); if (itr == currentSources.end()) @@ -389,7 +433,7 @@ namespace AppInstaller::Repository { THROW_HR_IF(E_INVALIDARG, name.empty()); - std::vector currentSources = GetSourcesFromSetting(s_RepositorySettings_UserSources); + std::vector currentSources = GetSources(); auto itr = FindSourceByName(currentSources, name); if (itr == currentSources.end()) @@ -418,7 +462,7 @@ namespace AppInstaller::Repository } else { - std::vector currentSources = GetSourcesFromSetting(s_RepositorySettings_UserSources); + std::vector currentSources = GetSources(); auto itr = FindSourceByName(currentSources, name); if (itr == currentSources.end()) From 01aa39450109577ad54d6f4faf0a9cf1f70287e5 Mon Sep 17 00:00:00 2001 From: John McPherson Date: Wed, 6 May 2020 16:47:52 -0700 Subject: [PATCH 2/4] Change default source name and add to tests --- src/AppInstallerCLITests/Sources.cpp | 2 ++ src/AppInstallerRepositoryCore/RepositorySource.cpp | 2 +- 2 files changed, 3 insertions(+), 1 deletion(-) diff --git a/src/AppInstallerCLITests/Sources.cpp b/src/AppInstallerCLITests/Sources.cpp index 07baa978d0..3db976e0bb 100644 --- a/src/AppInstallerCLITests/Sources.cpp +++ b/src/AppInstallerCLITests/Sources.cpp @@ -147,6 +147,7 @@ TEST_CASE("RepoSources_UserSettingDoesNotExist", "[sources]") std::vector sources = GetSources(); // The default source is added when no source exists REQUIRE(sources.size() == 1); + REQUIRE(sources[0].IsDefault); } TEST_CASE("RepoSources_EmptySourcesList", "[sources]") @@ -470,4 +471,5 @@ TEST_CASE("RepoSources_DropAllSources", "[sources]") sources = GetSources(); REQUIRE(sources.size() == 1); + REQUIRE(sources[0].IsDefault); } diff --git a/src/AppInstallerRepositoryCore/RepositorySource.cpp b/src/AppInstallerRepositoryCore/RepositorySource.cpp index 697b8f0833..c10f8e59e3 100644 --- a/src/AppInstallerRepositoryCore/RepositorySource.cpp +++ b/src/AppInstallerRepositoryCore/RepositorySource.cpp @@ -20,7 +20,7 @@ namespace AppInstaller::Repository constexpr std::string_view s_SourcesYaml_Source_LastUpdate = "LastUpdate"sv; constexpr std::string_view s_SourcesYaml_Source_IsDefault = "IsDefault"sv; - constexpr std::string_view s_Source_WingetCommunityDefault_Name = "winget-community"sv; + constexpr std::string_view s_Source_WingetCommunityDefault_Name = "winget"sv; constexpr std::string_view s_Source_WingetCommunityDefault_Arg = "https://winget.azureedge.net/cache"sv; namespace From 8fc9a8b45aebe617ebf567ffac4a8ad764c7a5e5 Mon Sep 17 00:00:00 2001 From: John McPherson Date: Wed, 6 May 2020 17:21:25 -0700 Subject: [PATCH 3/4] Remove expectation that there are no sources by default from E2E test --- src/AppInstallerCLIE2ETests/SourceCommand.cs | 7 ------- 1 file changed, 7 deletions(-) diff --git a/src/AppInstallerCLIE2ETests/SourceCommand.cs b/src/AppInstallerCLIE2ETests/SourceCommand.cs index aedd6c66f4..2a8b306b2e 100644 --- a/src/AppInstallerCLIE2ETests/SourceCommand.cs +++ b/src/AppInstallerCLIE2ETests/SourceCommand.cs @@ -74,13 +74,6 @@ public void SourceCommands() result = TestCommon.RunAICLICommand("source remove", $"-n {SourceTestSourceName}"); Assert.AreEqual(Constants.ErrorCode.S_OK, result.ExitCode); Assert.True(result.StdOut.Contains("Done")); - - TestCommon.WaitForDeploymentFinish(); - - // List should show no source available - result = TestCommon.RunAICLICommand("source list", ""); - Assert.AreEqual(Constants.ErrorCode.S_OK, result.ExitCode); - Assert.True(result.StdOut.Contains("There are no sources configured.")); } } } \ No newline at end of file From f30dc27d7d2fb21fb320b7e09510d3587fc5d571 Mon Sep 17 00:00:00 2001 From: John McPherson Date: Wed, 6 May 2020 23:05:27 -0700 Subject: [PATCH 4/4] Update comments from PR feedback --- src/AppInstallerCLICore/Workflows/SourceFlow.h | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/src/AppInstallerCLICore/Workflows/SourceFlow.h b/src/AppInstallerCLICore/Workflows/SourceFlow.h index 058f9adcb1..c8b80135e8 100644 --- a/src/AppInstallerCLICore/Workflows/SourceFlow.h +++ b/src/AppInstallerCLICore/Workflows/SourceFlow.h @@ -47,19 +47,19 @@ namespace AppInstaller::CLI::Workflow // Outputs: None void RemoveSources(Execution::Context& context); - // Removes the sources in SourceList. + // Asks the user if they are ok with dropping the sources in SourceList. // Required Args: None - // Inputs: None + // Inputs: SourceList // Outputs: None void QueryUserForSourceReset(Execution::Context& context); - // Removes the sources in SourceList. + // Drops the sources in SourceList. // Required Args: None // Inputs: SourceList // Outputs: None void ResetSourceList(Execution::Context& context); - // Removes the sources in SourceList. + // Drops all sources. // Required Args: None // Inputs: None // Outputs: None