From a96d701f7f55ff475e11ac9cda63b81c31c54e7a Mon Sep 17 00:00:00 2001 From: Daniel Mach 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 #include namespace libdnf { @@ -171,6 +172,21 @@ MergedTransaction::getConsoleOutput() return output; } + +static bool transaction_item_sort_function(const std::shared_ptr lhs, const std::shared_ptr 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: