Merge branch 'defaults-stream-conflict'

Author: sgallagher

modulemd/v1/include/modulemd-1.0/private/modulemd-private.h

@@ -26,6 +26,8 @@ enum
 
 #define MD_VERSION_LATEST MD_VERSION_2
 
+#define DEFAULT_MERGE_CONFLICT "__merge_conflict__"
+
 ModulemdModule *
 modulemd_module_new_from_modulestream (ModulemdModuleStream *stream);
 

modulemd/v1/modulemd-defaults.c

@@ -14,6 +14,7 @@
 #include "modulemd.h"
 #include "modulemd-defaults.h"
 #include "modulemd-simpleset.h"
+#include "private/modulemd-private.h"
 #include "private/modulemd-yaml.h"
 
 
@@ -148,6 +149,15 @@ modulemd_defaults_peek_default_stream (ModulemdDefaults *self)
 {
   g_return_val_if_fail (self, NULL);
 
+  if (self->default_stream &&
+      g_str_equal (self->default_stream, DEFAULT_MERGE_CONFLICT))
+    {
+      /* During an index merge, we determined that this was in conflict
+       * with another set of ModulemdDefaults for the same module. If we
+       * see this, treat it as no default stream when querying for it.
+       */
+      return NULL;
+    }
   return self->default_stream;
 }
 
@@ -157,6 +167,16 @@ modulemd_defaults_dup_default_stream (ModulemdDefaults *self)
 {
   g_return_val_if_fail (self, NULL);
 
+  if (self->default_stream &&
+      g_str_equal (self->default_stream, DEFAULT_MERGE_CONFLICT))
+    {
+      /* During an index merge, we determined that this was in conflict
+       * with another set of ModulemdDefaults for the same module. If we
+       * see this, treat it as no default stream when querying for it.
+       */
+      return NULL;
+    }
+
   return g_strdup (self->default_stream);
 }
 
@@ -755,24 +775,21 @@ modulemd_defaults_merge (ModulemdDefaults *first,
    * Merge them as best we can.
    */
 
+  defaults = modulemd_defaults_copy (first);
+
   /* First check for incompatibilities with the streams */
-  if (g_strcmp0 (modulemd_defaults_peek_default_stream (first),
-                 modulemd_defaults_peek_default_stream (second)))
+  if (g_strcmp0 (first->default_stream, second->default_stream))
     {
       /* Default streams don't match and override is not set.
        * Return an error
        */
-      g_set_error (
-        error,
-        MODULEMD_DEFAULTS_ERROR,
-        MODULEMD_DEFAULTS_ERROR_CONFLICTING_STREAMS,
-        "Conflicting default streams when merging defaults for module %s",
-        modulemd_defaults_peek_module_name (first));
-      return NULL;
+      /* They have conflicting default streams */
+      g_info ("Module stream mismatch in merge: %s != %s",
+              first->default_stream,
+              second->default_stream);
+      modulemd_defaults_set_default_stream (defaults, DEFAULT_MERGE_CONFLICT);
     }
 
-  defaults = modulemd_defaults_copy (first);
-
   /* Merge the profile defaults */
   profile_defaults = modulemd_defaults_peek_profile_defaults (defaults);
 

modulemd/v1/tests/test-modulemd-defaults.c

@@ -604,6 +604,7 @@ modulemd_defaults_test_prioritizer (DefaultsFixture *fixture,
   g_autofree gchar *yaml_override_path = NULL;
   g_autoptr (GPtrArray) base_objects = NULL;
   g_autoptr (GPtrArray) override_objects = NULL;
+  g_autoptr (GPtrArray) override_nodejs_objects = NULL;
   g_autoptr (GPtrArray) merged_objects = NULL;
   g_autoptr (ModulemdPrioritizer) prioritizer = NULL;
   g_autoptr (GError) error = NULL;
@@ -620,6 +621,18 @@ modulemd_defaults_test_prioritizer (DefaultsFixture *fixture,
   g_assert_nonnull (base_objects);
   g_assert_cmpint (base_objects->len, ==, 7);
 
+
+  yaml_override_path =
+    g_strdup_printf ("%s/test_data/defaults/overriding-nodejs.yaml",
+                     g_getenv ("MESON_SOURCE_ROOT"));
+  g_assert_nonnull (yaml_override_path);
+
+  override_nodejs_objects =
+    modulemd_objects_from_file (yaml_override_path, &error);
+  g_clear_pointer (&yaml_override_path, g_free);
+  g_assert_nonnull (override_nodejs_objects);
+  g_assert_cmpint (override_nodejs_objects->len, ==, 1);
+
   yaml_override_path = g_strdup_printf (
     "%s/test_data/defaults/overriding.yaml", g_getenv ("MESON_SOURCE_ROOT"));
   g_assert_nonnull (yaml_override_path);
@@ -661,6 +674,48 @@ modulemd_defaults_test_prioritizer (DefaultsFixture *fixture,
   g_assert_true (result);
 
 
+  /*
+   * Test that importing the nodejs overrides at the same priority level fails.
+   *
+   * This YAML has a conflicting default stream which should be ignored and set
+   * to "no default stream".
+   */
+
+  result = modulemd_prioritizer_add (
+    prioritizer, override_nodejs_objects, prio, &error);
+  g_assert_true (result);
+
+  merged_objects = modulemd_prioritizer_resolve (prioritizer, &error);
+  g_assert_nonnull (merged_objects);
+  g_assert_cmpint (merged_objects->len, ==, 3);
+
+  for (gint i = 0; i < merged_objects->len; i++)
+    {
+      if (MODULEMD_IS_DEFAULTS (g_ptr_array_index (merged_objects, i)))
+        {
+          defaults = g_ptr_array_index (merged_objects, i);
+          if (!g_strcmp0 (modulemd_defaults_peek_module_name (defaults),
+                          "nodejs"))
+            {
+              g_assert_null (modulemd_defaults_peek_default_stream (defaults));
+            }
+        }
+    }
+
+  g_clear_pointer (&merged_objects, g_ptr_array_unref);
+
+
+  /* Start over and test profile conflicts */
+  g_clear_pointer (&prioritizer, g_object_unref);
+  prioritizer = modulemd_prioritizer_new ();
+  result = modulemd_prioritizer_add (prioritizer, base_objects, prio, &error);
+  if (!result)
+    {
+      fprintf (stderr, "Merge error: %s", error->message);
+    }
+  g_assert_true (result);
+
+
   /*
    * Test that importing the overrides at the same priority level fails.
    *
@@ -673,7 +728,7 @@ modulemd_defaults_test_prioritizer (DefaultsFixture *fixture,
   g_assert_cmpstr (
     g_quark_to_string (error->domain), ==, "modulemd-defaults-error-quark");
   g_assert_cmpint (
-    error->code, ==, MODULEMD_DEFAULTS_ERROR_CONFLICTING_STREAMS);
+    error->code, ==, MODULEMD_DEFAULTS_ERROR_CONFLICTING_PROFILES);
   g_clear_pointer (&error, g_error_free);
 
   /* The object's internal state is undefined after an error, so delete it */
@@ -989,7 +1044,7 @@ modulemd_defaults_test_index_prioritizer (DefaultsFixture *fixture,
   g_assert_cmpstr (
     g_quark_to_string (error->domain), ==, "modulemd-defaults-error-quark");
   g_assert_cmpint (
-    error->code, ==, MODULEMD_DEFAULTS_ERROR_CONFLICTING_STREAMS);
+    error->code, ==, MODULEMD_DEFAULTS_ERROR_CONFLICTING_PROFILES);
   g_clear_pointer (&error, g_error_free);
 
   /* The object's internal state is undefined after an error, so delete it */
@@ -1140,6 +1195,11 @@ modulemd_regressions_issue44 (DefaultsFixture *fixture,
 
   /* Add another almost-identical document, except with a conflicting default
    * stream set.
+   *
+   * NOTE: when this was written (for issue 44 on GitHub), this was meant to be
+   * a hard error. As of 1.8.1 we expect this to just result in having no
+   * default stream for this module. This test has been slightly modified so
+   * that the expected result is now a pass.
    */
   yaml_conflicting_path = g_strdup_printf (
     "%s/test_data/defaults/issue44-2.yaml", g_getenv ("MESON_SOURCE_ROOT"));
@@ -1151,7 +1211,7 @@ modulemd_regressions_issue44 (DefaultsFixture *fixture,
 
   result =
     modulemd_prioritizer_add (prioritizer, conflicting_objects, 0, &error);
-  g_assert_false (result);
+  g_assert_true (result);
 }
 
 

modulemd/v2/include/modulemd-2.0/modulemd-module-index-merger.h

@@ -71,8 +71,8 @@ G_BEGIN_DECLS
  *     it.
  *   - If either repository specifies a default stream for the module and the
  *     other does not, use the one specified.
- *   - If both repositories specify different streams, this is an unresolvable
- *     merge conflict and the merge resolution will fail and report an error.
+ *   - If both repositories specify different default streams, the merge will
+ *     unset the default stream and proceed with the merge.
  *   - If both repositories specify a set of default profiles for a stream and
  *     the sets are equivalent, use that set.
  *   - If one repository specifies a set of default profiles for a stream and

modulemd/v2/include/modulemd-2.0/private/modulemd-defaults-v1-private.h

@@ -28,6 +28,8 @@ G_BEGIN_DECLS
  * internal consumers.
  */
 
+#define DEFAULT_MERGE_CONFLICT "__merge_conflict__"
+
 /**
  * modulemd_defaults_v1_parse_yaml:
  * @subdoc: (in): A #ModulemdSubdocumentInfo representing a defaults document

modulemd/v2/modulemd-defaults-v1.c

@@ -181,7 +181,18 @@ modulemd_defaults_v1_get_default_stream (ModulemdDefaultsV1 *self,
   g_return_val_if_fail (MODULEMD_IS_DEFAULTS_V1 (self), NULL);
 
   if (!intent)
-    return self->default_stream;
+    {
+      if (self->default_stream &&
+          g_str_equal (self->default_stream, DEFAULT_MERGE_CONFLICT))
+        {
+          /* During an index merge, we determined that this was in conflict
+           * with another set of ModulemdDefaults for the same module. If we
+           * see this, treat it as no default stream when querying for it.
+           */
+          return NULL;
+        }
+      return self->default_stream;
+    }
 
   default_stream = g_hash_table_lookup (self->intent_default_streams, intent);
   if (default_stream)
@@ -1144,8 +1155,6 @@ modulemd_defaults_v1_merge (const gchar *module_name,
                             ModulemdDefaultsV1 *into,
                             GError **error)
 {
-  const gchar *into_default_stream;
-  const gchar *from_default_stream;
   g_autoptr (ModulemdDefaultsV1) merged = NULL;
   GHashTableIter iter;
   gpointer key, value;
@@ -1160,33 +1169,41 @@ modulemd_defaults_v1_merge (const gchar *module_name,
   merged = modulemd_defaults_v1_new (module_name);
 
   /* Merge the default streams */
-  into_default_stream = modulemd_defaults_v1_get_default_stream (into, NULL);
-  from_default_stream = modulemd_defaults_v1_get_default_stream (from, NULL);
-
-  if (into_default_stream && !from_default_stream)
+  if (into->default_stream && !from->default_stream)
     {
       modulemd_defaults_v1_set_default_stream (
-        merged, into_default_stream, NULL);
+        merged, into->default_stream, NULL);
     }
-  else if (from_default_stream && !into_default_stream)
+  else if (from->default_stream && !into->default_stream)
     {
       modulemd_defaults_v1_set_default_stream (
-        merged, from_default_stream, NULL);
+        merged, from->default_stream, NULL);
     }
-  else if (into_default_stream && from_default_stream)
+  else if (into->default_stream && from->default_stream)
     {
-      if (!g_str_equal (into_default_stream, from_default_stream))
+      if (g_str_equal (into->default_stream, DEFAULT_MERGE_CONFLICT))
         {
-          g_set_error (error,
-                       MODULEMD_ERROR,
-                       MODULEMD_ERROR_VALIDATE,
-                       "Module stream mismatch in merge: %s != %s",
-                       into_default_stream,
-                       from_default_stream);
-          return NULL;
+          /* A previous pass over this same module encountered a merge
+           * conflict, so we need to propagate that.
+           */
+          modulemd_defaults_v1_set_default_stream (
+            merged, DEFAULT_MERGE_CONFLICT, NULL);
+        }
+      else if (!g_str_equal (into->default_stream, from->default_stream))
+        {
+          /* They have conflicting default streams */
+          g_info ("Module stream mismatch in merge: %s != %s",
+                  into->default_stream,
+                  from->default_stream);
+          modulemd_defaults_v1_set_default_stream (
+            merged, DEFAULT_MERGE_CONFLICT, NULL);
+        }
+      else
+        {
+          /* They're the same, so store that */
+          modulemd_defaults_v1_set_default_stream (
+            merged, into->default_stream, NULL);
         }
-      modulemd_defaults_v1_set_default_stream (
-        merged, into_default_stream, NULL);
     }
   else
     {

modulemd/v2/tests/ModulemdTests/merger.py

@@ -106,21 +106,36 @@ def test_merger(self):
         self.assertEqual(
             len(httpd_defaults.get_default_profiles_for_stream('2.6', 'workstation')), 3)
 
-        # Get another set of objects that will override the above
-        override_index = Modulemd.ModuleIndex()
-        override_index.update_from_file(
+        # Get another set of objects that will override the default stream for
+        # nodejs
+        override_nodejs_index = Modulemd.ModuleIndex()
+        override_nodejs_index.update_from_file(
             path.join(
                 self.source_root,
-                "modulemd/v2/tests/test_data/overriding.yaml"), True)
+                "modulemd/v2/tests/test_data/overriding-nodejs.yaml"), True)
 
-        # Test that adding both of these at the same priority level fails
-        # with a merge conflict.
+        # Test that adding both of these at the same priority level results in
+        # the no default stream
         merger = Modulemd.ModuleIndexMerger()
         merger.associate_index(base_index, 0)
-        merger.associate_index(override_index, 0)
+        merger.associate_index(override_nodejs_index, 0)
+
+        merged_index = merger.resolve()
+        self.assertIsNotNone(merged_index)
 
-        with self.assertRaisesRegex(GLib.GError, 'Module stream mismatch in merge'):
-            merged_index = merger.resolve()
+        nodejs = merged_index.get_module('nodejs')
+        self.assertIsNotNone(nodejs)
+
+        nodejs_defaults = nodejs.get_defaults()
+        self.assertIsNotNone(nodejs_defaults)
+        self.assertIsNone(nodejs_defaults.get_default_stream())
+
+        # Get another set of objects that will override the above
+        override_index = Modulemd.ModuleIndex()
+        override_index.update_from_file(
+            path.join(
+                self.source_root,
+                "modulemd/v2/tests/test_data/overriding.yaml"), True)
 
         # Test that override_index at a higher priority level succeeds
         # Test that adding both of these at the same priority level fails

modulemd/v2/tests/test_data/overriding-nodejs.yaml

@@ -0,0 +1,11 @@
+---
+# Override the default stream
+document: modulemd-defaults
+version: 1
+data:
+    module: nodejs
+    stream: '9.0'
+    profiles:
+        '6.0': [default]
+        '8.0': [super]
+        '9.0': [supermegaultra]

test_data/defaults/overriding-nodejs.yaml

@@ -0,0 +1,10 @@
+---
+document: modulemd-defaults
+version: 1
+data:
+    module: nodejs
+    stream: '9.0'
+    profiles:
+        '6.0': [default]
+        '8.0': [super]
+        '9.0': [supermegaultra]