Skip to content

fix: preserve original scalar text for null nodes so as<string> returns the source text#1460

Closed
mvanhorn wants to merge 2 commits into
jbeder:masterfrom
mvanhorn:fix/1290-preserve-null-scalar-text
Closed

fix: preserve original scalar text for null nodes so as<string> returns the source text#1460
mvanhorn wants to merge 2 commits into
jbeder:masterfrom
mvanhorn:fix/1290-preserve-null-scalar-text

Conversation

@mvanhorn

Copy link
Copy Markdown

Summary

Plumb the original token text through the null path without breaking the public EventHandler API: add a non-pure virtual OnNull(const Mark&, anchor_t, const std::string& value) overload to include/yaml-cpp/eventhandler.h whose default implementation delegates to the existing two-arg OnNull, so third-party handlers and the emitter/graphbuilder adapters keep working unchanged. SingleDocParser::HandleNode passes token.value at the null-plain-scalar site; NodeBuilder overrides the three-arg OnNull and calls a new node_data::set_null(const std::string& scalar) overload (include/yaml-cpp/node/detail/node_data.h + src/node_data.cpp) that sets m_type = NodeType::Null while storing the source text in m_scalar, exposed via the existing Scalar() accessor. Finally, the two as_if<std::string> specializations in include/yaml-cpp/node/impl.h return node.Scalar() for Null nodes instead of the literal "null", so key: yields "", key: ~ yields "~", and key: null yields "null", while IsNull() stays true for all of them.

Why this matters

Since yaml-cpp 0.8.0, any null-like plain scalar (<empty>, ~, null, Null, NULL) loses its original text: SingleDocParser::HandleNode (src/singledocparser.cpp, ~line 101) detects IsNullString(token.value...) and emits OnNull(mark, anchor), discarding token.value. NodeBuilder::OnNull then calls node.set_null(), which leaves m_scalar empty, and as_if<std::string> in include/yaml-cpp/node/impl.h (~lines 119 and 151) hard-codes the return value "null" for NodeType::Null. The reporter cannot distinguish message: (empty) from message: null because both Scalar() and as<std::string>() return fabricated values. Owner jbeder agreed the original text representation should be kept in Scalar() and returned by as<std::string>() ("Yeah that's reasonable").

See #1290.

Testing

  • Happy path: load message: null and assert IsNull() is true, Scalar() == "null", and as<std::string>() == "null" (test/integration/load_node_test.cpp). - Variants: key: yields "", key: ~ yields "~", key: Null / key: NULL yield their exact source text; quoted "null" remains a plain string scalar unaffected. - Edge cases: flow-map and block-seq missing values (structural nulls with no token) return "" from as<std::string>() and stay IsNull(); a default-constructed Node() still reports Null with empty scalar (test/node/node_test.cpp). - Error paths: as<std::string>() on a map/sequence still throws TypedBadConversion; as<std::string>(fallback) on non-scalar non-null still returns the fallback.

Fixes #1290

Five-line follow-up per bot review; full suite green.
@mvanhorn

Copy link
Copy Markdown
Author

Addressed in 30eade1. Full CTest suite (1029 tests) passes locally.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Empty strings are silently replaced with 'null' now

1 participant