Repository: bytebeamio/rumqtt (language: rust)

Issue:
Client ID should be optional
# Expected Behavior

When creating a client, using an empty client ID should work.

Per the 3.1.1 spec http://docs.oasis-open.org/mqtt/mqtt/v3.1.1/os/mqtt-v3.1.1-os.html

```
A Server MAY allow a Client to supply a ClientId that has a length of zero bytes, however if it does so the Server MUST treat this as a special case and assign a unique ClientId to that Client. It MUST then process the CONNECT packet as if the Client had provided that unique ClientId [MQTT-3.1.3-6].
```

# Current Behavior

Using an empty client id for the rumqttc client options results in a panic.

```
        let id = id.into();
        if id.starts_with(' ') || id.is_empty() {
            panic!("Invalid client id");
        }
```



Discussion / hints:
Hey, currently rumqttd doesn't generate random client ids, and will disconnect the clients without id. I think that's why we panic in rumqttc itself so user will know why it disconnected!

It is in plan to implement this, but as this is optional behaviour, it's on low priority 😅
This affects interaction with other (non rumqttd) servers.  Is rumqttc supposed to only be used with rumqttd?

Handling empty client ids is optional on the server, but supporting sending empty client ids is not optional for the client to be spec compliant.
> Is rumqttc supposed to only be used with rumqttd?

Nope. You are correct about it, we should fix it if it's not per specs 😅

Fix patch:
diff --git a/rumqttc/CHANGELOG.md b/rumqttc/CHANGELOG.md
index 0705a9db4..b9c6f73b8 100644
--- a/rumqttc/CHANGELOG.md
+++ b/rumqttc/CHANGELOG.md
@@ -16,6 +16,8 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 - Surfaced `AsyncClient`'s `from_senders` method to the `Client` as `from_sender`
 
 ### Changed
+- `MqttOptions::new` now accepts empty client id.
+- `MqttOptions::set_clean_session` now panics if client ID is empty and `clean_session` flag is set to false.
 - Synchronous client methods take `&self` instead of `&mut self` (#646)
 - Removed the `Key` enum: users do not need to specify the TLS key variant in the `TlsConfiguration` anymore, this is inferred automatically.
 To update your code simply remove `Key::ECC()` or `Key::RSA()` from the initialization.
diff --git a/rumqttc/src/lib.rs b/rumqttc/src/lib.rs
index 3ff32db43..ce52b452a 100644
--- a/rumqttc/src/lib.rs
+++ b/rumqttc/src/lib.rs
@@ -491,25 +491,14 @@ impl MqttOptions {
     /// # use rumqttc::MqttOptions;
     /// let options = MqttOptions::new("123", "localhost", 1883);
     /// ```
-    /// NOTE: you are not allowed to use an id that starts with a whitespace or is empty.
-    /// for example, the following code would panic:
-    /// ```should_panic
-    /// # use rumqttc::MqttOptions;
-    /// let options = MqttOptions::new("", "localhost", 1883);
-    /// ```
     pub fn new<S: Into<String>, T: Into<String>>(id: S, host: T, port: u16) -> MqttOptions {
-        let id = id.into();
-        if id.starts_with(' ') || id.is_empty() {
-            panic!("Invalid client id");
-        }
-
         MqttOptions {
             broker_addr: host.into(),
             port,
             transport: Transport::tcp(),
             keep_alive: Duration::from_secs(60),
             clean_session: true,
-            client_id: id,
+            client_id: id.into(),
             credentials: None,
             max_incoming_packet_size: 10 * 1024,
             max_outgoing_packet_size: 10 * 1024,
@@ -624,7 +613,21 @@ impl MqttOptions {
     /// When set `false`, broker will hold the client state and performs pending
     /// operations on the client when reconnection with same `client_id`
     /// happens. Local queue state is also held to retransmit packets after reconnection.
+    ///
+    /// # Panic
+    ///
+    /// Panics if `clean_session` is false when `client_id` is empty.
+    ///
+    /// ```should_panic
+    /// # use rumqttc::MqttOptions;
+    /// let mut options = MqttOptions::new("", "localhost", 1883);
+    /// options.set_clean_session(false);
+    /// ```
     pub fn set_clean_session(&mut self, clean_session: bool) -> &mut Self {
+        assert!(
+            !self.client_id.is_empty() || clean_session,
+            "Cannot unset clean session when client id is empty"
+        );
         self.clean_session = clean_session;
         self
     }
@@ -918,12 +921,6 @@ impl Debug for MqttOptions {
 mod test {
     use super::*;
 
-    #[test]
-    #[should_panic]
-    fn client_id_startswith_space() {
-        let _mqtt_opts = MqttOptions::new(" client_a", "127.0.0.1", 1883).set_clean_session(true);
-    }
-
     #[test]
     #[cfg(all(feature = "use-rustls", feature = "websocket"))]
     fn no_scheme() {
@@ -1008,8 +1005,14 @@ mod test {
     }
 
     #[test]
-    #[should_panic]
-    fn no_client_id() {
+    fn accept_empty_client_id() {
         let _mqtt_opts = MqttOptions::new("", "127.0.0.1", 1883).set_clean_session(true);
     }
+
+    #[test]
+    fn set_clean_session_when_client_id_present() {
+        let mut options = MqttOptions::new("client_id", "127.0.0.1", 1883);
+        options.set_clean_session(false);
+        options.set_clean_session(true);
+    }
 }
diff --git a/rumqttc/src/v5/mod.rs b/rumqttc/src/v5/mod.rs
index 4b6793b03..04a50a4c2 100644
--- a/rumqttc/src/v5/mod.rs
+++ b/rumqttc/src/v5/mod.rs
@@ -113,28 +113,17 @@ impl MqttOptions {
     /// - port: The port number on which broker must be listening for incoming connections
     ///
     /// ```
-    /// # use rumqttc::MqttOptions;
+    /// # use rumqttc::v5::MqttOptions;
     /// let options = MqttOptions::new("123", "localhost", 1883);
     /// ```
-    /// NOTE: you are not allowed to use an id that starts with a whitespace or is empty.
-    /// for example, the following code would panic:
-    /// ```should_panic
-    /// # use rumqttc::MqttOptions;
-    /// let options = MqttOptions::new("", "localhost", 1883);
-    /// ```
     pub fn new<S: Into<String>, T: Into<String>>(id: S, host: T, port: u16) -> MqttOptions {
-        let id = id.into();
-        if id.starts_with(' ') || id.is_empty() {
-            panic!("Invalid client id");
-        }
-
         MqttOptions {
             broker_addr: host.into(),
             port,
             transport: Transport::tcp(),
             keep_alive: Duration::from_secs(60),
             clean_start: true,
-            client_id: id,
+            client_id: id.into(),
             credentials: None,
             request_channel_capacity: 10,
             max_request_batch: 0,
@@ -730,12 +719,6 @@ impl Debug for MqttOptions {
 mod test {
     use super::*;
 
-    #[test]
-    #[should_panic]
-    fn client_id_startswith_space() {
-        let _mqtt_opts = MqttOptions::new(" client_a", "127.0.0.1", 1883).set_clean_start(true);
-    }
-
     #[test]
     #[cfg(all(feature = "use-rustls", feature = "websocket"))]
     fn no_scheme() {
@@ -821,8 +804,7 @@ mod test {
     }
 
     #[test]
-    #[should_panic]
-    fn no_client_id() {
+    fn allow_empty_client_id() {
         let _mqtt_opts = MqttOptions::new("", "127.0.0.1", 1883).set_clean_start(true);
     }
 }


Test patch: