Skip to content

Remove MQTT state vacuum value_template support.#33536

Merged
balloob merged 2 commits intohome-assistant:devfrom
emontnemery:mqtt_state_vacuum_fix
Apr 2, 2020
Merged

Remove MQTT state vacuum value_template support.#33536
balloob merged 2 commits intohome-assistant:devfrom
emontnemery:mqtt_state_vacuum_fix

Conversation

@emontnemery
Copy link
Copy Markdown
Contributor

@emontnemery emontnemery commented Apr 2, 2020

Proposed change

Remove MQTT state vacuum value_template support.
The implementation is not working, so this can not be considered a breaking change.

Type of change

  • Dependency upgrade
  • Bugfix (non-breaking change which fixes an issue)
  • New integration (thank you!)
  • New feature (which adds functionality to an existing integration)
  • Breaking change (fix/feature causing existing functionality to break)
  • Code quality improvements to existing code or addition of tests

Additional information

  • This PR fixes or closes issue: fixes #
  • This PR is related to issue:
  • Link to documentation pull request:

Checklist

  • The code change is tested and works locally.
  • Local tests pass. Your PR cannot be merged unless tests pass
  • There is no commented out code in this PR.
  • I have followed the development checklist
  • The code has been formatted using Black (black --fast homeassistant tests)
  • Tests have been added to verify that the new code works.

If user exposed functionality or configuration variables are added/changed:

If the code communicates with devices, web services, or third-party tools:

  • The manifest file has all fields filled out correctly.
    Updated and included derived files by running: python3 -m script.hassfest.
  • New or updated dependencies have been added to requirements_all.txt.
    Updated by running python3 -m script.gen_requirements_all.
  • Untested files have been added to .coveragerc.

The integration reached or maintains the following Integration Quality Scale:

  • No score or internal
  • 🥈 Silver
  • 🥇 Gold
  • 🏆 Platinum

@probot-home-assistant
Copy link
Copy Markdown

Hey there @home-assistant/core, mind taking a look at this pull request as its been labeled with a integration (mqtt) you are listed as a codeowner for? Thanks!

@emontnemery
Copy link
Copy Markdown
Contributor Author

@pszafer Please review if you have time.

@pszafer
Copy link
Copy Markdown
Contributor

pszafer commented Apr 2, 2020

I found one repo which uses string as state
https://github.com/mannkind/ESPHomeRoombaComponent/blob/master/ESPHomeRoombaComponent.h

but I think it couldn't work as author of this repo expected.

@emontnemery
Copy link
Copy Markdown
Contributor Author

@pszafer I think the code in your example won't be affected by this PR.
This code:
https://github.com/mannkind/ESPHomeRoombaComponent/blob/af753cd7210d1be600c4bb5ac0cd0a200f405d8b/ESPHomeRoombaComponent.h#L138-L142

Seems well aligned with the state vacuum MQTT schema without a value_template here: https://www.home-assistant.io/integrations/vacuum.mqtt/#state-mqtt-protocol

payload = template.async_render_with_possible_json_value(payload)
else:
payload = json.loads(payload)
payload = json.loads(payload)
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.

What happens if JSON parsing fails?

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.

We don't allow templates to render JSON that needs to be parsed.

payload = template.async_render_with_possible_json_value(payload)
else:
payload = json.loads(payload)
payload = json.loads(payload)
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.

We don't allow templates to render JSON that needs to be parsed.

@emontnemery
Copy link
Copy Markdown
Contributor Author

@balloob The payload expected on the state_topic is a valid JSON dict, and the output of a template is always a string so the template must render JSON. What's the problem with parsing the output?

This is similar to https://www.home-assistant.io/integrations/sensor.mqtt/#json-attributes-template-configuration.

Since the current implementation of the value_template for MQTT state vacuum is broken, another option would be to just remove the configuration option.

@emontnemery
Copy link
Copy Markdown
Contributor Author

This is the implementation of json_attributes_template:

@callback
@log_messages(self.hass, self.entity_id)
def attributes_message_received(msg: Message) -> None:
try:
payload = msg.payload
if attr_tpl is not None:
payload = attr_tpl.async_render_with_possible_json_value(payload)
json_dict = json.loads(payload)
if isinstance(json_dict, dict):
self._attributes = json_dict
self.async_write_ha_state()
else:
_LOGGER.warning("JSON result was not a dictionary")
self._attributes = None
except ValueError:
_LOGGER.warning("Erroneous JSON: %s", payload)
self._attributes = None

@emontnemery emontnemery changed the title Fix MQTT state vacuum value_template support. Remove MQTT state vacuum value_template support. Apr 2, 2020
@emontnemery
Copy link
Copy Markdown
Contributor Author

PR updated to remove state_template support instead.

@balloob balloob merged commit 3df46cd into home-assistant:dev Apr 2, 2020
@lock lock Bot locked and limited conversation to collaborators Apr 4, 2020
@emontnemery emontnemery deleted the mqtt_state_vacuum_fix branch June 10, 2020 18:47
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants