Configure scouting/*/autoconnect/* with a sequence (#1224)

* Configure `scouting/*/autoconnect` with a sequence * Fix `zenoh/tests/routing.rs` * Fix `zenoh/tests/routing.rs` (again) * Fix zenoh-config tests * Fix `scouting/multicast/autoconnect/router` in default config * Keep string representation * Fix `ModeDependentValue<WhatAmIMatcher>` de impl * Remove `#[serde(deserialize_with = "treat_error_as_none")]` This attribute makes it hard to debug certain errors and was only present on autoconnect fields.
eclipse-zenoh · Jul 19, 2024 · b31a410 · b31a410
1 parent c726e13
commit b31a410
Show file tree

Hide file tree

Showing 5 changed files with 45 additions and 60 deletions.
diff --git a/DEFAULT_CONFIG.json5 b/DEFAULT_CONFIG.json5
@@ -14,7 +14,7 @@
   /// The node's metadata (name, location, DNS name, etc.) Arbitrary JSON data not interpreted by zenohd and available in admin space @/router/<id>
   metadata: {
     name: "strawberry",
-    location: "Penny Lane"
+    location: "Penny Lane",
   },
 
   /// Which endpoints to connect to. E.g. tcp/localhost:7447.
@@ -27,7 +27,7 @@
     /// or different values for router, peer and client (e.g. timeout_ms: { router: -1, peer: -1, client: 0 }).
     timeout_ms: { router: -1, peer: -1, client: 0 },
 
-    /// The list of endpoints to connect to. 
+    /// The list of endpoints to connect to.
     /// Accepts a single list (e.g. endpoints: ["tcp/10.10.10.10:7447", "tcp/11.11.11.11:7447"])
     /// or different lists for router, peer and client (e.g. endpoints: { router: ["tcp/10.10.10.10:7447"], peer: ["tcp/11.11.11.11:7447"] }).
     endpoints: [
@@ -64,7 +64,7 @@
     /// or different values for router, peer and client (e.g. timeout_ms: { router: -1, peer: -1, client: 0 }).
     timeout_ms: 0,
 
-    /// The list of endpoints to listen on. 
+    /// The list of endpoints to listen on.
     /// Accepts a single list (e.g. endpoints: ["tcp/[::]:7447", "udp/[::]:7447"])
     /// or different lists for router, peer and client (e.g. endpoints: { router: ["tcp/[::]:7447"], peer: ["tcp/[::]:0"] }).
     endpoints: { router: ["tcp/[::]:7447"], peer: ["tcp/[::]:0"] },
@@ -104,10 +104,10 @@
       /// The time-to-live on multicast scouting packets
       ttl: 1,
       /// Which type of Zenoh instances to automatically establish sessions with upon discovery on UDP multicast.
-      /// Accepts a single value (e.g. autoconnect: "router|peer")
-      /// or different values for router, peer and client (e.g. autoconnect: { router: "", peer: "router|peer" }).
+      /// Accepts a single value (e.g. autoconnect: ["router", "peer"])
+      /// or different values for router, peer and client (e.g. autoconnect: { router: [], peer: ["router", "peer"] }).
       /// Each value is bit-or-like combinations of "peer", "router" and "client".
-      autoconnect: { router: "", peer: "router|peer" },
+      autoconnect: { router: [], peer: ["router", "peer"] },
       /// Whether or not to listen for scout messages on UDP multicast and reply to them.
       listen: true,
     },
@@ -122,10 +122,10 @@
       /// direct connectivity with each other.
       multihop: false,
       /// Which type of Zenoh instances to automatically establish sessions with upon discovery on gossip.
-      /// Accepts a single value (e.g. autoconnect: "router|peer")
-      /// or different values for router, peer and client (e.g. autoconnect: { router: "", peer: "router|peer" }).
+      /// Accepts a single value (e.g. autoconnect: ["router", "peer"])
+      /// or different values for router, peer and client (e.g. autoconnect: { router: [], peer: ["router", "peer"] }).
       /// Each value is bit-or-like combinations of "peer", "router" and "client".
-      autoconnect: { router: "", peer: "router|peer" },
+      autoconnect: { router: [], peer: ["router", "peer"] },
     },
   },
 
@@ -208,7 +208,7 @@
   //       "interfaces": [
   //         "lo0"
   //       ],
-  //       /// Subjects can be cert_common_names when using TLS or Quic 
+  //       /// Subjects can be cert_common_names when using TLS or Quic
   //       "cert_common_names": [
   //         "example.zenoh.io"
   //       ],
@@ -238,7 +238,7 @@
       /// NOTE: Currently, the LowLatency transport doesn't preserve QoS prioritization.
       /// NOTE: Due to the note above, 'lowlatency' is incompatible with 'qos' option, so in order to
       ///       enable 'lowlatency' you need to explicitly disable 'qos'.
-      /// NOTE: LowLatency transport does not support the fragmentation, so the message size should be 
+      /// NOTE: LowLatency transport does not support the fragmentation, so the message size should be
       ///       smaller than the tx batch_size.
       lowlatency: false,
       /// Enables QoS on unicast communications.
@@ -317,7 +317,7 @@
           /// Using CongestionControl::Drop the message might be dropped, depending on conditions configured here.
           congestion_control: {
             /// The maximum time in microseconds to wait for an available batch before dropping the message if still no batch is available.
-            wait_before_drop: 1000
+            wait_before_drop: 1000,
           },
           /// The initial exponential backoff time in nanoseconds to allow the batching to eventually progress.
           /// Higher values lead to a more aggressive batching but it will introduce additional latency.
@@ -539,5 +539,4 @@
   //     __config__: "./plugins/zenoh-plugin-storage-manager/config.json5",
   //   }
   // },
-
 }
diff --git a/commons/zenoh-config/src/lib.rs b/commons/zenoh-config/src/lib.rs
@@ -197,15 +197,6 @@ fn config_keys() {
     dbg!(c.keys());
 }
 
-fn treat_error_as_none<'a, T, D>(deserializer: D) -> Result<Option<T>, D::Error>
-where
-    T: serde::de::Deserialize<'a>,
-    D: serde::de::Deserializer<'a>,
-{
-    let value: Value = serde::de::Deserialize::deserialize(deserializer)?;
-    Ok(T::deserialize(value).ok())
-}
-
 validated_struct::validator! {
     /// The main configuration structure for Zenoh.
     ///
@@ -264,7 +255,6 @@ validated_struct::validator! {
                 /// The time-to-live on multicast scouting packets. (default: 1)
                 pub ttl: Option<u32>,
                 /// Which type of Zenoh instances to automatically establish sessions with upon discovery through UDP multicast.
-                #[serde(deserialize_with = "treat_error_as_none")]
                 autoconnect: Option<ModeDependentValue<WhatAmIMatcher>>,
                 /// Whether or not to listen for scout messages on UDP multicast and reply to them.
                 listen: Option<ModeDependentValue<bool>>,
@@ -281,7 +271,6 @@ validated_struct::validator! {
                 /// direct connectivity with each other.
                 multihop: Option<bool>,
                 /// Which type of Zenoh instances to automatically establish sessions with upon discovery through gossip.
-                #[serde(deserialize_with = "treat_error_as_none")]
                 autoconnect: Option<ModeDependentValue<WhatAmIMatcher>>,
             },
         },
@@ -571,7 +560,7 @@ fn config_deser() {
         scouting: {
           multicast: {
             enabled: false,
-            autoconnect: "peer|router"
+            autoconnect: ["peer", "router"]
           }
         }
       }"#,
@@ -598,7 +587,7 @@ fn config_deser() {
         scouting: {
           multicast: {
             enabled: false,
-            autoconnect: {router: "", peer: "peer|router"}
+            autoconnect: {router: [], peer: ["peer", "router"]}
           }
         }
       }"#,

diff --git a/commons/zenoh-config/src/mode_dependent.rs b/commons/zenoh-config/src/mode_dependent.rs
@@ -255,12 +255,12 @@ impl<'a> serde::Deserialize<'a> for ModeDependentValue<WhatAmIMatcher> {
                 formatter.write_str("WhatAmIMatcher or mode dependent WhatAmIMatcher")
             }
 
-            fn visit_str<E>(self, value: &str) -> Result<Self::Value, E>
+            fn visit_seq<A>(self, seq: A) -> Result<Self::Value, A::Error>
             where
-                E: de::Error,
+                A: de::SeqAccess<'de>,
             {
                 WhatAmIMatcherVisitor {}
-                    .visit_str(value)
+                    .visit_seq(seq)
                     .map(ModeDependentValue::Unique)
             }
 

diff --git a/commons/zenoh-protocol/src/core/whatami.rs b/commons/zenoh-protocol/src/core/whatami.rs
@@ -146,6 +146,7 @@ impl WhatAmIMatcher {
             Self::U8_R_C => formatcp!("{}|{}", WhatAmI::STR_R, WhatAmI::STR_C),
             Self::U8_P_C => formatcp!("{}|{}", WhatAmI::STR_P, WhatAmI::STR_C),
             Self::U8_R_P_C => formatcp!("{}|{}|{}", WhatAmI::STR_R, WhatAmI::STR_P, WhatAmI::STR_C),
+
             _ => unreachable!(),
         }
     }
@@ -329,41 +330,40 @@ impl<'de> serde::de::Visitor<'de> for WhatAmIMatcherVisitor {
     fn expecting(&self, formatter: &mut fmt::Formatter) -> fmt::Result {
         write!(
             formatter,
-            "a | separated list of whatami variants ('{}', '{}', '{}')",
+            "a list of whatami variants ('{}', '{}', '{}')",
             WhatAmI::STR_R,
             WhatAmI::STR_P,
             WhatAmI::STR_C
         )
     }
 
-    fn visit_str<E>(self, v: &str) -> Result<Self::Value, E>
+    fn visit_seq<A>(self, mut seq: A) -> Result<Self::Value, A::Error>
     where
-        E: serde::de::Error,
+        A: serde::de::SeqAccess<'de>,
     {
-        v.parse().map_err(|_| {
-            serde::de::Error::invalid_value(
-                serde::de::Unexpected::Str(v),
-                &formatcp!(
-                    "a | separated list of whatami variants ('{}', '{}', '{}')",
-                    WhatAmI::STR_R,
-                    WhatAmI::STR_P,
-                    WhatAmI::STR_C
-                ),
-            )
-        })
-    }
+        let mut inner = 0;
 
-    fn visit_borrowed_str<E>(self, v: &'de str) -> Result<Self::Value, E>
-    where
-        E: serde::de::Error,
-    {
-        self.visit_str(v)
-    }
-    fn visit_string<E>(self, v: String) -> Result<Self::Value, E>
-    where
-        E: serde::de::Error,
-    {
-        self.visit_str(&v)
+        while let Some(s) = seq.next_element::<String>()? {
+            match s.as_str() {
+                WhatAmI::STR_R => inner |= WhatAmI::U8_R,
+                WhatAmI::STR_P => inner |= WhatAmI::U8_P,
+                WhatAmI::STR_C => inner |= WhatAmI::U8_C,
+                _ => {
+                    return Err(serde::de::Error::invalid_value(
+                        serde::de::Unexpected::Str(&s),
+                        &formatcp!(
+                            "one of ('{}', '{}', '{}')",
+                            WhatAmI::STR_R,
+                            WhatAmI::STR_P,
+                            WhatAmI::STR_C
+                        ),
+                    ))
+                }
+            }
+        }
+
+        Ok(WhatAmIMatcher::try_from(inner)
+            .expect("`WhatAmIMatcher` should be valid by construction"))
     }
 }
 
@@ -372,6 +372,6 @@ impl<'de> serde::Deserialize<'de> for WhatAmIMatcher {
     where
         D: serde::Deserializer<'de>,
     {
-        deserializer.deserialize_str(WhatAmIMatcherVisitor)
+        deserializer.deserialize_seq(WhatAmIMatcherVisitor)
     }
 }
diff --git a/zenoh/tests/routing.rs b/zenoh/tests/routing.rs
@@ -12,7 +12,6 @@
 //   ZettaScale Zenoh Team, <zenoh@zettascale.tech>
 //
 use std::{
-    str::FromStr,
     sync::{
         atomic::{AtomicUsize, Ordering},
         Arc,
@@ -540,9 +539,7 @@ async fn static_failover_brokering() -> Result<()> {
         config
             .scouting
             .gossip
-            .set_autoconnect(Some(ModeDependentValue::Unique(
-                WhatAmIMatcher::from_str("").unwrap(),
-            )))
+            .set_autoconnect(Some(ModeDependentValue::Unique(WhatAmIMatcher::empty())))
             .unwrap();
         Some(config)
     };