Deduplicate module streams when merging

sgallagher · sgallagher · commit 210d185a33df · 2018-09-18T12:15:43.000-04:00
Signed-off-by: Stephen Gallagher &lt;sgallagh@redhat.com&gt;
diff --git a/modulemd/v1/modulemd-prioritizer.c b/modulemd/v1/modulemd-prioritizer.c
@@ -60,6 +60,11 @@ modulemd_prioritizer_class_init (ModulemdPrioritizerClass *klass)
   object_class->set_property = NULL;
 }
 
+static GPtrArray *
+_deduplicate_module_streams (const GPtrArray *first,
+                             const GPtrArray *second,
+                             GError **error);
+
 static void
 _modulemd_ptr_array_unref (gpointer ptr)
 {
@@ -82,7 +87,10 @@ modulemd_prioritizer_add (ModulemdPrioritizer *self,
 {
   GPtrArray *current_objects = NULL;
   g_autoptr (GPtrArray) concat_objects = NULL;
+  g_autoptr (GPtrArray) deduped_objects = NULL;
   g_autoptr (GPtrArray) merged_objects = NULL;
+  g_autoptr (ModulemdSimpleSet) nsvcs = modulemd_simpleset_new ();
+  g_autofree gchar *nsvc = NULL;
   gint64 *prio = NULL;
   gsize i;
 
@@ -142,14 +150,17 @@ modulemd_prioritizer_add (ModulemdPrioritizer *self,
         }
     }
 
-  for (i = 0; i < objects->len; i++)
+  deduped_objects =
+    _deduplicate_module_streams (concat_objects, objects, error);
+  if (!deduped_objects)
     {
-      g_ptr_array_add (concat_objects,
-                       g_object_ref (g_ptr_array_index (objects, i)));
+      /* Something went wrong. Return the error here. */
+      g_clear_pointer (&prio, g_free);
+      return FALSE;
     }
 
   merged_objects =
-    modulemd_merge_defaults (concat_objects, NULL, FALSE, error);
+    modulemd_merge_defaults (deduped_objects, NULL, FALSE, error);
   if (!merged_objects)
     {
       /* Something went wrong. Return the error here. */
@@ -188,18 +199,18 @@ GPtrArray *
 modulemd_prioritizer_resolve (ModulemdPrioritizer *self, GError **error)
 {
   g_autoptr (GPtrArray) current = NULL;
-  g_autoptr (GPtrArray) next = NULL;
+  g_autoptr (GPtrArray) prev = NULL;
   g_autoptr (GPtrArray) tmp = NULL;
+  g_autoptr (GPtrArray) deduped_objects = NULL;
   g_autoptr (GList) priority_levels = NULL;
   GList *current_level = NULL;
 
   g_return_val_if_fail (MODULEMD_IS_PRIORITIZER (self), FALSE);
   g_return_val_if_fail (error == NULL || *error == NULL, FALSE);
 
-  current_level = priority_levels =
-    _modulemd_ordered_int64_keys (self->priorities);
+  priority_levels = _modulemd_ordered_int64_keys (self->priorities);
 
-  if (!current_level)
+  if (!priority_levels)
     {
       /* Nothing has been added to the resolver. */
       g_set_error (error,
@@ -210,32 +221,144 @@ modulemd_prioritizer_resolve (ModulemdPrioritizer *self, GError **error)
       return NULL;
     }
 
+  /* Go through the merge from highest priority down to lowest. */
+  current_level = g_list_last (priority_levels);
+
   current = g_ptr_array_ref (
-    g_hash_table_lookup (self->priorities, priority_levels->data));
+    g_hash_table_lookup (self->priorities, current_level->data));
 
-  while (current_level->next)
+  while (current_level->prev)
     {
-      next = g_ptr_array_ref (
-        g_hash_table_lookup (self->priorities, current_level->next->data));
+      prev = g_ptr_array_ref (
+        g_hash_table_lookup (self->priorities, current_level->prev->data));
 
       /* Merge the values, replacing any conflicts */
-      tmp = modulemd_merge_defaults (current, next, TRUE, error);
+      tmp = modulemd_merge_defaults (prev, current, TRUE, error);
       if (!tmp)
         {
           /* Something went wrong with the merge. Return the error */
           return NULL;
         }
+
+      /* Deduplicate after the merge */
+      deduped_objects = _deduplicate_module_streams (tmp, NULL, error);
+      if (!deduped_objects)
+        {
+          /* Something went wrong. Return the error here. */
+          return NULL;
+        }
+
       g_clear_pointer (&current, g_ptr_array_unref);
-      current = g_ptr_array_ref (tmp);
+      current = g_ptr_array_ref (deduped_objects);
       g_clear_pointer (&tmp, g_ptr_array_unref);
-      g_clear_pointer (&next, g_ptr_array_unref);
+      g_clear_pointer (&deduped_objects, g_ptr_array_unref);
+      g_clear_pointer (&prev, g_ptr_array_unref);
 
-      current_level = current_level->next;
+      current_level = current_level->prev;
     }
 
   return g_ptr_array_ref (current);
 }
 
+static GPtrArray *
+_deduplicate_module_streams (const GPtrArray *first,
+                             const GPtrArray *second,
+                             GError **error)
+{
+  GObject *object = NULL;
+  g_autoptr (GPtrArray) deduplicated = NULL;
+  g_autoptr (ModulemdSimpleSet) nsvcs = modulemd_simpleset_new ();
+  g_autofree gchar *nsvc = NULL;
+  gssize reserved_size, i;
+
+  g_return_val_if_fail (first, NULL);
+  g_return_val_if_fail (error == NULL || *error == NULL, NULL);
+
+  /* Assume the common case that there are no duplicates and preallocate
+   * space to hold the entire set.
+   */
+  reserved_size = first->len;
+  if (second)
+    {
+      reserved_size += second->len;
+    }
+
+  deduplicated = g_ptr_array_new_full (reserved_size, g_object_unref);
+
+  /* We check the second list here as a preventative measure. In a proper
+   * implementation of this, we'd do a full check of the module stream entries
+   * to ensure that they don't have the same NSVC but different content. For
+   * now, however, we'll just assume that the second list has the right data
+   * since it's likely to be newer.
+   */
+  if (second)
+    for (i = 0; i < second->len; i++)
+      {
+        object = g_ptr_array_index (second, i);
+
+        if (MODULEMD_IS_MODULE (object))
+          {
+            nsvc = modulemd_module_dup_nsvc (MODULEMD_MODULE (object));
+          }
+        else if (MODULEMD_IS_MODULESTREAM (object))
+          {
+            nsvc =
+              modulemd_modulestream_get_nsvc (MODULEMD_MODULESTREAM (object));
+          }
+        if (nsvc)
+          {
+            if (modulemd_simpleset_contains (nsvcs, nsvc))
+              {
+                /* We've seen this NSVC before; skip it */
+                continue;
+              }
+
+            /* Save this NSVC so we don't add it twice */
+            modulemd_simpleset_add (nsvcs, nsvc);
+            g_clear_pointer (&nsvc, g_free);
+          }
+
+        g_ptr_array_add (deduplicated, g_object_ref (object));
+      }
+
+  /* For the 'first' list, go through in reverse order, because this may be
+   * called after the modulemd_merge_defaults() routine has already
+   * concatenated the higher-priority list. This will ensure that the
+   * other list wins any merge disputes.
+   */
+  for (i = (gsize)first->len - 1; i >= 0; i--)
+    {
+      object = g_ptr_array_index (first, i);
+
+      if (MODULEMD_IS_MODULE (object))
+        {
+          nsvc = modulemd_module_dup_nsvc (MODULEMD_MODULE (object));
+        }
+      else if (MODULEMD_IS_MODULESTREAM (object))
+        {
+          nsvc =
+            modulemd_modulestream_get_nsvc (MODULEMD_MODULESTREAM (object));
+        }
+
+      if (nsvc)
+        {
+          if (modulemd_simpleset_contains (nsvcs, nsvc))
+            {
+              /* We've seen this NSVC before; skip it */
+              continue;
+            }
+
+          /* Save this NSVC so we don't add it twice */
+          modulemd_simpleset_add (nsvcs, nsvc);
+          g_clear_pointer (&nsvc, g_free);
+        }
+
+      g_ptr_array_add (deduplicated, g_object_ref (object));
+    }
+
+  return g_ptr_array_ref (deduplicated);
+}
+
 
 GHashTable *
 modulemd_prioritizer_resolve_index (ModulemdPrioritizer *self, GError **error)
diff --git a/modulemd/v1/tests/test-modulemd-defaults.c b/modulemd/v1/tests/test-modulemd-defaults.c
@@ -718,7 +718,7 @@ modulemd_defaults_test_prioritizer (DefaultsFixture *fixture,
 
 
   /* HTTPD */
-  defaults = MODULEMD_DEFAULTS (g_ptr_array_index (merged_objects, 0));
+  defaults = MODULEMD_DEFAULTS (g_ptr_array_index (merged_objects, 2));
   g_assert_cmpstr (modulemd_defaults_peek_module_name (defaults), ==, "httpd");
   g_assert_cmpstr (
     modulemd_defaults_peek_default_stream (defaults), ==, "2.4");
@@ -760,7 +760,7 @@ modulemd_defaults_test_prioritizer (DefaultsFixture *fixture,
     g_hash_table_lookup (htable, "9.0"), "supermegaultra"));
 
   /* POSTGRESQL */
-  defaults = MODULEMD_DEFAULTS (g_ptr_array_index (merged_objects, 2));
+  defaults = MODULEMD_DEFAULTS (g_ptr_array_index (merged_objects, 0));
   g_assert_cmpstr (
     modulemd_defaults_peek_module_name (defaults), ==, "postgresql");
   g_assert_cmpstr (
@@ -900,7 +900,7 @@ modulemd_defaults_test_index_prioritizer (DefaultsFixture *fixture,
 
 
   /* HTTPD */
-  defaults = MODULEMD_DEFAULTS (g_ptr_array_index (merged_objects, 0));
+  defaults = MODULEMD_DEFAULTS (g_ptr_array_index (merged_objects, 2));
   g_assert_cmpstr (modulemd_defaults_peek_module_name (defaults), ==, "httpd");
   g_assert_cmpstr (
     modulemd_defaults_peek_default_stream (defaults), ==, "2.4");
@@ -942,7 +942,7 @@ modulemd_defaults_test_index_prioritizer (DefaultsFixture *fixture,
     g_hash_table_lookup (htable, "9.0"), "supermegaultra"));
 
   /* POSTGRESQL */
-  defaults = MODULEMD_DEFAULTS (g_ptr_array_index (merged_objects, 2));
+  defaults = MODULEMD_DEFAULTS (g_ptr_array_index (merged_objects, 0));
   g_assert_cmpstr (
     modulemd_defaults_peek_module_name (defaults), ==, "postgresql");
   g_assert_cmpstr (
diff --git a/modulemd/v1/tests/test-modulemd-python.py b/modulemd/v1/tests/test-modulemd-python.py
@@ -175,7 +175,6 @@ def test_issue25(self):
         assert mmd.peek_rpm_buildopts() != {}
         assert mmd.peek_rpm_buildopts()['macros'] == '%my_macro 1'
         dumped = mmd.dumps()
-        print("YAML:\n%s" % dumped, file=sys.stderr)
         mmd2 = Modulemd.Module.new_from_string(dumped)
         assert mmd2.peek_rpm_buildopts() != {}
         assert mmd2.peek_rpm_buildopts()['macros'] == '%my_macro 1'
@@ -186,7 +185,7 @@ def test_issue33(self):
         # double-freeing memory.
         defs = Modulemd.Module.new_all_from_file_ext(
             "%s/mod-defaults/spec.v1.yaml" % os.getenv('MESON_SOURCE_ROOT'))
-        print(defs)
+        assert defs
 
     def test_issue77(self):
         # This would crash on a type constraint accepting a signed value
@@ -266,6 +265,36 @@ def test_issue85(self):
         assert mmd3.get_module_components(
         )['testmodule'].peek_ref() == 'private-x'
 
+    def test_issue88(self):
+        # Here, load the same file twice.
+        # All entries in these two lists should be duplicates of each other.
+        objects_from_repo_a = Modulemd.objects_from_file(
+            '%s/test_data/issue88.yaml' % os.getenv('MESON_SOURCE_ROOT'))
+        objects_from_repo_b = Modulemd.objects_from_file(
+            '%s/test_data/issue88.yaml' % os.getenv('MESON_SOURCE_ROOT'))
+
+        # Test at the same priority level
+        prioritizer = Modulemd.Prioritizer()
+        prioritizer.add(objects_from_repo_a, 0)
+        prioritizer.add(objects_from_repo_b, 0)
+        supposedly_merged_objects = prioritizer.resolve()
+
+        # I would expect all three of these to be the same length... but they
+        # are not.
+        assert len(objects_from_repo_a) == len(objects_from_repo_b)
+        assert len(objects_from_repo_a) == len(supposedly_merged_objects)
+
+        # Test at different priorities
+        prioritizer = Modulemd.Prioritizer()
+        prioritizer.add(objects_from_repo_a, 0)
+        prioritizer.add(objects_from_repo_b, 1)
+        supposedly_merged_objects = prioritizer.resolve()
+
+        # I would expect all three of these to be the same length... but they
+        # are not.
+        assert len(objects_from_repo_a) == len(objects_from_repo_b)
+        assert len(objects_from_repo_a) == len(supposedly_merged_objects)
+
 
 class TestIntent(unittest.TestCase):
 
diff --git a/test_data/issue88.yaml b/test_data/issue88.yaml
