Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Revert ARP error change and AddOrUpdate return bool in 1.9 #4895

Merged
merged 2 commits into from
Oct 22, 2024
Merged

Revert ARP error change and AddOrUpdate return bool in 1.9 #4895

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
53b7b5d
Revert "Change meaning of AddOrUpdate return bool (#4885)"
ryfu-msft Oct 22, 2024
6097e96
Revert "Make adding overlapping ARP range a hard error (#4870)"
ryfu-msft Oct 22, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
83 changes: 2 additions & 81 deletions src/AppInstallerCLITests/SQLiteIndex.cpp
View file
Open in desktop
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
Open in desktop
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
Open in desktop
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
Open in desktop
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
Open in desktop
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
Open in desktop
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
Open in desktop
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
Open in desktop
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
Loading