diff --git a/src/rust/lqos_topology/src/lib.rs b/src/rust/lqos_topology/src/lib.rs index 8b6f2557..7f494a89 100644 --- a/src/rust/lqos_topology/src/lib.rs +++ b/src/rust/lqos_topology/src/lib.rs @@ -2485,11 +2485,14 @@ fn apply_effective_topology_reparenting_only( == effective_node.effective_attachment_id.as_deref() && already_anchored { - ensure_attachment_node_exists( - &mut out, - &selected_parent.parent_node_id, - ¤t_anchor_attachment, - ); + if should_anchor_reparent_under_attachment(&ui_node.node_id, ¤t_anchor_attachment) + { + ensure_attachment_node_exists( + &mut out, + &selected_parent.parent_node_id, + ¤t_anchor_attachment, + ); + } continue; } @@ -2497,17 +2500,26 @@ fn apply_effective_topology_reparenting_only( continue; }; let anchor_attachment = attachment_anchor_for_reparent(&node_value, target_attachment); - ensure_attachment_node_exists( - &mut out, - &selected_parent.parent_node_id, - &anchor_attachment, - ); - let _ = insert_node_under_parent_id( - &mut out, - &anchor_attachment.attachment_id, - &node_key, - node_value, - ); + if should_anchor_reparent_under_attachment(&ui_node.node_id, &anchor_attachment) { + ensure_attachment_node_exists( + &mut out, + &selected_parent.parent_node_id, + &anchor_attachment, + ); + let _ = insert_node_under_parent_id( + &mut out, + &anchor_attachment.attachment_id, + &node_key, + node_value, + ); + } else { + let _ = insert_node_under_parent_id( + &mut out, + &selected_parent.parent_node_id, + &node_key, + node_value, + ); + } } Value::Object(out) @@ -2798,6 +2810,13 @@ fn attachment_anchor_for_reparent( anchor } +fn should_anchor_reparent_under_attachment( + node_id: &str, + attachment: &TopologyAttachmentOption, +) -> bool { + attachment.attachment_id != node_id +} + fn update_node_bandwidths_by_id( root: &mut Map, node_id: &str, @@ -4004,7 +4023,9 @@ mod tests { } fn write_runtime_json_fixture(path: PathBuf, value: &Value, label: &str) { - let parent = path.parent().expect("runtime fixture path should have parent"); + let parent = path + .parent() + .expect("runtime fixture path should have parent"); fs::create_dir_all(parent).expect("runtime fixture parent should be creatable"); fs::write( &path, @@ -6499,6 +6520,140 @@ mod tests { assert!(beta_children.get("Beta - Alpha 60").is_some()); } + #[test] + fn site_reparenting_does_not_create_self_anchored_duplicate_site() { + let canonical = json!({ + "David Spence": { + "children": { + "Howard Loewen": { + "children": { + "Howard AP": { + "children": {}, + "downloadBandwidthMbps": 300, + "id": "howard-ap", + "name": "Howard AP", + "parent_site": "Howard Loewen", + "type": "AP", + "uploadBandwidthMbps": 300 + } + }, + "downloadBandwidthMbps": 500, + "id": "howard-site", + "name": "Howard Loewen", + "parent_site": "David Spence", + "type": "Site", + "uploadBandwidthMbps": 500 + } + }, + "downloadBandwidthMbps": 800, + "id": "david-site", + "name": "David Spence", + "type": "Site", + "uploadBandwidthMbps": 800 + } + }); + + let ui_state = TopologyEditorStateFile { + schema_version: 1, + source: "uisp/full2".to_string(), + generated_unix: None, + ingress_identity: None, + nodes: vec![TopologyEditorNode { + node_id: "howard-site".to_string(), + node_name: "Howard Loewen".to_string(), + latitude: None, + longitude: None, + current_parent_node_id: Some("david-site".to_string()), + current_parent_node_name: Some("David Spence".to_string()), + current_attachment_id: Some("howard-site".to_string()), + current_attachment_name: Some("Howard Loewen".to_string()), + can_move: true, + allowed_parents: vec![TopologyAllowedParent { + parent_node_id: "david-site".to_string(), + parent_node_name: "David Spence".to_string(), + attachment_options: vec![TopologyAttachmentOption { + attachment_id: "howard-site".to_string(), + attachment_name: "Howard Loewen".to_string(), + attachment_kind: "site".to_string(), + attachment_role: TopologyAttachmentRole::Unknown, + pair_id: None, + peer_attachment_id: None, + peer_attachment_name: None, + capacity_mbps: Some(500), + download_bandwidth_mbps: Some(500), + upload_bandwidth_mbps: Some(500), + transport_cap_mbps: None, + transport_cap_reason: None, + rate_source: TopologyAttachmentRateSource::Static, + can_override_rate: false, + rate_override_disabled_reason: None, + has_rate_override: false, + local_probe_ip: None, + remote_probe_ip: None, + probe_enabled: false, + probeable: false, + health_status: TopologyAttachmentHealthStatus::Disabled, + health_reason: None, + suppressed_until_unix: None, + effective_selected: false, + }], + all_attachments_suppressed: false, + has_probe_unavailable_attachments: false, + }], + queue_visibility_policy: TopologyQueueVisibilityPolicy::QueueVisible, + preferred_attachment_id: None, + preferred_attachment_name: None, + effective_attachment_id: None, + effective_attachment_name: None, + }], + }; + + let effective = TopologyEffectiveStateFile { + schema_version: 1, + generated_unix: None, + canonical_generated_unix: None, + health_generated_unix: None, + nodes: vec![TopologyEffectiveNodeState { + node_id: "howard-site".to_string(), + logical_parent_node_id: "david-site".to_string(), + preferred_attachment_id: Some("howard-site".to_string()), + effective_attachment_id: Some("howard-site".to_string()), + fallback_reason: None, + all_attachments_suppressed: false, + attachments: vec![], + }], + }; + + let moved = apply_effective_topology_to_network_json( + &Config::default(), + &canonical, + &ui_state, + &effective, + ); + + let david_children = moved["David Spence"]["children"] + .as_object() + .expect("David Spence should keep children"); + let howard = david_children + .get("Howard Loewen") + .and_then(Value::as_object) + .expect("Howard Loewen should remain a direct child of David Spence"); + assert_eq!(howard["id"].as_str(), Some("howard-site")); + let howard_children = howard["children"] + .as_object() + .expect("Howard Loewen should keep its children"); + assert!(howard_children.get("Howard Loewen").is_none()); + assert!(howard_children.get("Howard AP").is_some()); + validate_effective_topology_network( + &Config::default(), + &canonical, + &ui_state, + &effective, + &moved, + ) + .expect("self-anchored site attachment should not duplicate the site in the export"); + } + #[test] fn duplicate_device_candidates_do_not_block_valid_site_override_publish() { let mut config = Config::default(); diff --git a/src/rust/uisp_integration/src/strategies/common.rs b/src/rust/uisp_integration/src/strategies/common.rs index 437d8ec7..30f0348d 100644 --- a/src/rust/uisp_integration/src/strategies/common.rs +++ b/src/rust/uisp_integration/src/strategies/common.rs @@ -243,6 +243,41 @@ fn dedup_raw_devices_by_id( (deduped_devices, deduped_json) } +pub(crate) fn dedup_raw_sites_by_id(sites_raw: Vec) -> Vec { + let original_len = sites_raw.len(); + let mut seen_ids = HashSet::::new(); + let mut duplicate_counts = HashMap::::new(); + let mut deduped_sites = Vec::with_capacity(original_len); + + for site in sites_raw { + let site_id = site.id.clone(); + if !seen_ids.insert(site_id.clone()) { + *duplicate_counts.entry(site_id).or_insert(0) += 1; + continue; + } + deduped_sites.push(site); + } + + if !duplicate_counts.is_empty() { + let duplicate_rows = duplicate_counts.values().sum::(); + let mut duplicate_ids = duplicate_counts.into_iter().collect::>(); + duplicate_ids.sort_unstable_by(|left, right| left.0.cmp(&right.0)); + let sample_ids = duplicate_ids + .iter() + .take(5) + .map(|(site_id, count)| format!("{site_id} (+{count})")) + .collect::>(); + warn!( + duplicate_site_ids = duplicate_ids.len(), + duplicate_site_rows = duplicate_rows, + sample = ?sample_ids, + "UISP returned duplicate site IDs; deduping by site ID before topology processing" + ); + } + + deduped_sites +} + impl UispData { pub(crate) fn from_parts( sites_raw: Vec, @@ -332,6 +367,7 @@ impl UispData { // Obtain the UISP data and transform it into easier to work with types let (mut sites_raw, devices_raw, data_links_raw, devices_as_json) = load_uisp_data(config.clone()).await?; + sites_raw = dedup_raw_sites_by_id(sites_raw); let (devices_raw, devices_as_json) = dedup_raw_devices_by_id(devices_raw, devices_as_json); // Normalize endpoint/customer names before deduplication and downstream parsing. @@ -551,7 +587,10 @@ impl UispData { #[cfg(test)] mod tests { - use super::{dedup_raw_devices_by_id, dedup_site_names, normalize_client_site_names}; + use super::{ + dedup_raw_devices_by_id, dedup_raw_sites_by_id, dedup_site_names, + normalize_client_site_names, + }; use serde_json::json; use uisp::{Device, Site}; @@ -718,4 +757,19 @@ mod tests { Some("Unique Name") ); } + + #[test] + fn duplicate_raw_sites_are_deduped_by_id() { + let first = mk_site("site-1", "First Name", None, None); + let duplicate = mk_site("site-1", "Second Name", None, None); + let unique = mk_site("site-2", "Unique Name", None, None); + + let sites = dedup_raw_sites_by_id(vec![first, duplicate, unique]); + + assert_eq!(sites.len(), 2); + assert_eq!(sites[0].id, "site-1"); + assert_eq!(sites[0].name_or_blank(), "First Name"); + assert_eq!(sites[1].id, "site-2"); + assert_eq!(sites[1].name_or_blank(), "Unique Name"); + } } diff --git a/src/rust/uisp_integration/src/strategies/flat.rs b/src/rust/uisp_integration/src/strategies/flat.rs index 2158b755..a9e496d8 100644 --- a/src/rust/uisp_integration/src/strategies/flat.rs +++ b/src/rust/uisp_integration/src/strategies/flat.rs @@ -2,7 +2,7 @@ use crate::blackboard_blob; use crate::errors::UispIntegrationError; use crate::ethernet_advisory::{apply_ethernet_rate_cap, write_ethernet_advisories}; use crate::ip_ranges::IpRanges; -use crate::strategies::common::dedup_site_names; +use crate::strategies::common::{dedup_raw_sites_by_id, dedup_site_names}; use crate::strategies::full::shaped_devices_writer::{ShapedDevice, write_circuit_anchors}; use crate::uisp_types::UispDevice; use lqos_config::{ @@ -32,11 +32,12 @@ pub async fn build_flat_network( error!("{e:?}"); UispIntegrationError::UispConnectError })?; - let mut sites = uisp::load_all_sites(config.clone()).await.map_err(|e| { + let sites = uisp::load_all_sites(config.clone()).await.map_err(|e| { error!("Unable to load device list from UISP"); error!("{e:?}"); UispIntegrationError::UispConnectError })?; + let mut sites = dedup_raw_sites_by_id(sites); let data_links = uisp::load_all_data_links(config.clone()) .await .map_err(|e| {