commit 95dfc279bbf60ed4160cafbdfb1d17cb8fe40398
parent 21de1607ed5c90174e6bb931406975c18681a5dd
Author: JohnMcPMS <johnmcp@microsoft.com>
Date: Mon, 9 Oct 2023 08:53:27 -0700
Make manifest retrieval choice more dynamic (#3738)
This change lets the source implementation indicate whether manifests might contain more information than the search results. It then only applies that to REST, as the implementation is not under our control. This should have no net effect on the results quality, but should not attempt to retrieve manifests from the winget source when they are not providing any additional value currently.
Diffstat:
9 files changed, 121 insertions(+), 13 deletions(-)
diff --git a/src/AppInstallerCLITests/CompositeSource.cpp b/src/AppInstallerCLITests/CompositeSource.cpp
@@ -152,6 +152,12 @@ struct TestPackageHelper
return *this;
}
+ TestPackageHelper& HideSRS(bool value = true)
+ {
+ m_hideSystemReferenceStrings = value;
+ return *this;
+ }
+
operator std::shared_ptr<IPackage>()
{
if (!m_package)
@@ -162,7 +168,7 @@ struct TestPackageHelper
}
else
{
- m_package = TestPackage::Make(std::vector<Manifest::Manifest>{ m_manifest }, m_source);
+ m_package = TestPackage::Make(std::vector<Manifest::Manifest>{ m_manifest }, m_source, m_hideSystemReferenceStrings);
}
}
@@ -179,6 +185,7 @@ private:
Manifest::Manifest m_manifest;
std::shared_ptr<ISource> m_source;
std::shared_ptr<TestPackage> m_package;
+ bool m_hideSystemReferenceStrings = false;
};
TestPackageHelper MakeInstalled()
@@ -1475,3 +1482,52 @@ TEST_CASE("CompositeSource_CorrelateToInstalledContainsManifestData", "[Composit
request.Query = RequestMatch(MatchType::Exact, "NotForEverything");
SearchResult result = setup.Composite.Search(request);
}
+
+TEST_CASE("CompositeSource_Respects_FeatureFlag_ManifestMayContainAdditionalSystemReferenceStrings", "[CompositeSource]")
+{
+ std::string id = "Special test ID";
+ std::string productCode1 = "product-code1";
+
+ CompositeTestSetup setup;
+ bool productCodeSearched = false;
+ setup.Installed->SearchFunction = [&](const SearchRequest& request)
+ {
+ for (const auto& inclusion : request.Inclusions)
+ {
+ if (inclusion.Field == PackageMatchField::ProductCode)
+ {
+ productCodeSearched = true;
+ }
+ }
+
+ return SearchResult{};
+ };
+ setup.Available->SearchFunction = [&](const SearchRequest&)
+ {
+ SearchResult result;
+ result.Matches.emplace_back(MakeAvailable(setup.Available).WithId(id).WithPC(productCode1).HideSRS(), Criteria());
+ return result;
+ };
+
+ SECTION("Feature false")
+ {
+ SearchRequest request;
+ request.Query = RequestMatch(MatchType::Exact, "NotForEverything");
+ SearchResult result = setup.Composite.Search(request);
+
+ REQUIRE(!productCodeSearched);
+ }
+ SECTION("Feature true")
+ {
+ setup.Available->QueryFeatureFlagFunction = [](SourceFeatureFlag flag)
+ {
+ return (flag == SourceFeatureFlag::ManifestMayContainAdditionalSystemReferenceStrings);
+ };
+
+ SearchRequest request;
+ request.Query = RequestMatch(MatchType::Exact, "NotForEverything");
+ SearchResult result = setup.Composite.Search(request);
+
+ REQUIRE(productCodeSearched);
+ }
+}
diff --git a/src/AppInstallerCLITests/TestSource.cpp b/src/AppInstallerCLITests/TestSource.cpp
@@ -12,8 +12,8 @@ namespace TestCommon
TestPackageVersion::TestPackageVersion(const Manifest& manifest, MetadataMap installationMetadata, std::weak_ptr<const ISource> source) :
VersionManifest(manifest), Metadata(std::move(installationMetadata)), Source(source) {}
- TestPackageVersion::TestPackageVersion(const Manifest& manifest, std::weak_ptr<const ISource> source) :
- VersionManifest(manifest), Source(source) {}
+ TestPackageVersion::TestPackageVersion(const Manifest& manifest, std::weak_ptr<const ISource> source, bool hideSystemReferenceStrings) :
+ VersionManifest(manifest), Source(source), HideSystemReferenceStrings(hideSystemReferenceStrings) {}
TestPackageVersion::LocIndString TestPackageVersion::GetProperty(PackageVersionProperty property) const
{
@@ -47,16 +47,22 @@ namespace TestCommon
switch (property)
{
case PackageVersionMultiProperty::PackageFamilyName:
- for (const auto& installer : VersionManifest.Installers)
+ if (!HideSystemReferenceStrings)
{
- AddIfHasValueAndNotPresent(installer.PackageFamilyName, result, true);
+ for (const auto& installer : VersionManifest.Installers)
+ {
+ AddIfHasValueAndNotPresent(installer.PackageFamilyName, result, true);
+ }
}
break;
case PackageVersionMultiProperty::ProductCode:
- for (const auto& installer : VersionManifest.Installers)
+ if (!HideSystemReferenceStrings)
{
- bool shouldFoldCaseForNonPortable = installer.EffectiveInstallerType() != AppInstaller::Manifest::InstallerTypeEnum::Portable;
- AddIfHasValueAndNotPresent(installer.ProductCode, result, shouldFoldCaseForNonPortable);
+ for (const auto& installer : VersionManifest.Installers)
+ {
+ bool shouldFoldCaseForNonPortable = installer.EffectiveInstallerType() != AppInstaller::Manifest::InstallerTypeEnum::Portable;
+ AddIfHasValueAndNotPresent(installer.ProductCode, result, shouldFoldCaseForNonPortable);
+ }
}
break;
case PackageVersionMultiProperty::Name:
@@ -111,11 +117,11 @@ namespace TestCommon
}
}
- TestPackage::TestPackage(const std::vector<Manifest>& available, std::weak_ptr<const ISource> source)
+ TestPackage::TestPackage(const std::vector<Manifest>& available, std::weak_ptr<const ISource> source, bool hideSystemReferenceStringsOnVersion)
{
for (const auto& manifest : available)
{
- AvailableVersions.emplace_back(TestPackageVersion::Make(manifest, source));
+ AvailableVersions.emplace_back(TestPackageVersion::Make(manifest, source, hideSystemReferenceStringsOnVersion));
}
}
@@ -261,6 +267,11 @@ namespace TestCommon
return Information;
}
+ bool TestSource::QueryFeatureFlag(SourceFeatureFlag flag) const
+ {
+ return (QueryFeatureFlagFunction ? QueryFeatureFlagFunction(flag) : false);
+ }
+
SearchResult TestSource::Search(const SearchRequest& request) const
{
if (SearchFunction)
diff --git a/src/AppInstallerCLITests/TestSource.h b/src/AppInstallerCLITests/TestSource.h
@@ -18,7 +18,7 @@ namespace TestCommon
using LocIndString = AppInstaller::Utility::LocIndString;
using MetadataMap = AppInstaller::Repository::IPackageVersion::Metadata;
- TestPackageVersion(const Manifest& manifest, std::weak_ptr<const ISource> source = {});
+ TestPackageVersion(const Manifest& manifest, std::weak_ptr<const ISource> source = {}, bool hideSystemReferenceStrings = false);
TestPackageVersion(const Manifest& manifest, MetadataMap installationMetadata, std::weak_ptr<const ISource> source = {});
template <typename... Args>
@@ -36,6 +36,7 @@ namespace TestCommon
Manifest VersionManifest;
MetadataMap Metadata;
std::weak_ptr<const ISource> Source;
+ bool HideSystemReferenceStrings = false;
protected:
static void AddIfHasValueAndNotPresent(const AppInstaller::Utility::NormalizedString& value, std::vector<LocIndString>& target, bool folded = false);
@@ -52,7 +53,7 @@ namespace TestCommon
using MetadataMap = TestPackageVersion::MetadataMap;
// Create a package with only available versions using these manifests.
- TestPackage(const std::vector<Manifest>& available, std::weak_ptr<const ISource> source = {});
+ TestPackage(const std::vector<Manifest>& available, std::weak_ptr<const ISource> source = {}, bool hideSystemReferenceStringsOnVersion = false);
// Create a package with an installed version, metadata, and optionally available versions.
TestPackage(const Manifest& installed, MetadataMap installationMetadata, const std::vector<Manifest>& available = {}, std::weak_ptr<const ISource> source = {});
@@ -86,6 +87,9 @@ namespace TestCommon
const std::string& GetIdentifier() const override;
AppInstaller::Repository::SourceInformation GetInformation() const override;
+ bool QueryFeatureFlag(AppInstaller::Repository::SourceFeatureFlag flag) const override;
+ std::function<bool(AppInstaller::Repository::SourceFeatureFlag)> QueryFeatureFlagFunction;
+
AppInstaller::Repository::SearchResult Search(const AppInstaller::Repository::SearchRequest& request) const override;
void* CastTo(AppInstaller::Repository::ISourceType type) override;
diff --git a/src/AppInstallerRepositoryCore/CompositeSource.cpp b/src/AppInstallerRepositoryCore/CompositeSource.cpp
@@ -1519,13 +1519,14 @@ namespace AppInstaller::Repository
}
SearchResult availableResult = result.SearchAndHandleFailures(source, request);
+ bool downloadManifests = source.QueryFeatureFlag(SourceFeatureFlag::ManifestMayContainAdditionalSystemReferenceStrings);
for (auto&& match : availableResult.Matches)
{
// Check for a package already in the result that should have been correlated already.
// In cases that PackageData will be created, also download manifests for system reference strings
// when search result is small (currently limiting to 1).
- auto packageData = result.CheckForExistingResultFromAvailablePackageMatch(match, availableResult.Matches.size() == 1);
+ auto packageData = result.CheckForExistingResultFromAvailablePackageMatch(match, downloadManifests && availableResult.Matches.size() == 1);
// If found existing package in the result, continue
if (!packageData)
diff --git a/src/AppInstallerRepositoryCore/ISource.h b/src/AppInstallerRepositoryCore/ISource.h
@@ -35,6 +35,10 @@ namespace AppInstaller::Repository
// Get the source's information after the source is opened.
virtual SourceInformation GetInformation() const { return {}; };
+ // Query the value of the given feature flag.
+ // The default state of any new flag is false.
+ virtual bool QueryFeatureFlag(SourceFeatureFlag) const { return false; }
+
// Execute a search on the source.
virtual SearchResult Search(const SearchRequest& request) const = 0;
diff --git a/src/AppInstallerRepositoryCore/Public/winget/RepositorySource.h b/src/AppInstallerRepositoryCore/Public/winget/RepositorySource.h
@@ -168,6 +168,15 @@ namespace AppInstaller::Repository
std::vector<std::string> RequiredQueryParameters;
};
+ // Allows calling code to inquire about specific features of an ISource implementation.
+ // The default state of any new flag is false.
+ enum class SourceFeatureFlag
+ {
+ // If true, the manifests for this source may contain more data than is available from just the
+ // version information found from a search.
+ ManifestMayContainAdditionalSystemReferenceStrings,
+ };
+
// Represents a source which would be interacted from outside of repository lib.
struct Source
{
@@ -216,6 +225,10 @@ namespace AppInstaller::Repository
// Get the source's information.
SourceInformation GetInformation() const;
+ // Query the value of the given feature flag.
+ // The default state of any new flag is false.
+ bool QueryFeatureFlag(SourceFeatureFlag flag) const;
+
// Returns true if the origin type can contain available packages.
bool ContainsAvailablePackages() const;
diff --git a/src/AppInstallerRepositoryCore/RepositorySource.cpp b/src/AppInstallerRepositoryCore/RepositorySource.cpp
@@ -563,6 +563,12 @@ namespace AppInstaller::Repository
}
}
+ bool Source::QueryFeatureFlag(SourceFeatureFlag flag) const
+ {
+ THROW_HR_IF(HRESULT_FROM_WIN32(ERROR_INVALID_STATE), !m_source);
+ return m_source->QueryFeatureFlag(flag);
+ }
+
bool Source::ContainsAvailablePackages() const
{
THROW_HR_IF(HRESULT_FROM_WIN32(ERROR_INVALID_STATE), IsComposite());
diff --git a/src/AppInstallerRepositoryCore/Rest/RestSource.cpp b/src/AppInstallerRepositoryCore/Rest/RestSource.cpp
@@ -436,6 +436,17 @@ namespace AppInstaller::Repository::Rest
return m_information;
}
+ bool RestSource::QueryFeatureFlag(SourceFeatureFlag flag) const
+ {
+ switch (flag)
+ {
+ case SourceFeatureFlag::ManifestMayContainAdditionalSystemReferenceStrings:
+ return true;
+ }
+
+ return false;
+ }
+
SearchResult RestSource::Search(const SearchRequest& request) const
{
IRestClient::SearchResult results = m_restClient.Search(request);
diff --git a/src/AppInstallerRepositoryCore/Rest/RestSource.h b/src/AppInstallerRepositoryCore/Rest/RestSource.h
@@ -31,6 +31,8 @@ namespace AppInstaller::Repository::Rest
SourceInformation GetInformation() const override;
+ bool QueryFeatureFlag(SourceFeatureFlag flag) const override;
+
// Execute a search on the source.
SearchResult Search(const SearchRequest& request) const override;
