diff --git a/include/asdf/value.h b/include/asdf/value.h index 33e5ebf0..9feefd8d 100644 --- a/include/asdf/value.h +++ b/include/asdf/value.h @@ -296,6 +296,14 @@ typedef struct { */ ASDF_EXPORT asdf_mapping_iter_t *asdf_mapping_iter_init(asdf_mapping_t *mapping); +/** + * Create a new reverse iterator over ``mapping`` + * + * :param mapping: The `asdf_mapping_t *` to iterate over + * :return: A new `asdf_mapping_iter_t *` handle, or ``NULL`` on allocation failure + */ +ASDF_EXPORT asdf_mapping_iter_t *asdf_mapping_reverse_iter_init(asdf_mapping_t *mapping); + /** * Advance the iterator to the next mapping entry * @@ -544,6 +552,14 @@ typedef struct { */ ASDF_EXPORT asdf_sequence_iter_t *asdf_sequence_iter_init(asdf_sequence_t *sequence); +/** + * Create a new reverse iterator over ``sequence`` + * + * :param sequence: The `asdf_sequence_t *` to iterate over + * :return: A new `asdf_sequence_iter_t *` handle, or ``NULL`` on allocation failure + */ +ASDF_EXPORT asdf_sequence_iter_t *asdf_sequence_reverse_iter_init(asdf_sequence_t *sequence); + /** * Advance the iterator to the next sequence element * @@ -714,6 +730,16 @@ typedef struct { */ ASDF_EXPORT asdf_container_iter_t *asdf_container_iter_init(asdf_value_t *container); +/** + * Create a new reverse iterator over ``container`` (mapping or sequence) + * + * Returns ``NULL`` if ``container`` is neither a mapping nor a sequence. + * + * :param container: `asdf_value_t *` of mapping or sequence type + * :return: A new `asdf_container_iter_t *` handle, or ``NULL`` on failure + */ +ASDF_EXPORT asdf_container_iter_t *asdf_container_reverse_iter_init(asdf_value_t *container); + /** * Advance the generic container iterator to the next element * diff --git a/src/emitter.c b/src/emitter.c index 1a46abc2..db7fe74e 100644 --- a/src/emitter.c +++ b/src/emitter.c @@ -298,6 +298,7 @@ static int emit_prepare_root_node(asdf_emitter_t *emitter) { if (!meta) goto cleanup; + // Replace the original meta->asdf_library, if any asdf_software_destroy(meta->asdf_library); if (asdf_emitter_has_opt(emitter, ASDF_EMITTER_OPT_NO_EMIT_ASDF_LIBRARY)) { @@ -314,6 +315,14 @@ static int emit_prepare_root_node(asdf_emitter_t *emitter) { // that ought to be avoided somehow. Either change these to use STC // vectors or at least an internal sized array type... if (file->history_entries && *file->history_entries) { + // NOTE: If the history entries were added *before* a call to + // asdf_library_set_version then these history entries may show the + // "wrong", non-overridden version. In principle should fix up their + // attached software version as is done above with meta->asdf_library. + // We opt for now not to go out of our way to fix this, because + // overriding the library version is only needed for debug/tests. The + // workaround is to set the version override *before* adding any history + // entries. meta->history.entries = (const asdf_history_entry_t **)asdf_array_concat( (void **)meta->history.entries, (const void **)file->history_entries); @@ -340,7 +349,7 @@ static int emit_prepare_root_node(asdf_emitter_t *emitter) { if (asdf_get_mapping(file, "/", &root) != ASDF_VALUE_OK) goto cleanup; - if (asdf_mapping_update(root, meta_map) != ASDF_VALUE_OK) + if (asdf_mapping_update_ex(root, meta_map, true) != ASDF_VALUE_OK) goto cleanup; ret = 0; diff --git a/src/value.c b/src/value.c index eb1fc575..f6e6e40f 100644 --- a/src/value.c +++ b/src/value.c @@ -495,8 +495,8 @@ void asdf_mapping_destroy(asdf_mapping_t *mapping) { /** Helper for asdf_mapping_set_* methods */ -static inline asdf_value_err_t asdf_mapping_set_node( - asdf_mapping_t *mapping, const char *key, struct fy_node *value) { +static inline asdf_value_err_t asdf_mapping_set_node_ex( + asdf_mapping_t *mapping, const char *key, struct fy_node *value, bool prepend) { if (!mapping) return ASDF_VALUE_ERR_UNKNOWN; @@ -518,15 +518,28 @@ static inline asdf_value_err_t asdf_mapping_set_node( if (fy_node_pair_set_value(pair, value) != 0) { return ASDF_VALUE_ERR_OOM; } - } else if (fy_node_mapping_append(mapping->value.node, key_node, value) != 0) { - fy_node_free(key_node); - return ASDF_VALUE_ERR_OOM; + } else if (prepend) { + if (fy_node_mapping_prepend(mapping->value.node, key_node, value) != 0) { + fy_node_free(key_node); + return ASDF_VALUE_ERR_OOM; + } + } else { + if (fy_node_mapping_append(mapping->value.node, key_node, value) != 0) { + fy_node_free(key_node); + return ASDF_VALUE_ERR_OOM; + } } return ASDF_VALUE_OK; } +static inline asdf_value_err_t asdf_mapping_set_node( + asdf_mapping_t *mapping, const char *key, struct fy_node *value) { + return asdf_mapping_set_node_ex(mapping, key, value, false); +} + + asdf_value_err_t asdf_mapping_set_string( // NOLINTNEXTLINE(bugprone-easily-swappable-parameters) asdf_mapping_t *mapping, @@ -588,8 +601,10 @@ asdf_value_err_t asdf_mapping_set_null(asdf_mapping_t *mapping, const char *key) } -asdf_value_err_t asdf_mapping_set(asdf_mapping_t *mapping, const char *key, asdf_value_t *value) { - asdf_value_err_t err = asdf_mapping_set_node(mapping, key, asdf_value_normalize_node(value)); +static asdf_value_err_t asdf_mapping_set_ex( + asdf_mapping_t *mapping, const char *key, asdf_value_t *value, bool prepend) { + asdf_value_err_t err = asdf_mapping_set_node_ex( + mapping, key, asdf_value_normalize_node(value), prepend); /* fy_node_mapping_append implicitly frees the original node, so here set it * to null to avoid double-freeing it and then just destroy the asdf_value_t */ value->node = NULL; @@ -598,6 +613,11 @@ asdf_value_err_t asdf_mapping_set(asdf_mapping_t *mapping, const char *key, asdf } +asdf_value_err_t asdf_mapping_set(asdf_mapping_t *mapping, const char *key, asdf_value_t *value) { + return asdf_mapping_set_ex(mapping, key, value, false); +} + + #define ASDF_MAPPING_SET_CONTAINER_TYPE(type) \ asdf_value_err_t asdf_mapping_set_##type( \ asdf_mapping_t *mapping, const char *key, asdf_##type##_t *value) { \ @@ -620,7 +640,7 @@ ASDF_MAPPING_SET_CONTAINER_TYPE(mapping); ASDF_MAPPING_SET_CONTAINER_TYPE(sequence); -asdf_mapping_iter_t *asdf_mapping_iter_init(asdf_mapping_t *mapping) { +static asdf_mapping_iter_t *asdf_mapping_iter_init_ex(asdf_mapping_t *mapping, bool reverse) { asdf_mapping_iter_impl_t *impl = calloc(1, sizeof(asdf_mapping_iter_impl_t)); if (!impl) { @@ -629,10 +649,21 @@ asdf_mapping_iter_t *asdf_mapping_iter_init(asdf_mapping_t *mapping) { } impl->mapping = mapping; + impl->reverse = reverse; return (asdf_mapping_iter_t *)impl; } +asdf_mapping_iter_t *asdf_mapping_iter_init(asdf_mapping_t *mapping) { + return asdf_mapping_iter_init_ex(mapping, false); +} + + +asdf_mapping_iter_t *asdf_mapping_reverse_iter_init(asdf_mapping_t *mapping) { + return asdf_mapping_iter_init_ex(mapping, true); +} + + void asdf_mapping_iter_destroy(asdf_mapping_iter_t *iter) { if (!iter) return; @@ -661,9 +692,15 @@ bool asdf_mapping_iter_next(asdf_mapping_iter_t **iter_ptr) { goto cleanup; } - struct fy_node_pair *pair = fy_node_mapping_iterate(mapping->value.node, &impl->fy_iter); + struct fy_node_pair *pair = NULL; - if (!pair) + if (impl->reverse) { + pair = fy_node_mapping_reverse_iterate(mapping->value.node, &impl->fy_iter); + } else { + pair = fy_node_mapping_iterate(mapping->value.node, &impl->fy_iter); + } + + if (UNLIKELY(!pair)) goto cleanup; struct fy_node *key_node = fy_node_pair_key(pair); @@ -702,10 +739,20 @@ bool asdf_mapping_iter_next(asdf_mapping_iter_t **iter_ptr) { // NOLINTNEXTLINE(bugprone-easily-swappable-parameters) -asdf_value_err_t asdf_mapping_update(asdf_mapping_t *mapping, asdf_mapping_t *update) { - asdf_mapping_iter_t *iter = asdf_mapping_iter_init(update); +asdf_value_err_t asdf_mapping_update_ex( + asdf_mapping_t *mapping, asdf_mapping_t *update, bool prepend) { + asdf_mapping_iter_t *iter = NULL; + + if (prepend) { + // Reverse iterate so each item gets prepended in their original order + iter = asdf_mapping_reverse_iter_init(update); + } else { + iter = asdf_mapping_iter_init(update); + } + if (!iter) return ASDF_VALUE_ERR_OOM; + asdf_value_err_t err = ASDF_VALUE_OK; while (asdf_mapping_iter_next(&iter)) { @@ -714,7 +761,7 @@ asdf_value_err_t asdf_mapping_update(asdf_mapping_t *mapping, asdf_mapping_t *up asdf_mapping_iter_destroy(iter); return ASDF_VALUE_ERR_OOM; } - err = asdf_mapping_set(mapping, iter->key, clone); + err = asdf_mapping_set_ex(mapping, iter->key, clone, prepend); if (err) { asdf_mapping_iter_destroy(iter); break; @@ -725,6 +772,11 @@ asdf_value_err_t asdf_mapping_update(asdf_mapping_t *mapping, asdf_mapping_t *up } +asdf_value_err_t asdf_mapping_update(asdf_mapping_t *mapping, asdf_mapping_t *update) { + return asdf_mapping_update_ex(mapping, update, false); +} + + asdf_value_t *asdf_mapping_pop(asdf_mapping_t *mapping, const char *key) { if (UNLIKELY(!mapping || !key)) return NULL; @@ -840,7 +892,7 @@ void asdf_sequence_destroy(asdf_sequence_t *sequence) { } -asdf_sequence_iter_t *asdf_sequence_iter_init(asdf_sequence_t *sequence) { +asdf_sequence_iter_t *asdf_sequence_iter_init_ex(asdf_sequence_t *sequence, bool reverse) { asdf_sequence_iter_impl_t *impl = calloc(1, sizeof(asdf_sequence_iter_impl_t)); if (UNLIKELY(!impl)) { @@ -849,11 +901,28 @@ asdf_sequence_iter_t *asdf_sequence_iter_init(asdf_sequence_t *sequence) { } impl->sequence = sequence; - impl->pub.index = -1; + impl->reverse = reverse; + + if (reverse) { + impl->pub.index = asdf_sequence_size(sequence); + } else { + impl->pub.index = -1; + } + return (asdf_sequence_iter_t *)impl; } +asdf_sequence_iter_t *asdf_sequence_iter_init(asdf_sequence_t *sequence) { + return asdf_sequence_iter_init_ex(sequence, false); +} + + +asdf_sequence_iter_t *asdf_sequence_reverse_iter_init(asdf_sequence_t *sequence) { + return asdf_sequence_iter_init_ex(sequence, true); +} + + void asdf_sequence_iter_destroy(asdf_sequence_iter_t *iter) { if (!iter) return; @@ -882,12 +951,23 @@ bool asdf_sequence_iter_next(asdf_sequence_iter_t **iter_ptr) { goto cleanup; } - struct fy_node *value_node = fy_node_sequence_iterate(sequence->value.node, &impl->fy_iter); + struct fy_node *value_node = NULL; + + if (impl->reverse) { + value_node = fy_node_sequence_reverse_iterate(sequence->value.node, &impl->fy_iter); + } else { + value_node = fy_node_sequence_iterate(sequence->value.node, &impl->fy_iter); + } if (UNLIKELY(!value_node)) goto cleanup; - impl->pub.index++; + if (impl->reverse) { + impl->pub.index--; + } else { + impl->pub.index++; + } + asdf_value_t *value = asdf_value_create_ex( sequence->value.file, value_node, &sequence->value, NULL, impl->pub.index); @@ -1205,7 +1285,7 @@ asdf_value_t *asdf_sequence_pop(asdf_sequence_t *sequence, int index) { /** Generic container functions */ -asdf_container_iter_t *asdf_container_iter_init(asdf_value_t *container) { +asdf_container_iter_t *asdf_container_iter_init_ex(asdf_value_t *container, bool reverse) { if (!container) return NULL; @@ -1226,16 +1306,25 @@ asdf_container_iter_t *asdf_container_iter_init(asdf_value_t *container) { impl->container = container; impl->is_mapping = (container->raw_type == ASDF_VALUE_MAPPING); - impl->pub.index = -1; + + if (reverse) { + if (impl->is_mapping) { + impl->pub.index = asdf_mapping_size((asdf_mapping_t *)container); + } else { + impl->pub.index = asdf_sequence_size((asdf_sequence_t *)container); + } + } else { + impl->pub.index = -1; + } if (impl->is_mapping) { - impl->iter.mapping = asdf_mapping_iter_init((asdf_mapping_t *)container); + impl->iter.mapping = asdf_mapping_iter_init_ex((asdf_mapping_t *)container, reverse); if (!impl->iter.mapping) { free(impl); return NULL; } } else { - impl->iter.sequence = asdf_sequence_iter_init((asdf_sequence_t *)container); + impl->iter.sequence = asdf_sequence_iter_init_ex((asdf_sequence_t *)container, reverse); if (!impl->iter.sequence) { free(impl); return NULL; @@ -1246,6 +1335,16 @@ asdf_container_iter_t *asdf_container_iter_init(asdf_value_t *container) { } +asdf_container_iter_t *asdf_container_iter_init(asdf_value_t *container) { + return asdf_container_iter_init_ex(container, false); +} + + +asdf_container_iter_t *asdf_container_reverse_iter_init(asdf_value_t *container) { + return asdf_container_iter_init_ex(container, true); +} + + void asdf_container_iter_destroy(asdf_container_iter_t *iter) { if (!iter) return; @@ -1273,8 +1372,13 @@ bool asdf_container_iter_next(asdf_container_iter_t **iter_ptr) { goto cleanup; impl->pub.key = impl->iter.mapping->key; - impl->pub.index++; impl->pub.value = impl->iter.mapping->value; + + if (((asdf_mapping_iter_impl_t *)impl->iter.mapping)->reverse) { + impl->pub.index--; + } else { + impl->pub.index++; + } return true; } diff --git a/src/value.h b/src/value.h index b2fa7e84..0bc98a9d 100644 --- a/src/value.h +++ b/src/value.h @@ -53,6 +53,7 @@ typedef struct asdf_mapping_iter_impl { asdf_mapping_iter_t pub; asdf_mapping_t *mapping; void *fy_iter; + bool reverse; } asdf_mapping_iter_impl_t; @@ -66,6 +67,7 @@ typedef struct asdf_sequence_iter_impl { asdf_sequence_iter_t pub; asdf_sequence_t *sequence; void *fy_iter; + bool reverse; } asdf_sequence_iter_impl_t; @@ -179,6 +181,10 @@ ASDF_LOCAL struct fy_node *asdf_value_normalize_node(asdf_value_t *value); /** * Additional internal functions not yet exposed in the public API */ +/* Not sure I want asdf_mapping_update_ex public yet; right now it only takes a 'prepend' flag + * but I'm thinking it might want to take a more extended flag set for other options */ +ASDF_LOCAL asdf_value_err_t +asdf_mapping_update_ex(asdf_mapping_t *mapping, asdf_mapping_t *update, bool prepend); ASDF_LOCAL asdf_value_t *asdf_value_clone_deep(asdf_value_t *value); ASDF_LOCAL asdf_value_err_t asdf_node_insert_at( struct fy_document *doc, const char *path, struct fy_node *node, bool materialize); diff --git a/tests/Makefile.am b/tests/Makefile.am index 5e67bb80..f4a7a4a1 100644 --- a/tests/Makefile.am +++ b/tests/Makefile.am @@ -361,6 +361,7 @@ EXTRA_DIST += \ fixtures/info/structured.info.txt \ fixtures/info/unicode_bmp.info.txt \ fixtures/info/unicode_spp.info.txt \ + fixtures/issue-204.asdf \ fixtures/multi-block.asdf \ fixtures/multiple_hdu.asdf \ fixtures/nested.asdf \ diff --git a/tests/fixtures/issue-204.asdf b/tests/fixtures/issue-204.asdf new file mode 100644 index 00000000..afbf896a --- /dev/null +++ b/tests/fixtures/issue-204.asdf @@ -0,0 +1,24 @@ +#ASDF 1.0.0 +#ASDF_STANDARD 1.6.0 +%YAML 1.1 +%TAG ! tag:stsci.edu:asdf/ +--- !core/asdf-1.1.0 +asdf_library: !core/software-1.0.0 + name: libasdf + version: 0.0.0 + author: The libasdf Developers + homepage: https://github.com/asdf-format/libasdf +history: + entries: + - !core/history_entry-1.0.0 + description: Test metadata emission. + software: !core/software-1.0.0 + name: libasdf + version: 0.0.0 + author: The libasdf Developers + homepage: https://github.com/asdf-format/libasdf +observer: Dennis Richie +obstime: !time/time-1.4.0 + value: 1948.78707178 + format: jyear +... diff --git a/tests/fixtures/scalars-out.asdf b/tests/fixtures/scalars-out.asdf index f9d4be2a..436e7c8d 100644 --- a/tests/fixtures/scalars-out.asdf +++ b/tests/fixtures/scalars-out.asdf @@ -3,6 +3,11 @@ %YAML 1.1 %TAG ! tag:stsci.edu:asdf/ --- !core/asdf-1.1.0 +asdf_library: !core/software-1.0.0 + name: libasdf + version: 0.0.0 + author: The libasdf Developers + homepage: https://github.com/asdf-format/libasdf string: string string0: string0 null: null @@ -18,9 +23,4 @@ uint32: 4294967295 uint64: 18446744073709551615 float: 3.40282347e+38 double: 1.7976931348623157e+308 -asdf_library: !core/software-1.0.0 - name: libasdf - version: 0.0.0 - author: The libasdf Developers - homepage: https://github.com/asdf-format/libasdf ... diff --git a/tests/fixtures/trivial-extension.asdf b/tests/fixtures/trivial-extension.asdf index 7361d700..db8dc87f 100644 --- a/tests/fixtures/trivial-extension.asdf +++ b/tests/fixtures/trivial-extension.asdf @@ -3,10 +3,10 @@ %YAML 1.1 %TAG ! tag:stsci.edu:asdf/ --- !core/asdf-1.1.0 -foo: !tests/foo-1.0.0 foo asdf_library: !core/software-1.0.0 name: libasdf version: 0.0.0 author: The libasdf Developers homepage: https://github.com/asdf-format/libasdf +foo: !tests/foo-1.0.0 foo ... diff --git a/tests/test-emitter.c b/tests/test-emitter.c index 34e51cab..06c01cba 100644 --- a/tests/test-emitter.c +++ b/tests/test-emitter.c @@ -131,9 +131,36 @@ MU_TEST(test_emitter_stream_switch) { } +/** Regression test for issue #204 */ +MU_TEST(meta_prepended_to_root) { + const char *filename = get_temp_file_path(fixture->tempfile_prefix, ".asdf"); + asdf_file_t *file = asdf_open(NULL); + + // Add some keys to the file + asdf_set_string0(file, "observer", "Dennis Richie"); + + asdf_time_t time = { + .value = "1948.78707178", + .format = ASDF_TIME_FORMAT_JYEAR + }; + asdf_set_time(file, "obstime", &time); + + // Add a history entry + asdf_library_set_version(file, "0.0.0"); + assert_int(asdf_history_entry_add(file, "Test metadata emission."), ==, 0); + asdf_write_to(file, filename); + asdf_close(file); + + const char *fixture_filename = get_fixture_file_path("issue-204.asdf"); + assert_true(compare_files(filename, fixture_filename)); + return MUNIT_OK; +} + + MU_TEST_SUITE( emitter, - MU_RUN_TEST(test_emitter_stream_switch) + MU_RUN_TEST(test_emitter_stream_switch), + MU_RUN_TEST(meta_prepended_to_root) );