commit f71241785b0463c8783cca1c7d24f3a871f81a5a parent 56060c44be069bf65126d7c8b2f46795b850ca4e Author: yao-msft <50888816+yao-msft@users.noreply.github.com> Date: Thu, 18 May 2023 14:20:14 -0700 Improve correlation for optimized search in rest source and improve general correlation by downloading manifests (#3252) Diffstat:
12 files changed, 254 insertions(+), 152 deletions(-)
diff --git a/src/AppInstallerCLITests/CompositeSource.cpp b/src/AppInstallerCLITests/CompositeSource.cpp @@ -1448,3 +1448,38 @@ TEST_CASE("CompositeSource_Pinning_MultipleInstalled", "[CompositeSource][PinFlo RequireExpectedResultsWithPin(package1, expectedResult1); RequireExpectedResultsWithPin(package2, expectedResult2); } + +TEST_CASE("CompositeSource_CorrelateToInstalledContainsManifestData", "[CompositeSource]") +{ + CompositeTestSetup setup; + setup.Installed->SearchFunction = [&](const SearchRequest& request) + { + if (request.Purpose == SearchPurpose::CorrelationToInstalled) + { + bool expectedSearchFound = false; + for (const auto& inclusion : request.Inclusions) + { + if (inclusion.Field == PackageMatchField::ProductCode && inclusion.Value == "hello") + { + expectedSearchFound = true; + break; + } + } + + REQUIRE(expectedSearchFound); + } + + SearchResult result; + return result; + }; + setup.Available->SearchFunction = [&](const SearchRequest&) + { + SearchResult result; + result.Matches.emplace_back(MakeAvailable(setup.Available).WithPC("hello"), Criteria()); + return result; + }; + + SearchRequest request; + request.Query = RequestMatch(MatchType::Exact, "NotForEverything"); + SearchResult result = setup.Composite.Search(request); +} diff --git a/src/AppInstallerCLITests/HttpClientHelper.cpp b/src/AppInstallerCLITests/HttpClientHelper.cpp @@ -77,5 +77,5 @@ TEST_CASE("HttpClientHelper_PinningConfiguration", "[RestSource]") HttpClientHelper helper; helper.SetPinningConfiguration(config); - REQUIRE_THROWS_HR(helper.HandleGet(L"https://microsoft.com"), APPINSTALLER_CLI_ERROR_PINNED_CERTIFICATE_MISMATCH); + REQUIRE_THROWS_HR(helper.HandleGet(L"https://github.com"), APPINSTALLER_CLI_ERROR_PINNED_CERTIFICATE_MISMATCH); } diff --git a/src/AppInstallerCLITests/TestSource.cpp b/src/AppInstallerCLITests/TestSource.cpp @@ -9,27 +9,6 @@ using namespace AppInstaller::Repository; namespace TestCommon { - namespace - { - template<AppInstaller::Manifest::Localization Field> - void BuildPackageVersionMultiPropertyWithFallback(std::vector<Utility::LocIndString>& result, const Manifest::Manifest& VersionManifest) - { - result.emplace_back(VersionManifest.DefaultLocalization.Get<Field>()); - for (const auto& loc : VersionManifest.Localizations) - { - auto f = loc.Get<Field>(); - if (f.empty()) - { - result.emplace_back(loc.Get<Field>()); - } - else - { - result.emplace_back(std::move(f)); - } - } - } - } - TestPackageVersion::TestPackageVersion(const Manifest& manifest, MetadataMap installationMetadata, std::weak_ptr<const ISource> source) : VersionManifest(manifest), Metadata(std::move(installationMetadata)), Source(source) {} @@ -81,10 +60,16 @@ namespace TestCommon } break; case PackageVersionMultiProperty::Name: - BuildPackageVersionMultiPropertyWithFallback<AppInstaller::Manifest::Localization::PackageName>(result, VersionManifest); + for (auto name : VersionManifest.GetPackageNames()) + { + result.emplace_back(std::move(name)); + } break; case PackageVersionMultiProperty::Publisher: - BuildPackageVersionMultiPropertyWithFallback<AppInstaller::Manifest::Localization::Publisher>(result, VersionManifest); + for (auto publisher : VersionManifest.GetPublishers()) + { + result.emplace_back(std::move(publisher)); + } break; case PackageVersionMultiProperty::Locale: result.emplace_back(VersionManifest.DefaultLocalization.Locale); diff --git a/src/AppInstallerCommonCore/Manifest/Manifest.cpp b/src/AppInstallerCommonCore/Manifest/Manifest.cpp @@ -7,6 +7,17 @@ namespace AppInstaller::Manifest { + namespace + { + void AddFoldedStringToSetIfNotEmpty(std::set<string_t>& set, const string_t& value) + { + if (!value.empty()) + { + set.emplace(Utility::FoldCase(value)); + } + } + } + void Manifest::ApplyLocale(const std::string& locale) { CurrentLocalization = DefaultLocalization; @@ -126,4 +137,107 @@ namespace AppInstaller::Manifest return arpVersionFound ? Utility::VersionRange{ minVersion, maxVersion } : Utility::VersionRange{}; } + + std::vector<string_t> Manifest::GetPackageFamilyNames() const + { + return GetSystemReferenceStrings( + [](const ManifestInstaller& i) -> const Utility::NormalizedString& { return i.PackageFamilyName; }); + } + + std::vector<string_t> Manifest::GetProductCodes() const + { + return GetSystemReferenceStrings( + [](const ManifestInstaller& i) -> const Utility::NormalizedString& { return i.ProductCode; }, + [](const AppsAndFeaturesEntry& e) -> const Utility::NormalizedString& { return e.ProductCode; }); + } + + std::vector<string_t> Manifest::GetUpgradeCodes() const + { + return GetSystemReferenceStrings( + {}, + [](const AppsAndFeaturesEntry& e) -> const Utility::NormalizedString& { return e.UpgradeCode; }); + } + + std::vector<string_t> Manifest::GetPackageNames() const + { + std::set<string_t> set; + + AddFoldedStringToSetIfNotEmpty(set, DefaultLocalization.Get<Localization::PackageName>()); + for (const auto& loc : Localizations) + { + AddFoldedStringToSetIfNotEmpty(set, loc.Get<Localization::PackageName>()); + } + + // In addition to the names used for our display, add the display names from the ARP entries + for (const auto& installer : Installers) + { + for (const auto& appsAndFeaturesEntry : installer.AppsAndFeaturesEntries) + { + AddFoldedStringToSetIfNotEmpty(set, appsAndFeaturesEntry.DisplayName); + } + } + + std::vector<Utility::NormalizedString> result( + std::make_move_iterator(set.begin()), + std::make_move_iterator(set.end())); + + return result; + } + + std::vector<string_t> Manifest::GetPublishers() const + { + std::set<string_t> set; + + AddFoldedStringToSetIfNotEmpty(set, DefaultLocalization.Get<Localization::Publisher>()); + for (const auto& loc : Localizations) + { + AddFoldedStringToSetIfNotEmpty(set, loc.Get<Localization::Publisher>()); + } + + // In addition to the publishers used for our display, add the publisher from the ARP entries + for (const auto& installer : Installers) + { + for (const auto& appsAndFeaturesEntry : installer.AppsAndFeaturesEntries) + { + AddFoldedStringToSetIfNotEmpty(set, appsAndFeaturesEntry.Publisher); + } + } + + std::vector<Utility::NormalizedString> result( + std::make_move_iterator(set.begin()), + std::make_move_iterator(set.end())); + + return result; + } + + std::vector<string_t> Manifest::GetSystemReferenceStrings( + std::function<const string_t& (const ManifestInstaller&)> extractStringFromInstaller, + std::function<const string_t& (const AppsAndFeaturesEntry&)> extractStringFromAppsAndFeaturesEntry) const + { + std::set<string_t> set; + + for (const auto& installer : Installers) + { + if (extractStringFromInstaller) + { + const auto& installerString = extractStringFromInstaller(installer); + AddFoldedStringToSetIfNotEmpty(set, installerString); + } + + if (extractStringFromAppsAndFeaturesEntry) + { + for (const auto& entry : installer.AppsAndFeaturesEntries) + { + const auto& entryString = extractStringFromAppsAndFeaturesEntry(entry); + AddFoldedStringToSetIfNotEmpty(set, entryString); + } + } + } + + std::vector<Utility::NormalizedString> result( + std::make_move_iterator(set.begin()), + std::make_move_iterator(set.end())); + + return result; + } } \ No newline at end of file diff --git a/src/AppInstallerCommonCore/Public/winget/Manifest.h b/src/AppInstallerCommonCore/Public/winget/Manifest.h @@ -49,7 +49,27 @@ namespace AppInstaller::Manifest // Gets ARP version range if declared, otherwise an empty range is returned Utility::VersionRange GetArpVersionRange() const; + // Get package family names across installers, Case folded. + std::vector<string_t> GetPackageFamilyNames() const; + + // Get product codes across installers, Case folded. + std::vector<string_t> GetProductCodes() const; + + // Get upgrade codes across installers, Case folded. + std::vector<string_t> GetUpgradeCodes() const; + + // Get package names across localizations and installers, Case folded. + std::vector<string_t> GetPackageNames() const; + + // Get publishers across localizations and installers, Case folded. + std::vector<string_t> GetPublishers() const; + // If not empty, the SHA256 hash of the manifest stream itself. Utility::SHA256::HashBuffer StreamSha256; + + private: + std::vector<string_t> GetSystemReferenceStrings( + std::function<const string_t& (const ManifestInstaller&)> extractStringFromInstaller = {}, + std::function<const string_t& (const AppsAndFeaturesEntry&)> extractStringFromAppsAndFeaturesEntry = {}) const; }; } \ No newline at end of file diff --git a/src/AppInstallerRepositoryCore/CompositeSource.cpp b/src/AppInstallerRepositoryCore/CompositeSource.cpp @@ -872,10 +872,10 @@ namespace AppInstaller::Repository struct SystemReferenceString { SystemReferenceString(PackageMatchField field, Utility::LocIndString string) : - Field(field), String1(string) {} + Field(field), String1(Utility::FoldCase(string)) {} SystemReferenceString(PackageMatchField field, Utility::LocIndString string1, Utility::LocIndString string2) : - Field(field), String1(string1), String2(string2) {} + Field(field), String1(Utility::FoldCase(string1)), String2(Utility::FoldCase(string2)) {} bool operator<(const SystemReferenceString& other) const { @@ -953,7 +953,8 @@ namespace AppInstaller::Repository // 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<PackageData> CheckForExistingResultFromAvailablePackageMatch(const ResultMatch& availableMatch) + // downloadManifests: when creating system reference strings, also download manifests to get more data. + std::optional<PackageData> CheckForExistingResultFromAvailablePackageMatch(const ResultMatch& availableMatch, bool downloadManifests) { for (auto& match : Matches) { @@ -970,10 +971,19 @@ namespace AppInstaller::Repository } PackageData result; + constexpr int c_downloadManifestsLimit = 3; + int manifestsDownloaded = 0; for (auto const& versionKey : availableMatch.Package->GetAvailableVersionKeys()) { auto packageVersion = availableMatch.Package->GetAvailableVersion(versionKey); AddSystemReferenceStrings(packageVersion.get(), result); + + if (downloadManifests && manifestsDownloaded < c_downloadManifestsLimit) + { + auto manifest = packageVersion->GetManifest(); + AddSystemReferenceStringsFromManifest(manifest, result); + manifestsDownloaded++; + } } return result; } @@ -1111,6 +1121,32 @@ namespace AppInstaller::Repository data); } + void AddSystemReferenceStringsFromManifest(const Manifest::Manifest& manifest, PackageData& data) + { + for (const auto& pfn : manifest.GetPackageFamilyNames()) + { + data.AddIfNotPresent(SystemReferenceString{ PackageMatchField::PackageFamilyName, Utility::LocIndString{ pfn } }); + } + for (const auto& productCode : manifest.GetProductCodes()) + { + data.AddIfNotPresent(SystemReferenceString{ PackageMatchField::ProductCode, Utility::LocIndString{ productCode } }); + } + for (const auto& upgradeCode : manifest.GetUpgradeCodes()) + { + data.AddIfNotPresent(SystemReferenceString{ PackageMatchField::UpgradeCode, Utility::LocIndString{ upgradeCode } }); + } + for (const auto& name : manifest.GetPackageNames()) + { + for (const auto& publisher : manifest.GetPublishers()) + { + data.AddIfNotPresent(SystemReferenceString{ + PackageMatchField::NormalizedNameAndPublisher, + Utility::LocIndString{ name }, + Utility::LocIndString{ publisher } }); + } + } + } + void GetSystemReferenceStrings( IPackageVersion* installedVersion, PackageVersionMultiProperty prop, @@ -1460,7 +1496,9 @@ namespace AppInstaller::Repository for (auto&& match : availableResult.Matches) { // Check for a package already in the result that should have been correlated already. - auto packageData = result.CheckForExistingResultFromAvailablePackageMatch(match); + // 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); // If found existing package in the result, continue if (!packageData) diff --git a/src/AppInstallerRepositoryCore/Microsoft/Schema/1_1/Interface_1_1.cpp b/src/AppInstallerRepositoryCore/Microsoft/Schema/1_1/Interface_1_1.cpp @@ -25,57 +25,6 @@ namespace AppInstaller::Repository::Microsoft::Schema::V1_1 { - namespace - { - std::vector<Utility::NormalizedString> GetSystemReferenceStrings( - const Manifest::Manifest& manifest, - std::function<const Utility::NormalizedString&(const Manifest::ManifestInstaller&)> extractStringFromInstaller, - std::function<const Utility::NormalizedString&(const Manifest::AppsAndFeaturesEntry&)> extractStringFromAppsAndFeaturesEntry = {}) - { - std::set<Utility::NormalizedString> set; - - for (const auto& installer : manifest.Installers) - { - const auto& installerString = extractStringFromInstaller(installer); - if (!installerString.empty()) - { - set.emplace(Utility::FoldCase(installerString)); - } - - if (extractStringFromAppsAndFeaturesEntry) - { - for (const auto& entry : installer.AppsAndFeaturesEntries) - { - const auto& entryString = extractStringFromAppsAndFeaturesEntry(entry); - if (!entryString.empty()) - { - set.emplace(Utility::FoldCase(entryString)); - } - } - } - } - - std::vector<Utility::NormalizedString> result( - std::make_move_iterator(set.begin()), - std::make_move_iterator(set.end())); - - return result; - } - - std::vector<Utility::NormalizedString> GetPackageFamilyNames(const Manifest::Manifest& manifest) - { - return GetSystemReferenceStrings(manifest, [](const Manifest::ManifestInstaller& i) -> const Utility::NormalizedString& { return i.PackageFamilyName; }); - } - - std::vector<Utility::NormalizedString> GetProductCodes(const Manifest::Manifest& manifest) - { - return GetSystemReferenceStrings( - manifest, - [](const Manifest::ManifestInstaller& i) -> const Utility::NormalizedString& { return i.ProductCode; }, - [](const Manifest::AppsAndFeaturesEntry& e) -> const Utility::NormalizedString& { return e.ProductCode; }); - } - } - Schema::Version Interface::GetVersion() const { return { 1, 1 }; @@ -119,8 +68,8 @@ namespace AppInstaller::Repository::Microsoft::Schema::V1_1 // Add the new 1.1 data // These system reference strings are all stored with their cases folded so that they can be // looked up ordinally; enabling the index to provide efficient searches. - PackageFamilyNameTable::EnsureExistsAndInsert(connection, GetPackageFamilyNames(manifest), manifestId); - ProductCodeTable::EnsureExistsAndInsert(connection, GetProductCodes(manifest), manifestId); + PackageFamilyNameTable::EnsureExistsAndInsert(connection, manifest.GetPackageFamilyNames(), manifestId); + ProductCodeTable::EnsureExistsAndInsert(connection, manifest.GetProductCodes(), manifestId); savepoint.Commit(); @@ -134,8 +83,8 @@ namespace AppInstaller::Repository::Microsoft::Schema::V1_1 auto [indexModified, manifestId] = V1_0::Interface::UpdateManifest(connection, manifest, relativePath); // Update new 1:N tables as necessary - indexModified = PackageFamilyNameTable::UpdateIfNeededByManifestId(connection, GetPackageFamilyNames(manifest), manifestId) || indexModified; - indexModified = ProductCodeTable::UpdateIfNeededByManifestId(connection, GetProductCodes(manifest), manifestId) || indexModified; + indexModified = PackageFamilyNameTable::UpdateIfNeededByManifestId(connection, manifest.GetPackageFamilyNames(), manifestId) || indexModified; + indexModified = ProductCodeTable::UpdateIfNeededByManifestId(connection, manifest.GetProductCodes(), manifestId) || indexModified; savepoint.Commit(); diff --git a/src/AppInstallerRepositoryCore/Microsoft/Schema/1_2/Interface_1_2.cpp b/src/AppInstallerRepositoryCore/Microsoft/Schema/1_2/Interface_1_2.cpp @@ -268,9 +268,14 @@ namespace AppInstaller::Repository::Microsoft::Schema::V1_2 auto candidateInclusionsWithArch = request.Inclusions; if (UpdatePackageMatchFilters(candidateInclusionsWithArch, m_normalizer, Utility::NormalizationField::Architecture)) { - // If DisplayNames contain arch, only use values with arch for search + // If DisplayNames contain arch, only use Inclusions with arch for search request.Inclusions = candidateInclusionsWithArch; } + else + { + // Otherwise, just update the Inclusions with normalization + UpdatePackageMatchFilters(request.Inclusions, m_normalizer); + } return V1_1::Interface::SearchInternal(connection, request); } @@ -285,6 +290,7 @@ namespace AppInstaller::Repository::Microsoft::Schema::V1_2 { candidateSearches.emplace_back(std::move(candidateSearchWithArch)); } + UpdatePackageMatchFilters(request.Inclusions, m_normalizer); candidateSearches.emplace_back(request); SearchResult result; diff --git a/src/AppInstallerRepositoryCore/Microsoft/Schema/1_6/Interface_1_6.cpp b/src/AppInstallerRepositoryCore/Microsoft/Schema/1_6/Interface_1_6.cpp @@ -9,32 +9,6 @@ namespace AppInstaller::Repository::Microsoft::Schema::V1_6 { - namespace - { - std::vector<Utility::NormalizedString> GetUpgradeCodes(const Manifest::Manifest& manifest) - { - std::set<Utility::NormalizedString> set; - - for (const auto& installer : manifest.Installers) - { - for (const auto& appsAndFeaturesEntry : installer.AppsAndFeaturesEntries) - { - const Utility::NormalizedString& string = appsAndFeaturesEntry.UpgradeCode; - if (!string.empty()) - { - set.emplace(Utility::FoldCase(string)); - } - } - } - - std::vector<Utility::NormalizedString> result( - std::make_move_iterator(set.begin()), - std::make_move_iterator(set.end())); - - return result; - } - } - Interface::Interface(Utility::NormalizationVersion normVersion) : V1_5::Interface(normVersion) { } @@ -64,7 +38,7 @@ namespace AppInstaller::Repository::Microsoft::Schema::V1_6 // Add the new 1.6 data // These system reference strings are all stored with their cases folded so that they can be // looked up ordinally; enabling the index to provide efficient searches. - UpgradeCodeTable::EnsureExistsAndInsert(connection, GetUpgradeCodes(manifest), manifestId); + UpgradeCodeTable::EnsureExistsAndInsert(connection, manifest.GetUpgradeCodes(), manifestId); savepoint.Commit(); @@ -78,7 +52,7 @@ namespace AppInstaller::Repository::Microsoft::Schema::V1_6 auto [indexModified, manifestId] = V1_5::Interface::UpdateManifest(connection, manifest, relativePath); // Update new 1:N tables as necessary - indexModified = UpgradeCodeTable::UpdateIfNeededByManifestId(connection, GetUpgradeCodes(manifest), manifestId) || indexModified; + indexModified = UpgradeCodeTable::UpdateIfNeededByManifestId(connection, manifest.GetUpgradeCodes(), manifestId) || indexModified; savepoint.Commit(); diff --git a/src/AppInstallerRepositoryCore/Rest/RestSource.cpp b/src/AppInstallerRepositoryCore/Rest/RestSource.cpp @@ -262,7 +262,10 @@ namespace AppInstaller::Repository::Rest case PackageVersionMultiProperty::Name: if (m_versionInfo.Manifest) { - BuildPackageVersionMultiPropertyWithFallback<AppInstaller::Manifest::Localization::PackageName>(result); + for (auto name : m_versionInfo.Manifest->GetPackageNames()) + { + result.emplace_back(std::move(name)); + } } else { @@ -272,7 +275,10 @@ namespace AppInstaller::Repository::Rest case PackageVersionMultiProperty::Publisher: if (m_versionInfo.Manifest) { - BuildPackageVersionMultiPropertyWithFallback<AppInstaller::Manifest::Localization::Publisher>(result); + for (auto publisher : m_versionInfo.Manifest->GetPublishers()) + { + result.emplace_back(std::move(publisher)); + } } else { @@ -334,24 +340,6 @@ namespace AppInstaller::Repository::Rest } private: - template<AppInstaller::Manifest::Localization Field> - void BuildPackageVersionMultiPropertyWithFallback(std::vector<Utility::LocIndString>& result) const - { - result.emplace_back(m_versionInfo.Manifest->DefaultLocalization.Get<Field>()); - for (const auto& loc : m_versionInfo.Manifest->Localizations) - { - auto f = loc.Get<Field>(); - if (f.empty()) - { - result.emplace_back(loc.Get<Field>()); - } - else - { - result.emplace_back(std::move(f)); - } - } - } - std::shared_ptr<AvailablePackage> m_package; IRestClient::VersionInfo m_versionInfo; }; diff --git a/src/AppInstallerRepositoryCore/Rest/Schema/1_0/RestInterface_1_0.cpp b/src/AppInstallerRepositoryCore/Rest/Schema/1_0/RestInterface_1_0.cpp @@ -180,28 +180,19 @@ namespace AppInstaller::Repository::Rest::Schema::V1_0 std::vector<VersionInfo> versions; for (auto& manifestVersion : manifests) { - std::vector<std::string> packageFamilyNames; - std::vector<std::string> productCodes; - - for (auto& installer : manifestVersion.Installers) - { - if (!installer.PackageFamilyName.empty()) - { - packageFamilyNames.emplace_back(installer.PackageFamilyName); - } - - if (!installer.ProductCode.empty()) - { - productCodes.emplace_back(installer.ProductCode); - } - } - - std::vector<std::string> uniquePackageFamilyNames = RestHelper::GetUniqueItems(packageFamilyNames); - std::vector<std::string> uniqueProductCodes = RestHelper::GetUniqueItems(productCodes); + auto packageFamilyNames = manifestVersion.GetPackageFamilyNames(); + auto productCodes = manifestVersion.GetProductCodes(); + auto arpVersionRange = manifestVersion.GetArpVersionRange(); + auto upgradeCodes = manifestVersion.GetUpgradeCodes(); versions.emplace_back( - VersionInfo{ AppInstaller::Utility::VersionAndChannel {manifestVersion.Version, manifestVersion.Channel}, - manifestVersion, std::move(uniquePackageFamilyNames), std::move(uniqueProductCodes) }); + VersionInfo{ + AppInstaller::Utility::VersionAndChannel {manifestVersion.Version, manifestVersion.Channel}, + manifestVersion, + std::vector<std::string>{ packageFamilyNames.begin(), packageFamilyNames.end()}, + std::vector<std::string>{ productCodes.begin(), productCodes.end()}, + arpVersionRange.IsEmpty() ? std::vector<Utility::Version>{} : std::vector<Utility::Version>{ arpVersionRange.GetMinVersion(), arpVersionRange.GetMaxVersion() }, + std::vector<std::string>{ upgradeCodes.begin(), upgradeCodes.end()} }); } Package package = Package{ std::move(packageInfo), std::move(versions) }; diff --git a/src/AppInstallerRepositoryCore/Rest/Schema/IRestClient.h b/src/AppInstallerRepositoryCore/Rest/Schema/IRestClient.h @@ -24,6 +24,8 @@ namespace AppInstaller::Repository::Rest::Schema : PackageIdentifier(std::move(packageIdentifier)), PackageName(std::move(packageName)), Publisher(std::move(publisher)) {} }; + // NOTE: When changes are made to VersionInfo struct, remember to update the OptimizedSearch path in RestInterface1_0 + // where VersionInfo struct was directly created from manifest. struct VersionInfo { AppInstaller::Utility::VersionAndChannel VersionAndChannel;