commit 3165abec5bff3bf707ebc006e89483d975fd6974 parent bc9cb38e2ad941b6457d31f689f9dc912cc516b0 Author: yao-msft <50888816+yao-msft@users.noreply.github.com> Date: Thu, 6 Apr 2023 11:55:07 -0700 Improve correlation by keeping arch info declared in manifest arp DisplayName entry (#3100) Diffstat:
11 files changed, 255 insertions(+), 31 deletions(-)
diff --git a/src/AppInstallerCLIE2ETests/ListCommand.cs b/src/AppInstallerCLIE2ETests/ListCommand.cs @@ -194,6 +194,35 @@ namespace AppInstallerCLIE2ETests TestCommon.RemoveMsix(Constants.MsixInstallerName); } + /// <summary> + /// Test package correlation with same package name but different architecture is correct. + /// </summary> + [Test] + public void ListWithMappingWithArchitecture() + { + var installDir = TestCommon.GetRandomTestDir(); + var result = TestCommon.RunAICLICommand("install", $"AppInstallerTest.TestMappingWithArchitectureX86 -l {installDir}"); + Assert.AreEqual(Constants.ErrorCode.S_OK, result.ExitCode); + + // List with AppInstallerTest.TestMappingWithArchitectureX64 (from available to installed scenario) will not find the package. + result = TestCommon.RunAICLICommand("list", "AppInstallerTest.TestMappingWithArchitectureX64"); + Assert.AreEqual(Constants.ErrorCode.ERROR_NO_APPLICATIONS_FOUND, result.ExitCode); + Assert.False(result.StdOut.Contains("AppInstallerTest.TestMappingWithArchitectureX64")); + + // List with AppInstallerTest.TestMappingWithArchitectureX86 (from available to installed scenario) will find the package. + result = TestCommon.RunAICLICommand("list", "AppInstallerTest.TestMappingWithArchitectureX86"); + Assert.AreEqual(Constants.ErrorCode.S_OK, result.ExitCode); + Assert.True(result.StdOut.Contains("AppInstallerTest.TestMappingWithArchitectureX86")); + + // List with product code (from installed to available scenario) will find the AppInstallerTest.TestMappingWithArchitectureX86 package. + result = TestCommon.RunAICLICommand("list", "{0e426f01-b682-4e67-a357-52f9ecb4590d}"); + Assert.AreEqual(Constants.ErrorCode.S_OK, result.ExitCode); + Assert.True(result.StdOut.Contains("AppInstallerTest.TestMappingWithArchitectureX86")); + + // best effort clean up + TestCommon.RunCommand(Path.Combine(installDir, Constants.TestExeUninstallerFileName)); + } + private void ArpVersionMappingTest(string packageIdentifier, string displayNameOverride, string displayVersionOverride, string expectedListVersion, string notExpectedListVersion = "") { System.Guid guid = System.Guid.NewGuid(); diff --git a/src/AppInstallerCLIE2ETests/TestData/Manifests/TestMappingWithArchitectureX64.yaml b/src/AppInstallerCLIE2ETests/TestData/Manifests/TestMappingWithArchitectureX64.yaml @@ -0,0 +1,19 @@ +PackageIdentifier: AppInstallerTest.TestMappingWithArchitectureX64 +PackageVersion: 1.0 +PackageName: TestMappingWithArchitecture +PackageLocale: en-US +Publisher: Microsoft +License: Test +ShortDescription: E2E test for mapping with architecture. +Installers: + - Architecture: x64 + InstallerUrl: https://localhost:5001/TestKit/AppInstallerTestExeInstaller/AppInstallerTestExeInstaller.exe + InstallerType: exe + InstallerSha256: <EXEHASH> + AppsAndFeaturesEntries: + - DisplayName: "TestMappingWithArchitecture(X64)" + InstallerSwitches: + Custom: '/DisplayName TestMappingWithArchitecture(X64) /ProductID {0e426f01-b682-4e67-a357-52f9ecb4590d}' + InstallLocation: /InstallDir <INSTALLPATH> +ManifestType: singleton +ManifestVersion: 1.2.0+ \ No newline at end of file diff --git a/src/AppInstallerCLIE2ETests/TestData/Manifests/TestMappingWithArchitectureX86.yaml b/src/AppInstallerCLIE2ETests/TestData/Manifests/TestMappingWithArchitectureX86.yaml @@ -0,0 +1,19 @@ +PackageIdentifier: AppInstallerTest.TestMappingWithArchitectureX86 +PackageVersion: 1.0 +PackageName: TestMappingWithArchitecture +PackageLocale: en-US +Publisher: Microsoft +License: Test +ShortDescription: E2E test for mapping with architecture. +Installers: + - Architecture: x86 + InstallerUrl: https://localhost:5001/TestKit/AppInstallerTestExeInstaller/AppInstallerTestExeInstaller.exe + InstallerType: exe + InstallerSha256: <EXEHASH> + AppsAndFeaturesEntries: + - DisplayName: "TestMappingWithArchitecture(X86)" + InstallerSwitches: + Custom: '/DisplayName TestMappingWithArchitecture(X86) /ProductID {0e426f01-b682-4e67-a357-52f9ecb4590d}' + InstallLocation: /InstallDir <INSTALLPATH> +ManifestType: singleton +ManifestVersion: 1.2.0+ \ No newline at end of file diff --git a/src/AppInstallerCLITests/NameNormalization.cpp b/src/AppInstallerCLITests/NameNormalization.cpp @@ -139,3 +139,18 @@ TEST_CASE("NameNorm_Initial_PreserveWhitespace", "[name_norm]") REQUIRE(normer.NormalizeName("Some Name").Name() == "Some Name"); REQUIRE(normer.NormalizePublisher("Some Publisher Corp") == "Some Publisher"); } + +TEST_CASE("NameNorm_GetNormalizedName_GetNormalizedFields", "[name_norm]") +{ + NameNormalizer normer(NormalizationVersion::Initial); + + auto normalizedName = normer.NormalizeName("Name(X64)"); + REQUIRE(normalizedName.GetNormalizedName(NormalizationField::None) == "Name"); + REQUIRE(normalizedName.GetNormalizedName(NormalizationField::Architecture) == "Name(X64)"); + REQUIRE(normalizedName.GetNormalizedFields() == NormalizationField::Architecture); + + auto normalizedName2 = normer.NormalizeName("Name"); + REQUIRE(normalizedName2.GetNormalizedName(NormalizationField::None) == "Name"); + REQUIRE(normalizedName2.GetNormalizedName(NormalizationField::Architecture) == "Name"); + REQUIRE(normalizedName2.GetNormalizedFields() == NormalizationField::None); +}+ \ No newline at end of file diff --git a/src/AppInstallerCommonCore/NameNormalization.cpp b/src/AppInstallerCommonCore/NameNormalization.cpp @@ -501,4 +501,28 @@ namespace AppInstaller::Utility { return m_normalizer->NormalizePublisher(publisher); } + + std::string NormalizedName::GetNormalizedName(NormalizationField fieldsToInclude) const + { + std::string result = Name(); + + if (WI_IsFlagSet(fieldsToInclude, NormalizationField::Architecture) && m_arch != Utility::Architecture::Unknown) + { + result += '(' + std::string(Utility::ToString(m_arch)) + ')'; + } + + return result; + } + + NormalizationField NormalizedName::GetNormalizedFields() const + { + NormalizationField result = NormalizationField::None; + + if (m_arch != Utility::Architecture::Unknown) + { + result |= NormalizationField::Architecture; + } + + return result; + } } diff --git a/src/AppInstallerCommonCore/Public/winget/NameNormalization.h b/src/AppInstallerCommonCore/Public/winget/NameNormalization.h @@ -16,6 +16,16 @@ namespace AppInstaller::Utility InitialPreserveWhiteSpace, }; + // List of name normalization fields. Architecture, locale, etc. + // Currently only architecture is used. + enum class NormalizationField : uint32_t + { + None = 0x0, + Architecture = 0x1, + }; + + DEFINE_ENUM_FLAG_OPERATORS(NormalizationField); + struct NameNormalizer; // A package publisher and name that has been normalized, allowing direct @@ -39,6 +49,11 @@ namespace AppInstaller::Utility void Publisher(std::string&& publisher) { m_publisher = std::move(publisher); } void Publisher(std::string_view publisher) { m_publisher = publisher; } + // Gets normalized name with additional normalization fields included. + std::string GetNormalizedName(NormalizationField fieldsToInclude) const; + // Gets a flag indicating the list of fields detected in normalization. + NormalizationField GetNormalizedFields() const; + private: std::string m_name; Utility::Architecture m_arch = Utility::Architecture::Unknown; diff --git a/src/AppInstallerRepositoryCore/CompositeSource.cpp b/src/AppInstallerRepositoryCore/CompositeSource.cpp @@ -820,7 +820,8 @@ namespace AppInstaller::Repository return Field == other.Field && String1 == other.String1 && String2 == other.String2; } - void AddToFilters(std::vector<PackageMatchFilter>& filters) const + void AddToFilters( + std::vector<PackageMatchFilter>& filters) const { switch (Field) { @@ -852,13 +853,14 @@ namespace AppInstaller::Repository } } - SearchRequest CreateInclusionsSearchRequest() const + SearchRequest CreateInclusionsSearchRequest(SearchPurpose searchPurpose) const { SearchRequest result; for (const auto& srs : SystemReferenceStrings) { srs.AddToFilters(result.Inclusions); } + result.Purpose = searchPurpose; return result; } }; @@ -1221,7 +1223,8 @@ namespace AppInstaller::Repository // Create a search request to run against all available sources if (!installedPackageData.SystemReferenceStrings.empty()) { - SearchRequest systemReferenceSearch = installedPackageData.CreateInclusionsSearchRequest(); + SearchRequest systemReferenceSearch = installedPackageData.CreateInclusionsSearchRequest(SearchPurpose::CorrelationToAvailable); + AICLI_LOG(Repo, Info, << "Finding available package from installed package using system reference search: " << systemReferenceSearch.ToString()); Source trackedSource; std::shared_ptr<IPackage> trackingPackage; @@ -1239,8 +1242,8 @@ namespace AppInstaller::Repository std::shared_ptr<IPackage> 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() << "]"); + << "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"); }); @@ -1265,8 +1268,12 @@ namespace AppInstaller::Repository // Directly search for the available package from tracking information. if (trackingPackage) { - addedAvailablePackage = true; - compositePackage->AddAvailablePackage(GetTrackedPackageFromAvailableSource(result, trackedSource, trackingPackage->GetProperty(PackageProperty::Id))); + auto availablePackage = GetTrackedPackageFromAvailableSource(result, trackedSource, trackingPackage->GetProperty(PackageProperty::Id)); + if (availablePackage) + { + addedAvailablePackage = true; + compositePackage->AddAvailablePackage(std::move(availablePackage)); + } compositePackage->SetTracking(std::move(trackedSource), std::move(trackingPackage), std::move(trackingPackageVersion)); } @@ -1297,12 +1304,13 @@ namespace AppInstaller::Repository auto 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() << "]"); + << "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"); }); + // For non pinning cases. We found some matching packages here, don't keep going. addedAvailablePackage = true; compositePackage->AddAvailablePackage(std::move(availablePackage)); } @@ -1342,9 +1350,9 @@ namespace AppInstaller::Repository // 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(); + SearchRequest systemReferenceSearch = packageData->CreateInclusionsSearchRequest(SearchPurpose::CorrelationToInstalled); + AICLI_LOG(Repo, Info, << "Finding installed package from tracking package using system reference search: " << systemReferenceSearch.ToString()); // Correlate against installed (allow exceptions out as we own the installed source) SearchResult installedCrossRef = m_installedSource.Search(systemReferenceSearch); @@ -1391,8 +1399,9 @@ namespace AppInstaller::Repository if (packageData && !packageData->SystemReferenceStrings.empty()) { // Create a search request to run against the installed source - SearchRequest systemReferenceSearch = packageData->CreateInclusionsSearchRequest(); + SearchRequest systemReferenceSearch = packageData->CreateInclusionsSearchRequest(SearchPurpose::CorrelationToInstalled); + AICLI_LOG(Repo, Info, << "Finding installed package from available package using system reference search: " << systemReferenceSearch.ToString()); // Correlate against installed (allow exceptions out as we own the installed source) SearchResult installedCrossRef = m_installedSource.Search(systemReferenceSearch); diff --git a/src/AppInstallerRepositoryCore/Microsoft/ARPHelper.cpp b/src/AppInstallerRepositoryCore/Microsoft/ARPHelper.cpp @@ -363,13 +363,19 @@ namespace AppInstaller::Repository::Microsoft continue; } auto displayNameValue = displayName->GetValue<Registry::Value::Type::String>(); - manifest.DefaultLocalization.Add<Manifest::Localization::PackageName>(displayNameValue); if (displayNameValue.empty()) { AICLI_LOG(Repo, Verbose, << "Skipping " << productCode << " because DisplayName is empty"); continue; } + manifest.DefaultLocalization.Add<Manifest::Localization::PackageName>(displayNameValue); + // Add DisplayName to ARP entries too + // This is to help normalized publisher and name correlation where ARP DisplayName matching + // will be getting improved in future iterations. + manifest.Installers[0].AppsAndFeaturesEntries.emplace_back(); + manifest.Installers[0].AppsAndFeaturesEntries[0].DisplayName = displayNameValue; + // If no version can be determined, ignore this entry manifest.Version = DetermineVersion(arpKey); if (manifest.Version.empty()) @@ -404,7 +410,6 @@ namespace AppInstaller::Repository::Microsoft auto upgradeCodeItr = upgradeCodes.find(productCode); if (upgradeCodeItr != upgradeCodes.end()) { - manifest.Installers[0].AppsAndFeaturesEntries.emplace_back(); manifest.Installers[0].AppsAndFeaturesEntries[0].UpgradeCode = upgradeCodeItr->second; } } diff --git a/src/AppInstallerRepositoryCore/Microsoft/Schema/1_1/Interface_1_1.cpp b/src/AppInstallerRepositoryCore/Microsoft/Schema/1_1/Interface_1_1.cpp @@ -30,7 +30,7 @@ namespace AppInstaller::Repository::Microsoft::Schema::V1_1 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::function<const Utility::NormalizedString&(const Manifest::AppsAndFeaturesEntry&)> extractStringFromAppsAndFeaturesEntry = {}) { std::set<Utility::NormalizedString> set; diff --git a/src/AppInstallerRepositoryCore/Microsoft/Schema/1_2/Interface_1_2.cpp b/src/AppInstallerRepositoryCore/Microsoft/Schema/1_2/Interface_1_2.cpp @@ -13,9 +13,13 @@ namespace AppInstaller::Repository::Microsoft::Schema::V1_2 { namespace { - void AddNormalizedName(const Utility::NameNormalizer& normalizer, const Manifest::string_t& name, std::vector<Utility::NormalizedString>& out) + void AddNormalizedName( + const Utility::NameNormalizer& normalizer, + const Manifest::string_t& name, + std::vector<Utility::NormalizedString>& out, + Utility::NormalizationField fieldsToInclude = Utility::NormalizationField::None) { - Utility::NormalizedString value = normalizer.NormalizeName(Utility::FoldCase(name)).Name(); + Utility::NormalizedString value = normalizer.NormalizeName(Utility::FoldCase(name)).GetNormalizedName(fieldsToInclude); if (std::find(out.begin(), out.end(), value) == out.end()) { out.emplace_back(std::move(value)); @@ -65,6 +69,8 @@ namespace AppInstaller::Repository::Microsoft::Schema::V1_2 if (!appsAndFeaturesEntry.DisplayName.empty()) { AddNormalizedName(normalizer, appsAndFeaturesEntry.DisplayName, result); + // For arp display name, also add a copy with architecture info for more accurate correlation. + AddNormalizedName(normalizer, appsAndFeaturesEntry.DisplayName, result, Utility::NormalizationField::Architecture); } } } @@ -96,6 +102,48 @@ namespace AppInstaller::Repository::Microsoft::Schema::V1_2 return result; } + + // Update NormalizedNameAndPublisher with normalization and folding + // Returns true if the normalized name contains normalization field of fieldsToInclude + bool UpdateNormalizedNameAndPublisher( + PackageMatchFilter& filter, + const Utility::NameNormalizer& normalizer, + Utility::NormalizationField fieldsToInclude) + { + Utility::NormalizedName normalized = normalizer.Normalize(Utility::FoldCase(filter.Value), Utility::FoldCase(filter.Additional.value())); + filter.Value = normalized.GetNormalizedName(fieldsToInclude); + filter.Additional = normalized.Publisher(); + return WI_AreAllFlagsSet(normalized.GetNormalizedFields(), fieldsToInclude); + }; + + // Update NormalizedNameAndPublisher with normalization and folding + // Returns true if any of normalized name contains normalization field of fieldsToInclude + bool UpdatePackageMatchFilters( + std::vector<PackageMatchFilter>& filters, + const Utility::NameNormalizer& normalizer, + Utility::NormalizationField normalizedNameFieldsToFilter = Utility::NormalizationField::None) + { + bool normalizedNameFieldsFound = false; + for (auto itr = filters.begin(); itr != filters.end();) + { + if (itr->Field == PackageMatchField::NormalizedNameAndPublisher && itr->Type == MatchType::Exact) + { + if (!UpdateNormalizedNameAndPublisher(*itr, normalizer, normalizedNameFieldsToFilter)) + { + // If not matched, this package match filter will be removed. + // For example, if caller is trying to search with arch info only, values without arch will be removed from search. + itr = filters.erase(itr); + continue; + } + + normalizedNameFieldsFound = true; + } + + ++itr; + } + + return normalizedNameFieldsFound; + } } Interface::Interface(Utility::NormalizationVersion normVersion) : m_normalizer(normVersion) @@ -212,28 +260,52 @@ namespace AppInstaller::Repository::Microsoft::Schema::V1_2 ISQLiteIndex::SearchResult Interface::SearchInternal(const SQLite::Connection& connection, SearchRequest& request) const { - // Update NormalizedNameAndPublisher with normalization and folding - auto updateIfNeeded = [&](PackageMatchFilter& filter) + if (request.Purpose == SearchPurpose::CorrelationToInstalled) { - if (filter.Field == PackageMatchField::NormalizedNameAndPublisher && filter.Type == MatchType::Exact) + // Correlate from available package to installed package + // For available package to installed package mapping, only one try is needed. + // For example, if ARP DisplayName contains arch, then the installed package's ARP DisplayName should also include arch. + auto candidateInclusionsWithArch = request.Inclusions; + if (UpdatePackageMatchFilters(candidateInclusionsWithArch, m_normalizer, Utility::NormalizationField::Architecture)) { - Utility::NormalizedName normalized = m_normalizer.Normalize(Utility::FoldCase(filter.Value), Utility::FoldCase(filter.Additional.value())); - filter.Value = normalized.Name(); - filter.Additional = normalized.Publisher(); + // If DisplayNames contain arch, only use values with arch for search + request.Inclusions = candidateInclusionsWithArch; } - }; - for (auto& inclusion : request.Inclusions) - { - updateIfNeeded(inclusion); + return V1_1::Interface::SearchInternal(connection, request); } - - for (auto& filter : request.Filters) + else if (request.Purpose == SearchPurpose::CorrelationToAvailable) { - updateIfNeeded(filter); + // For installed package to available package correlation, + // try the search with NormalizedName with Arch first, if not found, try with all values. + // This can be extended in the future for more granular search requests. + std::vector<SearchRequest> candidateSearches; + auto candidateSearchWithArch = request; + if (UpdatePackageMatchFilters(candidateSearchWithArch.Inclusions, m_normalizer, Utility::NormalizationField::Architecture)) + { + candidateSearches.emplace_back(std::move(candidateSearchWithArch)); + } + candidateSearches.emplace_back(request); + + SearchResult result; + for (auto& candidateSearch : candidateSearches) + { + result = V1_1::Interface::SearchInternal(connection, candidateSearch); + if (!result.Matches.empty()) + { + break; + } + } + + return result; } + else + { + UpdatePackageMatchFilters(request.Inclusions, m_normalizer); + UpdatePackageMatchFilters(request.Filters, m_normalizer); - return V1_1::Interface::SearchInternal(connection, request); + return V1_1::Interface::SearchInternal(connection, request); + } } void Interface::PrepareForPackaging(SQLite::Connection& connection, bool vacuum) diff --git a/src/AppInstallerRepositoryCore/Public/winget/RepositorySearch.h b/src/AppInstallerRepositoryCore/Public/winget/RepositorySearch.h @@ -95,6 +95,17 @@ namespace AppInstaller::Repository } }; + // The search purpose of the search request. + enum class SearchPurpose + { + // Default search purpose. + Default, + // The result is used for correlation to an installed package. + CorrelationToInstalled, + // The result is used for correlation to an available package. + CorrelationToAvailable, + }; + // Container for data used to filter the available manifests in a source. // It can be thought of as: // (Query || Inclusions...) && Filters... @@ -113,6 +124,9 @@ namespace AppInstaller::Repository // Specific fields used to filter the data further. std::vector<PackageMatchFilter> Filters; + // The search purpose of the search request. + SearchPurpose Purpose = SearchPurpose::Default; + // The maximum number of results to return. // The default of 0 will place no limit. size_t MaximumResults{};