Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

MqttOptions::new should be refactored #434

Open
EdJoPaTo opened this issue Aug 21, 2022 · 2 comments · May be fixed by #789
Open

MqttOptions::new should be refactored #434

EdJoPaTo opened this issue Aug 21, 2022 · 2 comments · May be fixed by #789
Labels
stale Not moving forward; blocked

Comments

@EdJoPaTo
Copy link
Contributor

EdJoPaTo commented Aug 21, 2022 •
edited

MqttOptions::new(client_id, host, port) is only relevant for TCP.

For WebSockets on the other hand host has to be an URL and not only the hostname (including ws://). The Port is ignored (see #270). Looking at the Transport::Unix options I don't see a use for the port either. So host and port differ based on the chosen transport.

Personally I would include host and port into the Transport enum then or provide different new methods like MqttOptions::new_tcp().

If possible also allow for the same MqttOptions with MQTT v3 / v5. Currently they are split which makes usage harder.
@EdJoPaTo EdJoPaTo mentioned this issue Aug 21, 2022
Closed
@EdJoPaTo
Copy link
Contributor Author

Also the MqttOptions::parse_url logic does not handle this so its incapable of handling WebSockets at the moment:

rumqtt/rumqttc/src/lib.rs

Lines 609 to 631 in 2f11546

let host = url.host_str().unwrap_or_default().to_owned();
let (transport, default_port) = match url.scheme() {
// Encrypted connections are supported, but require explicit TLS configuration. We fall
// back to the unencrypted transport layer, so that `set_transport` can be used to
// configure the encrypted transport layer with the provided TLS configuration.
"mqtts" | "ssl" => (Transport::Tcp, 8883),
"mqtt" | "tcp" => (Transport::Tcp, 1883),
#[cfg(feature = "websocket")]
"ws" | "wss" => (Transport::Ws, 8000),
_ => return Err(OptionError::Scheme),
};
let port = url.port().unwrap_or(default_port);
let mut queries = url.query_pairs().collect::<HashMap<_, _>>();
let id = queries
.remove("client_id")
.ok_or(OptionError::ClientId)?
.into_owned();
let mut options = MqttOptions::new(id, host, port);

I wrote my own logic to handle this:

https://github.com/EdJoPaTo/mqttui/blob/4759bef5005aeb9aef1c8129aef1a021ff36cb3c/src/main.rs#L29-L60

@EdJoPaTo EdJoPaTo mentioned this issue Aug 23, 2022
Closed
@Jimmy-Z Jimmy-Z mentioned this issue Aug 25, 2022
Closed
@EdJoPaTo
Copy link
Contributor Author

EdJoPaTo commented Sep 9, 2022

Maybe also relevant for this discussion is my decision against using MqttOptions as URL query parameters as they would be mixed up with the WebSocket connection URL. Also the username/password are for the MQTT connection itself. keeping it inside the URL will send them to the web server too. The web server does not care about the MQTT auth and also might have its own auth requiring different username/password than the MQTT auth.
(More background is here EdJoPaTo/mqttui#70 (comment))

I like the idea of combining it to only set one URL but its incorrect and might create conflicts in the long run. Maybe that should be considered when refactoring MqttOptions.

Personally I would just get rid of the parse_url logic. A better MqttOptions interface and usage of enums would also solve the hazzle of configuring the connection. Providing an optional clap feature which implements clap::Derive might also be neat for having multiple tools using the same command line interface option set but thats even further into the future.

@stale stale bot added the stale Not moving forward; blocked label Sep 29, 2022
@swanandx swanandx linked a pull request Jan 29, 2024 that will close this issue
Draft
2 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
stale Not moving forward; blocked
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

refactor Mqttoptions bytebeamio/rumqtt
1 participant
@EdJoPaTo