Skip to content

Commit a73bfef

Browse files
fix(compare): skip malformed compare_list entries instead of crashing, with one shared check (#5488)
* fix(compare): skip a compare_list entry with no id instead of crashing (#5472) Compare.run_all() indexed tariff["id"] directly, so a compare_list entry missing its id line raised KeyError every 5-minute cycle - and because the crash skipped predbat.py's compare_active reset, the comparison (and the crash) kept re-triggering until the config was fixed. The id lookup and its warning are now in their own get_tariff_id() method, so the guard can be tested directly rather than only by inspecting run_all()'s source text (code review for #5472 found the source-inspection test alone would pass a guard that checked the wrong field, e.g. tariff.get("name")). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * test(compare): behavioural tests for the id-less skip and compare_active reset (#5472) Replace T28's source-text check with a run_all() run against the real fixture (run_single/save_yaml stubbed) showing only the valid tariff is compared and persisted. Extract the compare_active set/clear into PredBat.run_comparison() so T29 can make run_all() raise and check the flag is cleared, without driving all of update_pred(). Both tests fail with the fixes reverted. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> * fix(compare): validate compare_list once, skipping every malformed entry (#5472) get_tariff_id() only caught a missing id. A non-dict entry, a duplicate id or a nameless entry still misbehaved in run_all(), and an unquoted numeric id (YAML int) still crashed publish_data() building the sensor name - every cycle, via publish_only(). Replace it with module-level compare_entry_problem()/valid_compare_list(), which run_all(), publish_only() and load_yaml() use to drop bad entries (warning with the entry's position) and normalise ids to strings. load_yaml() also str()s stored keys so a numeric key saved earlier can't crash startup. T29 now restores compare_active on the shared fixture. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> * fix(web): share compare.py's compare_list rules in the editor and compare page (#5472) _validate_compare_list() now applies compare_entry_problem(), so the apps.yaml editor refuses exactly what a comparison would skip - now including a blank id and a numeric id clashing with the same id quoted. html_compare() reads valid_compare_list(), so a malformed entry no longer crashes the page and a numeric id finds its string-keyed result. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> * fix(compare): one check_compare_list() for engine, editor and page (#5472) Review follow-up. check_compare_list() returns the usable entries plus a message per skipped one, so web.py's validator no longer re-implements the loop. It also covers a compare_list that isn't a list (was a TypeError every cycle via publish_only), strips padded ids so ' agile' can't sidestep the duplicate check, and str()s a numeric name, which otherwise crashed publish_data() - and, once saved, every startup. The Compare page now says why an entry was skipped instead of dropping it silently or claiming nothing is configured. Tests now assert the run_all() warnings and the editor's refusal reasons. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> * fix(compare): reject a non-mapping config, blank name; report every problem (#5472) Second review round. check_compare_list() now skips an entry whose config: isn't a mapping (it crashed run_all() outside the per-tariff try) and treats an empty config: as no overrides; strips the name so a blank one is refused like a blank id. publish_data() str()s a stored name so one saved as a number can't stop startup. The editor lists every compare_list problem at once rather than one per save. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> * fix(compare): keep treating an empty config: list or string as no overrides (#5472) Third review round. The previous commit skipped any entry whose config: wasn't a mapping, which wrongly dropped working tariffs with config: [] or "" (both always meant no overrides). Only a non-empty non-mapping config - which crashed run_all()'s override snapshot - is skipped now, and the comment names that real crash site. T28 now drives a list config through run_all() itself. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> * fix(compare): spell out which config: values mean no overrides (#5472) Review follow-up. Only None, "", [] and {} count as an empty config: and are dropped; any other non-mapping (false, 0, a string, a list) is skipped with a warning instead of silently losing the overrides. check_compare_list()'s docstring now states the shape it guarantees, and the comment names where a list config really crashed (overrides looked up by key in run_all()/fetch_config()). Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> * test(compare): pin which config: values are kept, dropped or skipped (#5472) Review follow-up. One EMPTY_COMPARE_CONFIG tuple now decides both the skip and the drop, rather than a tuple in one place and truthiness in the other. T27 checks the reason each config entry was skipped, and adds a mapping config, which must survive as the tariff's overrides - nothing tested that, so a check skipping or dropping every config would have passed. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> --------- Co-authored-by: Claude Sonnet 5 <noreply@anthropic.com>
1 parent 0bca948 commit a73bfef

6 files changed

Lines changed: 361 additions & 35 deletions

File tree

‎apps/predbat/compare.py‎

Lines changed: 71 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -27,6 +27,65 @@
2727
# 3. Consider Octopus API key to access current tariff info and switch links
2828

2929

30+
# An empty 'config:' line means no overrides; any other non-mapping value can't be applied
31+
EMPTY_COMPARE_CONFIG = (None, "", [], {})
32+
33+
34+
def check_compare_list(compare_list):
35+
"""Split compare_list into the entries that can be compared and a message per skipped entry (#5472).
36+
37+
Each kept entry is a copy with its id and name as stripped strings, and its config either a
38+
mapping of overrides or absent, so run_all() and fetch_config() can rely on that shape.
39+
"""
40+
if not compare_list:
41+
return [], []
42+
if not isinstance(compare_list, list):
43+
return [], ["compare_list must be a list of tariffs, each with an id and a name"]
44+
45+
valid = []
46+
problems = []
47+
seen_ids = set()
48+
for index, entry in enumerate(compare_list):
49+
problem = None
50+
entry_id = ""
51+
if not isinstance(entry, dict):
52+
problem = "must be a dictionary with an id and a name"
53+
else:
54+
# Results, sensor names and the web page are all keyed by the id as a string, so an
55+
# unquoted numeric id in apps.yaml must not reach them as an int
56+
entry_id = "" if entry.get("id") is None else str(entry["id"]).strip()
57+
# A numeric name would likewise break publish_data() building the friendly name
58+
entry_name = "" if entry.get("name") is None else str(entry["name"]).strip()
59+
if not entry_id:
60+
problem = "requires a non-empty 'id'"
61+
elif entry_id in seen_ids:
62+
problem = "has a duplicate id '{}'".format(entry_id)
63+
elif not entry_name:
64+
problem = "('{}') requires a non-empty 'name'".format(entry_id)
65+
elif entry.get("config") not in EMPTY_COMPARE_CONFIG and not isinstance(entry["config"], dict):
66+
# run_all() and fetch_config() look overrides up by key; a list of them raised TypeError
67+
# there and ended the whole comparison, and any other value was never a usable override
68+
problem = "('{}') has a 'config' that isn't a set of 'setting: value' overrides".format(entry_id)
69+
if problem:
70+
problems.append("compare_list entry {} {}".format(index + 1, problem))
71+
continue
72+
seen_ids.add(entry_id)
73+
checked = dict(entry, id=entry_id, name=entry_name)
74+
if checked.get("config") in EMPTY_COMPARE_CONFIG:
75+
checked.pop("config", None)
76+
valid.append(checked)
77+
return valid, problems
78+
79+
80+
def valid_compare_list(compare_list, log=None):
81+
"""Return the compare_list entries that can be compared, logging a warning for each one skipped (#5472)."""
82+
valid, problems = check_compare_list(compare_list)
83+
if log:
84+
for problem in problems:
85+
log("Warn: Compare, {}, skipping it".format(problem))
86+
return valid
87+
88+
3089
class Compare:
3190
"""Tariff comparison engine.
3291
@@ -483,12 +542,14 @@ def load_yaml(self):
483542
try:
484543
data = yaml.safe_load(f)
485544
if data:
486-
self.comparisons = data.get("comparisons", {})
545+
# str() keys: a numeric id saved before ids were normalised (#5472) would
546+
# otherwise break publish_data() and never match its compare_list entry again
547+
self.comparisons = {str(tariff_id): result for tariff_id, result in (data.get("comparisons") or {}).items()}
487548
except yaml.YAMLError as exc:
488549
self.log("Error loading comparisons: {}".format(exc))
489550

490551
if self.comparisons:
491-
compare_list = self.pb.get_arg("compare_list", [])
552+
compare_list = valid_compare_list(self.pb.get_arg("compare_list", []))
492553
self.select_best(compare_list, self.comparisons)
493554
self.publish_data()
494555

@@ -512,7 +573,8 @@ def publish_data(self):
512573

513574
if result:
514575
cost = result.get("cost", 0)
515-
name = result.get("name", "")
576+
# str(): a numeric name saved before names were normalised (#5472) must not crash startup
577+
name = str(result.get("name", ""))
516578
attributes = {
517579
"friendly_name": "Compare " + name,
518580
"state_class": "measurement",
@@ -539,7 +601,7 @@ def publish_only(self):
539601
"""
540602
Update HA sensors only
541603
"""
542-
compare_list = self.pb.get_arg("compare_list", [])
604+
compare_list = valid_compare_list(self.pb.get_arg("compare_list", []))
543605
if not compare_list:
544606
return
545607

@@ -597,7 +659,7 @@ def run_all(self, debug=False, fetch_sensor=True):
597659
Compare a comparison in prices across multiple energy tariffs and report results
598660
take care not to destroy the state of the system for the primary settings
599661
"""
600-
compare_list = self.pb.get_arg("compare_list", [])
662+
compare_list = valid_compare_list(self.pb.get_arg("compare_list", []), log=self.log)
601663
if not compare_list:
602664
return
603665

@@ -672,7 +734,8 @@ def run_all(self, debug=False, fetch_sensor=True):
672734

673735
try:
674736
for tariff in compare_list:
675-
prior = results.get(tariff["id"], {})
737+
tariff_id = tariff["id"]
738+
prior = results.get(tariff_id, {})
676739
if prior:
677740
prior_date = prior.get("date", "")[:10]
678741
if prior_date == today_date:
@@ -684,11 +747,11 @@ def run_all(self, debug=False, fetch_sensor=True):
684747
else:
685748
# First ever run for this tariff: start from actual midnight SoC
686749
start_soc = soc_midnight_fallback
687-
self.log("Compare tariff {} starting SoC: {:.2f} kWh".format(tariff.get("id", ""), start_soc))
750+
self.log("Compare tariff {} starting SoC: {:.2f} kWh".format(tariff_id, start_soc))
688751
try:
689752
result_data = self.run_single(tariff, rate_import_base, rate_export_base, end_record, debug=debug, fetch_sensor=fetch_sensor, car_charging_slots=save_car_charging_slots, start_soc=start_soc, io_adjusted_base=io_adjusted_base)
690753
if result_data is not None:
691-
results[tariff["id"]] = result_data
754+
results[tariff_id] = result_data
692755
finally:
693756
# Restore hardware settings after each tariff so overrides don't bleed into the next tariff;
694757
# in a finally so a raised run_single() can't leave compare values applied to the live instance

‎apps/predbat/predbat.py‎

Lines changed: 10 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -1070,6 +1070,15 @@ def update_car_manual_soc(self):
10701070
soc_next = min(soc_next, self.car_charging_limit[car_n])
10711071
self.expose_config("car_charging_manual_soc_kwh" + car_postfix, dp3(soc_next))
10721072

1073+
def run_comparison(self):
1074+
"""Run the tariff comparison, clearing compare_active afterwards even if it raises (#5472)."""
1075+
# Without the finally, a crashed comparison leaves compare_active on and re-triggers itself every cycle
1076+
self.expose_config("compare_active", True)
1077+
try:
1078+
self.comparison.run_all()
1079+
finally:
1080+
self.expose_config("compare_active", False)
1081+
10731082
def update_pred(self, scheduled=True):
10741083
"""
10751084
Update the prediction state, everything is called from here right now
@@ -1431,9 +1440,7 @@ def update_pred(self, scheduled=True):
14311440
if self.comparison:
14321441
if (scheduled and self.minutes_now < RUN_EVERY) or self.get_arg("compare_active", False):
14331442
# Compare tariffs either when triggered or daily at midnight
1434-
self.expose_config("compare_active", True)
1435-
self.comparison.run_all()
1436-
self.expose_config("compare_active", False)
1443+
self.run_comparison()
14371444
else:
14381445
# Otherwise just update HA sensors to prevent then expiring
14391446
self.comparison.publish_only()

0 commit comments

Comments
 (0)