From c0bee1d95d628f12fa0e1e61eaad28b24d4416bc Mon Sep 17 00:00:00 2001 From: Stephen Gallagher Date: Wed, 18 Sep 2019 10:26:25 -0400 Subject: [PATCH] Improvements to ModuleIndex.update_from_defaults_dir() - Import each file in the directory as a merge rather than an an overwrite so we can detect conflicts. - Modify the meaning of the 'strict' argument to mean that it should fail if the merge would result in a conflict in the default stream setting for a module. - Reduce the opportunities for a failure to corrupt the index. Fixes: https://github.com/fedora-modularity/libmodulemd/issues/366 Signed-off-by: Stephen Gallagher --- .../modulemd-2.0/modulemd-module-index.h | 2 +- .../private/modulemd-rpm-map-entry-private.h | 4 +- modulemd/modulemd-module-index.c | 122 ++++++++++++------ modulemd/tests/test-modulemd-moduleindex.c | 69 +++++++++- .../tests/test_data/bad_defaults/meson-2.yaml | 7 + .../tests/test_data/bad_defaults/meson.yaml | 7 + .../tests/test_data/bad_defaults/ninja.yaml | 7 + .../tests/test_data/bad_defaults/nodejs.yaml | 9 ++ 8 files changed, 183 insertions(+), 44 deletions(-) create mode 100644 modulemd/tests/test_data/bad_defaults/meson-2.yaml create mode 100644 modulemd/tests/test_data/bad_defaults/meson.yaml create mode 100644 modulemd/tests/test_data/bad_defaults/ninja.yaml create mode 100644 modulemd/tests/test_data/bad_defaults/nodejs.yaml diff --git a/modulemd/include/modulemd-2.0/modulemd-module-index.h b/modulemd/include/modulemd-2.0/modulemd-module-index.h index 483a82f1e0292232181a39a78089b6d715c57805..d899f8223aecdac13b3fab0ad849350bffa09c65 100644 --- a/modulemd/include/modulemd-2.0/modulemd-module-index.h +++ b/modulemd/include/modulemd-2.0/modulemd-module-index.h @@ -251,7 +251,7 @@ modulemd_module_index_update_from_custom (ModulemdModuleIndex *self, * @self: This #ModulemdModuleIndex object. * @path: (in): The path to a directory containing defaults documents. * @strict: (in): Whether the parser should return failure if it encounters an - * unknown mapping key or if it should ignore it. + * unknown mapping key or a conflict in module default streams. * @overrides_path: (in) (nullable): If non-NULL, the path to a directory * containing defaults documents that should override those in @path. * @error: (out): A #GError indicating why this function failed. diff --git a/modulemd/include/private/modulemd-rpm-map-entry-private.h b/modulemd/include/private/modulemd-rpm-map-entry-private.h index a9cff19929037e3eaa0f14a999587af1e3b8f7bc..3f9945821dea63ba722736036e9f4daf803401be 100644 --- a/modulemd/include/private/modulemd-rpm-map-entry-private.h +++ b/modulemd/include/private/modulemd-rpm-map-entry-private.h @@ -37,8 +37,8 @@ */ ModulemdRpmMapEntry * modulemd_rpm_map_entry_parse_yaml (yaml_parser_t *parser, - gboolean strict, - GError **error); + gboolean strict, + GError **error); /** diff --git a/modulemd/modulemd-module-index.c b/modulemd/modulemd-module-index.c index af0c8e96291d9b3e1534cfeac80dbc8abe262330..ab81bf421b38554e813dec6e3767ac9c8a2baf35 100644 --- a/modulemd/modulemd-module-index.c +++ b/modulemd/modulemd-module-index.c @@ -671,76 +671,124 @@ modulemd_module_index_update_from_custom (ModulemdModuleIndex *self, } -gboolean -modulemd_module_index_update_from_defaults_directory ( - ModulemdModuleIndex *self, - const gchar *path, - gboolean strict, - const gchar *overrides_path, - GError **error) +/* + * modules_from_directory: + * @path: A directory containing one or more modulemd YAML documents + * @file_suffix: A file suffix to limit the files to be read. Pass "" if you + * need to read all files. + * @strict: Whether to fail on unknown fields + * @strict_default_streams: Whether to fail on default stream merges. + * @error: Error return value + */ +static ModulemdModuleIndex * +modules_from_directory (const gchar *path, + const gchar *file_suffix, + gboolean strict, + gboolean strict_default_streams, + GError **error) { const gchar *filename = NULL; + g_autoptr (GDir) dir = NULL; g_autofree gchar *filepath = NULL; - g_autoptr (GDir) defdir = NULL; - g_autoptr (GDir) overridedir = NULL; + g_autoptr (ModulemdModuleIndex) index = NULL; + g_autoptr (ModulemdModuleIndex) intermediate = NULL; g_autoptr (GPtrArray) failures = NULL; - g_autoptr (ModulemdModuleIndex) override_idx = NULL; + g_autoptr (GError) nested_error = NULL; - /* Read the regular path first */ - defdir = g_dir_open (path, 0, error); - if (!defdir) + index = modulemd_module_index_new (); + + /* Open the directory */ + dir = g_dir_open (path, 0, &nested_error); + if (!dir) { + g_propagate_error (error, g_steal_pointer (&nested_error)); return FALSE; } - while ((filename = g_dir_read_name (defdir)) != NULL) + while ((filename = g_dir_read_name (dir)) != NULL) { - if (g_str_has_suffix (filename, MMD_YAML_SUFFIX)) + if (g_str_has_suffix (filename, file_suffix)) { + intermediate = modulemd_module_index_new (); + filepath = g_build_path ("/", path, filename, NULL); - g_debug ("Reading defaults from %s", filepath); + g_debug ("Reading modulemd from %s", filepath); if (!modulemd_module_index_update_from_file ( - self, filepath, strict, &failures, error)) + intermediate, filepath, strict, &failures, error)) { return FALSE; } + + if (!modulemd_module_index_merge (intermediate, + index, + FALSE, + strict_default_streams, + &nested_error)) + { + g_propagate_error (error, g_steal_pointer (&nested_error)); + return FALSE; + } + g_clear_pointer (&failures, g_ptr_array_unref); g_clear_pointer (&filepath, g_free); + g_clear_object (&intermediate); } } + return g_steal_pointer (&index); +} + + +gboolean +modulemd_module_index_update_from_defaults_directory ( + ModulemdModuleIndex *self, + const gchar *path, + gboolean strict, + const gchar *overrides_path, + GError **error) +{ + g_autoptr (ModulemdModuleIndex) defaults_idx = NULL; + g_autoptr (ModulemdModuleIndex) override_idx = NULL; + g_autoptr (GError) nested_error = NULL; + + /* Read the regular path first */ + defaults_idx = modules_from_directory ( + path, MMD_YAML_SUFFIX, strict, strict, &nested_error); + if (!defaults_idx) + { + g_propagate_error (error, g_steal_pointer (&nested_error)); + return FALSE; + } + + /* If an override path was provided, use that too */ if (overrides_path) { - overridedir = g_dir_open (overrides_path, 0, error); - if (!overridedir) + override_idx = modules_from_directory ( + overrides_path, MMD_YAML_SUFFIX, strict, strict, &nested_error); + if (!override_idx) { + g_propagate_error (error, g_steal_pointer (&nested_error)); return FALSE; } - override_idx = modulemd_module_index_new (); - while ((filename = g_dir_read_name (overridedir)) != NULL) - { - if (g_str_has_suffix (filename, MMD_YAML_SUFFIX)) - { - filepath = g_build_path ("/", overrides_path, filename, NULL); - g_debug ("Reading default overrides from %s", filepath); - if (!modulemd_module_index_update_from_file ( - override_idx, filepath, strict, &failures, error)) - { - return FALSE; - } - g_clear_pointer (&failures, g_ptr_array_unref); - g_clear_pointer (&filepath, g_free); - } - } - if (!modulemd_module_index_merge ( - override_idx, self, TRUE, FALSE, error)) + override_idx, defaults_idx, TRUE, strict, &nested_error)) { + g_propagate_error (error, g_steal_pointer (&nested_error)); return FALSE; } } + /* Now that we've verified that the content in the two paths is compatible, + * attempt to merge it into the existing index. + */ + if (!modulemd_module_index_merge ( + defaults_idx, self, TRUE, strict, &nested_error)) + { + g_propagate_error (error, g_steal_pointer (&nested_error)); + return FALSE; + } + return TRUE; } diff --git a/modulemd/tests/test-modulemd-moduleindex.c b/modulemd/tests/test-modulemd-moduleindex.c index 03db9de9c707d0c6c96d7f21ed5aabddf315fbba..2fc1237ff0006a329abd2f0f311b7743d8dbba31 100644 --- a/modulemd/tests/test-modulemd-moduleindex.c +++ b/modulemd/tests/test-modulemd-moduleindex.c @@ -1193,6 +1193,8 @@ test_module_index_read_def_dir (void) g_autoptr (GError) error = NULL; g_autofree gchar *path = g_build_path ("/", g_getenv ("TEST_DATA_PATH"), "defaults", NULL); + g_autofree gchar *bad_path = + g_build_path ("/", g_getenv ("TEST_DATA_PATH"), "bad_defaults", NULL); g_autofree gchar *overrides_path = g_build_path ("/", path, "overrides", NULL); g_auto (GStrv) module_names = NULL; @@ -1213,8 +1215,6 @@ test_module_index_read_def_dir (void) module_names = modulemd_module_index_get_module_names_as_strv (idx); g_assert_nonnull (module_names); - g_assert_cmpint (g_strv_length (module_names), ==, 3); - g_assert_true ( g_strv_contains ((const gchar *const *)module_names, "meson")); g_assert_true ( @@ -1222,6 +1222,8 @@ test_module_index_read_def_dir (void) g_assert_true ( g_strv_contains ((const gchar *const *)module_names, "nodejs")); + g_assert_cmpint (g_strv_length (module_names), ==, 3); + defaultdict = modulemd_module_index_get_default_streams_as_hash_table (idx, NULL); g_assert_nonnull (defaultdict); @@ -1248,8 +1250,6 @@ test_module_index_read_def_dir (void) module_names = modulemd_module_index_get_module_names_as_strv (idx); g_assert_nonnull (module_names); - g_assert_cmpint (g_strv_length (module_names), ==, 4); - g_assert_true ( g_strv_contains ((const gchar *const *)module_names, "meson")); g_assert_true ( @@ -1259,6 +1259,8 @@ test_module_index_read_def_dir (void) g_assert_true ( g_strv_contains ((const gchar *const *)module_names, "testmodule")); + g_assert_cmpint (g_strv_length (module_names), ==, 4); + defaultdict = modulemd_module_index_get_default_streams_as_hash_table (idx, NULL); g_assert_nonnull (defaultdict); @@ -1284,6 +1286,65 @@ test_module_index_read_def_dir (void) idx, path, TRUE, "nonexistent", &error)); g_assert_error (error, G_FILE_ERROR, G_FILE_ERROR_NOENT); g_clear_error (&error); + + + g_clear_object (&idx); + idx = modulemd_module_index_new (); + g_assert_nonnull (idx); + + /* Base directory contains two defaults with conflicting streams for the + * same module in separate files. Strict mode. + */ + g_assert_false (modulemd_module_index_update_from_defaults_directory ( + idx, bad_path, TRUE, NULL, &error)); + g_assert_error (error, MODULEMD_ERROR, MODULEMD_ERROR_VALIDATE); + g_clear_error (&error); + + /* Verify that the index has not been modified as a side-effect */ + module_names = modulemd_module_index_get_module_names_as_strv (idx); + g_assert_cmpint (g_strv_length (module_names), ==, 0); + + g_clear_pointer (&module_names, g_strfreev); + g_clear_object (&idx); + idx = modulemd_module_index_new (); + g_assert_nonnull (idx); + + /* Base directory contains two defaults with conflicting streams for the + * same module in separate files. Non-strict mode. + */ + g_assert_true (modulemd_module_index_update_from_defaults_directory ( + idx, bad_path, FALSE, NULL, &error)); + g_assert_no_error (error); + + + /* There should be three modules in the index now: + * - meson + * - ninja + * - nodejs + */ + module_names = modulemd_module_index_get_module_names_as_strv (idx); + g_assert_nonnull (module_names); + + g_assert_true ( + g_strv_contains ((const gchar *const *)module_names, "meson")); + g_assert_true ( + g_strv_contains ((const gchar *const *)module_names, "ninja")); + g_assert_true ( + g_strv_contains ((const gchar *const *)module_names, "nodejs")); + + g_assert_cmpint (g_strv_length (module_names), ==, 3); + + defaultdict = + modulemd_module_index_get_default_streams_as_hash_table (idx, NULL); + g_assert_nonnull (defaultdict); + + /* Make sure that in non-strict mode, meson's default is reset to NULL */ + g_assert_cmpstr (g_hash_table_lookup (defaultdict, "meson"), ==, NULL); + g_assert_cmpstr (g_hash_table_lookup (defaultdict, "ninja"), ==, "latest"); + g_assert_cmpstr (g_hash_table_lookup (defaultdict, "nodejs"), ==, NULL); + + g_clear_pointer (&module_names, g_strfreev); + g_clear_pointer (&defaultdict, g_hash_table_unref); } diff --git a/modulemd/tests/test_data/bad_defaults/meson-2.yaml b/modulemd/tests/test_data/bad_defaults/meson-2.yaml new file mode 100644 index 0000000000000000000000000000000000000000..9ebec5ed3de80307bd19c46e1a776a9e782ce56d --- /dev/null +++ b/modulemd/tests/test_data/bad_defaults/meson-2.yaml @@ -0,0 +1,7 @@ +document: modulemd-defaults +version: 1 +data: + module: meson + stream: old + profiles: + latest: [default] diff --git a/modulemd/tests/test_data/bad_defaults/meson.yaml b/modulemd/tests/test_data/bad_defaults/meson.yaml new file mode 100644 index 0000000000000000000000000000000000000000..65c892223f41faf103db47698136b44e6543631d --- /dev/null +++ b/modulemd/tests/test_data/bad_defaults/meson.yaml @@ -0,0 +1,7 @@ +document: modulemd-defaults +version: 1 +data: + module: meson + stream: latest + profiles: + latest: [default] diff --git a/modulemd/tests/test_data/bad_defaults/ninja.yaml b/modulemd/tests/test_data/bad_defaults/ninja.yaml new file mode 100644 index 0000000000000000000000000000000000000000..a7ecb6e58c0125bcd957c4bb9c9fd4a375d80bb6 --- /dev/null +++ b/modulemd/tests/test_data/bad_defaults/ninja.yaml @@ -0,0 +1,7 @@ +document: modulemd-defaults +version: 1 +data: + module: ninja + stream: latest + profiles: + latest: [default] diff --git a/modulemd/tests/test_data/bad_defaults/nodejs.yaml b/modulemd/tests/test_data/bad_defaults/nodejs.yaml new file mode 100644 index 0000000000000000000000000000000000000000..9d2b38d52ce0136a2df34f145a54f33b15015767 --- /dev/null +++ b/modulemd/tests/test_data/bad_defaults/nodejs.yaml @@ -0,0 +1,9 @@ +document: modulemd-defaults +version: 1 +data: + modified: 201906261200 + module: nodejs + profiles: + 8: [default] + 10: [default] + 12: [default] -- 2.23.0