From a96d701f7f55ff475e11ac9cda63b81c31c54e7a Mon Sep 17 00:00:00 2001
From: Daniel Mach <dmach@redhat.com>
Date: Wed, 6 May 2020 08:34:46 +0200
Subject: [PATCH] history: Fix dnf history rollback when a package was removed
(RhBug:1683134)
---
libdnf/transaction/MergedTransaction.cpp | 25 +++-
.../transaction/MergedTransactionTest.cpp | 120 ++++++++++++++++--
.../transaction/MergedTransactionTest.hpp | 6 +-
3 files changed, 137 insertions(+), 14 deletions(-)
diff --git a/libdnf/transaction/MergedTransaction.cpp b/libdnf/transaction/MergedTransaction.cpp
index a7c06ffa9..a8d878cb5 100644
--- a/libdnf/transaction/MergedTransaction.cpp
+++ b/libdnf/transaction/MergedTransaction.cpp
@@ -19,6 +19,7 @@
*/
#include "MergedTransaction.hpp"
+#include <algorithm>
#include <vector>
namespace libdnf {
@@ -171,6 +172,21 @@ MergedTransaction::getConsoleOutput()
return output;
}
+
+static bool transaction_item_sort_function(const std::shared_ptr<TransactionItemBase> lhs, const std::shared_ptr<TransactionItemBase> rhs) {
+ if (lhs->isForwardAction() && rhs->isForwardAction()) {
+ return false;
+ }
+ if (lhs->isBackwardAction() && rhs->isBackwardAction()) {
+ return false;
+ }
+ if (lhs->isBackwardAction()) {
+ return true;
+ }
+ return false;
+}
+
+
/**
* Get list of transaction items involved in the merged transaction
* Actions are merged using following rules:
@@ -203,6 +219,9 @@ MergedTransaction::getItems()
// iterate over transaction
for (auto t : transactions) {
auto transItems = t->getItems();
+ // sort transaction items by their action type - forward/backward
+ // this fixes behavior of the merging algorithm in several edge cases
+ std::sort(transItems.begin(), transItems.end(), transaction_item_sort_function);
// iterate over transaction items
for (auto transItem : transItems) {
// get item and its type
@@ -383,10 +402,6 @@ MergedTransaction::mergeItem(ItemPairMap &itemPairMap, TransactionItemBasePtr mT
auto firstState = previousItemPair.first->getAction();
auto newState = mTransItem->getAction();
- if (firstState == TransactionItemAction::INSTALL && mTransItem->isBackwardAction()) {
- return;
- }
-
switch (firstState) {
case TransactionItemAction::REMOVE:
case TransactionItemAction::OBSOLETED:
@@ -399,6 +414,8 @@ MergedTransaction::mergeItem(ItemPairMap &itemPairMap, TransactionItemBasePtr mT
// Install -> Remove = (nothing)
itemPairMap.erase(name);
break;
+ } else if (mTransItem->isBackwardAction()) {
+ break;
}
// altered -> transfer install to the altered package
mTransItem->setAction(TransactionItemAction::INSTALL);
diff --git a/tests/libdnf/transaction/MergedTransactionTest.cpp b/tests/libdnf/transaction/MergedTransactionTest.cpp
index 90ad182cf..52507700b 100644
--- a/tests/libdnf/transaction/MergedTransactionTest.cpp
+++ b/tests/libdnf/transaction/MergedTransactionTest.cpp
@@ -700,7 +700,7 @@ MergedTransactionTest::test_downgrade()
}
void
-MergedTransactionTest::test_install_downgrade()
+MergedTransactionTest::test_install_downgrade_upgrade_remove()
{
auto trans1 = std::make_shared< libdnf::swdb_private::Transaction >(conn);
trans1->addItem(
@@ -724,19 +724,123 @@ MergedTransactionTest::test_install_downgrade()
TransactionItemReason::USER
);
+ auto trans3 = std::make_shared< libdnf::swdb_private::Transaction >(conn);
+ trans3->addItem(
+ nevraToRPMItem(conn, "tour-0:4.6-1.noarch"),
+ "repo2",
+ TransactionItemAction::UPGRADED,
+ TransactionItemReason::USER
+ );
+ trans3->addItem(
+ nevraToRPMItem(conn, "tour-0:4.8-1.noarch"),
+ "repo1",
+ TransactionItemAction::UPGRADE,
+ TransactionItemReason::USER
+ );
+
+ auto trans4 = std::make_shared< libdnf::swdb_private::Transaction >(conn);
+ trans4->addItem(
+ nevraToRPMItem(conn, "tour-0:4.8-1.noarch"),
+ "repo1",
+ TransactionItemAction::REMOVE,
+ TransactionItemReason::USER
+ );
+
MergedTransaction merged(trans1);
+
+ // test merging trans1, trans2
merged.merge(trans2);
+ auto items2 = merged.getItems();
+ CPPUNIT_ASSERT_EQUAL(1, (int)items2.size());
+ auto item2 = items2.at(0);
+ CPPUNIT_ASSERT_EQUAL(std::string("tour-4.6-1.noarch"), item2->getItem()->toStr());
+ CPPUNIT_ASSERT_EQUAL(std::string("repo2"), item2->getRepoid());
+ CPPUNIT_ASSERT_EQUAL(TransactionItemAction::INSTALL, item2->getAction());
+ CPPUNIT_ASSERT_EQUAL(TransactionItemReason::USER, item2->getReason());
- auto items = merged.getItems();
- CPPUNIT_ASSERT_EQUAL(1, (int)items.size());
+ // test merging trans1, trans2, trans3
+ merged.merge(trans3);
+ auto items3 = merged.getItems();
+ CPPUNIT_ASSERT_EQUAL(1, (int)items3.size());
+ auto item3 = items3.at(0);
+ CPPUNIT_ASSERT_EQUAL(std::string("tour-4.8-1.noarch"), item3->getItem()->toStr());
+ CPPUNIT_ASSERT_EQUAL(std::string("repo1"), item3->getRepoid());
+ CPPUNIT_ASSERT_EQUAL(TransactionItemAction::INSTALL, item3->getAction());
+ CPPUNIT_ASSERT_EQUAL(TransactionItemReason::USER, item3->getReason());
- auto item = items.at(0);
- CPPUNIT_ASSERT_EQUAL(std::string("tour-4.6-1.noarch"), item->getItem()->toStr());
- CPPUNIT_ASSERT_EQUAL(std::string("repo2"), item->getRepoid());
- CPPUNIT_ASSERT_EQUAL(TransactionItemAction::INSTALL, item->getAction());
- CPPUNIT_ASSERT_EQUAL(TransactionItemReason::USER, item->getReason());
+ // test merging trans1, trans2, trans3, trans4
+ merged.merge(trans4);
+ auto items4 = merged.getItems();
+ CPPUNIT_ASSERT_EQUAL(0, (int)items4.size());
+ // trans4 removes the package, empty output is expected
}
+
+void
+MergedTransactionTest::test_downgrade_upgrade_remove()
+{
+ auto trans1 = std::make_shared< libdnf::swdb_private::Transaction >(conn);
+ trans1->addItem(
+ nevraToRPMItem(conn, "tour-0:4.6-1.noarch"),
+ "repo2",
+ TransactionItemAction::DOWNGRADE,
+ TransactionItemReason::USER
+ );
+ trans1->addItem(
+ nevraToRPMItem(conn, "tour-0:4.8-1.noarch"),
+ "repo1",
+ TransactionItemAction::DOWNGRADED,
+ TransactionItemReason::USER
+ );
+
+ // items are in reversed order than in test_install_downgrade_upgrade_remove()
+ // fixing this required ordering transaction items by forward/backward action
+ auto trans2 = std::make_shared< libdnf::swdb_private::Transaction >(conn);
+ trans2->addItem(
+ nevraToRPMItem(conn, "tour-0:4.8-1.noarch"),
+ "repo1",
+ TransactionItemAction::UPGRADE,
+ TransactionItemReason::USER
+ );
+ trans2->addItem(
+ nevraToRPMItem(conn, "tour-0:4.6-1.noarch"),
+ "repo2",
+ TransactionItemAction::UPGRADED,
+ TransactionItemReason::USER
+ );
+
+ auto trans3 = std::make_shared< libdnf::swdb_private::Transaction >(conn);
+ trans3->addItem(
+ nevraToRPMItem(conn, "tour-0:4.8-1.noarch"),
+ "repo1",
+ TransactionItemAction::REMOVE,
+ TransactionItemReason::USER
+ );
+
+ MergedTransaction merged(trans1);
+
+ // test merging trans1, trans2
+ merged.merge(trans2);
+ auto items2 = merged.getItems();
+ CPPUNIT_ASSERT_EQUAL(1, (int)items2.size());
+ auto item2 = items2.at(0);
+ CPPUNIT_ASSERT_EQUAL(std::string("tour-4.8-1.noarch"), item2->getItem()->toStr());
+ CPPUNIT_ASSERT_EQUAL(std::string("repo1"), item2->getRepoid());
+ CPPUNIT_ASSERT_EQUAL(TransactionItemAction::REINSTALL, item2->getAction());
+ CPPUNIT_ASSERT_EQUAL(TransactionItemReason::USER, item2->getReason());
+
+ // test merging trans1, trans2, trans3
+ merged.merge(trans3);
+ auto items3 = merged.getItems();
+ CPPUNIT_ASSERT_EQUAL(1, (int)items3.size());
+ auto item3 = items3.at(0);
+ CPPUNIT_ASSERT_EQUAL(std::string("tour-4.8-1.noarch"), item3->getItem()->toStr());
+ CPPUNIT_ASSERT_EQUAL(std::string("repo1"), item3->getRepoid());
+ CPPUNIT_ASSERT_EQUAL(TransactionItemAction::REMOVE, item3->getAction());
+ CPPUNIT_ASSERT_EQUAL(TransactionItemReason::USER, item3->getReason());
+}
+
+
void
MergedTransactionTest::test_multilib_identity()
{
diff --git a/tests/libdnf/transaction/MergedTransactionTest.hpp b/tests/libdnf/transaction/MergedTransactionTest.hpp
index 9f1ed660a..77585e865 100644
--- a/tests/libdnf/transaction/MergedTransactionTest.hpp
+++ b/tests/libdnf/transaction/MergedTransactionTest.hpp
@@ -26,7 +26,8 @@ class MergedTransactionTest : public CppUnit::TestCase {
CPPUNIT_TEST(test_add_obsoleted_obsoleted);
CPPUNIT_TEST(test_downgrade);
- CPPUNIT_TEST(test_install_downgrade);
+ CPPUNIT_TEST(test_install_downgrade_upgrade_remove);
+ CPPUNIT_TEST(test_downgrade_upgrade_remove);
CPPUNIT_TEST(test_multilib_identity);
@@ -56,7 +57,8 @@ class MergedTransactionTest : public CppUnit::TestCase {
// END: tests ported from DNF unit tests
void test_downgrade();
- void test_install_downgrade();
+ void test_install_downgrade_upgrade_remove();
+ void test_downgrade_upgrade_remove();
void test_multilib_identity();
private: