Skip to content

Commit 31e415a

Browse files
wyfowyfo
authored
fix: fix suffix splitting in Resource::get_best_key (#1757)
* fix: fix suffix splitting in `Resource::get_best_key` Refactor code to uniformize suffix splitting, and remove a bunch of copy-paste. * fix: typo * test: add get_best_key test
1 parent 22e6b2e commit 31e415a

File tree

2 files changed

+126
-99
lines changed

2 files changed

+126
-99
lines changed

‎zenoh/src/net/routing/dispatcher/resource.rs

Lines changed: 68 additions & 98 deletions
Original file line numberDiff line numberDiff line change
@@ -352,105 +352,57 @@ impl Resource {
352352
from: &mut Arc<Resource>,
353353
suffix: &str,
354354
) -> Arc<Resource> {
355-
if suffix.is_empty() {
355+
let Some((chunk, rest)) = Self::split_first_chunk(suffix) else {
356356
Resource::upgrade_resource(from, tables.hat_code.new_resource());
357-
from.clone()
358-
} else if let Some(stripped_suffix) = suffix.strip_prefix('/') {
359-
let (chunk, rest) = match stripped_suffix.find('/') {
360-
Some(idx) => (&suffix[0..(idx + 1)], &suffix[(idx + 1)..]),
361-
None => (suffix, ""),
362-
};
363-
364-
match get_mut_unchecked(from).children.get_mut(chunk) {
365-
Some(res) => Resource::make_resource(tables, res, rest),
366-
None => {
367-
let mut new = Arc::new(Resource::new(from, chunk, None));
368-
if tracing::enabled!(tracing::Level::DEBUG) && rest.is_empty() {
369-
tracing::debug!("Register resource {}", new.expr());
370-
}
371-
let res = Resource::make_resource(tables, &mut new, rest);
372-
get_mut_unchecked(from)
373-
.children
374-
.insert(String::from(chunk), new);
375-
res
376-
}
377-
}
378-
} else {
379-
match from.parent.clone() {
380-
Some(mut parent) => {
381-
Resource::make_resource(tables, &mut parent, &[&from.suffix, suffix].concat())
382-
}
383-
None => {
384-
let (chunk, rest) = match suffix[1..].find('/') {
385-
Some(idx) => (&suffix[0..(idx + 1)], &suffix[(idx + 1)..]),
386-
None => (suffix, ""),
387-
};
388-
389-
match get_mut_unchecked(from).children.get_mut(chunk) {
390-
Some(res) => Resource::make_resource(tables, res, rest),
391-
None => {
392-
let mut new = Arc::new(Resource::new(from, chunk, None));
393-
if tracing::enabled!(tracing::Level::DEBUG) && rest.is_empty() {
394-
tracing::debug!("Register resource {}", new.expr());
395-
}
396-
let res = Resource::make_resource(tables, &mut new, rest);
397-
get_mut_unchecked(from)
398-
.children
399-
.insert(String::from(chunk), new);
400-
res
401-
}
402-
}
403-
}
357+
return from.clone();
358+
};
359+
if !chunk.starts_with('/') {
360+
if let Some(parent) = &mut from.parent.clone() {
361+
return Resource::make_resource(tables, parent, &[&from.suffix, suffix].concat());
404362
}
405363
}
364+
if let Some(child) = get_mut_unchecked(from).children.get_mut(chunk) {
365+
return Resource::make_resource(tables, child, rest);
366+
}
367+
let mut new = Arc::new(Resource::new(from, chunk, None));
368+
if rest.is_empty() {
369+
tracing::debug!("Register resource {}", new.expr());
370+
}
371+
let res = Resource::make_resource(tables, &mut new, rest);
372+
get_mut_unchecked(from)
373+
.children
374+
.insert(String::from(chunk), new);
375+
res
406376
}
407377

408378
#[inline]
409379
pub fn get_resource(from: &Arc<Resource>, suffix: &str) -> Option<Arc<Resource>> {
410-
if suffix.is_empty() {
411-
Some(from.clone())
412-
} else if let Some(stripped_suffix) = suffix.strip_prefix('/') {
413-
let (chunk, rest) = match stripped_suffix.find('/') {
414-
Some(idx) => (&suffix[0..(idx + 1)], &suffix[(idx + 1)..]),
415-
None => (suffix, ""),
416-
};
417-
418-
match from.children.get(chunk) {
419-
Some(res) => Resource::get_resource(res, rest),
420-
None => None,
421-
}
422-
} else {
423-
match &from.parent {
424-
Some(parent) => Resource::get_resource(parent, &[&from.suffix, suffix].concat()),
425-
None => {
426-
let (chunk, rest) = match suffix[1..].find('/') {
427-
Some(idx) => (&suffix[0..(idx + 1)], &suffix[(idx + 1)..]),
428-
None => (suffix, ""),
429-
};
430-
431-
match from.children.get(chunk) {
432-
Some(res) => Resource::get_resource(res, rest),
433-
None => None,
434-
}
435-
}
380+
let Some((chunk, rest)) = Self::split_first_chunk(suffix) else {
381+
return Some(from.clone());
382+
};
383+
if !chunk.starts_with('/') {
384+
if let Some(parent) = &from.parent {
385+
return Resource::get_resource(parent, &[&from.suffix, suffix].concat());
436386
}
437387
}
388+
Resource::get_resource(from.children.get(chunk)?, rest)
438389
}
439390

440-
fn fst_chunk(key_expr: &keyexpr) -> (&keyexpr, Option<&keyexpr>) {
441-
match key_expr.as_bytes().iter().position(|c| *c == b'/') {
442-
Some(pos) => {
443-
let left = &key_expr.as_bytes()[..pos];
444-
let right = &key_expr.as_bytes()[pos + 1..];
445-
unsafe {
446-
(
447-
keyexpr::from_slice_unchecked(left),
448-
Some(keyexpr::from_slice_unchecked(right)),
449-
)
450-
}
451-
}
452-
None => (key_expr, None),
391+
/// Split the suffix at the next '/' (after leading one), returning None if the suffix is empty.
392+
///
393+
/// Suffix usually starts with '/', so this first slash is kept as part of the split chunk.
394+
/// The rest will contain the slash of the split.
395+
/// For example `split_first_chunk("/a/b") == Some(("/a", "/b"))`.
396+
fn split_first_chunk(suffix: &str) -> Option<(&str, &str)> {
397+
if suffix.is_empty() {
398+
return None;
453399
}
400+
// don't count the first char which may be a leading slash to find the next one
401+
Some(match suffix[1..].find('/') {
402+
// don't forget to add 1 to the index because of `[1..]` slice above
403+
Some(idx) => suffix.split_at(idx + 1),
404+
None => (suffix, ""),
405+
})
454406
}
455407

456408
#[inline]
@@ -524,7 +476,17 @@ impl Resource {
524476
}
525477
}
526478

479+
/// Return the best locally/remotely declared keyexpr, i.e. with the smallest suffix, matching
480+
/// the given suffix and session id.
481+
///
482+
/// The goal is to save bandwidth by using the shortest keyexpr on the wire. It works by
483+
/// recursively walk through the children tree, looking for an already declared keyexpr for the
484+
/// session.
485+
/// If none is found, and if the tested resource itself doesn't have a declared keyexpr,
486+
/// then the parent tree is walked through. If there is still no declared keyexpr, the whole
487+
/// prefix+suffix string is used.
527488
pub fn get_best_key<'a>(&self, suffix: &'a str, sid: usize) -> WireExpr<'a> {
489+
/// Retrieve a declared keyexpr, either local or remote.
528490
fn get_wire_expr<'a>(
529491
prefix: &Resource,
530492
suffix: impl FnOnce() -> Cow<'a, str>,
@@ -542,19 +504,18 @@ impl Resource {
542504
mapping,
543505
})
544506
}
507+
/// Walk through the children tree, looking for a declared keyexpr.
545508
fn get_best_child_key<'a>(
546509
prefix: &Resource,
547510
suffix: &'a str,
548511
sid: usize,
549512
) -> Option<WireExpr<'a>> {
550-
if suffix.is_empty() {
551-
return None;
552-
}
553-
let (chunk, remain) = suffix.split_at(suffix.find('/').unwrap_or(suffix.len()));
513+
let (chunk, rest) = Resource::split_first_chunk(suffix)?;
554514
let child = prefix.children.get(chunk)?;
555-
get_best_child_key(child, remain, sid)
556-
.or_else(|| get_wire_expr(child, || remain.into(), sid))
515+
get_best_child_key(child, rest, sid)
516+
.or_else(|| get_wire_expr(child, || rest.into(), sid))
557517
}
518+
/// Walk through the parent tree, looking for a declared keyexpr.
558519
fn get_best_parent_key<'a>(
559520
prefix: &Resource,
560521
suffix: &'a str,
@@ -597,11 +558,20 @@ impl Resource {
597558
.unwrap_or(&from.suffix)
598559
.try_into()
599560
.unwrap();
600-
let (chunk, rest) = Resource::fst_chunk(key_expr);
601-
if chunk.intersects(suffix) {
602-
match rest {
561+
let (ke_chunk, ke_rest) = match key_expr.split_once('/') {
562+
// SAFETY: chunks of keyexpr are valid keyexprs
563+
Some((chunk, rest)) => unsafe {
564+
(
565+
keyexpr::from_str_unchecked(chunk),
566+
Some(keyexpr::from_str_unchecked(rest)),
567+
)
568+
},
569+
None => (key_expr, None),
570+
};
571+
if ke_chunk.intersects(suffix) {
572+
match ke_rest {
603573
None => {
604-
if chunk.as_bytes() == b"**" {
574+
if ke_chunk.as_bytes() == b"**" {
605575
recursive_push(from, matches)
606576
} else {
607577
if from.context.is_some() {
@@ -624,7 +594,7 @@ impl Resource {
624594
Some(rest) if rest.as_bytes() == b"**" => recursive_push(from, matches),
625595
Some(rest) => {
626596
let recheck_keyexpr_one_level_lower =
627-
chunk.as_bytes() == b"**" || suffix.as_bytes() == b"**";
597+
ke_chunk.as_bytes() == b"**" || suffix.as_bytes() == b"**";
628598
for child in from.children.values() {
629599
get_matches_from(rest, child, matches);
630600
if recheck_keyexpr_one_level_lower {

‎zenoh/src/net/tests/tables.rs

Lines changed: 58 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -32,7 +32,11 @@ use zenoh_protocol::{
3232
use crate::net::{
3333
primitives::{DummyPrimitives, EPrimitives, Primitives},
3434
routing::{
35-
dispatcher::{face::FaceState, pubsub::SubscriberInfo, tables::Tables},
35+
dispatcher::{
36+
face::{Face, FaceState},
37+
pubsub::SubscriberInfo,
38+
tables::Tables,
39+
},
3640
router::*,
3741
RoutingContext,
3842
},
@@ -828,3 +832,56 @@ fn client_test() {
828832
// mapping strategy check
829833
// assert_eq!(primitives2.get_last_key().unwrap(), KeyExpr::IdWithSuffix(31, "/z2_pub1".to_string()));
830834
}
835+
836+
#[test]
837+
fn get_best_key_test() {
838+
let config = Config::default();
839+
let router = Router::new(
840+
ZenohIdProto::try_from([1]).unwrap(),
841+
WhatAmI::Client,
842+
None,
843+
&config,
844+
)
845+
.unwrap();
846+
847+
let primitives = Arc::new(DummyPrimitives {});
848+
let face1 = router.new_primitives(primitives.clone());
849+
let face2 = router.new_primitives(primitives.clone());
850+
let face3 = router.new_primitives(primitives);
851+
852+
let root = zread!(router.tables.tables)._get_root().clone();
853+
let register_expr = |face: &Face, id: ExprId, expr: &str| {
854+
register_expr(&router.tables, &mut face.state.clone(), id, &expr.into());
855+
};
856+
let get_best_key = |resource, suffix, face: &Face| {
857+
Resource::get_resource(&root, resource)
858+
.unwrap()
859+
.get_best_key(suffix, face.state.id)
860+
};
861+
862+
register_expr(&face1, 1, "a");
863+
register_expr(&face2, 2, "a/b");
864+
register_expr(&face2, 3, "a/b/c");
865+
register_expr(&face3, 4, "a/d");
866+
867+
macro_rules! assert_wire_expr {
868+
($key:expr, {scope: $scope:expr, suffix: $suffix:expr}) => {
869+
assert_eq!($key.scope, $scope);
870+
assert_eq!($key.suffix, $suffix);
871+
};
872+
}
873+
assert_wire_expr!(get_best_key("", "a", &face1), { scope: 1, suffix: "" });
874+
assert_wire_expr!(get_best_key("", "a/b", &face1), { scope: 1, suffix: "/b" });
875+
assert_wire_expr!(get_best_key("a", "", &face1), { scope: 1, suffix: "" });
876+
assert_wire_expr!(get_best_key("a", "/b", &face1), { scope: 1, suffix: "/b" });
877+
assert_wire_expr!(get_best_key("a/b", "", &face1), { scope: 1, suffix: "/b" });
878+
assert_wire_expr!(get_best_key("", "e", &face1), { scope: 0, suffix: "e" });
879+
assert_wire_expr!(get_best_key("", "a", &face2), { scope: 0, suffix: "a" });
880+
assert_wire_expr!(get_best_key("", "a/b", &face2), { scope: 2, suffix: "" });
881+
assert_wire_expr!(get_best_key("", "a/b/c", &face2), { scope: 3, suffix: "" });
882+
assert_wire_expr!(get_best_key("", "a/b/c/d", &face2), { scope: 3, suffix: "/d" });
883+
assert_wire_expr!(get_best_key("a", "", &face2), { scope: 0, suffix: "a" });
884+
assert_wire_expr!(get_best_key("a", "/b", &face2), { scope: 2, suffix: "" });
885+
assert_wire_expr!(get_best_key("a", "/d", &face2), { scope: 0, suffix: "a/d" });
886+
assert_wire_expr!(get_best_key("a/b", "", &face2), { scope: 2, suffix: "" });
887+
}

0 commit comments

Comments
 (0)