Skip to content

Validate raw reservation entries before writing them - #149

Merged
eman merged 2 commits into
mainfrom
fix/148-validate-reservation-entries
Oct 1, 2026
Merged

eman merged 2 commits into
mainfrom
fix/148-validate-reservation-entries

Conversation

@eman

@eman eman commented Oct 1, 2026

Copy link
Copy Markdown
Owner

Closes #148.

update_reservations() and update_reservations_confirmed() sent raw entry dicts unchecked. Mode 7, hour 99 or a setpoint of 200 (100 °C) went to the device as-is. A Python True for enable was coerced to 1, which means disabled, so the confirmed helper waited for the wrong echo.

What changes

  • New validate_reservation_entries() in nwp500.mqtt.control, next to validate_recirculation_schedule(). Each entry needs the six protocol fields as plain integers, so bool and float are rejected. enable must be 1 or 2, week a day bitfield (2–254, bit 0 clear), hour 0–23, min 0–59, and mode a DhwOperationSetting id.
  • The heater's own setpoint limits. update_reservations() holds each param to dhw_temperature_min_raw..dhw_temperature_max_raw from the device's feature data. Both are half-degrees Celsius, so no conversion is needed. Feature data is requested if not cached, as set_dhw_temperature() does. If none arrives, it raises DeviceCapabilityError. An empty list needs no feature data.
  • Fail before any I/O. Structural checks run before the feature request, and the confirmed helper runs them before subscribing.
  • Errors are the existing ParameterValidationError and RangeValidationError. They name the entry, as in reservation[2].hour, and range errors carry the bounds.

ReservationEntry stays lenient, so device read-backs always parse. Validation lives on the write path only.

Behaviour change. Callers that wrote out-of-range entries and relied on the heater to clamp them now get an error. For ha_nwp500:

  • its control path always sets one day bit and a real setpoint, Vacation included, so valid plans are unaffected;
  • a plan with a setpoint outside the heater's range now errors instead of being clamped;
  • its raw "set reservations" service schema still allows week 0, which now errors.

Not changed. add_reservation() and update_reservation() still raise ValueError for their own arguments, and still pass the builder's default bounds. The device's limits apply anyway when they write.

Checks. 912 tests pass, 40 of them new in tests/test_reservation_validation.py. Ruff is clean, pyright adds no warnings, and Sphinx builds without new warnings. The API reference, scheduling how-to and CHANGELOG are updated.

update_reservations() and update_reservations_confirmed() sent their
entry dicts unchecked: mode 7, hour 99 or a setpoint of 200 (100 degC)
went to the device as-is. A Python True for enable was coerced to 1,
which means disabled, so the confirmed helper expected the wrong echo.

- Add validate_reservation_entries() in nwp500.mqtt.control, next to
  validate_recirculation_schedule(). Each entry needs the six protocol
  fields as plain integers (bool and float rejected), enable 1 or 2,
  week a day bitfield (2-254, bit 0 clear), hour 0-23, min 0-59 and
  mode a DhwOperationSetting id.
- The controller's update_reservations() also holds each param to the
  setpoint range the heater reports in its feature data
  (dhw_temperature_min_raw..dhw_temperature_max_raw, half-degrees C like
  param). Feature data is requested if not cached, as for
  set_dhw_temperature(); none raises DeviceCapabilityError. An empty
  list needs none.
- Structural checks run before the feature request, and the confirmed
  helper runs them before subscribing.

ReservationEntry stays lenient so device read-backs always parse.

Closes #148
@eman
eman requested a balanced review from Copilot October 1, 2026 20:52

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Copilot review overview

🟡 Changes recommended

Feature-fetch timeouts propagate RuntimeError instead of the promised DeviceCapabilityError.

Review effort: Balanced
Findings: 1 Medium severity

Open (1)
What changed in this PR

Adds write-path validation for raw MQTT reservations, using device-reported temperature limits while keeping read-back models lenient.

Changes:

  • Checks required fields, integer types, protocol ranges, and device setpoint limits.
  • Validates confirmed writes before subscribing.
  • Adds regression tests and updates API documentation, scheduling guidance, and release notes.

Validation: Project tests, linting, and type checking were not run; a focused timeout check was performed.

File Description
tests/​test_reservation_validation.py Tests validation failures, boundaries, and write behavior.
src/​nwp500/​reservations.py Validates confirmed writes before subscribing.
src/​nwp500/​mqtt/​control.py Adds entry validation and device-limit checks.
docs/​reference/​python_api/​mqtt_client.rst Documents validation rules and exceptions.
docs/​how-to/​schedule-operation.rst Explains raw-write validation behavior.
CHANGELOG.rst Records the behavior change.

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Comment thread src/nwp500/mqtt/control.py Outdated
"""
validate_reservation_entries(reservations)
if reservations:
features = await self._get_device_features(device)

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

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

Fixed in 85e47a3. The fetch now goes through _get_reservation_limits(), which wraps a failed fetch in DeviceCapabilityError the way requires_capability does, with the original RuntimeError chained as the cause. The new test, test_feature_fetch_timeout_raises_capability_error, uses the real fetch path with a device-info callback that reports a timeout. It fails without the fix and passes with it.

…ity error

On a cache miss, a feature-response timeout makes _get_device_features()
raise RuntimeError, which escaped update_reservations() before its
"features is None" check, so callers got RuntimeError instead of the
documented DeviceCapabilityError. Commands behind requires_capability
don't see this: the decorator wraps any fetch failure.

Fetch the limits through _get_reservation_limits(), which wraps a failed
fetch the same way and chains the original error. The new test drives the
real fetch path with a device-info callback that times out.

Addresses the review of #149.
@eman
eman merged commit e6da6e1 into main Oct 1, 2026
8 checks passed
@eman
eman deleted the fix/148-validate-reservation-entries branch October 1, 2026 21:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Validate reservation entries on the raw write path (update_reservations / update_reservations_confirmed)

2 participants