Fix version only sorting in the manifest selection (microsoft#61)

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<InstallerHandlerBase> InstallFlow::GetInstallerHandler()
     {
         switch (m_selectedInstaller.InstallerType)

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

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()

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:
 

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::Manifest> 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);
     }
 }

src/AppInstallerCLICore/Workflows/WorkflowBase.h

@@ -22,11 +22,23 @@ namespace AppInstaller::Workflow
 
         bool IndexSearch();
 
-        bool EnsureOneMatchFromSearchResult();
-
         void ReportSearchResult();
 
         std::shared_ptr<AppInstaller::Repository::ISource> 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;
+    };
 }

src/AppInstallerCLICore/pch.h

@@ -31,6 +31,7 @@
 #include <AppInstallerErrors.h>
 #include <AppInstallerLogging.h>
 #include <AppInstallerMsixInfo.h>
+#include <AppInstallerRepositorySearch.h>
 #include <AppInstallerRepositorySource.h>
 #include <AppInstallerRuntime.h>
 #include <AppInstallerSHA256.h>

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<VersionAndChannel> 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 };

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());
 }

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<Manifest> GetManifest(const NormalizedString&, const NormalizedString&) override
         {
             return m_manifest;
         }