commit 83cba4835230e168dc9e9c5537b4bbb5934e2ddb
parent 5c2ff990d93536bcc0a0db25abc0d3e9ec6900aa
Author: yao-msft <50888816+yao-msft@users.noreply.github.com>
Date: Wed, 13 Jul 2022 16:19:00 -0700
Fix WinGetUtil index operations for manifest dependencies without min version (#2337)
Diffstat:
4 files changed, 73 insertions(+), 54 deletions(-)
diff --git a/src/AppInstallerCLITests/SQLiteIndex.cpp b/src/AppInstallerCLITests/SQLiteIndex.cpp
@@ -485,7 +485,7 @@ TEST_CASE("SQLiteIndex_VersionReferencedByDependenciesClearsUnusedVersionAndKeep
}
}
-TEST_CASE("SQLiteIndex_AddManifestWithDependencies", "[sqliteindex][V1_4]")
+TEST_CASE("SQLiteIndex_AddUpdateRemoveManifestWithDependencies", "[sqliteindex][V1_4]")
{
TempFile tempFile{ "repolibtest_tempdb"s, ".db"s };
INFO("Using temporary file named: " << tempFile.GetPath());
@@ -503,6 +503,8 @@ TEST_CASE("SQLiteIndex_AddManifestWithDependencies", "[sqliteindex][V1_4]")
manifest.Installers[0].Dependencies.Add(Dependency(DependencyType::Package, dependencyManifest2.Id, "1.0.0"));
index.AddManifest(manifest, GetPathFromManifest(manifest));
+ index.UpdateManifest(manifest, GetPathFromManifest(manifest));
+ index.RemoveManifest(manifest);
}
TEST_CASE("SQLiteIndex_AddManifestWithDependencies_MissingPackage", "[sqliteindex][V1_4]")
@@ -525,7 +527,7 @@ TEST_CASE("SQLiteIndex_AddManifestWithDependencies_MissingPackage", "[sqliteinde
REQUIRE_THROWS_HR(index.AddManifest(manifest, GetPathFromManifest(manifest)), APPINSTALLER_CLI_ERROR_MISSING_PACKAGE);
}
-TEST_CASE("SQLiteIndex_AddManifestWithDependencies_MissingVersion", "[sqliteindex][V1_4]")
+TEST_CASE("SQLiteIndex_AddUpdateRemoveManifestWithDependencies_MissingVersion", "[sqliteindex][V1_4]")
{
TempFile tempFile{ "repolibtest_tempdb"s, ".db"s };
INFO("Using temporary file named: " << tempFile.GetPath());
@@ -543,9 +545,11 @@ TEST_CASE("SQLiteIndex_AddManifestWithDependencies_MissingVersion", "[sqliteinde
manifest.Installers[0].Dependencies.Add(Dependency(DependencyType::Package, dependencyManifest2.Id, "0.0.2"));
index.AddManifest(manifest, GetPathFromManifest(manifest));
+ index.UpdateManifest(manifest, GetPathFromManifest(manifest));
+ index.RemoveManifest(manifest);
}
-TEST_CASE("SQLiteIndex_AddManifestWithDependencies_EmptyManifestVersion", "[sqliteindex][V1_4]")
+TEST_CASE("SQLiteIndex_AddUpdateRemoveManifestWithDependencies_EmptyManifestVersion", "[sqliteindex][V1_4]")
{
TempFile tempFile{ "repolibtest_tempdb"s, ".db"s };
INFO("Using temporary file named: " << tempFile.GetPath());
@@ -563,6 +567,8 @@ TEST_CASE("SQLiteIndex_AddManifestWithDependencies_EmptyManifestVersion", "[sqli
manifest.Installers[0].Dependencies.Add(Dependency(DependencyType::Package, dependencyManifest2.Id));
index.AddManifest(manifest, GetPathFromManifest(manifest));
+ index.UpdateManifest(manifest, GetPathFromManifest(manifest));
+ index.RemoveManifest(manifest);
}
TEST_CASE("SQLiteIndex_DependenciesTable_CheckConsistency", "[sqliteindex][V1_4]")
diff --git a/src/AppInstallerRepositoryCore/Microsoft/Schema/1_0/Interface_1_0.cpp b/src/AppInstallerRepositoryCore/Microsoft/Schema/1_0/Interface_1_0.cpp
@@ -63,7 +63,7 @@ namespace AppInstaller::Repository::Microsoft::Schema::V1_0
std::optional<SQLite::rowid_t> channelIdOpt = ChannelTable::SelectIdByValue(connection, channel, true);
if (!channelIdOpt && !channel.empty())
{
- // If an empty channel was given but none was found, we will just not filter on channel.
+ // If a non-empty channel was given but none was found, we will just not filter on channel.
AICLI_LOG(Repo, Info, << "Did not find a Channel { " << channel << " }");
return {};
}
diff --git a/src/AppInstallerRepositoryCore/Microsoft/Schema/1_4/DependenciesTable.cpp b/src/AppInstallerRepositoryCore/Microsoft/Schema/1_4/DependenciesTable.cpp
@@ -138,7 +138,6 @@ namespace AppInstaller::Repository::Microsoft::Schema::V1_4
.Values(Unbound, Unbound, Unbound);
SQLite::Statement insert = insertBuilder.Prepare(connection);
-
for (const auto& dep : dependenciesTableRows)
{
insert.Reset();
@@ -296,8 +295,8 @@ namespace AppInstaller::Repository::Microsoft::Schema::V1_4
}
);
- bool tableUpdated = InsertManifestDependencies(connection, toAddDependencies);
- tableUpdated = RemoveDependenciesByRowIds(connection, toRemoveDependencies) || tableUpdated;
+ bool tableUpdated = RemoveDependenciesByRowIds(connection, toRemoveDependencies);
+ tableUpdated = InsertManifestDependencies(connection, toAddDependencies) || tableUpdated;
savepoint.Commit();
return tableUpdated;
@@ -327,17 +326,18 @@ namespace AppInstaller::Repository::Microsoft::Schema::V1_4
StatementBuilder builder;
- // Find all manifest that depend on this package.
- // SELECT [dep].[manifest], [pId].[id], [minV].[version] FROM [dependencies] AS [dep]
- // JOIN [versions] AS [minV] ON [dep].[min_version] = [minV].[rowid]
+ // Find all manifest that depend on this package. Use outer join for joining version table as min_version may be NULL.
+ // SELECT [dep].[manifest], [dep].[min_version], [pId].[id], [minV].[version] FROM [dependencies] AS [dep]
+ // LEFT OUTER JOIN [versions] AS [minV] ON [dep].[min_version] = [minV].[rowid]
// JOIN [ids] AS [pId] ON [pId].[rowid] = [dep].[package_id]
// WHERE [pId].[id] = ?
builder.Select()
.Column(QCol(depTableAlias, s_DependenciesTable_Manifest_Column_Name))
+ .Column(QCol(depTableAlias, s_DependenciesTable_MinVersion_Column_Name))
.Column(QCol(packageIdAlias, IdTable::ValueName()))
.Column(QCol(minVersionAlias, VersionTable::ValueName()))
.From({ s_DependenciesTable_Table_Name }).As(depTableAlias)
- .Join({ VersionTable::TableName() }).As(minVersionAlias)
+ .LeftOuterJoin({ VersionTable::TableName() }).As(minVersionAlias)
.On(QCol(depTableAlias, s_DependenciesTable_MinVersion_Column_Name), QCol(minVersionAlias, SQLite::RowIDName))
.Join({ IdTable::TableName() }).As(packageIdAlias)
.On(QCol(packageIdAlias, SQLite::RowIDName), QCol(depTableAlias, s_DependenciesTable_PackageId_Column_Name))
@@ -350,8 +350,14 @@ namespace AppInstaller::Repository::Microsoft::Schema::V1_4
while (stmt.Step())
{
+ Utility::NormalizedString version = "";
+ if (!stmt.GetColumnIsNull(1))
+ {
+ // If min_version is not NULL, use the corresponding value from Version table.
+ version = stmt.GetColumn<std::string>(3);
+ }
resultSet.emplace_back(
- std::make_pair(stmt.GetColumn<SQLite::rowid_t>(0), Utility::NormalizedString(stmt.GetColumn<std::string>(2))));
+ std::make_pair(stmt.GetColumn<SQLite::rowid_t>(0), std::move(version)));
}
return resultSet;
@@ -366,14 +372,16 @@ namespace AppInstaller::Repository::Microsoft::Schema::V1_4
std::set<std::pair<SQLite::rowid_t, Utility::NormalizedString>> resultSet;
- // SELECT [dep].[package_id], [minV].[version] FROM [dependencies] AS [dep]
- // JOIN [versions] AS [minV] ON [minV].[rowid] = [dep].[min_version]
+ // Use Outer join since min_version could have NULL value.
+ // SELECT [dep].[package_id], [dep].[min_version], [minV].[version] FROM [dependencies] AS [dep]
+ // LEFT OUTER JOIN [versions] AS [minV] ON [minV].[rowid] = [dep].[min_version]
// WHERE [dep].[manifest] = ?
builder.Select()
.Column(QCol(depTableAlias, s_DependenciesTable_PackageId_Column_Name))
+ .Column(QCol(depTableAlias, s_DependenciesTable_MinVersion_Column_Name))
.Column(QCol(minVersionAlias, VersionTable::ValueName()))
.From({ s_DependenciesTable_Table_Name }).As(depTableAlias)
- .Join({ VersionTable::TableName() }).As(minVersionAlias)
+ .LeftOuterJoin({ VersionTable::TableName() }).As(minVersionAlias)
.On(QCol(minVersionAlias, SQLite::RowIDName), QCol(depTableAlias, s_DependenciesTable_MinVersion_Column_Name))
.Where(QCol(depTableAlias, s_DependenciesTable_Manifest_Column_Name)).Equals(Unbound);
@@ -385,9 +393,10 @@ namespace AppInstaller::Repository::Microsoft::Schema::V1_4
Utility::NormalizedString version = "";
if (!select.GetColumnIsNull(1))
{
- version = select.GetColumn<std::string>(1);
+ // If min_version is not NULL, use the corresponding value from Version table.
+ version = select.GetColumn<std::string>(2);
}
- resultSet.emplace(std::make_pair(select.GetColumn<SQLite::rowid_t>(0), version));
+ resultSet.emplace(std::make_pair(select.GetColumn<SQLite::rowid_t>(0), std::move(version)));
}
return resultSet;
diff --git a/src/AppInstallerRepositoryCore/PackageDependenciesValidation.cpp b/src/AppInstallerRepositoryCore/PackageDependenciesValidation.cpp
@@ -110,47 +110,51 @@ namespace AppInstaller::Repository
std::vector<ValidationError> dependenciesError;
bool foundErrors = false;
- DependencyGraph graph(rootId, [&](const Dependency& node) {
-
- DependencyList depList;
- if (node.Id == rootId.Id)
- {
- return GetDependencies(manifest, DependencyType::Package);
- }
-
- auto packageLatest = GetPackageLatestVersion(index, node.Id);
- if (!packageLatest.has_value())
+ DependencyGraph graph
+ {
+ rootId,
+ [&](const Dependency& node)
{
- std::string error = ManifestError::MissingManifestDependenciesNode;
- error.append(" ").append(node.Id);
- dependenciesError.emplace_back(ValidationError(error));
- foundErrors = true;
- return depList;
- }
+ DependencyList depList;
+ if (node.Id == rootId.Id)
+ {
+ return GetDependencies(manifest, DependencyType::Package);
+ }
- if (node.MinVersion > packageLatest.value().second)
- {
- std::string error = ManifestError::NoSuitableMinVersion;
- error.append(" ").append(node.Id);
- dependenciesError.emplace_back(ValidationError(error));
- foundErrors = true;
- return depList;
- }
+ auto packageLatest = GetPackageLatestVersion(index, node.Id);
+ if (!packageLatest.has_value())
+ {
+ std::string error = ManifestError::MissingManifestDependenciesNode;
+ error.append(" ").append(node.Id);
+ dependenciesError.emplace_back(ValidationError(error));
+ foundErrors = true;
+ return depList;
+ }
- auto packageLatestDependencies = index->GetDependenciesByManifestRowId(packageLatest.value().first);
- std::for_each(
- packageLatestDependencies.begin(),
- packageLatestDependencies.end(),
- [&](std::pair<SQLite::rowid_t, Utility::NormalizedString> row)
+ if (node.MinVersion > packageLatest.value().second)
{
- auto manifestRowId = index->GetManifestIdByKey(row.first, "", "");
- auto packageId = index->GetPropertyByManifestId(manifestRowId.value(), PackageVersionProperty::Id);
- Dependency dep(DependencyType::Package, packageId.value(), row.second);
- depList.Add(dep);
- });
+ std::string error = ManifestError::NoSuitableMinVersion;
+ error.append(" ").append(node.Id);
+ dependenciesError.emplace_back(ValidationError(error));
+ foundErrors = true;
+ return depList;
+ }
- return depList;
- });
+ auto packageLatestDependencies = index->GetDependenciesByManifestRowId(packageLatest.value().first);
+ std::for_each(
+ packageLatestDependencies.begin(),
+ packageLatestDependencies.end(),
+ [&](std::pair<SQLite::rowid_t, Utility::NormalizedString> row)
+ {
+ auto manifestRowId = index->GetManifestIdByKey(row.first, "", "");
+ auto packageId = index->GetPropertyByManifestId(manifestRowId.value(), PackageVersionProperty::Id);
+ Dependency dep(DependencyType::Package, packageId.value(), row.second);
+ depList.Add(dep);
+ });
+
+ return depList;
+ }
+ };
graph.BuildGraph();
@@ -228,7 +232,7 @@ namespace AppInstaller::Repository
}
);
- if (breakingManifests.size())
+ if (!breakingManifests.empty())
{
ThrowOnManifestValidationFailed(
breakingManifests, Manifest::ManifestError::MultiManifestPackageHasDependencies);
