From 209ee6e00e67230b5a94b8fff20658df6fadba52 Mon Sep 17 00:00:00 2001 From: Leonardo Rodoni Date: Fri, 21 Aug 2026 15:37:04 +0200 Subject: [PATCH 1/7] fix(yang-push): resolve xpath-filter target modules by prefix Fetching a YANG Library by subscription id failed for devices that send an inline datastore-xpath-filter without xmlns bindings (e.g. Cisco IOS-XR), because module resolution only looked at declared namespace prefixes. The target module was silently dropped and every subsequent notification failed validation. Resolve xpath-filter modules per the RFC 8641 XPath context: use a declared xmlns binding when present (e.g. Huawei), otherwise treat the path prefix as the YANG module name (e.g. Cisco IOS-XR). Both are conformant. Subtree and stream filters keep namespace-based lookup. An empty resolution result is now a hard error instead of silently caching an incomplete library, and errors are now typed instead of generic IO errors. Add unit tests covering the two resolution cases, and extra trace-level logging for debugging. --- crates/netconf-proto/src/client.rs | 19 + crates/netconf-proto/src/xml_utils.rs | 94 ++-- crates/netconf-proto/src/yang_push/filters.rs | 14 + crates/netconf-proto/src/yang_push/tests.rs | 28 ++ crates/yang-push/src/cache/fetcher.rs | 469 +++++++++++++++--- crates/yang-push/src/cache/storage.rs | 25 + 6 files changed, 534 insertions(+), 115 deletions(-) diff --git a/crates/netconf-proto/src/client.rs b/crates/netconf-proto/src/client.rs index 479a1c44..a8c4b51c 100644 --- a/crates/netconf-proto/src/client.rs +++ b/crates/netconf-proto/src/client.rs @@ -755,10 +755,14 @@ impl NetConfSshClient { if let Some(RpcResponse::WellKnown(WellKnownRpcResponse::Data(data))) = rpc_reply.reply().responses() { + trace!("[{}] Raw response for filters: `{data}`", self.peer); + let mut reader = NsReader::from_str(data); reader.config_mut().trim_text(true); let mut parser = crate::xml_utils::XmlParser::new(reader)?; let filters = Filters::xml_deserialize(&mut parser)?; + + trace!("[{}] Parsed filters: {filters:?}", self.peer); return Ok(filters); } Err(NetConfSshClientError::UnexpectedMessage { @@ -792,6 +796,10 @@ impl NetConfSshClient { if let Some(RpcResponse::WellKnown(WellKnownRpcResponse::Data(data))) = rpc_reply.reply().responses() { + trace!( + "[{}] Raw response for subscription {id}: `{data}`", + self.peer + ); // Parse the response streams if any returned, filters if any returned // and then the subscription details let mut reader = NsReader::from_str(data); @@ -801,6 +809,10 @@ impl NetConfSshClient { { parser.open(Some(SUBSCRIBED_NOTIFICATIONS_NS), "subscriptions")?; let mut subscription = Subscription::xml_deserialize(&mut parser)?; + trace!( + "[{}] Parsed subscription {id} before target-by-reference resolution: {subscription:?}", + self.peer + ); if let Target::Datastore(datastore_target) = &mut subscription.target && let DatastoreSelectionFilterObjects::ByReference(name) = &datastore_target.selection @@ -818,6 +830,13 @@ impl NetConfSshClient { } } parser.close()?; + trace!( + "[{}] Final subscription {id}: target={:?} module_version={:?} yang_library_content_id={:?}", + self.peer, + subscription.target, + subscription.module_version, + subscription.yang_library_content_id + ); subscription } else { return Err(NetConfSshClientError::ParsingError( diff --git a/crates/netconf-proto/src/xml_utils.rs b/crates/netconf-proto/src/xml_utils.rs index b6a066b1..7d671923 100644 --- a/crates/netconf-proto/src/xml_utils.rs +++ b/crates/netconf-proto/src/xml_utils.rs @@ -764,7 +764,7 @@ impl<'a, R: io::BufRead> XmlParser<'a, R> { all_namespaces.insert(prefix, String::from_utf8_lossy(ns).into_owned()); } let path = self.tag_string()?; - let used_namespaces = Self::find_xpath_prefixes(&path); + let used_namespaces = find_xpath_prefixes(&path); let namespaces: IndexMap = all_namespaces .into_iter() .filter(|(prefix, _)| used_namespaces.contains(prefix)) @@ -811,59 +811,61 @@ impl<'a, R: io::BufRead> XmlParser<'a, R> { } } } +} - /// Find prefixes used within an Xpath expression - fn find_xpath_prefixes(xpath: &str) -> HashSet { - let mut prefixes = HashSet::new(); - let mut chars = xpath.char_indices().peekable(); - let mut in_single = false; - let mut in_double = false; - - while let Some((i, c)) = chars.next() { - // Skip over string literals — colons inside them aren't prefixes. - if in_single { - if c == '\'' { - in_single = false; - } - continue; +/// Find the prefixes used within an Xpath expression (e.g. the `if` in +/// `/if:interfaces/if:interface`). String literals are skipped and axis +/// specifiers (`child::`) are not treated as prefixes. +pub(crate) fn find_xpath_prefixes(xpath: &str) -> HashSet { + let mut prefixes = HashSet::new(); + let mut chars = xpath.char_indices().peekable(); + let mut in_single = false; + let mut in_double = false; + + while let Some((i, c)) = chars.next() { + // Skip over string literals — colons inside them aren't prefixes. + if in_single { + if c == '\'' { + in_single = false; } - if in_double { - if c == '"' { - in_double = false; - } - continue; + continue; + } + if in_double { + if c == '"' { + in_double = false; } - match c { - '\'' => in_single = true, - '"' => in_double = true, - c if c.is_ascii_alphabetic() || c == '_' => { - let start = i; - let mut end = i + c.len_utf8(); - while let Some(&(_, nc)) = chars.peek() { - if nc.is_ascii_alphanumeric() || nc == '_' || nc == '-' || nc == '.' { - chars.next(); - end += nc.len_utf8(); - } else { - break; - } + continue; + } + match c { + '\'' => in_single = true, + '"' => in_double = true, + c if c.is_ascii_alphabetic() || c == '_' => { + let start = i; + let mut end = i + c.len_utf8(); + while let Some(&(_, nc)) = chars.peek() { + if nc.is_ascii_alphanumeric() || nc == '_' || nc == '-' || nc == '.' { + chars.next(); + end += nc.len_utf8(); + } else { + break; } - // A prefix is an NCName followed by exactly one ':' - // (two colons = axis specifier like `child::`). - if let Some(&(_, ':')) = chars.peek() { - let mut look = chars.clone(); - look.next(); - let is_axis = matches!(look.peek(), Some(&(_, ':'))); - if !is_axis { - prefixes.insert(xpath[start..end].to_string()); - chars.next(); // consume the ':' - } + } + // A prefix is an NCName followed by exactly one ':' + // (two colons = axis specifier like `child::`). + if let Some(&(_, ':')) = chars.peek() { + let mut look = chars.clone(); + look.next(); + let is_axis = matches!(look.peek(), Some(&(_, ':'))); + if !is_axis { + prefixes.insert(xpath[start..end].to_string()); + chars.next(); // consume the ':' } } - _ => {} } + _ => {} } - prefixes } + prefixes } /// Format a `DateTime` as YANG `date-and-time` (RFC 3339, UTC). @@ -1473,7 +1475,7 @@ mod tests { fn assert_prefixes(expr: &str, expected: HashSet) { assert_eq!( - XmlParser::>::find_xpath_prefixes(expr), + find_xpath_prefixes(expr), expected, "unexpected prefix set for: {expr}" ); diff --git a/crates/netconf-proto/src/yang_push/filters.rs b/crates/netconf-proto/src/yang_push/filters.rs index 2d982505..f2b226dd 100644 --- a/crates/netconf-proto/src/yang_push/filters.rs +++ b/crates/netconf-proto/src/yang_push/filters.rs @@ -1,3 +1,4 @@ +// Copyright (C) 2026-present The NetCalyx Authors. // Copyright (C) 2026-present The NetGauze Authors. // // Licensed under the Apache License, Version 2.0 (the "License"); @@ -353,6 +354,19 @@ pub struct DatastoreXPathFilter { pub path: Box, } +impl DatastoreXPathFilter { + /// Prefixes used in the xpath `path` (e.g. the `if` in + /// `/if:interfaces/if:interface`), sorted for determinism. Axis specifiers + /// and string literals are not treated as prefixes. + pub fn path_prefixes(&self) -> Vec { + let mut prefixes: Vec = crate::xml_utils::find_xpath_prefixes(&self.path) + .into_iter() + .collect(); + prefixes.sort_unstable(); + prefixes + } +} + impl XmlSerialize for DatastoreXPathFilter { fn xml_serialize( &self, diff --git a/crates/netconf-proto/src/yang_push/tests.rs b/crates/netconf-proto/src/yang_push/tests.rs index 8e1a407c..7eed95d7 100644 --- a/crates/netconf-proto/src/yang_push/tests.rs +++ b/crates/netconf-proto/src/yang_push/tests.rs @@ -1,3 +1,4 @@ +// Copyright (C) 2026-present The NetCalyx Authors. // Copyright (C) 2026-present The NetGauze Authors. // // Licensed under the Apache License, Version 2.0 (the "License"); @@ -782,3 +783,30 @@ fn test_yang_push_module_version_json_serde() { let deserialized: YangPushModuleVersion = serde_json::from_value(json_value).unwrap(); assert_eq!(deserialized, modeled); } + +#[test] +fn test_datastore_xpath_filter_path_prefixes() { + // Cisco IOS-XR: prefix equals the module name, no xmlns binding declared. + let cisco = DatastoreXPathFilter { + namespaces: Box::new([]), + path: "Cisco-IOS-XR-procmem-oper:processes-memory/nodes/node/process-ids/process-id".into(), + }; + assert_eq!(cisco.path_prefixes(), vec!["Cisco-IOS-XR-procmem-oper"]); + + // Multi-module xpath (Huawei-style), distinct prefixes (sorted). + let multi = DatastoreXPathFilter { + namespaces: Box::new([ + ("devm".into(), "urn:huawei:yang:huawei-devm".into()), + ("driver".into(), "urn:huawei:yang:huawei-driver".into()), + ]), + path: "/devm:devm/devm:chassiss/devm:chassis/driver:power-supply-attribute".into(), + }; + assert_eq!(multi.path_prefixes(), vec!["devm", "driver"]); + + // Unprefixed (default-namespace) steps contribute no prefixes. + let unprefixed = DatastoreXPathFilter { + namespaces: Box::new([]), + path: "/interfaces/interface/state/counters".into(), + }; + assert!(unprefixed.path_prefixes().is_empty()); +} diff --git a/crates/yang-push/src/cache/fetcher.rs b/crates/yang-push/src/cache/fetcher.rs index 6c30570e..9aa828ce 100644 --- a/crates/yang-push/src/cache/fetcher.rs +++ b/crates/yang-push/src/cache/fetcher.rs @@ -29,7 +29,9 @@ use crate::cache::storage::{SubscriptionInfo, YangLibraryCacheError}; use netcalyx_netconf_proto::capabilities::{Capability, NetconfVersion}; use netcalyx_netconf_proto::client::{NetconfSshConnectConfig, SshAuth, SshHandler, connect}; -use netcalyx_netconf_proto::yang_push::filters::StreamSelectionFilterObjects; +use netcalyx_netconf_proto::yang_push::filters::{ + DatastoreFilterSpec, DatastoreXPathFilter, StreamSelectionFilterObjects, +}; use netcalyx_netconf_proto::yang_push::subscription::{ DatastoreSelectionFilterObjects, Target, YangPushModuleVersion, }; @@ -49,6 +51,123 @@ pub type FetcherResult = Result< Box<(SubscriptionInfo, YangLibraryCacheError)>, >; +/// Append `module` to `modules` as a [`YangPushModuleVersion`], skipping it if +/// a module with the same name is already present. +fn push_module( + modules: &mut Vec, + module: &netcalyx_netconf_proto::yanglib::Module, +) { + if modules.iter().any(|m| m.name() == module.name()) { + return; + } + modules.push(YangPushModuleVersion::new( + module.name().into(), + module.revision().map(|x| x.into()), + None, + )); +} + +/// Resolve every module referenced by a namespace binding against the device +/// YANG Library. Used for subtree filters and stream filters, where modules +/// are identified by the root element namespace. +fn resolve_by_namespaces( + router_yang_library: &YangLibrary, + ds_name: &DatastoreName, + namespaces: &[(Box, Box)], + empty: &SubscriptionInfo, +) -> Result, Box<(SubscriptionInfo, YangLibraryCacheError)>> { + let mut ret = Vec::with_capacity(namespaces.len()); + for (_prefix, namespace) in namespaces { + let module = router_yang_library + .find_module_by_datastore_and_ns(ds_name, namespace) + .ok_or_else(|| { + error!( + %namespace, + %ds_name, + "target module not found in device YANG Library by namespace", + ); + Box::new(( + empty.clone(), + YangLibraryCacheError::ModuleNamespaceNotFound { + namespace: namespace.clone(), + datastore: ds_name.to_string().into_boxed_str(), + }, + )) + })?; + trace!(namespace=%namespace, module=%module.name(), "resolved target module by namespace"); + push_module(&mut ret, module); + } + Ok(ret) +} + +/// Resolve xpath-filter modules per the RFC 8641 XPath context for +/// `datastore-xpath-filter`: prefixes declared via `xmlns` take precedence +/// (e.g. Huawei), otherwise the prefix is the YANG module name from the +/// server's base context (e.g. Cisco IOS-XR). Both are conformant; we try the +/// declared binding first, then the module name. +fn resolve_by_xpath( + router_yang_library: &YangLibrary, + ds_name: &DatastoreName, + xpath: &DatastoreXPathFilter, + empty: &SubscriptionInfo, +) -> Result, Box<(SubscriptionInfo, YangLibraryCacheError)>> { + let declared: HashMap<&str, &str> = xpath + .namespaces + .iter() + .map(|(prefix, ns)| (prefix.as_ref(), ns.as_ref())) + .collect(); + let mut ret = Vec::new(); + let prefixes = xpath.path_prefixes(); + trace!( + %ds_name, + path = %xpath.path, + ?prefixes, + declared_prefixes = declared.len(), + "resolving target modules from xpath filter", + ); + for prefix in prefixes { + let module = if let Some(namespace) = declared.get(prefix.as_str()) { + router_yang_library + .find_module_by_datastore_and_ns(ds_name, namespace) + .ok_or_else(|| { + error!( + %prefix, + namespace, + %ds_name, + "target module not found for declared xpath prefix namespace", + ); + Box::new(( + empty.clone(), + YangLibraryCacheError::ModuleNamespaceNotFound { + namespace: (*namespace).into(), + datastore: ds_name.to_string().into_boxed_str(), + }, + )) + })? + } else { + // No xmlns binding: per RFC 8641 the prefix is the YANG module + // name in the server's base XPath context. + debug!( + %prefix, + "xpath prefix has no declared namespace binding, resolving it as a module name", + ); + router_yang_library.find_module(&prefix).ok_or_else(|| { + error!( + %prefix, + "target module not found when resolving xpath prefix as a module name", + ); + Box::new(( + empty.clone(), + YangLibraryCacheError::ModulePrefixNotFound(prefix.clone().into_boxed_str()), + )) + })? + }; + trace!(%prefix, module=%module.name(), "resolved target module from xpath prefix"); + push_module(&mut ret, module); + } + Ok(ret) +} + /// Fetch YANG Library and schemas from an external source pub trait YangLibraryFetcher { /// A non-blocking version which returns a [JoinHandle] @@ -275,89 +394,91 @@ impl NetconfYangLibraryFetcher { let modules = if let Some(modules) = &subscription.module_version { debug!( - peer_ip=%peer_ip, + host=%host, subscription_id, - modules=?modules, - "using module-version reported by device for subscription", + module_count = modules.len(), + "device reported module-version for subscription, using it to resolve target modules", ); modules.clone().to_vec() } else { - let (ds_name, namespaces) = match &subscription.target { - Target::Stream(stream_target) => { - match &stream_target.filter { - StreamSelectionFilterObjects::ByReference(name) => { - // references are resolved in the NETCONF client, - // if we reach this point, there must be a misconfigured router, - return Err(Box::new(( - empty, - YangLibraryCacheError::IoError(std::io::Error::other(format!( - "cannot fetch YANG Library for stream selection filter by reference for {name}" - ))), - ))); - } - StreamSelectionFilterObjects::WithInSubscription(filter) => { - (DatastoreName::Running, filter.namespaces()) - } + let (ds_name, modules) = match &subscription.target { + Target::Stream(stream_target) => match &stream_target.filter { + StreamSelectionFilterObjects::ByReference(name) => { + // references are resolved in the NETCONF client, + // if we reach this point, there must be a misconfigured router, + error!( + %name, + subscription_id, + "stream selection filter reached fetcher unresolved by reference, likely a misconfigured router", + ); + return Err(Box::new(( + empty, + YangLibraryCacheError::UnresolvedFilterReference(name.clone()), + ))); } - } + StreamSelectionFilterObjects::WithInSubscription(filter) => { + let ds_name = DatastoreName::Running; + let modules = resolve_by_namespaces( + &router_yang_library, + &ds_name, + filter.namespaces(), + &empty, + )?; + (ds_name, modules) + } + }, Target::Datastore(datastore_target) => match &datastore_target.selection { DatastoreSelectionFilterObjects::ByReference(name) => { + error!( + %name, + subscription_id, + "datastore selection filter reached fetcher unresolved by reference, likely a misconfigured router", + ); return Err(Box::new(( empty, - YangLibraryCacheError::IoError(std::io::Error::other(format!( - "cannot fetch YANG Library for datastore selection filter by reference for {name}" - ))), + YangLibraryCacheError::UnresolvedFilterReference(name.clone()), ))); } DatastoreSelectionFilterObjects::WithInSubscription(filter) => { - (datastore_target.datastore.clone(), filter.namespaces()) + let ds_name = datastore_target.datastore.clone(); + let modules = match filter { + DatastoreFilterSpec::Xpath(xpath) => { + resolve_by_xpath(&router_yang_library, &ds_name, xpath, &empty)? + } + DatastoreFilterSpec::Subtree(subtree) => resolve_by_namespaces( + &router_yang_library, + &ds_name, + &subtree.namespaces, + &empty, + )?, + }; + (ds_name, modules) } }, }; - debug!( - peer_ip=%peer_ip, - subscription_id, - ds_name=?ds_name, - namespaces=?namespaces, - target=?subscription.target, - "no module-version reported by device, resolving target namespaces against YANG Library instead", - ); - let mut ret = Vec::with_capacity(namespaces.len()); - for (prefix, namespace) in namespaces { - let module = router_yang_library.find_module_by_datastore_and_ns(&ds_name, namespace).ok_or_else(|| { - warn!( - peer_ip=%peer_ip, - subscription_id, - ds_name=?ds_name, - prefix, - namespace, - "module with namespace not found in YANG Library for datastore", - ); - Box::new((empty.clone(), YangLibraryCacheError::IoError(std::io::Error::other(format!("module with namespace {namespace} not found in YANG Library for datastore {ds_name}"))))) - })?; - trace!( - peer_ip=%peer_ip, + if modules.is_empty() { + error!( + host=%host, subscription_id, - prefix, - namespace, - module_name=module.name(), - "resolved xpath-filter prefix to module via namespace", - ); - ret.push(YangPushModuleVersion::new( - module.name().into(), - module.revision().map(|x| x.into()), - None, - )); - } - if ret.is_empty() { - warn!( - peer_ip=%peer_ip, - subscription_id, - target=?subscription.target, - "target namespaces resolution produced no modules; the target's YANG module(s) will not be fetched", + %ds_name, + "no target modules could be resolved from subscription filter", ); + return Err(Box::new(( + empty, + YangLibraryCacheError::NoTargetModulesResolved { + subscription_id, + datastore: ds_name.to_string().into_boxed_str(), + }, + ))); } - ret + debug!( + host=%host, + subscription_id, + %ds_name, + modules = ?modules.iter().map(|m| m.name()).collect::>(), + "resolved target modules from subscription filter", + ); + modules }; let mut module_names = modules.iter().map(|x| x.name()).collect::>(); @@ -387,9 +508,7 @@ impl NetconfYangLibraryFetcher { let subscription_target = subscription.target.try_into().map_err(|err| { Box::new(( empty, - YangLibraryCacheError::IoError(std::io::Error::other(format!( - "invalid subscription target: {err}" - ))), + YangLibraryCacheError::InvalidSubscriptionTarget(format!("{err}").into_boxed_str()), )) })?; let subscription_info = SubscriptionInfo::new( @@ -818,3 +937,215 @@ mod retry_tests { ); } } + +#[cfg(test)] +mod resolve_tests { + use super::*; + use netcalyx_netconf_proto::yang_push::filters::DatastoreXPathFilter; + use netcalyx_netconf_proto::yanglib::{Datastore, Module, ModuleSet, Schema, YangLibrary}; + + fn empty_info() -> SubscriptionInfo { + SubscriptionInfo::new_empty("127.0.0.1".parse().unwrap(), 1) + } + + /// A single-datastore, single-module-set YANG Library fixture. + /// `modules` are `(name, namespace)` pairs. + fn make_yang_library(ds_name: DatastoreName, modules: &[(&str, &str)]) -> YangLibrary { + let modules = modules + .iter() + .map(|(name, ns)| { + Module::new( + (*name).into(), + None, + (*ns).into(), + Box::new([]), + Box::new([]), + Box::new([]), + Box::new([]), + Box::new([]), + ) + }) + .collect(); + YangLibrary::new( + "test-content-id".into(), + vec![ModuleSet::new("modules".into(), modules, vec![])], + vec![Schema::new("schema".into(), Box::new(["modules".into()]))], + vec![Datastore::new(ds_name, "schema".into())], + ) + } + + fn xpath_filter(namespaces: &[(&str, &str)], path: &str) -> DatastoreXPathFilter { + DatastoreXPathFilter { + namespaces: namespaces + .iter() + .map(|(p, ns)| ((*p).into(), (*ns).into())) + .collect(), + path: path.into(), + } + } + + #[test] + fn test_resolve_by_namespaces_resolves_each_namespace_to_a_module() { + let yang_lib = make_yang_library( + DatastoreName::Running, + &[("if-mod", "urn:example:interfaces")], + ); + let namespaces: Box<[(Box, Box)]> = + Box::new([("if".into(), "urn:example:interfaces".into())]); + + let result = resolve_by_namespaces( + &yang_lib, + &DatastoreName::Running, + &namespaces, + &empty_info(), + ) + .expect("namespace should resolve to a module"); + + assert_eq!(result.len(), 1); + assert_eq!(result[0].name(), "if-mod"); + } + + #[test] + fn test_resolve_by_namespaces_dedups_same_module_seen_twice() { + // Two distinct namespace bindings resolving to the same module must + // only appear once in the result (push_module dedup). + let yang_lib = make_yang_library( + DatastoreName::Running, + &[("if-mod", "urn:example:interfaces")], + ); + let namespaces: Box<[(Box, Box)]> = Box::new([ + ("a".into(), "urn:example:interfaces".into()), + ("b".into(), "urn:example:interfaces".into()), + ]); + + let result = resolve_by_namespaces( + &yang_lib, + &DatastoreName::Running, + &namespaces, + &empty_info(), + ) + .expect("namespaces should resolve"); + + assert_eq!(result.len(), 1); + assert_eq!(result[0].name(), "if-mod"); + } + + #[test] + fn test_resolve_by_namespaces_unknown_namespace_is_a_hard_error() { + let yang_lib = make_yang_library(DatastoreName::Running, &[]); + let namespaces: Box<[(Box, Box)]> = + Box::new([("if".into(), "urn:example:unknown".into())]); + + let err = resolve_by_namespaces( + &yang_lib, + &DatastoreName::Running, + &namespaces, + &empty_info(), + ) + .expect_err("unknown namespace must not resolve"); + + assert!(matches!( + err.1, + YangLibraryCacheError::ModuleNamespaceNotFound { .. } + )); + } + + /// Cisco IOS-XR style: no `xmlns` binding declared, prefix equals the + /// YANG module name directly (RFC 8641 base XPath context). + #[test] + fn test_resolve_by_xpath_falls_back_to_module_name_when_undeclared() { + let yang_lib = make_yang_library( + DatastoreName::Running, + &[( + "Cisco-IOS-XR-procmem-oper", + "urn:cisco:params:xml:ns:yang:procmem-oper", + )], + ); + let filter = xpath_filter( + &[], + "/Cisco-IOS-XR-procmem-oper:processes-memory/nodes/node", + ); + + let result = resolve_by_xpath(&yang_lib, &DatastoreName::Running, &filter, &empty_info()) + .expect("undeclared prefix should resolve as a module name"); + + assert_eq!(result.len(), 1); + assert_eq!(result[0].name(), "Cisco-IOS-XR-procmem-oper"); + } + + /// Huawei style: `xmlns` binding declared on the filter takes precedence + /// over treating the prefix as a module name. + #[test] + fn test_resolve_by_xpath_prefers_declared_namespace_binding() { + let yang_lib = make_yang_library( + DatastoreName::Running, + &[("huawei-devm", "urn:huawei:yang:huawei-devm")], + ); + let filter = xpath_filter( + &[("devm", "urn:huawei:yang:huawei-devm")], + "/devm:devm/devm:chassis", + ); + + let result = resolve_by_xpath(&yang_lib, &DatastoreName::Running, &filter, &empty_info()) + .expect("declared xmlns binding should resolve the module"); + + assert_eq!(result.len(), 1); + assert_eq!(result[0].name(), "huawei-devm"); + } + + #[test] + fn test_resolve_by_xpath_multiple_distinct_prefixes_resolve_independently() { + let yang_lib = make_yang_library( + DatastoreName::Running, + &[ + ("if-mod", "urn:example:interfaces"), + ("rt-mod", "urn:example:routing"), + ], + ); + let filter = xpath_filter( + &[ + ("if", "urn:example:interfaces"), + ("rt", "urn:example:routing"), + ], + "/if:interfaces/if:interface | /rt:routing/rt:ribs", + ); + + let mut result = + resolve_by_xpath(&yang_lib, &DatastoreName::Running, &filter, &empty_info()) + .expect("both prefixes should resolve") + .into_iter() + .map(|m| m.name().to_string()) + .collect::>(); + result.sort_unstable(); + + assert_eq!(result, vec!["if-mod", "rt-mod"]); + } + + #[test] + fn test_resolve_by_xpath_declared_namespace_not_in_library_is_a_hard_error() { + let yang_lib = make_yang_library(DatastoreName::Running, &[]); + let filter = xpath_filter(&[("devm", "urn:huawei:yang:huawei-devm")], "/devm:devm"); + + let err = resolve_by_xpath(&yang_lib, &DatastoreName::Running, &filter, &empty_info()) + .expect_err("declared namespace missing from library must fail"); + + assert!(matches!( + err.1, + YangLibraryCacheError::ModuleNamespaceNotFound { .. } + )); + } + + #[test] + fn test_resolve_by_xpath_undeclared_prefix_not_a_module_name_is_a_hard_error() { + let yang_lib = make_yang_library(DatastoreName::Running, &[]); + let filter = xpath_filter(&[], "/bogus:interfaces"); + + let err = resolve_by_xpath(&yang_lib, &DatastoreName::Running, &filter, &empty_info()) + .expect_err("prefix with no binding and no matching module must fail"); + + assert!(matches!( + err.1, + YangLibraryCacheError::ModulePrefixNotFound(ref p) if p.as_ref() == "bogus" + )); + } +} diff --git a/crates/yang-push/src/cache/storage.rs b/crates/yang-push/src/cache/storage.rs index c00ad316..5c0331bd 100644 --- a/crates/yang-push/src/cache/storage.rs +++ b/crates/yang-push/src/cache/storage.rs @@ -164,6 +164,31 @@ pub enum YangLibraryCacheError { #[strum(to_string = "failed to connect to netconf server: {0}")] NetConfClientError(netcalyx_netconf_proto::client::NetConfSshClientError), + + #[strum(to_string = "cannot fetch YANG Library for selection filter by reference '{0}'")] + UnresolvedFilterReference(Box), + + #[strum( + to_string = "module with namespace '{namespace}' not found in YANG Library for datastore '{datastore}'" + )] + ModuleNamespaceNotFound { + namespace: Box, + datastore: Box, + }, + + #[strum(to_string = "module '{0}' (used as xpath prefix) not found in YANG Library")] + ModulePrefixNotFound(Box), + + #[strum( + to_string = "no target modules could be resolved from subscription {subscription_id} filter for datastore '{datastore}'" + )] + NoTargetModulesResolved { + subscription_id: SubscriptionId, + datastore: Box, + }, + + #[strum(to_string = "invalid subscription target: {0}")] + InvalidSubscriptionTarget(Box), } impl std::error::Error for YangLibraryCacheError {} From 7d62119374bd42fc3a592309836686f9c9977390 Mon Sep 17 00:00:00 2001 From: Leonardo Rodoni Date: Fri, 21 Aug 2026 16:27:07 +0200 Subject: [PATCH 2/7] fix(yang-push): scope xpath prefix module lookup by datastore The Cisco-style xpath prefix fallback (prefix == module name) resolved modules via YangLibrary::find_module, which searches every module set in the library regardless of datastore. A module name can be pinned at different revisions in different module sets (RFC 8525), so an unscoped lookup could silently fetch and cache the wrong revision for a subscription's target datastore. Add YangLibrary::find_module_by_datastore_and_name, mirroring the existing namespace-scoped lookup, and use it for the prefix-as-name fallback so both resolution paths are scoped consistently. Add a regression test with the same module name at two revisions in two datastores. --- crates/netconf-proto/src/yanglib.rs | 48 +++++++++++++++++ crates/yang-push/src/cache/fetcher.rs | 74 +++++++++++++++++++++++---- 2 files changed, 112 insertions(+), 10 deletions(-) diff --git a/crates/netconf-proto/src/yanglib.rs b/crates/netconf-proto/src/yanglib.rs index f65d7dcc..f31d8ff5 100644 --- a/crates/netconf-proto/src/yanglib.rs +++ b/crates/netconf-proto/src/yanglib.rs @@ -1,3 +1,19 @@ +// Copyright (C) 2026-present The NetCalyx Authors. +// Copyright (C) 2025-present The NetGauze Authors. +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or +// implied. +// See the License for the specific language governing permissions and +// limitations under the License. + use crate::xml_utils::{ParsingError, XmlDeserialize, XmlParser, XmlSerialize, XmlWriter}; use crate::yangparser::{YangDependencies, extract_yang_dependencies}; use crate::{YANG_DATASTORES_NS_STR, YANG_LIBRARY_AUGMENTED_BY_NS, YANG_LIBRARY_NS}; @@ -110,6 +126,14 @@ impl YangLibrary { None } + /// Find a module by namespace, scoped to the module sets referenced by + /// `datastore_name`'s schema (RFC 8525 datastore -> schema -> module-set). + /// + /// Unlike [Self::find_module], this only considers module sets reachable + /// from the given datastore, since different datastores can be backed by + /// different schemas/module-sets and may pin different revisions of the + /// same module. Returns `None` if the datastore, its schema, or a + /// matching module cannot be found. pub fn find_module_by_datastore_and_ns( &self, datastore_name: &DatastoreName, @@ -130,6 +154,30 @@ impl YangLibrary { None } + /// Find a module by name, scoped to the module sets referenced by + /// `datastore_name`'s schema (RFC 8525 datastore -> schema -> module-set). + /// + /// Unlike [Self::find_module], this only considers module sets reachable + /// from the given datastore, since different datastores can be backed by + /// different schemas/module-sets and may pin different revisions of the + /// same module name. Returns `None` if the datastore, its schema, or a + /// matching module cannot be found. + pub fn find_module_by_datastore_and_name( + &self, + datastore_name: &DatastoreName, + name: &str, + ) -> Option<&Module> { + let datastore = self.datastores().get(datastore_name)?; + let schema = self.schemas().get(datastore.schema())?; + for module_set_name in schema.modules_sets() { + let module_set = self.module_sets().get(module_set_name)?; + if let Some(module) = module_set.modules().get(name) { + return Some(module); + } + } + None + } + /// Register the YANG Lib to in the Confluent Schema Registry. /// /// `root_schema_name` is the name of the root module to register. diff --git a/crates/yang-push/src/cache/fetcher.rs b/crates/yang-push/src/cache/fetcher.rs index 9aa828ce..60af0b25 100644 --- a/crates/yang-push/src/cache/fetcher.rs +++ b/crates/yang-push/src/cache/fetcher.rs @@ -151,16 +151,20 @@ fn resolve_by_xpath( %prefix, "xpath prefix has no declared namespace binding, resolving it as a module name", ); - router_yang_library.find_module(&prefix).ok_or_else(|| { - error!( - %prefix, - "target module not found when resolving xpath prefix as a module name", - ); - Box::new(( - empty.clone(), - YangLibraryCacheError::ModulePrefixNotFound(prefix.clone().into_boxed_str()), - )) - })? + router_yang_library + .find_module_by_datastore_and_name(ds_name, &prefix) + .ok_or_else(|| { + error!( + %prefix, + "target module not found when resolving xpath prefix as a module name", + ); + Box::new(( + empty.clone(), + YangLibraryCacheError::ModulePrefixNotFound( + prefix.clone().into_boxed_str(), + ), + )) + })? }; trace!(%prefix, module=%module.name(), "resolved target module from xpath prefix"); push_module(&mut ret, module); @@ -984,6 +988,38 @@ mod resolve_tests { } } + /// Two datastores, each with its own schema/module-set pinning a + /// different revision of the same module name. + fn make_multi_datastore_yang_library() -> YangLibrary { + let module = |revision: &str| { + Module::new( + "foo-mod".into(), + Some(revision.into()), + "urn:example:foo".into(), + Box::new([]), + Box::new([]), + Box::new([]), + Box::new([]), + Box::new([]), + ) + }; + YangLibrary::new( + "test-content-id".into(), + vec![ + ModuleSet::new("operational-set".into(), vec![module("2020-01-01")], vec![]), + ModuleSet::new("running-set".into(), vec![module("2023-01-01")], vec![]), + ], + vec![ + Schema::new("op-schema".into(), Box::new(["operational-set".into()])), + Schema::new("run-schema".into(), Box::new(["running-set".into()])), + ], + vec![ + Datastore::new(DatastoreName::Operational, "op-schema".into()), + Datastore::new(DatastoreName::Running, "run-schema".into()), + ], + ) + } + #[test] fn test_resolve_by_namespaces_resolves_each_namespace_to_a_module() { let yang_lib = make_yang_library( @@ -1073,6 +1109,24 @@ mod resolve_tests { assert_eq!(result[0].name(), "Cisco-IOS-XR-procmem-oper"); } + /// The module-name fallback must scope its lookup to the target + /// datastore, the same as the declared-namespace path does — not return + /// whichever module-set happens to be first in the library. Regression + /// test for a module name present, at different revisions, in two + /// datastores' module sets. + #[test] + fn test_resolve_by_xpath_module_name_fallback_is_scoped_by_datastore() { + let yang_lib = make_multi_datastore_yang_library(); + let filter = xpath_filter(&[], "/foo-mod:thing"); + + let result = resolve_by_xpath(&yang_lib, &DatastoreName::Running, &filter, &empty_info()) + .expect("module name should resolve within the running datastore"); + + assert_eq!(result.len(), 1); + assert_eq!(result[0].name(), "foo-mod"); + assert_eq!(result[0].revision(), Some("2023-01-01")); + } + /// Huawei style: `xmlns` binding declared on the filter takes precedence /// over treating the prefix as a module name. #[test] From de80e562d0ce0c944b99331ac9dc8f4a2c5199c5 Mon Sep 17 00:00:00 2001 From: Leonardo Rodoni Date: Tue, 25 Aug 2026 17:45:30 +0200 Subject: [PATCH 3/7] refactor(netconf-proto): consolidate xpath helpers into new module Move find_xpath_prefixes out of xml_utils.rs (a broad, unrelated XML-parsing grab-bag) into a new xpath.rs module, the shared home for XPath 1.0 subset text utilities. Register the module in lib.rs and update its two call sites. Pure refactor, no behavior change. Sets up xpath.rs as the target for the normalize_path engine added next. --- crates/netconf-proto/src/lib.rs | 2 + crates/netconf-proto/src/xml_utils.rs | 227 +--------------- crates/netconf-proto/src/xpath.rs | 256 ++++++++++++++++++ crates/netconf-proto/src/yang_push/filters.rs | 2 +- 4 files changed, 260 insertions(+), 227 deletions(-) create mode 100644 crates/netconf-proto/src/xpath.rs diff --git a/crates/netconf-proto/src/lib.rs b/crates/netconf-proto/src/lib.rs index de62f629..60976c24 100644 --- a/crates/netconf-proto/src/lib.rs +++ b/crates/netconf-proto/src/lib.rs @@ -1,3 +1,4 @@ +// Copyright (C) 2026-present The NetCalyx Authors. // Copyright (C) 2025-present The NetGauze Authors. // // Licensed under the Apache License, Version 2.0 (the "License"); @@ -30,6 +31,7 @@ pub mod client; pub mod codec; pub mod protocol; pub mod xml_utils; +pub mod xpath; pub mod yang_push; pub mod yanglib; pub mod yangparser; diff --git a/crates/netconf-proto/src/xml_utils.rs b/crates/netconf-proto/src/xml_utils.rs index 7d671923..31b92a49 100644 --- a/crates/netconf-proto/src/xml_utils.rs +++ b/crates/netconf-proto/src/xml_utils.rs @@ -764,7 +764,7 @@ impl<'a, R: io::BufRead> XmlParser<'a, R> { all_namespaces.insert(prefix, String::from_utf8_lossy(ns).into_owned()); } let path = self.tag_string()?; - let used_namespaces = find_xpath_prefixes(&path); + let used_namespaces = crate::xpath::find_xpath_prefixes(&path); let namespaces: IndexMap = all_namespaces .into_iter() .filter(|(prefix, _)| used_namespaces.contains(prefix)) @@ -813,61 +813,6 @@ impl<'a, R: io::BufRead> XmlParser<'a, R> { } } -/// Find the prefixes used within an Xpath expression (e.g. the `if` in -/// `/if:interfaces/if:interface`). String literals are skipped and axis -/// specifiers (`child::`) are not treated as prefixes. -pub(crate) fn find_xpath_prefixes(xpath: &str) -> HashSet { - let mut prefixes = HashSet::new(); - let mut chars = xpath.char_indices().peekable(); - let mut in_single = false; - let mut in_double = false; - - while let Some((i, c)) = chars.next() { - // Skip over string literals — colons inside them aren't prefixes. - if in_single { - if c == '\'' { - in_single = false; - } - continue; - } - if in_double { - if c == '"' { - in_double = false; - } - continue; - } - match c { - '\'' => in_single = true, - '"' => in_double = true, - c if c.is_ascii_alphabetic() || c == '_' => { - let start = i; - let mut end = i + c.len_utf8(); - while let Some(&(_, nc)) = chars.peek() { - if nc.is_ascii_alphanumeric() || nc == '_' || nc == '-' || nc == '.' { - chars.next(); - end += nc.len_utf8(); - } else { - break; - } - } - // A prefix is an NCName followed by exactly one ':' - // (two colons = axis specifier like `child::`). - if let Some(&(_, ':')) = chars.peek() { - let mut look = chars.clone(); - look.next(); - let is_axis = matches!(look.peek(), Some(&(_, ':'))); - if !is_axis { - prefixes.insert(xpath[start..end].to_string()); - chars.next(); // consume the ':' - } - } - } - _ => {} - } - } - prefixes -} - /// Format a `DateTime` as YANG `date-and-time` (RFC 3339, UTC). pub fn format_datetime(ts: &DateTime) -> String { format!( @@ -1469,176 +1414,6 @@ mod tests { assert!(xml_writer.ns_applied); } - fn set(items: [&str; N]) -> HashSet { - items.iter().map(|s| s.to_string()).collect() - } - - fn assert_prefixes(expr: &str, expected: HashSet) { - assert_eq!( - find_xpath_prefixes(expr), - expected, - "unexpected prefix set for: {expr}" - ); - } - - #[test] - fn test_find_xpath_prefixes_yields_empty_when_no_qnames_present() { - // Empty/whitespace input, unprefixed paths, pure numeric/operator - // expressions, the `current()` function, and bare node tests all - // contain no QNames — so nothing should be reported. - for expr in [ - "", - " \n\t", - "/interfaces/interface/name", - "1 + 2.5 - 3 <= 4 and 5 != 6", - "current()", - "node() | text() | comment() | processing-instruction()", - ] { - assert_prefixes(expr, HashSet::new()); - } - } - - #[test] - fn test_find_xpath_prefixes_test_extracts_prefixes_from_simple_location_paths() { - // Motivating Huawei debug case, RFC 8641 Figure 12 (`/ex:foo`), - // the subscribed-notifications `/int:interfaces` example, - // prefix deduplication, and multi-prefix paths. - let cases: &[(&str, HashSet)] = &[ - ( - "/debug:debug/debug:board-resouce-states/debug:board-resouce-state", - set(["debug"]), - ), - ("/ex:foo", set(["ex"])), - ("/int:interfaces", set(["int"])), - ("/if:interfaces/if:interface/if:name", set(["if"])), - ("/a:x/b:y/c:z", set(["a", "b", "c"])), - ]; - for (expr, expected) in cases { - assert_prefixes(expr, expected.clone()); - } - } - - #[test] - fn test_find_xpath_prefixes_recognizes_full_ncname_charset_in_prefixes() { - // NCName permits letters, digits, `_`, `-`, `.` - // (the last three may not start the name). - let cases: &[(&str, HashSet)] = &[ - // Hyphenated — common in OpenConfig. - ( - "/oc-if:interfaces/oc-if:interface[oc-if:name='eth0']", - set(["oc-if"]), - ), - // Dot in the middle (legal NCName, rare in practice). - ("/a.b:c", set(["a.b"])), - // Underscore-leading. - ("/_ns:leaf", set(["_ns"])), - ]; - for (expr, expected) in cases { - assert_prefixes(expr, expected.clone()); - } - } - - #[test] - fn test_find_xpath_prefixes_handles_prefixed_wildcards_and_attributes() { - let cases: &[(&str, HashSet)] = &[ - ("/ex:*", set(["ex"])), - ("//@ex:id", set(["ex"])), - ("/if:interface[@nc:operation='delete']", set(["if", "nc"])), - ]; - for (expr, expected) in cases { - assert_prefixes(expr, expected.clone()); - } - } - - #[test] - fn test_find_xpath_prefixes_xpath_axes_are_never_reported_as_prefixes() { - // Every XPath 1.0 axis name followed by `::` must be skipped, - // since the `::` is an axis separator rather than a prefix colon. - const AXES: &[&str] = &[ - "ancestor", - "ancestor-or-self", - "attribute", - "child", - "descendant", - "descendant-or-self", - "following", - "following-sibling", - "namespace", - "parent", - "preceding", - "preceding-sibling", - "self", - ]; - for axis in AXES { - assert_prefixes(&format!("{axis}::node()"), HashSet::new()); - } - // Axes can still coexist with real prefixes in the same expression. - assert_prefixes("descendant::if:interface/child::if:name", set(["if"])); - } - - #[test] - fn test_find_xpath_prefixes_skips_colons_inside_string_literals() { - // Single-quoted identityref comparisons (RFC 7950 §9.10), - // double-quoted variants, and mixed-quote expressions. - let cases: &[(&str, HashSet)] = &[ - ("../crypto = 'mc:aes'", HashSet::new()), - ("name() = \"ns:bogus\"", HashSet::new()), - ("@a:x = 'p:q' or @b:y = \"r:s\"", set(["a", "b"])), - ]; - for (expr, expected) in cases { - assert_prefixes(expr, expected.clone()); - } - } - - #[test] - fn test_find_xpath_prefixes_handles_compound_expressions() { - // Function calls, leafref-style predicates with current(), - // unions, boolean ops across modules, and nested predicates. - let cases: &[(&str, HashSet)] = &[ - ("ex:size(@id)", set(["ex"])), - ( - "/if:interfaces/if:interface[if:name = current()/../if:name]", - set(["if"]), - ), - ("/a:foo | /b:bar", set(["a", "b"])), - ( - "(/if:interfaces/if:interface/if:enabled = 'true') \ - and count(/rt:routing/rt:routes) > 0", - set(["if", "rt"]), - ), - ("/a:x[a:y[b:z = '1']/a:w = c:fn()]", set(["a", "b", "c"])), - ]; - for (expr, expected) in cases { - assert_prefixes(expr, expected.clone()); - } - } - - #[test] - fn test_find_xpath_prefixes_real_world_yang_expressions() { - // ietf-interfaces-style `must`: only `if:` is a live prefix; - // the `ianaift:*` tokens are identityref values inside string - // literals and must not be reported. - let must_expr = "(/if:interfaces/if:interface[if:name=current()]/if:type \ - = 'ianaift:ethernetCsmacd') \ - or \ - (/if:interfaces/if:interface[if:name=current()]/if:type \ - = 'ianaift:ieee8023adLag')"; - assert_prefixes(must_expr, set(["if"])); - - // Multi-module subscriber filter for yp:datastore-xpath-filter. - let filter_expr = "/if:interfaces/if:interface[if:name='eth0'] \ - | /rt:routing/rt:ribs/rt:rib[rt:name=current()/ref:rib]"; - assert_prefixes(filter_expr, set(["if", "rt", "ref"])); - } - - #[test] - fn test_find_xpath_prefixes_whitespace_between_ncname_and_colon_breaks_qname() { - // In XPath 1.0 a QName is lexically `NCName ':' NCName` with no - // whitespace. `if : interfaces` is three tokens, so `if` must not - // be reported as a prefix. This behavior is intentional. - assert_prefixes(" / if : interfaces ", HashSet::new()); - } - #[test] fn test_read_xpath_with_namespaces_basic_filter() { // One prefix declared on the filter element, used in the path. diff --git a/crates/netconf-proto/src/xpath.rs b/crates/netconf-proto/src/xpath.rs new file mode 100644 index 00000000..681de446 --- /dev/null +++ b/crates/netconf-proto/src/xpath.rs @@ -0,0 +1,256 @@ +// Copyright (C) 2026-present The NetCalyx Authors. +// Copyright (C) 2026-present The NetGauze Authors. +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or +// implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +//! XPath 1.0 subset text utilities. +//! +//! These are pure, schema-agnostic string functions over the restricted +//! XPath 1.0 grammar used by NETCONF/YANG-Push filters (plain location paths +//! with implicit `child`-axis steps and simple predicates) — no dependency +//! on a YANG context or any particular filter type. They back +//! [`crate::yang_push::filters::DatastoreXPathFilter::path_prefixes`] and +//! [`crate::xml_utils::XmlParser::read_xpath_with_namespaces`]. + +use std::collections::HashSet; + +/// Find the prefixes used within an Xpath expression (e.g. the `if` in +/// `/if:interfaces/if:interface`). String literals are skipped and axis +/// specifiers (`child::`) are not treated as prefixes. +pub(crate) fn find_xpath_prefixes(xpath: &str) -> HashSet { + let mut prefixes = HashSet::new(); + let mut chars = xpath.char_indices().peekable(); + let mut in_single = false; + let mut in_double = false; + + while let Some((i, c)) = chars.next() { + // Skip over string literals — colons inside them aren't prefixes. + if in_single { + if c == '\'' { + in_single = false; + } + continue; + } + if in_double { + if c == '"' { + in_double = false; + } + continue; + } + match c { + '\'' => in_single = true, + '"' => in_double = true, + c if c.is_ascii_alphabetic() || c == '_' => { + let start = i; + let mut end = i + c.len_utf8(); + while let Some(&(_, nc)) = chars.peek() { + if nc.is_ascii_alphanumeric() || nc == '_' || nc == '-' || nc == '.' { + chars.next(); + end += nc.len_utf8(); + } else { + break; + } + } + // A prefix is an NCName followed by exactly one ':' + // (two colons = axis specifier like `child::`). + if let Some(&(_, ':')) = chars.peek() { + let mut look = chars.clone(); + look.next(); + let is_axis = matches!(look.peek(), Some(&(_, ':'))); + if !is_axis { + prefixes.insert(xpath[start..end].to_string()); + chars.next(); // consume the ':' + } + } + } + _ => {} + } + } + prefixes +} + +#[cfg(test)] +mod tests { + use super::*; + + fn set(items: [&str; N]) -> HashSet { + items.iter().map(|s| s.to_string()).collect() + } + + fn assert_prefixes(expr: &str, expected: HashSet) { + assert_eq!( + find_xpath_prefixes(expr), + expected, + "unexpected prefix set for: {expr}" + ); + } + + #[test] + fn test_find_xpath_prefixes_yields_empty_when_no_qnames_present() { + // Empty/whitespace input, unprefixed paths, pure numeric/operator + // expressions, the `current()` function, and bare node tests all + // contain no QNames — so nothing should be reported. + for expr in [ + "", + " \n\t", + "/interfaces/interface/name", + "1 + 2.5 - 3 <= 4 and 5 != 6", + "current()", + "node() | text() | comment() | processing-instruction()", + ] { + assert_prefixes(expr, HashSet::new()); + } + } + + #[test] + fn test_find_xpath_prefixes_test_extracts_prefixes_from_simple_location_paths() { + // Motivating Huawei debug case, RFC 8641 Figure 12 (`/ex:foo`), + // the subscribed-notifications `/int:interfaces` example, + // prefix deduplication, and multi-prefix paths. + let cases: &[(&str, HashSet)] = &[ + ( + "/debug:debug/debug:board-resouce-states/debug:board-resouce-state", + set(["debug"]), + ), + ("/ex:foo", set(["ex"])), + ("/int:interfaces", set(["int"])), + ("/if:interfaces/if:interface/if:name", set(["if"])), + ("/a:x/b:y/c:z", set(["a", "b", "c"])), + ]; + for (expr, expected) in cases { + assert_prefixes(expr, expected.clone()); + } + } + + #[test] + fn test_find_xpath_prefixes_recognizes_full_ncname_charset_in_prefixes() { + // NCName permits letters, digits, `_`, `-`, `.` + // (the last three may not start the name). + let cases: &[(&str, HashSet)] = &[ + // Hyphenated — common in OpenConfig. + ( + "/oc-if:interfaces/oc-if:interface[oc-if:name='eth0']", + set(["oc-if"]), + ), + // Dot in the middle (legal NCName, rare in practice). + ("/a.b:c", set(["a.b"])), + // Underscore-leading. + ("/_ns:leaf", set(["_ns"])), + ]; + for (expr, expected) in cases { + assert_prefixes(expr, expected.clone()); + } + } + + #[test] + fn test_find_xpath_prefixes_handles_prefixed_wildcards_and_attributes() { + let cases: &[(&str, HashSet)] = &[ + ("/ex:*", set(["ex"])), + ("//@ex:id", set(["ex"])), + ("/if:interface[@nc:operation='delete']", set(["if", "nc"])), + ]; + for (expr, expected) in cases { + assert_prefixes(expr, expected.clone()); + } + } + + #[test] + fn test_find_xpath_prefixes_xpath_axes_are_never_reported_as_prefixes() { + // Every XPath 1.0 axis name followed by `::` must be skipped, + // since the `::` is an axis separator rather than a prefix colon. + const AXES: &[&str] = &[ + "ancestor", + "ancestor-or-self", + "attribute", + "child", + "descendant", + "descendant-or-self", + "following", + "following-sibling", + "namespace", + "parent", + "preceding", + "preceding-sibling", + "self", + ]; + for axis in AXES { + assert_prefixes(&format!("{axis}::node()"), HashSet::new()); + } + // Axes can still coexist with real prefixes in the same expression. + assert_prefixes("descendant::if:interface/child::if:name", set(["if"])); + } + + #[test] + fn test_find_xpath_prefixes_skips_colons_inside_string_literals() { + // Single-quoted identityref comparisons (RFC 7950 §9.10), + // double-quoted variants, and mixed-quote expressions. + let cases: &[(&str, HashSet)] = &[ + ("../crypto = 'mc:aes'", HashSet::new()), + ("name() = \"ns:bogus\"", HashSet::new()), + ("@a:x = 'p:q' or @b:y = \"r:s\"", set(["a", "b"])), + ]; + for (expr, expected) in cases { + assert_prefixes(expr, expected.clone()); + } + } + + #[test] + fn test_find_xpath_prefixes_handles_compound_expressions() { + // Function calls, leafref-style predicates with current(), + // unions, boolean ops across modules, and nested predicates. + let cases: &[(&str, HashSet)] = &[ + ("ex:size(@id)", set(["ex"])), + ( + "/if:interfaces/if:interface[if:name = current()/../if:name]", + set(["if"]), + ), + ("/a:foo | /b:bar", set(["a", "b"])), + ( + "(/if:interfaces/if:interface/if:enabled = 'true') \ + and count(/rt:routing/rt:routes) > 0", + set(["if", "rt"]), + ), + ("/a:x[a:y[b:z = '1']/a:w = c:fn()]", set(["a", "b", "c"])), + ]; + for (expr, expected) in cases { + assert_prefixes(expr, expected.clone()); + } + } + + #[test] + fn test_find_xpath_prefixes_real_world_yang_expressions() { + // ietf-interfaces-style `must`: only `if:` is a live prefix; + // the `ianaift:*` tokens are identityref values inside string + // literals and must not be reported. + let must_expr = "(/if:interfaces/if:interface[if:name=current()]/if:type \ + = 'ianaift:ethernetCsmacd') \ + or \ + (/if:interfaces/if:interface[if:name=current()]/if:type \ + = 'ianaift:ieee8023adLag')"; + assert_prefixes(must_expr, set(["if"])); + + // Multi-module subscriber filter for yp:datastore-xpath-filter. + let filter_expr = "/if:interfaces/if:interface[if:name='eth0'] \ + | /rt:routing/rt:ribs/rt:rib[rt:name=current()/ref:rib]"; + assert_prefixes(filter_expr, set(["if", "rt", "ref"])); + } + + #[test] + fn test_find_xpath_prefixes_whitespace_between_ncname_and_colon_breaks_qname() { + // In XPath 1.0 a QName is lexically `NCName ':' NCName` with no + // whitespace. `if : interfaces` is three tokens, so `if` must not + // be reported as a prefix. This behavior is intentional. + assert_prefixes(" / if : interfaces ", HashSet::new()); + } +} diff --git a/crates/netconf-proto/src/yang_push/filters.rs b/crates/netconf-proto/src/yang_push/filters.rs index f2b226dd..8bae9478 100644 --- a/crates/netconf-proto/src/yang_push/filters.rs +++ b/crates/netconf-proto/src/yang_push/filters.rs @@ -359,7 +359,7 @@ impl DatastoreXPathFilter { /// `/if:interfaces/if:interface`), sorted for determinism. Axis specifiers /// and string literals are not treated as prefixes. pub fn path_prefixes(&self) -> Vec { - let mut prefixes: Vec = crate::xml_utils::find_xpath_prefixes(&self.path) + let mut prefixes: Vec = crate::xpath::find_xpath_prefixes(&self.path) .into_iter() .collect(); prefixes.sort_unstable(); From c45c5a9b9e1cc56bfb0510552b7ad4f18d942e1e Mon Sep 17 00:00:00 2001 From: Leonardo Rodoni Date: Tue, 25 Aug 2026 17:51:21 +0200 Subject: [PATCH 4/7] feat(netconf-proto): add xpath normalize_path engine Add DatastoreXPathFilter::normalize_path: converts an xpath to RFC 8641's canonical, module-name-qualified, prefix-on-change form (matches libyang's SchemaPathFormat::DATA), whether the source path uses declared xmlns prefixes or bare module-name prefixes. Backed by three new pure helpers in xpath.rs: split_location_path, parse_node_test, is_ncname. Bails to None (caller keeps the original path) for unsupported XPath 1.0 constructs or an unresolvable declared prefix. Not wired into any caller yet. --- crates/netconf-proto/src/xpath.rs | 111 +++++ crates/netconf-proto/src/yang_push/filters.rs | 221 ++++++++++ crates/netconf-proto/src/yang_push/tests.rs | 380 +++++++++++++++++- 3 files changed, 698 insertions(+), 14 deletions(-) diff --git a/crates/netconf-proto/src/xpath.rs b/crates/netconf-proto/src/xpath.rs index 681de446..d7c184c1 100644 --- a/crates/netconf-proto/src/xpath.rs +++ b/crates/netconf-proto/src/xpath.rs @@ -80,10 +80,121 @@ pub(crate) fn find_xpath_prefixes(xpath: &str) -> HashSet { prefixes } +/// Split an XPath 1.0 location path into `/`-separated steps, honoring +/// bracketed predicates and quoted strings so a `/` inside `[...]` or a +/// string literal is not mistaken for a step separator. Returns `None` if +/// brackets or quotes are unbalanced. +pub(crate) fn split_location_path(path: &str) -> Option> { + let mut segments = Vec::new(); + let mut depth: i32 = 0; + let mut in_single = false; + let mut in_double = false; + let mut start = 0usize; + for (i, c) in path.char_indices() { + match c { + '\'' if !in_double => in_single = !in_single, + '"' if !in_single => in_double = !in_double, + '[' if !in_single && !in_double => depth += 1, + ']' if !in_single && !in_double => { + depth -= 1; + if depth < 0 { + return None; + } + } + '/' if depth == 0 && !in_single && !in_double => { + segments.push(&path[start..i]); + start = i + 1; + } + _ => {} + } + } + if depth != 0 || in_single || in_double { + return None; + } + segments.push(&path[start..]); + Some(segments) +} + +/// Parse a node test of the form `(prefix ':')? (NCName | '*')`, returning +/// `(prefix, local)`. Returns `None` for anything else (functions, axes, `@`, +/// `.`/`..`, embedded whitespace), which signals an unsupported path. +pub(crate) fn parse_node_test(head: &str) -> Option<(Option<&str>, &str)> { + let head = head.trim(); + if head.is_empty() { + return None; + } + let (prefix, local) = match head.split_once(':') { + Some((p, l)) => (Some(p), l), + None => (None, head), + }; + if let Some(p) = prefix + && !is_ncname(p) + { + return None; + } + if local != "*" && !is_ncname(local) { + return None; + } + Some((prefix, local)) +} + +/// Whether `s` is a YANG/XML NCName: a leading letter or `_`, followed by +/// letters, digits, `_`, `-`, or `.`. +fn is_ncname(s: &str) -> bool { + let mut chars = s.chars(); + match chars.next() { + Some(c) if c.is_ascii_alphabetic() || c == '_' => {} + _ => return false, + } + chars.all(|c| c.is_ascii_alphanumeric() || c == '_' || c == '-' || c == '.') +} + #[cfg(test)] mod tests { use super::*; + #[test] + fn test_split_location_path_splits_on_slash_outside_brackets_and_quotes() { + assert_eq!( + split_location_path("/a/b[c='/']/d"), + Some(vec!["", "a", "b[c='/']", "d"]) + ); + } + + #[test] + fn test_split_location_path_rejects_unbalanced_brackets_or_quotes() { + assert_eq!(split_location_path("/a[b"), None); + assert_eq!(split_location_path("/a]"), None); + assert_eq!(split_location_path("/a[b='c]"), None); + } + + #[test] + fn test_parse_node_test_accepts_prefixed_names_and_wildcards() { + assert_eq!( + parse_node_test("if:interface"), + Some((Some("if"), "interface")) + ); + assert_eq!(parse_node_test("*"), Some((None, "*"))); + assert_eq!(parse_node_test("if:*"), Some((Some("if"), "*"))); + } + + #[test] + fn test_parse_node_test_rejects_functions_axes_and_special_steps() { + for head in ["current()", "node()", "@id", ".", "..", "if : interface"] { + assert_eq!(parse_node_test(head), None, "should reject `{head}`"); + } + } + + #[test] + fn test_is_ncname_accepts_valid_and_rejects_invalid_names() { + for valid in ["if", "_ns", "oc-if", "a.b", "a1"] { + assert!(is_ncname(valid), "should accept `{valid}`"); + } + for invalid in ["", "1if", "-if", ".if", "if:name", "if name"] { + assert!(!is_ncname(invalid), "should reject `{invalid}`"); + } + } + fn set(items: [&str; N]) -> HashSet { items.iter().map(|s| s.to_string()).collect() } diff --git a/crates/netconf-proto/src/yang_push/filters.rs b/crates/netconf-proto/src/yang_push/filters.rs index 8bae9478..397a024c 100644 --- a/crates/netconf-proto/src/yang_push/filters.rs +++ b/crates/netconf-proto/src/yang_push/filters.rs @@ -365,6 +365,227 @@ impl DatastoreXPathFilter { prefixes.sort_unstable(); prefixes } + + /// Look up the namespace URI declared for `prefix` on this filter. + fn namespace_uri(&self, prefix: &str) -> Option<&str> { + self.namespaces + .iter() + .find(|(p, _)| p.as_ref() == prefix) + .map(|(_, uri)| uri.as_ref()) + } + + /// Normalize `path` to RFC 8641's base XPath context: + /// module-name-qualified, prefix emitted only on module change (matches + /// libyang's canonical schema path format). E.g. + /// `/debug:debug/debug:board-resouce-state` (xmlns-prefixed) and + /// `/huawei-debug:debug/board-resouce-state` (module-name prefixed) + /// both normalize to the latter. + /// + /// How a step's module is determined, in order: if its prefix has a + /// declared `xmlns` binding, that namespace URI is resolved to a module + /// name via `resolve_module`; if the prefix is undeclared, the prefix + /// text is itself already the module name; if the step has no prefix at + /// all, it inherits the module of the preceding step. Whatever module is + /// found is only written back out as a prefix when it differs from the + /// previous step's — that's the "prefix on change" part. Predicates + /// (`[...]`) get the same per-token treatment for any `prefix:name` found + /// in their text (string literals are left alone), except the prefix is + /// dropped instead of kept when it matches the *enclosing* step's module. + /// + /// Only a single, plain location path with implicit `child`-axis steps is + /// supported (not the full XPath 1.0 grammar — no functions, unions, or + /// explicit axes). The result always starts with `/` (inserted if + /// missing): per RFC 8641 the context node is always the datastore root. + /// + /// Returns `None` — keep the original path — for unsupported constructs + /// or a declared prefix `resolve_module` can't map. Idempotent. + pub fn normalize_path(&self, resolve_module: F) -> Option + where + F: Fn(&str) -> Option>, + { + let path = self.path.trim(); + if path.is_empty() { + return None; + } + let segments = crate::xpath::split_location_path(path)?; + let mut out = String::with_capacity(path.len() + 1); + let mut current_module: Option> = None; + for (i, seg) in segments.iter().enumerate() { + if i > 0 { + out.push('/'); + } + // Empty segment: leading '/' (absolute) or '//' (descendant). + if seg.is_empty() { + continue; + } + // Split the node test from any trailing predicate(s). + let head_end = seg.find('[').unwrap_or(seg.len()); + let (prefix, local) = crate::xpath::parse_node_test(&seg[..head_end])?; + let predicates = &seg[head_end..]; + + let module: Option> = match prefix { + Some(p) => { + if let Some(uri) = self.namespace_uri(p) { + Some(resolve_module(uri)?) + } else { + Some(p.into()) + } + } + // Bare wildcard cannot be module-qualified; leave it as-is. + None if local == "*" => None, + None => current_module.clone(), + }; + match module { + Some(m) => { + if current_module.as_deref() != Some(m.as_ref()) { + out.push_str(&m); + out.push(':'); + current_module = Some(m); + } + out.push_str(local); + } + None => out.push_str(local), + } + if predicates.is_empty() { + continue; + } + let rewritten = self.rewrite_predicate_prefixes( + predicates, + current_module.as_deref(), + &resolve_module, + )?; + out.push_str(&rewritten); + } + if !out.starts_with('/') { + out.insert(0, '/'); + } + Some(out) + } + + /// Rewrite `prefix:name` tokens found in predicate text (`[...]`), + /// mirroring the module resolution applied to location steps in + /// [`Self::normalize_path`]: a declared prefix is mapped to its module via + /// `resolve_module`; an undeclared prefix is itself the module name. The + /// prefix is then dropped if the resolved module matches + /// `enclosing_module` (the module of the step the predicate is attached + /// to), or rewritten to the resolved module name otherwise. + /// + /// String-literal content (`'...'` / `"..."`) is skipped verbatim — a + /// `prefix:name`-shaped substring inside a literal (e.g. an identityref + /// value) is data, not a reference, and must not be touched. This is a + /// quote-aware tokenizer, not a full XPath expression parser: any bare + /// `NCName ':' NCName` (or `NCName ':' '*'`) outside a literal and not + /// part of an axis specifier (`::`) is treated as a prefixed name. + /// + /// Returns `None` if a `:` is found outside a literal that isn't part of + /// a well-formed prefixed name or axis specifier, or if a prefix with a + /// declared `xmlns` binding can't be resolved to a module — same + /// bail-and-keep-original contract as the rest of `normalize_path`. + fn rewrite_predicate_prefixes( + &self, + predicate: &str, + enclosing_module: Option<&str>, + resolve_module: &F, + ) -> Option + where + F: Fn(&str) -> Option>, + { + let mut out = String::with_capacity(predicate.len()); + let mut chars = predicate.char_indices().peekable(); + let mut in_single = false; + let mut in_double = false; + while let Some((i, c)) = chars.next() { + if in_single { + out.push(c); + if c == '\'' { + in_single = false; + } + continue; + } + if in_double { + out.push(c); + if c == '"' { + in_double = false; + } + continue; + } + match c { + '\'' => { + in_single = true; + out.push(c); + } + '"' => { + in_double = true; + out.push(c); + } + c if c.is_ascii_alphabetic() || c == '_' => { + let start = i; + let mut end = i + c.len_utf8(); + while let Some(&(_, nc)) = chars.peek() { + if nc.is_ascii_alphanumeric() || nc == '_' || nc == '-' || nc == '.' { + chars.next(); + end += nc.len_utf8(); + } else { + break; + } + } + let ident = &predicate[start..end]; + // A prefix is an NCName followed by exactly one ':' + // (two colons = axis specifier like `child::`). + let Some(&(_, ':')) = chars.peek() else { + out.push_str(ident); + continue; + }; + let mut look = chars.clone(); + look.next(); + if matches!(look.peek(), Some(&(_, ':'))) { + out.push_str(ident); + continue; + } + chars.next(); // consume the ':' + let (local_start, mut local_end) = match chars.peek() { + Some(&(li, '*')) => { + chars.next(); + (li, li + 1) + } + Some(&(li, lc)) if lc.is_ascii_alphabetic() || lc == '_' => { + chars.next(); + (li, li + lc.len_utf8()) + } + // ':' not followed by a valid NCName or '*' — unsupported. + _ => return None, + }; + // Wildcard local names (`prefix:*`) have no further + // characters to scan; NCName locals continue below. + if !predicate[local_start..].starts_with('*') { + while let Some(&(_, nc)) = chars.peek() { + if nc.is_ascii_alphanumeric() || nc == '_' || nc == '-' || nc == '.' { + chars.next(); + local_end += nc.len_utf8(); + } else { + break; + } + } + } + let local = &predicate[local_start..local_end]; + let module: Box = if let Some(uri) = self.namespace_uri(ident) { + resolve_module(uri)? + } else { + ident.into() + }; + if enclosing_module == Some(module.as_ref()) { + out.push_str(local); + } else { + out.push_str(&module); + out.push(':'); + out.push_str(local); + } + } + _ => out.push(c), + } + } + Some(out) + } } impl XmlSerialize for DatastoreXPathFilter { diff --git a/crates/netconf-proto/src/yang_push/tests.rs b/crates/netconf-proto/src/yang_push/tests.rs index 7eed95d7..ed75933c 100644 --- a/crates/netconf-proto/src/yang_push/tests.rs +++ b/crates/netconf-proto/src/yang_push/tests.rs @@ -784,29 +784,381 @@ fn test_yang_push_module_version_json_serde() { assert_eq!(deserialized, modeled); } +/// A prefix with no `xmlns` binding declared equals the module name. #[test] -fn test_datastore_xpath_filter_path_prefixes() { - // Cisco IOS-XR: prefix equals the module name, no xmlns binding declared. - let cisco = DatastoreXPathFilter { +fn test_datastore_xpath_filter_path_prefixes_module_name_prefix() { + let filter = DatastoreXPathFilter { namespaces: Box::new([]), - path: "Cisco-IOS-XR-procmem-oper:processes-memory/nodes/node/process-ids/process-id".into(), + path: "example-procmem-oper:processes-memory/nodes/node/process-ids/process-id".into(), }; - assert_eq!(cisco.path_prefixes(), vec!["Cisco-IOS-XR-procmem-oper"]); + assert_eq!(filter.path_prefixes(), vec!["example-procmem-oper"]); +} - // Multi-module xpath (Huawei-style), distinct prefixes (sorted). - let multi = DatastoreXPathFilter { +/// Distinct prefixes across a multi-module path are returned sorted. +#[test] +fn test_datastore_xpath_filter_path_prefixes_multi_module_are_sorted() { + let filter = DatastoreXPathFilter { namespaces: Box::new([ - ("devm".into(), "urn:huawei:yang:huawei-devm".into()), - ("driver".into(), "urn:huawei:yang:huawei-driver".into()), + ("a".into(), "urn:example:yang:example-a".into()), + ("b".into(), "urn:example:yang:example-b".into()), ]), - path: "/devm:devm/devm:chassiss/devm:chassis/driver:power-supply-attribute".into(), + path: "/b:root/b:child/b:leaf/a:sibling".into(), }; - assert_eq!(multi.path_prefixes(), vec!["devm", "driver"]); + assert_eq!(filter.path_prefixes(), vec!["a", "b"]); +} - // Unprefixed (default-namespace) steps contribute no prefixes. - let unprefixed = DatastoreXPathFilter { +/// Unprefixed (default-namespace) steps contribute no prefixes. +#[test] +fn test_datastore_xpath_filter_path_prefixes_unprefixed_steps_are_empty() { + let filter = DatastoreXPathFilter { namespaces: Box::new([]), path: "/interfaces/interface/state/counters".into(), }; - assert!(unprefixed.path_prefixes().is_empty()); + assert!(filter.path_prefixes().is_empty()); +} + +/// A declared `xmlns` binding and an undeclared, module-name-as-prefix +/// encoding of the same target must converge onto the same canonical form. +#[test] +fn test_datastore_xpath_filter_normalize_path_converges_declared_and_undeclared_prefixes() { + let resolve = |uri: &str| -> Option> { + match uri { + "urn:example:yang:example-debug" => Some("example-debug".into()), + _ => None, + } + }; + + let declared = DatastoreXPathFilter { + namespaces: Box::new([("debug".into(), "urn:example:yang:example-debug".into())]), + path: "/debug:debug/debug:board-state".into(), + }; + let undeclared = DatastoreXPathFilter { + namespaces: Box::new([]), + path: "/example-debug:debug/board-state".into(), + }; + let canonical = "/example-debug:debug/board-state"; + assert_eq!(declared.normalize_path(resolve).as_deref(), Some(canonical)); + assert_eq!( + undeclared.normalize_path(resolve).as_deref(), + Some(canonical), + ); +} + +/// Normalizing an already-canonical form yields itself. +#[test] +fn test_datastore_xpath_filter_normalize_path_is_idempotent() { + let resolve = |_: &str| -> Option> { None }; + let canonical = "/example-debug:debug/board-state"; + let already = DatastoreXPathFilter { + namespaces: Box::new([]), + path: canonical.into(), + }; + assert_eq!(already.normalize_path(resolve).as_deref(), Some(canonical)); +} + +/// A module-qualifying prefix is only emitted when the module changes. +#[test] +fn test_datastore_xpath_filter_normalize_path_multi_module_emits_prefix_only_on_change() { + let resolve = |uri: &str| -> Option> { + match uri { + "urn:example:yang:example-a" => Some("example-a".into()), + "urn:example:yang:example-b" => Some("example-b".into()), + _ => None, + } + }; + + let multi = DatastoreXPathFilter { + namespaces: Box::new([ + ("a".into(), "urn:example:yang:example-a".into()), + ("b".into(), "urn:example:yang:example-b".into()), + ]), + path: "/a:root/a:child/a:leaf/b:sibling".into(), + }; + assert_eq!( + multi.normalize_path(resolve).as_deref(), + Some("/example-a:root/child/leaf/example-b:sibling"), + ); +} + +/// With no `xmlns` binding declared, an undeclared prefix is itself the +/// module name (the base XPath context), and a relative path is made +/// absolute. +#[test] +fn test_datastore_xpath_filter_normalize_path_module_name_prefix_is_made_absolute() { + let resolve = |_: &str| -> Option> { None }; + + let relative = DatastoreXPathFilter { + namespaces: Box::new([]), + path: "example-oper:processes/process/pids/pid".into(), + }; + let canonical = "/example-oper:processes/process/pids/pid"; + assert_eq!(relative.normalize_path(resolve).as_deref(), Some(canonical)); + + // An already-absolute equivalent normalizes identically. + let absolute = DatastoreXPathFilter { + namespaces: Box::new([]), + path: canonical.into(), + }; + assert_eq!(absolute.normalize_path(resolve).as_deref(), Some(canonical)); +} + +/// A predicate's own prefix is resolved the same as a step's: dropped +/// entirely when it matches the enclosing step's module (redundant, per RFC +/// 7950 §6.4.1 unprefixed-name-inherits-context-node rule), and a '/' inside +/// the predicate's value must not be mistaken for a path separator. +#[test] +fn test_datastore_xpath_filter_normalize_path_drops_redundant_predicate_prefix() { + let resolve = |uri: &str| -> Option> { + match uri { + "urn:example:yang:example-debug" => Some("example-debug".into()), + _ => None, + } + }; + + let with_pred = DatastoreXPathFilter { + namespaces: Box::new([("debug".into(), "urn:example:yang:example-debug".into())]), + path: "/debug:debug/debug:board-state[debug:id='1/2']".into(), + }; + assert_eq!( + with_pred.normalize_path(resolve).as_deref(), + Some("/example-debug:debug/board-state[id='1/2']"), + ); +} + +/// A predicate prefix that resolves to a *different* module than the +/// enclosing step is canonicalized to the resolved module name, not dropped. +#[test] +fn test_datastore_xpath_filter_normalize_path_predicate_prefix_different_module_is_canonicalized() { + let resolve = |uri: &str| -> Option> { + match uri { + "urn:ietf:params:xml:ns:yang:ietf-interfaces" => Some("ietf-interfaces".into()), + "urn:example:oper-ext" => Some("example-oper-ext".into()), + _ => None, + } + }; + + let filter = DatastoreXPathFilter { + namespaces: Box::new([ + ( + "if".into(), + "urn:ietf:params:xml:ns:yang:ietf-interfaces".into(), + ), + ("ext".into(), "urn:example:oper-ext".into()), + ]), + path: "/if:interfaces/if:interface[ext:tag='x']".into(), + }; + assert_eq!( + filter.normalize_path(resolve).as_deref(), + Some("/ietf-interfaces:interfaces/interface[example-oper-ext:tag='x']"), + ); +} + +/// A predicate prefix with a declared `xmlns` binding whose namespace cannot +/// be resolved to a module bails (`None`), same as an unresolvable step +/// prefix. +#[test] +fn test_datastore_xpath_filter_normalize_path_unresolvable_predicate_prefix_bails() { + let resolve = |uri: &str| -> Option> { + match uri { + "urn:ietf:params:xml:ns:yang:ietf-interfaces" => Some("ietf-interfaces".into()), + _ => None, + } + }; + + let filter = DatastoreXPathFilter { + namespaces: Box::new([ + ( + "if".into(), + "urn:ietf:params:xml:ns:yang:ietf-interfaces".into(), + ), + ("unknown".into(), "urn:unresolvable".into()), + ]), + path: "/if:interfaces/if:interface[unknown:name='eth0']".into(), + }; + assert_eq!(filter.normalize_path(resolve), None); +} + +/// Reproduces the 6wind sub 303 shape: a fully-prefix-qualified path where +/// the predicate's prefix is the same module as the enclosing step and must +/// be dropped, same as the outer steps' redundant prefixes are dropped. +#[test] +fn test_datastore_xpath_filter_normalize_path_sub_303_redundant_predicate_prefix() { + let resolve = |uri: &str| -> Option> { + match uri { + "urn:6wind:vrouter" => Some("vrouter".into()), + "urn:6wind:vrouter/interface" => Some("vrouter-interface".into()), + _ => None, + } + }; + + let filter = DatastoreXPathFilter { + namespaces: Box::new([ + ("vrouter".into(), "urn:6wind:vrouter".into()), + ("vrouter-interface".into(), "urn:6wind:vrouter/interface".into()), + ]), + path: "/vrouter:state/vrouter:vrf/vrouter-interface:interface/vrouter-interface:physical[vrouter-interface:name='ens192']/vrouter-interface:oper-status".into(), + }; + assert_eq!( + filter.normalize_path(resolve).as_deref(), + Some("/vrouter:state/vrf/vrouter-interface:interface/physical[name='ens192']/oper-status"), + ); +} + +/// A declared prefix whose namespace cannot be resolved bails (`None`). +#[test] +fn test_datastore_xpath_filter_normalize_path_unresolvable_prefix_bails() { + let resolve = |_: &str| -> Option> { None }; + + let unresolvable = DatastoreXPathFilter { + namespaces: Box::new([("x".into(), "urn:unknown".into())]), + path: "/x:foo/x:bar".into(), + }; + assert_eq!(unresolvable.normalize_path(resolve), None); +} + +/// Unsupported XPath constructs bail (`None`); the caller keeps the original. +#[test] +fn test_datastore_xpath_filter_normalize_path_unsupported_constructs_bail() { + let resolve = |_: &str| -> Option> { None }; + + // some unsupported xpath examples + for path in [ + "count(/if:interfaces) > 0", + "/a:x | /b:y", + "descendant::if:name", + ] { + let f = DatastoreXPathFilter { + namespaces: Box::new([]), + path: path.into(), + }; + assert_eq!(f.normalize_path(resolve), None, "should bail for `{path}`"); + } +} + +/// Instantiated (real key value) predicate containing '/' must not be +/// mistaken for a path separator. +#[test] +fn test_datastore_xpath_filter_normalize_path_instantiated_predicate_with_slash() { + let resolve = |_: &str| -> Option> { None }; + + let filter = DatastoreXPathFilter { + namespaces: Box::new([]), + path: "openconfig-interfaces:interfaces/interface[name='TenGigE0/0/0/14']".into(), + }; + assert_eq!( + filter.normalize_path(resolve).as_deref(), + Some("/openconfig-interfaces:interfaces/interface[name='TenGigE0/0/0/14']"), + ); +} + +/// A nested (multi-step) predicate path with a module-qualified identityref +/// value; both are preserved verbatim. +#[test] +fn test_datastore_xpath_filter_normalize_path_instantiated_predicate_with_nested_path() { + let resolve = |_: &str| -> Option> { None }; + + let filter = DatastoreXPathFilter { + namespaces: Box::new([]), + path: + "openconfig-interfaces:interfaces/interface[state/type='iana-if-type:ethernetCsmacd']" + .into(), + }; + assert_eq!( + filter.normalize_path(resolve).as_deref(), + Some( + "/openconfig-interfaces:interfaces/interface[state/type='iana-if-type:ethernetCsmacd']" + ), + ); +} + +/// A declared `xmlns` binding combined with an instantiated predicate value: +/// the predicate's own `if:` prefix resolves to the same module +/// (`ietf-interfaces`) as the enclosing `interface` step, so it's dropped. +#[test] +fn test_datastore_xpath_filter_normalize_path_declared_namespace_with_instantiated_predicate() { + let resolve = |uri: &str| -> Option> { + match uri { + "urn:ietf:params:xml:ns:yang:ietf-interfaces" => Some("ietf-interfaces".into()), + _ => None, + } + }; + + let filter = DatastoreXPathFilter { + namespaces: Box::new([( + "if".into(), + "urn:ietf:params:xml:ns:yang:ietf-interfaces".into(), + )]), + path: "/if:interfaces/if:interface[if:name='GigabitEthernet0/0/0']".into(), + }; + assert_eq!( + filter.normalize_path(resolve).as_deref(), + Some("/ietf-interfaces:interfaces/interface[name='GigabitEthernet0/0/0']"), + ); +} + +/// A declared binding that only appears inside a predicate *string literal* +/// value must not affect module tracking on the location path, and the +/// literal's content is left untouched (it's data, not a reference) even +/// though it's shaped like a `prefix:name` token. The predicate's own, +/// non-literal `if:type` prefix is still resolved/dropped like any other. +#[test] +fn test_datastore_xpath_filter_normalize_path_unused_namespace_binding_in_predicate() { + let resolve = |uri: &str| -> Option> { + match uri { + "urn:ietf:params:xml:ns:yang:ietf-interfaces" => Some("ietf-interfaces".into()), + _ => None, + } + }; + + let filter = DatastoreXPathFilter { + namespaces: Box::new([ + ( + "if".into(), + "urn:ietf:params:xml:ns:yang:ietf-interfaces".into(), + ), + ( + "iana-if-type".into(), + "urn:ietf:params:xml:ns:yang:iana-if-type".into(), + ), + ]), + path: "/if:interfaces/if:interface[if:type='iana-if-type:ethernetCsmacd']".into(), + }; + assert_eq!( + filter.normalize_path(resolve).as_deref(), + Some("/ietf-interfaces:interfaces/interface[type='iana-if-type:ethernetCsmacd']"), + ); +} + +/// A multi-module path (a step from an augmenting module mid-path) combined +/// with an instantiated predicate value containing both '/' and ':'; the +/// predicate's `if:` prefix resolves to the same module as the enclosing +/// `interface` step and is dropped, while the following `ext:` step (a +/// different module) keeps its prefix as usual. +#[test] +fn test_datastore_xpath_filter_normalize_path_multi_module_with_instantiated_predicate() { + let resolve = |uri: &str| -> Option> { + match uri { + "urn:ietf:params:xml:ns:yang:ietf-interfaces" => Some("ietf-interfaces".into()), + "urn:example:oper-ext" => Some("example-oper-ext".into()), + _ => None, + } + }; + + let filter = DatastoreXPathFilter { + namespaces: Box::new([ + ( + "if".into(), + "urn:ietf:params:xml:ns:yang:ietf-interfaces".into(), + ), + ("ext".into(), "urn:example:oper-ext".into()), + ]), + path: "/if:interfaces/if:interface[if:name='TenGigE0/0/0/14']/ext:oper-status-detail" + .into(), + }; + assert_eq!( + filter.normalize_path(resolve).as_deref(), + Some( + "/ietf-interfaces:interfaces/interface[name='TenGigE0/0/0/14']/example-oper-ext:oper-status-detail" + ), + ); } From cfda43bf61f5ea7e14297288c38193de1d6f38d6 Mon Sep 17 00:00:00 2001 From: Leonardo Rodoni Date: Tue, 25 Aug 2026 17:54:00 +0200 Subject: [PATCH 5/7] feat(yang-push): normalize NETCONF-sourced target xpath filter Apply DatastoreXPathFilter::normalize_path to the datastore xpath filter fetched via get_yang_push_subscription_by_id, so the cached target is always in canonical module-name-qualified form regardless of how the device encoded prefixes (declared xmlns vs. bare module name). Falls back to the original path (with a warning) when the path can't be confidently normalized. --- crates/yang-push/src/cache/fetcher.rs | 35 ++++++++++++++++++++++++++- 1 file changed, 34 insertions(+), 1 deletion(-) diff --git a/crates/yang-push/src/cache/fetcher.rs b/crates/yang-push/src/cache/fetcher.rs index 60af0b25..78edcf64 100644 --- a/crates/yang-push/src/cache/fetcher.rs +++ b/crates/yang-push/src/cache/fetcher.rs @@ -387,7 +387,7 @@ impl NetconfYangLibraryFetcher { } }; - let subscription = client + let mut subscription = client .get_yang_push_subscription_by_id(subscription_id) .await .map_err(|err| Box::new((empty.clone(), err.into())))?; @@ -509,6 +509,39 @@ impl NetconfYangLibraryFetcher { warn!(host=%host, error=%err, "Timeout while closing SSH connection") } } + // Normalize the target xpath filter to the canonical module-name, + // prefix-on-change form so the downstream pipeline sees one encoding + // regardless of how the device reported it (xmlns-declared prefixes vs + // module-name base context). + if let Target::Datastore(datastore_target) = &mut subscription.target + && let DatastoreSelectionFilterObjects::WithInSubscription(DatastoreFilterSpec::Xpath( + xpath, + )) = &mut datastore_target.selection + { + match xpath.normalize_path(|uri| { + router_yang_library + .find_module_by_datastore_and_ns(&datastore_target.datastore, uri) + .map(|m| m.name().into()) + }) { + Some(normalized) => { + if normalized.as_str() != xpath.path.as_ref() { + debug!( + subscription_id, + from = %xpath.path, + to = %normalized, + "normalized target xpath filter", + ); + } + xpath.path = normalized.into_boxed_str(); + xpath.namespaces = Box::new([]); + } + None => warn!( + subscription_id, + path = %xpath.path, + "could not normalize target xpath filter, keeping original", + ), + } + } let subscription_target = subscription.target.try_into().map_err(|err| { Box::new(( empty, From a041df5a1d243b20bf5d6412e75b7ef074670e6d Mon Sep 17 00:00:00 2001 From: Leonardo Rodoni Date: Tue, 25 Aug 2026 17:57:51 +0200 Subject: [PATCH 6/7] feat(yang-push): normalize JSON-sourced target xpath filter JSON-encoded SubscriptionStarted/Modified notifications carry their datastore-xpath-filter as a plain string with no xmlns table. Normalize it the same way as the NETCONF/XML path, in build_subscription_info, via normalize_json_target_xpath. Needs no schema access: passing an empty namespace table makes normalize_path treat every prefix as already-resolved. Reduces noise in the canonical-form diagnostic and avoids spurious subscription-changed refetches from pure prefix-style variance. --- crates/yang-push/src/validation/mod.rs | 121 ++++++++++++++++++++++++- 1 file changed, 119 insertions(+), 2 deletions(-) diff --git a/crates/yang-push/src/validation/mod.rs b/crates/yang-push/src/validation/mod.rs index a8bdb74d..4fe5c25d 100644 --- a/crates/yang-push/src/validation/mod.rs +++ b/crates/yang-push/src/validation/mod.rs @@ -125,10 +125,13 @@ use crate::{ ContentId, OTL_YANG_PUSH_SUBSCRIPTION_ID_KEY, OTL_YANG_PUSH_SUBSCRIPTION_ROUTER_CONTENT_ID_KEY, OTL_YANG_PUSH_SUBSCRIPTION_TARGET_KEY, }; +use netcalyx_netconf_proto::yang_push::filters::DatastoreXPathFilter; use netcalyx_netconf_proto::yang_push::subscription::YangPushModuleVersion; use netcalyx_netconf_proto::yang_push::types::SubscriptionId; use netcalyx_udp_notif_pkt::decoded::{UdpNotifPacketDecoded, UdpNotifPayload}; -use netcalyx_udp_notif_pkt::notification::{NotificationVariant, SubscriptionStartedModified}; +use netcalyx_udp_notif_pkt::notification::{ + NotificationVariant, SubscriptionStartedModified, Target, +}; use netcalyx_udp_notif_pkt::raw::UdpNotifPacket; use netcalyx_udp_notif_service::{OTL_UDP_NOTIF_PUBLISHER_ID_KEY, SessionInfo, UdpNotifRequest}; use rustc_hash::FxHashMap; @@ -1336,6 +1339,57 @@ impl ValidationActor { Ok(()) } + /// Normalize the inline `datastore-xpath-filter` carried by a JSON + /// `SubscriptionStarted`/`SubscriptionModified` notification to the same + /// module-name-qualified, prefix-on-change canonical form the fetcher + /// applies to NETCONF/XML-sourced targets (see + /// `DatastoreXPathFilter::normalize_path`). + /// + /// Unlike the XML case, JSON xpath strings never carry `xmlns` prefix + /// declarations — per RFC 7951/8641 a prefix in the path text is already + /// the module name itself. So this is a pure string transform: wrapping + /// the path in a `DatastoreXPathFilter` with an empty namespace table + /// makes every prefix fall into `normalize_path`'s "undeclared prefix is + /// the module name" branch, meaning `resolve_module` is never invoked and + /// no schema/YANG-library access is needed here. + /// + /// No-op if the target has no datastore xpath filter. Leaves the path + /// untouched (but logs a warning) if it can't be confidently normalized + /// (e.g. functions, unions, or other unsupported XPath 1.0 constructs). + fn normalize_json_target_xpath( + peer: SocketAddr, + subscription_id: SubscriptionId, + target: &mut Target, + ) { + let Some(original) = target.datastore_xpath_filter.as_deref() else { + return; + }; + let filter = DatastoreXPathFilter { + namespaces: Box::new([]), + path: original.into(), + }; + match filter.normalize_path(|_uri| None) { + Some(normalized) => { + if normalized != original { + debug!( + %peer, + subscription_id, + from = %original, + to = %normalized, + "normalized target xpath filter", + ); + } + target.datastore_xpath_filter = Some(normalized); + } + None => warn!( + %peer, + subscription_id, + path = %original, + "could not normalize target xpath filter, keeping original", + ), + } + } + /// Construct a `SubscriptionInfo` from a `SubscriptionStarted/Modified` /// notification. Returns `None` if module-version is absent. fn build_subscription_info( @@ -1368,10 +1422,13 @@ impl ValidationActor { } }; + let mut target = sub_started.target().clone(); + Self::normalize_json_target_xpath(peer, sub_started.id(), &mut target); + Some(SubscriptionInfo::new( peer.ip(), sub_started.id(), - sub_started.target().clone(), + target, sub_started.stop_time().cloned(), sub_started.transport().cloned(), sub_started.encoding().cloned(), @@ -2939,4 +2996,64 @@ mod tests { caching_handle.shutdown().await.unwrap(); caching_join_handle.await.unwrap().unwrap(); } + + /// A JSON `datastore-xpath-filter` with a redundant module prefix on + /// every step must be collapsed to the prefix-on-change canonical form, + /// same as the fetcher-side XML normalization. + #[test] + fn test_normalize_json_target_xpath_collapses_redundant_prefixes() { + let mut target = Target::new_datastore( + "ietf-datastores:operational".to_string(), + either::Right( + "/ietf-interfaces:interfaces/ietf-interfaces:interface[ietf-interfaces:name='eth0']/ietf-interfaces:oper-status" + .to_string(), + ), + ); + ValidationActor::normalize_json_target_xpath( + SocketAddr::from(([127, 0, 0, 1], 0)), + 1, + &mut target, + ); + assert_eq!( + target.datastore_xpath_filter.as_deref(), + Some("/ietf-interfaces:interfaces/interface[name='eth0']/oper-status") + ); + } + + /// An already-canonical JSON xpath must be left unchanged + /// (idempotent transform). + #[test] + fn test_normalize_json_target_xpath_idempotent_on_canonical_path() { + let mut target = Target::new_datastore( + "ietf-datastores:operational".to_string(), + either::Right("/ietf-interfaces:interfaces/interface".to_string()), + ); + ValidationActor::normalize_json_target_xpath( + SocketAddr::from(([127, 0, 0, 1], 0)), + 1, + &mut target, + ); + assert_eq!( + target.datastore_xpath_filter.as_deref(), + Some("/ietf-interfaces:interfaces/interface") + ); + } + + /// A target without a datastore xpath filter (e.g. a stream target) must + /// be left untouched. + #[test] + fn test_normalize_json_target_xpath_noop_without_xpath_filter() { + let mut target = Target::new_stream( + "NETCONF".to_string(), + None, + either::Left(serde_json::Value::Null), + ); + let before = target.clone(); + ValidationActor::normalize_json_target_xpath( + SocketAddr::from(([127, 0, 0, 1], 0)), + 1, + &mut target, + ); + assert_eq!(target, before); + } } From 6a66ed96e026d4b252a7689ddb98e5b251bfc703 Mon Sep 17 00:00:00 2001 From: Leonardo Rodoni Date: Tue, 25 Aug 2026 18:05:01 +0200 Subject: [PATCH 7/7] feat(yang-push): warn on non-canonical or unresolvable target xpath Add check_xpath_target_resolves: after a schema loads, evaluate the subscription's datastore-xpath-filter against the libyang context and warn if it does not resolve to a schema node, or resolves but isn't in libyang's canonical (SchemaPathFormat:: DATA) form. Diagnostic only, never mutates the target. Move xpath_diff and strip_xpath_predicates into netconf-proto's xpath module so they're reusable and directly testable. --- crates/netconf-proto/src/xpath.rs | 139 ++++++++++++++- crates/yang-push/src/validation/mod.rs | 234 ++++++++++++++++++++++++- 2 files changed, 365 insertions(+), 8 deletions(-) diff --git a/crates/netconf-proto/src/xpath.rs b/crates/netconf-proto/src/xpath.rs index d7c184c1..917b22e8 100644 --- a/crates/netconf-proto/src/xpath.rs +++ b/crates/netconf-proto/src/xpath.rs @@ -1,5 +1,4 @@ // Copyright (C) 2026-present The NetCalyx Authors. -// Copyright (C) 2026-present The NetGauze Authors. // // Licensed under the Apache License, Version 2.0 (the "License"); // you may not use this file except in compliance with the License. @@ -20,8 +19,10 @@ //! XPath 1.0 grammar used by NETCONF/YANG-Push filters (plain location paths //! with implicit `child`-axis steps and simple predicates) — no dependency //! on a YANG context or any particular filter type. They back -//! [`crate::yang_push::filters::DatastoreXPathFilter::path_prefixes`] and -//! [`crate::xml_utils::XmlParser::read_xpath_with_namespaces`]. +//! [`crate::yang_push::filters::DatastoreXPathFilter::normalize_path`] and +//! [`crate::xml_utils::XmlParser::read_xpath_with_namespaces`], and are also +//! used outside this crate to diagnose xpath targets reported by a publisher +//! against a loaded schema. use std::collections::HashSet; @@ -80,10 +81,8 @@ pub(crate) fn find_xpath_prefixes(xpath: &str) -> HashSet { prefixes } -/// Split an XPath 1.0 location path into `/`-separated steps, honoring -/// bracketed predicates and quoted strings so a `/` inside `[...]` or a -/// string literal is not mistaken for a step separator. Returns `None` if -/// brackets or quotes are unbalanced. +/// Split an xpath location path on `/` at bracket depth 0 and outside string +/// literals. Returns `None` if quotes or brackets are unbalanced. pub(crate) fn split_location_path(path: &str) -> Option> { let mut segments = Vec::new(); let mut depth: i32 = 0; @@ -149,6 +148,73 @@ fn is_ncname(s: &str) -> bool { chars.all(|c| c.is_ascii_alphanumeric() || c == '_' || c == '-' || c == '.') } +/// Reduce `provided` vs `canonical` to the char index where they first +/// diverge, plus the substrings unique to each side (with the common +/// prefix/suffix stripped off), so small differences (e.g. a missing +/// leading slash) are obvious without scanning both full paths. +pub fn xpath_diff(provided: &str, canonical: &str) -> (usize, String, String) { + let prefix_chars = provided + .chars() + .zip(canonical.chars()) + .take_while(|(a, b)| a == b) + .count(); + let provided_chars = provided.chars().count(); + let canonical_chars = canonical.chars().count(); + let max_suffix = (provided_chars - prefix_chars).min(canonical_chars - prefix_chars); + let suffix_chars = provided + .chars() + .rev() + .zip(canonical.chars().rev()) + .take_while(|(a, b)| a == b) + .count() + .min(max_suffix); + + let byte_offset = |s: &str, chars: usize| -> usize { + s.char_indices() + .nth(chars) + .map(|(i, _)| i) + .unwrap_or(s.len()) + }; + let provided_unique = &provided + [byte_offset(provided, prefix_chars)..byte_offset(provided, provided_chars - suffix_chars)]; + let canonical_unique = &canonical[byte_offset(canonical, prefix_chars) + ..byte_offset(canonical, canonical_chars - suffix_chars)]; + + ( + prefix_chars, + provided_unique.to_string(), + canonical_unique.to_string(), + ) +} + +/// Remove XPath predicate groups (`[...]`) from a location path, honoring +/// quoted strings and nested brackets so predicate contents (including a +/// `]` inside a string literal) are not miscounted. +pub fn strip_xpath_predicates(path: &str) -> String { + let mut out = String::with_capacity(path.len()); + let mut depth: u32 = 0; + let mut in_single = false; + let mut in_double = false; + for c in path.chars() { + if depth == 0 { + if c == '[' { + depth = 1; + } else { + out.push(c); + } + } else { + match c { + '\'' if !in_double => in_single = !in_single, + '"' if !in_single => in_double = !in_double, + '[' if !in_single && !in_double => depth += 1, + ']' if !in_single && !in_double => depth -= 1, + _ => {} + } + } + } + out +} + #[cfg(test)] mod tests { use super::*; @@ -364,4 +430,63 @@ mod tests { // be reported as a prefix. This behavior is intentional. assert_prefixes(" / if : interfaces ", HashSet::new()); } + + #[test] + fn test_strip_xpath_predicates_removes_single_and_multiple_predicates() { + assert_eq!( + strip_xpath_predicates("/if:interfaces/if:interface[if:name='eth0']/if:oper-status"), + "/if:interfaces/if:interface/if:oper-status" + ); + assert_eq!( + strip_xpath_predicates("/a:x[1]/a:y[a:z='w'][@id='2']"), + "/a:x/a:y" + ); + } + + #[test] + fn test_strip_xpath_predicates_ignores_brackets_inside_string_literals() { + // A `]` inside a quoted predicate value must not be mistaken for the + // end of the predicate. + assert_eq!( + strip_xpath_predicates(r#"/a:x[a:y='[literal]']/a:z"#), + "/a:x/a:z" + ); + } + + #[test] + fn test_strip_xpath_predicates_noop_without_predicates() { + assert_eq!( + strip_xpath_predicates("/if:interfaces/if:interface"), + "/if:interfaces/if:interface" + ); + } + + #[test] + fn test_xpath_diff_reports_common_prefix_and_unique_suffixes() { + let (diverges_at, provided_unique, canonical_unique) = + xpath_diff("if:interfaces/interface", "/if:interfaces/interface"); + assert_eq!(diverges_at, 0); + assert_eq!(provided_unique, ""); + assert_eq!(canonical_unique, "/"); + } + + #[test] + fn test_xpath_diff_isolates_a_single_differing_segment() { + let (diverges_at, provided_unique, canonical_unique) = xpath_diff( + "/if:interfaces/if:interface/oper-status", + "/if:interfaces/interface/oper-status", + ); + assert_eq!(diverges_at, "/if:interfaces/i".chars().count()); + assert_eq!(provided_unique, "f:i"); + assert_eq!(canonical_unique, ""); + } + + #[test] + fn test_xpath_diff_identical_paths_yield_no_unique_substrings() { + let (diverges_at, provided_unique, canonical_unique) = + xpath_diff("/if:interfaces/interface", "/if:interfaces/interface"); + assert_eq!(diverges_at, "/if:interfaces/interface".chars().count()); + assert!(provided_unique.is_empty()); + assert!(canonical_unique.is_empty()); + } } diff --git a/crates/yang-push/src/validation/mod.rs b/crates/yang-push/src/validation/mod.rs index 4fe5c25d..3314e7cd 100644 --- a/crates/yang-push/src/validation/mod.rs +++ b/crates/yang-push/src/validation/mod.rs @@ -125,6 +125,7 @@ use crate::{ ContentId, OTL_YANG_PUSH_SUBSCRIPTION_ID_KEY, OTL_YANG_PUSH_SUBSCRIPTION_ROUTER_CONTENT_ID_KEY, OTL_YANG_PUSH_SUBSCRIPTION_TARGET_KEY, }; +use netcalyx_netconf_proto::xpath::{strip_xpath_predicates, xpath_diff}; use netcalyx_netconf_proto::yang_push::filters::DatastoreXPathFilter; use netcalyx_netconf_proto::yang_push::subscription::YangPushModuleVersion; use netcalyx_netconf_proto::yang_push::types::SubscriptionId; @@ -142,6 +143,7 @@ use strum::VariantNames; use tokio::sync::mpsc; use tracing::{debug, info, trace, warn}; use yang5::data::{DataFormat, DataOperation, DataParserFlags, DataValidationFlags}; +use yang5::schema::SchemaPathFormat; // Attribute key shared by the `dropped` and `skipped` counters. const REASON_KEY: &str = "reason"; @@ -1257,7 +1259,6 @@ impl ValidationActor { }; // Update subscription info in the cache - subscription_cache.subscription_info = subscription_info.clone(); if let Some(yang_lib_ref) = yang_lib_ref { let search_dir = yang_lib_ref.search_dir(); let yang_ctx_result = yang5::context::Context::new_from_yang_library_file( @@ -1285,6 +1286,12 @@ impl ValidationActor { None } }; + // Sanity-check the subscription target resolves against the schema + // and warn if it is missing or not in canonical form. Diagnostic + // only: the target is never modified here. + if let Some(ctx) = yang_ctx.as_ref() { + Self::check_xpath_target_resolves(&subscription_info, ctx); + } subscription_cache.cached_content_id = cached_content_id.clone(); subscription_cache.yang_ctx = yang_ctx; } else { @@ -1292,6 +1299,9 @@ impl ValidationActor { subscription_cache.cached_content_id = None; subscription_cache.yang_ctx = None; } + + // Store the subscription info in the cache. + subscription_cache.subscription_info = subscription_info.clone(); subscription_cache.schema_fetch_pending = false; let buffered_packets = std::mem::take(&mut subscription_cache.buffered_packets); let drained = buffered_packets.len(); @@ -1339,6 +1349,97 @@ impl ValidationActor { Ok(()) } + /// Resolve the inline `datastore-xpath-filter` target against the loaded + /// schema and warn if it does not resolve or is not in libyang's canonical + /// form (`LYSC_PATH_DATA`). Purely diagnostic: the target is never + /// modified. A well-behaved publisher should always send a resolvable, + /// canonical path, so a mismatch flags a bad xpath from the router (or + /// a gap in the fetcher-side normalization). Skipped for targets without a + /// datastore xpath filter. + /// + /// Predicates (`[...]`) are stripped before the equality check: libyang's + /// schema path never carries them, so we compare the structural + /// location path and still detect real prefix/structure differences + /// without a spurious mismatch from the predicate itself. + fn check_xpath_target_resolves( + subscription_info: &SubscriptionInfo, + yang_ctx: &yang5::context::Context, + ) { + let subscription_id = subscription_info.id(); + let peer = subscription_info.peer_ip(); + + let Some(original) = subscription_info.target.datastore_xpath_filter.as_deref() else { + return; + }; + + // find_xpath is evaluated against the original query (predicates + // included) so a malformed predicate/typo'd leaf name still surfaces + // as a real evaluation error. + let nodes = match yang_ctx.find_xpath(original) { + Ok(set) => set.collect::>(), + Err(err) => { + warn!( + %peer, + subscription_id, + path = %original, + error = %err, + "target xpath failed to evaluate against the schema (bad xpath from the publisher?)", + ); + return; + } + }; + + match nodes.as_slice() { + [node] => { + let canonical = node.path(SchemaPathFormat::DATA); + // Schema paths are absolute and never carry predicates; strip + // predicates from the provided path and tolerate a + // device-omitted leading slash before comparing, so only + // genuine structural/prefix differences are reported. + let stripped = strip_xpath_predicates(original); + let comparison = if stripped.starts_with('/') { + stripped + } else { + format!("/{stripped}") + }; + if canonical != comparison { + let (diverges_at, provided_unique, canonical_unique) = + xpath_diff(&comparison, &canonical); + warn!( + %peer, + subscription_id, + provided = %original, + canonical = %canonical, + diverges_at, + provided_unique, + canonical_unique, + "target xpath differs from the schema canonical form", + ); + } else { + trace!( + %peer, + subscription_id, + path = %original, + "target xpath resolves to a schema node and is canonical", + ); + } + } + [] => warn!( + %peer, + subscription_id, + path = %original, + "target xpath does not resolve to any schema node (bad xpath from the publisher?)", + ), + many => trace!( + %peer, + subscription_id, + path = %original, + node_count = many.len(), + "target xpath resolves to multiple schema nodes", + ), + } + } + /// Normalize the inline `datastore-xpath-filter` carried by a JSON /// `SubscriptionStarted`/`SubscriptionModified` notification to the same /// module-name-qualified, prefix-on-change canonical form the fetcher @@ -1582,6 +1683,7 @@ mod tests { use super::*; use crate::cache::actor::tests::setup_actor_with_empty_cache; use bytes::Bytes; + use netcalyx_netconf_proto::yang_push::identities::{Encoding, Transport}; use netcalyx_udp_notif_pkt::raw::MediaType; use std::collections::HashMap; use std::time::Duration; @@ -3056,4 +3158,134 @@ mod tests { ); assert_eq!(target, before); } + + /// Loads a `yang5::context::Context` from the bundled `ietf-interfaces` + /// test schema (the same assets used by the cache actor tests), for + /// tests that need a real schema to resolve xpaths against. + fn load_test_yang_ctx() -> yang5::context::Context { + yang5::context::Context::new_from_yang_library_file( + std::path::Path::new("../../assets/yang/ietf-interfaces/yang-lib.xml"), + DataFormat::XML, + std::path::Path::new("../../assets/yang/ietf-interfaces/modules"), + yang5::context::ContextFlags::empty(), + ) + .expect("Failed to load test YANG context") + } + + fn test_subscription_info_with_target(target: Target) -> SubscriptionInfo { + SubscriptionInfo::new( + IpAddr::from([127, 0, 0, 1]), + 1, + target, + None, + Some(Transport::UDPNotif), + Some(Encoding::Json), + None, + None, + Box::new([]), + ContentId::from("test-content-id".to_string()), + ) + } + + /// A target xpath that resolves to exactly one schema node and is + /// already in canonical form must not produce any warning. + #[test] + #[tracing_test::traced_test] + fn test_check_xpath_target_resolves_canonical_path_is_silent() { + let yang_ctx = load_test_yang_ctx(); + let subscription_info = test_subscription_info_with_target(Target::new_datastore( + "ietf-datastores:operational".to_string(), + either::Right("/ietf-interfaces:interfaces/interface/oper-status".to_string()), + )); + ValidationActor::check_xpath_target_resolves(&subscription_info, &yang_ctx); + assert!(!logs_contain( + "target xpath differs from the schema canonical form" + )); + assert!(!logs_contain("target xpath does not resolve")); + assert!(!logs_contain("target xpath failed to evaluate")); + } + + /// A target xpath that resolves but is not in libyang's canonical + /// (prefix-on-change) form must warn with the divergence details. + #[test] + #[tracing_test::traced_test] + fn test_check_xpath_target_resolves_non_canonical_path_warns() { + let yang_ctx = load_test_yang_ctx(); + let subscription_info = test_subscription_info_with_target(Target::new_datastore( + "ietf-datastores:operational".to_string(), + either::Right( + "/ietf-interfaces:interfaces/ietf-interfaces:interface/ietf-interfaces:oper-status" + .to_string(), + ), + )); + ValidationActor::check_xpath_target_resolves(&subscription_info, &yang_ctx); + assert!(logs_contain( + "target xpath differs from the schema canonical form" + )); + } + + /// Predicates must be stripped before the canonical-form comparison, so + /// an instantiated key predicate alone does not trigger a false warning. + #[test] + #[tracing_test::traced_test] + fn test_check_xpath_target_resolves_ignores_predicates_when_comparing() { + let yang_ctx = load_test_yang_ctx(); + let subscription_info = test_subscription_info_with_target(Target::new_datastore( + "ietf-datastores:operational".to_string(), + either::Right( + "/ietf-interfaces:interfaces/interface[name='eth0']/oper-status".to_string(), + ), + )); + ValidationActor::check_xpath_target_resolves(&subscription_info, &yang_ctx); + assert!(!logs_contain( + "target xpath differs from the schema canonical form" + )); + } + + /// An xpath referring to a node that doesn't exist in the schema must + /// warn that it does not resolve to any schema node. + #[test] + #[tracing_test::traced_test] + fn test_check_xpath_target_resolves_missing_node_warns() { + let yang_ctx = load_test_yang_ctx(); + let subscription_info = test_subscription_info_with_target(Target::new_datastore( + "ietf-datastores:operational".to_string(), + either::Right("/ietf-interfaces:interfaces/interface/no-such-leaf".to_string()), + )); + ValidationActor::check_xpath_target_resolves(&subscription_info, &yang_ctx); + assert!(logs_contain( + "target xpath does not resolve to any schema node" + )); + } + + /// A syntactically invalid xpath must warn that it failed to evaluate, + /// rather than panicking or silently passing. + #[test] + #[tracing_test::traced_test] + fn test_check_xpath_target_resolves_invalid_xpath_warns() { + let yang_ctx = load_test_yang_ctx(); + let subscription_info = test_subscription_info_with_target(Target::new_datastore( + "ietf-datastores:operational".to_string(), + either::Right("/ietf-interfaces:interfaces[".to_string()), + )); + ValidationActor::check_xpath_target_resolves(&subscription_info, &yang_ctx); + assert!(logs_contain( + "target xpath failed to evaluate against the schema" + )); + } + + /// A target without a datastore xpath filter (e.g. a stream target) + /// must be a no-op: no warnings, no panics. + #[test] + #[tracing_test::traced_test] + fn test_check_xpath_target_resolves_noop_without_xpath_filter() { + let yang_ctx = load_test_yang_ctx(); + let subscription_info = test_subscription_info_with_target(Target::new_stream( + "NETCONF".to_string(), + None, + either::Left(serde_json::Value::Null), + )); + ValidationActor::check_xpath_target_resolves(&subscription_info, &yang_ctx); + assert!(!logs_contain("target xpath")); + } }