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

correct MQTT subscription filter #7269

Merged
merged 6 commits into from May 2, 2017

Conversation

Projects
None yet
7 participants
@amigian74
Copy link
Contributor

commented Apr 24, 2017

The current topic filter within the mqtt component won't match something like "+/something/#". This change should work for all subscriptions

Issue: #6449

@mention-bot

This comment has been minimized.

Copy link

commented Apr 24, 2017

@amigian74, 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.

@homeassistant

This comment has been minimized.

Copy link

commented Apr 24, 2017

Hi @amigian74,

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!

@balloob

This comment has been minimized.

Copy link
Member

commented Apr 24, 2017

Please add tests to show that this is working.

"""Test the subscription of wildcard topics."""
mqtt.subscribe(self.hass, '+/test-topic/#', self.record_calls)

fire_mqtt_message(self.hass, 'hello/another-test-topic', 'test-payload')

This comment has been minimized.

Copy link
@houndci-bot

houndci-bot May 2, 2017

line too long (80 > 79 characters)

"""Test the subscription of wildcard topics."""
mqtt.subscribe(self.hass, '+/test-topic/#', self.record_calls)

fire_mqtt_message(self.hass, 'hello/here-iam/test-topic', 'test-payload')

This comment has been minimized.

Copy link
@houndci-bot

houndci-bot May 2, 2017

line too long (81 > 79 characters)

self.assertEqual('hello/test-topic/here-iam', self.calls[0][0])
self.assertEqual('test-payload', self.calls[0][1])

def test_subscribe_topic_level_wildcard_and_wildcard_level_wildcard_no_match(self):

This comment has been minimized.

Copy link
@houndci-bot

houndci-bot May 2, 2017

line too long (87 > 79 characters)

"""Test the subscription of wildcard topics."""
mqtt.subscribe(self.hass, '+/test-topic/#', self.record_calls)

fire_mqtt_message(self.hass, 'hello/test-topic/here-iam', 'test-payload')

This comment has been minimized.

Copy link
@houndci-bot

houndci-bot May 2, 2017

line too long (81 > 79 characters)

"""Test the subscription of wildcard topics."""
mqtt.subscribe(self.hass, '+/test-topic/#', self.record_calls)

fire_mqtt_message(self.hass, 'hello/another-test-topic', 'test-payload')

This comment has been minimized.

Copy link
@houndci-bot

houndci-bot May 2, 2017

line too long (80 > 79 characters)

"""Test the subscription of wildcard topics."""
mqtt.subscribe(self.hass, '+/test-topic/#', self.record_calls)

fire_mqtt_message(self.hass, 'hello/here-iam/test-topic', 'test-payload')

This comment has been minimized.

Copy link
@houndci-bot

houndci-bot May 2, 2017

line too long (81 > 79 characters)

self.assertEqual('hello/test-topic/here-iam', self.calls[0][0])
self.assertEqual('test-payload', self.calls[0][1])

def test_subscribe_topic_level_wildcard_and_wildcard_level_wildcard_no_match(self):

This comment has been minimized.

Copy link
@houndci-bot

houndci-bot May 2, 2017

line too long (87 > 79 characters)

"""Test the subscription of wildcard topics."""
mqtt.subscribe(self.hass, '+/test-topic/#', self.record_calls)

fire_mqtt_message(self.hass, 'hello/test-topic/here-iam', 'test-payload')

This comment has been minimized.

Copy link
@houndci-bot

houndci-bot May 2, 2017

line too long (81 > 79 characters)

amigian74 added some commits May 2, 2017

using raw strings for regular expression
import order changed
@balloob

This comment has been minimized.

Copy link
Member

commented May 2, 2017

Perfect, thanks! 🐬

@balloob balloob merged commit 0e08925 into home-assistant:dev May 2, 2017

3 of 4 checks passed

continuous-integration/travis-ci/pr The Travis CI build failed
Details
cla-bot Everyone involved has signed the CLA
coverage/coveralls Coverage decreased (-0.1%) to 93.479%
Details
hound No violations found. Woof!

@amigian74 amigian74 deleted the amigian74:mqtt-filter branch May 4, 2017

@balloob balloob referenced this pull request May 5, 2017

Merged

0.44 #7444

if sub_part == "+":
reg_ex_parts.append(r"([^\/]+)")
else:
reg_ex_parts.append(sub_part)

This comment has been minimized.

Copy link
@pezinek

pezinek Jun 15, 2017

Contributor

This is not escaping the characters that do have special meaning in regular expressions, and in fact causes issue #7478. Using regular expressions in case where both subscription and topic are variables is only asking for troubles and this fix should be reverted IMO.

@pezinek pezinek referenced this pull request Jun 16, 2017

Merged

No update in MQTT Binary Sensor #7478 #8057

2 of 2 tasks complete

@home-assistant home-assistant locked and limited conversation to collaborators Oct 20, 2017

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
You can’t perform that action at this time.