From d94b94bfaff4b2262780a7c80a837fefe8d55870 Mon Sep 17 00:00:00 2001 From: JohnMcPMS Date: Wed, 18 Mar 2020 13:05:58 -0700 Subject: [PATCH] Fix version only sorting in the manifest selection (#61) --- .../Workflows/InstallFlow.cpp | 29 ++------ .../Workflows/InstallFlow.h | 8 +- .../Workflows/ShowFlow.cpp | 44 +++++------ src/AppInstallerCLICore/Workflows/ShowFlow.h | 6 +- .../Workflows/WorkflowBase.cpp | 74 ++++++++++++++----- .../Workflows/WorkflowBase.h | 16 +++- src/AppInstallerCLICore/pch.h | 1 + src/AppInstallerCLITests/SQLiteIndex.cpp | 50 +++++++++++++ .../SQLiteIndexSource.cpp | 21 ++++-- src/AppInstallerCLITests/WorkFlow.cpp | 2 +- .../Microsoft/SQLiteIndexSource.cpp | 9 ++- .../Microsoft/Schema/1_0/Interface.cpp | 4 +- .../Public/AppInstallerRepositorySearch.h | 2 +- 13 files changed, 176 insertions(+), 90 deletions(-) diff --git a/src/AppInstallerCLICore/Workflows/InstallFlow.cpp b/src/AppInstallerCLICore/Workflows/InstallFlow.cpp index f1788d554df1a..ae0909a882b33 100644 --- a/src/AppInstallerCLICore/Workflows/InstallFlow.cpp +++ b/src/AppInstallerCLICore/Workflows/InstallFlow.cpp @@ -3,7 +3,6 @@ #include "pch.h" #include "InstallFlow.h" -#include "ManifestComparator.h" #include "ShellExecuteInstallerHandler.h" #include "MsixInstallerHandler.h" @@ -18,46 +17,30 @@ namespace AppInstaller::Workflow if (m_argsRef.Contains(ExecutionArgs::Type::Manifest)) { m_manifest = Manifest::Manifest::CreateFromPath(*(m_argsRef.GetArg(ExecutionArgs::Type::Manifest))); - InstallInternal(); } else { - if (WorkflowBase::IndexSearch() && WorkflowBase::EnsureOneMatchFromSearchResult()) + if (!IndexSearch() || !EnsureOneMatchFromSearchResult() || !GetManifest()) { - GetManifest(); - InstallInternal(); + return; } + m_reporterRef.ShowMsg("Found app: " + m_searchResult.Matches[0].Application->GetName()); } + + SelectInstaller(); + InstallInternal(); } void InstallFlow::InstallInternal() { Logging::Telemetry().LogManifestFields(m_manifest.Name, m_manifest.Version); - // Select Installer - ManifestComparator manifestComparator(m_manifest, m_reporterRef); - m_selectedInstaller = manifestComparator.GetPreferredInstaller(m_argsRef); - auto installerHandler = GetInstallerHandler(); installerHandler->Download(); installerHandler->Install(); } - void InstallFlow::GetManifest() - { - auto app = m_searchResult.Matches.at(0).Application.get(); - - Logging::Telemetry().LogManifestFields(app->GetName(), app->GetId()); - m_reporterRef.ShowMsg("Found app: " + app->GetName()); - - // Todo: handle failure if necessary after real search is in place - m_manifest = app->GetManifest( - m_argsRef.Contains(ExecutionArgs::Type::Version) ? *m_argsRef.GetArg(ExecutionArgs::Type::Version) : "", - m_argsRef.Contains(ExecutionArgs::Type::Channel) ? *m_argsRef.GetArg(ExecutionArgs::Type::Channel) : "" - ); - } - std::unique_ptr InstallFlow::GetInstallerHandler() { switch (m_selectedInstaller.InstallerType) diff --git a/src/AppInstallerCLICore/Workflows/InstallFlow.h b/src/AppInstallerCLICore/Workflows/InstallFlow.h index bbecf2e7d44ad..7929b4aaeb476 100644 --- a/src/AppInstallerCLICore/Workflows/InstallFlow.h +++ b/src/AppInstallerCLICore/Workflows/InstallFlow.h @@ -9,20 +9,16 @@ namespace AppInstaller::Workflow { - class InstallFlow : public WorkflowBase + class InstallFlow : public SingleManifestWorkflow { public: - InstallFlow(AppInstaller::CLI::ExecutionContext& context) : WorkflowBase(context) {} + InstallFlow(AppInstaller::CLI::ExecutionContext& context) : SingleManifestWorkflow(context) {} // Execute will perform a query against index and do app install if a target app is found. // If a manifest is given with /manifest, use the manifest and no index search is performed. void Execute(); protected: - AppInstaller::Manifest::Manifest m_manifest; - AppInstaller::Manifest::ManifestInstaller m_selectedInstaller; - - void GetManifest(); void InstallInternal(); // Creates corresponding InstallerHandler according to InstallerType diff --git a/src/AppInstallerCLICore/Workflows/ShowFlow.cpp b/src/AppInstallerCLICore/Workflows/ShowFlow.cpp index 9e7029387aace..b2fb9407a25bf 100644 --- a/src/AppInstallerCLICore/Workflows/ShowFlow.cpp +++ b/src/AppInstallerCLICore/Workflows/ShowFlow.cpp @@ -12,7 +12,7 @@ namespace AppInstaller::Workflow { void ShowFlow::Execute() { - if (WorkflowBase::IndexSearch() && WorkflowBase::EnsureOneMatchFromSearchResult()) + if (IndexSearch() && EnsureOneMatchFromSearchResult()) { if (m_argsRef.Contains(ExecutionArgs::Type::ListVersions)) { @@ -27,31 +27,27 @@ namespace AppInstaller::Workflow void ShowFlow::ShowAppInfo() { - auto app = m_searchResult.Matches.at(0).Application.get(); - - auto manifest = app->GetManifest( - m_argsRef.Contains(ExecutionArgs::Type::Version) ? *m_argsRef.GetArg(ExecutionArgs::Type::Version) : "", - m_argsRef.Contains(ExecutionArgs::Type::Channel) ? *m_argsRef.GetArg(ExecutionArgs::Type::Channel) : "" - ); - - ManifestComparator manifestComparator(manifest, m_reporterRef); - auto selectedLocalization = manifestComparator.GetPreferredLocalization(m_argsRef); - auto selectedInstaller = manifestComparator.GetPreferredInstaller(m_argsRef); + if (GetManifest()) + { + SelectInstaller(); + ManifestComparator manifestComparator(m_manifest, m_reporterRef); + auto selectedLocalization = manifestComparator.GetPreferredLocalization(m_argsRef); - m_reporterRef.ShowMsg("Id: " + manifest.Id); - m_reporterRef.ShowMsg("Name: " + manifest.Name); - m_reporterRef.ShowMsg("Version: " + manifest.Version); - m_reporterRef.ShowMsg("Author: " + manifest.Author); - m_reporterRef.ShowMsg("AppMoniker: " + manifest.AppMoniker); - m_reporterRef.ShowMsg("Description: " + selectedLocalization.Description); - m_reporterRef.ShowMsg("Homepage: " + selectedLocalization.Homepage); - m_reporterRef.ShowMsg("License: " + selectedLocalization.LicenseUrl); + m_reporterRef.ShowMsg("Id: " + m_manifest.Id); + m_reporterRef.ShowMsg("Name: " + m_manifest.Name); + m_reporterRef.ShowMsg("Version: " + m_manifest.Version); + m_reporterRef.ShowMsg("Author: " + m_manifest.Author); + m_reporterRef.ShowMsg("AppMoniker: " + m_manifest.AppMoniker); + m_reporterRef.ShowMsg("Description: " + selectedLocalization.Description); + m_reporterRef.ShowMsg("Homepage: " + selectedLocalization.Homepage); + m_reporterRef.ShowMsg("License: " + selectedLocalization.LicenseUrl); - m_reporterRef.ShowMsg("Installer info:" + manifest.Id); - m_reporterRef.ShowMsg("--Installer Language: " + selectedInstaller.Language); - m_reporterRef.ShowMsg("--Installer SHA256: " + Utility::SHA256::ConvertToString(selectedInstaller.Sha256)); - m_reporterRef.ShowMsg("--Installer Download Url: " + selectedInstaller.Url); - m_reporterRef.ShowMsg("--Installer Type: " + Manifest::ManifestInstaller::InstallerTypeToString(selectedInstaller.InstallerType)); + m_reporterRef.ShowMsg("Installer info:" + m_manifest.Id); + m_reporterRef.ShowMsg("--Installer Language: " + m_selectedInstaller.Language); + m_reporterRef.ShowMsg("--Installer SHA256: " + Utility::SHA256::ConvertToString(m_selectedInstaller.Sha256)); + m_reporterRef.ShowMsg("--Installer Download Url: " + m_selectedInstaller.Url); + m_reporterRef.ShowMsg("--Installer Type: " + Manifest::ManifestInstaller::InstallerTypeToString(m_selectedInstaller.InstallerType)); + } } void ShowFlow::ShowAppVersion() diff --git a/src/AppInstallerCLICore/Workflows/ShowFlow.h b/src/AppInstallerCLICore/Workflows/ShowFlow.h index 00fc666704d6c..ce3f4580c7b64 100644 --- a/src/AppInstallerCLICore/Workflows/ShowFlow.h +++ b/src/AppInstallerCLICore/Workflows/ShowFlow.h @@ -7,12 +7,12 @@ namespace AppInstaller::Workflow { - class ShowFlow : public WorkflowBase + class ShowFlow : public SingleManifestWorkflow { public: - ShowFlow(AppInstaller::CLI::ExecutionContext& context) : WorkflowBase(context) {} + ShowFlow(AppInstaller::CLI::ExecutionContext& context) : SingleManifestWorkflow(context) {} - void Execute();; + void Execute(); protected: diff --git a/src/AppInstallerCLICore/Workflows/WorkflowBase.cpp b/src/AppInstallerCLICore/Workflows/WorkflowBase.cpp index 171042dd675e6..96934a926c670 100644 --- a/src/AppInstallerCLICore/Workflows/WorkflowBase.cpp +++ b/src/AppInstallerCLICore/Workflows/WorkflowBase.cpp @@ -1,10 +1,8 @@ // Copyright (c) Microsoft Corporation. // Licensed under the MIT License. - #include "pch.h" #include "WorkflowBase.h" -#include "Public/AppInstallerRepositorySearch.h" -#include "Public/AppInstallerRepositorySource.h" +#include "ManifestComparator.h" using namespace AppInstaller::CLI; using namespace AppInstaller::Repository; @@ -104,7 +102,30 @@ namespace AppInstaller::Workflow return true; } - bool WorkflowBase::EnsureOneMatchFromSearchResult() + void WorkflowBase::ReportSearchResult() + { + for (auto& match : m_searchResult.Matches) + { + auto app = match.Application.get(); + auto allVersions = app->GetVersions(); + + // Todo: Assume versions are sorted when returned so we'll use the first one as the latest version + // Need to call sort if the above is not the case. + std::string msg = app->GetId() + ", " + app->GetName() + ", " + allVersions.at(0).GetVersion().ToString(); + + if (match.MatchCriteria.Field != ApplicationMatchField::Id && match.MatchCriteria.Field != ApplicationMatchField::Name) + { + msg += ", ["; + msg += ApplicationMatchFieldToString(match.MatchCriteria.Field); + msg += ": " + match.MatchCriteria.Value + "]"; + } + + Logging::Telemetry().LogSearchResultCount(m_searchResult.Matches.size()); + m_reporterRef.ShowMsg(msg); + } + } + + bool SingleManifestWorkflow::EnsureOneMatchFromSearchResult() { if (m_searchResult.Matches.size() == 0) { @@ -124,26 +145,43 @@ namespace AppInstaller::Workflow return true; } - void WorkflowBase::ReportSearchResult() + bool SingleManifestWorkflow::GetManifest() { - for (auto& match : m_searchResult.Matches) - { - auto app = match.Application.get(); - auto allVersions = app->GetVersions(); + auto app = m_searchResult.Matches.at(0).Application.get(); - // Todo: Assume versions are sorted when returned so we'll use the first one as the latest version - // Need to call sort if the above is not the case. - std::string msg = app->GetId() + ", " + app->GetName() + ", " + allVersions.at(0).GetVersion().ToString(); + Logging::Telemetry().LogManifestFields(app->GetName(), app->GetId()); - if (match.MatchCriteria.Field != ApplicationMatchField::Id && match.MatchCriteria.Field != ApplicationMatchField::Name) + std::string_view version = (m_argsRef.Contains(ExecutionArgs::Type::Version) ? *m_argsRef.GetArg(ExecutionArgs::Type::Version) : std::string_view{}); + std::string_view channel = (m_argsRef.Contains(ExecutionArgs::Type::Channel) ? *m_argsRef.GetArg(ExecutionArgs::Type::Channel) : std::string_view{}); + + std::optional manifest = app->GetManifest(version, channel); + + if (!manifest) + { + std::string message = "No version found matching "; + if (!version.empty()) { - msg += ", ["; - msg += ApplicationMatchFieldToString(match.MatchCriteria.Field); - msg += ": " + match.MatchCriteria.Value + "]"; + message += version; + } + if (!channel.empty()) + { + message += '['; + message += channel; + message += ']'; } - Logging::Telemetry().LogSearchResultCount(m_searchResult.Matches.size()); - m_reporterRef.ShowMsg(msg); + m_reporterRef.ShowMsg(message, ExecutionReporter::Level::Warning); + return false; } + + m_manifest = std::move(manifest.value()); + + return true; + } + + void SingleManifestWorkflow::SelectInstaller() + { + ManifestComparator manifestComparator(m_manifest, m_reporterRef); + m_selectedInstaller = manifestComparator.GetPreferredInstaller(m_argsRef); } } \ No newline at end of file diff --git a/src/AppInstallerCLICore/Workflows/WorkflowBase.h b/src/AppInstallerCLICore/Workflows/WorkflowBase.h index 0b96bfd807fd0..3cea82b7158ad 100644 --- a/src/AppInstallerCLICore/Workflows/WorkflowBase.h +++ b/src/AppInstallerCLICore/Workflows/WorkflowBase.h @@ -22,11 +22,23 @@ namespace AppInstaller::Workflow bool IndexSearch(); - bool EnsureOneMatchFromSearchResult(); - void ReportSearchResult(); std::shared_ptr m_source; AppInstaller::Repository::SearchResult m_searchResult; }; + + // A workflow that requires a single manifest to operate properly. + class SingleManifestWorkflow : public WorkflowBase + { + protected: + using WorkflowBase::WorkflowBase; + + bool EnsureOneMatchFromSearchResult(); + bool GetManifest(); + void SelectInstaller(); + + AppInstaller::Manifest::Manifest m_manifest; + AppInstaller::Manifest::ManifestInstaller m_selectedInstaller; + }; } \ No newline at end of file diff --git a/src/AppInstallerCLICore/pch.h b/src/AppInstallerCLICore/pch.h index dd4a8285e589d..247e05d29adc8 100644 --- a/src/AppInstallerCLICore/pch.h +++ b/src/AppInstallerCLICore/pch.h @@ -31,6 +31,7 @@ #include #include #include +#include #include #include #include diff --git a/src/AppInstallerCLITests/SQLiteIndex.cpp b/src/AppInstallerCLITests/SQLiteIndex.cpp index 9ba27d815cd14..85a88199f8e19 100644 --- a/src/AppInstallerCLITests/SQLiteIndex.cpp +++ b/src/AppInstallerCLITests/SQLiteIndex.cpp @@ -721,6 +721,56 @@ TEST_CASE("SQLiteIndex_Search_VersionSorting", "[sqliteindex]") } } +TEST_CASE("SQLiteIndex_PathString_VersionSorting", "[sqliteindex]") +{ + TempFile tempFile{ "repolibtest_tempdb"s, ".db"s }; + INFO("Using temporary file named: " << tempFile.GetPath()); + + std::vector sortedList = + { + { Version("15.0.0"), Channel("") }, + { Version("14.0.0"), Channel("") }, + { Version("13.2.0-bugfix"), Channel("") }, + { Version("13.2.0"), Channel("") }, + { Version("13.0.0"), Channel("") }, + { Version("16.0.0"), Channel("alpha") }, + { Version("15.8.0"), Channel("alpha") }, + { Version("15.1.0"), Channel("beta") }, + }; + + SQLiteIndex index = SearchTestSetup(tempFile, { + { "Id", "Name", "Moniker", "14.0.0", "", { "foot" }, { "com34" }, "Path1" }, + { "Id", "Name", "Moniker", "16.0.0", "alpha", { "floor" }, { "com3" }, "Path2" }, + { "Id", "Name", "Moniker", "15.0.0", "", {}, { "Command" }, "Path3" }, + { "Id", "Name", "Moniker", "13.2.0", "", {}, { "Command" }, "Path4" }, + { "Id", "Name", "Moniker", "15.1.0", "beta", { "foo" }, { "com3" }, "Path5" }, + { "Id", "Name", "Moniker", "15.8.0", "alpha", { "foo" }, { "com3" }, "Path6" }, + { "Id", "Name", "Moniker", "13.2.0-bugfix", "", { "foo" }, { "com3" }, "Path7" }, + { "Id", "Name", "Moniker", "13.0.0", "", { "foo" }, { "com3" }, "Path8" }, + }); + + SearchRequest request; + request.Filters.emplace_back(ApplicationMatchField::Id, MatchType::Exact, "Id"); + + auto results = index.Search(request); + REQUIRE(results.size() == 1); + + auto result = index.GetPathStringByKey(results[0].first, "", ""); + REQUIRE(result.has_value()); + REQUIRE(result.value() == "Path3"); + + result = index.GetPathStringByKey(results[0].first, "", "alpha"); + REQUIRE(result.has_value()); + REQUIRE(result.value() == "Path2"); + + result = index.GetPathStringByKey(results[0].first, "", "beta"); + REQUIRE(result.has_value()); + REQUIRE(result.value() == "Path5"); + + result = index.GetPathStringByKey(results[0].first, "", "gamma"); + REQUIRE(!result.has_value()); +} + TEST_CASE("SQLiteIndex_SearchResultsTableSearches", "[sqliteindex][V1_0]") { TempFile tempFile{ "repolibtest_tempdb"s, ".db"s }; diff --git a/src/AppInstallerCLITests/SQLiteIndexSource.cpp b/src/AppInstallerCLITests/SQLiteIndexSource.cpp index ba2c921afb235..263857280a6b7 100644 --- a/src/AppInstallerCLITests/SQLiteIndexSource.cpp +++ b/src/AppInstallerCLITests/SQLiteIndexSource.cpp @@ -154,14 +154,19 @@ TEST_CASE("SQLiteIndexSource_GetManifest", "[sqliteindexsource]") IApplication* app = results.Matches[0].Application.get(); auto specificResult = app->GetManifest(manifest.Version, manifest.Channel); - REQUIRE(specificResult.Id == manifest.Id); - REQUIRE(specificResult.Name == manifest.Name); - REQUIRE(specificResult.Version == manifest.Version); - REQUIRE(specificResult.Channel == manifest.Channel); + REQUIRE(specificResult.has_value()); + REQUIRE(specificResult->Id == manifest.Id); + REQUIRE(specificResult->Name == manifest.Name); + REQUIRE(specificResult->Version == manifest.Version); + REQUIRE(specificResult->Channel == manifest.Channel); auto latestResult = app->GetManifest("", manifest.Channel); - REQUIRE(latestResult.Id == manifest.Id); - REQUIRE(latestResult.Name == manifest.Name); - REQUIRE(latestResult.Version == manifest.Version); - REQUIRE(latestResult.Channel == manifest.Channel); + REQUIRE(latestResult.has_value()); + REQUIRE(latestResult->Id == manifest.Id); + REQUIRE(latestResult->Name == manifest.Name); + REQUIRE(latestResult->Version == manifest.Version); + REQUIRE(latestResult->Channel == manifest.Channel); + + auto noResult = app->GetManifest("blargle", "flargle"); + REQUIRE(!noResult.has_value()); } diff --git a/src/AppInstallerCLITests/WorkFlow.cpp b/src/AppInstallerCLITests/WorkFlow.cpp index aeadd7ad679c6..e927b1bcaa9db 100644 --- a/src/AppInstallerCLITests/WorkFlow.cpp +++ b/src/AppInstallerCLITests/WorkFlow.cpp @@ -70,7 +70,7 @@ struct TestSource : public ISource { TestApplication(const Manifest manifest) : m_manifest(manifest) {} - Manifest GetManifest(const NormalizedString&, const NormalizedString&) override + std::optional GetManifest(const NormalizedString&, const NormalizedString&) override { return m_manifest; } diff --git a/src/AppInstallerRepositoryCore/Microsoft/SQLiteIndexSource.cpp b/src/AppInstallerRepositoryCore/Microsoft/SQLiteIndexSource.cpp index 2b5e9e116f510..aa2c18a38b296 100644 --- a/src/AppInstallerRepositoryCore/Microsoft/SQLiteIndexSource.cpp +++ b/src/AppInstallerRepositoryCore/Microsoft/SQLiteIndexSource.cpp @@ -26,10 +26,15 @@ namespace AppInstaller::Repository::Microsoft return GetSource()->GetIndex().GetNameStringById(m_id).value(); } - Manifest::Manifest GetManifest(const Utility::NormalizedString& version, const Utility::NormalizedString& channel) override + std::optional GetManifest(const Utility::NormalizedString& version, const Utility::NormalizedString& channel) override { std::shared_ptr source = GetSource(); - std::string relativePath = source->GetIndex().GetPathStringByKey(m_id, version, channel).value(); + std::optional relativePathOpt = source->GetIndex().GetPathStringByKey(m_id, version, channel); + if (!relativePathOpt) + { + return {}; + } + std::string relativePath = relativePathOpt.value(); std::string fullPath = source->GetDetails().Arg; if (fullPath.back() != '/') diff --git a/src/AppInstallerRepositoryCore/Microsoft/Schema/1_0/Interface.cpp b/src/AppInstallerRepositoryCore/Microsoft/Schema/1_0/Interface.cpp index e474555ecd98c..bcb4b1bab7462 100644 --- a/src/AppInstallerRepositoryCore/Microsoft/Schema/1_0/Interface.cpp +++ b/src/AppInstallerRepositoryCore/Microsoft/Schema/1_0/Interface.cpp @@ -98,8 +98,8 @@ namespace AppInstaller::Repository::Microsoft::Schema::V1_0 std::sort(versions.begin(), versions.end()); - // Get the first version in the list and its rowid - const std::string& latestVersion = versions[0].ToString(); + // Get the last version in the list (the highest version) and its rowid + const std::string& latestVersion = versions.back().ToString(); versionIdOpt = VersionTable::SelectIdByValue(connection, latestVersion); } else diff --git a/src/AppInstallerRepositoryCore/Public/AppInstallerRepositorySearch.h b/src/AppInstallerRepositoryCore/Public/AppInstallerRepositorySearch.h index 03a23c170373b..9b4d5598337b2 100644 --- a/src/AppInstallerRepositoryCore/Public/AppInstallerRepositorySearch.h +++ b/src/AppInstallerRepositoryCore/Public/AppInstallerRepositorySearch.h @@ -81,7 +81,7 @@ namespace AppInstaller::Repository // Gets a manifest for this application. // An empty version implies 'latest'. // An empty channel is the 'general audience'. - virtual Manifest::Manifest GetManifest(const Utility::NormalizedString& version, const Utility::NormalizedString& channel) = 0; + virtual std::optional GetManifest(const Utility::NormalizedString& version, const Utility::NormalizedString& channel) = 0; // Gets all versions of this application. // The versions will be returned in sorted, descending order.