Skip to content

Commit

Permalink
Make adding overlapping ARP range a hard error (microsoft#4870)
Browse files Browse the repository at this point in the history
## Change
Implements two independent but related changes:
1. Makes adding/updating an overlapping ARP range a hard error
2. Adds `AddOrUpdate` functionality to the index so that callers don't
need to know if the manifest is already present

## Validation
Added/updated unit tests for ARP overlap.
Added unit and interop test for AddOrUpdate.
  • Loading branch information
JohnMcPMS authored Oct 11, 2024
1 parent 19ce709 commit e80ca4a
Show file tree
Hide file tree
Showing 11 changed files with 310 additions and 22 deletions.
83 changes: 81 additions & 2 deletions src/AppInstallerCLITests/SQLiteIndex.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -3241,7 +3241,7 @@ TEST_CASE("SQLiteIndex_RemoveManifestArpVersionKeepUsedDeleteUnused", "[sqlitein
}
}

TEST_CASE("SQLiteIndex_ManifestArpVersion_CheckConsistency", "[sqliteindex]")
TEST_CASE("SQLiteIndex_ManifestArpVersionConflict_AddThrows", "[sqliteindex]")
{
TempFile tempFile{ "repolibtest_tempdb"s, ".db"s };
INFO("Using temporary file named: " << tempFile.GetPath());
Expand All @@ -3267,9 +3267,44 @@ TEST_CASE("SQLiteIndex_ManifestArpVersion_CheckConsistency", "[sqliteindex]")
// Add a conflicting one
manifest.Version = "10.1";

REQUIRE_THROWS_HR(index.AddManifest(manifest, "path2"), APPINSTALLER_CLI_ERROR_ARP_VERSION_VALIDATION_FAILED);
}

TEST_CASE("SQLiteIndex_ManifestArpVersionConflict_UpdateThrows", "[sqliteindex]")
{
TempFile tempFile{ "repolibtest_tempdb"s, ".db"s };
INFO("Using temporary file named: " << tempFile.GetPath());

SQLiteIndex index = CreateTestIndex(tempFile, SQLiteVersion::Latest());

Manifest manifest;
manifest.Id = "Foo";
manifest.Version = "10.0";
manifest.DefaultLocalization.Add<Localization::PackageName>("ArpVersionCheckConsistencyTest");
manifest.Moniker = "testmoniker";
manifest.Installers.push_back({});
manifest.Installers[0].BaseInstallerType = InstallerTypeEnum::Exe;
manifest.Installers[0].AppsAndFeaturesEntries.push_back({});
manifest.Installers[0].AppsAndFeaturesEntries[0].DisplayVersion = "1.0";
manifest.Installers[0].AppsAndFeaturesEntries.push_back({});
manifest.Installers[0].AppsAndFeaturesEntries[1].DisplayVersion = "1.1";

index.AddManifest(manifest, "path");
REQUIRE(index.CheckConsistency(true));

// Add another version
manifest.Version = "10.1";
manifest.Installers[0].AppsAndFeaturesEntries[0].DisplayVersion = "2.0";
manifest.Installers[0].AppsAndFeaturesEntries[1].DisplayVersion = "2.1";

index.AddManifest(manifest, "path2");
REQUIRE(index.CheckConsistency(true));

REQUIRE_FALSE(index.CheckConsistency(true));
// Update to a conflict
manifest.Installers[0].AppsAndFeaturesEntries[0].DisplayVersion = "1.0";
manifest.Installers[0].AppsAndFeaturesEntries[1].DisplayVersion = "2.1";

REQUIRE_THROWS_HR(index.UpdateManifest(manifest, "path2"), APPINSTALLER_CLI_ERROR_ARP_VERSION_VALIDATION_FAILED);
}

TEST_CASE("SQLiteIndex_ManifestArpVersion_ValidateManifestAgainstIndex", "[sqliteindex]")
Expand Down Expand Up @@ -3856,3 +3891,47 @@ TEST_CASE("SQLiteIndex_DependencyWithCaseMismatch", "[sqliteindex][V1_4]")

index.AddManifest(manifest, GetPathFromManifest(manifest));
}

TEST_CASE("SQLiteIndex_AddOrUpdateManifest", "[sqliteindex]")
{
TempFile tempFile{ "repolibtest_tempdb"s, ".db"s };
INFO("Using temporary file named: " << tempFile.GetPath());

std::string manifestPath = "test/id/test.id-1.0.0.yaml";
Manifest manifest;
manifest.Installers.push_back({});
manifest.Id = "test.id";
manifest.DefaultLocalization.Add < Localization::PackageName>("Test Name");
manifest.Moniker = "testmoniker";
manifest.Version = "1.0.0";
manifest.Channel = "test";
manifest.DefaultLocalization.Add<Localization::Tags>({ "t1", "t2" });
manifest.Installers[0].Commands = { "test1", "test2" };

{
auto version = GENERATE(SQLiteVersion{ 1, 0 }, SQLiteVersion::Latest());
SQLiteIndex index = SQLiteIndex::CreateNew(tempFile, version);

REQUIRE(index.AddOrUpdateManifest(manifest, manifestPath));
}

{
SQLiteIndex index = SQLiteIndex::Open(tempFile, SQLiteStorageBase::OpenDisposition::ReadWrite);

// Update with no updates should return false
REQUIRE(!index.AddOrUpdateManifest(manifest, manifestPath));

manifest.DefaultLocalization.Add<Localization::Description>("description2");

// Update with no indexed updates should return false
REQUIRE(!index.AddOrUpdateManifest(manifest, manifestPath));

// Update with indexed changes
manifest.DefaultLocalization.Add<Localization::PackageName>("Test Name2");
manifest.Moniker = "testmoniker2";
manifest.DefaultLocalization.Add<Localization::Tags>({ "t1", "t2", "t3" });
manifest.Installers[0].Commands = {};

REQUIRE(index.AddOrUpdateManifest(manifest, manifestPath));
}
}
44 changes: 44 additions & 0 deletions src/AppInstallerRepositoryCore/Microsoft/SQLiteIndex.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -109,6 +109,11 @@ namespace AppInstaller::Repository::Microsoft
SQLiteIndex::IdType SQLiteIndex::AddManifestInternal(const Manifest::Manifest& manifest, const std::optional<std::filesystem::path>& relativePath)
{
std::lock_guard<std::mutex> lockInterface{ *m_interfaceLock };
return AddManifestInternalHoldingLock(manifest, relativePath);
}

SQLiteIndex::IdType SQLiteIndex::AddManifestInternalHoldingLock(const Manifest::Manifest& manifest, const std::optional<std::filesystem::path>& relativePath)
{
AICLI_LOG(Repo, Verbose, << "Adding manifest for [" << manifest.Id << ", " << manifest.Version << "] at relative path [" << relativePath.value_or("") << "]");

SQLite::Savepoint savepoint = SQLite::Savepoint::Create(m_dbconn, "sqliteindex_addmanifest");
Expand Down Expand Up @@ -143,6 +148,11 @@ namespace AppInstaller::Repository::Microsoft
bool SQLiteIndex::UpdateManifestInternal(const Manifest::Manifest& manifest, const std::optional<std::filesystem::path>& relativePath)
{
std::lock_guard<std::mutex> lockInterface{ *m_interfaceLock };
return UpdateManifestInternalHoldingLock(manifest, relativePath);
}

bool SQLiteIndex::UpdateManifestInternalHoldingLock(const Manifest::Manifest& manifest, const std::optional<std::filesystem::path>& relativePath)
{
AICLI_LOG(Repo, Verbose, << "Updating manifest for [" << manifest.Id << ", " << manifest.Version << "] at relative path [" << relativePath.value_or("") << "]");

SQLite::Savepoint savepoint = SQLite::Savepoint::Create(m_dbconn, "sqliteindex_updatemanifest");
Expand All @@ -159,6 +169,40 @@ namespace AppInstaller::Repository::Microsoft
return result;
}

bool SQLiteIndex::AddOrUpdateManifest(const std::filesystem::path& manifestPath, const std::filesystem::path& relativePath)
{
AICLI_LOG(Repo, Verbose, << "Adding or Updating manifest from file [" << manifestPath << "]");

Manifest::Manifest manifest = Manifest::YamlParser::CreateFromPath(manifestPath);
return AddOrUpdateManifestInternal(manifest, relativePath);
}

bool SQLiteIndex::AddOrUpdateManifest(const Manifest::Manifest& manifest, const std::filesystem::path& relativePath)
{
return AddOrUpdateManifestInternal(manifest, relativePath);
}

bool SQLiteIndex::AddOrUpdateManifest(const Manifest::Manifest& manifest)
{
return AddOrUpdateManifestInternal(manifest, {});
}

bool SQLiteIndex::AddOrUpdateManifestInternal(const Manifest::Manifest& manifest, const std::optional<std::filesystem::path>& relativePath)
{
std::lock_guard<std::mutex> lockInterface{ *m_interfaceLock };
AICLI_LOG(Repo, Verbose, << "Adding or Updating manifest for [" << manifest.Id << ", " << manifest.Version << "] at relative path [" << relativePath.value_or("") << "]");

if (m_interface->GetManifestIdByManifest(m_dbconn, manifest))
{
return UpdateManifestInternalHoldingLock(manifest, relativePath);
}
else
{
AddManifestInternalHoldingLock(manifest, relativePath);
return true;
}
}

void SQLiteIndex::RemoveManifest(const std::filesystem::path& manifestPath, const std::filesystem::path& relativePath)
{
AICLI_LOG(Repo, Verbose, << "Removing manifest from file [" << manifestPath << "]");
Expand Down
15 changes: 15 additions & 0 deletions src/AppInstallerRepositoryCore/Microsoft/SQLiteIndex.h
Original file line number Diff line number Diff line change
Expand Up @@ -94,6 +94,18 @@ namespace AppInstaller::Repository::Microsoft
// The return value indicates whether the index was modified by the function.
bool UpdateManifest(const Manifest::Manifest& manifest);

// Adds or updates the manifest with matching { Id, Version, Channel } in the index.
// The return value indicates whether the index was modified by the function.
bool AddOrUpdateManifest(const std::filesystem::path& manifestPath, const std::filesystem::path& relativePath);

// Updates the manifest with matching { Id, Version, Channel } in the index.
// The return value indicates whether the index was modified by the function.
bool AddOrUpdateManifest(const Manifest::Manifest& manifest, const std::filesystem::path& relativePath);

// Updates the manifest with matching { Id, Version, Channel } in the index.
// The return value indicates whether the index was modified by the function.
bool AddOrUpdateManifest(const Manifest::Manifest& manifest);

// Removes the manifest with matching { Id, Version, Channel } from the index.
void RemoveManifest(const std::filesystem::path& manifestPath, const std::filesystem::path& relativePath);

Expand Down Expand Up @@ -176,7 +188,10 @@ namespace AppInstaller::Repository::Microsoft

// Internal functions to normalize on the relativePath being present.
IdType AddManifestInternal(const Manifest::Manifest& manifest, const std::optional<std::filesystem::path>& relativePath);
IdType AddManifestInternalHoldingLock(const Manifest::Manifest& manifest, const std::optional<std::filesystem::path>& relativePath);
bool UpdateManifestInternal(const Manifest::Manifest& manifest, const std::optional<std::filesystem::path>& relativePath);
bool UpdateManifestInternalHoldingLock(const Manifest::Manifest& manifest, const std::optional<std::filesystem::path>& relativePath);
bool AddOrUpdateManifestInternal(const Manifest::Manifest& manifest, const std::optional<std::filesystem::path>& relativePath);

std::unique_ptr<Schema::ISQLiteIndex> m_interface;
Schema::SQLiteIndexContextData m_contextData;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -26,8 +26,11 @@ namespace AppInstaller::Repository::Microsoft::Schema::V1_5
// Gets a property already knowing that the manifest id is valid.
std::optional<std::string> GetPropertyByManifestIdInternal(const SQLite::Connection& connection, SQLite::rowid_t manifestId, PackageVersionProperty property) const override;

private:
private:
// Gets the ARP version ranges for the given package identifier.
std::vector<Utility::VersionRange> GetArpVersionRanges(const SQLite::Connection& connection, SQLite::rowid_t packageIdentifier) const;

// Semantic check to validate all arp version ranges within the index
bool ValidateArpVersionConsistency(const SQLite::Connection& connection, bool log) const;
};
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@
#include "Microsoft/Schema/1_5/Interface.h"
#include "Microsoft/Schema/1_5/ArpVersionVirtualTable.h"
#include "Microsoft/Schema/1_0/ManifestTable.h"
#include "Microsoft/Schema/1_0/IdTable.h"
#include "Microsoft/Schema/1_0/VersionTable.h"

namespace AppInstaller::Repository::Microsoft::Schema::V1_5
Expand Down Expand Up @@ -36,8 +37,32 @@ namespace AppInstaller::Repository::Microsoft::Schema::V1_5
SQLite::rowid_t manifestId = V1_4::Interface::AddManifest(connection, manifest, relativePath);

auto arpVersionRange = manifest.GetArpVersionRange();
Manifest::string_t arpMinVersion = arpVersionRange.IsEmpty() ? "" : arpVersionRange.GetMinVersion().ToString();
Manifest::string_t arpMaxVersion = arpVersionRange.IsEmpty() ? "" : arpVersionRange.GetMaxVersion().ToString();
Manifest::string_t arpMinVersion, arpMaxVersion;

if (!arpVersionRange.IsEmpty())
{
// Check to see if adding this version range will create a conflict
SQLite::rowid_t packageIdentifier = V1_0::ManifestTable::GetIdById<V1_0::IdTable>(connection, manifestId).value();
std::vector<Utility::VersionRange> ranges = GetArpVersionRanges(connection, packageIdentifier);
ranges.push_back(arpVersionRange);

if (Utility::HasOverlapInVersionRanges(ranges))
{
AICLI_LOG(Repo, Error, << "Overlapped Arp version ranges found for package. All ranges currently in index followed by new range:\n" << [&]() {
std::stringstream stream;
for (const auto& range : ranges)
{
stream << '[' << range.GetMinVersion().ToString() << "] - [" << range.GetMaxVersion().ToString() << "]\n";
}
return std::move(stream).str();
}());
THROW_HR(APPINSTALLER_CLI_ERROR_ARP_VERSION_VALIDATION_FAILED);
}

arpMinVersion = arpVersionRange.GetMinVersion().ToString();
arpMaxVersion = arpVersionRange.GetMaxVersion().ToString();
}

SQLite::rowid_t arpMinVersionId = V1_0::VersionTable::EnsureExists(connection, arpMinVersion);
SQLite::rowid_t arpMaxVersionId = V1_0::VersionTable::EnsureExists(connection, arpMaxVersion);
V1_0::ManifestTable::UpdateValueIdById<ArpMinVersionVirtualTable>(connection, manifestId, arpMinVersionId);
Expand Down Expand Up @@ -82,6 +107,27 @@ namespace AppInstaller::Repository::Microsoft::Schema::V1_5
indexModified = true;
}

if (!arpVersionRange.IsEmpty())
{
// Check to see if the new set of version ranges created a conflict.
// We could have done this before attempting the update but it would be more complex and SQLite gives us easy rollback.
SQLite::rowid_t packageIdentifier = V1_0::ManifestTable::GetIdById<V1_0::IdTable>(connection, manifestId).value();
std::vector<Utility::VersionRange> ranges = GetArpVersionRanges(connection, packageIdentifier);

if (Utility::HasOverlapInVersionRanges(ranges))
{
AICLI_LOG(Repo, Error, << "Overlapped Arp version ranges found for package. Ranges that would be present with attempted upgrade:\n" << [&]() {
std::stringstream stream;
for (const auto& range : ranges)
{
stream << '[' << range.GetMinVersion().ToString() << "] - [" << range.GetMaxVersion().ToString() << "]\n";
}
return std::move(stream).str();
}());
THROW_HR(APPINSTALLER_CLI_ERROR_ARP_VERSION_VALIDATION_FAILED);
}
}

if (cleanOldMinVersionId && NotNeeded(connection, V1_0::VersionTable::TableName(), V1_0::VersionTable::ValueName(), oldMinVersionId))
{
V1_0::VersionTable::DeleteById(connection, oldMinVersionId);
Expand Down Expand Up @@ -181,6 +227,26 @@ namespace AppInstaller::Repository::Microsoft::Schema::V1_5
}
}

std::vector<Utility::VersionRange> Interface::GetArpVersionRanges(const SQLite::Connection& connection, SQLite::rowid_t packageIdentifier) const
{
std::vector<Utility::VersionRange> ranges;
auto versionKeys = GetVersionKeysById(connection, packageIdentifier);
for (auto const& versionKey : versionKeys)
{
auto arpMinVersion = GetPropertyByPrimaryId(connection, versionKey.ManifestId, PackageVersionProperty::ArpMinVersion).value_or("");
auto arpMaxVersion = GetPropertyByPrimaryId(connection, versionKey.ManifestId, PackageVersionProperty::ArpMaxVersion).value_or("");

// Either both empty or both not empty
THROW_HR_IF(E_UNEXPECTED, arpMinVersion.empty() != arpMaxVersion.empty());

if (!arpMinVersion.empty() && !arpMaxVersion.empty())
{
ranges.emplace_back(Utility::VersionRange{ Utility::Version{ std::move(arpMinVersion) }, Utility::Version{ std::move(arpMaxVersion) } });
}
}
return ranges;
}

bool Interface::ValidateArpVersionConsistency(const SQLite::Connection& connection, bool log) const
{
try
Expand All @@ -193,21 +259,7 @@ namespace AppInstaller::Repository::Microsoft::Schema::V1_5
for (auto const& match : searchResult.Matches)
{
// Get arp version ranges for each package to check
std::vector<Utility::VersionRange> ranges;
auto versionKeys = GetVersionKeysById(connection, match.first);
for (auto const& versionKey : versionKeys)
{
auto arpMinVersion = GetPropertyByPrimaryId(connection, versionKey.ManifestId, PackageVersionProperty::ArpMinVersion).value_or("");
auto arpMaxVersion = GetPropertyByPrimaryId(connection, versionKey.ManifestId, PackageVersionProperty::ArpMaxVersion).value_or("");

// Either both empty or both not empty
THROW_HR_IF(E_UNEXPECTED, arpMinVersion.empty() != arpMaxVersion.empty());

if (!arpMinVersion.empty() && !arpMaxVersion.empty())
{
ranges.emplace_back(Utility::VersionRange{ Utility::Version{ std::move(arpMinVersion) }, Utility::Version{ std::move(arpMaxVersion) } });
}
}
std::vector<Utility::VersionRange> ranges = GetArpVersionRanges(connection, match.first);

// Check overlap
if (Utility::HasOverlapInVersionRanges(ranges))
Expand Down
20 changes: 20 additions & 0 deletions src/WinGetUtil/Exports.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -192,6 +192,26 @@ extern "C"
}
CATCH_RETURN()

WINGET_UTIL_API WinGetSQLiteIndexAddOrUpdateManifest(
WINGET_SQLITE_INDEX_HANDLE index,
WINGET_STRING manifestPath,
WINGET_STRING relativePath,
BOOL* indexModified) try
{
THROW_HR_IF(E_INVALIDARG, !index);
THROW_HR_IF(E_INVALIDARG, !manifestPath);
THROW_HR_IF(E_INVALIDARG, !relativePath);

bool result = reinterpret_cast<SQLiteIndex*>(index)->AddOrUpdateManifest(manifestPath, relativePath);
if (indexModified)
{
*indexModified = (result ? TRUE : FALSE);
}

return S_OK;
}
CATCH_RETURN()

WINGET_UTIL_API WinGetSQLiteIndexRemoveManifest(
WINGET_SQLITE_INDEX_HANDLE index,
WINGET_STRING manifestPath,
Expand Down
1 change: 1 addition & 0 deletions src/WinGetUtil/Source.def
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@ EXPORTS
WinGetSQLiteIndexClose
WinGetSQLiteIndexAddManifest
WinGetSQLiteIndexUpdateManifest
WinGetSQLiteIndexAddOrUpdateManifest
WinGetSQLiteIndexRemoveManifest
WinGetSQLiteIndexPrepareForPackaging
WinGetSQLiteIndexCheckConsistency
Expand Down
8 changes: 8 additions & 0 deletions src/WinGetUtil/WinGetUtil.h
Original file line number Diff line number Diff line change
Expand Up @@ -158,6 +158,14 @@ extern "C"
WINGET_STRING relativePath,
BOOL* indexModified);

// Adds or Updates the manifest with matching { Id, Version, Channel } in the index.
// The return value indicates whether the index was modified by the function.
WINGET_UTIL_API WinGetSQLiteIndexAddOrUpdateManifest(
WINGET_SQLITE_INDEX_HANDLE index,
WINGET_STRING manifestPath,
WINGET_STRING relativePath,
BOOL* indexModified);

// Removes the manifest with matching { Id, Version, Channel } from the index.
// Path is currently ignored.
WINGET_UTIL_API WinGetSQLiteIndexRemoveManifest(
Expand Down
Loading

0 comments on commit e80ca4a

Please sign in to comment.