fix: Warn + preserve unknown tags

Author: icweaver

src/ASDF.jl

@@ -883,6 +883,65 @@ function correct_byteorder(data, ::Ucs4Datatype, byteorder::Byteorder)
     return data
 end
 
+"""
+    TaggedMapping{D} <: AbstractDict
+    TaggedSequence{V} <: AbstractVector
+    TaggedScalar <: AbstractString
+
+A YAML node that carries an unrecognized (extension) `tag`, produced by [`ASDF.load_file`](@ref)
+when called with `extensions = true`. Each behaves exactly like its wrapped `value` — a mapping
+indexes and iterates as a dict, a sequence as a vector, a scalar as a string — while retaining
+the original `tag` so the node round-trips unchanged through [`ASDF.write_file`](@ref).
+"""
+struct TaggedMapping{D<:AbstractDict} <: AbstractDict{Any, Any}
+    tag::String
+    value::D
+end
+# `keys`, `values`, `haskey`, `get`, etc. fall back to these via the `AbstractDict` interface.
+Base.getindex(m::TaggedMapping, k) = getindex(m.value, k)
+Base.length(m::TaggedMapping) = length(m.value)
+Base.iterate(m::TaggedMapping, state...) = iterate(m.value, state...)
+
+@doc (@doc TaggedMapping)
+struct TaggedSequence{V<:AbstractVector} <: AbstractVector{Any}
+    tag::String
+    value::V
+end
+Base.size(s::TaggedSequence) = size(s.value)
+Base.getindex(s::TaggedSequence, i::Int) = getindex(s.value, i)
+Base.IndexStyle(::Type{<:TaggedSequence}) = IndexLinear()
+
+@doc (@doc TaggedMapping)
+struct TaggedScalar <: AbstractString
+    tag::String
+    value::String
+end
+Base.ncodeunits(s::TaggedScalar) = ncodeunits(s.value)
+Base.codeunit(s::TaggedScalar) = codeunit(s.value)
+Base.codeunit(s::TaggedScalar, i::Integer) = codeunit(s.value, i)
+Base.isvalid(s::TaggedScalar, i::Integer) = isvalid(s.value, i)
+Base.iterate(s::TaggedScalar, i::Integer = 1) = iterate(s.value, i)
+
+# Serialize a tagged node by emitting its verbatim tag (`!<...>`) right before the wrapped
+# value, mirroring how `NDArrayWrapper` writes `!core/ndarray-1.0.0`. The YAML writer breaks the
+# line after the key only for a *non-empty* block collection, so the tag goes on its own
+# indented line there; for scalars and for empty collections (printed inline as `{}`/`[]`) the
+# writer leaves us on the key's line, so the tag stays inline. Both forms parse back to a tagged
+# node.
+function YAML._print(io::IO, val::Union{TaggedMapping, TaggedSequence}, level::Int = 0, ignore_level::Bool = false)
+    if isempty(val.value)
+        print(io, "!<", val.tag, "> ")
+        YAML._print(io, val.value, level, ignore_level)
+    else
+        print(io, YAML._indent("!<" * val.tag * ">\n", level, ignore_level))
+        YAML._print(io, val.value, level)
+    end
+end
+function YAML._print(io::IO, val::TaggedScalar, level::Int = 0, ignore_level::Bool = false)
+    print(io, "!<", val.tag, "> ")
+    YAML._print(io, val.value, level, ignore_level)
+end
+
 function YAML._print(io::IO, val::NDArray, level::Int = 0, ignore_level::Bool = false)
     # TODO: Get compression from underlying header block?
     YAML._print(io, NDArrayWrapper(val[]; compression = C_None), level, ignore_level)
@@ -1164,7 +1223,7 @@ Reads an ASDF file from disk.
 | Parameter           | Description                                                                                                      |
 | :------------------ | :--------------------------------------------------------------------------------------------------------------- |
 | `filename`          | Path to the `.asdf` file                                                                                         |
-| `extensions`        | When `true`, unknown YAML tags are parsed leniently (as maps, sequences, or scalars) instead of raising an error |
+| `extensions`        | When `true`, unknown YAML tags are parsed leniently (as maps, sequences, or scalars) instead of raising an error, and a warning naming each unrecognized tag is emitted (once per distinct tag). The tag is retained (see [`ASDF.TaggedMapping`](@ref)) so the node round-trips unchanged on write. |
 | `validate_checksum` | When `true`, each block's MD5 checksum is verified against the stored value                                      |
 
 Block data is located lazily. Block headers are scanned after the YAML is parsed, and array data (`ndarray`) is read only when [`Base.getindex(ndarray::NDArray)`](@ref) is called, i.e., `ndarray[]`.
@@ -1181,14 +1240,21 @@ function load_file(filename::AbstractString; extensions = false, validate_checks
     asdf_constructors["tag:stsci.edu:asdf/core/extension_metadata-1.0.0"] = ordered_map_constructor
 
     if extensions
-        # Use fallbacks for now
+        # Use fallbacks for now. Track which unrecognized tags we have already
+        # warned about so each distinct tag is reported at most once per load.
+        warned_tags = Set{Any}()
         asdf_constructors[nothing] = (constructor, node) -> begin
+            if node.tag ∉ warned_tags
+                push!(warned_tags, node.tag)
+                @warn "Unrecognized tag encountered while loading; falling back to a generic representation" tag = node.tag
+            end
+            # Wrap in a `Tagged*` type so the unrecognized tag is retained and round-trips on write.
             if node isa YAML.MappingNode
-                return YAML.construct_mapping(constructor, node)
+                return TaggedMapping(node.tag, YAML.construct_mapping(OrderedDict{Any, Any}, constructor, node))
             elseif node isa YAML.SequenceNode
-                return YAML.construct_sequence(constructor, node)
+                return TaggedSequence(node.tag, YAML.construct_sequence(constructor, node))
             else
-                return YAML.construct_scalar(constructor, node)
+                return TaggedScalar(node.tag, YAML.construct_scalar(constructor, node))
             end
         end
     end
@@ -1444,7 +1510,11 @@ function write_file(filename::AbstractString, document::AbstractDict)
     # - [ ] maybe make the document not a `Dict` but the stuff with the `metadata` that the writer returns?
     # - [ ] preserve insertion order? https://github.com/JuliaAstro/ASDF.jl/tree/ordered
     library = ASDFLibrary(software_name, software_author, software_homepage, software_version)
-    full_document = merge(document, OrderedDict{Any, Any}("asdf_library" => library))
+    # Build the output tree as an `OrderedDict` regardless of the input dict's concrete type, so
+    # insertion order is preserved (for a `TaggedMapping` document, `merge` would otherwise fall
+    # back to an unordered `Dict` and drop the order). The provenance entry is stamped last.
+    full_document = OrderedDict{Any, Any}(document)
+    full_document["asdf_library"] = library
 
     # Write YAML part of file
     io = open(filename, "w")

test/test-read_fallbacks.jl

@@ -33,8 +33,6 @@ end
 
     # The default `extensions = false` case
     @test_throws Exception load_tag(tag_unknown_mapping; extensions = false)
-    @test_throws Exception load_tag(tag_unknown_sequence; extensions = false)
-    @test_throws Exception load_tag(tag_unknown_scalar; extensions = false)
 
     # Should fall back to an `AbstractDict`
     af = load_tag(tag_unknown_mapping; extensions = true)
@@ -46,10 +44,12 @@ end
     # Known key still parsed as normal
     @test af.metadata["known_key"] == "hello"
 
-    # And loading the file multiple times does not mutate any shared/global state
+    # Two loads must not cross-contaminate via shared/global state: load a second file with a
+    # different value *after* the first, then check the first still reads its own value.
     af1 = load_tag(tag_unknown_mapping; extensions = true)
-    af2 = load_tag(tag_unknown_mapping; extensions = true)
-    @test af1.metadata["custom_obj"]["width"] == af2.metadata["custom_obj"]["width"]
+    af2 = load_tag(replace(tag_unknown_mapping, "width: 42" => "width: 99"); extensions = true)
+    @test af1.metadata["custom_obj"]["width"] == 42
+    @test af2.metadata["custom_obj"]["width"] == 99
 end
 
 @testset "unknown sequence" begin
@@ -61,6 +61,9 @@ end
       - gamma
     """
 
+    # The default `extensions = false` case
+    @test_throws Exception load_tag(tag_unknown_sequence; extensions = false)
+
     # Should fall back to an `AbstractVector`
     af = load_tag(tag_unknown_sequence; extensions = true)
     list = af.metadata["custom_list"]
@@ -79,6 +82,9 @@ end
     custom_value: !<tag:example.org:mylib/quantity-1.0.0> 3.14
     """
 
+    # The default `extensions = false` case
+    @test_throws Exception load_tag(tag_unknown_scalar; extensions = false)
+
     # Should fall back to an `AbstractString`
     af = load_tag(tag_unknown_scalar; extensions = true)
     value = af.metadata["custom_value"]
@@ -118,3 +124,73 @@ end
     # Known key unaffected
     @test md["known_key"] == "hello"
 end
+
+@testset "unrecognized tag warning" begin
+    tag_unknown_scalar = """
+    custom_value: !<tag:example.org:mylib/quantity-1.0.0> 3.14
+    """
+
+    # A warning naming the offending tag should be emitted on load.
+    @test_logs (:warn, r"Unrecognized tag") match_mode = :any begin
+        load_tag(tag_unknown_scalar; extensions = true)
+    end
+
+    # The same tag repeated should warn only once per load. Count the matching warnings
+    # directly rather than via a strict `@test_logs`, so unrelated records do not interfere.
+    # On Windows, `load_tag`'s retained file handle makes `mktempdir` emit a "cleanup" error.
+    tag_repeated = """
+    a: !<tag:example.org:mylib/quantity-1.0.0> 1
+    b: !<tag:example.org:mylib/quantity-1.0.0> 2
+    """
+    logs, _ = Test.collect_test_logs() do
+        load_tag(tag_repeated; extensions = true)
+    end
+    @test count(r -> occursin("Unrecognized tag", string(r.message)), logs) == 1
+end
+
+@testset "unknown tag roundtrip" begin
+    # Loading with `extensions = true`, writing back out, and reloading must preserve both the
+    # unrecognized tag and the value, for mappings, sequences, and scalars alike.
+    tag_unknown_all = """
+    mapping_node: !<tag:example.org:mylib/widget-1.0.0>
+      width: 42
+      height: 7
+    sequence_node: !<tag:example.org:mylib/series-1.0.0>
+      - alpha
+      - beta
+    scalar_node: !<tag:example.org:mylib/quantity-1.0.0> 3.14
+    empty_mapping: !<tag:example.org:mylib/widget-1.0.0> {}
+    empty_sequence: !<tag:example.org:mylib/series-1.0.0> []
+    """
+
+    af = load_tag(tag_unknown_all; extensions = true)
+    af2 = mktempdir() do dir
+        path = joinpath(dir, "roundtrip.asdf")
+        ASDF.write_file(path, af.metadata)
+        ASDF.load_file(path; extensions = true)
+    end
+    md = af2.metadata
+
+    @test md["mapping_node"] isa ASDF.TaggedMapping
+    @test md["mapping_node"].tag == "tag:example.org:mylib/widget-1.0.0"
+    @test md["mapping_node"]["width"] == 42
+    @test md["mapping_node"]["height"] == 7
+
+    @test md["sequence_node"] isa ASDF.TaggedSequence
+    @test md["sequence_node"].tag == "tag:example.org:mylib/series-1.0.0"
+    @test md["sequence_node"][1] == "alpha"
+    @test md["sequence_node"][2] == "beta"
+
+    @test md["scalar_node"] isa ASDF.TaggedScalar
+    @test md["scalar_node"].tag == "tag:example.org:mylib/quantity-1.0.0"
+    @test md["scalar_node"] == "3.14"
+
+    # Empty tagged collections must survive too (they serialize inline as `{}` / `[]`).
+    @test md["empty_mapping"] isa ASDF.TaggedMapping
+    @test md["empty_mapping"].tag == "tag:example.org:mylib/widget-1.0.0"
+    @test isempty(md["empty_mapping"])
+
+    @test md["empty_sequence"] isa ASDF.TaggedSequence
+    @test md["empty_sequence"].tag == "tag:example.org:mylib/series-1.0.0"
+    @test isempty(md["empty_sequence"])
+end

test/test-write.jl

@@ -58,3 +58,23 @@ end
     @test ASDF.native2big_U8(0x05) == [0x05]
     @test ASDF.native2big_U8(5) == [0x05]
 end
+
+@testset "write preserves key order for non-OrderedDict documents" begin
+    dir = mktempdir(; cleanup = true)
+    path = joinpath(dir, "order.asdf")
+
+    # A `TaggedMapping` top-level document is an `AbstractDict` that is *not* an `OrderedDict`.
+    # The old `merge(document, ...)` degraded such inputs to an unordered `Dict`, scrambling key
+    # order; `write_file` now rebuilds an `OrderedDict` so insertion order is preserved. Use a
+    # deliberately non-alphabetical order so hash ordering would not match by coincidence.
+    doc = ASDF.TaggedMapping(
+        "tag:example.org:mylib/root-1.0.0",
+        ASDF.OrderedDict{Any, Any}("zebra" => 1, "apple" => 2, "mango" => 3),
+    )
+
+    ASDF.write_file(path, doc)
+    reloaded = ASDF.load_file(path)
+
+    # User keys keep their insertion order; the auto-inserted provenance entry comes last.
+    @test collect(keys(reloaded.metadata)) == ["zebra", "apple", "mango", "asdf_library"]
+end