dnf5daemon: Fix goal.resolve() return value

To correctly present resolved transaction to the user we need to change the structure of transaction item returned from goal.resolve(). The new transaction item structure consists of: - string object_type. This allows the client to correctly interpret data stored in transaction object map. Value is one of libdnf::transaction::TransactionItemType enum. - string action. One of libdnf::transaction::TransactionItemAction - string reason. One of libdnf::transaction::TransactionItemReason enum - {string:variant} map with other transaction item attributes. Currently used for group id in case of REASON_CHANGE action and GROUP reason. - {string:variant} map with object - package / group / environment / module, according to object_type. All enum values are passed as string using corresponding libdnf::transaction::*_to_string() methods. Server and client can use different versions of libdnf with potentially different enum definitions. Using strings makes communication more reliable.
rpm-software-management · Mar 1, 2023 · 673adfe · 673adfe
1 parent 730bf1b
commit 673adfe
Show file tree

Hide file tree

Showing 8 changed files with 144 additions and 18 deletions.
diff --git a/dnf5daemon-client/wrappers/dbus_goal_wrapper.cpp b/dnf5daemon-client/wrappers/dbus_goal_wrapper.cpp
@@ -19,11 +19,18 @@ along with libdnf.  If not, see <https://www.gnu.org/licenses/>.
 
 #include "dbus_goal_wrapper.hpp"
 
+#include <libdnf/transaction/transaction_item_type.hpp>
+
 namespace dnfdaemon::client {
 
 DbusGoalWrapper::DbusGoalWrapper(std::vector<dnfdaemon::DbusTransactionItem> transaction) {
     for (auto & ti : transaction) {
-        transaction_packages.push_back(DbusTransactionPackageWrapper(ti));
+        auto object_type = libdnf::transaction::transaction_item_type_from_string(std::get<0>(ti));
+        if (object_type == libdnf::transaction::TransactionItemType::PACKAGE) {
+            transaction_packages.push_back(DbusTransactionPackageWrapper(ti));
+        } else if (object_type == libdnf::transaction::TransactionItemType::GROUP) {
+            transaction_groups.push_back(DbusTransactionGroupWrapper(ti));
+        }
     }
 }
 

diff --git a/dnf5daemon-client/wrappers/dbus_transaction_group_wrapper.hpp b/dnf5daemon-client/wrappers/dbus_transaction_group_wrapper.hpp
@@ -34,19 +34,18 @@ namespace dnfdaemon::client {
 class DbusTransactionGroupWrapper {
 public:
     explicit DbusTransactionGroupWrapper(const dnfdaemon::DbusTransactionItem & dti)
-        : group(std::get<1>(dti)),
-          action(static_cast<libdnf::transaction::TransactionItemAction>(std::get<0>(dti))),
-          // TODO(lukash) reason needs to be added to dbus
-          reason(libdnf::transaction::TransactionItemReason::NONE) {}
+        : action(libdnf::transaction::transaction_item_action_from_string(std::get<1>(dti))),
+          reason(libdnf::transaction::transaction_item_reason_from_string(std::get<2>(dti))),
+          group(std::get<4>(dti)) {}
 
     DbusGroupWrapper get_group() const { return group; }
     libdnf::transaction::TransactionItemAction get_action() const { return action; }
     libdnf::transaction::TransactionItemReason get_reason() const { return reason; }
 
 private:
-    DbusGroupWrapper group;
     libdnf::transaction::TransactionItemAction action;
     libdnf::transaction::TransactionItemReason reason;
+    DbusGroupWrapper group;
 };
 
 }  // namespace dnfdaemon::client

diff --git a/dnf5daemon-client/wrappers/dbus_transaction_package_wrapper.hpp b/dnf5daemon-client/wrappers/dbus_transaction_package_wrapper.hpp
@@ -34,10 +34,9 @@ namespace dnfdaemon::client {
 class DbusTransactionPackageWrapper {
 public:
     explicit DbusTransactionPackageWrapper(const dnfdaemon::DbusTransactionItem & dti)
-        : package(std::get<1>(dti)),
-          action(static_cast<libdnf::transaction::TransactionItemAction>(std::get<0>(dti))),
-          // TODO(lukash) reason needs to be added to dbus
-          reason(libdnf::transaction::TransactionItemReason::NONE) {}
+        : action(libdnf::transaction::transaction_item_action_from_string(std::get<1>(dti))),
+          reason(libdnf::transaction::transaction_item_reason_from_string(std::get<2>(dti))),
+          package(std::get<4>(dti)) {}
 
     DbusPackageWrapper get_package() const { return package; }
     libdnf::transaction::TransactionItemAction get_action() const { return action; }
@@ -47,9 +46,9 @@ class DbusTransactionPackageWrapper {
     const std::vector<DbusPackageWrapper> get_replaces() const noexcept { return {}; }
 
 private:
-    DbusPackageWrapper package;
     libdnf::transaction::TransactionItemAction action;
     libdnf::transaction::TransactionItemReason reason;
+    DbusPackageWrapper package;
 };
 
 }  // namespace dnfdaemon::client

diff --git a/dnf5daemon-server/dbus.hpp b/dnf5daemon-server/dbus.hpp
@@ -34,8 +34,12 @@ enum class RepoStatus { NOT_READY, PENDING, READY, ERROR };
 enum class ResolveResult { NO_PROBLEM, WARNING, ERROR };
 
 using DbusTransactionItem = sdbus::Struct<
-    unsigned int,  //action
-    KeyValueMap>;
+    std::string,   // libdnf::transaction::TransactionItemType
+    std::string,   // libdnf::transaction::TransactionItemAction
+    std::string,   // libdnf::transaction::TransactionItemReason
+    KeyValueMap,   // other transaction item attributes - e.g. group id for REASON_CHANGE to GROUP,
+                   // packages that are replaced by this transaction item
+    KeyValueMap>;  // transaction object (package / group / module)
 
 using Changelog = sdbus::Struct<int64_t, std::string, std::string>;
 

diff --git a/dnf5daemon-server/dbus/interfaces/org.rpm.dnf.v0.Goal.xml b/dnf5daemon-server/dbus/interfaces/org.rpm.dnf.v0.Goal.xml
@@ -28,7 +28,7 @@ along with libdnf.  If not, see <https://www.gnu.org/licenses/>.
     <!--
         resolve:
         @options: an array of key/value pairs to modify dependency resolving
-        @transaction: array of (action, {package}) tuples where action is one of libdnf::transaction::TransactionItemAction enum and package is a map {atttibute: value}. These package attributes are returned: "name", "epoch", "version", "release", "arch", "repo", "package_size", "install_size", "evr".
+        @transaction_items: array of (object_type, action, reason, {transaction_item_attributes}, {object}) tuples. object_type is one of libdnf::transaction::TransactionItemType, action is one of libdnf::transaction::TransactionItemAction, reason is one of libdnf::transaction::TransactionItemReason enums. Each of these enums are represented as strings using corresponding libdnf::transaction::*_to_string() methods. transaction_item_attributes is a map {attribute: value}. Currently "reason_change_group_id" attribute is used for target group id in case the action is ReasonChange and the reason Group, and "replaces" containing a vector of packages ids being replaced by this item. Finally the object is a map {attribute: value} and represents (according to object_type) package, group, environment, or module. These package attributes are returned: "name", "epoch", "version", "release", "arch", "repo_id", "from_repo_id", "package_size", "install_size", "evr", and "reason". For groups the object contains "groupid" and "name" attributes.
         @result: problems detected during transaction resolving. Possible values are 0 - no problem, 1 - no problem, but some info / warnings are present or 2 - resolving failed.
 
         Resolve the transaction.
@@ -41,7 +41,7 @@ along with libdnf.  If not, see <https://www.gnu.org/licenses/>.
     -->
     <method name="resolve">
         <arg name="options" type="a{sv}" direction="in" />
-        <arg name="transaction" type="a(ua{sv})" direction="out" />
+        <arg name="transaction_items" type="a(sssa{sv}a{sv})" direction="out" />
         <arg name="result" type="u" direction="out" />
     </method>
 

diff --git a/dnf5daemon-server/group.cpp b/dnf5daemon-server/group.cpp
@@ -0,0 +1,56 @@
+/*
+Copyright Contributors to the libdnf project.
+
+This file is part of libdnf: https://github.com/rpm-software-management/libdnf/
+
+Libdnf is free software: you can redistribute it and/or modify
+it under the terms of the GNU General Public License as published by
+the Free Software Foundation, either version 2 of the License, or
+(at your option) any later version.
+
+Libdnf is distributed in the hope that it will be useful,
+but WITHOUT ANY WARRANTY; without even the implied warranty of
+MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+GNU General Public License for more details.
+
+You should have received a copy of the GNU General Public License
+along with libdnf.  If not, see <https://www.gnu.org/licenses/>.
+*/
+
+#include "group.hpp"
+
+#include <fmt/format.h>
+
+#include <map>
+
+
+// map string group attribute name to actual attribute
+const std::map<std::string, GroupAttribute> group_attributes{
+    {"groupid", GroupAttribute::groupid}, {"name", GroupAttribute::name}, {"description", GroupAttribute::description}};
+
+
+dnfdaemon::KeyValueMap group_to_map(
+    const libdnf::comps::Group & libdnf_group, const std::vector<std::string> & attributes) {
+    dnfdaemon::KeyValueMap dbus_group;
+    // add group id by default
+    dbus_group.emplace(std::make_pair("groupid", libdnf_group.get_groupid()));
+    // attributes required by client
+    for (auto & attr : attributes) {
+        auto it = group_attributes.find(attr);
+        if (it == group_attributes.end()) {
+            throw std::runtime_error(fmt::format("Group attribute '{}' not supported", attr));
+        }
+        switch (it->second) {
+            case GroupAttribute::groupid:
+                // already added by default
+                break;
+            case GroupAttribute::name:
+                dbus_group.emplace(attr, libdnf_group.get_name());
+                break;
+            case GroupAttribute::description:
+                dbus_group.emplace(attr, libdnf_group.get_description());
+                break;
+        }
+    }
+    return dbus_group;
+}
diff --git a/dnf5daemon-server/group.hpp b/dnf5daemon-server/group.hpp
@@ -0,0 +1,41 @@
+/*
+Copyright Contributors to the libdnf project.
+
+This file is part of libdnf: https://github.com/rpm-software-management/libdnf/
+
+Libdnf is free software: you can redistribute it and/or modify
+it under the terms of the GNU General Public License as published by
+the Free Software Foundation, either version 2 of the License, or
+(at your option) any later version.
+
+Libdnf is distributed in the hope that it will be useful,
+but WITHOUT ANY WARRANTY; without even the implied warranty of
+MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+GNU General Public License for more details.
+
+You should have received a copy of the GNU General Public License
+along with libdnf.  If not, see <https://www.gnu.org/licenses/>.
+*/
+
+#ifndef DNF5DAEMON_SERVER_GROUP_HPP
+#define DNF5DAEMON_SERVER_GROUP_HPP
+
+#include "dbus.hpp"
+
+#include <libdnf/comps/group/group.hpp>
+
+#include <string>
+#include <vector>
+
+// group attributes available to be retrieved
+enum class GroupAttribute {
+    groupid,
+    name,
+    description
+    // TODO(mblaha): translated_name, translated_description, packages, reason
+};
+
+dnfdaemon::KeyValueMap group_to_map(
+    const libdnf::comps::Group & libdnf_group, const std::vector<std::string> & attributes);
+
+#endif
diff --git a/dnf5daemon-server/services/goal/goal.cpp b/dnf5daemon-server/services/goal/goal.cpp
@@ -21,6 +21,7 @@ along with libdnf.  If not, see <https://www.gnu.org/licenses/>.
 
 #include "callbacks.hpp"
 #include "dbus.hpp"
+#include "group.hpp"
 #include "package.hpp"
 #include "transaction.hpp"
 #include "utils.hpp"
@@ -29,6 +30,7 @@ along with libdnf.  If not, see <https://www.gnu.org/licenses/>.
 #include <fmt/format.h>
 #include <libdnf/repo/package_downloader.hpp>
 #include <libdnf/transaction/transaction_item.hpp>
+#include <libdnf/transaction/transaction_item_action.hpp>
 #include <sdbus-c++/sdbus-c++.h>
 
 #include <chrono>
@@ -42,7 +44,7 @@ void Goal::dbus_register() {
     // TODO(mblaha) Adjust resolve method to accomodate also groups, environments,
     // and modules as part of the transaction
     dbus_object->registerMethod(
-        dnfdaemon::INTERFACE_GOAL, "resolve", "a{sv}", "a(ua{sv})u", [this](sdbus::MethodCall call) -> void {
+        dnfdaemon::INTERFACE_GOAL, "resolve", "a{sv}", "a(sssa{sv}a{sv})u", [this](sdbus::MethodCall call) -> void {
             session.get_threads_manager().handle_method(*this, &Goal::resolve, call, session.session_locale);
         });
     dbus_object->registerMethod(
@@ -78,7 +80,7 @@ sdbus::MethodReply Goal::resolve(sdbus::MethodCall & call) {
     auto overall_result = dnfdaemon::ResolveResult::ERROR;
     if (transaction.get_problems() == libdnf::GoalProblem::NO_PROBLEM) {
         // return the transaction only if there were no problems
-        std::vector<std::string> attr{
+        std::vector<std::string> pkg_attrs{
             "name",
             "epoch",
             "version",
@@ -91,8 +93,26 @@ sdbus::MethodReply Goal::resolve(sdbus::MethodCall & call) {
             "evr",
             "reason"};
         for (auto & tspkg : transaction.get_transaction_packages()) {
+            dnfdaemon::KeyValueMap trans_item_attrs{};
+            if (tspkg.get_reason_change_group_id()) {
+                trans_item_attrs.emplace("reason_change_group_id", *tspkg.get_reason_change_group_id());
+            }
+            dbus_transaction.push_back(dnfdaemon::DbusTransactionItem(
+                transaction_item_type_to_string(libdnf::transaction::TransactionItemType::PACKAGE),
+                transaction_item_action_to_string(tspkg.get_action()),
+                transaction_item_reason_to_string(tspkg.get_reason()),
+                trans_item_attrs,
+                package_to_map(tspkg.get_package(), pkg_attrs)));
+        }
+        std::vector<std::string> grp_attrs{"name"};
+        dnfdaemon::KeyValueMap trans_item_attrs{};
+        for (auto & tsgrp : transaction.get_transaction_groups()) {
             dbus_transaction.push_back(dnfdaemon::DbusTransactionItem(
-                static_cast<uint32_t>(tspkg.get_action()), package_to_map(tspkg.get_package(), attr)));
+                transaction_item_type_to_string(libdnf::transaction::TransactionItemType::GROUP),
+                transaction_item_action_to_string(tsgrp.get_action()),
+                transaction_item_reason_to_string(tsgrp.get_reason()),
+                trans_item_attrs,
+                group_to_map(tsgrp.get_group(), grp_attrs)));
         }
         // there are transactions resolved without problems but still resolve_logs
         // may contain some warnings / informations