Water_heater: add temperature input-validation - #178579
Conversation
|
Hey there @home-assistant/core, mind taking a look at this pull request as it has been labeled with an integration ( Code owner commandsCode owners of
|
There was a problem hiding this comment.
Pull request overview
Adds range validation to water-heater temperature service calls.
Changes:
- Validates converted temperatures against entity limits.
- Adds a translated validation error.
- Tests native and converted temperature scenarios.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
homeassistant/components/water_heater/__init__.py |
Adds temperature range validation. |
homeassistant/components/water_heater/strings.json |
Adds the validation error message. |
tests/components/water_heater/test_init.py |
Adds out-of-range service tests. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated no new comments.
Suppressed comments (2)
homeassistant/components/water_heater/strings.json:134
- Include the temperature unit in this validation message. When Home Assistant and the entity use different units, the reported values are converted to the entity unit (for example, the new test submits 40 °C but reports 104.0, 110, and 140), so the current message leaves users unable to tell that those values are Fahrenheit.
"message": "Temperature {check_temp} is not valid for {entity_id}. Value must be between {min_temp} and {max_temp}."
tests/components/water_heater/test_init.py:140
- Parameterize these two cases and share the entity/platform setup. This test duplicates almost the entire preceding test even though only the entity unit, input, and expected converted value differ, making future service-test changes needlessly error-prone.
async def test_set_temperature_raises_out_of_range_with_conversion(
hass: HomeAssistant, config_flow_fixture: None
) -> None:
"""Test setting temperature outside of range with unit conversion raises validation error."""
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated no new comments.
Suppressed comments (4)
tests/components/water_heater/test_init.py:184
- Exercise the upper-bound branch in the conversion case. The current 40 °C input converts to 104 °F, so both new tests only cover values below the minimum and a regression in the maximum check would pass unnoticed.
"""Test if async turn_off calls sync turn_off."""
tests/components/water_heater/test_init.py:141
- Parameterize the two out-of-range cases and share their integration setup. These tests duplicate nearly the entire setup and assertion body, differing only in unit, input, and expected converted value.
"set_temperature",
data,
blocking=True,
)
homeassistant/components/water_heater/init.py:447
- Skip range validation when an entity has no numeric bounds.
tado/water_heater.py:111-125,159-161,196-204can expose the target-temperature feature while both bounds areNone; comparing those values below raisesTypeErrorand prevents a previously valid service call.
min_temp = entity.min_temp
max_temp = entity.max_temp
homeassistant/components/water_heater/init.py:435
- Validate against the same rounded system-unit limits exposed to callers, and clamp boundary values before forwarding them. For a Fahrenheit entity using the default 110 °F minimum on a Celsius system,
capability_attributesadvertises 43.3 °C, but converting that input here produces 109.94 °F and the subsequent check rejects the advertised minimum.
check_temp = TemperatureConverter.convert(
temp, hass.config.units.temperature_unit, entity.temperature_unit
)
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated no new comments.
Suppressed comments (1)
homeassistant/components/water_heater/init.py:447
- Accept displayed boundary values when conversion rounding differs. Capability attributes round via
display_temp; for example, a 110 °F minimum is exposed as 43.3 °C, but converting that submitted value back yields 109.94 °F, so this check rejects the UI's advertised minimum. Compare using the displayed bounds or clamp values that equal a displayed boundary before calling the entity, and cover this conversion case in the test.
if not min_temp <= check_temp <= max_temp:
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
Suppressed comments (1)
homeassistant/components/water_heater/init.py:434
- Rely on the entity base class's non-optional temperature bounds.
WaterHeaterEntity.min_tempandmax_tempreturnfloatand provide defaults when attributes are absent (__init__.py:386-402), so theseNonebranches mask an invalid entity implementation and allow range validation to be skipped; remove the nullable handling and the corresponding no-bounds test.
min_temp_displayed = (
show_temp(hass, min_temp, entity.temperature_unit, entity.precision)
if min_temp is not None
else None
)
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated no new comments.
Suppressed comments (1)
homeassistant/components/water_heater/init.py:454
- Add coverage for the displayed maximum boundary. The new maximum-equality branch is not exercised—the boundary test only verifies the minimum—so the conversion/clamping behavior at the advertised maximum could regress unnoticed; parameterize that test for both boundaries.
elif (
max_temp_displayed is not None
and temp == max_temp_displayed
and max_temp is not None
):
check_temp = max_temp
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated no new comments.
Suppressed comments (2)
tests/components/water_heater/test_init.py:247
- Remove this unsupported
Noneoverride and the associated no-bounds path.WaterHeaterEntity.min_tempandmax_tempare declared to returnfloatand already provide default bounds, so these overrides violate the entity API (and static type checking) instead of modeling a valid water-heater entity.
async def test_set_temperature_with_no_bounds_skips_validation(
hass: HomeAssistant, config_flow_fixture: None
) -> None:
"""Test set temperature does not validate when bounds are not available."""
class NoBoundsWaterHeater(MockWaterHeaterEntity):
homeassistant/components/water_heater/init.py:454
- Add a test for accepting the displayed maximum boundary. The new maximum-specific correction branch is not exercised by the minimum-boundary test, so a regression here could reject the exact
max_tempadvertised to callers.
elif (
max_temp_displayed is not None
and temp == max_temp_displayed
and max_temp is not None
):
check_temp = max_temp
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 4 out of 4 changed files in this pull request and generated no new comments.
Suppressed comments (1)
homeassistant/components/water_heater/init.py:472
- Keep the Airzone API-error tests inside the advertised temperature range.
tests/components/airzone/test_water_heater.py:226sends 80 above its 75 maximum, andtests/components/airzone_cloud/test_water_heater.py:180sends 80 above its 60 maximum, so this new check raisesServiceValidationError(aHomeAssistantError) before either mocked API is called; both tests therefore pass without exercising the failure paths they claim to cover.
if min_temp is not None and max_temp is not None:
if not min_temp <= check_temp <= max_temp:
raise ServiceValidationError(
|
I've started on this PR thinking it could be useful. |
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
7a491fd to
131862b
Compare
| ) | ||
| if min_temp is not None and max_temp is not None: | ||
| if not min_temp <= check_temp <= max_temp: | ||
| raise ServiceValidationError( |
There was a problem hiding this comment.
This PR is changing our entity model. Before we can review this PR there needs to be approval in a discussion in our architecture repository.
https://github.com/home-assistant/architecture/discussions
https://developers.home-assistant.io/docs/core/entity#changing-the-entity-model
There was a problem hiding this comment.
Ok understood, Joost told me something similar :)
|
I'll go ahead and close here for now, since we don't keep pull requests open that are waiting for an architecture discussion. Thanks for your willingness to contribute! |
Proposed change
Add input-validation to the
async_set_temperature()function of thewater_heaterplatform.Type of change
Additional information
Checklist
ruff format 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.To help with the load of incoming pull requests: