From 8774f0fe0e3265d5e731ae96de9859e039db9190 Mon Sep 17 00:00:00 2001 From: yao-msft <50888816+yao-msft@users.noreply.github.com> Date: Wed, 22 Jun 2022 11:10:21 -0700 Subject: [PATCH] Improve index dependency consistency check (#2247) --- src/AppInstallerCLITests/SQLiteIndex.cpp | 48 +++++++++++++++++++ .../CompositeSource.cpp | 7 ++- .../Schema/1_4/DependenciesTable.cpp | 44 ++++++++++++++++- .../Microsoft/Schema/1_4/DependenciesTable.h | 5 ++ .../Microsoft/Schema/1_4/Interface.h | 4 ++ .../Microsoft/Schema/1_4/Interface_1_4.cpp | 46 ++++++++++++++++++ .../Microsoft/Schema/1_5/Interface.h | 2 +- .../Microsoft/Schema/1_5/Interface_1_5.cpp | 15 ++++-- src/WinGetUtil/Exports.cpp | 23 ++++++++- src/WinGetUtil/WinGetUtil.h | 10 +++- 10 files changed, 194 insertions(+), 10 deletions(-) diff --git a/src/AppInstallerCLITests/SQLiteIndex.cpp b/src/AppInstallerCLITests/SQLiteIndex.cpp index b8783144b4..f63c56bb4e 100644 --- a/src/AppInstallerCLITests/SQLiteIndex.cpp +++ b/src/AppInstallerCLITests/SQLiteIndex.cpp @@ -609,6 +609,54 @@ TEST_CASE("SQLiteIndex_DependenciesTable_CheckConsistency", "[sqliteindex][V1_4] REQUIRE(!index.CheckConsistency(true)); } + + TempFile tempFile2{ "repolibtest_tempdb"s, ".db"s }; + INFO("Using temporary file named: " << tempFile2.GetPath()); + + { + SQLiteIndex index = CreateTestIndex(tempFile2, Schema::Version::Latest()); + + Manifest manifest; + manifest.Id = "Foo"; + manifest.Version = "10.0"; + + index.AddManifest(manifest, "path"); + + REQUIRE(index.CheckConsistency(true)); + + // Add dependency that does not require min version + Manifest manifestWithDependency1; + manifestWithDependency1.Id = "Bar1"; + manifestWithDependency1.Version = "10.0"; + manifestWithDependency1.Installers.push_back({}); + manifestWithDependency1.Installers[0].Dependencies.Add(Dependency(DependencyType::Package, manifest.Id)); + + index.AddManifest(manifestWithDependency1, "path1"); + + REQUIRE(index.CheckConsistency(true)); + + // Add dependency with min version satisfied + Manifest manifestWithDependency2; + manifestWithDependency2.Id = "Bar2"; + manifestWithDependency2.Version = "10.0"; + manifestWithDependency2.Installers.push_back({}); + manifestWithDependency2.Installers[0].Dependencies.Add(Dependency(DependencyType::Package, manifest.Id, "1.0")); + + index.AddManifest(manifestWithDependency2, "path2"); + + REQUIRE(index.CheckConsistency(true)); + + // Add dependency with min version not satisfied + Manifest manifestWithDependency3; + manifestWithDependency3.Id = "Bar3"; + manifestWithDependency3.Version = "10.0"; + manifestWithDependency3.Installers.push_back({}); + manifestWithDependency3.Installers[0].Dependencies.Add(Dependency(DependencyType::Package, manifest.Id, "11.0")); + + index.AddManifest(manifestWithDependency3, "path3"); + + REQUIRE_FALSE(index.CheckConsistency(true)); + } } TEST_CASE("SQLiteIndex_RemoveManifestFile_NotPresent", "[sqliteindex]") diff --git a/src/AppInstallerRepositoryCore/CompositeSource.cpp b/src/AppInstallerRepositoryCore/CompositeSource.cpp index f2cd8b0642..6cfb1a66bc 100644 --- a/src/AppInstallerRepositoryCore/CompositeSource.cpp +++ b/src/AppInstallerRepositoryCore/CompositeSource.cpp @@ -159,7 +159,12 @@ namespace AppInstaller::Repository { auto&& [manifestVersion, arpMinVersion, arpMaxVersion] = std::move(tuple); Utility::VersionRange arpVersionRange{ Utility::Version(std::move(arpMinVersion)), Utility::Version(std::move(arpMaxVersion)) }; - arpVersionMap.emplace_back(std::make_pair(Utility::Version{ std::move(manifestVersion) }, std::move(arpVersionRange))); + Utility::Version manifestVer{ std::move(manifestVersion) }; + // Skip mapping to unknown version + if (!manifestVer.IsUnknown()) + { + arpVersionMap.emplace_back(std::make_pair(std::move(manifestVer), std::move(arpVersionRange))); + } } // Go through the arp version map and determine what mapping should be performed. diff --git a/src/AppInstallerRepositoryCore/Microsoft/Schema/1_4/DependenciesTable.cpp b/src/AppInstallerRepositoryCore/Microsoft/Schema/1_4/DependenciesTable.cpp index f7b802c746..381e5ef29c 100644 --- a/src/AppInstallerRepositoryCore/Microsoft/Schema/1_4/DependenciesTable.cpp +++ b/src/AppInstallerRepositoryCore/Microsoft/Schema/1_4/DependenciesTable.cpp @@ -426,7 +426,7 @@ namespace AppInstaller::Repository::Microsoft::Schema::V1_4 .LeftOuterJoin(VersionTable::TableName()) .On(QCol(s_DependenciesTable_Table_Name, s_DependenciesTable_MinVersion_Column_Name), QCol(VersionTable::TableName(), SQLite::RowIDName)) .Where(QCol(ManifestTable::TableName(), SQLite::RowIDName)).IsNull() - .Or(QCol(VersionTable::TableName(), SQLite::RowIDName)).IsNull() + .Or(QCol(VersionTable::TableName(), SQLite::RowIDName)).IsNull().And(QCol(s_DependenciesTable_Table_Name, s_DependenciesTable_MinVersion_Column_Name)).IsNotNull() .Or(QCol(IdTable::TableName(), SQLite::RowIDName)).IsNull(); SQLite::Statement select = builder.Prepare(connection); @@ -515,4 +515,46 @@ namespace AppInstaller::Repository::Microsoft::Schema::V1_4 return result; } + + std::vector> DependenciesTable::GetAllDependenciesWithMinVersions(const SQLite::Connection& connection) + { + if (!Exists(connection)) + { + return {}; + } + + std::vector> result; + + constexpr std::string_view depTableAlias = "dep"; + constexpr std::string_view minVersionAlias = "minV"; + + StatementBuilder builder; + + // SELECT [dep].[package_id], [minV].[version] FROM [dependencies] AS [dep] + // JOIN [versions] AS [minV] ON [minV].[rowid] = [dep].[min_version] + builder.Select() + .Column(QCol(depTableAlias, s_DependenciesTable_PackageId_Column_Name)) + .Column(QCol(minVersionAlias, VersionTable::ValueName())) + .From({ s_DependenciesTable_Table_Name }).As(depTableAlias) + .Join({ VersionTable::TableName() }).As(minVersionAlias) + .On(QCol(minVersionAlias, SQLite::RowIDName), QCol(depTableAlias, s_DependenciesTable_MinVersion_Column_Name)); + + SQLite::Statement select = builder.Prepare(connection); + + while (select.Step()) + { + Utility::NormalizedString version = ""; + if (!select.GetColumnIsNull(1)) + { + version = select.GetColumn(1); + } + + if (!version.empty()) + { + result.emplace_back(std::make_pair(select.GetColumn(0), version)); + } + } + + return result; + } } \ No newline at end of file diff --git a/src/AppInstallerRepositoryCore/Microsoft/Schema/1_4/DependenciesTable.h b/src/AppInstallerRepositoryCore/Microsoft/Schema/1_4/DependenciesTable.h index 22f23c3f55..703b58ccc1 100644 --- a/src/AppInstallerRepositoryCore/Microsoft/Schema/1_4/DependenciesTable.h +++ b/src/AppInstallerRepositoryCore/Microsoft/Schema/1_4/DependenciesTable.h @@ -46,8 +46,13 @@ namespace AppInstaller::Repository::Microsoft::Schema::V1_4 // Checks if the row id is present in the column denoted by the value supplied. static bool IsValueReferenced(const SQLite::Connection& connection, std::string_view valueName, SQLite::rowid_t valueRowId); + // The dependencies table and corresponding index are dropped. static void PrepareForPackaging(SQLite::Connection& connection); + // Get all min version values of the given manifest's dependencies, used for VersionTable cleanup when updating or removing a manifest. static std::vector GetDependenciesMinVersionsRowIdByManifestId(const SQLite::Connection& connection, SQLite::rowid_t manifestRowId); + + // Get all dependencies with min versions in the dependencies table, used during consistency check. Returning a list of pair. + static std::vector> GetAllDependenciesWithMinVersions(const SQLite::Connection& connection); }; } \ No newline at end of file diff --git a/src/AppInstallerRepositoryCore/Microsoft/Schema/1_4/Interface.h b/src/AppInstallerRepositoryCore/Microsoft/Schema/1_4/Interface.h index 7906c6d568..a88777ee75 100644 --- a/src/AppInstallerRepositoryCore/Microsoft/Schema/1_4/Interface.h +++ b/src/AppInstallerRepositoryCore/Microsoft/Schema/1_4/Interface.h @@ -25,5 +25,9 @@ namespace AppInstaller::Repository::Microsoft::Schema::V1_4 protected: bool NotNeeded(const SQLite::Connection& connection, std::string_view tableName, std::string_view valueName, SQLite::rowid_t id) const override; + + private: + // Semantic check to validate dependencies with min versions are satisfied. + bool ValidateDependenciesWithMinVersions(const SQLite::Connection& connection, bool log) const; }; } \ No newline at end of file diff --git a/src/AppInstallerRepositoryCore/Microsoft/Schema/1_4/Interface_1_4.cpp b/src/AppInstallerRepositoryCore/Microsoft/Schema/1_4/Interface_1_4.cpp index fe24a483cb..9c3bf3a686 100644 --- a/src/AppInstallerRepositoryCore/Microsoft/Schema/1_4/Interface_1_4.cpp +++ b/src/AppInstallerRepositoryCore/Microsoft/Schema/1_4/Interface_1_4.cpp @@ -120,6 +120,11 @@ namespace AppInstaller::Repository::Microsoft::Schema::V1_4 result = DependenciesTable::DependenciesTableCheckConsistency(connection, log) && result; } + if (result || log) + { + result = ValidateDependenciesWithMinVersions(connection, log) && result; + } + return result; } @@ -132,4 +137,45 @@ namespace AppInstaller::Repository::Microsoft::Schema::V1_4 { return DependenciesTable::GetDependentsById(connection, packageId); } + + bool Interface::ValidateDependenciesWithMinVersions(const SQLite::Connection& connection, bool log) const + { + try + { + bool result = true; + // A map to store already checked dependency package latest versions. + std::map checkedVersions; + + auto dependencies = DependenciesTable::GetAllDependenciesWithMinVersions(connection); + for (auto const& dependency : dependencies) + { + // If the dependency package has not been checked yet, add to the map. + if (checkedVersions.find(dependency.first) == checkedVersions.end()) + { + auto versionKeys = GetVersionKeysById(connection, dependency.first); + THROW_HR_IF(E_UNEXPECTED, versionKeys.empty()); + checkedVersions.emplace(dependency.first, versionKeys[0].GetVersion()); + } + + // If the latest version is less than min version required, fail the validation. + if (checkedVersions[dependency.first] < Utility::Version{ dependency.second }) + { + AICLI_LOG(Repo, Error, << "Dependency with min version not satisfied. Dependency package row id: " << dependency.first << " min version: " << dependency.second); + result = false; + + if (!log) + { + break; + } + } + } + + return result; + } + catch (...) + { + AICLI_LOG(Repo, Error, << "ValidateDependenciesWithMinVersions() encountered internal error. Returning false."); + return false; + } + } } \ No newline at end of file diff --git a/src/AppInstallerRepositoryCore/Microsoft/Schema/1_5/Interface.h b/src/AppInstallerRepositoryCore/Microsoft/Schema/1_5/Interface.h index 72922eb1d6..c456cec422 100644 --- a/src/AppInstallerRepositoryCore/Microsoft/Schema/1_5/Interface.h +++ b/src/AppInstallerRepositoryCore/Microsoft/Schema/1_5/Interface.h @@ -28,6 +28,6 @@ namespace AppInstaller::Repository::Microsoft::Schema::V1_5 private: // Semantic check to validate all arp version ranges within the index - bool ValidateArpVersionConsistency(const SQLite::Connection& connection) const; + bool ValidateArpVersionConsistency(const SQLite::Connection& connection, bool log) const; }; } \ No newline at end of file diff --git a/src/AppInstallerRepositoryCore/Microsoft/Schema/1_5/Interface_1_5.cpp b/src/AppInstallerRepositoryCore/Microsoft/Schema/1_5/Interface_1_5.cpp index 6a528f6a0c..38844df57c 100644 --- a/src/AppInstallerRepositoryCore/Microsoft/Schema/1_5/Interface_1_5.cpp +++ b/src/AppInstallerRepositoryCore/Microsoft/Schema/1_5/Interface_1_5.cpp @@ -162,7 +162,7 @@ namespace AppInstaller::Repository::Microsoft::Schema::V1_5 if (result || log) { - result = ValidateArpVersionConsistency(connection) && result; + result = ValidateArpVersionConsistency(connection, log) && result; } return result; @@ -181,10 +181,12 @@ namespace AppInstaller::Repository::Microsoft::Schema::V1_5 } } - bool Interface::ValidateArpVersionConsistency(const SQLite::Connection& connection) const + bool Interface::ValidateArpVersionConsistency(const SQLite::Connection& connection, bool log) const { try { + bool result = true; + // Search everything SearchRequest request; auto searchResult = Search(connection, request); @@ -215,11 +217,16 @@ namespace AppInstaller::Repository::Microsoft::Schema::V1_5 if (Utility::HasOverlapInVersionRanges(ranges)) { AICLI_LOG(Repo, Error, << "Overlapped Arp version ranges found for package. PackageRowId: " << match.first); - return false; + result = false; + + if (!log) + { + break; + } } } - return true; + return result; } catch (...) { diff --git a/src/WinGetUtil/Exports.cpp b/src/WinGetUtil/Exports.cpp index f32de32d2f..12d9e48581 100644 --- a/src/WinGetUtil/Exports.cpp +++ b/src/WinGetUtil/Exports.cpp @@ -307,8 +307,6 @@ extern "C" } CATCH_RETURN() - DEFINE_ENUM_FLAG_OPERATORS(WinGetValidateManifestResult); - WINGET_UTIL_API WinGetValidateManifestV3( WINGET_MANIFEST_HANDLE manifest, WINGET_SQLITE_INDEX_HANDLE index, @@ -370,6 +368,27 @@ extern "C" } } + if (WI_IsFlagSet(option, WinGetValidateManifestOptionV2::InstallerValidation)) + { + try + { + auto errors = ValidateManifestInstallers(*manifestPtr); + if (errors.size() > 0) + { + // Throw the errors as ManifestExceptions to get processed errors and message. + THROW_EXCEPTION(ManifestException({ std::move(errors) })); + } + } + catch (const ManifestException& e) + { + WI_SetFlagIf(validationResult, WinGetValidateManifestResult::InstallerValidationFailure, !e.IsWarningOnly()); + if (message) + { + validationMessage += e.GetManifestErrorMessage(); + } + } + } + *result = validationResult; if (message) { diff --git a/src/WinGetUtil/WinGetUtil.h b/src/WinGetUtil/WinGetUtil.h index 719b3466df..b7067c1f06 100644 --- a/src/WinGetUtil/WinGetUtil.h +++ b/src/WinGetUtil/WinGetUtil.h @@ -28,6 +28,8 @@ extern "C" InstallerValidations = 0x4, }; + DEFINE_ENUM_FLAG_OPERATORS(WinGetValidateManifestOption); + enum WinGetCreateManifestOption { // Just create the manifest without any validation @@ -43,6 +45,8 @@ extern "C" ReturnErrorOnVerifiedPublisherFields = 0x1000, }; + DEFINE_ENUM_FLAG_OPERATORS(WinGetCreateManifestOption); + enum WinGetValidateManifestOptionV2 { // No validation, caller will get E_INVALIDARG @@ -55,6 +59,8 @@ extern "C" InstallerValidation = 0x4, }; + DEFINE_ENUM_FLAG_OPERATORS(WinGetValidateManifestOptionV2); + enum WinGetValidateManifestOperationType { OperationTypeAdd = 0, @@ -75,13 +81,15 @@ extern "C" InternalError = 0x1000, }; + DEFINE_ENUM_FLAG_OPERATORS(WinGetValidateManifestResult); + enum WinGetValidateManifestDependenciesOption { DefaultValidation = 0, ForDelete = 0x1, }; - DEFINE_ENUM_FLAG_OPERATORS(WinGetValidateManifestOption); + DEFINE_ENUM_FLAG_OPERATORS(WinGetValidateManifestDependenciesOption); // Initializes the logging infrastructure. WINGET_UTIL_API WinGetLoggingInit(