Merge pull request #1059 from LibreQoE/bridge_setup_2

Fix topology site export and dedupe UISP site IDs

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,
-                &current_anchor_attachment,
-            );
+            if should_anchor_reparent_under_attachment(&ui_node.node_id, &current_anchor_attachment)
+            {
+                ensure_attachment_node_exists(
+                    &mut out,
+                    &selected_parent.parent_node_id,
+                    &current_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<String, Value>,
     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();

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<Site>) -> Vec<Site> {
+    let original_len = sites_raw.len();
+    let mut seen_ids = HashSet::<String>::new();
+    let mut duplicate_counts = HashMap::<String, usize>::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::<usize>();
+        let mut duplicate_ids = duplicate_counts.into_iter().collect::<Vec<_>>();
+        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::<Vec<_>>();
+        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<Site>,
@@ -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");
+    }
 }

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| {