modules: Respect defaults when enabling multiple streams of a module

dnf5/commands/module/module_enable.cpp

@@ -44,6 +44,7 @@ void ModuleEnableCommand::set_argument_parser() {
     keys->set_complete_hook_func([&ctx](const char * arg) { return match_specs(ctx, arg, false, true, true, false); });
     cmd.register_positional_arg(keys);
 
+    auto skip_broken = std::make_unique<SkipBrokenOption>(*this);
     auto skip_unavailable = std::make_unique<SkipUnavailableOption>(*this);
 }
 

include/libdnf5/base/goal_elements.hpp

@@ -81,7 +81,8 @@ enum class GoalProblem : uint32_t {
     ALREADY_INSTALLED = (1 << 12),
     SOLVER_PROBLEM_STRICT_RESOLVEMENT = (1 << 13),
     WRITE_DEBUG = (1 << 14),
-    UNSUPPORTED_ACTION = (1 << 15)
+    UNSUPPORTED_ACTION = (1 << 15),
+    MULTIPLE_STREAMS = (1 << 16)
 };
 
 /// Types of Goal actions

libdnf5/base/goal.cpp

@@ -569,9 +569,25 @@ GoalProblem Goal::Impl::add_module_specs_to_goal(base::Transaction & transaction
     for (auto & [action, spec, settings] : module_specs) {
         try {
             switch (action) {
-                case GoalAction::ENABLE:
-                    module_sack.p_impl->enable(spec);
+                case GoalAction::ENABLE: {
+                    bool skip_broken = settings.resolve_skip_broken(base->get_config());
+                    auto log_level = skip_broken ? libdnf5::Logger::Level::WARNING : libdnf5::Logger::Level::ERROR;
+                    const auto & enable_ret = module_sack.p_impl->enable(spec);
+                    if (!enable_ret.second.empty()) {
+                        transaction.p_impl->add_resolve_log(
+                            action,
+                            GoalProblem::MULTIPLE_STREAMS,
+                            GoalJobSettings(),
+                            libdnf5::transaction::TransactionItemType::MODULE,
+                            spec,
+                            enable_ret.second,
+                            log_level);
+                        if (!skip_broken) {
+                            ret |= GoalProblem::MULTIPLE_STREAMS;
+                        }
+                    }
                     break;
+                }
                 case GoalAction::DISABLE:
                     module_sack.p_impl->disable(spec);
                     break;
@@ -588,7 +604,7 @@ GoalProblem Goal::Impl::add_module_specs_to_goal(base::Transaction & transaction
                 action,
                 GoalProblem::NOT_FOUND,
                 GoalJobSettings(),
-                libdnf5::transaction::TransactionItemType::GROUP,
+                libdnf5::transaction::TransactionItemType::MODULE,
                 spec,
                 {},
                 log_level);

libdnf5/base/log_event.cpp

@@ -163,6 +163,27 @@ std::string LogEvent::to_string(
         case GoalProblem::UNSUPPORTED_ACTION:
             return ret.append(utils::sformat(
                 _("{} action for argument \"{}\" is not supported."), goal_action_to_string(action), *spec));
+        case GoalProblem::MULTIPLE_STREAMS: {
+            // Create module dict { name : { stream } } out of the additional data.
+            std::map<std::string, std::set<std::string>> module_dict;
+            for (const auto & module_stream : additional_data) {
+                const auto pos = module_stream.find(":");
+                module_dict[module_stream.substr(0, pos)].insert(module_stream.substr(pos + 1));
+            }
+            // Create the error message describing all the streams of modules that were matched.
+            std::string error_message;
+            for (const auto & module_dict_iter : module_dict) {
+                error_message.append(utils::sformat(
+                    _("\n  - Argument '{}' matches {} streams ('{}') of module '{}', but none of the streams are "
+                      "enabled or "
+                      "default."),
+                    *spec,
+                    module_dict_iter.second.size(),
+                    utils::string::join(module_dict_iter.second, "', '"),
+                    module_dict_iter.first));
+            }
+            return ret.append(utils::sformat(_("Unable to resolve argument '{}':{}"), *spec, error_message));
+        }
     }
     return ret;
 }

libdnf5/module/module_sack.cpp

@@ -777,18 +777,96 @@ bool ModuleSack::Impl::enable(const std::string & name, const std::string & stre
 }
 
 
-bool ModuleSack::Impl::enable(const std::string & module_spec, bool count) {
+// module dict { name : { stream : [solvable id] } }
+static std::unordered_map<std::string, std::unordered_map<std::string, libdnf5::solv::IdQueue>> create_module_dict(
+    const ModuleQuery & module_query) {
+    std::unordered_map<std::string, std::unordered_map<std::string, libdnf5::solv::IdQueue>> module_dict;
+    for (const auto & module_item : module_query.list()) {
+        module_dict[module_item.get_name()][module_item.get_stream()].push_back(module_item.get_id().id);
+    }
+    return module_dict;
+}
+
+
+std::set<std::string> ModuleSack::Impl::prune_module_dict(
+    std::unordered_map<std::string, std::unordered_map<std::string, libdnf5::solv::IdQueue>> & module_dict) {
+    // Vector of module names with multiple streams to enable
+    std::set<std::string> multiple_stream_modules;
+
+    for (auto & module_dict_iter : module_dict) {
+        auto name = module_dict_iter.first;
+        auto & stream_dict = module_dict_iter.second;
+        auto module_status = module_db->get_status(name);
+
+        // Multiple streams match the requested spec
+        if (stream_dict.size() > 1) {
+            // Get stream that is either enabled (for ENABLED module), or default (otherwise)
+            std::string enabled_or_default_stream;
+            if (module_status == ModuleStatus::ENABLED) {
+                enabled_or_default_stream = module_db->get_enabled_stream(name);
+            } else {
+                enabled_or_default_stream = module_sack->get_default_stream(name);
+            }
+
+            // Module doesn't have any enabled nor default stream
+            if (enabled_or_default_stream.empty()) {
+                for (const auto & stream_pair : stream_dict) {
+                    multiple_stream_modules.insert(fmt::format("{}:{}", name, stream_pair.first));
+                }
+                continue;
+            }
+
+            // The enabled or default stream is not one of the possible streams
+            if (stream_dict.find(enabled_or_default_stream) == stream_dict.end()) {
+                for (const auto & stream_pair : stream_dict) {
+                    multiple_stream_modules.insert(fmt::format("{}:{}", name, stream_pair.first));
+                }
+                continue;
+            }
+
+            // Remove all streams except for the enabled_or_default_stream
+            for (auto iter = stream_dict.begin(); iter != stream_dict.end();) {
+                if (iter->first != enabled_or_default_stream) {
+                    stream_dict.erase(iter++);
+                } else {
+                    ++iter;
+                }
+            }
+        }
+    }
+    return multiple_stream_modules;
+}
+
+
+std::pair<bool, std::set<std::string>> ModuleSack::Impl::enable(const std::string & module_spec, bool count) {
     module_db->initialize();
 
+    // For the given module_spec, create a dict { name : {stream : [solvable id] }
+    auto module_dict = create_module_dict(module_spec_to_query(base, module_spec));
+    // Keep only enabled or default streams if possible
+    auto multiple_stream_modules = prune_module_dict(module_dict);
+
+    // If there are any modules with multiple streams to be enabled, return immediately the set of these
+    // module:stream strings.
+    if (!multiple_stream_modules.empty()) {
+        return std::make_pair(false, multiple_stream_modules);
+    }
+
     bool changed = false;
     libdnf5::solv::IdQueue queue;
-    for (const auto & module_item : module_spec_to_query(base, module_spec)) {
-        queue.push_back(module_item.get_id().id);
-        changed |= enable(module_item.get_name(), module_item.get_stream(), count);
+    for (const auto & module_dict_iter : module_dict) {
+        std::string name = module_dict_iter.first;
+        for (const auto & stream_dict_iter : module_dict_iter.second) {
+            // Enable this stream
+            changed |= enable(name, stream_dict_iter.first, count);
+            // Create a queue of ids for the stream to be enabled, because it better matches user requirements
+            // than just "module(name:stream)" provides. (E.g. user might have requested specific context or version.)
+            queue += stream_dict_iter.second;
+        }
     }
     modules_to_enable.push_back(queue);
 
-    return changed;
+    return std::make_pair(changed, multiple_stream_modules);
 }
 
 

libdnf5/module/module_sack_impl.hpp

@@ -129,11 +129,11 @@ class ModuleSack::Impl {
     /// Enable module stream.
     /// @param module_spec module to be enabled.
     /// @param count if `true`, count the change towards the limit of module status modifications.
-    /// @return `true` if requested change really triggers a change in the ModuleDB, `false` otherwise.
-    /// @throw EnableMultipleStreamsError in case of conflicting enable requests.
+    /// @return `true` if requested change realy triggers a change in the ModuleDB, `false` otherwise,
+    ///         and a set of module:stream strings for modules with multiple streams and no enabled or default one.
     /// @throw NoModuleError if the module doesn't exist.
     /// @since 5.0.14
-    bool enable(const std::string & module_spec, bool count = true);
+    std::pair<bool, std::set<std::string>> enable(const std::string & module_spec, bool count = true);
     /// Disable module.
     /// @param module_spec module to be disabled.
     /// @param count if `true`, count the change towards the limit of module status modifications.
@@ -188,6 +188,12 @@ class ModuleSack::Impl {
     /// @return If platform id was detected, it returns a pair where the first item is the platform
     ///         module name and second is the platform stream. Otherwise std::nullopt is returned.
     std::optional<std::pair<std::string, std::string>> detect_platform_name_and_stream() const;
+
+    /// @brief Keep only one stream for each module. If more than one stream is originally there, keep only
+    // the enabled or default one if possible.
+    /// @return Set of module:stream strings for modules with multiple streams and no enabled or default one.
+    std::set<std::string> prune_module_dict(
+        std::unordered_map<std::string, std::unordered_map<std::string, libdnf5::solv::IdQueue>> & module_dict);
 };
 
 inline const std::vector<std::unique_ptr<ModuleItem>> & ModuleSack::Impl::get_modules() {

test/data/repos-repomd/repomd-modules/repodata/modules.yaml

@@ -259,6 +259,25 @@ data:
     - requires:
         gooseberry: [5.4]
 ...
+---
+  document: modulemd
+  version: 2
+  data:
+    name: NoStaticContext
+    stream: latest
+    version: 1
+    arch: x86_64
+    summary: Test module
+    description: Test module
+    license:
+      module: [MIT]
+    profiles:
+      minimal:
+        rpms: []
+    dependencies:
+    - requires:
+        gooseberry: [5.5]
+...
 ---
 document: modulemd-defaults
 version: 1
@@ -268,3 +287,12 @@ data:
     profiles:
       main: [minimal]
 ...
+---
+document: modulemd-defaults
+version: 1
+data:
+    module: NoStaticContext
+    stream: latest
+    profiles:
+      main: [minimal]
+...

test/data/repos-repomd/repomd-modules/repodata/repomd.xml

@@ -9,8 +9,8 @@
     <open-size>864433</open-size>
   </data>
   <data type="modules">
-    <checksum type="sha256">3afb8a29deb024cf0bfecf0623d1a0feb11d0ca77e4564f5137cbd3fd91a718a</checksum>
-    <open-checksum type="sha256">3afb8a29deb024cf0bfecf0623d1a0feb11d0ca77e4564f5137cbd3fd91a718a</open-checksum>
+    <checksum type="sha256">1f1a76629e8eeb80db6d5de9d43a36529882e2864c97283288f24d03ec31748b</checksum>
+    <open-checksum type="sha256">1f1a76629e8eeb80db6d5de9d43a36529882e2864c97283288f24d03ec31748b</open-checksum>
     <location href="repodata/modules.yaml" />
     <timestamp>1641802880</timestamp>
     <size>492</size>