From a146a463c2b3017e2d66b3e80e981a6d152f7898 Mon Sep 17 00:00:00 2001 From: JohnMcPMS Date: Thu, 28 Oct 2021 11:25:59 -0700 Subject: [PATCH 1/4] Update to place tracking catalog in Source and make it available to composite --- .../Workflows/InstallFlow.cpp | 3 +- .../Workflows/UninstallFlow.cpp | 3 +- .../PackageTrackingCatalog.cpp | 51 +++++---- .../CompositeSource.cpp | 104 +++++++++++++++--- .../CompositeSource.h | 14 +-- src/AppInstallerRepositoryCore/ISource.h | 7 -- .../PackageTrackingCatalog.cpp | 6 + .../Public/winget/PackageTrackingCatalog.h | 23 +++- .../Public/winget/RepositorySource.h | 7 ++ .../RepositorySource.cpp | 45 ++++---- 10 files changed, 180 insertions(+), 83 deletions(-) diff --git a/src/AppInstallerCLICore/Workflows/InstallFlow.cpp b/src/AppInstallerCLICore/Workflows/InstallFlow.cpp index 2beb699cfc..8d3e1448b9 100644 --- a/src/AppInstallerCLICore/Workflows/InstallFlow.cpp +++ b/src/AppInstallerCLICore/Workflows/InstallFlow.cpp @@ -11,7 +11,6 @@ #include "MsiInstallFlow.h" #include "WorkflowBase.h" #include "Workflows/DependenciesFlow.h" -#include #include using namespace winrt::Windows::ApplicationModel::Store::Preview::InstallControl; @@ -683,7 +682,7 @@ namespace AppInstaller::CLI::Workflow return; } - auto trackingCatalog = PackageTrackingCatalog::CreateForSource(context.Get()->GetSource()); + auto trackingCatalog = context.Get()->GetSource().GetTrackingCatalog(); trackingCatalog.RecordInstall( context.Get(), diff --git a/src/AppInstallerCLICore/Workflows/UninstallFlow.cpp b/src/AppInstallerCLICore/Workflows/UninstallFlow.cpp index 8bf2a877c5..e1bdca3140 100644 --- a/src/AppInstallerCLICore/Workflows/UninstallFlow.cpp +++ b/src/AppInstallerCLICore/Workflows/UninstallFlow.cpp @@ -7,7 +7,6 @@ #include "AppInstallerMsixInfo.h" #include -#include using namespace AppInstaller::CLI::Execution; using namespace AppInstaller::Manifest; @@ -181,7 +180,7 @@ namespace AppInstaller::CLI::Workflow // Finally record the uninstall for each found value for (const auto& item : correlatedSources.Items) { - auto trackingCatalog = PackageTrackingCatalog::CreateForSource(item.FromSource); + auto trackingCatalog = item.FromSource.GetTrackingCatalog(); trackingCatalog.RecordUninstall(item.Identifier); } } diff --git a/src/AppInstallerCLITests/PackageTrackingCatalog.cpp b/src/AppInstallerCLITests/PackageTrackingCatalog.cpp index 7a08c94970..08ecfb4d9f 100644 --- a/src/AppInstallerCLITests/PackageTrackingCatalog.cpp +++ b/src/AppInstallerCLITests/PackageTrackingCatalog.cpp @@ -14,28 +14,41 @@ using namespace AppInstaller::Repository::Microsoft; using namespace AppInstaller::Repository::SQLite; using namespace AppInstaller::Utility; -static Source SimpleTestSetup(const std::string& filePath, SourceDetails& details, Manifest& manifest, std::string& relativePath) +namespace { - SQLiteIndex index = SQLiteIndex::CreateNew(filePath, Schema::Version::Latest()); + static Source SimpleTestSetup(const std::string& filePath, SourceDetails& details, Manifest& manifest, std::string& relativePath) + { + SQLiteIndex index = SQLiteIndex::CreateNew(filePath, Schema::Version::Latest()); - TestDataFile testManifest("Manifest-Good.yaml"); - manifest = YamlParser::CreateFromPath(testManifest); + TestDataFile testManifest("Manifest-Good.yaml"); + manifest = YamlParser::CreateFromPath(testManifest); - relativePath = testManifest.GetPath().filename().u8string(); + relativePath = testManifest.GetPath().filename().u8string(); - index.AddManifest(manifest, relativePath); + index.AddManifest(manifest, relativePath); - details.Identifier = "*SimpleTestSetup"; - details.Name = "TestName"; - details.Type = "TestType"; - details.Arg = testManifest.GetPath().parent_path().u8string(); - details.Data = ""; + details.Identifier = "*SimpleTestSetup"; + details.Name = "TestName"; + details.Type = "TestType"; + details.Arg = testManifest.GetPath().parent_path().u8string(); + details.Data = ""; - auto result = std::make_shared(details, std::move(index)); + auto result = std::make_shared(details, std::move(index)); - PackageTrackingCatalog::RemoveForSource(result->GetIdentifier()); + PackageTrackingCatalog::RemoveForSource(result->GetIdentifier()); - return { result }; + return { result }; + } + + struct TestCatalog : public PackageTrackingCatalog + { + using PackageTrackingCatalog::CreateForSource; + }; + + PackageTrackingCatalog CreatePackageTrackingCatalogForSource(const Source& source) + { + return TestCatalog::CreateForSource(source); + } } TEST_CASE("TrackingCatalog_Create", "[tracking_catalog]") @@ -48,7 +61,7 @@ TEST_CASE("TrackingCatalog_Create", "[tracking_catalog]") std::string relativePath; auto source = SimpleTestSetup(tempFile, details, manifest, relativePath); - PackageTrackingCatalog catalog = PackageTrackingCatalog::CreateForSource(source); + PackageTrackingCatalog catalog = CreatePackageTrackingCatalogForSource(source); } TEST_CASE("TrackingCatalog_Install", "[tracking_catalog]") @@ -61,7 +74,7 @@ TEST_CASE("TrackingCatalog_Install", "[tracking_catalog]") std::string relativePath; auto source = SimpleTestSetup(tempFile, details, manifest, relativePath); - PackageTrackingCatalog catalog = PackageTrackingCatalog::CreateForSource(source); + PackageTrackingCatalog catalog = CreatePackageTrackingCatalogForSource(source); SearchRequest request; request.Filters.emplace_back(PackageMatchField::Id, MatchType::Exact, manifest.Id); @@ -91,7 +104,7 @@ TEST_CASE("TrackingCatalog_Reinstall", "[tracking_catalog]") std::string relativePath; auto source = SimpleTestSetup(tempFile, details, manifest, relativePath); - PackageTrackingCatalog catalog = PackageTrackingCatalog::CreateForSource(source); + PackageTrackingCatalog catalog = CreatePackageTrackingCatalogForSource(source); SearchRequest request; request.Filters.emplace_back(PackageMatchField::Id, MatchType::Exact, manifest.Id); @@ -125,7 +138,7 @@ TEST_CASE("TrackingCatalog_Upgrade", "[tracking_catalog]") std::string relativePath; auto source = SimpleTestSetup(tempFile, details, manifest, relativePath); - PackageTrackingCatalog catalog = PackageTrackingCatalog::CreateForSource(source); + PackageTrackingCatalog catalog = CreatePackageTrackingCatalogForSource(source); SearchRequest request; request.Filters.emplace_back(PackageMatchField::Id, MatchType::Exact, manifest.Id); @@ -158,7 +171,7 @@ TEST_CASE("TrackingCatalog_Uninstall", "[tracking_catalog]") std::string relativePath; auto source = SimpleTestSetup(tempFile, details, manifest, relativePath); - PackageTrackingCatalog catalog = PackageTrackingCatalog::CreateForSource(source); + PackageTrackingCatalog catalog = CreatePackageTrackingCatalogForSource(source); SearchRequest request; request.Filters.emplace_back(PackageMatchField::Id, MatchType::Exact, manifest.Id); diff --git a/src/AppInstallerRepositoryCore/CompositeSource.cpp b/src/AppInstallerRepositoryCore/CompositeSource.cpp index 5edb31dafc..442d37819a 100644 --- a/src/AppInstallerRepositoryCore/CompositeSource.cpp +++ b/src/AppInstallerRepositoryCore/CompositeSource.cpp @@ -435,9 +435,9 @@ namespace AppInstaller::Repository } } - void CompositeSource::AddAvailableSource(std::shared_ptr source) + void CompositeSource::AddAvailableSource(const Source& source) { - m_availableSources.emplace_back(std::move(source)); + m_availableSources.emplace_back(source); } void CompositeSource::SetInstalledSource(std::shared_ptr source, CompositeSearchBehavior searchBehavior) @@ -479,13 +479,87 @@ namespace AppInstaller::Repository srs.AddToFilters(systemReferenceSearch.Inclusions); } + std::shared_ptr trackingPackage; std::shared_ptr availablePackage; + // Check the tracking catalog first to see if there is a correlation there. + // TODO: When the issue with support for multiple available packages is fixed, this should move into + // the below available sources loop as we will check all sources at that point. + //for (const auto& source : m_availableSources) + { + //auto trackingCatalog = source.GetTrackingCatalog(); + //SearchResult trackingResult; + + //try + //{ + // trackingResult = trackingCatalog.Search(systemReferenceSearch); + //} + //catch (...) + //{ + // if (result.AddFailureIfSourceNotPresent({ source->GetDetails().Name, std::current_exception() })) + // { + // LOG_CAUGHT_EXCEPTION(); + // AICLI_LOG(Repo, Warning, << "Failed to search source for correlation: " << source->GetDetails().Name); + // } + //} + + //// Move failures into the single result + //for (SearchResult::Failure& failure : availableResult.Failures) + //{ + // result.AddFailureIfSourceNotPresent(std::move(failure)); + //} + + //if (availableResult.Matches.empty()) + //{ + // continue; + //} + + //if (availableResult.Matches.size() == 1) + //{ + // availablePackage = std::move(availableResult.Matches[0].Package); + //} + //else // availableResult.Matches.size() > 1 + //{ + // AICLI_LOG(Repo, Info, + // << "Found multiple matches for installed package [" << installedVersion->GetProperty(PackageVersionProperty::Id) << + // "] in source [" << source->GetIdentifier() << "] when searching for [" << systemReferenceSearch.ToString() << "]"); + + // // More than one match found for the system reference; run some heuristics to check for a match + // for (auto&& availableMatch : availableResult.Matches) + // { + // AICLI_LOG(Repo, Info, << " Checking match with package id: " << + // availableMatch.Package->GetLatestAvailableVersion()->GetProperty(PackageVersionProperty::Id)); + + // if (IsStrongMatchField(availableMatch.MatchCriteria.Field)) + // { + // if (!availablePackage) + // { + // availablePackage = std::move(availableMatch.Package); + // } + // else + // { + // AICLI_LOG(Repo, Info, << " Found multiple packages with strong match fields"); + // availablePackage.reset(); + // break; + // } + // } + // } + + // if (!availablePackage) + // { + // AICLI_LOG(Repo, Warning, << " Appropriate available package could not be determined"); + // } + //} + + //// We found some matching packages here, don't keep going + //break; + } + // Search sources and add to result for (const auto& source : m_availableSources) { // Do not attempt to correlate local packages against this source - if (!source->GetDetails().SupportInstalledSearchCorrelation) + if (!source.GetDetails().SupportInstalledSearchCorrelation) { continue; } @@ -494,14 +568,14 @@ namespace AppInstaller::Repository try { - availableResult = source->Search(systemReferenceSearch); + availableResult = source.Search(systemReferenceSearch); } catch (...) { - if (result.AddFailureIfSourceNotPresent({ source->GetDetails().Name, std::current_exception() })) + if (result.AddFailureIfSourceNotPresent({ source.GetDetails().Name, std::current_exception() })) { LOG_CAUGHT_EXCEPTION(); - AICLI_LOG(Repo, Warning, << "Failed to search source for correlation: " << source->GetDetails().Name); + AICLI_LOG(Repo, Warning, << "Failed to search source for correlation: " << source.GetDetails().Name); } } @@ -524,7 +598,7 @@ namespace AppInstaller::Repository { AICLI_LOG(Repo, Info, << "Found multiple matches for installed package [" << installedVersion->GetProperty(PackageVersionProperty::Id) << - "] in source [" << source->GetIdentifier() << "] when searching for [" << systemReferenceSearch.ToString() << "]"); + "] in source [" << source.GetIdentifier() << "] when searching for [" << systemReferenceSearch.ToString() << "]"); // More than one match found for the system reference; run some heuristics to check for a match for (auto&& availableMatch : availableResult.Matches) @@ -575,7 +649,7 @@ namespace AppInstaller::Repository for (const auto& source : m_availableSources) { // Do not attempt to correlate local packages against this source. - if (m_searchBehavior == CompositeSearchBehavior::Installed && !source->GetDetails().SupportInstalledSearchCorrelation) + if (m_searchBehavior == CompositeSearchBehavior::Installed && !source.GetDetails().SupportInstalledSearchCorrelation) { continue; } @@ -584,13 +658,13 @@ namespace AppInstaller::Repository try { - availableResult = source->Search(request); + availableResult = source.Search(request); } catch (...) { LOG_CAUGHT_EXCEPTION(); - AICLI_LOG(Repo, Warning, << "Failed to search source: " << source->GetDetails().Name); - result.AddFailureIfSourceNotPresent({ source->GetDetails().Name, std::current_exception() }); + AICLI_LOG(Repo, Warning, << "Failed to search source: " << source.GetDetails().Name); + result.AddFailureIfSourceNotPresent({ source.GetDetails().Name, std::current_exception() }); } // Move failures into the single result @@ -634,7 +708,7 @@ namespace AppInstaller::Repository { AICLI_LOG(Repo, Info, << "Found multiple matches for available package [" << match.Package->GetProperty(PackageProperty::Id) << - "] in source [" << source->GetIdentifier() << "] when searching for [" << systemReferenceSearch.ToString() << "]"); + "] in source [" << source.GetIdentifier() << "] when searching for [" << systemReferenceSearch.ToString() << "]"); // More than one match found for the system reference; run some heuristics to check for a match for (auto&& crossRef : installedCrossRef.Matches) @@ -695,13 +769,13 @@ namespace AppInstaller::Repository try { - oneSourceResult = source->Search(request); + oneSourceResult = source.Search(request); } catch (...) { LOG_CAUGHT_EXCEPTION(); - AICLI_LOG(Repo, Warning, << "Failed to search source: " << source->GetDetails().Name); - result.Failures.emplace_back(SearchResult::Failure{ source->GetDetails().Name, std::current_exception() }); + AICLI_LOG(Repo, Warning, << "Failed to search source: " << source.GetDetails().Name); + result.Failures.emplace_back(SearchResult::Failure{ source.GetDetails().Name, std::current_exception() }); } // Move into the single result diff --git a/src/AppInstallerRepositoryCore/CompositeSource.h b/src/AppInstallerRepositoryCore/CompositeSource.h index 305d5325d5..62c24fb800 100644 --- a/src/AppInstallerRepositoryCore/CompositeSource.h +++ b/src/AppInstallerRepositoryCore/CompositeSource.h @@ -28,20 +28,16 @@ namespace AppInstaller::Repository // Must be suitable for filesystem names. const std::string& GetIdentifier() const override; - // Gets a value indicating whether this source is a composite of other sources, - // and thus the packages may come from disparate sources as well. - bool IsComposite() const override { return true; } - - // Gets the available sources if the source is composite. - std::vector> GetAvailableSources() const override { return m_availableSources; } - // Execute a search on the source. SearchResult Search(const SearchRequest& request) const override; // ~ISource // Adds an available source to be aggregated. - void AddAvailableSource(std::shared_ptr source); + void AddAvailableSource(const Source& source); + + // Gets the available sources if the source is composite. + std::vector GetAvailableSources() const { return m_availableSources; } // Checks if any available sources are present bool HasAvailableSource() const { return !m_availableSources.empty(); } @@ -57,7 +53,7 @@ namespace AppInstaller::Repository SearchResult SearchAvailable(const SearchRequest& request) const; std::shared_ptr m_installedSource; - std::vector> m_availableSources; + std::vector m_availableSources; SourceDetails m_details; CompositeSearchBehavior m_searchBehavior; }; diff --git a/src/AppInstallerRepositoryCore/ISource.h b/src/AppInstallerRepositoryCore/ISource.h index c29f58b6b4..2749b26e99 100644 --- a/src/AppInstallerRepositoryCore/ISource.h +++ b/src/AppInstallerRepositoryCore/ISource.h @@ -24,13 +24,6 @@ namespace AppInstaller::Repository // Execute a search on the source. virtual SearchResult Search(const SearchRequest& request) const = 0; - - // Gets a value indicating whether this source is a composite of other sources, - // and thus the packages may come from disparate sources as well. - virtual bool IsComposite() const { return false; } - - // Gets the available sources if the source is composite. - virtual std::vector> GetAvailableSources() const { return {}; } }; // Internal interface to represents source information; basically SourceDetails but with methods to enable differential behaviors. diff --git a/src/AppInstallerRepositoryCore/PackageTrackingCatalog.cpp b/src/AppInstallerRepositoryCore/PackageTrackingCatalog.cpp index ecc59897a9..29ad65464a 100644 --- a/src/AppInstallerRepositoryCore/PackageTrackingCatalog.cpp +++ b/src/AppInstallerRepositoryCore/PackageTrackingCatalog.cpp @@ -2,6 +2,7 @@ // Licensed under the MIT License. #include "pch.h" #include "winget/PackageTrackingCatalog.h" +#include "winget/RepositorySource.h" #include "Microsoft/SQLiteIndexSource.h" #include "AppInstallerDateTime.h" @@ -111,6 +112,11 @@ namespace AppInstaller::Repository } } + PackageTrackingCatalog::operator bool() const + { + return static_cast(m_implementation); + } + SearchResult PackageTrackingCatalog::Search(const SearchRequest& request) const { return m_implementation->Source->Search(request); diff --git a/src/AppInstallerRepositoryCore/Public/winget/PackageTrackingCatalog.h b/src/AppInstallerRepositoryCore/Public/winget/PackageTrackingCatalog.h index e893779bca..4759b970b8 100644 --- a/src/AppInstallerRepositoryCore/Public/winget/PackageTrackingCatalog.h +++ b/src/AppInstallerRepositoryCore/Public/winget/PackageTrackingCatalog.h @@ -1,16 +1,24 @@ // Copyright (c) Microsoft Corporation. // Licensed under the MIT License. #pragma once -#include +#include +#include #include namespace AppInstaller::Repository { + struct Source; + struct SearchRequest; + struct SearchResult; + enum class PackageVersionMetadata; + // A catalog for tracking package actions from a given source. struct PackageTrackingCatalog { + friend Source; + PackageTrackingCatalog(); PackageTrackingCatalog(const PackageTrackingCatalog&); PackageTrackingCatalog& operator=(const PackageTrackingCatalog&); @@ -18,13 +26,12 @@ namespace AppInstaller::Repository PackageTrackingCatalog& operator=(PackageTrackingCatalog&&) noexcept; ~PackageTrackingCatalog(); - // Creates or opens the tracking catalog for the given source. - // TODO: Make creation exclusive to the refactored Source type. - static PackageTrackingCatalog CreateForSource(const Source& source); - - // Removes the package tracking catalog for a given source. + // Removes the package tracking catalog for a given source identifier. static void RemoveForSource(const std::string& identifier); + // Determines if the curent object holds anything. + operator bool() const; + // Execute a search against the catalog. // Note that the packages in the results have the versions under "available" in order to // expose all versions contained therein (in the event that this is deemed useful at some point). @@ -57,6 +64,10 @@ namespace AppInstaller::Repository // Records an uninstall of the given package. void RecordUninstall(const Utility::LocIndString& packageIdentifier); + protected: + // Creates or opens the tracking catalog for the given source. + static PackageTrackingCatalog CreateForSource(const Source& source); + private: struct implementation; std::shared_ptr m_implementation; diff --git a/src/AppInstallerRepositoryCore/Public/winget/RepositorySource.h b/src/AppInstallerRepositoryCore/Public/winget/RepositorySource.h index 6ce7501179..f5b4b43ff8 100644 --- a/src/AppInstallerRepositoryCore/Public/winget/RepositorySource.h +++ b/src/AppInstallerRepositoryCore/Public/winget/RepositorySource.h @@ -2,6 +2,7 @@ // Licensed under the MIT License. #pragma once #include +#include #include #include @@ -174,6 +175,7 @@ namespace AppInstaller::Repository CompositeSearchBehavior searchBehavior = CompositeSearchBehavior::Installed); // Constructor for creating a Source object from an existing ISource. + // Should only be used internally by ISource implementations to return the value from IPackageVersion::GetSource. Source(std::shared_ptr source); // Bool operator to check if a source reference is successfully acquired. @@ -244,6 +246,9 @@ namespace AppInstaller::Repository // Remove source. Source remove command. bool Remove(IProgressCallback& progress); + // Gets the tracking catalog for the current source. + PackageTrackingCatalog GetTrackingCatalog() const; + // Drop source. Source reset command. static bool DropSource(std::string_view name); @@ -256,5 +261,7 @@ namespace AppInstaller::Repository std::vector> m_sourceReferences; std::shared_ptr m_source; bool m_isSourceToBeAdded = false; + bool m_isComposite = false; + mutable PackageTrackingCatalog m_trackingCatalog; }; } diff --git a/src/AppInstallerRepositoryCore/RepositorySource.cpp b/src/AppInstallerRepositoryCore/RepositorySource.cpp index d16a7068e2..ad25381068 100644 --- a/src/AppInstallerRepositoryCore/RepositorySource.cpp +++ b/src/AppInstallerRepositoryCore/RepositorySource.cpp @@ -270,11 +270,12 @@ namespace AppInstaller::Repository } m_source = compositeSource; + m_isComposite = true; } Source::Source(const Source& installedSource, const Source& availableSource, CompositeSearchBehavior searchBehavior) { - THROW_HR_IF(E_INVALIDARG, !installedSource.m_source || installedSource.m_source->IsComposite() || !availableSource.m_source); + THROW_HR_IF(E_INVALIDARG, !installedSource.m_source || installedSource.m_isComposite || !availableSource.m_source); std::shared_ptr compositeSource = std::dynamic_pointer_cast(availableSource.m_source); @@ -287,6 +288,7 @@ namespace AppInstaller::Repository compositeSource->SetInstalledSource(installedSource.m_source, searchBehavior); m_source = compositeSource; + m_isComposite = true; } Source::Source(std::shared_ptr source) : m_source(std::move(source)) {} @@ -315,13 +317,14 @@ namespace AppInstaller::Repository else { AICLI_LOG(Repo, Info, << "Default source requested, multiple sources available, adding all to source references."); - //auto aggregatedSource = std::make_shared("*DefaultSource"); for (auto& source : currentSources) { AICLI_LOG(Repo, Info, << "Adding to source references " << source.get().Name); m_sourceReferences.emplace_back(CreateSourceFromDetails(source)); } + + m_isComposite = true; } } else @@ -373,7 +376,7 @@ namespace AppInstaller::Repository SourceInformation Source::GetInformation() const { - if (m_source && !m_source->IsComposite()) + if (m_source && !m_isComposite) { return m_source->GetInformation(); } @@ -441,31 +444,17 @@ namespace AppInstaller::Repository bool Source::IsComposite() const { - if (m_source) - { - return m_source->IsComposite(); - } - else if (m_sourceReferences.size() > 0) - { - return m_sourceReferences.size() > 1; - } - else - { - THROW_HR(HRESULT_FROM_WIN32(ERROR_INVALID_STATE)); - } + return m_isComposite; } std::vector Source::GetAvailableSources() const { - THROW_HR_IF(HRESULT_FROM_WIN32(ERROR_INVALID_STATE), !m_source || !m_source->IsComposite()); + THROW_HR_IF(HRESULT_FROM_WIN32(ERROR_INVALID_STATE), !m_source || !m_isComposite); - std::vector result; - for (auto const& availableSource : m_source->GetAvailableSources()) - { - result.emplace_back(availableSource); - } + auto compositeSource = std::dynamic_pointer_cast(m_source); + THROW_HR_IF(HRESULT_FROM_WIN32(ERROR_INVALID_STATE), !compositeSource); - return result; + return compositeSource->GetAvailableSources(); } void Source::AddPackageVersion(const Manifest::Manifest& manifest, const std::filesystem::path& relativePath) @@ -554,7 +543,7 @@ namespace AppInstaller::Repository // Place all of the proxies into the source to be searched later for (auto& proxy : openExceptionProxies) { - aggregatedSource->AddAvailableSource(std::move(proxy)); + aggregatedSource->AddAvailableSource(Source{ std::move(proxy) }); } m_source = aggregatedSource; @@ -664,6 +653,16 @@ namespace AppInstaller::Repository return result; } + PackageTrackingCatalog Source::GetTrackingCatalog() const + { + if (!m_trackingCatalog) + { + m_trackingCatalog = PackageTrackingCatalog::CreateForSource(*this); + } + + return m_trackingCatalog; + } + std::vector Source::GetCurrentSources() { SourceList sourceList; From 466fda497ccff3bb5606f85226457470fe49d1e9 Mon Sep 17 00:00:00 2001 From: JohnMcPMS Date: Mon, 1 Nov 2021 11:48:52 -0700 Subject: [PATCH 2/4] Primary code done, needs tests --- src/AppInstallerCLITests/CompositeSource.cpp | 16 +- src/AppInstallerCLITests/TestSource.cpp | 5 - src/AppInstallerCLITests/TestSource.h | 2 - .../CompositeSource.cpp | 541 ++++++++++++------ .../CompositeSource.h | 1 - 5 files changed, 361 insertions(+), 204 deletions(-) diff --git a/src/AppInstallerCLITests/CompositeSource.cpp b/src/AppInstallerCLITests/CompositeSource.cpp index 02a51e2c2d..a997018ffb 100644 --- a/src/AppInstallerCLITests/CompositeSource.cpp +++ b/src/AppInstallerCLITests/CompositeSource.cpp @@ -44,7 +44,7 @@ struct CompositeTestSetup Installed = std::make_shared(); Available = std::make_shared(); Composite.SetInstalledSource(Installed); - Composite.AddAvailableSource(Available); + Composite.AddAvailableSource(Source{ Available }); } SearchResult Search() @@ -544,7 +544,7 @@ TEST_CASE("CompositeSource_MultipleAvailableSources_MatchFirst", "[CompositeSour CompositeTestSetup setup; std::shared_ptr secondAvailable = std::make_shared(); - setup.Composite.AddAvailableSource(secondAvailable); + setup.Composite.AddAvailableSource(Source{ secondAvailable }); setup.Installed->Everything.Matches.emplace_back(MakeInstalled().WithPFN(pfn), Criteria()); @@ -582,7 +582,7 @@ TEST_CASE("CompositeSource_MultipleAvailableSources_MatchSecond", "[CompositeSou CompositeTestSetup setup; std::shared_ptr secondAvailable = std::make_shared(); - setup.Composite.AddAvailableSource(secondAvailable); + setup.Composite.AddAvailableSource(Source{ secondAvailable }); setup.Installed->Everything.Matches.emplace_back(MakeInstalled().WithPFN(pfn), Criteria()); @@ -611,7 +611,7 @@ TEST_CASE("CompositeSource_MultipleAvailableSources_ReverseMatchBoth", "[Composi CompositeTestSetup setup; std::shared_ptr secondAvailable = std::make_shared(); - setup.Composite.AddAvailableSource(secondAvailable); + setup.Composite.AddAvailableSource(Source{ secondAvailable }); setup.Installed->SearchFunction = [&](const SearchRequest& request) { @@ -665,8 +665,8 @@ TEST_CASE("CompositeSource_AvailableSearchFailure", "[CompositeSource]") AvailableFails->Details.Name = "The one that fails"; CompositeSource Composite("*CompositeSource_AvailableSearchFailure"); - Composite.AddAvailableSource(AvailableSucceeds); - Composite.AddAvailableSource(AvailableFails); + Composite.AddAvailableSource(Source{ AvailableSucceeds }); + Composite.AddAvailableSource(Source{ AvailableFails }); SearchResult result = Composite.Search({}); @@ -706,7 +706,7 @@ TEST_CASE("CompositeSource_InstalledToAvailableCorrelationSearchFailure", "[Comp AvailableFails->SearchFunction = [&](const SearchRequest&) -> SearchResult { THROW_HR(expectedHR); }; AvailableFails->Details.Name = "The one that fails"; - setup.Composite.AddAvailableSource(AvailableFails); + setup.Composite.AddAvailableSource(Source{ AvailableFails }); SearchResult result = setup.Search(); @@ -746,7 +746,7 @@ TEST_CASE("CompositeSource_InstalledAvailableSearchFailure", "[CompositeSource]" AvailableFails->SearchFunction = [&](const SearchRequest&) -> SearchResult { THROW_HR(expectedHR); }; AvailableFails->Details.Name = "The one that fails"; - setup.Composite.AddAvailableSource(AvailableFails); + setup.Composite.AddAvailableSource(Source{ AvailableFails }); setup.Composite.SetInstalledSource(setup.Installed, CompositeSearchBehavior::AvailablePackages); diff --git a/src/AppInstallerCLITests/TestSource.cpp b/src/AppInstallerCLITests/TestSource.cpp index d975a82467..77c6d6322f 100644 --- a/src/AppInstallerCLITests/TestSource.cpp +++ b/src/AppInstallerCLITests/TestSource.cpp @@ -271,11 +271,6 @@ namespace TestCommon } } - bool TestSource::IsComposite() const - { - return Composite; - } - std::shared_ptr TestSourceFactory::Create(const SourceDetails& details) { if (OnOpenWithCustomHeader) diff --git a/src/AppInstallerCLITests/TestSource.h b/src/AppInstallerCLITests/TestSource.h index 69d6f8229b..f99bab03a4 100644 --- a/src/AppInstallerCLITests/TestSource.h +++ b/src/AppInstallerCLITests/TestSource.h @@ -82,12 +82,10 @@ namespace TestCommon AppInstaller::Repository::SourceInformation GetInformation() const override; AppInstaller::Repository::SearchResult Search(const AppInstaller::Repository::SearchRequest& request) const override; - bool IsComposite() const override; AppInstaller::Repository::SourceDetails Details = { "TestSource", "Microsoft.TestSource", "//arg", "", "*TestSource" }; AppInstaller::Repository::SourceInformation Information; std::function SearchFunction; - bool Composite = false; TestSource() = default; TestSource(const AppInstaller::Repository::SourceDetails& details) : Details(details) {} diff --git a/src/AppInstallerRepositoryCore/CompositeSource.cpp b/src/AppInstallerRepositoryCore/CompositeSource.cpp index 442d37819a..1692948e45 100644 --- a/src/AppInstallerRepositoryCore/CompositeSource.cpp +++ b/src/AppInstallerRepositoryCore/CompositeSource.cpp @@ -30,6 +30,138 @@ namespace AppInstaller::Repository return false; } + // Move returns if there is only one package in the matches that is strong; otherwise returns an empty value. + std::shared_ptr FindOnlyStrongMatchFieldResult(std::vector& matches) + { + std::shared_ptr result; + + for (auto&& match : matches) + { + AICLI_LOG(Repo, Info, << " Checking match with package id: " << match.Package->GetProperty(PackageProperty::Id)); + + if (IsStrongMatchField(match.MatchCriteria.Field)) + { + if (!result) + { + result = std::move(match.Package); + } + else + { + AICLI_LOG(Repo, Info, << " Found multiple packages with strong match fields"); + result.reset(); + break; + } + } + } + + return result; + } + + // Gets a single matching package from the results + template + std::shared_ptr GetMatchingPackage(std::vector& matches, MultipleIntro&& multipleIntro, Indeterminate&& indeterminate) + { + if (matches.empty()) + { + return {}; + } + else if (matches.size() == 1) + { + return std::move(matches[0].Package); + } + else + { + multipleIntro(); + + auto result = FindOnlyStrongMatchFieldResult(matches); + + if (!result) + { + indeterminate(); + } + + return result; + } + } + + // For a given package from a tracking catalog, get the latest write time. + // Look at all versions rather than just the latest to account for the potential of downgrading. + std::chrono::system_clock::time_point GetLatestTrackingPackageWriteTime(const std::shared_ptr& trackingPackage) + { + std::chrono::system_clock::time_point result{}; + + for (const auto& key : trackingPackage->GetAvailableVersionKeys()) + { + auto version = trackingPackage->GetAvailableVersion(key); + if (version) + { + auto metadata = version->GetMetadata(); + auto itr = metadata.find(PackageVersionMetadata::TrackingWriteTime); + if (itr != metadata.end()) + { + std::int64_t unixEpoch = 0; + try + { + unixEpoch = std::stoll(itr->second); + } + CATCH_LOG(); + + std::chrono::system_clock::time_point versionTime = Utility::ConvertUnixEpochToSystemClock(unixEpoch); + + if (versionTime > result) + { + result = versionTime; + } + } + } + } + + return result; + } + + // A composite package installed version that allows us to override the source of the version. + struct CompositeInstalledVersion : public IPackageVersion + { + CompositeInstalledVersion(std::shared_ptr baseInstalledVersion, Source trackingSource) : + m_baseInstalledVersion(std::move(baseInstalledVersion)), m_trackingSource(std::move(trackingSource)) + {} + + Utility::LocIndString GetProperty(PackageVersionProperty property) const override + { + return m_baseInstalledVersion->GetProperty(property); + } + + std::vector GetMultiProperty(PackageVersionMultiProperty property) const override + { + return m_baseInstalledVersion->GetMultiProperty(property); + } + + Manifest::Manifest GetManifest() override + { + return m_baseInstalledVersion->GetManifest(); + } + + Source GetSource() const override + { + // If there is a tracking source, use it instead to indicate that it came from there. + if (m_trackingSource) + { + return m_trackingSource; + } + + return m_baseInstalledVersion->GetSource(); + } + + Metadata GetMetadata() const override + { + return m_baseInstalledVersion->GetMetadata(); + } + + private: + std::shared_ptr m_baseInstalledVersion; + Source m_trackingSource; + }; + // A composite package for the CompositeSource. struct CompositePackage : public IPackage { @@ -50,6 +182,10 @@ namespace AppInstaller::Repository Utility::LocIndString GetProperty(PackageProperty property) const override { std::shared_ptr truth = GetLatestAvailableVersion(); + if (!truth && m_trackingPackage) + { + truth = m_trackingPackage->GetLatestAvailableVersion(); + } if (!truth) { truth = GetInstalledVersion(); @@ -70,7 +206,14 @@ namespace AppInstaller::Repository { if (m_installedPackage) { - return m_installedPackage->GetInstalledVersion(); + if (m_trackingSource) + { + return std::make_shared(m_installedPackage->GetInstalledVersion(), m_trackingSource); + } + else + { + return m_installedPackage->GetInstalledVersion(); + } } return {}; @@ -149,15 +292,28 @@ namespace AppInstaller::Repository return m_availablePackage; } + const std::shared_ptr& GetTrackingPackage() + { + return m_trackingPackage; + } + void SetAvailablePackage(std::shared_ptr availablePackage) { m_availablePackage = std::move(availablePackage); } + void SetTracking(Source trackingSource, std::shared_ptr trackingPackage) + { + m_trackingSource = std::move(trackingSource); + m_trackingPackage = std::move(trackingPackage); + } + private: std::shared_ptr m_installedPackage; Utility::LocIndString m_installedChannel; std::shared_ptr m_availablePackage; + Source m_trackingSource; + std::shared_ptr m_trackingPackage; }; // The comparator compares the ResultMatch by MatchType first, then Field in a predefined order. @@ -260,6 +416,16 @@ namespace AppInstaller::Repository SystemReferenceStrings.emplace(std::move(srs)); } } + + SearchRequest CreateInclusionsSearchRequest() const + { + SearchRequest result; + for (const auto& srs : SystemReferenceStrings) + { + srs.AddToFilters(result.Inclusions); + } + return result; + } }; // For a given package version, prepares the results for it. @@ -298,6 +464,34 @@ namespace AppInstaller::Repository return result; } + // Check for a package already in the result that should have been correlated already. + // If we find one, see if we should upgrade it's match criteria. + // If we don't, return package data for further use. + std::optional CheckForExistingResultFromTrackingPackageMatch(const ResultMatch& trackingMatch) + { + for (auto& match : Matches) + { + const std::shared_ptr& trackingPackage = match.Package->GetTrackingPackage(); + if (trackingPackage && trackingPackage->IsSame(trackingMatch.Package.get())) + { + if (ResultMatchComparator{}(trackingMatch, match)) + { + match.MatchCriteria = trackingMatch.MatchCriteria; + } + + return {}; + } + } + + PackageData result; + for (auto const& versionKey : trackingMatch.Package->GetAvailableVersionKeys()) + { + auto packageVersion = trackingMatch.Package->GetAvailableVersion(versionKey); + AddSystemReferenceStrings(packageVersion.get(), result); + } + return result; + } + // Determines if the results contain the given installed package. bool ContainsInstalledPackage(const IPackage* installedPackage) { @@ -347,6 +541,32 @@ namespace AppInstaller::Repository return false; } + SearchResult SearchAndHandleFailures(const Source& source, const SearchRequest& request) + { + SearchResult result; + + try + { + result = source.Search(request); + } + catch (...) + { + if (AddFailureIfSourceNotPresent({ source.GetDetails().Name, std::current_exception() })) + { + LOG_CAUGHT_EXCEPTION(); + AICLI_LOG(Repo, Warning, << "Failed to search source for correlation: " << source.GetDetails().Name); + } + } + + // Move failures into the result + for (SearchResult::Failure& failure : result.Failures) + { + AddFailureIfSourceNotPresent(std::move(failure)); + } + + return result; + } + std::vector Matches; bool Truncated = false; std::vector Failures; @@ -399,6 +619,29 @@ namespace AppInstaller::Repository } } }; + + std::shared_ptr GetTrackedPackageFromAvailableSource(CompositeResult& result, const Source& source, const Utility::LocIndString& identifier) + { + SearchRequest directRequest; + directRequest.Filters.emplace_back(PackageMatchField::Id, MatchType::CaseInsensitive, identifier.get()); + + SearchResult directResult = result.SearchAndHandleFailures(source, directRequest); + + if (directResult.Matches.empty()) + { + AICLI_LOG(Repo, Warning, << "Did not find Id [" << identifier << "] in tracked source: " << source.GetDetails().Name); + } + else if (directResult.Matches.size() == 1) + { + return std::move(directResult.Matches[0].Package); + } + else + { + AICLI_LOG(Repo, Warning, << "Found multiple results for Id [" << identifier << "] in tracked source: " << source.GetDetails().Name); + } + + return {}; + } } CompositeSource::CompositeSource(std::string identifier) @@ -466,172 +709,90 @@ namespace AppInstaller::Repository { auto compositePackage = std::make_shared(std::move(match.Package)); - // Create a search request to run against all available sources - SearchRequest systemReferenceSearch; - auto installedVersion = compositePackage->GetInstalledVersion(); auto installedPackageData = result.GetSystemReferenceStrings(installedVersion.get()); + // Create a search request to run against all available sources if (!installedPackageData.SystemReferenceStrings.empty()) { - for (const auto& srs : installedPackageData.SystemReferenceStrings) - { - srs.AddToFilters(systemReferenceSearch.Inclusions); - } + SearchRequest systemReferenceSearch = installedPackageData.CreateInclusionsSearchRequest(); + Source trackedSource; std::shared_ptr trackingPackage; + std::chrono::system_clock::time_point trackingPackageTime; std::shared_ptr availablePackage; // Check the tracking catalog first to see if there is a correlation there. // TODO: When the issue with support for multiple available packages is fixed, this should move into // the below available sources loop as we will check all sources at that point. - //for (const auto& source : m_availableSources) - { - //auto trackingCatalog = source.GetTrackingCatalog(); - //SearchResult trackingResult; - - //try - //{ - // trackingResult = trackingCatalog.Search(systemReferenceSearch); - //} - //catch (...) - //{ - // if (result.AddFailureIfSourceNotPresent({ source->GetDetails().Name, std::current_exception() })) - // { - // LOG_CAUGHT_EXCEPTION(); - // AICLI_LOG(Repo, Warning, << "Failed to search source for correlation: " << source->GetDetails().Name); - // } - //} - - //// Move failures into the single result - //for (SearchResult::Failure& failure : availableResult.Failures) - //{ - // result.AddFailureIfSourceNotPresent(std::move(failure)); - //} - - //if (availableResult.Matches.empty()) - //{ - // continue; - //} - - //if (availableResult.Matches.size() == 1) - //{ - // availablePackage = std::move(availableResult.Matches[0].Package); - //} - //else // availableResult.Matches.size() > 1 - //{ - // AICLI_LOG(Repo, Info, - // << "Found multiple matches for installed package [" << installedVersion->GetProperty(PackageVersionProperty::Id) << - // "] in source [" << source->GetIdentifier() << "] when searching for [" << systemReferenceSearch.ToString() << "]"); - - // // More than one match found for the system reference; run some heuristics to check for a match - // for (auto&& availableMatch : availableResult.Matches) - // { - // AICLI_LOG(Repo, Info, << " Checking match with package id: " << - // availableMatch.Package->GetLatestAvailableVersion()->GetProperty(PackageVersionProperty::Id)); - - // if (IsStrongMatchField(availableMatch.MatchCriteria.Field)) - // { - // if (!availablePackage) - // { - // availablePackage = std::move(availableMatch.Package); - // } - // else - // { - // AICLI_LOG(Repo, Info, << " Found multiple packages with strong match fields"); - // availablePackage.reset(); - // break; - // } - // } - // } - - // if (!availablePackage) - // { - // AICLI_LOG(Repo, Warning, << " Appropriate available package could not be determined"); - // } - //} - - //// We found some matching packages here, don't keep going - //break; - } - - // Search sources and add to result for (const auto& source : m_availableSources) { - // Do not attempt to correlate local packages against this source - if (!source.GetDetails().SupportInstalledSearchCorrelation) + auto trackingCatalog = source.GetTrackingCatalog(); + SearchResult trackingResult = trackingCatalog.Search(systemReferenceSearch); + + std::shared_ptr candidatePackage = GetMatchingPackage(trackingResult.Matches, + [&]() { + AICLI_LOG(Repo, Info, + << "Found multiple matches for installed package [" << installedVersion->GetProperty(PackageVersionProperty::Id) << + "] in tracking catalog for source [" << source.GetIdentifier() << "] when searching for [" << systemReferenceSearch.ToString() << "]"); + }, [&] { + AICLI_LOG(Repo, Warning, << " Appropriate tracking package could not be determined"); + }); + + // Determine the candidate package with the latest install time + if (candidatePackage) { - continue; - } - - SearchResult availableResult; + std::chrono::system_clock::time_point candidateTime = GetLatestTrackingPackageWriteTime(candidatePackage); - try - { - availableResult = source.Search(systemReferenceSearch); - } - catch (...) - { - if (result.AddFailureIfSourceNotPresent({ source.GetDetails().Name, std::current_exception() })) + if (!trackingPackage || candidateTime > trackingPackageTime) { - LOG_CAUGHT_EXCEPTION(); - AICLI_LOG(Repo, Warning, << "Failed to search source for correlation: " << source.GetDetails().Name); + trackedSource = source; + trackingPackage = std::move(candidatePackage); + trackingPackageTime = candidateTime; } } + } - // Move failures into the single result - for (SearchResult::Failure& failure : availableResult.Failures) - { - result.AddFailureIfSourceNotPresent(std::move(failure)); - } - - if (availableResult.Matches.empty()) - { - continue; - } + // Directly search for the available package from tracking information. + if (trackingPackage) + { + availablePackage = GetTrackedPackageFromAvailableSource(result, trackedSource, trackingPackage->GetProperty(PackageProperty::Id)); + } - if (availableResult.Matches.size() == 1) - { - availablePackage = std::move(availableResult.Matches[0].Package); - } - else // availableResult.Matches.size() > 1 + if (!availablePackage) + { + // Search sources and add to result + for (const auto& source : m_availableSources) { - AICLI_LOG(Repo, Info, - << "Found multiple matches for installed package [" << installedVersion->GetProperty(PackageVersionProperty::Id) << - "] in source [" << source.GetIdentifier() << "] when searching for [" << systemReferenceSearch.ToString() << "]"); - - // More than one match found for the system reference; run some heuristics to check for a match - for (auto&& availableMatch : availableResult.Matches) + // Do not attempt to correlate local packages against this source + if (!source.GetDetails().SupportInstalledSearchCorrelation) { - AICLI_LOG(Repo, Info, << " Checking match with package id: " << - availableMatch.Package->GetLatestAvailableVersion()->GetProperty(PackageVersionProperty::Id)); - - if (IsStrongMatchField(availableMatch.MatchCriteria.Field)) - { - if (!availablePackage) - { - availablePackage = std::move(availableMatch.Package); - } - else - { - AICLI_LOG(Repo, Info, << " Found multiple packages with strong match fields"); - availablePackage.reset(); - break; - } - } + continue; } - if (!availablePackage) + SearchResult availableResult = result.SearchAndHandleFailures(source, systemReferenceSearch); + + if (availableResult.Matches.empty()) { - AICLI_LOG(Repo, Warning, << " Appropriate available package could not be determined"); + continue; } - } - // We found some matching packages here, don't keep going - break; + availablePackage = GetMatchingPackage(availableResult.Matches, + [&]() { + AICLI_LOG(Repo, Info, + << "Found multiple matches for installed package [" << installedVersion->GetProperty(PackageVersionProperty::Id) << + "] in source [" << source.GetIdentifier() << "] when searching for [" << systemReferenceSearch.ToString() << "]"); + }, [&] { + AICLI_LOG(Repo, Warning, << " Appropriate available package could not be determined"); + }); + + // We found some matching packages here, don't keep going + break; + } } compositePackage->SetAvailablePackage(std::move(availablePackage)); + compositePackage->SetTracking(std::move(trackedSource), std::move(trackingPackage)); } // Move the installed result into the composite result @@ -648,31 +809,61 @@ namespace AppInstaller::Repository // Search available sources for (const auto& source : m_availableSources) { - // Do not attempt to correlate local packages against this source. - if (m_searchBehavior == CompositeSearchBehavior::Installed && !source.GetDetails().SupportInstalledSearchCorrelation) + // Search the tracking catalog as it can potentially get better correlations + auto trackingCatalog = source.GetTrackingCatalog(); + SearchResult trackingResult = trackingCatalog.Search(request); + + for (auto&& match : trackingResult.Matches) { - continue; - } + // Check for a package already in the result that should have been correlated already. + auto packageData = result.CheckForExistingResultFromTrackingPackageMatch(match); + + // If found existing package in the result, continue + if (!packageData) + { + continue; + } - SearchResult availableResult; + // If no package was found that was already in the results, do a correlation lookup with the installed + // source to create a new composite package entry if we find any packages there. + if (packageData && !packageData->SystemReferenceStrings.empty()) + { + // Create a search request to run against the installed source + SearchRequest systemReferenceSearch = packageData->CreateInclusionsSearchRequest(); - try - { - availableResult = source.Search(request); - } - catch (...) - { - LOG_CAUGHT_EXCEPTION(); - AICLI_LOG(Repo, Warning, << "Failed to search source: " << source.GetDetails().Name); - result.AddFailureIfSourceNotPresent({ source.GetDetails().Name, std::current_exception() }); + // Correlate against installed (allow exceptions out as we own the installed source) + SearchResult installedCrossRef = m_installedSource->Search(systemReferenceSearch); + + std::shared_ptr installedPackage = GetMatchingPackage(installedCrossRef.Matches, + [&]() { + AICLI_LOG(Repo, Info, + << "Found multiple matches for tracking package [" << match.Package->GetProperty(PackageProperty::Id) << + "] in source [" << source.GetIdentifier() << "] when searching for [" << systemReferenceSearch.ToString() << "]"); + }, [&] { + AICLI_LOG(Repo, Warning, << " Appropriate installed package could not be determined"); + }); + + if (installedPackage && !result.ContainsInstalledPackage(installedPackage.get())) + { + auto compositePackage = std::make_shared( + std::move(installedPackage), + GetTrackedPackageFromAvailableSource(result, source, match.Package->GetProperty(PackageProperty::Id))); + + compositePackage->SetTracking(source, std::move(match.Package)); + + result.Matches.emplace_back(std::move(compositePackage), match.MatchCriteria); + } + } } - // Move failures into the single result - for (auto& failure : availableResult.Failures) + // Do not attempt to correlate local packages against this source. + if (m_searchBehavior == CompositeSearchBehavior::Installed && !source.GetDetails().SupportInstalledSearchCorrelation) { - result.AddFailureIfSourceNotPresent(std::move(failure)); + continue; } + SearchResult availableResult = result.SearchAndHandleFailures(source, request); + for (auto&& match : availableResult.Matches) { // Check for a package already in the result that should have been correlated already. @@ -690,45 +881,19 @@ namespace AppInstaller::Repository if (packageData && !packageData->SystemReferenceStrings.empty()) { // Create a search request to run against the installed source - SearchRequest systemReferenceSearch; - for (const auto& srs : packageData->SystemReferenceStrings) - { - srs.AddToFilters(systemReferenceSearch.Inclusions); - } + SearchRequest systemReferenceSearch = packageData->CreateInclusionsSearchRequest(); // Correlate against installed (allow exceptions out as we own the installed source) SearchResult installedCrossRef = m_installedSource->Search(systemReferenceSearch); - std::shared_ptr installedPackage; - if (installedCrossRef.Matches.size() == 1) - { - installedPackage = std::move(installedCrossRef.Matches[0].Package); - } - else if (installedCrossRef.Matches.size() > 1) - { - AICLI_LOG(Repo, Info, - << "Found multiple matches for available package [" << match.Package->GetProperty(PackageProperty::Id) << - "] in source [" << source.GetIdentifier() << "] when searching for [" << systemReferenceSearch.ToString() << "]"); - - // More than one match found for the system reference; run some heuristics to check for a match - for (auto&& crossRef : installedCrossRef.Matches) - { - AICLI_LOG(Repo, Info, << " Checking match with package id: " << crossRef.Package->GetProperty(PackageProperty::Id)); - if (IsStrongMatchField(crossRef.MatchCriteria.Field)) - { - if (!installedPackage) - { - installedPackage = std::move(crossRef.Package); - } - else - { - AICLI_LOG(Repo, Info, << " Found multiple packages with strong match fields"); - installedPackage.reset(); - break; - } - } - } - } + std::shared_ptr installedPackage = GetMatchingPackage(installedCrossRef.Matches, + [&]() { + AICLI_LOG(Repo, Info, + << "Found multiple matches for available package [" << match.Package->GetProperty(PackageProperty::Id) << + "] in source [" << source.GetIdentifier() << "] when searching for [" << systemReferenceSearch.ToString() << "]"); + }, [&] { + AICLI_LOG(Repo, Warning, << " Appropriate installed package could not be determined"); + }); if (installedPackage && !result.ContainsInstalledPackage(installedPackage.get())) { diff --git a/src/AppInstallerRepositoryCore/CompositeSource.h b/src/AppInstallerRepositoryCore/CompositeSource.h index 62c24fb800..f426d75203 100644 --- a/src/AppInstallerRepositoryCore/CompositeSource.h +++ b/src/AppInstallerRepositoryCore/CompositeSource.h @@ -1,6 +1,5 @@ // Copyright (c) Microsoft Corporation. // Licensed under the MIT License. - #pragma once #include "ISource.h" From c5b60dddb1665d41b282fd72102d85f42b7aa3eb Mon Sep 17 00:00:00 2001 From: JohnMcPMS Date: Wed, 3 Nov 2021 15:16:14 -0700 Subject: [PATCH 3/4] Add tests and testability --- src/AppInstallerCLITests/CompositeSource.cpp | 178 +++++++++++++++++- src/AppInstallerCLITests/main.cpp | 23 +++ .../AppInstallerRepositoryCore.vcxproj | 1 + ...AppInstallerRepositoryCore.vcxproj.filters | 3 + .../CompositeSource.cpp | 20 ++ .../PackageTrackingCatalog.cpp | 157 +++++++++++---- .../PackageTrackingCatalogSourceFactory.h | 25 +++ .../Public/winget/PackageTrackingCatalog.h | 2 +- .../RepositorySource.cpp | 92 ++++----- .../SourceFactory.h | 3 + 10 files changed, 418 insertions(+), 86 deletions(-) create mode 100644 src/AppInstallerRepositoryCore/PackageTrackingCatalogSourceFactory.h diff --git a/src/AppInstallerCLITests/CompositeSource.cpp b/src/AppInstallerCLITests/CompositeSource.cpp index a997018ffb..d0a9c71297 100644 --- a/src/AppInstallerCLITests/CompositeSource.cpp +++ b/src/AppInstallerCLITests/CompositeSource.cpp @@ -3,13 +3,17 @@ #include "pch.h" #include "TestCommon.h" #include "TestSource.h" +#include "TestHooks.h" #include +#include +#include using namespace std::string_literals; using namespace std::string_view_literals; using namespace TestCommon; using namespace AppInstaller; using namespace AppInstaller::Repository; +using namespace AppInstaller::Repository::Microsoft; using namespace AppInstaller::Utility; constexpr std::string_view s_Everything_Query = "everything"sv; @@ -21,6 +25,13 @@ constexpr std::string_view s_Everything_Query = "everything"sv; // enable verification of expectations. struct ComponentTestSource : public TestSource { + ComponentTestSource() = default; + + ComponentTestSource(std::string_view identifier) + { + Details.Identifier = identifier; + } + SearchResult Search(const SearchRequest& request) const override { if (request.Query && request.Query.value().Value == s_Everything_Query) @@ -41,8 +52,8 @@ struct CompositeTestSetup { CompositeTestSetup() : Composite("*Tests") { - Installed = std::make_shared(); - Available = std::make_shared(); + Installed = std::make_shared("InstalledTestSource1"); + Available = std::make_shared("AvailableTestSource1"); Composite.SetInstalledSource(Installed); Composite.AddAvailableSource(Source{ Available }); } @@ -59,6 +70,24 @@ struct CompositeTestSetup CompositeSource Composite; }; +// A helper to create the sources used by the majority of tests in this file. +struct CompositeWithTrackingTestSetup : public CompositeTestSetup +{ + CompositeWithTrackingTestSetup() : TrackingFactory([&](const SourceDetails&) { return Tracking; }) + { + Tracking = std::make_shared(SourceDetails{}, SQLiteIndex::CreateNew(SQLITE_MEMORY_DB_CONNECTION_TARGET)); + TestHook_SetSourceFactoryOverride(std::string{ PackageTrackingCatalogSourceFactory::Type() }, TrackingFactory); + } + + ~CompositeWithTrackingTestSetup() + { + TestHook_ClearSourceFactoryOverrides(); + } + + TestSourceFactory TrackingFactory; + std::shared_ptr Tracking; +}; + // A helper to make matches. struct Criteria : public PackageMatchFilter { @@ -95,9 +124,9 @@ struct TestPackageHelper return *this; } - TestPackageHelper& WithDefaultName(const std::string& name) + TestPackageHelper& WithDefaultName(std::string_view name) { - m_manifest.DefaultLocalization.Add(name); + m_manifest.DefaultLocalization.Add(std::string{ name }); return *this; } @@ -130,6 +159,11 @@ struct TestPackageHelper return m_package; } + operator const Manifest::Manifest& () const + { + return m_manifest; + } + private: bool m_isInstalled; Manifest::Manifest m_manifest; @@ -772,3 +806,139 @@ TEST_CASE("CompositeSource_InstalledAvailableSearchFailure", "[CompositeSource]" REQUIRE(searchFailure == expectedHR); } + +TEST_CASE("CompositeSource_TrackingPackageFound", "[CompositeSource]") +{ + std::string availableID = "Available.ID"; + std::string pfn = "sortof_apfn"; + + auto installedPackage = MakeInstalled().WithPFN(pfn); + auto availablePackage = MakeAvailable().WithPFN(pfn).WithId(availableID).WithDefaultName(s_Everything_Query); + + CompositeWithTrackingTestSetup setup; + setup.Installed->Everything.Matches.emplace_back(installedPackage, Criteria()); + setup.Installed->SearchFunction = [&](const SearchRequest& request) + { + RequireIncludes(request.Inclusions, PackageMatchField::PackageFamilyName, MatchType::Exact, pfn); + + SearchResult result; + result.Matches.emplace_back(installedPackage, Criteria()); + return result; + }; + + setup.Available->Everything.Matches.emplace_back(availablePackage, Criteria()); + setup.Available->SearchFunction = [&](const SearchRequest& request) + { + if (request.Filters.empty()) + { + RequireIncludes(request.Inclusions, PackageMatchField::PackageFamilyName, MatchType::Exact, pfn); + } + else + { + REQUIRE(request.Filters.size() == 1); + RequireIncludes(request.Filters, PackageMatchField::Id, MatchType::CaseInsensitive, availableID); + } + + SearchResult result; + result.Matches.emplace_back(availablePackage, Criteria()); + return result; + }; + + setup.Tracking->GetIndex().AddManifest(availablePackage); + + SearchResult result = setup.Search(); + + REQUIRE(result.Matches.size() == 1); + REQUIRE(result.Matches[0].Package); + REQUIRE(result.Matches[0].Package->GetInstalledVersion()); + REQUIRE(result.Matches[0].Package->GetInstalledVersion()->GetSource().GetIdentifier() == setup.Available->Details.Identifier); + REQUIRE(result.Matches[0].Package->GetLatestAvailableVersion()); +} + +TEST_CASE("CompositeSource_TrackingFound_AvailableNot", "[CompositeSource]") +{ + std::string availableID = "Available.ID"; + std::string pfn = "sortof_apfn"; + + auto installedPackage = MakeInstalled().WithPFN(pfn); + auto availablePackage = MakeAvailable().WithPFN(pfn).WithId(availableID).WithDefaultName(s_Everything_Query); + + CompositeWithTrackingTestSetup setup; + setup.Installed->Everything.Matches.emplace_back(installedPackage, Criteria()); + setup.Installed->SearchFunction = [&](const SearchRequest& request) + { + RequireIncludes(request.Inclusions, PackageMatchField::PackageFamilyName, MatchType::Exact, pfn); + + SearchResult result; + result.Matches.emplace_back(installedPackage, Criteria()); + return result; + }; + + setup.Tracking->GetIndex().AddManifest(availablePackage); + + SearchResult result = setup.Search(); + + REQUIRE(result.Matches.size() == 1); + REQUIRE(result.Matches[0].Package); + REQUIRE(result.Matches[0].Package->GetInstalledVersion()); + REQUIRE(result.Matches[0].Package->GetInstalledVersion()->GetSource().GetIdentifier() == setup.Available->Details.Identifier); + REQUIRE(!result.Matches[0].Package->GetLatestAvailableVersion()); +} + +TEST_CASE("CompositeSource_TrackingFound_AvailablePath", "[CompositeSource]") +{ + std::string availableID = "Available.ID"; + std::string pfn = "sortof_apfn"; + + auto installedPackage = MakeInstalled().WithPFN(pfn); + auto availablePackage = MakeAvailable().WithPFN(pfn).WithId(availableID).WithDefaultName(s_Everything_Query); + + CompositeWithTrackingTestSetup setup; + setup.Installed->SearchFunction = [&](const SearchRequest& request) + { + RequireIncludes(request.Inclusions, PackageMatchField::PackageFamilyName, MatchType::Exact, pfn); + + SearchResult result; + result.Matches.emplace_back(installedPackage, Criteria()); + return result; + }; + + setup.Available->Everything.Matches.emplace_back(availablePackage, Criteria()); + setup.Available->SearchFunction = [&](const SearchRequest& request) + { + REQUIRE(request.Filters.size() == 1); + RequireIncludes(request.Filters, PackageMatchField::Id, MatchType::CaseInsensitive, availableID); + + SearchResult result; + result.Matches.emplace_back(availablePackage, Criteria()); + return result; + }; + + setup.Tracking->GetIndex().AddManifest(availablePackage); + + SearchResult result = setup.Search(); + + REQUIRE(result.Matches.size() == 1); + REQUIRE(result.Matches[0].Package); + REQUIRE(result.Matches[0].Package->GetInstalledVersion()); + REQUIRE(result.Matches[0].Package->GetInstalledVersion()->GetSource().GetIdentifier() == setup.Available->Details.Identifier); + REQUIRE(result.Matches[0].Package->GetLatestAvailableVersion()); +} + +TEST_CASE("CompositeSource_TrackingFound_NotInstalled", "[CompositeSource]") +{ + std::string availableID = "Available.ID"; + std::string pfn = "sortof_apfn"; + + auto installedPackage = MakeInstalled().WithPFN(pfn); + auto availablePackage = MakeAvailable().WithPFN(pfn).WithId(availableID).WithDefaultName(s_Everything_Query); + + CompositeWithTrackingTestSetup setup; + setup.Available->Everything.Matches.emplace_back(availablePackage, Criteria()); + + setup.Tracking->GetIndex().AddManifest(availablePackage); + + SearchResult result = setup.Search(); + + REQUIRE(result.Matches.empty()); +} diff --git a/src/AppInstallerCLITests/main.cpp b/src/AppInstallerCLITests/main.cpp index b8c79633a2..85ae9083f9 100644 --- a/src/AppInstallerCLITests/main.cpp +++ b/src/AppInstallerCLITests/main.cpp @@ -47,6 +47,27 @@ struct LoggingBreakListener : public Catch::TestEventListenerBase }; CATCH_REGISTER_LISTENER(LoggingBreakListener); +// Map CATCH exceptions so that WIL doesn't fail fast the tests +HRESULT __stdcall CatchResultFromCaughtException() WI_PFN_NOEXCEPT +{ + try + { + throw; + } + catch (const Catch::TestFailureException&) + { + // REC_E_TEST_FAILURE :: Test failure. + // Not sure what could generate this, but it is unlikely that we use it. + // Since the message is aligned with the issue it should help diagnosis. + return 0x8b051032; + } + catch (...) + { + // Means we couldn't map the exception + return S_OK; + } +} + int main(int argc, char** argv) { init_apartment(); @@ -123,6 +144,8 @@ int main(int argc, char** argv) Logging::Log().SetLevel(Logging::Level::Verbose); Logging::EnableWilFailureTelemetry(); + wil::SetResultFromCaughtExceptionCallback(CatchResultFromCaughtException); + // Forcibly enable event writing to catch bugs in that code g_IsTelemetryProviderEnabled = true; diff --git a/src/AppInstallerRepositoryCore/AppInstallerRepositoryCore.vcxproj b/src/AppInstallerRepositoryCore/AppInstallerRepositoryCore.vcxproj index 7fdacc4e1f..ea61b97a93 100644 --- a/src/AppInstallerRepositoryCore/AppInstallerRepositoryCore.vcxproj +++ b/src/AppInstallerRepositoryCore/AppInstallerRepositoryCore.vcxproj @@ -266,6 +266,7 @@ + diff --git a/src/AppInstallerRepositoryCore/AppInstallerRepositoryCore.vcxproj.filters b/src/AppInstallerRepositoryCore/AppInstallerRepositoryCore.vcxproj.filters index 8a7e0ccaa3..b15978d088 100644 --- a/src/AppInstallerRepositoryCore/AppInstallerRepositoryCore.vcxproj.filters +++ b/src/AppInstallerRepositoryCore/AppInstallerRepositoryCore.vcxproj.filters @@ -249,6 +249,9 @@ Header Files + + Header Files + diff --git a/src/AppInstallerRepositoryCore/CompositeSource.cpp b/src/AppInstallerRepositoryCore/CompositeSource.cpp index 1692948e45..04d842a012 100644 --- a/src/AppInstallerRepositoryCore/CompositeSource.cpp +++ b/src/AppInstallerRepositoryCore/CompositeSource.cpp @@ -693,6 +693,26 @@ namespace AppInstaller::Repository // Next the search is performed against the available sources and correlated with the installed source. A result will only // be added if there exists an installed package that was not found by the initial search. // This allows for search terms to find installed packages by their available metadata, as well as the local values. + // + // Search flow: + // Installed :: Search incoming request + // For each result + // For each available source + // Tracking :: Search system references + // If tracking found + // Available :: Search tracking ID + // If no available, for each available source + // Available :: Search system references + // + // For each available source + // Tracking :: Search incoming request + // For each result + // Installed :: Search system references + // If found + // Available :: Search tracking ID + // Available :: Search incoming request + // For each result + // Installed :: Search system references SearchResult CompositeSource::SearchInstalled(const SearchRequest& request) const { CompositeResult result; diff --git a/src/AppInstallerRepositoryCore/PackageTrackingCatalog.cpp b/src/AppInstallerRepositoryCore/PackageTrackingCatalog.cpp index 29ad65464a..7efe7c4704 100644 --- a/src/AppInstallerRepositoryCore/PackageTrackingCatalog.cpp +++ b/src/AppInstallerRepositoryCore/PackageTrackingCatalog.cpp @@ -2,6 +2,7 @@ // Licensed under the MIT License. #include "pch.h" #include "winget/PackageTrackingCatalog.h" +#include "PackageTrackingCatalogSourceFactory.h" #include "winget/RepositorySource.h" #include "Microsoft/SQLiteIndexSource.h" #include "AppInstallerDateTime.h" @@ -28,6 +29,108 @@ namespace AppInstaller::Repository result /= c_PackageTrackingFileName; return result; } + + struct PackageTrackingCatalogSourceReference : public ISourceReference + { + PackageTrackingCatalogSourceReference(const SourceDetails& details) : m_details(details) {} + + SourceDetails& GetDetails() override + { + return m_details; + } + + std::string GetIdentifier() override + { + return m_details.Identifier; + } + + SourceInformation GetInformation() override + { + return {}; + } + + // Set custom header. Returns false if custom header is not supported. + bool SetCustomHeader(std::optional header) override + { + return false; + } + + std::shared_ptr Open(IProgressCallback&) override + { + m_details.Arg = Utility::MakeSuitablePathPart(m_details.Data); + std::filesystem::path trackingDB = GetPackageTrackingFilePath(m_details.Arg); + + std::string lockName = CreateNameForCPRWL(m_details.Arg); + + if (!std::filesystem::exists(trackingDB)) + { + auto exclusiveLock = Synchronization::CrossProcessReaderWriteLock::LockExclusive(lockName); + + if (!std::filesystem::exists(trackingDB)) + { + std::filesystem::create_directories(trackingDB.parent_path()); + SQLiteIndex::CreateNew(trackingDB.u8string(), Schema::Version::Latest(), SQLiteIndex::CreateOptions::SupportPathless); + } + } + + auto lock = Synchronization::CrossProcessReaderWriteLock::LockShared(lockName); + + SQLiteIndex index = SQLiteIndex::Open(trackingDB.u8string(), SQLiteIndex::OpenDisposition::ReadWrite); + + // TODO: Check schema version and upgrade as necessary when there is a relevant new schema. + // Could write this all now but it will be better tested when there is a new schema. + + return std::make_shared(m_details, std::move(index), std::move(lock)); + } + + private: + // Store the indentifier of the source in the Data field. + SourceDetails m_details; + }; + + struct PackageTrackingCatalogSourceFactoryImpl : public ISourceFactory + { + std::shared_ptr Create(const SourceDetails& details) override final + { + THROW_HR_IF(E_INVALIDARG, !Utility::CaseInsensitiveEquals(details.Type, PackageTrackingCatalogSourceFactory::Type())); + + return std::make_shared(details); + } + + bool Add(SourceDetails&, IProgressCallback&) override final + { + THROW_HR(E_NOTIMPL); + } + + bool Update(const SourceDetails&, IProgressCallback&) override final + { + THROW_HR(E_NOTIMPL); + } + + bool Remove(const SourceDetails& details, IProgressCallback& progress) override final + { + THROW_HR_IF(E_INVALIDARG, !Utility::CaseInsensitiveEquals(details.Type, PackageTrackingCatalogSourceFactory::Type())); + + std::string pathName = Utility::MakeSuitablePathPart(details.Data); + + std::string lockName = CreateNameForCPRWL(pathName); + auto lock = Synchronization::CrossProcessReaderWriteLock::LockExclusive(lockName, progress); + + if (!lock) + { + return false; + } + + std::filesystem::path trackingDB = GetPackageTrackingFilePath(pathName); + + if (std::filesystem::exists(trackingDB)) + { + std::filesystem::remove(trackingDB); + } + + return true; + } + }; } struct PackageTrackingCatalog::implementation @@ -51,43 +154,19 @@ namespace AppInstaller::Repository THROW_HR(E_INVALIDARG); } - std::string pathName = Utility::MakeSuitablePathPart(sourceIdentifier); - - std::string lockName = CreateNameForCPRWL(pathName); - auto lock = Synchronization::CrossProcessReaderWriteLock::LockShared(lockName); - - std::filesystem::path trackingDB = GetPackageTrackingFilePath(pathName); - - if (!std::filesystem::exists(trackingDB)) - { - lock.Release(); - lock = Synchronization::CrossProcessReaderWriteLock::LockExclusive(lockName); - - if (!std::filesystem::exists(trackingDB)) - { - std::filesystem::create_directories(trackingDB.parent_path()); - SQLiteIndex::CreateNew(trackingDB.u8string(), Schema::Version::Latest(), SQLiteIndex::CreateOptions::SupportPathless); - } - - lock.Release(); - lock = Synchronization::CrossProcessReaderWriteLock::LockShared(lockName); - } - - SQLiteIndex index = SQLiteIndex::Open(trackingDB.u8string(), SQLiteIndex::OpenDisposition::ReadWrite); - - // TODO: Check schema version and upgrade as necessary when there is a relevant new schema. - // Could write this all now but it will be better tested when there is a new schema. - // Create fake details for the source while stashing some information that might be helpful for debugging SourceDetails details; + details.Type = PackageTrackingCatalogSourceFactory::Type(); details.Identifier = "*Tracking"; details.Name = "Tracking for "s + source.GetDetails().Name; details.Origin = SourceOrigin::PackageTracking; - details.Arg = pathName; + details.Data = sourceIdentifier; + + ProgressCallback dummyProgress; PackageTrackingCatalog result; result.m_implementation = std::make_shared(); - result.m_implementation->Source = std::make_shared(details, std::move(index), std::move(lock)); + result.m_implementation->Source = std::dynamic_pointer_cast(ISourceFactory::GetForType(details.Type)->Create(details)->Open(dummyProgress)); return result; } @@ -99,17 +178,14 @@ namespace AppInstaller::Repository THROW_HR(E_INVALIDARG); } - std::string pathName = Utility::MakeSuitablePathPart(identifier); + // Create details to pass to the factory; the identifier of the source is passed in the Data field. + SourceDetails dummyDetails; + dummyDetails.Type = PackageTrackingCatalogSourceFactory::Type(); + dummyDetails.Data = identifier; - std::string lockName = CreateNameForCPRWL(pathName); - auto lock = Synchronization::CrossProcessReaderWriteLock::LockExclusive(lockName); + ProgressCallback dummyProgress; - std::filesystem::path trackingDB = GetPackageTrackingFilePath(pathName); - - if (std::filesystem::exists(trackingDB)) - { - std::filesystem::remove(trackingDB); - } + ISourceFactory::GetForType(dummyDetails.Type)->Remove(dummyDetails, dummyProgress); } PackageTrackingCatalog::operator bool() const @@ -202,4 +278,9 @@ namespace AppInstaller::Repository } } } + + std::unique_ptr PackageTrackingCatalogSourceFactory::Create() + { + return std::make_unique(); + } } diff --git a/src/AppInstallerRepositoryCore/PackageTrackingCatalogSourceFactory.h b/src/AppInstallerRepositoryCore/PackageTrackingCatalogSourceFactory.h new file mode 100644 index 0000000000..dabbe2c660 --- /dev/null +++ b/src/AppInstallerRepositoryCore/PackageTrackingCatalogSourceFactory.h @@ -0,0 +1,25 @@ +// Copyright (c) Microsoft Corporation. +// Licensed under the MIT License. +#pragma once +#include "SourceFactory.h" + +using namespace std::string_view_literals; + + +namespace AppInstaller::Repository +{ + // Enable test to inject a custom tracking source by integrating the internal source creation into the standard flows. + // If overriding, the opened ISource must be a SQLiteIndexSource or the code will fail. + struct PackageTrackingCatalogSourceFactory + { + // Get the type string for this source. + static constexpr std::string_view Type() + { + using namespace std::string_view_literals; + return "Microsoft.PackageTracking"sv; + } + + // Creates a source factory for this type. + static std::unique_ptr Create(); + }; +} diff --git a/src/AppInstallerRepositoryCore/Public/winget/PackageTrackingCatalog.h b/src/AppInstallerRepositoryCore/Public/winget/PackageTrackingCatalog.h index 4759b970b8..bbaf55f1d3 100644 --- a/src/AppInstallerRepositoryCore/Public/winget/PackageTrackingCatalog.h +++ b/src/AppInstallerRepositoryCore/Public/winget/PackageTrackingCatalog.h @@ -29,7 +29,7 @@ namespace AppInstaller::Repository // Removes the package tracking catalog for a given source identifier. static void RemoveForSource(const std::string& identifier); - // Determines if the curent object holds anything. + // Determines if the current object holds anything. operator bool() const; // Execute a search against the catalog. diff --git a/src/AppInstallerRepositoryCore/RepositorySource.cpp b/src/AppInstallerRepositoryCore/RepositorySource.cpp index ad25381068..957814d8a2 100644 --- a/src/AppInstallerRepositoryCore/RepositorySource.cpp +++ b/src/AppInstallerRepositoryCore/RepositorySource.cpp @@ -10,6 +10,7 @@ #include "Microsoft/PredefinedWriteableSourceFactory.h" #include "Microsoft/PreIndexedPackageSourceFactory.h" #include "Rest/RestSourceFactory.h" +#include "PackageTrackingCatalogSourceFactory.h" #ifndef AICLI_DISABLE_TEST_HOOKS #include "Microsoft/ConfigurableTestSourceFactory.h" @@ -28,56 +29,16 @@ namespace AppInstaller::Repository static std::map()>> s_Sources_TestHook_SourceFactories; #endif - std::unique_ptr GetFactoryForType(std::string_view type) - { -#ifndef AICLI_DISABLE_TEST_HOOKS - // Tests can ensure case matching - auto itr = s_Sources_TestHook_SourceFactories.find(std::string(type)); - if (itr != s_Sources_TestHook_SourceFactories.end()) - { - return itr->second(); - } - - if (Utility::CaseInsensitiveEquals(Microsoft::ConfigurableTestSourceFactory::Type(), type)) - { - return Microsoft::ConfigurableTestSourceFactory::Create(); - } -#endif - - // For now, enable an empty type to represent the only one we have. - if (type.empty() || - Utility::CaseInsensitiveEquals(Microsoft::PreIndexedPackageSourceFactory::Type(), type)) - { - return Microsoft::PreIndexedPackageSourceFactory::Create(); - } - // Should always come from code, so no need for case insensitivity - else if (Microsoft::PredefinedInstalledSourceFactory::Type() == type) - { - return Microsoft::PredefinedInstalledSourceFactory::Create(); - } - // Should always come from code, so no need for case insensitivity - else if (Microsoft::PredefinedWriteableSourceFactory::Type() == type) - { - return Microsoft::PredefinedWriteableSourceFactory::Create(); - } - else if (Utility::CaseInsensitiveEquals(Rest::RestSourceFactory::Type(), type)) - { - return Rest::RestSourceFactory::Create(); - } - - THROW_HR(APPINSTALLER_CLI_ERROR_INVALID_SOURCE_TYPE); - } - std::shared_ptr CreateSourceFromDetails(const SourceDetails& details) { - return GetFactoryForType(details.Type)->Create(details); + return ISourceFactory::GetForType(details.Type)->Create(details); } template bool AddOrUpdateFromDetails(SourceDetails& details, MemberFunc member, IProgressCallback& progress) { bool result = false; - auto factory = GetFactoryForType(details.Type); + auto factory = ISourceFactory::GetForType(details.Type); // Attempt; if it fails, wait a short time and retry. try @@ -120,7 +81,7 @@ namespace AppInstaller::Repository bool RemoveSourceFromDetails(const SourceDetails& details, IProgressCallback& progress) { - auto factory = GetFactoryForType(details.Type); + auto factory = ISourceFactory::GetForType(details.Type); return factory->Remove(details, progress); } @@ -211,6 +172,51 @@ namespace AppInstaller::Repository }; } + std::unique_ptr ISourceFactory::GetForType(std::string_view type) + { +#ifndef AICLI_DISABLE_TEST_HOOKS + // Tests can ensure case matching + auto itr = s_Sources_TestHook_SourceFactories.find(std::string(type)); + if (itr != s_Sources_TestHook_SourceFactories.end()) + { + return itr->second(); + } + + if (Utility::CaseInsensitiveEquals(Microsoft::ConfigurableTestSourceFactory::Type(), type)) + { + return Microsoft::ConfigurableTestSourceFactory::Create(); + } +#endif + + // For now, enable an empty type to represent the only one we have. + if (type.empty() || + Utility::CaseInsensitiveEquals(Microsoft::PreIndexedPackageSourceFactory::Type(), type)) + { + return Microsoft::PreIndexedPackageSourceFactory::Create(); + } + // Should always come from code, so no need for case insensitivity + else if (Microsoft::PredefinedInstalledSourceFactory::Type() == type) + { + return Microsoft::PredefinedInstalledSourceFactory::Create(); + } + // Should always come from code, so no need for case insensitivity + else if (Microsoft::PredefinedWriteableSourceFactory::Type() == type) + { + return Microsoft::PredefinedWriteableSourceFactory::Create(); + } + // Should always come from code, so no need for case insensitivity + else if (PackageTrackingCatalogSourceFactory::Type() == type) + { + return PackageTrackingCatalogSourceFactory::Create(); + } + else if (Utility::CaseInsensitiveEquals(Rest::RestSourceFactory::Type(), type)) + { + return Rest::RestSourceFactory::Create(); + } + + THROW_HR(APPINSTALLER_CLI_ERROR_INVALID_SOURCE_TYPE); + } + std::string_view ToString(SourceOrigin origin) { switch (origin) diff --git a/src/AppInstallerRepositoryCore/SourceFactory.h b/src/AppInstallerRepositoryCore/SourceFactory.h index 71d6916b25..6aff2ee36f 100644 --- a/src/AppInstallerRepositoryCore/SourceFactory.h +++ b/src/AppInstallerRepositoryCore/SourceFactory.h @@ -37,5 +37,8 @@ namespace AppInstaller::Repository // Removes the source from the given details. // Return value indicates whether the action completed. virtual bool Remove(const SourceDetails& details, IProgressCallback& progress) = 0; + + // Gets the factory for the given type. + static std::unique_ptr GetForType(std::string_view type); }; } From 2c042c531d024764ae03c8f0f6bebb824b4a30ad Mon Sep 17 00:00:00 2001 From: JohnMcPMS Date: Thu, 4 Nov 2021 17:48:52 -0700 Subject: [PATCH 4/4] PR feedback --- src/AppInstallerCLITests/CompositeSource.cpp | 4 ++-- src/AppInstallerRepositoryCore/CompositeSource.cpp | 8 ++++---- src/AppInstallerRepositoryCore/CompositeSource.h | 4 ++-- .../PackageTrackingCatalog.cpp | 13 +------------ src/AppInstallerRepositoryCore/RepositorySource.cpp | 2 +- 5 files changed, 10 insertions(+), 21 deletions(-) diff --git a/src/AppInstallerCLITests/CompositeSource.cpp b/src/AppInstallerCLITests/CompositeSource.cpp index d0a9c71297..e1eb0865a3 100644 --- a/src/AppInstallerCLITests/CompositeSource.cpp +++ b/src/AppInstallerCLITests/CompositeSource.cpp @@ -54,7 +54,7 @@ struct CompositeTestSetup { Installed = std::make_shared("InstalledTestSource1"); Available = std::make_shared("AvailableTestSource1"); - Composite.SetInstalledSource(Installed); + Composite.SetInstalledSource(Source{ Installed }); Composite.AddAvailableSource(Source{ Available }); } @@ -782,7 +782,7 @@ TEST_CASE("CompositeSource_InstalledAvailableSearchFailure", "[CompositeSource]" setup.Composite.AddAvailableSource(Source{ AvailableFails }); - setup.Composite.SetInstalledSource(setup.Installed, CompositeSearchBehavior::AvailablePackages); + setup.Composite.SetInstalledSource(Source{ setup.Installed }, CompositeSearchBehavior::AvailablePackages); SearchRequest request; request.Query = RequestMatch{ MatchType::Exact, "whatever" }; diff --git a/src/AppInstallerRepositoryCore/CompositeSource.cpp b/src/AppInstallerRepositoryCore/CompositeSource.cpp index 04d842a012..61c3bc4e7b 100644 --- a/src/AppInstallerRepositoryCore/CompositeSource.cpp +++ b/src/AppInstallerRepositoryCore/CompositeSource.cpp @@ -683,7 +683,7 @@ namespace AppInstaller::Repository m_availableSources.emplace_back(source); } - void CompositeSource::SetInstalledSource(std::shared_ptr source, CompositeSearchBehavior searchBehavior) + void CompositeSource::SetInstalledSource(Source source, CompositeSearchBehavior searchBehavior) { m_installedSource = std::move(source); m_searchBehavior = searchBehavior; @@ -722,7 +722,7 @@ namespace AppInstaller::Repository if (m_searchBehavior == CompositeSearchBehavior::AllPackages || m_searchBehavior == CompositeSearchBehavior::Installed) { // Search installed source (allow exceptions out as we own the installed source) - SearchResult installedResult = m_installedSource->Search(request); + SearchResult installedResult = m_installedSource.Search(request); result.Truncated = installedResult.Truncated; for (auto&& match : installedResult.Matches) @@ -852,7 +852,7 @@ namespace AppInstaller::Repository SearchRequest systemReferenceSearch = packageData->CreateInclusionsSearchRequest(); // Correlate against installed (allow exceptions out as we own the installed source) - SearchResult installedCrossRef = m_installedSource->Search(systemReferenceSearch); + SearchResult installedCrossRef = m_installedSource.Search(systemReferenceSearch); std::shared_ptr installedPackage = GetMatchingPackage(installedCrossRef.Matches, [&]() { @@ -904,7 +904,7 @@ namespace AppInstaller::Repository SearchRequest systemReferenceSearch = packageData->CreateInclusionsSearchRequest(); // Correlate against installed (allow exceptions out as we own the installed source) - SearchResult installedCrossRef = m_installedSource->Search(systemReferenceSearch); + SearchResult installedCrossRef = m_installedSource.Search(systemReferenceSearch); std::shared_ptr installedPackage = GetMatchingPackage(installedCrossRef.Matches, [&]() { diff --git a/src/AppInstallerRepositoryCore/CompositeSource.h b/src/AppInstallerRepositoryCore/CompositeSource.h index f426d75203..48828a040c 100644 --- a/src/AppInstallerRepositoryCore/CompositeSource.h +++ b/src/AppInstallerRepositoryCore/CompositeSource.h @@ -42,7 +42,7 @@ namespace AppInstaller::Repository bool HasAvailableSource() const { return !m_availableSources.empty(); } // Sets the installed source to be composited. - void SetInstalledSource(std::shared_ptr source, CompositeSearchBehavior searchBehavior = CompositeSearchBehavior::Installed); + void SetInstalledSource(Source source, CompositeSearchBehavior searchBehavior = CompositeSearchBehavior::Installed); private: // Performs a search when an installed source is present. @@ -51,7 +51,7 @@ namespace AppInstaller::Repository // Performs a search when no installed source is present. SearchResult SearchAvailable(const SearchRequest& request) const; - std::shared_ptr m_installedSource; + Source m_installedSource; std::vector m_availableSources; SourceDetails m_details; CompositeSearchBehavior m_searchBehavior; diff --git a/src/AppInstallerRepositoryCore/PackageTrackingCatalog.cpp b/src/AppInstallerRepositoryCore/PackageTrackingCatalog.cpp index 7efe7c4704..920a2c5ee5 100644 --- a/src/AppInstallerRepositoryCore/PackageTrackingCatalog.cpp +++ b/src/AppInstallerRepositoryCore/PackageTrackingCatalog.cpp @@ -44,17 +44,6 @@ namespace AppInstaller::Repository return m_details.Identifier; } - SourceInformation GetInformation() override - { - return {}; - } - - // Set custom header. Returns false if custom header is not supported. - bool SetCustomHeader(std::optional header) override - { - return false; - } - std::shared_ptr Open(IProgressCallback&) override { m_details.Arg = Utility::MakeSuitablePathPart(m_details.Data); @@ -84,7 +73,7 @@ namespace AppInstaller::Repository } private: - // Store the indentifier of the source in the Data field. + // Store the identifier of the source in the Data field. SourceDetails m_details; }; diff --git a/src/AppInstallerRepositoryCore/RepositorySource.cpp b/src/AppInstallerRepositoryCore/RepositorySource.cpp index 957814d8a2..73883e3bda 100644 --- a/src/AppInstallerRepositoryCore/RepositorySource.cpp +++ b/src/AppInstallerRepositoryCore/RepositorySource.cpp @@ -291,7 +291,7 @@ namespace AppInstaller::Repository compositeSource->AddAvailableSource(availableSource.m_source); } - compositeSource->SetInstalledSource(installedSource.m_source, searchBehavior); + compositeSource->SetInstalledSource(installedSource, searchBehavior); m_source = compositeSource; m_isComposite = true;