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: