rumqttc: Fix: subscribe_many cannot work as implemented, always causing EmptySubscription error.

[FSMaxB]

Type of change

Bug fix (non-breaking change which fixes an issue)

Checklist:

• Formatted with cargo fmt
• Make an entry to CHANGELOG.md if it's relevant to the users of the library. If it's not relevant mention why.
  • Not making an entry in the changelog because this is a regression that was introduced on main and hasn't found it's way into a release yet.

Notes

This fixes the issue with subscribe_many as detailed in #837.
I know that that PR was closed in favor of #813 but that apparently didn't go anywhere for months so I suggest to merge a smaller scoped fix in the meantime. This might also prevent others like me from having to search for this Bug for hours just to find out it was already known and had a fix that just wasn't merged.

To recap: The issue is that subscribe_many consumes the iterator over topics in order to validate them, leaving no actual topics left to subscribe to. This then always leads to an EmptySubscription error in the EventLoop.

[swanandx]

Thanks for the PR! any particular reason to move validation of filters from client.rs to mqttbytes?

[FSMaxB]

any particular reason to move validation of filters from client.rs to mqttbytes?

In order to not consume the iterator, validation had to happen on the finished Subscribe packet (although an alternative would have been to return a Result from Subscribe::new_many. Instead of having to reach into the Subscribe from the outside to validate, it seemed more natural to have a method on it, especially because it can also easily check that it isn't empty (no filters). This then also leads to code-deduplication.

After I did this for subscribe_many, it's only consistent to do the same for the single-topic subscribe as well. This also comes with the additional benefit of having one less Clone of the topic string, because before my change, the let topic = topic.into() would create a String, but Subscribe::new(&topic) would then copy the string again unnecessarily. By passing the Into<String> parameter directly to Subscribe::new, it only needs to turn it into a String once.

[FSMaxB]

Or is this an architectural concern? Something like mqttbytes only containing non-logic data types or similar? I can just as well turn the method into a free-form function that takes a reference if you want.

[swanandx]

Or is this an architectural concern? Something like mqttbytes only containing non-logic data types or similar? I can just as well turn the method into a free-form function that takes a reference if you want.

kinda yes. Anyways filters field of Subscribe is pub, so after constructing Subscribe in client.rs, we can just do something like:

if subscribe.filters.is_empty() || subscribe.filters.iter().any(invalid) {
    reuturn Err;
}
// process normally

wdyt? open to discussion!

[FSMaxB]

Three ideas:

1. Keep the methods as-is, but put the impl block in client.rs. This leaves the code out of mqttbytes but still allows the convenient method syntax.
2. Introduce a function called subscribe_has_valid_filters that takes a reference to Subscribe. This way the same code doesn't have to be written over and over.
3. Inline everything. I don't see the benefit, but if that's what you want, I can do it.

[swanandx]

for now i think 3 ( or if not it then 2 ) will work out best :)

[FSMaxB]

I've pushed an update that implements 2.

[swanandx]

LGTM 🚀 Thanks for the fix!

[coveralls]

Pull Request Test Coverage Report for Build 9642819082

Details

• 0 of 72 (0.0%) changed or added relevant lines in 3 files are covered.
• No unchanged relevant lines lost coverage.
• Overall coverage increased (+0.006%) to 36.129%

---

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
rumqttc/src/v5/mod.rs	0	3	0.0%
rumqttc/src/v5/client.rs	0	33	0.0%
rumqttc/src/client.rs	0	36	0.0%

Totals
Change from base Build 9561486268:	0.006%
Covered Lines:	5970
Relevant Lines:	16524

---

💛 - Coveralls