Only accept valid hvac actions sent via mqtt #59919
Conversation
probot-home-assistant
bot
added
bugfix
core
has-tests
integration: mqtt
small-pr
|
Hey there @emontnemery, mind taking a look at this pull request as it has been labeled with an integration ( |
|
Well, my test did run through locally last time I tried, fixed that locally now. Will look at the other tests failing. |
|
|
||
| self._action = payload | ||
| self.async_write_ha_state() | ||
| if payload is None or payload in CURRENT_HVAC_ACTIONS: |
There was a problem hiding this comment.
First of all, it's not clear from the documentation that the action should be set to None.
Is that allowed?
We probably also need a special payload which will be interpreted as None when templates are not used, if it's allowed to set the action to None. This is used in MQTT fan to reset the percentage to None: https://www.home-assistant.io/integrations/fan.mqtt/#payload_reset_percentage
There was a problem hiding this comment.
I basically just added None there based on @bdraco's comment. Setting None via async_fire_mqtt_message doesn't work apparently(?).
Anyway, based on the doc, would it make sense to have the unit test run something along the lines of
assert all(elem in actions for elem in CURRENT_HVAC_ACTIONS) in order to catch future edits of CURRENT_HVAC_ACTIONS?
There was a problem hiding this comment.
It's not possible to receive None directly over MQTT, an empty MQTT payload will be interpreted as the empty string.
If it's necessary to allow resetting the action to None, we should enable that functionality in the same way as for MQTT fan.
If it's not necessary, drop the None-check.
Your idea for a test seems reasonable.
There was a problem hiding this comment.
tbh I am not sure None is a valid action that can be set given the description of HVAC action. I suppose None really just makes sense as the initial value to be interpreted as "unknown" - which no further update should result in? Unless a device is paired/reset again maybe.
I removed the None check for now given your remarks and added the assert all
There was a problem hiding this comment.
For clarity
The explicit None is a valid value hvac_action
The string "None" is not a valid value for hvac_action. (which is what was seen in the HomeKit PR)
There was a problem hiding this comment.
In my mind, if we allow None as a HVAC action (or perhaps rather the absence of a HVAC action) , we should:
- Update the developer docs to make that clear in the same way as it's made clear for fan https://www.home-assistant.io/integrations/fan.mqtt/#payload_reset_percentage
- Make it possible to configure the "None" payload for MQTT Climate, it can perhaps default to
"None".
There's a somewhat related discussion about Fan, about no longer supporting None: #59688
There was a problem hiding this comment.
For reference, https://www.home-assistant.io/integrations/climate.mqtt#action_topic currently only lists the 6 valid values checked in this PR.
Based on @bdraco's comment above, we could default to None (not "None") upon receiving an invalid value since there is some unknown state not handled by HA in place until further notice (similar to what the state within HA is upon startup, thus being a valid case?).
However, any side-effects of doing so are sorta unknown I guess? A warning should be logged in that case imho
There was a problem hiding this comment.
If None isn't actually supported now there is no need to add it. It would also be perfectly reasonable to reject invalid values instead of setting them to None
There was a problem hiding this comment.
So, how should we proceed? Take this PR as the bugfix that it is intended to be and not accept values not documented or extend it to allow some variation of None?
There was a problem hiding this comment.
I think None is only valid as an initial state. There is no reason to reset it later e.g. if the device becomes unavailable. This is different from the MQTT fan where speed percentage and preset_mode state can become invalid, e.g. when a fan has one mode auto, and setting a fixed speed percentage will reset this mode.
Further: The value template will return None if a value not could be parsed, we should not use this for other means. I think this PR is fine, but I would log a warning, instead of an error.
Breaking change
Proposed change
Given issue #59330 and PR #59575 (and especially the remarks by @bdraco ), I went ahead and added analogous checks for the hvac action being set in https://github.com/home-assistant/core/blob/dev/homeassistant/components/mqtt/climate.py#L407
I do still suspect the "mode" attr is still being set to "none" (str) based on debugging the test I added. Not sure if the mode is valid that way.
Either way, the test has the list of actions to check hardcoded. I did consider crosschecking against the list at https://github.com/home-assistant/core/blob/dev/homeassistant/components/climate/const.py#L90 given the diff to the documentation https://developers.home-assistant.io/docs/core/entity/climate/#hvac-action (missing CURRENT_HVAC_FAN).
Either way, I still think PR #59575 was a valid PR since the homekit code should handle data more robust.
Type of change
Additional information
Checklist
black --fast homeassistant tests)If user exposed functionality or configuration variables are added/changed:
If the code communicates with devices, web services, or third-party tools:
Updated and included derived files by running:
python3 -m script.hassfest.requirements_all.txt.Updated by running
python3 -m script.gen_requirements_all..coveragerc.The integration reached or maintains the following Integration Quality Scale:
To help with the load of incoming pull requests: