Fixed mqtt subscription filter on sys $ topics#8166
Conversation
|
Hello @natemason, When attempting to inspect the commits of your pull request for CLA signature status among all authors we encountered commit(s) which were not linked to a GitHub account, thus not allowing us to determine their status(es). The commits that are missing a linked GitHub account are the following:
Unfortunately, we are unable to accept this pull request until this situation is corrected. Here are your options:
We apologize for this inconvenience, especially since it usually bites new contributors to Home Assistant. We hope you understand the need for us to protect ourselves and the great community we all have built legally. The best thing to come out of this is that you only need to fix this once and it benefits the entire Home Assistant and GitHub community. Thanks, I look forward to checking this PR again soon! ❤️ |
|
@natemason, thanks for your PR! By analyzing the history of the files in this pull request, we identified @balloob, @pvizeli and @fabaff to be potential reviewers. |
| subscription = subscription[:-2] | ||
| suffix = "(.*)" | ||
| if subscription.startswith('$'): | ||
| subscription = "\\" + subscription |
|
Hi @natemason, It seems you haven't yet signed a CLA. Please do so here. Once you do that we will be able to review and accept this pull request. Thanks! |
|
Please add a unittest for it |
balloob
left a comment
balloob
left a comment
There was a problem hiding this comment.
This will also require a test to be written.
| if subscription.endswith('#'): | ||
| subscription = subscription[:-2] | ||
| suffix = "(.*)" | ||
| if subscription.startswith('$'): |
There was a problem hiding this comment.
I don't think that this is the correct solution. Instead, we should call re.escape() on each part of the topic that's not +.
| """Test the subscription of $ root and wildcard subtree topics.""" | ||
| mqtt.subscribe(self.hass, '$test-topic/subtree/#', self.record_calls) | ||
|
|
||
| fire_mqtt_message(self.hass, '$test-topic/subtree/some-topic', 'test-payload') |
There was a problem hiding this comment.
line too long (86 > 79 characters)
| """Test the subscription of $ root and wildcard subtree topics.""" | ||
| mqtt.subscribe(self.hass, '$test-topic/subtree/#', self.record_calls) | ||
|
|
||
| fire_mqtt_message(self.hass, '$test-topic/subtree/some-topic', 'test-payload') |
There was a problem hiding this comment.
line too long (86 > 79 characters)
balloob
force-pushed
the
fix-mqtt-sys-topic
branch
from
4f58723 to
491cdba
Compare
|
Oh no, it looks like the same fix was already merged 9 days ago in #8057. I however still think that your tests are valuable ! |
|
Okay thanks Paulus. Just glad to contribute (even if it was already fixed :) ) |
* Fixed mqtt subscription filter on sys $ topics * fixed linting issue * added unit tests for $ topics and changed fix to use re.escape * merge upstream/dev mqtt unit tests * Update test_init.py
7 participants
Description:
MQTT subscriptions to a topic starting with $ (typically system/management topics) aren't handled as the $ isn't escaped and regex interprets it as end of line. This PR fixes support those topics.
Pull request in home-assistant.github.io with documentation (if applicable): home-assistant/home-assistant.io#8166
Checklist:
toxrun successfully.