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.

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

rumqttc: unable to specify subscription options in v5 #697

Open
swanandx opened this issue Aug 27, 2023 · 5 comments
Open

rumqttc: unable to specify subscription options in v5 #697

swanandx opened this issue Aug 27, 2023 · 5 comments
Labels
good first issue Good for newcomers

Comments

@swanandx
Copy link
Member

The current API of v5 client doesn't let us specify subscription options while subscribing.

  • rumqttc version: "0.22.0"

Context

MQTTv5 introduces subscription options. This options like preserve_retain, retain_forward_rule are preset in Filter defined here:

pub struct Filter {
    pub path: String,
    pub qos: QoS,
    pub nolocal: bool,
    pub preserve_retain: bool,
    pub retain_forward_rule: RetainForwardRule,
}

but currently we can't pass this options to Filter::new(_), instead it uses default values:

impl Filter {
    pub fn new<T: Into<String>>(topic: T, qos: QoS) -> Self {
        Self {
            path: topic.into(),
            qos,
            ..Default::default()
        }
    }
    //...
}

we should be able to pass down those options while creating Filter.

Possible solutions ideas / challenges

We can either just add more arguments to new(_) or use a builder pattern for creating Filter, whichever is better.

We also need to think upon how the users will specify them:

  • Do we pass this subscription options as arguments to subscribe(_)? ( would make API very verbose unnecessary as most users won't use these )
  • Do we introduce a separate function? ( the we will have way too many fns just for subscribing haha ).

What I would prefer is: Instead of passing `topic`, `qos` to `subscribe(_)`, user will build `Filter` using builder pattern and the pass it to `subscribe(filter: Filter)`. Something like:
- pub async fn subscribe<S: Into<String>>(&self, topic: S, qos: QoS) -> Result<(), ClientError> {
+ pub async fn subscribe(&self, filter: Filter) -> Result<(), ClientError> {

Please feel free to comment your ideas / suggestions 🚀

@swanandx swanandx added the good first issue Good for newcomers label Aug 27, 2023
ecarrara added a commit to ecarrara/rumqtt that referenced this issue Aug 28, 2023
Create `FilterBuilder` to build `Filter` using the
builder pattern.

Closes bytebeamio#697
@ecarrara
Copy link

@swanandx I made a PR that:

  • Create the FilterBuilder
  • Refactor the subscribe fns to receive a instance of Filter

It looks like a big breaking change. I'm not sure if this is the best way to do that, but I liked how the API looks receiving a Filter instance built with the builder pattern:

client
    .subscribe(
        Filter::builder()
            .path("hello/world")
            .qos(Some(QoS::AtLeastOnce))
            .build(),
    )
    .await
    .unwrap();

@swanandx
Copy link
Member Author

hey @ecarrara , thank you so much for the PR! ( I didn't expect PR this fast haha 🚀 )

It looks like a big breaking change

Yes, I do agree with that. We are planning to reconsider the APIs and integrate builder pattern wherever required in rumqttc, then we can introduce breaking changes at once, but currently we are totally occupied with rumqttd related tasks 😅 , so it might take a while to review #698 .

Thank you for understanding.

@brianmay
Copy link
Contributor

It is possible to construct a Filter object by hand, the values are public. But unfortunately the RetainForwardRule type is in the subscribe.rs which is not public, and as a result it is not possible to create the retain_forward_rule value.

brianmay added a commit to brianmay/robotica-rust that referenced this issue Dec 12, 2023
@drauschenbach
Copy link

It is possible to construct a Filter object by hand, the values are public. But unfortunately the RetainForwardRule type is in the subscribe.rs which is not public, and as a result it is not possible to create the retain_forward_rule value.

And likewise the Strategy enum is private, even though it is used in the public member RouterConfig.shared_subscriptions_strategy, which is interfering with my ability to construct a RouterConfig from a unit test.

@swanandx
Copy link
Member Author

Hey @drauschenbach , good catch! would you like to open PR to solve the issue? ( just need to add pub use Strategy in router/mod.rs ig )

Otherwise, I will do it first thing tomorrow!

Thanks 😁

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
good first issue Good for newcomers
Projects
None yet
Development

No branches or pull requests

4 participants