From 562876bdd3659eee707cd9496935abbe5dfa920b Mon Sep 17 00:00:00 2001
From: Huang-Huang Bao <eh5@sokka.cn>
Date: Sun, 5 Nov 2023 21:37:52 +0800
Subject: [PATCH] =?UTF-8?q?=F0=9F=90=9B=20zb:=20use=20the=20correct=20trav?=
 =?UTF-8?q?ersal=20order=20for=20node=20tree=20introspection?=
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

The previous implementation is just incomplete and would cause all
child nodes to be concated one by one instead of being enclosed.

Also updated the e2e tests to test this case.

Fixes 916b4cce00a0b85cb6e9a19695f9f1387832682e
---
 zbus/src/object_server/mod.rs | 86 +++++++++++++++++++++++------------
 zbus/tests/e2e.rs             | 23 ++++++++++
 2 files changed, 81 insertions(+), 28 deletions(-)

diff --git a/zbus/src/object_server/mod.rs b/zbus/src/object_server/mod.rs
index 85c6aab7a..cbd3dc03c 100644
--- a/zbus/src/object_server/mod.rs
+++ b/zbus/src/object_server/mod.rs
@@ -3,7 +3,7 @@
 use event_listener::{Event, EventListener};
 use serde::Serialize;
 use std::{
-    collections::{hash_map::Entry, HashMap, VecDeque},
+    collections::{hash_map::Entry, HashMap},
     fmt::Write,
     marker::PhantomData,
     ops::{Deref, DerefMut},
@@ -298,38 +298,68 @@ impl Node {
     }
 
     async fn introspect_to_writer<W: Write + Send>(&self, writer: &mut W) {
-        let mut node_list = VecDeque::new();
-        node_list.push_back((self, "", 0));
-        while let Some((node, path, level)) = node_list.pop_front() {
-            if level == 0 {
-                writeln!(
-                    writer,
-                    r#"
+        enum Fragment<'a> {
+            /// Represent an unclosed node tree, could be further splitted into sub-`Fragment`s
+            Node {
+                name: &'a str,
+                node: &'a Node,
+                level: usize,
+            },
+            /// Represent a closing `</node>`
+            End { level: usize },
+        }
+
+        let mut stack = Vec::new();
+        stack.push(Fragment::Node {
+            name: "",
+            node: self,
+            level: 0,
+        });
+
+        // This can be seen as traversing the fragment tree in pre-order DFS with formatted XML
+        // fragment, splitted `Fragment::Node`s and `Fragment::End` being current node, left
+        // subtree and right leaf respectively.
+        while let Some(fragment) = stack.pop() {
+            match fragment {
+                Fragment::Node { name, node, level } => {
+                    stack.push(Fragment::End { level });
+
+                    for (name, node) in &node.children {
+                        stack.push(Fragment::Node {
+                            name,
+                            node,
+                            level: level + 2,
+                        })
+                    }
+
+                    if level == 0 {
+                        writeln!(
+                            writer,
+                            r#"
 <!DOCTYPE node PUBLIC "-//freedesktop//DTD D-BUS Object Introspection 1.0//EN"
  "http://www.freedesktop.org/standards/dbus/1.0/introspect.dtd">
 <node>"#
-                )
-                .unwrap();
-            } else {
-                writeln!(
-                    writer,
-                    "{:indent$}<node name=\"{}\">",
-                    "",
-                    path,
-                    indent = level
-                )
-                .unwrap();
-            }
-
-            for iface in node.interfaces.values() {
-                iface.read().await.introspect_to_writer(writer, level + 2);
-            }
+                        )
+                        .unwrap();
+                    } else {
+                        writeln!(
+                            writer,
+                            "{:indent$}<node name=\"{}\">",
+                            "",
+                            name,
+                            indent = level
+                        )
+                        .unwrap();
+                    }
 
-            for (path, node) in &node.children {
-                node_list.push_front((node, path, level + 2));
+                    for iface in node.interfaces.values() {
+                        iface.read().await.introspect_to_writer(writer, level + 2);
+                    }
+                }
+                Fragment::End { level } => {
+                    writeln!(writer, "{:indent$}</node>", "", indent = level).unwrap();
+                }
             }
-
-            writeln!(writer, "{:indent$}</node>", "", indent = level).unwrap();
         }
     }
 
diff --git a/zbus/tests/e2e.rs b/zbus/tests/e2e.rs
index e2b692c11..faf74339d 100644
--- a/zbus/tests/e2e.rs
+++ b/zbus/tests/e2e.rs
@@ -473,6 +473,29 @@ async fn my_iface_test(conn: Connection, event: Event) -> zbus::Result<u32> {
     }
     drop(stream);
 
+    let root_introspect_proxy = zbus::fdo::IntrospectableProxy::builder(&conn)
+        .destination("org.freedesktop.MyService")?
+        .path("/")?
+        .build()
+        .await?;
+    debug!("Created: {:?}", root_introspect_proxy);
+
+    let root_xml = root_introspect_proxy.introspect().await?;
+    let root_node = zbus_xml::Node::from_reader(root_xml.as_bytes())
+        .map_err(|e| Error::Failure(e.to_string()))?;
+    let mut node = &root_node;
+    for name in ["org", "freedesktop", "MyService"] {
+        node = node
+            .nodes()
+            .iter()
+            .find(|&n| n.name().is_some_and(|n| n == name))
+            .expect("Child node not exist");
+    }
+    assert!(node
+        .interfaces()
+        .iter()
+        .any(|i| i.name() == "org.freedesktop.MyIface"));
+
     let proxy = MyIfaceProxy::builder(&conn)
         .destination("org.freedesktop.MyService")?
         .path("/org/freedesktop/MyService")?