diff --git a/rust/src/dns/set/mod.rs b/rust/src/dns/set/mod.rs index 544d029..e2d315f 100644 --- a/rust/src/dns/set/mod.rs +++ b/rust/src/dns/set/mod.rs @@ -8,8 +8,8 @@ use crate::scopes::DOMAINS_DNS_UPDATE; use super::records::verify_with_list_action; use super::records::{ - RecordOptions, RecordWriteArgs, fetch_records, validate_caa_fields, validate_svcb_fields, - validate_tlsa_fields, + RecordOptions, RecordWriteArgs, fetch_records, record_value, validate_caa_fields, + validate_svcb_fields, validate_tlsa_fields, }; mod outcome; @@ -140,12 +140,20 @@ pub(super) fn command() -> RuntimeCommandSpec { .with_fix(fix) .into_cli_error()); } - let existing_ids: Vec = existing + // (record_id, current value) per existing record — `plan_set` pairs a + // desired value with the record already holding it before falling back + // to position, so a retained value is never re-created. + let existing_pairs: Vec<(String, String)> = existing .iter() - .filter_map(|r| r.record_id.clone()) + .filter_map(|r| { + Some(( + r.record_id.clone()?, + record_value(r).unwrap_or_default().to_owned(), + )) + }) .collect(); - let plan = plan_set(&existing_ids, &data); + let plan = plan_set(&existing_pairs, &data); if ctx.dry_run() { return Ok(CommandResult::new(dry_run_set_preview( &domain, diff --git a/rust/src/dns/set/outcome.rs b/rust/src/dns/set/outcome.rs index 31bc99d..80cd896 100644 --- a/rust/src/dns/set/outcome.rs +++ b/rust/src/dns/set/outcome.rs @@ -206,7 +206,11 @@ mod tests { #[test] fn dry_run_set_preview_matches_the_plan_it_would_execute() { let plan = plan_set( - &["r1".to_string(), "r2".to_string(), "r3".to_string()], + &[ + ("r1".to_string(), "1.1.1.1".to_string()), + ("r2".to_string(), "2.2.2.2".to_string()), + ("r3".to_string(), "3.3.3.3".to_string()), + ], &["9.9.9.9".to_string(), "8.8.8.8".to_string()], ); let preview = dry_run_set_preview("example.com", "A", "www", &plan); @@ -225,7 +229,10 @@ mod tests { /// catch, since it never goes through the projection). #[test] fn dry_run_set_preview_survives_default_field_projection() { - let plan = plan_set(&["r1".to_string()], &["9.9.9.9".to_string()]); + let plan = plan_set( + &[("r1".to_string(), "1.1.1.1".to_string())], + &["9.9.9.9".to_string()], + ); let preview = dry_run_set_preview("example.com", "A", "www", &plan); let default_fields = "domain,type,name,replaced,created,deleted,action,plan"; let projected = cli_engine::output::filter_fields(&preview, default_fields); @@ -245,7 +252,7 @@ mod tests { #[test] fn dry_run_set_preview_renders_plan_as_a_nested_table() { let plan = plan_set( - &["r1".to_string()], + &[("r1".to_string(), "1.1.1.1".to_string())], &["9.9.9.9".to_string(), "8.8.8.8".to_string()], ); let preview = dry_run_set_preview("example.com", "A", "www", &plan); diff --git a/rust/src/dns/set/plan.rs b/rust/src/dns/set/plan.rs index 5a1a695..e2ea72c 100644 --- a/rust/src/dns/set/plan.rs +++ b/rust/src/dns/set/plan.rs @@ -1,4 +1,4 @@ -//! Reconcile planning for `dns set` — deciding, from the existing record ids +//! Reconcile planning for `dns set` — deciding, from the existing records //! and desired `--data` values, which per-record ops to run. /// One reconcile action for `dns set` over v3's per-record endpoints. @@ -20,27 +20,63 @@ pub(super) enum SetAction { Create { data: String }, } -/// Reconcile the existing records for a type+name (their ids, in list order) with -/// the desired `--data` values: pair up the overlap (`Replace`), `Delete` -/// the surplus existing, `Create` the surplus desired. Pure so the plan — the -/// least-destructive way to emulate a set-replace over per-record v3 ops — is -/// unit-testable. -pub(super) fn plan_set(existing_ids: &[String], desired: &[String]) -> Vec { - let overlap = existing_ids.len().min(desired.len()); - let mut actions = Vec::with_capacity(existing_ids.len().max(desired.len())); - for i in 0..overlap { - actions.push(SetAction::Replace { - record_id: existing_ids[i].clone(), - data: desired[i].clone(), - }); +/// Reconcile the existing records for a type+name — `(record_id, current value)` +/// in list order — with the desired `--data` values: pair them up (`Replace`), +/// `Delete` the surplus existing, `Create` the surplus desired. Pure so the +/// plan — the least-destructive way to emulate a set-replace over per-record v3 +/// ops — is unit-testable. +/// +/// A desired value that some existing record already holds is paired with *that* +/// record before anything is paired by position. v3 keys record identity on +/// (name, type, data), so creating a value another record still holds fails with +/// `DUPLICATE_RECORD`. Under a purely positional pairing that is not a harmless +/// no-op: [`super::write::apply_replace`] keeps the old record when its create +/// fails, while the surplus `Delete` later in the plan still runs — so narrowing +/// `www A {1.2.3.4, 5.6.7.8}` to just `5.6.7.8` would delete `5.6.7.8` and leave +/// `1.2.3.4` behind, the exact inverse of what was asked. Pairing retained values +/// first means a value that stays in the set is never re-created. +pub(super) fn plan_set(existing: &[(String, String)], desired: &[String]) -> Vec { + let mut paired: Vec> = vec![None; desired.len()]; + let mut taken = vec![false; existing.len()]; + + // Pass 1: pair by content, wherever the holder sits in the list. + for (d, want) in desired.iter().enumerate() { + if let Some(e) = (0..existing.len()).find(|&e| !taken[e] && &existing[e].1 == want) { + taken[e] = true; + paired[d] = Some(e); + } + } + + // Pass 2: whatever is left pairs by position, as before. + let mut free: Vec = (0..existing.len()).filter(|&e| !taken[e]).rev().collect(); + for slot in paired.iter_mut().filter(|s| s.is_none()) { + let Some(e) = free.pop() else { break }; + taken[e] = true; + *slot = Some(e); } - for id in &existing_ids[overlap..] { - actions.push(SetAction::Delete { - record_id: id.clone(), - }); + + let mut actions = Vec::with_capacity(existing.len().max(desired.len())); + for (d, want) in desired.iter().enumerate() { + if let Some(e) = paired[d] { + actions.push(SetAction::Replace { + record_id: existing[e].0.clone(), + data: want.clone(), + }); + } + } + // Deletes stay ahead of creates: freeing a surplus record's value is what + // lets a create of that same value succeed. + for (e, (id, _)) in existing.iter().enumerate() { + if !taken[e] { + actions.push(SetAction::Delete { + record_id: id.clone(), + }); + } } - for d in &desired[overlap..] { - actions.push(SetAction::Create { data: d.clone() }); + for (d, want) in desired.iter().enumerate() { + if paired[d].is_none() { + actions.push(SetAction::Create { data: want.clone() }); + } } actions } @@ -49,10 +85,18 @@ pub(super) fn plan_set(existing_ids: &[String], desired: &[String]) -> Vec Vec<(String, String)> { + pairs + .iter() + .map(|(id, value)| ((*id).to_string(), (*value).to_string())) + .collect() + } + #[test] fn plan_set_reuses_overlap_deletes_extra_creates_shortfall() { - // 3 existing, 2 desired → reuse 2 ids, delete the 3rd. - let existing = vec!["r1".to_string(), "r2".to_string(), "r3".to_string()]; + // 3 existing, 2 desired, no value in common → reuse 2 ids, delete the 3rd. + let existing = ids(&[("r1", "1.1.1.1"), ("r2", "2.2.2.2"), ("r3", "3.3.3.3")]); let desired = vec!["9.9.9.9".to_string(), "8.8.8.8".to_string()]; assert_eq!( plan_set(&existing, &desired), @@ -73,7 +117,7 @@ mod tests { // 1 existing, 3 desired → reuse 1 id, create 2. assert_eq!( plan_set( - &["r1".to_string()], + &ids(&[("r1", "z")]), &["a".to_string(), "b".to_string(), "c".to_string()] ), vec![ @@ -91,4 +135,87 @@ mod tests { vec![SetAction::Create { data: "x".into() }] ); } + + #[test] + fn plan_set_pairs_a_retained_value_with_the_record_that_holds_it() { + // Narrowing `www A {1.2.3.4, 5.6.7.8}` down to just 5.6.7.8 must pair the + // desired value with r2, which already holds it, and delete r1. Pairing + // positionally instead (Replace{r1 → 5.6.7.8}, Delete{r2}) makes the + // create collide with r2's still-live duplicate, so `apply_replace` keeps + // r1 while the surplus delete still removes r2 — leaving the zone holding + // exactly the value the user asked to drop. + let existing = ids(&[("r1", "1.2.3.4"), ("r2", "5.6.7.8")]); + assert_eq!( + plan_set(&existing, &["5.6.7.8".to_string()]), + vec![ + SetAction::Replace { + record_id: "r2".into(), + data: "5.6.7.8".into() + }, + SetAction::Delete { + record_id: "r1".into() + }, + ] + ); + } + + #[test] + fn plan_set_reordering_the_same_values_pairs_each_to_itself() { + // A pure reorder is a no-op set. Every pairing must land on the record + // that already holds the value, so `apply_replace` short-circuits each + // one instead of issuing two creates that both fail as duplicates. + let existing = ids(&[("r1", "1.2.3.4"), ("r2", "5.6.7.8")]); + assert_eq!( + plan_set(&existing, &["5.6.7.8".to_string(), "1.2.3.4".to_string()]), + vec![ + SetAction::Replace { + record_id: "r2".into(), + data: "5.6.7.8".into() + }, + SetAction::Replace { + record_id: "r1".into(), + data: "1.2.3.4".into() + }, + ] + ); + } + + #[test] + fn plan_set_keeps_a_retained_value_and_still_replaces_the_rest() { + // Mixed case: 5.6.7.8 stays (pair with r2), 1.2.3.4 goes, 9.9.9.9 is new. + // The freed id r1 is reused positionally rather than deleted-and-created. + let existing = ids(&[("r1", "1.2.3.4"), ("r2", "5.6.7.8")]); + assert_eq!( + plan_set(&existing, &["5.6.7.8".to_string(), "9.9.9.9".to_string()]), + vec![ + SetAction::Replace { + record_id: "r2".into(), + data: "5.6.7.8".into() + }, + SetAction::Replace { + record_id: "r1".into(), + data: "9.9.9.9".into() + }, + ] + ); + } + + #[test] + fn plan_set_duplicate_desired_values_pair_distinct_records() { + // Two identical --data values must not both pair with the same record. + let existing = ids(&[("r1", "1.2.3.4"), ("r2", "1.2.3.4")]); + assert_eq!( + plan_set(&existing, &["1.2.3.4".to_string(), "1.2.3.4".to_string()]), + vec![ + SetAction::Replace { + record_id: "r1".into(), + data: "1.2.3.4".into() + }, + SetAction::Replace { + record_id: "r2".into(), + data: "1.2.3.4".into() + }, + ] + ); + } }