Skip to content

allow wildcards in subscription #12247

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

Merged
balloob merged 8 commits into from
Feb 9, 2018
Merged

allow wildcards in subscription #12247

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
35fc491
allow wildcards in subscription
escoand Feb 8, 2018
8e75869
remove whitespaces
escoand Feb 8, 2018
5bbe118
make function public
escoand Feb 8, 2018
d5fe32e
also implement for mqtt_json
escoand Feb 8, 2018
b90a414
avoid mqtt-outside topic matching
escoand Feb 8, 2018
8ba8f3b
add wildcard tests
escoand Feb 9, 2018
c0c1936
add not matching wildcard tests
escoand Feb 9, 2018
0a25ac6
fix not-matching tests
escoand Feb 9, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 5 additions & 2 deletions homeassistant/components/device_tracker/mqtt.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,8 +36,11 @@ def async_setup_scanner(hass, config, async_see, discovery_info=None):
@callback
def async_tracker_message_received(topic, payload, qos):
"""Handle received MQTT message."""
hass.async_add_job(
async_see(dev_id=dev_id_lookup[topic], location_name=payload))
for subscription in dev_id_lookup:
if mqtt.match_topic(subscription, topic):

@OttoWinter OttoWinter Feb 8, 2018

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why not just create a separate callback for each subscription?

@OttoWinter OttoWinter Feb 8, 2018 •
edited
Loading

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Like this:

for dev_id, topic in devices.items():
    @callback
    def async_tracker_message_received(topic, payload, qos):
        """Handle received MQTT message."""
        hass.async_add_job(
            async_see(dev_id=dev_id, location_name=payload))

    yield from mqtt.async_subscribe(
        hass, topic, async_tracker_message_received, qos)

@escoand escoand Feb 8, 2018 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it good style to pass the loop-local dev_id variable into a callback?
What value has the callback internal dev_id?
The one from the loop or something unpredictable?

@OttoWinter OttoWinter Feb 8, 2018 •
edited
Loading

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That is indeed true, this wouldn't work
image

But using a callback creator should work:

def make_callback(dev_id):
    @callback
    def async_tracker_message_received(...):
        # [...]
    return async_tracker_message_received

for dev_id, topic in devices.items():
    yield from mqtt.async_subscribe(hass, topic, make_callback(dev_id), qos)

I believe this would still be better than handling the topic matching yourself, since mqtt already handles matching and knows best how to do that.

@escoand escoand Feb 8, 2018

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

According to https://stackoverflow.com/a/3431699/3775041 this should also work:

    for dev_id, topic in devices.items():
        @callback
        def async_tracker_message_received(topic, payload, qos, dev_id=dev_id):
            """Handle received MQTT message."""
            hass.async_add_job(
                async_see(dev_id=dev_id, location_name=payload))

        yield from mqtt.async_subscribe(
            hass, topic, async_tracker_message_received, qos)

@escoand escoand Feb 8, 2018

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But you are right, the mqtt-outside handling of topics is not the best idea.

@escoand escoand Feb 8, 2018

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

BTW the old code also uses this strange callback-outside variable style with the dev_id_lookup dictionary.

hass.async_add_job(async_see(
dev_id=dev_id_lookup[subscription],
location_name=payload))

for dev_id, topic in devices.items():
dev_id_lookup[topic] = dev_id
Expand Down
10 changes: 5 additions & 5 deletions homeassistant/components/device_tracker/mqtt_json.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -46,8 +46,6 @@ def async_setup_scanner(hass, config, async_see, discovery_info=None):
@callback
def async_tracker_message_received(topic, payload, qos):
"""Handle received MQTT message."""
dev_id = dev_id_lookup[topic]

try:
data = GPS_JSON_PAYLOAD_SCHEMA(json.loads(payload))
except vol.MultipleInvalid:
Expand All @@ -59,9 +57,11 @@ def async_tracker_message_received(topic, payload, qos):
_LOGGER.error("Error parsing JSON payload: %s", payload)
return

kwargs = _parse_see_args(dev_id, data)
hass.async_add_job(
async_see(**kwargs))
for subscription in dev_id_lookup:
if mqtt.match_topic(subscription, topic):
kwargs = _parse_see_args(dev_id_lookup[subscription], data)
hass.async_add_job(
async_see(**kwargs))

for dev_id, topic in devices.items():
dev_id_lookup[topic] = dev_id
Expand Down
44 changes: 22 additions & 22 deletions homeassistant/components/mqtt/__init__.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -105,6 +105,27 @@ def valid_discovery_topic(value):
return valid_subscribe_topic(value, invalid_chars='#+\0/')


def match_topic(subscription, topic):
"""Test if topic matches subscription."""
reg_ex_parts = []
suffix = ""
if subscription.endswith('#'):
subscription = subscription[:-2]
suffix = "(.*)"
sub_parts = subscription.split('/')
for sub_part in sub_parts:
if sub_part == "+":
reg_ex_parts.append(r"([^\/]+)")
else:
reg_ex_parts.append(re.escape(sub_part))

reg_ex = "^" + (r'\/'.join(reg_ex_parts)) + suffix + "$"

reg = re.compile(reg_ex)

return reg.match(topic) is not None


_VALID_QOS_SCHEMA = vol.All(vol.Coerce(int), vol.In([0, 1, 2]))

CLIENT_KEY_AUTH_MSG = 'client_key and client_cert must both be present in ' \
Expand Down Expand Up @@ -225,7 +246,7 @@ def async_subscribe(hass, topic, msg_callback, qos=DEFAULT_QOS,
@callback
def async_mqtt_topic_subscriber(dp_topic, dp_payload, dp_qos):
"""Match subscribed MQTT topic."""
if not _match_topic(topic, dp_topic):
if not match_topic(topic, dp_topic):
return

if encoding is not None:
Expand Down Expand Up @@ -642,27 +663,6 @@ def _raise_on_error(result):
'Error talking to MQTT: {}'.format(mqtt.error_string(result)))


def _match_topic(subscription, topic):
"""Test if topic matches subscription."""
reg_ex_parts = []
suffix = ""
if subscription.endswith('#'):
subscription = subscription[:-2]
suffix = "(.*)"
sub_parts = subscription.split('/')
for sub_part in sub_parts:
if sub_part == "+":
reg_ex_parts.append(r"([^\/]+)")
else:
reg_ex_parts.append(re.escape(sub_part))

reg_ex = "^" + (r'\/'.join(reg_ex_parts)) + suffix + "$"

reg = re.compile(reg_ex)

return reg.match(topic) is not None


class MqttAvailability(Entity):
"""Mixin used for platforms that report availability."""

Expand Down