northbound: enforce valid YANG CRUD callbacks at compile time

Not every YANG node structurally supports every CRUD operation (e.g. a
leaf with a plain default and no when/case can't be deleted, list keys
can't be modified or deleted). This was only checked at daemon startup
(validate_callback), so an invalid registration like .delete_apply()
on such a node compiled fine and only surfaced as a runtime crash.

Make YangPath and CallbacksBuilder a typestate: yang_codegen now emits
a zero-sized Caps marker per node and implements SupportsCreate/
SupportsModify/SupportsDelete/SupportsLookup on it based on the same
CallbackOp::is_valid rules enforced at runtime today. CallbacksBuilder's
create_*/modify_*/delete_*/lookup methods are now gated on the matching
trait, so an invalid registration fails to compile instead of panicking
at startup.

Drop validate_callback, now fully superseded. validate_callbacks
(checks for missing, not invalid, callbacks) is unaffected. Basically
making sure invalid calls are caught at compile time not at runtime.

Concern raised when invalid callbacks were made in a boolean Yang Path:
PR: holo-routing#133
Comment: holo-routing#133 (comment)

Signed-off-by: Paul Wekesa <paul1tw1@gmail.com>

Author: Paul-weqe

holo-daemon/src/northbound/core.rs

@@ -801,9 +801,8 @@ async fn load_callbacks(
         // Receive response.
         let provider_response = responder_rx.await.unwrap();
 
-        // Validate and store callback key.
+        // Store callback key.
         for cb_key in provider_response.callbacks {
-            validate_callback(&cb_key);
             callbacks.insert(cb_key, provider_tx.downgrade());
         }
     }
@@ -847,17 +846,3 @@ fn validate_callbacks(
         std::process::exit(1);
     }
 }
-
-// Checks whether the callback key is valid.
-fn validate_callback(callback: &CallbackKey) {
-    let yang_ctx = YANG_CTX.get().unwrap();
-
-    if let Ok(snode) = yang_ctx.find_path(&callback.path)
-        && !callback.operation.is_valid(&snode)
-    {
-        error!(xpath = %callback.path, operation = ?callback.operation,
-            "invalid callback",
-        );
-        std::process::exit(1);
-    }
-}

holo-northbound/src/configuration.rs

@@ -67,8 +67,20 @@ pub struct CallbacksNode<P: Provider> {
     pub apply: Option<CallbackPhaseTwo<P>>,
 }
 
-pub struct CallbacksBuilder<P: Provider> {
-    path: Option<YangPath>,
+/// Marker traits, implemented by generated per-node `Caps` marker types
+/// (see `holo_northbound::yang_codegen`), that record which CRUD operations
+/// are structurally valid for a given YANG node. `CallbacksBuilder`'s
+/// `create_*`/`modify_*`/`delete_*`/`lookup` methods are only available when
+/// the path's `Caps` type implements the corresponding trait, turning an
+/// invalid callback registration (e.g. `.delete_apply()` on a node that
+/// can't be deleted) into a compile error instead of a runtime one.
+pub trait SupportsCreate {}
+pub trait SupportsModify {}
+pub trait SupportsDelete {}
+pub trait SupportsLookup {}
+
+pub struct CallbacksBuilder<P: Provider, Caps = ()> {
+    path: Option<YangPath<Caps>>,
     callbacks: Callbacks<P>,
 }
 
@@ -91,7 +103,7 @@ pub struct ValidationCallbacks(pub HashMap<String, ValidationCallback>);
 
 #[derive(Default)]
 pub struct ValidationCallbacksBuilder {
-    path: Option<YangPath>,
+    path: Option<String>,
     callbacks: ValidationCallbacks,
 }
 
@@ -298,23 +310,21 @@ where
 
 // ===== impl CallbacksBuilder =====
 
-impl<P> CallbacksBuilder<P>
+impl<P, Caps> CallbacksBuilder<P, Caps>
 where
     P: Provider,
 {
-    pub fn new(callbacks: Callbacks<P>) -> Self {
+    #[must_use]
+    pub fn path<Caps2>(
+        self,
+        path: YangPath<Caps2>,
+    ) -> CallbacksBuilder<P, Caps2> {
         CallbacksBuilder {
-            path: None,
-            callbacks,
+            path: Some(path),
+            callbacks: self.callbacks,
         }
     }
 
-    #[must_use]
-    pub fn path(mut self, path: YangPath) -> Self {
-        self.path = Some(path);
-        self
-    }
-
     #[must_use]
     fn load_prepare(
         mut self,
@@ -352,13 +362,40 @@ where
     }
 
     #[must_use]
-    pub fn lookup(mut self, cb: CallbackLookup<P>) -> Self {
-        let path = self.path.unwrap().to_string();
-        let key = CallbackKey::new(path, CallbackOp::Lookup);
-        self.callbacks.0.entry(key).or_default().lookup = Some(cb);
-        self
+    pub fn build(self) -> Callbacks<P> {
+        self.callbacks
     }
+}
 
+impl<P> CallbacksBuilder<P, ()>
+where
+    P: Provider,
+{
+    pub fn new(callbacks: Callbacks<P>) -> Self {
+        CallbacksBuilder {
+            path: None,
+            callbacks,
+        }
+    }
+}
+
+impl<P> Default for CallbacksBuilder<P, ()>
+where
+    P: Provider,
+{
+    fn default() -> Self {
+        CallbacksBuilder {
+            path: None,
+            callbacks: Callbacks::default(),
+        }
+    }
+}
+
+impl<P, Caps> CallbacksBuilder<P, Caps>
+where
+    P: Provider,
+    Caps: SupportsCreate,
+{
     #[must_use]
     pub fn create_prepare(self, cb: CallbackPhaseOne<P>) -> Self {
         self.load_prepare(CallbackOp::Create, cb)
@@ -373,22 +410,13 @@ where
     pub fn create_apply(self, cb: CallbackPhaseTwo<P>) -> Self {
         self.load_apply(CallbackOp::Create, cb)
     }
+}
 
-    #[must_use]
-    pub fn delete_prepare(self, cb: CallbackPhaseOne<P>) -> Self {
-        self.load_prepare(CallbackOp::Delete, cb)
-    }
-
-    #[must_use]
-    pub fn delete_abort(self, cb: CallbackPhaseTwo<P>) -> Self {
-        self.load_abort(CallbackOp::Delete, cb)
-    }
-
-    #[must_use]
-    pub fn delete_apply(self, cb: CallbackPhaseTwo<P>) -> Self {
-        self.load_apply(CallbackOp::Delete, cb)
-    }
-
+impl<P, Caps> CallbacksBuilder<P, Caps>
+where
+    P: Provider,
+    Caps: SupportsModify,
+{
     #[must_use]
     pub fn modify_prepare(self, cb: CallbackPhaseOne<P>) -> Self {
         self.load_prepare(CallbackOp::Modify, cb)
@@ -403,29 +431,47 @@ where
     pub fn modify_apply(self, cb: CallbackPhaseTwo<P>) -> Self {
         self.load_apply(CallbackOp::Modify, cb)
     }
+}
 
+impl<P, Caps> CallbacksBuilder<P, Caps>
+where
+    P: Provider,
+    Caps: SupportsDelete,
+{
     #[must_use]
-    pub fn build(self) -> Callbacks<P> {
-        self.callbacks
+    pub fn delete_prepare(self, cb: CallbackPhaseOne<P>) -> Self {
+        self.load_prepare(CallbackOp::Delete, cb)
+    }
+
+    #[must_use]
+    pub fn delete_abort(self, cb: CallbackPhaseTwo<P>) -> Self {
+        self.load_abort(CallbackOp::Delete, cb)
+    }
+
+    #[must_use]
+    pub fn delete_apply(self, cb: CallbackPhaseTwo<P>) -> Self {
+        self.load_apply(CallbackOp::Delete, cb)
     }
 }
 
-impl<P> Default for CallbacksBuilder<P>
+impl<P, Caps> CallbacksBuilder<P, Caps>
 where
     P: Provider,
+    Caps: SupportsLookup,
 {
-    fn default() -> Self {
-        CallbacksBuilder {
-            path: None,
-            callbacks: Callbacks::default(),
-        }
+    #[must_use]
+    pub fn lookup(mut self, cb: CallbackLookup<P>) -> Self {
+        let path = self.path.unwrap().to_string();
+        let key = CallbackKey::new(path, CallbackOp::Lookup);
+        self.callbacks.0.entry(key).or_default().lookup = Some(cb);
+        self
     }
 }
 
 // ===== impl ValidationCallbacks =====
 
 impl ValidationCallbacks {
-    pub fn load(&mut self, path: YangPath, cb: ValidationCallback) {
+    pub fn load<Caps>(&mut self, path: YangPath<Caps>, cb: ValidationCallback) {
         let path = path.to_string();
         self.0.insert(path, cb);
     }
@@ -446,14 +492,14 @@ impl ValidationCallbacksBuilder {
     }
 
     #[must_use]
-    pub fn path(mut self, path: YangPath) -> Self {
-        self.path = Some(path);
+    pub fn path<Caps>(mut self, path: YangPath<Caps>) -> Self {
+        self.path = Some(path.to_string());
         self
     }
 
     #[must_use]
     pub fn validate(mut self, cb: ValidationCallback) -> Self {
-        let path = self.path.unwrap().to_string();
+        let path = self.path.take().unwrap();
         self.callbacks.0.insert(path, cb);
         self
     }

holo-northbound/src/lib.rs

@@ -65,8 +65,15 @@ impl<T: YangObject> YangObjectDyn for T {
 // Instances of this structure are created automatically at build-time, and
 // their use should be preferred over regular strings for extra type safety.
 //
-#[derive(Clone, Copy, Debug)]
-pub struct YangPath(&'static str);
+// The `Caps` type parameter is a zero-sized marker, generated alongside each
+// path constant, that records which CRUD operations are structurally valid
+// for the corresponding YANG node (see `configuration::Supports*` traits).
+// It lets `CallbacksBuilder` reject invalid callback registrations (e.g.
+// `.delete_apply()` on a node that doesn't support delete) at compile time.
+pub struct YangPath<Caps = ()>(
+    &'static str,
+    std::marker::PhantomData<fn() -> Caps>,
+);
 
 // A YANG data path, represented as a sequence of elements.
 #[derive(Debug, Clone, PartialEq, Eq)]
@@ -96,19 +103,36 @@ pub type NbProviderReceiver = UnboundedReceiver<api::provider::Notification>;
 
 // ===== impl YangPath =====
 
-impl YangPath {
-    pub const fn new(path: &'static str) -> YangPath {
-        YangPath(path)
+impl<Caps> YangPath<Caps> {
+    pub const fn new(path: &'static str) -> YangPath<Caps> {
+        YangPath(path, std::marker::PhantomData)
     }
 }
 
-impl std::fmt::Display for YangPath {
+// Hand-written instead of derived: a `#[derive(..)]` would add a spurious
+// `Caps: Clone/Copy/Debug` bound, even though `Caps` only ever appears
+// inside `PhantomData`.
+impl<Caps> Clone for YangPath<Caps> {
+    fn clone(&self) -> Self {
+        *self
+    }
+}
+
+impl<Caps> Copy for YangPath<Caps> {}
+
+impl<Caps> std::fmt::Debug for YangPath<Caps> {
+    fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
+        f.debug_tuple("YangPath").field(&self.0).finish()
+    }
+}
+
+impl<Caps> std::fmt::Display for YangPath<Caps> {
     fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
         write!(f, "{}", self.0)
     }
 }
 
-impl AsRef<str> for YangPath {
+impl<Caps> AsRef<str> for YangPath<Caps> {
     fn as_ref(&self) -> &str {
         self.0
     }

holo-northbound/src/yang_codegen/mod.rs

@@ -231,11 +231,32 @@ fn generate_paths(
     w: &mut CodeWriter,
     snode: &SchemaNode<'_>,
 ) -> std::fmt::Result {
+    // Zero-sized marker type recording which CRUD operations this node
+    // structurally supports, computed with the same rules enforced at
+    // runtime by `CallbackOp::is_valid`. This lets `CallbacksBuilder` reject
+    // invalid callback registrations (e.g. `.delete_apply()` on a node that
+    // doesn't support delete) at compile time.
+    emit!(w, 1, "pub struct Caps;")?;
+    for (op, trait_name) in [
+        (crate::configuration::CallbackOp::Create, "SupportsCreate"),
+        (crate::configuration::CallbackOp::Modify, "SupportsModify"),
+        (crate::configuration::CallbackOp::Delete, "SupportsDelete"),
+        (crate::configuration::CallbackOp::Lookup, "SupportsLookup"),
+    ] {
+        if op.is_valid(snode) {
+            emit!(
+                w,
+                1,
+                "impl holo_northbound::configuration::{trait_name} for Caps {{}}"
+            )?;
+        }
+    }
+
     let path = snode.path(SchemaPathFormat::DATA);
     emit!(
         w,
         1,
-        "pub const PATH: YangPath = YangPath::new(\"{path}\");"
+        "pub const PATH: YangPath<Caps> = YangPath::new(\"{path}\");"
     )?;
 
     // For notifications, also generate data path relative to the nearest