[P1] Use semantic key identity in borrowed Node merges

src/de.rs

@@ -23,7 +23,9 @@ use crate::{
     Error, ErrorPathSegment, Mapping, Node, NodeValue, Number, Span, Tag, TaggedValue, Value,
     ast::MergePolicy,
     error::utf8_error_span,
-    key_identity::{DuplicateKeyTracker, check_duplicate_with_tracker_at_depth_limit},
+    key_identity::{
+        DuplicateKeyTracker, check_duplicate_with_tracker_at_depth_limit, same_key_identity,
+    },
     schema::{DEFAULT_MAX_NESTING_DEPTH, LoadOptions},
     yaml11,
 };
@@ -551,7 +553,7 @@ fn merged_node_ref_entries_at_depth(
         .filter_map(|(index, (key, value))| (index != merge_index).then_some((key, value)))
         .collect::<Vec<_>>();
     let merge_entries = node_ref_merge_entries(&entries[merge_index].1, depth)?;
-    insert_missing_node_ref_entries(&mut merged, merge_entries);
+    insert_missing_node_ref_entries(&mut merged, merge_entries)?;
     Ok(Some(merged))
 }
 
@@ -575,7 +577,7 @@ fn node_ref_merge_entries(merge: &Node, depth: usize) -> Result<Vec<(&Node, &Nod
                             .unwrap_or_else(|| {
                                 mapping.iter().map(|(key, value)| (key, value)).collect()
                             });
-                        insert_missing_node_ref_entries(&mut entries, merge_entries);
+                        insert_missing_node_ref_entries(&mut entries, merge_entries)?;
                     }
                     NodeValue::Sequence(_) => {
                         return Err(value_merge_error(
@@ -604,12 +606,20 @@ fn node_ref_merge_entries(merge: &Node, depth: usize) -> Result<Vec<(&Node, &Nod
 fn insert_missing_node_ref_entries<'a>(
     entries: &mut Vec<(&'a Node, &'a Node)>,
     merge_entries: Vec<(&'a Node, &'a Node)>,
-) {
+) -> Result<(), Error> {
     for (key, value) in merge_entries {
-        if entries.iter().all(|(existing_key, _)| *existing_key != key) {
+        let mut already_present = false;
+        for (existing_key, _) in entries.iter() {
+            if same_key_identity(existing_key, key)? {
+                already_present = true;
+                break;
+            }
+        }
+        if !already_present {
             entries.push((key, value));
         }
     }
+    Ok(())
 }
 
 fn apply_default_value_merges(mut value: Value) -> Result<Value, Error> {

tests/serde_value_api.rs

@@ -271,6 +271,14 @@ fn unsigned_node(value: u128) -> Node {
     Node::new(NodeValue::Number(Number::Unsigned(value)), Span::default())
 }
 
+fn string_node_at(value: &str, span: Span) -> Node {
+    Node::new(NodeValue::String(value.to_string()), span)
+}
+
+fn unsigned_node_at(value: u128, span: Span) -> Node {
+    Node::new(NodeValue::Number(Number::Unsigned(value)), span)
+}
+
 fn mapping_node(entries: Vec<(Node, Node)>) -> Node {
     Node::new(NodeValue::Mapping(entries), Span::default())
 }
@@ -291,6 +299,38 @@ fn caller_built_merge_node() -> Node {
     mapping_node(vec![(string_node("job"), job)])
 }
 
+fn caller_built_merge_node_with_keys(source_key: Node, local_key: Node) -> Node {
+    let defaults = mapping_node(vec![(source_key, string_node("from-merge"))]);
+    let job = mapping_node(vec![
+        (string_node("<<"), defaults),
+        (local_key, string_node("local")),
+    ]);
+    mapping_node(vec![(string_node("job"), job)])
+}
+
+fn assert_borrowed_node_merge_key_identity(source_key: Node, local_key: Node, expected_key: Value) {
+    assert_ne!(
+        source_key, local_key,
+        "regression keys must differ as complete Nodes"
+    );
+    let node = caller_built_merge_node_with_keys(source_key, local_key);
+
+    let from_node: Value = saneyaml::from_node(&node).expect("from_node expands merge");
+    let borrowed: Value = Value::deserialize(&node).expect("&Node expands merge");
+    let owned: Value = Value::deserialize(node.clone()).expect("owned Node expands merge");
+
+    for value in [&from_node, &borrowed, &owned] {
+        let mapping = value["job"].as_mapping().expect("merged job mapping");
+        assert_eq!(mapping.len(), 1, "semantic duplicate must be omitted");
+        let (key, value) = mapping.iter().next().expect("local merged entry");
+        assert_eq!(key, &expected_key, "the explicit key must be retained");
+        assert_eq!(value.as_str(), Some("local"), "local value wins the merge");
+    }
+
+    assert_eq!(from_node, borrowed);
+    assert_eq!(borrowed, owned);
+}
+
 fn caller_built_enum_merge_value() -> Value {
     let mut action_payload = Mapping::new();
     action_payload.insert(Value::from("run"), Value::from("cargo test"));
@@ -5719,6 +5759,50 @@ fn serde_api_caller_built_node_deserializers_expand_merge_keys_by_default() {
     }
 }
 
+#[test]
+fn serde_api_borrowed_node_merge_uses_semantic_key_identity() {
+    assert_borrowed_node_merge_key_identity(
+        string_node_at("shared", Span::new(10, 16, 2, 3)),
+        string_node_at("shared", Span::new(30, 36, 4, 3)),
+        Value::from("shared"),
+    );
+
+    assert_borrowed_node_merge_key_identity(
+        Node::new(
+            NodeValue::Number(Number::Integer(7)),
+            Span::new(40, 41, 5, 3),
+        ),
+        unsigned_node_at(7, Span::new(50, 51, 6, 3)),
+        Value::from(7u64),
+    );
+
+    let source_key = mapping_node(vec![
+        (
+            string_node_at("a", Span::new(60, 61, 7, 4)),
+            unsigned_node_at(1, Span::new(62, 63, 7, 6)),
+        ),
+        (
+            string_node_at("b", Span::new(64, 65, 7, 8)),
+            unsigned_node_at(2, Span::new(66, 67, 7, 10)),
+        ),
+    ]);
+    let local_key = mapping_node(vec![
+        (
+            string_node_at("b", Span::new(70, 71, 8, 4)),
+            unsigned_node_at(2, Span::new(72, 73, 8, 6)),
+        ),
+        (
+            string_node_at("a", Span::new(74, 75, 8, 8)),
+            unsigned_node_at(1, Span::new(76, 77, 8, 10)),
+        ),
+    ]);
+    let mut expected_key = Mapping::new();
+    expected_key.insert(Value::from("b"), Value::from(2u64));
+    expected_key.insert(Value::from("a"), Value::from(1u64));
+
+    assert_borrowed_node_merge_key_identity(source_key, local_key, Value::Mapping(expected_key));
+}
+
 /// Builds a `Value` mapping whose `<<` merge source nests `depth` levels deep.
 /// Each level carries a unique sibling key so the merge resolves to a single
 /// flattened mapping, exercising real merge expansion at the requested depth.