Skip to content

Commit

Permalink
Revert ARP error change and AddOrUpdate return bool in 1.9 (#4895)
Browse files Browse the repository at this point in the history
  • Loading branch information
@ryfu-msft
ryfu-msft authored Oct 22, 2024
1 parent a03440d commit 874acff
Show file tree
Hide file tree
Showing 11 changed files with 22 additions and 311 deletions.
83 changes: 2 additions & 81 deletions src/AppInstallerCLITests/SQLiteIndex.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -3241,7 +3241,7 @@ TEST_CASE("SQLiteIndex_RemoveManifestArpVersionKeepUsedDeleteUnused", "[sqlitein
}
}

TEST_CASE("SQLiteIndex_ManifestArpVersionConflict_AddThrows", "[sqliteindex]")
TEST_CASE("SQLiteIndex_ManifestArpVersion_CheckConsistency", "[sqliteindex]")
{
TempFile tempFile{ "repolibtest_tempdb"s, ".db"s };
INFO("Using temporary file named: " << tempFile.GetPath());
Expand All @@ -3267,44 +3267,9 @@ TEST_CASE("SQLiteIndex_ManifestArpVersionConflict_AddThrows", "[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));

// 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);
REQUIRE_FALSE(index.CheckConsistency(true));
}

TEST_CASE("SQLiteIndex_ManifestArpVersion_ValidateManifestAgainstIndex", "[sqliteindex]")
Expand Down Expand Up @@ -3891,47 +3856,3 @@ 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 should return false
REQUIRE(!index.AddOrUpdateManifest(manifest, manifestPath));

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

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

// Update with indexed changes should still return false
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));
}
}
45 changes: 0 additions & 45 deletions src/AppInstallerRepositoryCore/Microsoft/SQLiteIndex.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -109,11 +109,6 @@ 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 @@ -148,11 +143,6 @@ 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 @@ -169,41 +159,6 @@ 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))
{
UpdateManifestInternalHoldingLock(manifest, relativePath);
return false;
}
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: 0 additions & 15 deletions src/AppInstallerRepositoryCore/Microsoft/SQLiteIndex.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -94,18 +94,6 @@ 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 manifest was added (true) or updated (false).
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 manifest was added (true) or updated (false).
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 manifest was added (true) or updated (false).
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 @@ -188,10 +176,7 @@ 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
7 changes: 2 additions & 5 deletions src/AppInstallerRepositoryCore/Microsoft/Schema/1_5/Interface.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -26,11 +26,8 @@ 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:
// Gets the ARP version ranges for the given package identifier.
std::vector<Utility::VersionRange> GetArpVersionRanges(const SQLite::Connection& connection, SQLite::rowid_t packageIdentifier) const;

private:
// Semantic check to validate all arp version ranges within the index
bool ValidateArpVersionConsistency(const SQLite::Connection& connection, bool log) const;
};
}
}
86 changes: 17 additions & 69 deletions src/AppInstallerRepositoryCore/Microsoft/Schema/1_5/Interface_1_5.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,6 @@
#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 @@ -37,32 +36,8 @@ 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, 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();
}

Manifest::string_t arpMinVersion = arpVersionRange.IsEmpty() ? "" : arpVersionRange.GetMinVersion().ToString();
Manifest::string_t arpMaxVersion = arpVersionRange.IsEmpty() ? "" : 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 @@ -107,27 +82,6 @@ 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 @@ -227,26 +181,6 @@ 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 @@ -259,7 +193,21 @@ 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 = GetArpVersionRanges(connection, match.first);
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) } });
}
}

// Check overlap
if (Utility::HasOverlapInVersionRanges(ranges))
Expand Down
20 changes: 0 additions & 20 deletions src/WinGetUtil/Exports.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -192,26 +192,6 @@ 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: 0 additions & 1 deletion src/WinGetUtil/Source.def
View file
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,6 @@ EXPORTS
WinGetSQLiteIndexClose
WinGetSQLiteIndexAddManifest
WinGetSQLiteIndexUpdateManifest
WinGetSQLiteIndexAddOrUpdateManifest
WinGetSQLiteIndexRemoveManifest
WinGetSQLiteIndexPrepareForPackaging
WinGetSQLiteIndexCheckConsistency
Expand Down
8 changes: 0 additions & 8 deletions src/WinGetUtil/WinGetUtil.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -158,14 +158,6 @@ 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 manifest was added (true) or updated (false).
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 874acff

Please sign in to comment.