Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Make manifest retrieval choice more dynamic #3738

Merged
merged 1 commit into from Oct 9, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
58 changes: 57 additions & 1 deletion src/AppInstallerCLITests/CompositeSource.cpp
Expand Up @@ -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)
Expand All @@ -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);
}
}

Expand All @@ -179,6 +185,7 @@ struct TestPackageHelper
Manifest::Manifest m_manifest;
std::shared_ptr<ISource> m_source;
std::shared_ptr<TestPackage> m_package;
bool m_hideSystemReferenceStrings = false;
};

TestPackageHelper MakeInstalled()
Expand Down Expand Up @@ -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);
}
}
29 changes: 20 additions & 9 deletions src/AppInstallerCLITests/TestSource.cpp
Expand Up @@ -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
{
Expand Down Expand Up @@ -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:
Expand Down Expand Up @@ -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));
}
}

Expand Down Expand Up @@ -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)
Expand Down
8 changes: 6 additions & 2 deletions src/AppInstallerCLITests/TestSource.h
Expand Up @@ -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>
Expand All @@ -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);
Expand All @@ -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 = {});
Expand Down Expand Up @@ -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;

Expand Down
3 changes: 2 additions & 1 deletion src/AppInstallerRepositoryCore/CompositeSource.cpp
Expand Up @@ -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)
Expand Down
4 changes: 4 additions & 0 deletions src/AppInstallerRepositoryCore/ISource.h
Expand Up @@ -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;

Expand Down
13 changes: 13 additions & 0 deletions src/AppInstallerRepositoryCore/Public/winget/RepositorySource.h
Expand Up @@ -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
{
Expand Down Expand Up @@ -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;

Expand Down
6 changes: 6 additions & 0 deletions src/AppInstallerRepositoryCore/RepositorySource.cpp
Expand Up @@ -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());
Expand Down
11 changes: 11 additions & 0 deletions src/AppInstallerRepositoryCore/Rest/RestSource.cpp
Expand Up @@ -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);
Expand Down
2 changes: 2 additions & 0 deletions src/AppInstallerRepositoryCore/Rest/RestSource.h
Expand Up @@ -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;

Expand Down