From 774f36026f1d3a4215be67845c2873135ceab6e4 Mon Sep 17 00:00:00 2001

From: Stephen Gallagher <sgallagh@redhat.com>

Date: Wed, 16 Jan 2019 13:21:44 -0500

Subject: [PATCH] Don't fail merges when default streams differ

Instead of failing the merge on a default stream conflict, we will

instead treat it as having no default set. This is safe because if

the module is not yet installed, then this just means its packages

aren't visible and it needs to be selected explicitly.

If some packages from it are already installed, then the module

stream is already set, and thus changing the default won't matter.

Update documentation to reflect modified field in Defaults

Resolves: rhbz#1666871

Signed-off-by: Stephen Gallagher <sgallagh@redhat.com>

---

 .../modulemd-1.0/private/modulemd-private.h   |  2 +

 modulemd/v1/modulemd-defaults.c               | 39 +++++++----

 modulemd/v1/tests/test-modulemd-defaults.c    | 66 ++++++++++++++++++-

 .../modulemd-module-index-merger.h            | 18 +++--

 .../private/modulemd-defaults-v1-private.h    |  2 +

 modulemd/v2/modulemd-defaults-v1.c            | 59 +++++++++++------

 modulemd/v2/tests/ModulemdTests/merger.py     | 33 +++++++---

 .../v2/tests/test_data/overriding-nodejs.yaml | 11 ++++

 test_data/defaults/overriding-nodejs.yaml     | 10 +++

 9 files changed, 191 insertions(+), 49 deletions(-)

 create mode 100644 modulemd/v2/tests/test_data/overriding-nodejs.yaml

 create mode 100644 test_data/defaults/overriding-nodejs.yaml

diff --git a/modulemd/v1/include/modulemd-1.0/private/modulemd-private.h b/modulemd/v1/include/modulemd-1.0/private/modulemd-private.h

index 20604efab843de6ac64cd9e181ce93d7e3a798fe..e9a785aed0d38598ce08d58e7e507d01698788a0 100644

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

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

@@ -24,10 +24,12 @@ enum

   MD_VERSION_MAX = G_MAXUINT64

 };

 #define MD_VERSION_LATEST MD_VERSION_2

+#define DEFAULT_MERGE_CONFLICT "__merge_conflict__"

+

 ModulemdModule *

 modulemd_module_new_from_modulestream (ModulemdModuleStream *stream);

 ModulemdModuleStream *

 modulemd_module_peek_modulestream (ModulemdModule *self);

diff --git a/modulemd/v1/modulemd-defaults.c b/modulemd/v1/modulemd-defaults.c

index 77ce30733945e30ffb8368986fa8c54c45697b5a..d44076eef0ac3bffae05f74b852fb6b51c2aee64 100644

--- a/modulemd/v1/modulemd-defaults.c

+++ b/modulemd/v1/modulemd-defaults.c

@@ -12,10 +12,11 @@

  */

 #include "modulemd.h"

 #include "modulemd-defaults.h"

 #include "modulemd-simpleset.h"

+#include "private/modulemd-private.h"

 #include "private/modulemd-yaml.h"

 GQuark

 modulemd_defaults_error_quark (void)

@@ -146,19 +147,38 @@ modulemd_defaults_set_default_stream (ModulemdDefaults *self,

 const gchar *

 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;

 }

 gchar *

 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);

 }

 void

@@ -753,28 +773,25 @@ modulemd_defaults_merge (ModulemdDefaults *first,

   /* They had the same 'modified' value (such as both zero, for

    * backwards-compatibility with 1.7.x and older.

    * 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);

   g_hash_table_iter_init (&iter,

                           modulemd_defaults_peek_profile_defaults (second));

diff --git a/modulemd/v1/tests/test-modulemd-defaults.c b/modulemd/v1/tests/test-modulemd-defaults.c

index 5d032d83cbca4046af13719f68bdbc6778d6879e..35142b885b3ec30f5109d53befe17be3e08d7b5a 100644

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

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

@@ -602,10 +602,11 @@ modulemd_defaults_test_prioritizer (DefaultsFixture *fixture,

 {

   g_autofree gchar *yaml_base_path = NULL;

   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;

   GHashTable *htable = NULL;

   gint64 prio;

@@ -618,10 +619,22 @@ modulemd_defaults_test_prioritizer (DefaultsFixture *fixture,

   base_objects = modulemd_objects_from_file (yaml_base_path, &error);

   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);

   override_objects = modulemd_objects_from_file (yaml_override_path, &error);

@@ -659,10 +672,52 @@ modulemd_defaults_test_prioritizer (DefaultsFixture *fixture,

       fprintf (stderr, "Merge error: %s", error->message);

     }

   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.

    *

    * These objects have several conflicts with the base objects that cannot be

    * merged.

@@ -671,11 +726,11 @@ modulemd_defaults_test_prioritizer (DefaultsFixture *fixture,

     modulemd_prioritizer_add (prioritizer, override_objects, prio, &error);

   g_assert_false (result);

   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 */

   g_clear_pointer (&prioritizer, g_object_unref);

@@ -987,11 +1042,11 @@ modulemd_defaults_test_index_prioritizer (DefaultsFixture *fixture,

     modulemd_prioritizer_add_index (prioritizer, override_index, prio, &error);

   g_assert_false (result);

   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 */

   g_clear_pointer (&prioritizer, g_object_unref);

@@ -1138,10 +1193,15 @@ modulemd_regressions_issue44 (DefaultsFixture *fixture,

   g_assert_true (result);

   /* 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"));

   g_assert_nonnull (yaml_conflicting_path);

@@ -1149,11 +1209,11 @@ modulemd_regressions_issue44 (DefaultsFixture *fixture,

     modulemd_objects_from_file (yaml_conflicting_path, &error);

   g_assert_nonnull (conflicting_objects);

   result =

     modulemd_prioritizer_add (prioritizer, conflicting_objects, 0, &error);

-  g_assert_false (result);

+  g_assert_true (result);

 }

 static void

 modulemd_regressions_issue45 (DefaultsFixture *fixture,

diff --git a/modulemd/v2/include/modulemd-2.0/modulemd-module-index-merger.h b/modulemd/v2/include/modulemd-2.0/modulemd-module-index-merger.h

index 27cccb738dc805724268afa04acc13d4e250eae2..b019f0ed856684a003e9c2f6abefc70e1448246a 100644

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

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

@@ -51,22 +51,30 @@ G_BEGIN_DECLS

  * there exists two #ModulemdModuleStream entries that have different content

  * for the same NSVC, the behavior is undefined.

  *

  * Merging #ModulemdDefaults entries behaves as follows:

  *

+ * - Within a ModuleIndex, if two or more default entries reference the same

+ *   module, the one with the highest modified field will be used and the

+ *   others discarded.

+ * - When merging ModuleIndexes, if two or more indexes contain Defaults for

+ *   the same module, but different modified values, the one with the highest

+ *   modified value will be used and the others discarded.

  * - Any module default that is provided by a single repository is

  *   authoritative.

  * - If the repos have different priorities (not common), then the default for

  *   this module and stream name coming from the repo of higher priority will

- *   be used and the default from the lower-priority repo will not be included.

+ *   be used and the default from the lower-priority repo will not be included

+ *   even if it has a higher modified value.

  * - If the repos have the same priority (such as "fedora" and "updates" in the

- *   Fedora Project), the entries will be merged as follows:

- *   - If both repositories specify a default stream for the module, use it.

+ *   Fedora Project) and modified value, the entries will be merged as follows:

+ *   - If both repositories specify the same default stream for the module, use

+ *     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

  *     the other does not, use the one specified.

  *   - If both repositories specify a set of default profiles for a stream and

diff --git a/modulemd/v2/include/modulemd-2.0/private/modulemd-defaults-v1-private.h b/modulemd/v2/include/modulemd-2.0/private/modulemd-defaults-v1-private.h

index 9cac6cc53175a0eecca986f4fa96a7b7b1070957..de2ede98e6fb49bfd792b5f2913868fad6ff27db 100644

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

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

@@ -26,10 +26,12 @@ G_BEGIN_DECLS

  * @stability: private

  * @short_description: #ModulemdDefault methods that should only be used by

  * internal consumers.

  */

+#define DEFAULT_MERGE_CONFLICT "__merge_conflict__"

+

 /**

  * modulemd_defaults_v1_parse_yaml:

  * @subdoc: (in): A #ModulemdSubdocumentInfo representing a defaults document

  * of metadata version 1.

  * @strict: (in): Whether the parser should return failure if it encounters an

diff --git a/modulemd/v2/modulemd-defaults-v1.c b/modulemd/v2/modulemd-defaults-v1.c

index c8f983105b529519365d0a653d8d07715f610b97..1c1cdd7b1036734a477b72d12f12526af1de777d 100644

--- a/modulemd/v2/modulemd-defaults-v1.c

+++ b/modulemd/v2/modulemd-defaults-v1.c

@@ -179,11 +179,22 @@ modulemd_defaults_v1_get_default_stream (ModulemdDefaultsV1 *self,

 {

   const gchar *default_stream = NULL;

   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)

     {

       if (default_stream[0] == '\0')

@@ -1159,12 +1170,10 @@ ModulemdDefaults *

 modulemd_defaults_v1_merge (const gchar *module_name,

                             ModulemdDefaultsV1 *from,

                             ModulemdDefaultsV1 *into,

                             GError **error)

 {

-  const gchar *into_default_stream;

-  const gchar *from_default_stream;

   g_autoptr (ModulemdDefaultsV1) merged = NULL;

   GHashTableIter iter;

   gpointer key, value;

   GHashTable *intent_profiles = NULL;

   GHashTable *merged_intent_profiles = NULL;

@@ -1175,37 +1184,45 @@ modulemd_defaults_v1_merge (const gchar *module_name,

   g_autoptr (GError) nested_error = NULL;

   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

     {

       /* Both values were NULL.

        * Nothing to do, leave it blank.

diff --git a/modulemd/v2/tests/ModulemdTests/merger.py b/modulemd/v2/tests/ModulemdTests/merger.py

index e7cbf4214bb111fbc5732fe983ee5ccee134fd83..49c2722959486aeb250d88daf7f068cacc8c3fe7 100644

--- a/modulemd/v2/tests/ModulemdTests/merger.py

+++ b/modulemd/v2/tests/ModulemdTests/merger.py

@@ -104,26 +104,41 @@ class TestModuleIndexMerger(TestBase):

         self.assertEqual(

             len(httpd_defaults.get_default_profiles_for_stream('2.4', 'workstation')), 1)

         self.assertEqual(

             len(httpd_defaults.get_default_profiles_for_stream('2.6', 'workstation')), 3)

+        # 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-nodejs.yaml"), True)

+

+        # 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_nodejs_index, 0)

+

+        merged_index = merger.resolve()

+        self.assertIsNotNone(merged_index)

+

+        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 adding both of these at the same priority level fails

-        # with a merge conflict.

-        merger = Modulemd.ModuleIndexMerger()

-        merger.associate_index(base_index, 0)

-        merger.associate_index(override_index, 0)

-

-        with self.assertRaisesRegex(GLib.GError, 'Module stream mismatch in merge'):

-            merged_index = merger.resolve()

-

         # Test that override_index at a higher priority level succeeds

         # Test that adding both of these at the same priority level fails

         # with a merge conflict.

         # Use randomly-selected high and low values to make sure we don't have

         # sorting issues.

diff --git a/modulemd/v2/tests/test_data/overriding-nodejs.yaml b/modulemd/v2/tests/test_data/overriding-nodejs.yaml

new file mode 100644

index 0000000000000000000000000000000000000000..b588f5f542d84755fa74fc1dbf059c0766d2a712

--- /dev/null

+++ b/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]

\ No newline at end of file

diff --git a/test_data/defaults/overriding-nodejs.yaml b/test_data/defaults/overriding-nodejs.yaml

new file mode 100644

index 0000000000000000000000000000000000000000..937a2b899afc6b5254fd3b3e61e5b32035a3130a

--- /dev/null

+++ b/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]

\ No newline at end of file

-- 

2.20.1