Unify return values from add_node() methods (#153)

## Description

This PR unifies the return values from the different methods for adding
server nodes. They now always return the node ID of the inserted node.

This also uses the more idiomatic `Option` for telling the server to
pick a random, unused node ID when no specific node ID is requested.

Co-authored-by: Uwe Klotz <uwe.klotz@gmail.com>

CHANGELOG.md

@@ -44,6 +44,8 @@ adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.html).
   `ua::BrowseDescription` instead of `ua::NodeId` for better control over the resulting references.
 - Breaking: Return typed variant `DataValue` instead of `ua::DataValue` from `AsyncClient` read
   operations.
+- Breaking: Adjust signatures of `Server::add_object_node()` and `Server::add_variable_node()` to
+  match the new methods, returning the inserted node IDs.
 
 ## [0.6.0-pre.5] - 2024-05-31
 

examples/server.rs

@@ -18,27 +18,25 @@ fn main() -> anyhow::Result<()> {
     println!("Adding server nodes");
 
     let object_node = ObjectNode {
-        requested_new_node_id: ua::NodeId::string(1, "the.folder"),
+        requested_new_node_id: Some(ua::NodeId::string(1, "the.folder")),
         parent_node_id: ua::NodeId::ns0(UA_NS0ID_OBJECTSFOLDER),
         reference_type_id: ua::NodeId::ns0(UA_NS0ID_ORGANIZES),
         browse_name: ua::QualifiedName::new(1, "the folder"),
         type_definition: ua::NodeId::ns0(UA_NS0ID_FOLDERTYPE),
         attributes: ua::ObjectAttributes::default(),
     };
+    let object_node_id = server.add_object_node(object_node)?;
 
-    let variable_node_id = ua::NodeId::string(1, "the.answer");
     let variable_node = VariableNode {
-        requested_new_node_id: variable_node_id.clone(),
-        parent_node_id: object_node.requested_new_node_id.clone(),
+        requested_new_node_id: Some(ua::NodeId::string(1, "the.answer")),
+        parent_node_id: object_node_id.clone(),
         reference_type_id: ua::NodeId::ns0(UA_NS0ID_ORGANIZES),
         browse_name: ua::QualifiedName::new(1, "the answer"),
         type_definition: ua::NodeId::ns0(UA_NS0ID_BASEDATAVARIABLETYPE),
         attributes: ua::VariableAttributes::default()
             .with_data_type(&ua::NodeId::ns0(UA_NS0ID_STRING)),
     };
-
-    server.add_object_node(object_node)?;
-    server.add_variable_node(variable_node)?;
+    let variable_node_id = server.add_variable_node(variable_node)?;
 
     server.write_variable_string(&variable_node_id, "foobar")?;
 

examples/server_data_source.rs

@@ -69,18 +69,20 @@ fn main() -> anyhow::Result<()> {
     println!("Adding server nodes");
 
     let object_node = ObjectNode {
-        requested_new_node_id: ua::NodeId::string(1, "the.folder"),
+        requested_new_node_id: Some(ua::NodeId::string(1, "the.folder")),
         parent_node_id: ua::NodeId::ns0(UA_NS0ID_OBJECTSFOLDER),
         reference_type_id: ua::NodeId::ns0(UA_NS0ID_ORGANIZES),
         browse_name: ua::QualifiedName::new(1, "the folder"),
         type_definition: ua::NodeId::ns0(UA_NS0ID_FOLDERTYPE),
         attributes: ua::ObjectAttributes::default(),
     };
+    let object_node_id = server
+        .add_object_node(object_node)
+        .context("add object node")?;
 
-    let variable_node_id = ua::NodeId::string(1, "the.answer");
     let variable_node = VariableNode {
-        requested_new_node_id: variable_node_id.clone(),
-        parent_node_id: object_node.requested_new_node_id.clone(),
+        requested_new_node_id: Some(ua::NodeId::string(1, "the.answer")),
+        parent_node_id: object_node_id.clone(),
         reference_type_id: ua::NodeId::ns0(UA_NS0ID_ORGANIZES),
         browse_name: ua::QualifiedName::new(1, "the answer"),
         type_definition: ua::NodeId::ns0(UA_NS0ID_BASEDATAVARIABLETYPE),
@@ -92,13 +94,8 @@ fn main() -> anyhow::Result<()> {
                     .with_current_write(true),
             ),
     };
-
     let data_source = DynamicDataSource::new("Lorem ipsum");
-
-    server
-        .add_object_node(object_node)
-        .context("add object node")?;
-    server
+    let variable_node_id = server
         .add_data_source_variable_node(variable_node, data_source)
         .context("add variable node")?;
 

examples/server_method_callback.rs

@@ -63,7 +63,7 @@ fn main() -> anyhow::Result<()> {
         .with_value_rank(-1);
 
     let method_node = MethodNode {
-        requested_new_node_id: ua::NodeId::numeric(1, 62541),
+        requested_new_node_id: Some(ua::NodeId::numeric(1, 62541)),
         parent_node_id: ua::NodeId::ns0(UA_NS0ID_OBJECTSFOLDER),
         reference_type_id: ua::NodeId::ns0(UA_NS0ID_HASCOMPONENT),
         browse_name: ua::QualifiedName::new(1, "hello world"),
@@ -76,7 +76,6 @@ fn main() -> anyhow::Result<()> {
         output_arguments: ua::Array::from_slice(&[output_argument]),
         output_arguments_requested_new_node_id: None,
     };
-
     let (method_node_id, _) = server.add_method_node(method_node, ExampleCallback {})?;
 
     // Start runner task that handles incoming connections (events).

src/server.rs

@@ -324,9 +324,12 @@ impl Server {
             context,
         } = node;
 
+        let requested_new_node_id = requested_new_node_id.unwrap_or(ua::NodeId::null());
+
         // This out variable must be initialized without memory allocation because the call below
         // overwrites it in place, without releasing any held data first.
         let mut out_new_node_id = ua::NodeId::null();
+
         let status_code = ua::StatusCode::new(unsafe {
             __UA_Server_addNode(
                 // SAFETY: Cast to `mut` pointer, function is marked `UA_THREADSAFE`.
@@ -346,104 +349,168 @@ impl Server {
             )
         });
         Error::verify_good(&status_code)?;
+
         Ok(out_new_node_id)
     }
 
     /// Adds object node to address space.
     ///
+    /// This returns the node ID that was actually inserted (when no explicit requested new node ID
+    /// was given in `node`).
+    ///
     /// # Errors
     ///
     /// This fails when the node cannot be added.
-    pub fn add_object_node(&self, object_node: ObjectNode) -> Result<()> {
+    pub fn add_object_node(&self, object_node: ObjectNode) -> Result<ua::NodeId> {
+        let ObjectNode {
+            requested_new_node_id,
+            parent_node_id,
+            reference_type_id,
+            browse_name,
+            type_definition,
+            attributes,
+        } = object_node;
+
+        let requested_new_node_id = requested_new_node_id.unwrap_or(ua::NodeId::null());
+
+        // This out variable must be initialized without memory allocation because the call below
+        // overwrites it in place, without releasing any held data first.
+        let mut out_new_node_id = ua::NodeId::null();
+
         let status_code = ua::StatusCode::new(unsafe {
             __UA_Server_addNode(
                 // SAFETY: Cast to `mut` pointer, function is marked `UA_THREADSAFE`.
                 self.0.as_ptr().cast_mut(),
                 // Passing ownership is trivial with primitive value (`u32`).
                 ua::NodeClass::OBJECT.into_raw(),
-                object_node.requested_new_node_id.as_ptr(),
-                object_node.parent_node_id.as_ptr(),
-                object_node.reference_type_id.as_ptr(),
+                requested_new_node_id.as_ptr(),
+                parent_node_id.as_ptr(),
+                reference_type_id.as_ptr(),
                 // TODO: Verify that `__UA_Server_addNode()` takes ownership.
-                object_node.browse_name.into_raw(),
-                object_node.type_definition.as_ptr(),
-                object_node.attributes.as_node_attributes().as_ptr(),
+                browse_name.into_raw(),
+                type_definition.as_ptr(),
+                attributes.as_node_attributes().as_ptr(),
                 ua::ObjectAttributes::data_type(),
                 ptr::null_mut(),
-                ptr::null_mut(),
+                out_new_node_id.as_mut_ptr(),
             )
         });
-        Error::verify_good(&status_code)
+        Error::verify_good(&status_code)?;
+
+        Ok(out_new_node_id)
     }
 
     /// Adds variable node to address space.
     ///
+    /// This returns the node ID that was actually inserted (when no explicit requested new node ID
+    /// was given in `node`).
+    ///
     /// # Errors
     ///
     /// This fails when the node cannot be added.
-    pub fn add_variable_node(&self, variable_node: VariableNode) -> Result<()> {
+    pub fn add_variable_node(&self, variable_node: VariableNode) -> Result<ua::NodeId> {
+        let VariableNode {
+            requested_new_node_id,
+            parent_node_id,
+            reference_type_id,
+            browse_name,
+            type_definition,
+            attributes,
+        } = variable_node;
+
+        let requested_new_node_id = requested_new_node_id.unwrap_or(ua::NodeId::null());
+
+        // This out variable must be initialized without memory allocation because the call below
+        // overwrites it in place, without releasing any held data first.
+        let mut out_new_node_id = ua::NodeId::null();
+
         let status_code = ua::StatusCode::new(unsafe {
             __UA_Server_addNode(
                 // SAFETY: Cast to `mut` pointer, function is marked `UA_THREADSAFE`.
                 self.0.as_ptr().cast_mut(),
                 // Passing ownership is trivial with primitive value (`u32`).
                 ua::NodeClass::VARIABLE.into_raw(),
-                variable_node.requested_new_node_id.as_ptr(),
-                variable_node.parent_node_id.as_ptr(),
-                variable_node.reference_type_id.as_ptr(),
+                requested_new_node_id.as_ptr(),
+                parent_node_id.as_ptr(),
+                reference_type_id.as_ptr(),
                 // TODO: Verify that `__UA_Server_addNode()` takes ownership.
-                variable_node.browse_name.into_raw(),
-                variable_node.type_definition.as_ptr(),
-                variable_node.attributes.as_node_attributes().as_ptr(),
+                browse_name.into_raw(),
+                type_definition.as_ptr(),
+                attributes.as_node_attributes().as_ptr(),
                 ua::VariableAttributes::data_type(),
                 ptr::null_mut(),
-                ptr::null_mut(),
+                out_new_node_id.as_mut_ptr(),
             )
         });
-        Error::verify_good(&status_code)
+        Error::verify_good(&status_code)?;
+
+        Ok(out_new_node_id)
     }
 
     /// Adds variable node with data source to address space.
     ///
+    /// This returns the node ID that was actually inserted (when no explicit requested new node ID
+    /// was given in `node`).
+    ///
     /// # Errors
     ///
     /// This fails when the node cannot be added.
     pub fn add_data_source_variable_node(
         &self,
         variable_node: VariableNode,
         data_source: impl DataSource + 'static,
-    ) -> Result<()> {
+    ) -> Result<ua::NodeId> {
+        let VariableNode {
+            requested_new_node_id,
+            parent_node_id,
+            reference_type_id,
+            browse_name,
+            type_definition,
+            attributes,
+        } = variable_node;
+
+        let requested_new_node_id = requested_new_node_id.unwrap_or(ua::NodeId::null());
+
+        // This out variable must be initialized without memory allocation because the call below
+        // overwrites it in place, without releasing any held data first.
+        let mut out_new_node_id = ua::NodeId::null();
+
         // SAFETY: We store `node_context` inside the node to keep `data_source` alive.
         let (data_source, node_context) = unsafe { data_source::wrap_data_source(data_source) };
         let status_code = ua::StatusCode::new(unsafe {
             UA_Server_addDataSourceVariableNode(
                 // SAFETY: Cast to `mut` pointer, function is marked `UA_THREADSAFE`.
                 self.0.as_ptr().cast_mut(),
                 // TODO: Verify that `UA_Server_addDataSourceVariableNode()` takes ownership.
-                variable_node.requested_new_node_id.into_raw(),
+                requested_new_node_id.into_raw(),
                 // TODO: Verify that `UA_Server_addDataSourceVariableNode()` takes ownership.
-                variable_node.parent_node_id.into_raw(),
+                parent_node_id.into_raw(),
                 // TODO: Verify that `UA_Server_addDataSourceVariableNode()` takes ownership.
-                variable_node.reference_type_id.into_raw(),
+                reference_type_id.into_raw(),
                 // TODO: Verify that `UA_Server_addDataSourceVariableNode()` takes ownership.
-                variable_node.browse_name.into_raw(),
+                browse_name.into_raw(),
                 // TODO: Verify that `UA_Server_addDataSourceVariableNode()` takes ownership.
-                variable_node.type_definition.into_raw(),
+                type_definition.into_raw(),
                 // TODO: Verify that `UA_Server_addDataSourceVariableNode()` takes ownership.
-                variable_node.attributes.into_raw(),
+                attributes.into_raw(),
                 data_source,
                 node_context.leak(),
-                ptr::null_mut(),
+                out_new_node_id.as_mut_ptr(),
             )
         });
         // In case of an error, the node context has already been freed by the destructor. We must
         // not consume it ourselves (to avoid double-freeing). In case of success, the node context
         // will be consumed when the node is eventually deleted (`UA_ServerConfig::nodeLifecycle`).
-        Error::verify_good(&status_code)
+        Error::verify_good(&status_code)?;
+
+        Ok(out_new_node_id)
     }
 
     /// Adds method node to address space.
     ///
+    /// This returns the node ID that was actually inserted (when no explicit requested new node ID
+    /// was given in `node`), along with the node IDs for the input and output argument nodes.
+    ///
     /// # Errors
     ///
     /// This fails when the node cannot be added.
@@ -464,6 +531,8 @@ impl Server {
             output_arguments_requested_new_node_id,
         } = method_node;
 
+        let requested_new_node_id = requested_new_node_id.unwrap_or(ua::NodeId::null());
+
         // SAFETY: We store `node_context` inside the node to keep `data_source` alive.
         let (method_callback, node_context) =
             unsafe { method_callback::wrap_method_callback(callback) };
@@ -511,6 +580,7 @@ impl Server {
             )
         });
         Error::verify_good(&status_code)?;
+
         Ok((
             out_new_node_id,
             (

src/server/node_types.rs

@@ -3,7 +3,7 @@ use crate::{ua, Attributes, DataType};
 use crate::server::NodeContext;
 
 pub struct Node<T> {
-    pub(crate) requested_new_node_id: ua::NodeId,
+    pub(crate) requested_new_node_id: Option<ua::NodeId>,
     pub(crate) parent_node_id: ua::NodeId,
     pub(crate) reference_type_id: ua::NodeId,
     pub(crate) browse_name: ua::QualifiedName,
@@ -16,7 +16,7 @@ impl<T: Attributes> Node<T> {
     #[must_use]
     pub fn init() -> Self {
         Self {
-            requested_new_node_id: ua::NodeId::null(),
+            requested_new_node_id: None,
             parent_node_id: ua::NodeId::null(),
             reference_type_id: ua::NodeId::null(),
             browse_name: ua::QualifiedName::init(),
@@ -34,7 +34,7 @@ impl<T: Attributes> Node<T> {
         attributes: T,
     ) -> Self {
         Self {
-            requested_new_node_id: ua::NodeId::null(),
+            requested_new_node_id: None,
             parent_node_id,
             reference_type_id,
             browse_name,
@@ -46,7 +46,7 @@ impl<T: Attributes> Node<T> {
 
     #[must_use]
     pub fn with_requested_new_node_id(mut self, requested_new_node_id: ua::NodeId) -> Self {
-        self.requested_new_node_id = requested_new_node_id;
+        self.requested_new_node_id = Some(requested_new_node_id);
         self
     }
 
@@ -57,8 +57,8 @@ impl<T: Attributes> Node<T> {
     }
 
     #[must_use]
-    pub const fn requested_new_node_id(&self) -> &ua::NodeId {
-        &self.requested_new_node_id
+    pub const fn requested_new_node_id(&self) -> Option<&ua::NodeId> {
+        self.requested_new_node_id.as_ref()
     }
 
     #[must_use]
@@ -89,7 +89,7 @@ impl<T: Attributes> Node<T> {
 
 #[derive(Debug, Clone)]
 pub struct ObjectNode {
-    pub requested_new_node_id: ua::NodeId,
+    pub requested_new_node_id: Option<ua::NodeId>,
     pub parent_node_id: ua::NodeId,
     pub reference_type_id: ua::NodeId,
     pub browse_name: ua::QualifiedName,
@@ -99,7 +99,7 @@ pub struct ObjectNode {
 
 #[derive(Debug, Clone)]
 pub struct VariableNode {
-    pub requested_new_node_id: ua::NodeId,
+    pub requested_new_node_id: Option<ua::NodeId>,
     pub parent_node_id: ua::NodeId,
     pub reference_type_id: ua::NodeId,
     pub browse_name: ua::QualifiedName,
@@ -108,7 +108,7 @@ pub struct VariableNode {
 }
 
 pub struct MethodNode {
-    pub requested_new_node_id: ua::NodeId,
+    pub requested_new_node_id: Option<ua::NodeId>,
     pub parent_node_id: ua::NodeId,
     pub reference_type_id: ua::NodeId,
     pub browse_name: ua::QualifiedName,