From 71e82eccf77770e795171685151085026a3a4e7f Mon Sep 17 00:00:00 2001 From: Claude Date: Tue, 17 Mar 2026 18:26:03 +0000 Subject: [PATCH 01/19] Add Pydantic config validation to eliminate type coercion bugs Introduces Pydantic models for all configuration sections that validate and coerce types at load time, fixing HA addon string-to-int/float issues (e.g. MQTT/EVCC port, time_resolution_minutes, vat/fees/markup). - Add config_model.py with typed models for all config sections - Integrate validate_config() into load_config() in setup.py - Remove manual type coercion from core.py and dynamictariff.py - Add 33 new tests covering model validation and type coercion - Add pydantic>=2.0 dependency https://claude.ai/code/session_015rEfxnRN99qtwpBmEZ3GpP --- pyproject.toml | 3 +- src/batcontrol/config_model.py | 215 +++++++++++ src/batcontrol/core.py | 20 +- src/batcontrol/dynamictariff/dynamictariff.py | 25 +- src/batcontrol/setup.py | 7 +- .../test_config_load_integration.py | 77 ++++ tests/batcontrol/test_config_model.py | 342 ++++++++++++++++++ tests/batcontrol/test_core.py | 27 +- 8 files changed, 674 insertions(+), 42 deletions(-) create mode 100644 src/batcontrol/config_model.py create mode 100644 tests/batcontrol/test_config_load_integration.py create mode 100644 tests/batcontrol/test_config_model.py diff --git a/pyproject.toml b/pyproject.toml index cf64fdf1..d82f8ca4 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -17,7 +17,8 @@ dependencies = [ "cachetools>=5.0", "websockets>=11.0", "schedule>=1.2.0", - "pytz>=2024.2" + "pytz>=2024.2", + "pydantic>=2.0,<3.0" ] # Config for the build system diff --git a/src/batcontrol/config_model.py b/src/batcontrol/config_model.py new file mode 100644 index 00000000..1a8f7953 --- /dev/null +++ b/src/batcontrol/config_model.py @@ -0,0 +1,215 @@ +"""Pydantic configuration models for Batcontrol. + +These models validate and coerce types at config load time, +eliminating scattered type conversion code throughout the codebase. +They also fix HA addon issues where numeric values arrive as strings. +""" +from typing import List, Optional, Union +from pydantic import BaseModel, ConfigDict, field_validator + + +class BatteryControlConfig(BaseModel): + """Battery control parameters.""" + model_config = ConfigDict(extra='allow') + + min_price_difference: float = 0.05 + min_price_difference_rel: float = 0.10 + always_allow_discharge_limit: float = 0.90 + max_charging_from_grid_limit: float = 0.89 + min_recharge_amount: float = 100.0 + + +class BatteryControlExpertConfig(BaseModel): + """Expert tuning parameters for battery control.""" + model_config = ConfigDict(extra='allow') + + charge_rate_multiplier: float = 1.1 + soften_price_difference_on_charging: bool = False + soften_price_difference_on_charging_factor: int = 5 + round_price_digits: int = 4 + production_offset_percent: float = 1.0 + + +class InverterConfig(BaseModel): + """Inverter configuration.""" + model_config = ConfigDict(extra='allow') + + type: str = 'dummy' + address: Optional[str] = None + user: Optional[str] = None + password: Optional[str] = None + max_grid_charge_rate: float = 5000 + max_pv_charge_rate: float = 0 + min_pv_charge_rate: float = 0 + fronius_inverter_id: Optional[str] = None + fronius_controller_id: Optional[str] = None + enable_resilient_wrapper: bool = False + outage_tolerance_minutes: float = 24 + retry_backoff_seconds: float = 60 + # MQTT inverter fields + capacity: Optional[int] = None + min_soc: Optional[int] = None + max_soc: Optional[int] = None + base_topic: Optional[str] = None + + +class UtilityConfig(BaseModel): + """Dynamic tariff provider configuration.""" + model_config = ConfigDict(extra='allow') + + type: str + apikey: Optional[str] = None + url: Optional[str] = None + vat: float = 0.0 + fees: float = 0.0 + markup: float = 0.0 + # Tariff zone fields + tariff_zone_1: Optional[float] = None + zone_1_hours: Optional[str] = None + tariff_zone_2: Optional[float] = None + zone_2_hours: Optional[str] = None + tariff_zone_3: Optional[float] = None + zone_3_hours: Optional[str] = None + + +class MqttConfig(BaseModel): + """MQTT API configuration.""" + model_config = ConfigDict(extra='allow') + + enabled: bool = False + logger: bool = False + broker: str = 'localhost' + port: int = 1883 + topic: str = 'house/batcontrol' + username: Optional[str] = None + password: Optional[str] = None + retry_attempts: int = 5 + retry_delay: int = 10 + tls: bool = False + cafile: Optional[str] = None + certfile: Optional[str] = None + keyfile: Optional[str] = None + tls_version: Optional[str] = None + auto_discover_enable: bool = True + auto_discover_topic: str = 'homeassistant' + + +class EvccConfig(BaseModel): + """EVCC connection configuration.""" + model_config = ConfigDict(extra='allow') + + enabled: bool = False + broker: str = 'localhost' + port: int = 1883 + status_topic: str = 'evcc/status' + loadpoint_topic: Union[str, List[str]] = 'evcc/loadpoints/1/charging' + block_battery_while_charging: bool = True + battery_halt_topic: Optional[str] = None + username: Optional[str] = None + password: Optional[str] = None + tls: bool = False + cafile: Optional[str] = None + certfile: Optional[str] = None + keyfile: Optional[str] = None + tls_version: Optional[str] = None + + +class PvInstallationConfig(BaseModel): + """Single PV installation configuration.""" + model_config = ConfigDict(extra='allow') + + name: str = '' + type: Optional[str] = None + lat: Optional[float] = None + lon: Optional[float] = None + declination: Optional[float] = None + azimuth: Optional[float] = None + kWp: Optional[float] = None # pylint: disable=invalid-name + url: Optional[str] = None + horizon: Optional[str] = None + apikey: Optional[str] = None + algorithm: Optional[str] = None + item: Optional[str] = None + token: Optional[str] = None + # HA Solar Forecast ML fields + base_url: Optional[str] = None + api_token: Optional[str] = None + entity_id: Optional[str] = None + sensor_unit: Optional[str] = None + cache_ttl_hours: Optional[float] = None + + +class ConsumptionForecastConfig(BaseModel): + """Consumption forecast configuration.""" + model_config = ConfigDict(extra='allow') + + type: str = 'csv' + # CSV fields + annual_consumption: Optional[float] = None + load_profile: Optional[str] = None + csv: Optional[dict] = None + # HomeAssistant API fields + homeassistant_api: Optional[dict] = None + base_url: Optional[str] = None + apitoken: Optional[str] = None + entity_id: Optional[str] = None + history_days: Optional[Union[str, List[int]]] = None + history_weights: Optional[Union[str, List[int]]] = None + cache_ttl_hours: Optional[float] = None + multiplier: Optional[float] = None + sensor_unit: Optional[str] = None + + +class BatcontrolConfig(BaseModel): + """Top-level Batcontrol configuration model. + + Validates and coerces all configuration values at load time. + Uses extra='allow' to preserve unknown fields for forward compatibility. + """ + model_config = ConfigDict(extra='allow') + + timezone: str = 'Europe/Berlin' + time_resolution_minutes: int = 60 + loglevel: str = 'info' + logfile_enabled: bool = True + log_everything: bool = False + max_logfile_size: int = 200 + logfile_path: str = 'logs/batcontrol.log' + solar_forecast_provider: str = 'fcsolarapi' + + battery_control: BatteryControlConfig = BatteryControlConfig() + battery_control_expert: Optional[BatteryControlExpertConfig] = None + inverter: InverterConfig = InverterConfig() + utility: UtilityConfig + mqtt: Optional[MqttConfig] = None + evcc: Optional[EvccConfig] = None + pvinstallations: List[PvInstallationConfig] = [] + consumption_forecast: ConsumptionForecastConfig = ( + ConsumptionForecastConfig() + ) + + @field_validator('time_resolution_minutes') + @classmethod + def validate_time_resolution(cls, v): + """Validate time_resolution_minutes is 15 or 60.""" + if v not in (15, 60): + raise ValueError( + f"time_resolution_minutes must be 15 or 60, got {v}" + ) + return v + + +def validate_config(config_dict: dict) -> dict: + """Validate and coerce a raw config dict using Pydantic models. + + Args: + config_dict: Raw configuration dictionary (from YAML/JSON). + + Returns: + Validated and type-coerced configuration dictionary. + + Raises: + pydantic.ValidationError: If validation fails. + """ + validated = BatcontrolConfig(**config_dict) + return validated.model_dump() diff --git a/src/batcontrol/core.py b/src/batcontrol/core.py index 5d6c4768..86b7d706 100644 --- a/src/batcontrol/core.py +++ b/src/batcontrol/core.py @@ -89,23 +89,9 @@ def __init__(self, configdict: dict): self.config = configdict config = configdict - # Extract and validate time resolution (15 or 60 minutes) - # Get time resolution from config, convert string to int if needed - # (HomeAssistant form fields may provide string values) - time_resolution_raw = config.get('time_resolution_minutes', 60) - if isinstance(time_resolution_raw, str): - self.time_resolution = int(time_resolution_raw) - else: - self.time_resolution = time_resolution_raw - - if self.time_resolution not in [15, 60]: - # Note: Python3.11 had issue with f-strings and multiline. Using format() here. - error_message = "time_resolution_minutes must be either " + \ - "15 (quarter-hourly) or 60 (hourly), " + \ - " got '%s'.".format(self.time_resolution) + \ - " Please update your configuration file." - - raise ValueError(error_message) + # time_resolution_minutes is validated and coerced by Pydantic + # in config_model.py (must be int, must be 15 or 60) + self.time_resolution = config.get('time_resolution_minutes', 60) self.intervals_per_hour = 60 // self.time_resolution logger.info( diff --git a/src/batcontrol/dynamictariff/dynamictariff.py b/src/batcontrol/dynamictariff/dynamictariff.py index 030b7d14..45e6fcc5 100644 --- a/src/batcontrol/dynamictariff/dynamictariff.py +++ b/src/batcontrol/dynamictariff/dynamictariff.py @@ -50,9 +50,10 @@ def create_tarif_provider(config: dict, timezone, raise RuntimeError( f'[DynTariff] Please include {field} in your configuration file' ) - vat = float(config.get('vat', 0)) - markup = float(config.get('markup', 0)) - fees = float(config.get('fees', 0)) + # Types already coerced to float by Pydantic config validation + vat = config.get('vat', 0) + markup = config.get('markup', 0) + fees = config.get('fees', 0) selected_tariff = Awattar( timezone, 'at', min_time_between_api_calls, @@ -68,9 +69,9 @@ def create_tarif_provider(config: dict, timezone, raise RuntimeError( f'[DynTariff] Please include {field} in your configuration file' ) - vat = float(config.get('vat', 0)) - markup = float(config.get('markup', 0)) - fees = float(config.get('fees', 0)) + vat = config.get('vat', 0) + markup = config.get('markup', 0) + fees = config.get('fees', 0) selected_tariff = Awattar( timezone, 'de', min_time_between_api_calls, @@ -115,9 +116,9 @@ def create_tarif_provider(config: dict, timezone, raise RuntimeError( f'[DynTariff] Please include {field} in your configuration file' ) - vat = float(config.get('vat', 0)) - markup = float(config.get('markup', 0)) - fees = float(config.get('fees', 0)) + vat = config.get('vat', 0) + markup = config.get('markup', 0) + fees = config.get('fees', 0) token = config.get('apikey') selected_tariff = Energyforecast( timezone, @@ -144,11 +145,11 @@ def create_tarif_provider(config: dict, timezone, min_time_between_api_calls, delay_evaluation_by_seconds, target_resolution=target_resolution, - tariff_zone_1=float(config['tariff_zone_1']), + tariff_zone_1=config['tariff_zone_1'], zone_1_hours=config['zone_1_hours'], - tariff_zone_2=float(config['tariff_zone_2']), + tariff_zone_2=config['tariff_zone_2'], zone_2_hours=config['zone_2_hours'], - tariff_zone_3=float(tariff_zone_3) if tariff_zone_3 is not None else None, + tariff_zone_3=tariff_zone_3, zone_3_hours=zone_3_hours, ) diff --git a/src/batcontrol/setup.py b/src/batcontrol/setup.py index 353d955b..2ce5ad21 100644 --- a/src/batcontrol/setup.py +++ b/src/batcontrol/setup.py @@ -4,6 +4,9 @@ import yaml from logging.handlers import RotatingFileHandler +from .config_model import validate_config + + def setup_logging(level=logging.INFO, logfile=None, max_logfile_size_kb=200): """Configure root logger with consistent formatting. @@ -70,5 +73,7 @@ def load_config(configfile:str) -> dict: pass else: raise RuntimeError('No PV Installation found') - + + config = validate_config(config) + return config diff --git a/tests/batcontrol/test_config_load_integration.py b/tests/batcontrol/test_config_load_integration.py new file mode 100644 index 00000000..f6ab245f --- /dev/null +++ b/tests/batcontrol/test_config_load_integration.py @@ -0,0 +1,77 @@ +"""Integration tests for load_config with Pydantic validation.""" +import os +import tempfile +import pytest +import yaml +from batcontrol.setup import load_config + + +class TestLoadConfigIntegration: + """Test load_config() with Pydantic validation integrated.""" + + def _write_config(self, config_dict, tmpdir): + """Write config dict to a YAML file and return the path.""" + path = os.path.join(tmpdir, 'test_config.yaml') + with open(path, 'w', encoding='UTF-8') as f: + yaml.dump(config_dict, f) + return path + + def _minimal_config(self): + """Return a minimal valid config.""" + return { + 'timezone': 'Europe/Berlin', + 'utility': {'type': 'awattar_de'}, + 'pvinstallations': [{'name': 'Test', 'kWp': 10.0}], + } + + def test_load_config_returns_validated_dict(self, tmp_path): + """Test that load_config returns a validated dict.""" + path = self._write_config(self._minimal_config(), str(tmp_path)) + result = load_config(path) + assert isinstance(result, dict) + assert result['timezone'] == 'Europe/Berlin' + # Pydantic defaults should be filled in + assert result['time_resolution_minutes'] == 60 + + def test_load_config_coerces_string_port(self, tmp_path): + """Test that string ports are coerced to int via load_config.""" + config = self._minimal_config() + config['mqtt'] = { + 'enabled': False, + 'broker': 'localhost', + 'port': '1883', + 'topic': 'test/topic', + 'tls': False, + } + path = self._write_config(config, str(tmp_path)) + result = load_config(path) + assert result['mqtt']['port'] == 1883 + assert isinstance(result['mqtt']['port'], int) + + def test_load_config_missing_file(self): + """Test that missing file raises RuntimeError.""" + with pytest.raises(RuntimeError, match='not found'): + load_config('/nonexistent/path/config.yaml') + + def test_load_config_no_pvinstallations(self, tmp_path): + """Test that empty pvinstallations raises RuntimeError.""" + config = self._minimal_config() + config['pvinstallations'] = [] + path = self._write_config(config, str(tmp_path)) + with pytest.raises(RuntimeError, match='No PV Installation'): + load_config(path) + + def test_load_config_with_dummy_config_file(self): + """Test loading the actual dummy config file.""" + dummy_path = os.path.join( + os.path.dirname(__file__), + '..', '..', 'config', 'batcontrol_config_dummy.yaml' + ) + if not os.path.exists(dummy_path): + pytest.skip('Dummy config file not found') + result = load_config(dummy_path) + assert isinstance(result, dict) + assert result['timezone'] == 'Europe/Berlin' + assert isinstance(result['time_resolution_minutes'], int) + assert isinstance(result['mqtt']['port'], int) + assert isinstance(result['evcc']['port'], int) diff --git a/tests/batcontrol/test_config_model.py b/tests/batcontrol/test_config_model.py new file mode 100644 index 00000000..aa4701dc --- /dev/null +++ b/tests/batcontrol/test_config_model.py @@ -0,0 +1,342 @@ +"""Tests for Pydantic configuration models.""" +import pytest +from pydantic import ValidationError +from batcontrol.config_model import ( + BatcontrolConfig, + BatteryControlConfig, + BatteryControlExpertConfig, + InverterConfig, + UtilityConfig, + MqttConfig, + EvccConfig, + PvInstallationConfig, + ConsumptionForecastConfig, + validate_config, +) + + +class TestBatcontrolConfig: + """Tests for the top-level BatcontrolConfig model.""" + + def _minimal_config(self): + """Return a minimal valid config dict.""" + return { + 'timezone': 'Europe/Berlin', + 'utility': {'type': 'awattar_de'}, + 'pvinstallations': [{'name': 'Test PV', 'kWp': 10.0}], + } + + def test_minimal_config(self): + """Test that a minimal config is valid.""" + cfg = BatcontrolConfig(**self._minimal_config()) + assert cfg.timezone == 'Europe/Berlin' + assert cfg.time_resolution_minutes == 60 + + def test_defaults_applied(self): + """Test that defaults are applied for missing fields.""" + cfg = BatcontrolConfig(**self._minimal_config()) + assert cfg.loglevel == 'info' + assert cfg.logfile_enabled is True + assert cfg.log_everything is False + assert cfg.max_logfile_size == 200 + assert cfg.solar_forecast_provider == 'fcsolarapi' + + def test_time_resolution_string_coercion(self): + """Test that string time_resolution_minutes is coerced to int.""" + data = self._minimal_config() + data['time_resolution_minutes'] = '15' + cfg = BatcontrolConfig(**data) + assert cfg.time_resolution_minutes == 15 + assert isinstance(cfg.time_resolution_minutes, int) + + def test_time_resolution_invalid_value(self): + """Test that invalid time_resolution_minutes raises error.""" + data = self._minimal_config() + data['time_resolution_minutes'] = 30 + with pytest.raises(ValidationError, match='time_resolution_minutes'): + BatcontrolConfig(**data) + + def test_time_resolution_valid_values(self): + """Test both valid time resolution values.""" + for val in [15, 60, '15', '60']: + data = self._minimal_config() + data['time_resolution_minutes'] = val + cfg = BatcontrolConfig(**data) + assert cfg.time_resolution_minutes in (15, 60) + + def test_extra_fields_preserved(self): + """Test that unknown fields are preserved (extra='allow').""" + data = self._minimal_config() + data['custom_field'] = 'custom_value' + cfg = BatcontrolConfig(**data) + assert cfg.custom_field == 'custom_value' + + +class TestBatteryControlConfig: + """Tests for BatteryControlConfig.""" + + def test_defaults(self): + cfg = BatteryControlConfig() + assert cfg.min_price_difference == 0.05 + assert cfg.always_allow_discharge_limit == 0.90 + + def test_string_float_coercion(self): + """Test that string floats are coerced.""" + cfg = BatteryControlConfig( + min_price_difference='0.03', + always_allow_discharge_limit='0.85', + ) + assert cfg.min_price_difference == 0.03 + assert isinstance(cfg.min_price_difference, float) + + +class TestInverterConfig: + """Tests for InverterConfig.""" + + def test_defaults(self): + cfg = InverterConfig() + assert cfg.type == 'dummy' + assert cfg.max_grid_charge_rate == 5000 + assert cfg.enable_resilient_wrapper is False + + def test_string_numeric_coercion(self): + """Test that string numerics are coerced (HA addon issue).""" + cfg = InverterConfig( + max_grid_charge_rate='3000', + outage_tolerance_minutes='30', + ) + assert cfg.max_grid_charge_rate == 3000.0 + assert cfg.outage_tolerance_minutes == 30.0 + + +class TestUtilityConfig: + """Tests for UtilityConfig.""" + + def test_type_required(self): + with pytest.raises(ValidationError): + UtilityConfig() + + def test_string_float_coercion(self): + cfg = UtilityConfig( + type='awattar_de', + vat='0.19', + fees='0.015', + markup='0.03', + ) + assert cfg.vat == 0.19 + assert isinstance(cfg.vat, float) + + def test_tariff_zone_coercion(self): + cfg = UtilityConfig( + type='tariff_zones', + tariff_zone_1='0.2733', + tariff_zone_2='0.1734', + ) + assert cfg.tariff_zone_1 == 0.2733 + assert isinstance(cfg.tariff_zone_1, float) + + +class TestMqttConfig: + """Tests for MqttConfig.""" + + def test_port_string_coercion(self): + """Test the critical HA addon bug: port arrives as string.""" + cfg = MqttConfig(port='1883') + assert cfg.port == 1883 + assert isinstance(cfg.port, int) + + def test_retry_string_coercion(self): + cfg = MqttConfig(retry_attempts='3', retry_delay='5') + assert cfg.retry_attempts == 3 + assert cfg.retry_delay == 5 + + def test_defaults(self): + cfg = MqttConfig() + assert cfg.enabled is False + assert cfg.port == 1883 + assert cfg.broker == 'localhost' + + +class TestEvccConfig: + """Tests for EvccConfig.""" + + def test_port_string_coercion(self): + """Test the critical HA addon bug: port arrives as string.""" + cfg = EvccConfig(port='1883') + assert cfg.port == 1883 + assert isinstance(cfg.port, int) + + def test_loadpoint_topic_string(self): + cfg = EvccConfig(loadpoint_topic='evcc/loadpoints/1/charging') + assert cfg.loadpoint_topic == 'evcc/loadpoints/1/charging' + + def test_loadpoint_topic_list(self): + cfg = EvccConfig( + loadpoint_topic=[ + 'evcc/loadpoints/1/charging', + 'evcc/loadpoints/2/charging', + ] + ) + assert isinstance(cfg.loadpoint_topic, list) + assert len(cfg.loadpoint_topic) == 2 + + +class TestPvInstallationConfig: + """Tests for PvInstallationConfig.""" + + def test_float_coercion(self): + """Test that numeric PV fields are coerced from strings.""" + cfg = PvInstallationConfig( + name='Test', + lat='48.43', + lon='8.77', + kWp='10.5', + declination='30', + azimuth='-90', + ) + assert cfg.lat == 48.43 + assert cfg.kWp == 10.5 + assert cfg.azimuth == -90.0 + assert isinstance(cfg.lat, float) + + +class TestConsumptionForecastConfig: + """Tests for ConsumptionForecastConfig.""" + + def test_defaults(self): + cfg = ConsumptionForecastConfig() + assert cfg.type == 'csv' + + def test_annual_consumption_coercion(self): + cfg = ConsumptionForecastConfig(annual_consumption='4500') + assert cfg.annual_consumption == 4500.0 + + +class TestValidateConfig: + """Tests for the validate_config() function.""" + + def _full_config(self): + """Return a full config dict similar to batcontrol_config_dummy.yaml.""" + return { + 'timezone': 'Europe/Berlin', + 'time_resolution_minutes': 60, + 'loglevel': 'debug', + 'logfile_enabled': True, + 'log_everything': False, + 'max_logfile_size': 200, + 'logfile_path': 'logs/batcontrol.log', + 'battery_control': { + 'min_price_difference': 0.05, + 'min_price_difference_rel': 0.10, + 'always_allow_discharge_limit': 0.90, + 'max_charging_from_grid_limit': 0.89, + 'min_recharge_amount': 100, + }, + 'inverter': { + 'type': 'dummy', + 'max_grid_charge_rate': 5000, + }, + 'utility': { + 'type': 'awattar_de', + 'vat': 0.19, + 'fees': 0.015, + 'markup': 0.03, + }, + 'pvinstallations': [ + { + 'name': 'Test PV', + 'lat': 48.43, + 'lon': 8.77, + 'kWp': 10.0, + 'declination': 30, + 'azimuth': 0, + } + ], + 'consumption_forecast': { + 'type': 'csv', + 'csv': { + 'annual_consumption': 4500, + 'load_profile': 'load_profile.csv', + }, + }, + 'mqtt': { + 'enabled': False, + 'broker': 'localhost', + 'port': 1883, + 'topic': 'house/batcontrol', + 'tls': False, + }, + 'evcc': { + 'enabled': False, + 'broker': 'localhost', + 'port': 1883, + 'status_topic': 'evcc/status', + 'loadpoint_topic': ['evcc/loadpoints/1/charging'], + 'tls': False, + }, + } + + def test_validate_full_config(self): + """Test validation of a complete config dict.""" + result = validate_config(self._full_config()) + assert isinstance(result, dict) + assert result['timezone'] == 'Europe/Berlin' + assert result['time_resolution_minutes'] == 60 + + def test_validate_returns_dict(self): + """Test that validate_config returns a plain dict.""" + result = validate_config(self._full_config()) + assert type(result) is dict + + def test_validate_coerces_string_types(self): + """Test that validation coerces HA addon string values.""" + config = self._full_config() + config['time_resolution_minutes'] = '60' + config['mqtt']['port'] = '1883' + config['evcc']['port'] = '1883' + config['utility']['vat'] = '0.19' + result = validate_config(config) + assert result['time_resolution_minutes'] == 60 + assert result['mqtt']['port'] == 1883 + assert result['evcc']['port'] == 1883 + assert result['utility']['vat'] == 0.19 + + def test_validate_preserves_nested_structure(self): + """Test that nested dict structure is preserved.""" + result = validate_config(self._full_config()) + assert 'battery_control' in result + assert result['battery_control']['min_price_difference'] == 0.05 + assert 'inverter' in result + assert result['inverter']['type'] == 'dummy' + + def test_validate_invalid_time_resolution(self): + """Test that invalid time_resolution_minutes fails validation.""" + config = self._full_config() + config['time_resolution_minutes'] = 45 + with pytest.raises(ValidationError): + validate_config(config) + + def test_ha_addon_string_config(self): + """Simulate HA addon options.json where many values are strings.""" + config = self._full_config() + # Simulate HA string coercion for all numeric fields + config['time_resolution_minutes'] = '60' + config['max_logfile_size'] = '200' + config['battery_control']['min_price_difference'] = '0.05' + config['battery_control']['min_recharge_amount'] = '100' + config['inverter']['max_grid_charge_rate'] = '5000' + config['mqtt']['port'] = '1883' + config['mqtt']['retry_attempts'] = '5' + config['mqtt']['retry_delay'] = '10' + config['evcc']['port'] = '1883' + + result = validate_config(config) + + assert isinstance(result['time_resolution_minutes'], int) + assert isinstance(result['max_logfile_size'], int) + assert isinstance( + result['battery_control']['min_price_difference'], float) + assert isinstance(result['inverter']['max_grid_charge_rate'], float) + assert isinstance(result['mqtt']['port'], int) + assert isinstance(result['mqtt']['retry_attempts'], int) + assert isinstance(result['evcc']['port'], int) diff --git a/tests/batcontrol/test_core.py b/tests/batcontrol/test_core.py index 633566b6..3a77646e 100644 --- a/tests/batcontrol/test_core.py +++ b/tests/batcontrol/test_core.py @@ -290,7 +290,12 @@ def test_api_set_limit_applies_immediately_in_mode_8( class TestTimeResolutionString: - """Test that time_resolution_minutes provided as string (e.g. from Home Assistant) is handled correctly""" + """Test that time_resolution_minutes coercion is handled by Pydantic config validation. + + Since Pydantic validates and coerces types in load_config() before Batcontrol + receives the config dict, Batcontrol.__init__() expects properly typed values. + String-to-int coercion is tested in test_config_model.py. + """ @pytest.fixture def base_mock_config(self): @@ -321,16 +326,16 @@ def base_mock_config(self): } } - @pytest.mark.parametrize('resolution_str,expected_int', [('60', 60), ('15', 15)]) + @pytest.mark.parametrize('resolution_int', [60, 15]) @patch('batcontrol.core.tariff_factory.create_tarif_provider') @patch('batcontrol.core.inverter_factory.create_inverter') @patch('batcontrol.core.solar_factory.create_solar_provider') @patch('batcontrol.core.consumption_factory.create_consumption') - def test_string_time_resolution_initialises_without_error( + def test_time_resolution_initialises_without_error( self, mock_consumption, mock_solar, mock_inverter_factory, mock_tariff, - base_mock_config, resolution_str, expected_int + base_mock_config, resolution_int ): - """Batcontrol must not crash when time_resolution_minutes is a string""" + """Batcontrol initialises correctly with validated int time_resolution_minutes""" mock_inverter = MagicMock() mock_inverter.get_max_capacity = MagicMock(return_value=10000) mock_inverter_factory.return_value = mock_inverter @@ -338,22 +343,22 @@ def test_string_time_resolution_initialises_without_error( mock_solar.return_value = MagicMock() mock_consumption.return_value = MagicMock() - base_mock_config['time_resolution_minutes'] = resolution_str + base_mock_config['time_resolution_minutes'] = resolution_int bc = Batcontrol(base_mock_config) assert isinstance(bc.time_resolution, int) - assert bc.time_resolution == expected_int + assert bc.time_resolution == resolution_int - @pytest.mark.parametrize('resolution_str', ['60', '15']) - def test_logic_factory_accepts_string_resolution_as_int(self, resolution_str): + @pytest.mark.parametrize('resolution_int', [60, 15]) + def test_logic_factory_accepts_resolution_as_int(self, resolution_int): """Logic factory must produce a valid logic instance when given an int resolution""" logic = LogicFactory.create_logic( - int(resolution_str), + resolution_int, {'type': 'default'}, datetime.timezone.utc ) assert logic is not None - assert logic.interval_minutes == int(resolution_str) + assert logic.interval_minutes == resolution_int if __name__ == '__main__': From 2601ea1a137dbd92bb9bfaa1be119dbfeae99272 Mon Sep 17 00:00:00 2001 From: Claude Date: Tue, 17 Mar 2026 18:34:58 +0000 Subject: [PATCH 02/19] Enhance Pydantic config: history_days parsing, inverter alias, loglevel validation - Add field_validators for history_days/history_weights semicolon string parsing (e.g. "-7;-14;-21" -> [-7, -14, -21]) from HA addon - Add model_validator for legacy max_charge_rate -> max_grid_charge_rate rename - Add loglevel validator (must be debug/info/warning/error, normalized to lowercase) - Remove manual type coercion from consumption.py and inverter.py - Add 10 new tests (367 total passing) https://claude.ai/code/session_015rEfxnRN99qtwpBmEZ3GpP --- src/batcontrol/config_model.py | 50 ++++++++++++++- .../forecastconsumption/consumption.py | 14 +---- src/batcontrol/inverter/inverter.py | 11 +--- tests/batcontrol/test_config_model.py | 61 +++++++++++++++++++ 4 files changed, 115 insertions(+), 21 deletions(-) diff --git a/src/batcontrol/config_model.py b/src/batcontrol/config_model.py index 1a8f7953..ce2e016f 100644 --- a/src/batcontrol/config_model.py +++ b/src/batcontrol/config_model.py @@ -5,7 +5,23 @@ They also fix HA addon issues where numeric values arrive as strings. """ from typing import List, Optional, Union -from pydantic import BaseModel, ConfigDict, field_validator +from pydantic import BaseModel, ConfigDict, field_validator, model_validator + + +def _parse_semicolon_int_list(v): + """Parse a semicolon-separated string into a list of ints. + + Handles HA addon config where lists are passed as strings like + "-7;-14;-21". Also accepts regular lists and converts items to int. + Returns None if input is None. + """ + if v is None: + return None + if isinstance(v, str): + return [int(x.strip()) for x in v.split(';')] + if isinstance(v, list): + return [int(x) for x in v] + return v class BatteryControlConfig(BaseModel): @@ -52,6 +68,15 @@ class InverterConfig(BaseModel): max_soc: Optional[int] = None base_topic: Optional[str] = None + @model_validator(mode='before') + @classmethod + def handle_max_charge_rate_rename(cls, data): + """Support legacy config key max_charge_rate -> max_grid_charge_rate.""" + if isinstance(data, dict): + if 'max_charge_rate' in data and 'max_grid_charge_rate' not in data: + data['max_grid_charge_rate'] = data.pop('max_charge_rate') + return data + class UtilityConfig(BaseModel): """Dynamic tariff provider configuration.""" @@ -159,6 +184,18 @@ class ConsumptionForecastConfig(BaseModel): multiplier: Optional[float] = None sensor_unit: Optional[str] = None + @field_validator('history_days', mode='before') + @classmethod + def parse_history_days(cls, v): + """Parse semicolon-separated string to list of ints.""" + return _parse_semicolon_int_list(v) + + @field_validator('history_weights', mode='before') + @classmethod + def parse_history_weights(cls, v): + """Parse semicolon-separated string to list of ints.""" + return _parse_semicolon_int_list(v) + class BatcontrolConfig(BaseModel): """Top-level Batcontrol configuration model. @@ -198,6 +235,17 @@ def validate_time_resolution(cls, v): ) return v + @field_validator('loglevel') + @classmethod + def validate_loglevel(cls, v): + """Validate loglevel is a recognized level.""" + valid = ('debug', 'info', 'warning', 'error') + if v.lower() not in valid: + raise ValueError( + f"loglevel must be one of {valid}, got '{v}'" + ) + return v.lower() + def validate_config(config_dict: dict) -> dict: """Validate and coerce a raw config dict using Pydantic models. diff --git a/src/batcontrol/forecastconsumption/consumption.py b/src/batcontrol/forecastconsumption/consumption.py index 817ed060..0661d66e 100644 --- a/src/batcontrol/forecastconsumption/consumption.py +++ b/src/batcontrol/forecastconsumption/consumption.py @@ -72,21 +72,11 @@ def _create_homeassistant_forecast( base_url = ha_config['base_url'] api_token = ha_config['apitoken'] entity_id = ha_config['entity_id'] + # history_days/history_weights are already parsed by Pydantic validation: + # semicolon-separated strings are converted to int lists at config load time history_days = ha_config.get('history_days', [-7, -14, -21]) history_weights = ha_config.get('history_weights', [1, 1, 1]) - # Configure String -1;-2;-3 to a list and remove spaces - if isinstance(history_days, str): - history_days = [x.strip() for x in history_days.split(';')] - if isinstance(history_weights, str): - history_weights = [x.strip() for x in history_weights.split(';')] - - # Convert string lists to int/float lists (HomeAssistant config quirk) - if isinstance(history_days, list): - history_days = [int(x) for x in history_days] - if isinstance(history_weights, list): - history_weights = [int(x) for x in history_weights] - cache_ttl_hours = ha_config.get('cache_ttl_hours', 48.0) multiplier = ha_config.get('multiplier', 1.0) sensor_unit = ha_config.get('sensor_unit', "auto") diff --git a/src/batcontrol/inverter/inverter.py b/src/batcontrol/inverter/inverter.py index c12306c4..acaa0172 100644 --- a/src/batcontrol/inverter/inverter.py +++ b/src/batcontrol/inverter/inverter.py @@ -18,14 +18,9 @@ class Inverter: @staticmethod def create_inverter(config: dict) -> InverterInterface: """ Select and configure an inverter based on the given configuration """ - # renaming of parameters max_charge_rate -> max_grid_charge_rate - if not 'max_grid_charge_rate' in config.keys(): - config['max_grid_charge_rate'] = config['max_charge_rate'] - - # introducing parameter max_pv_charge_rate. Assign default value here, - # in case there is no value defined in the config file to avoid a KeyError - if not 'max_pv_charge_rate' in config.keys(): - config['max_pv_charge_rate'] = 0 + # Legacy rename max_charge_rate -> max_grid_charge_rate and + # default for max_pv_charge_rate are now handled by Pydantic + # InverterConfig model_validator in config_model.py inverter = None diff --git a/tests/batcontrol/test_config_model.py b/tests/batcontrol/test_config_model.py index aa4701dc..6385cd5a 100644 --- a/tests/batcontrol/test_config_model.py +++ b/tests/batcontrol/test_config_model.py @@ -71,6 +71,28 @@ def test_extra_fields_preserved(self): cfg = BatcontrolConfig(**data) assert cfg.custom_field == 'custom_value' + def test_loglevel_valid_values(self): + """Test all valid loglevel values.""" + for level in ['debug', 'info', 'warning', 'error', 'DEBUG', 'Info']: + data = self._minimal_config() + data['loglevel'] = level + cfg = BatcontrolConfig(**data) + assert cfg.loglevel == level.lower() + + def test_loglevel_invalid_value(self): + """Test that invalid loglevel raises error.""" + data = self._minimal_config() + data['loglevel'] = 'verbose' + with pytest.raises(ValidationError, match='loglevel'): + BatcontrolConfig(**data) + + def test_loglevel_normalized_to_lowercase(self): + """Test that loglevel is normalized to lowercase.""" + data = self._minimal_config() + data['loglevel'] = 'DEBUG' + cfg = BatcontrolConfig(**data) + assert cfg.loglevel == 'debug' + class TestBatteryControlConfig: """Tests for BatteryControlConfig.""" @@ -108,6 +130,19 @@ def test_string_numeric_coercion(self): assert cfg.max_grid_charge_rate == 3000.0 assert cfg.outage_tolerance_minutes == 30.0 + def test_legacy_max_charge_rate_rename(self): + """Test backward compat: max_charge_rate -> max_grid_charge_rate.""" + cfg = InverterConfig(max_charge_rate=4000) + assert cfg.max_grid_charge_rate == 4000.0 + + def test_max_grid_charge_rate_takes_precedence(self): + """Test that max_grid_charge_rate is used when both are present.""" + cfg = InverterConfig( + max_charge_rate=4000, + max_grid_charge_rate=3000, + ) + assert cfg.max_grid_charge_rate == 3000.0 + class TestUtilityConfig: """Tests for UtilityConfig.""" @@ -211,6 +246,32 @@ def test_annual_consumption_coercion(self): cfg = ConsumptionForecastConfig(annual_consumption='4500') assert cfg.annual_consumption == 4500.0 + def test_history_days_semicolon_string(self): + """Test HA addon semicolon-separated string parsing.""" + cfg = ConsumptionForecastConfig(history_days='-7;-14;-21') + assert cfg.history_days == [-7, -14, -21] + assert all(isinstance(x, int) for x in cfg.history_days) + + def test_history_weights_semicolon_string(self): + """Test HA addon semicolon-separated string parsing.""" + cfg = ConsumptionForecastConfig(history_weights='1;1;1') + assert cfg.history_weights == [1, 1, 1] + + def test_history_days_list_passthrough(self): + """Test that regular lists are preserved and items coerced to int.""" + cfg = ConsumptionForecastConfig(history_days=[-7, -14, -21]) + assert cfg.history_days == [-7, -14, -21] + + def test_history_days_string_list_coercion(self): + """Test that list of strings is coerced to list of ints.""" + cfg = ConsumptionForecastConfig(history_days=['-7', '-14', '-21']) + assert cfg.history_days == [-7, -14, -21] + + def test_history_days_none(self): + """Test that None is preserved.""" + cfg = ConsumptionForecastConfig(history_days=None) + assert cfg.history_days is None + class TestValidateConfig: """Tests for the validate_config() function.""" From 387e9483c8b0455f10c7763b2431fd26d26d29c5 Mon Sep 17 00:00:00 2001 From: Claude Date: Tue, 17 Mar 2026 18:39:57 +0000 Subject: [PATCH 03/19] Add missing cache_ttl to InverterConfig, fix mqtt inverter passthrough - Add cache_ttl field to InverterConfig Pydantic model (used by MQTT inverter) - Fix pre-existing bug: cache_ttl was not passed through in inverter factory's MQTT path, always falling back to default 120 https://claude.ai/code/session_015rEfxnRN99qtwpBmEZ3GpP --- src/batcontrol/config_model.py | 1 + src/batcontrol/inverter/inverter.py | 3 ++- tests/batcontrol/test_config_model.py | 6 ++++++ 3 files changed, 9 insertions(+), 1 deletion(-) diff --git a/src/batcontrol/config_model.py b/src/batcontrol/config_model.py index ce2e016f..98653a08 100644 --- a/src/batcontrol/config_model.py +++ b/src/batcontrol/config_model.py @@ -67,6 +67,7 @@ class InverterConfig(BaseModel): min_soc: Optional[int] = None max_soc: Optional[int] = None base_topic: Optional[str] = None + cache_ttl: Optional[int] = None @model_validator(mode='before') @classmethod diff --git a/src/batcontrol/inverter/inverter.py b/src/batcontrol/inverter/inverter.py index acaa0172..cc16c515 100644 --- a/src/batcontrol/inverter/inverter.py +++ b/src/batcontrol/inverter/inverter.py @@ -50,7 +50,8 @@ def create_inverter(config: dict) -> InverterInterface: 'capacity': config['capacity'], 'min_soc': config.get('min_soc', 5), 'max_soc': config.get('max_soc', 100), - 'max_grid_charge_rate': config['max_grid_charge_rate'] + 'max_grid_charge_rate': config['max_grid_charge_rate'], + 'cache_ttl': config.get('cache_ttl', 120) } inverter=MqttInverter(iv_config) else: diff --git a/tests/batcontrol/test_config_model.py b/tests/batcontrol/test_config_model.py index 6385cd5a..8a5bce5a 100644 --- a/tests/batcontrol/test_config_model.py +++ b/tests/batcontrol/test_config_model.py @@ -143,6 +143,12 @@ def test_max_grid_charge_rate_takes_precedence(self): ) assert cfg.max_grid_charge_rate == 3000.0 + def test_cache_ttl_coercion(self): + """Test that cache_ttl string is coerced to int (MQTT inverter).""" + cfg = InverterConfig(cache_ttl='120') + assert cfg.cache_ttl == 120 + assert isinstance(cfg.cache_ttl, int) + class TestUtilityConfig: """Tests for UtilityConfig.""" From bfb4ec176449a0ed9207f8e267991b5a54a01f3b Mon Sep 17 00:00:00 2001 From: Claude Date: Tue, 17 Mar 2026 18:59:36 +0000 Subject: [PATCH 04/19] Add Pydantic config migration docs for AI-assisted development Documents what was done, what remains, config model reference, and HA addon coercion table. https://claude.ai/code/session_015rEfxnRN99qtwpBmEZ3GpP --- docs/pydantic-config-migration.md | 152 ++++++++++++++++++++++++++++++ 1 file changed, 152 insertions(+) create mode 100644 docs/pydantic-config-migration.md diff --git a/docs/pydantic-config-migration.md b/docs/pydantic-config-migration.md new file mode 100644 index 00000000..929019b8 --- /dev/null +++ b/docs/pydantic-config-migration.md @@ -0,0 +1,152 @@ +# Pydantic Config Migration + +## Status: In Progress + +Pydantic validation layer added at config load time. Type coercion and validation +that was scattered across the codebase is now centralized in `config_model.py`. + +## What Was Done + +### New Files +- `src/batcontrol/config_model.py` — Pydantic v2 models for all config sections +- `tests/batcontrol/test_config_model.py` — Unit tests for every model and coercion +- `tests/batcontrol/test_config_load_integration.py` — Integration test with real YAML + +### Modified Files +- `src/batcontrol/setup.py` — Calls `validate_config()` after YAML load +- `src/batcontrol/core.py` — Removed manual `time_resolution_minutes` string-to-int conversion and validation +- `src/batcontrol/dynamictariff/dynamictariff.py` — Removed `float()` casts on `vat`, `markup`, `fees`, `tariff_zone_*` +- `src/batcontrol/forecastconsumption/consumption.py` — Removed manual semicolon-string parsing for `history_days`/`history_weights` +- `src/batcontrol/inverter/inverter.py` — Removed manual `max_charge_rate` rename and `max_pv_charge_rate` default; added `cache_ttl` passthrough +- `pyproject.toml` — Added `pydantic>=2.0,<3.0` dependency + +### Design Decisions +- All models use `extra='allow'` so unknown config keys are preserved (forward compat) +- `validate_config()` returns a plain `dict` (via `model_dump()`) so downstream code needs zero changes +- Validation happens once in `setup.py:load_config()`, not scattered per-module +- Legacy key rename (`max_charge_rate` -> `max_grid_charge_rate`) handled by `model_validator` + +## What Was NOT Done (Remaining Work) + +### TLS Config Bug (Both MQTT and EVCC) +**Files:** `src/batcontrol/mqtt_api.py:92-100`, `src/batcontrol/evcc_api.py:115-123` + +The TLS code checks `config['tls'] is True` (bool) then subscripts it as a dict +(`config['tls']['ca_certs']`). This crashes with `TypeError` when TLS is enabled. +The fix should use sibling keys (`config['cafile']`, etc.) which are already defined +in the Pydantic models `MqttConfig` and `EvccConfig`. + +**Not fixed** because the code is marked "not tested yet" and needs real TLS testing. + +### MQTT API Type Coercions +**File:** `src/batcontrol/mqtt_api.py` + +Still has manual `int()` casts on `port`, `retry_attempts`, `retry_delay`. +These are now redundant since `MqttConfig` handles coercion, but removal was deferred +to minimize diff size. Safe to remove in a follow-up. + +### EVCC API Type Coercions +**File:** `src/batcontrol/evcc_api.py` + +Same situation — manual `int(config['port'])` is redundant. Safe to remove. + +### HA Addon Config Passthrough +**Repo:** `MaStr/batcontrol_ha_addon` + +The HA addon's `run.sh` passes config values from `options.json` into the YAML config. +No changes were made there. The Pydantic models handle string-to-numeric coercion +that the addon introduces, so no addon changes are needed. + +## Config Model Reference + +``` +BatcontrolConfig (top-level) + ├── timezone: str = 'Europe/Berlin' + ├── time_resolution_minutes: int = 60 [validated: 15 or 60] + ├── loglevel: str = 'info' [validated + lowercased] + ├── logfile_enabled: bool = True + ├── log_everything: bool = False + ├── max_logfile_size: int = 200 + ├── logfile_path: str = 'logs/batcontrol.log' + ├── solar_forecast_provider: str = 'fcsolarapi' + │ + ├── battery_control: BatteryControlConfig + │ ├── min_price_difference: float = 0.05 + │ ├── min_price_difference_rel: float = 0.10 + │ ├── always_allow_discharge_limit: float = 0.90 + │ ├── max_charging_from_grid_limit: float = 0.89 + │ └── min_recharge_amount: float = 100.0 + │ + ├── battery_control_expert: BatteryControlExpertConfig (optional) + │ ├── charge_rate_multiplier: float = 1.1 + │ ├── soften_price_difference_on_charging: bool = False + │ ├── soften_price_difference_on_charging_factor: int = 5 + │ ├── round_price_digits: int = 4 + │ └── production_offset_percent: float = 1.0 + │ + ├── inverter: InverterConfig + │ ├── type: str = 'dummy' [fronius_gen24, sungrow, huawei, mqtt, dummy] + │ ├── address, user, password: Optional[str] + │ ├── max_grid_charge_rate: float = 5000 [alias: max_charge_rate] + │ ├── max_pv_charge_rate: float = 0 + │ ├── min_pv_charge_rate: float = 0 + │ ├── enable_resilient_wrapper: bool = False + │ ├── outage_tolerance_minutes: float = 24 + │ ├── retry_backoff_seconds: float = 60 + │ └── capacity, min_soc, max_soc, base_topic, cache_ttl: Optional (MQTT) + │ + ├── utility: UtilityConfig (required) + │ ├── type: str [awattar_at, awattar_de, entsoe, evcc, tariff_zones] + │ ├── vat: float = 0.0 + │ ├── fees: float = 0.0 + │ ├── markup: float = 0.0 + │ └── tariff_zone_1/2/3, zone_1/2/3_hours: Optional + │ + ├── mqtt: MqttConfig (optional) + │ ├── enabled: bool = False + │ ├── broker: str, port: int = 1883 + │ ├── topic: str, username/password: Optional[str] + │ ├── tls: bool = False + │ ├── cafile, certfile, keyfile, tls_version: Optional[str] + │ └── auto_discover_enable: bool, auto_discover_topic: str + │ + ├── evcc: EvccConfig (optional) + │ ├── enabled: bool = False + │ ├── broker: str, port: int = 1883 + │ ├── loadpoint_topic: Union[str, List[str]] + │ ├── block_battery_while_charging: bool = True + │ ├── tls: bool = False + │ └── cafile, certfile, keyfile, tls_version: Optional[str] + │ + ├── pvinstallations: List[PvInstallationConfig] + │ ├── name, type: str + │ ├── lat, lon, declination, azimuth, kWp: Optional[float] + │ └── url, horizon, apikey, algorithm, item, token: Optional[str] + │ + └── consumption_forecast: ConsumptionForecastConfig + ├── type: str = 'csv' + ├── annual_consumption: Optional[float] + ├── history_days: Optional[List[int]] [parses "−7;−14;−21"] + ├── history_weights: Optional[List[int]] [parses "1;1;1"] + └── base_url, apitoken, entity_id, cache_ttl_hours, multiplier: Optional +``` + +## Key HA Addon Coercions Handled + +| Config Path | Raw Type (HA) | Coerced Type | Example | +|---|---|---|---| +| `time_resolution_minutes` | `str` | `int` | `"15"` -> `15` | +| `mqtt.port` | `str` | `int` | `"1883"` -> `1883` | +| `evcc.port` | `str` | `int` | `"1883"` -> `1883` | +| `utility.vat` | `str` | `float` | `"0.19"` -> `0.19` | +| `inverter.max_grid_charge_rate` | `str` | `float` | `"5000"` -> `5000.0` | +| `battery_control.*` | `str` | `float` | `"0.05"` -> `0.05` | +| `consumption_forecast.history_days` | `str` | `List[int]` | `"-7;-14;-21"` -> `[-7,-14,-21]` | + +## How to Run Tests + +```bash +cd /home/user/batcontrol +python -m pytest tests/batcontrol/test_config_model.py -v +python -m pytest tests/batcontrol/test_config_load_integration.py -v +``` From 8bbc57f395218dfbfd6d595ae18363d1af8ef5ff Mon Sep 17 00:00:00 2001 From: Matthias Strubel Date: Tue, 17 Mar 2026 20:13:39 +0100 Subject: [PATCH 05/19] Address PR review: exclude_none, required pvinstallations, cache_ttl default Fixes all 5 review comments from Copilot: 1. Use model_dump(exclude_none=True) so None optional fields don't appear as keys in the output dict. Downstream code checks key presence (e.g. 'csv' in config, required_fields checks) and would break with None values present. 2. Make pvinstallations required (no default) - Pydantic now raises ValidationError if missing, instead of silently defaulting to []. 3. Give cache_ttl a real default (120) instead of Optional[None] to prevent None propagating into TTLCache. 4. Reorder load_config() to run validate_config() before the pvinstallations check, so Pydantic provides clear error messages. 5. tariff_zone_* None issue resolved by exclude_none=True. Co-Authored-By: Claude Opus 4.6 (1M context) --- src/batcontrol/config_model.py | 6 ++-- src/batcontrol/setup.py | 9 +++--- .../test_config_load_integration.py | 11 ++++++++ tests/batcontrol/test_config_model.py | 28 +++++++++++++++++++ 4 files changed, 46 insertions(+), 8 deletions(-) diff --git a/src/batcontrol/config_model.py b/src/batcontrol/config_model.py index 98653a08..136a7e6f 100644 --- a/src/batcontrol/config_model.py +++ b/src/batcontrol/config_model.py @@ -67,7 +67,7 @@ class InverterConfig(BaseModel): min_soc: Optional[int] = None max_soc: Optional[int] = None base_topic: Optional[str] = None - cache_ttl: Optional[int] = None + cache_ttl: int = 120 @model_validator(mode='before') @classmethod @@ -221,7 +221,7 @@ class BatcontrolConfig(BaseModel): utility: UtilityConfig mqtt: Optional[MqttConfig] = None evcc: Optional[EvccConfig] = None - pvinstallations: List[PvInstallationConfig] = [] + pvinstallations: List[PvInstallationConfig] consumption_forecast: ConsumptionForecastConfig = ( ConsumptionForecastConfig() ) @@ -261,4 +261,4 @@ def validate_config(config_dict: dict) -> dict: pydantic.ValidationError: If validation fails. """ validated = BatcontrolConfig(**config_dict) - return validated.model_dump() + return validated.model_dump(exclude_none=True) diff --git a/src/batcontrol/setup.py b/src/batcontrol/setup.py index 2ce5ad21..0339c5ac 100644 --- a/src/batcontrol/setup.py +++ b/src/batcontrol/setup.py @@ -69,11 +69,10 @@ def load_config(configfile:str) -> dict: config = yaml.safe_load(config_str) - if config['pvinstallations']: - pass - else: - raise RuntimeError('No PV Installation found') - + # Validate and coerce types via Pydantic before any other checks config = validate_config(config) + if not config.get('pvinstallations'): + raise RuntimeError('No PV Installation found') + return config diff --git a/tests/batcontrol/test_config_load_integration.py b/tests/batcontrol/test_config_load_integration.py index f6ab245f..7d9e3212 100644 --- a/tests/batcontrol/test_config_load_integration.py +++ b/tests/batcontrol/test_config_load_integration.py @@ -3,6 +3,7 @@ import tempfile import pytest import yaml +from pydantic import ValidationError from batcontrol.setup import load_config @@ -61,6 +62,16 @@ def test_load_config_no_pvinstallations(self, tmp_path): with pytest.raises(RuntimeError, match='No PV Installation'): load_config(path) + def test_load_config_missing_pvinstallations_key(self, tmp_path): + """Test that missing pvinstallations key fails Pydantic validation.""" + config = { + 'timezone': 'Europe/Berlin', + 'utility': {'type': 'awattar_de'}, + } + path = self._write_config(config, str(tmp_path)) + with pytest.raises(ValidationError, match='pvinstallations'): + load_config(path) + def test_load_config_with_dummy_config_file(self): """Test loading the actual dummy config file.""" dummy_path = os.path.join( diff --git a/tests/batcontrol/test_config_model.py b/tests/batcontrol/test_config_model.py index 8a5bce5a..7dcbfac4 100644 --- a/tests/batcontrol/test_config_model.py +++ b/tests/batcontrol/test_config_model.py @@ -93,6 +93,15 @@ def test_loglevel_normalized_to_lowercase(self): cfg = BatcontrolConfig(**data) assert cfg.loglevel == 'debug' + def test_pvinstallations_required(self): + """Test that pvinstallations is required (no default).""" + data = { + 'timezone': 'Europe/Berlin', + 'utility': {'type': 'awattar_de'}, + } + with pytest.raises(ValidationError, match='pvinstallations'): + BatcontrolConfig(**data) + class TestBatteryControlConfig: """Tests for BatteryControlConfig.""" @@ -149,6 +158,11 @@ def test_cache_ttl_coercion(self): assert cfg.cache_ttl == 120 assert isinstance(cfg.cache_ttl, int) + def test_cache_ttl_default(self): + """Test that cache_ttl has a real default, not None.""" + cfg = InverterConfig() + assert cfg.cache_ttl == 120 + class TestUtilityConfig: """Tests for UtilityConfig.""" @@ -383,6 +397,20 @@ def test_validate_invalid_time_resolution(self): with pytest.raises(ValidationError): validate_config(config) + def test_validate_excludes_none_fields(self): + """Test that None optional fields are excluded from output dict. + + Downstream code checks key presence (e.g. 'csv' in config), + so None values must not appear as keys. + """ + result = validate_config(self._full_config()) + # These optional fields were not set, so must be absent + assert 'apikey' not in result['utility'] + assert 'url' not in result['utility'] + # Inverter optional fields should be absent + assert 'address' not in result['inverter'] + assert 'user' not in result['inverter'] + def test_ha_addon_string_config(self): """Simulate HA addon options.json where many values are strings.""" config = self._full_config() From 5115e03d0aaa60e00cc82cfb8aea5a4a515ab78a Mon Sep 17 00:00:00 2001 From: Matthias Strubel Date: Tue, 17 Mar 2026 20:23:23 +0100 Subject: [PATCH 06/19] Address PR review round 2: nested HA config parsing, validator mutation Fixes 2 valid review comments (3 others were already fixed): 1. Restore history_days/history_weights parsing in consumption factory for the nested homeassistant_api dict case. Pydantic only validates top-level ConsumptionForecastConfig fields; when values come from the nested homeassistant_api dict they bypass Pydantic validation and may still be semicolon-separated strings. 2. Copy input dict in InverterConfig model_validator before mutating via pop(). Prevents surprising side effects on callers who reuse the same dict. Co-Authored-By: Claude Opus 4.6 (1M context) --- src/batcontrol/config_model.py | 1 + src/batcontrol/forecastconsumption/consumption.py | 15 +++++++++++++-- tests/batcontrol/test_config_model.py | 6 ++++++ 3 files changed, 20 insertions(+), 2 deletions(-) diff --git a/src/batcontrol/config_model.py b/src/batcontrol/config_model.py index 136a7e6f..9d724d25 100644 --- a/src/batcontrol/config_model.py +++ b/src/batcontrol/config_model.py @@ -75,6 +75,7 @@ def handle_max_charge_rate_rename(cls, data): """Support legacy config key max_charge_rate -> max_grid_charge_rate.""" if isinstance(data, dict): if 'max_charge_rate' in data and 'max_grid_charge_rate' not in data: + data = dict(data) data['max_grid_charge_rate'] = data.pop('max_charge_rate') return data diff --git a/src/batcontrol/forecastconsumption/consumption.py b/src/batcontrol/forecastconsumption/consumption.py index 0661d66e..1ad63462 100644 --- a/src/batcontrol/forecastconsumption/consumption.py +++ b/src/batcontrol/forecastconsumption/consumption.py @@ -72,11 +72,22 @@ def _create_homeassistant_forecast( base_url = ha_config['base_url'] api_token = ha_config['apitoken'] entity_id = ha_config['entity_id'] - # history_days/history_weights are already parsed by Pydantic validation: - # semicolon-separated strings are converted to int lists at config load time history_days = ha_config.get('history_days', [-7, -14, -21]) history_weights = ha_config.get('history_weights', [1, 1, 1]) + # Pydantic validates top-level consumption_forecast.history_days (flat HA + # addon config), but when nested under homeassistant_api dict the values + # may still be semicolon-separated strings. Parse here for that case. + if isinstance(history_days, str): + history_days = [int(x.strip()) for x in history_days.split(';')] + if isinstance(history_weights, str): + history_weights = [int(x.strip()) for x in history_weights.split(';')] + # Ensure list items are ints (handles list-of-strings edge case) + if isinstance(history_days, list): + history_days = [int(x) for x in history_days] + if isinstance(history_weights, list): + history_weights = [int(x) for x in history_weights] + cache_ttl_hours = ha_config.get('cache_ttl_hours', 48.0) multiplier = ha_config.get('multiplier', 1.0) sensor_unit = ha_config.get('sensor_unit', "auto") diff --git a/tests/batcontrol/test_config_model.py b/tests/batcontrol/test_config_model.py index 7dcbfac4..5ca768cb 100644 --- a/tests/batcontrol/test_config_model.py +++ b/tests/batcontrol/test_config_model.py @@ -163,6 +163,12 @@ def test_cache_ttl_default(self): cfg = InverterConfig() assert cfg.cache_ttl == 120 + def test_legacy_rename_does_not_mutate_input(self): + """Test that model_validator doesn't mutate the caller's dict.""" + data = {'max_charge_rate': 4000} + InverterConfig(**data) + assert 'max_charge_rate' in data + class TestUtilityConfig: """Tests for UtilityConfig.""" From d61e9927ba6aa110f08b7ae745640e232893e927 Mon Sep 17 00:00:00 2001 From: Matthias Strubel Date: Tue, 17 Mar 2026 20:35:03 +0100 Subject: [PATCH 07/19] Address PR review round 3: Optional vat/fees/markup, nested dict coercion, lint Fixes 4 new review comments: 1. Make vat/fees/markup Optional[float]=None in UtilityConfig so downstream required_fields checks in dynamictariff.py can detect missing config (previously defaults of 0.0 masked misconfigurations). 2. Add float() coercion for cache_ttl_hours, multiplier, and annual_consumption in consumption factory for the nested homeassistant_api/csv dict paths that bypass Pydantic validation. 3. Remove unused import tempfile from test_config_load_integration.py. 4. Remove unused import BatteryControlExpertConfig from test_config_model.py. Co-Authored-By: Claude Opus 4.6 (1M context) --- src/batcontrol/config_model.py | 9 ++++--- .../forecastconsumption/consumption.py | 7 +++--- .../test_config_load_integration.py | 1 - tests/batcontrol/test_config_model.py | 25 ++++++++++++++++--- 4 files changed, 31 insertions(+), 11 deletions(-) diff --git a/src/batcontrol/config_model.py b/src/batcontrol/config_model.py index 9d724d25..39741567 100644 --- a/src/batcontrol/config_model.py +++ b/src/batcontrol/config_model.py @@ -87,9 +87,12 @@ class UtilityConfig(BaseModel): type: str apikey: Optional[str] = None url: Optional[str] = None - vat: float = 0.0 - fees: float = 0.0 - markup: float = 0.0 + # vat/fees/markup are Optional so that downstream required_fields checks + # (in dynamictariff.py) can detect missing config for providers that need them. + # With exclude_none=True, absent keys won't appear in the output dict. + vat: Optional[float] = None + fees: Optional[float] = None + markup: Optional[float] = None # Tariff zone fields tariff_zone_1: Optional[float] = None zone_1_hours: Optional[str] = None diff --git a/src/batcontrol/forecastconsumption/consumption.py b/src/batcontrol/forecastconsumption/consumption.py index 1ad63462..0b9b3369 100644 --- a/src/batcontrol/forecastconsumption/consumption.py +++ b/src/batcontrol/forecastconsumption/consumption.py @@ -88,8 +88,9 @@ def _create_homeassistant_forecast( if isinstance(history_weights, list): history_weights = [int(x) for x in history_weights] - cache_ttl_hours = ha_config.get('cache_ttl_hours', 48.0) - multiplier = ha_config.get('multiplier', 1.0) + # Coerce numeric fields that may arrive as strings from nested HA config + cache_ttl_hours = float(ha_config.get('cache_ttl_hours', 48.0)) + multiplier = float(ha_config.get('multiplier', 1.0)) sensor_unit = ha_config.get('sensor_unit', "auto") logger.info( @@ -140,7 +141,7 @@ def _create_csv_forecast( consumption = ForecastConsumptionCsv( 'config/' + csv_config['load_profile'], tz, - csv_config.get('annual_consumption', 0), + float(csv_config.get('annual_consumption', 0)), target_resolution=target_resolution ) return consumption diff --git a/tests/batcontrol/test_config_load_integration.py b/tests/batcontrol/test_config_load_integration.py index 7d9e3212..8f8f80c5 100644 --- a/tests/batcontrol/test_config_load_integration.py +++ b/tests/batcontrol/test_config_load_integration.py @@ -1,6 +1,5 @@ """Integration tests for load_config with Pydantic validation.""" import os -import tempfile import pytest import yaml from pydantic import ValidationError diff --git a/tests/batcontrol/test_config_model.py b/tests/batcontrol/test_config_model.py index 5ca768cb..6a041517 100644 --- a/tests/batcontrol/test_config_model.py +++ b/tests/batcontrol/test_config_model.py @@ -4,7 +4,6 @@ from batcontrol.config_model import ( BatcontrolConfig, BatteryControlConfig, - BatteryControlExpertConfig, InverterConfig, UtilityConfig, MqttConfig, @@ -187,6 +186,14 @@ def test_string_float_coercion(self): assert cfg.vat == 0.19 assert isinstance(cfg.vat, float) + def test_vat_fees_markup_absent_when_not_set(self): + """Test that vat/fees/markup are None when not set, + so downstream required_fields checks detect missing config.""" + cfg = UtilityConfig(type='tibber') + assert cfg.vat is None + assert cfg.fees is None + assert cfg.markup is None + def test_tariff_zone_coercion(self): cfg = UtilityConfig( type='tariff_zones', @@ -410,13 +417,23 @@ def test_validate_excludes_none_fields(self): so None values must not appear as keys. """ result = validate_config(self._full_config()) - # These optional fields were not set, so must be absent - assert 'apikey' not in result['utility'] - assert 'url' not in result['utility'] # Inverter optional fields should be absent assert 'address' not in result['inverter'] assert 'user' not in result['inverter'] + def test_validate_utility_vat_excluded_when_not_set(self): + """Test that vat/fees/markup are absent when not configured, + so dynamictariff required_fields checks still catch misconfig.""" + config = self._full_config() + # Remove vat/fees/markup from utility (simulating tibber config) + del config['utility']['vat'] + del config['utility']['fees'] + del config['utility']['markup'] + result = validate_config(config) + assert 'vat' not in result['utility'] + assert 'fees' not in result['utility'] + assert 'markup' not in result['utility'] + def test_ha_addon_string_config(self): """Simulate HA addon options.json where many values are strings.""" config = self._full_config() From 96dff23295b3ac8e990d2c9e77df7f3fc06924a8 Mon Sep 17 00:00:00 2001 From: Matthias Strubel Date: Tue, 17 Mar 2026 21:07:45 +0100 Subject: [PATCH 08/19] Fix docs: correct UtilityConfig types, provider list, and parsing description - vat/fees/markup are Optional[float]=None, not float=0.0 - Remove nonexistent 'entsoe' provider, list actual providers - Clarify that semicolon parsing was kept for nested HA config case - Note exclude_none=True in design decisions - Remove nonexistent sungrow/huawei inverter types Co-Authored-By: Claude Opus 4.6 (1M context) --- docs/pydantic-config-migration.md | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) diff --git a/docs/pydantic-config-migration.md b/docs/pydantic-config-migration.md index 929019b8..86214dc5 100644 --- a/docs/pydantic-config-migration.md +++ b/docs/pydantic-config-migration.md @@ -16,13 +16,13 @@ that was scattered across the codebase is now centralized in `config_model.py`. - `src/batcontrol/setup.py` — Calls `validate_config()` after YAML load - `src/batcontrol/core.py` — Removed manual `time_resolution_minutes` string-to-int conversion and validation - `src/batcontrol/dynamictariff/dynamictariff.py` — Removed `float()` casts on `vat`, `markup`, `fees`, `tariff_zone_*` -- `src/batcontrol/forecastconsumption/consumption.py` — Removed manual semicolon-string parsing for `history_days`/`history_weights` +- `src/batcontrol/forecastconsumption/consumption.py` — Pydantic handles semicolon-string parsing for flat HA addon config; factory retains parsing for nested `homeassistant_api` dict case - `src/batcontrol/inverter/inverter.py` — Removed manual `max_charge_rate` rename and `max_pv_charge_rate` default; added `cache_ttl` passthrough - `pyproject.toml` — Added `pydantic>=2.0,<3.0` dependency ### Design Decisions - All models use `extra='allow'` so unknown config keys are preserved (forward compat) -- `validate_config()` returns a plain `dict` (via `model_dump()`) so downstream code needs zero changes +- `validate_config()` returns a plain `dict` (via `model_dump(exclude_none=True)`) so downstream key-presence checks work unchanged - Validation happens once in `setup.py:load_config()`, not scattered per-module - Legacy key rename (`max_charge_rate` -> `max_grid_charge_rate`) handled by `model_validator` @@ -85,7 +85,7 @@ BatcontrolConfig (top-level) │ └── production_offset_percent: float = 1.0 │ ├── inverter: InverterConfig - │ ├── type: str = 'dummy' [fronius_gen24, sungrow, huawei, mqtt, dummy] + │ ├── type: str = 'dummy' [fronius_gen24, mqtt, dummy] │ ├── address, user, password: Optional[str] │ ├── max_grid_charge_rate: float = 5000 [alias: max_charge_rate] │ ├── max_pv_charge_rate: float = 0 @@ -96,10 +96,10 @@ BatcontrolConfig (top-level) │ └── capacity, min_soc, max_soc, base_topic, cache_ttl: Optional (MQTT) │ ├── utility: UtilityConfig (required) - │ ├── type: str [awattar_at, awattar_de, entsoe, evcc, tariff_zones] - │ ├── vat: float = 0.0 - │ ├── fees: float = 0.0 - │ ├── markup: float = 0.0 + │ ├── type: str [tibber, awattar_at, awattar_de, evcc, energyforecast, tariff_zones] + │ ├── vat: Optional[float] = None [excluded from output when not set] + │ ├── fees: Optional[float] = None [excluded from output when not set] + │ ├── markup: Optional[float] = None [excluded from output when not set] │ └── tariff_zone_1/2/3, zone_1/2/3_hours: Optional │ ├── mqtt: MqttConfig (optional) From 750a7eb75c1d21b9e49f8135589efd5d5c48404e Mon Sep 17 00:00:00 2001 From: Matthias Strubel Date: Tue, 17 Mar 2026 21:28:07 +0100 Subject: [PATCH 09/19] Address PR review round 4: empty YAML guard, rename misleading test class - setup.py: raise RuntimeError when yaml.safe_load() returns None (empty or non-mapping YAML) before calling validate_config(), giving a clear error instead of a TypeError from **None unpacking - test_core.py: rename TestTimeResolutionString -> TestTimeResolutionValidation and update docstring to reflect tests use int values (string coercion is covered in test_config_model.py) Co-Authored-By: Claude Sonnet 4.6 --- src/batcontrol/setup.py | 3 +++ tests/batcontrol/test_core.py | 6 +++--- 2 files changed, 6 insertions(+), 3 deletions(-) diff --git a/src/batcontrol/setup.py b/src/batcontrol/setup.py index 0339c5ac..cb848b61 100644 --- a/src/batcontrol/setup.py +++ b/src/batcontrol/setup.py @@ -69,6 +69,9 @@ def load_config(configfile:str) -> dict: config = yaml.safe_load(config_str) + if not isinstance(config, dict): + raise RuntimeError(f'Configfile {configfile} is empty or not a valid YAML mapping') + # Validate and coerce types via Pydantic before any other checks config = validate_config(config) diff --git a/tests/batcontrol/test_core.py b/tests/batcontrol/test_core.py index 3a77646e..30fc406c 100644 --- a/tests/batcontrol/test_core.py +++ b/tests/batcontrol/test_core.py @@ -289,11 +289,11 @@ def test_api_set_limit_applies_immediately_in_mode_8( mock_inverter.set_mode_limit_battery_charge.assert_called_once_with(2000) -class TestTimeResolutionString: - """Test that time_resolution_minutes coercion is handled by Pydantic config validation. +class TestTimeResolutionValidation: + """Test that time_resolution_minutes values are accepted by Batcontrol. Since Pydantic validates and coerces types in load_config() before Batcontrol - receives the config dict, Batcontrol.__init__() expects properly typed values. + receives the config dict, Batcontrol.__init__() expects properly typed int values. String-to-int coercion is tested in test_config_model.py. """ From 3a5a379decbb6ceec77b9e4b0fc0efe2a3d1f92f Mon Sep 17 00:00:00 2001 From: Matthias Strubel Date: Thu, 2 Apr 2026 16:06:53 +0200 Subject: [PATCH 10/19] Address PR review round 5: mutable defaults, float(None) guard, defensive validation - Use Field(default_factory=...) for nested model defaults in BatcontrolConfig to follow Pydantic best practices (even though v2 deep-copies, this is cleaner) - Guard against None values before float() cast in consumption.py HA config (YAML null values would previously raise TypeError) - Add defensive time_resolution_minutes validation in Batcontrol.__init__ so direct instantiation without load_config() gives a clear error Co-Authored-By: Claude Sonnet 4.6 --- src/batcontrol/config_model.py | 10 +++++----- src/batcontrol/core.py | 4 ++++ src/batcontrol/forecastconsumption/consumption.py | 9 ++++++--- 3 files changed, 15 insertions(+), 8 deletions(-) diff --git a/src/batcontrol/config_model.py b/src/batcontrol/config_model.py index 39741567..e8365195 100644 --- a/src/batcontrol/config_model.py +++ b/src/batcontrol/config_model.py @@ -5,7 +5,7 @@ They also fix HA addon issues where numeric values arrive as strings. """ from typing import List, Optional, Union -from pydantic import BaseModel, ConfigDict, field_validator, model_validator +from pydantic import BaseModel, ConfigDict, Field, field_validator, model_validator def _parse_semicolon_int_list(v): @@ -219,15 +219,15 @@ class BatcontrolConfig(BaseModel): logfile_path: str = 'logs/batcontrol.log' solar_forecast_provider: str = 'fcsolarapi' - battery_control: BatteryControlConfig = BatteryControlConfig() + battery_control: BatteryControlConfig = Field(default_factory=BatteryControlConfig) battery_control_expert: Optional[BatteryControlExpertConfig] = None - inverter: InverterConfig = InverterConfig() + inverter: InverterConfig = Field(default_factory=InverterConfig) utility: UtilityConfig mqtt: Optional[MqttConfig] = None evcc: Optional[EvccConfig] = None pvinstallations: List[PvInstallationConfig] - consumption_forecast: ConsumptionForecastConfig = ( - ConsumptionForecastConfig() + consumption_forecast: ConsumptionForecastConfig = Field( + default_factory=ConsumptionForecastConfig ) @field_validator('time_resolution_minutes') diff --git a/src/batcontrol/core.py b/src/batcontrol/core.py index 86b7d706..e9f0eb9d 100644 --- a/src/batcontrol/core.py +++ b/src/batcontrol/core.py @@ -92,6 +92,10 @@ def __init__(self, configdict: dict): # time_resolution_minutes is validated and coerced by Pydantic # in config_model.py (must be int, must be 15 or 60) self.time_resolution = config.get('time_resolution_minutes', 60) + if self.time_resolution not in (15, 60): + raise ValueError( + f"time_resolution_minutes must be 15 or 60, got {self.time_resolution!r}" + ) self.intervals_per_hour = 60 // self.time_resolution logger.info( diff --git a/src/batcontrol/forecastconsumption/consumption.py b/src/batcontrol/forecastconsumption/consumption.py index 0b9b3369..2bccff6c 100644 --- a/src/batcontrol/forecastconsumption/consumption.py +++ b/src/batcontrol/forecastconsumption/consumption.py @@ -88,9 +88,12 @@ def _create_homeassistant_forecast( if isinstance(history_weights, list): history_weights = [int(x) for x in history_weights] - # Coerce numeric fields that may arrive as strings from nested HA config - cache_ttl_hours = float(ha_config.get('cache_ttl_hours', 48.0)) - multiplier = float(ha_config.get('multiplier', 1.0)) + # Coerce numeric fields that may arrive as strings from nested HA config. + # Use 'or default' to guard against null/None values in YAML config. + _cache_ttl_hours = ha_config.get('cache_ttl_hours') + cache_ttl_hours = float(_cache_ttl_hours if _cache_ttl_hours is not None else 48.0) + _multiplier = ha_config.get('multiplier') + multiplier = float(_multiplier if _multiplier is not None else 1.0) sensor_unit = ha_config.get('sensor_unit', "auto") logger.info( From 954f0a6369a3c1aa9fa85320cd4df0202dd9d582 Mon Sep 17 00:00:00 2001 From: Matthias Strubel Date: Thu, 2 Apr 2026 16:17:24 +0200 Subject: [PATCH 11/19] Address PR review round 6: ValidationError wrapping, yaml.safe_dump in tests - Catch pydantic.ValidationError in load_config() and re-raise as RuntimeError so callers (e.g. __main__.py) continue to see only RuntimeError as documented - Update docstring to reflect that malformed config raises RuntimeError - Replace yaml.dump() with yaml.safe_dump() in integration test helper to match load_config()'s use of yaml.safe_load() and avoid Python-specific YAML tags - Update test expecting ValidationError to expect RuntimeError (new behavior) - Remove now-unused ValidationError import from integration test file Co-Authored-By: Claude Sonnet 4.6 --- src/batcontrol/setup.py | 12 +++++++++--- tests/batcontrol/test_config_load_integration.py | 5 ++--- 2 files changed, 11 insertions(+), 6 deletions(-) diff --git a/src/batcontrol/setup.py b/src/batcontrol/setup.py index cb848b61..349dcac4 100644 --- a/src/batcontrol/setup.py +++ b/src/batcontrol/setup.py @@ -4,6 +4,8 @@ import yaml from logging.handlers import RotatingFileHandler +from pydantic import ValidationError + from .config_model import validate_config @@ -58,7 +60,7 @@ def load_config(configfile:str) -> dict: dict: The loaded configuration Raises: - RuntimeError: If the config file is not found or no PV installations are found + RuntimeError: If the config file is not found, config is invalid/malformed, or no PV installations are found """ if not os.path.isfile(configfile): @@ -72,8 +74,12 @@ def load_config(configfile:str) -> dict: if not isinstance(config, dict): raise RuntimeError(f'Configfile {configfile} is empty or not a valid YAML mapping') - # Validate and coerce types via Pydantic before any other checks - config = validate_config(config) + # Validate and coerce types via Pydantic before any other checks. + # Re-raise ValidationError as RuntimeError to keep callers' expected error type. + try: + config = validate_config(config) + except ValidationError as exc: + raise RuntimeError(f'Config validation failed: {exc}') from exc if not config.get('pvinstallations'): raise RuntimeError('No PV Installation found') diff --git a/tests/batcontrol/test_config_load_integration.py b/tests/batcontrol/test_config_load_integration.py index 8f8f80c5..47a06c1c 100644 --- a/tests/batcontrol/test_config_load_integration.py +++ b/tests/batcontrol/test_config_load_integration.py @@ -2,7 +2,6 @@ import os import pytest import yaml -from pydantic import ValidationError from batcontrol.setup import load_config @@ -13,7 +12,7 @@ def _write_config(self, config_dict, tmpdir): """Write config dict to a YAML file and return the path.""" path = os.path.join(tmpdir, 'test_config.yaml') with open(path, 'w', encoding='UTF-8') as f: - yaml.dump(config_dict, f) + yaml.safe_dump(config_dict, f) return path def _minimal_config(self): @@ -68,7 +67,7 @@ def test_load_config_missing_pvinstallations_key(self, tmp_path): 'utility': {'type': 'awattar_de'}, } path = self._write_config(config, str(tmp_path)) - with pytest.raises(ValidationError, match='pvinstallations'): + with pytest.raises(RuntimeError, match='pvinstallations'): load_config(path) def test_load_config_with_dummy_config_file(self): From 2f73082f375405b4ddedba2d85e659a86b140b15 Mon Sep 17 00:00:00 2001 From: Matthias Strubel Date: Thu, 2 Apr 2026 16:26:08 +0200 Subject: [PATCH 12/19] Address PR review round 7: YAML error handling, inverter type validation, name required MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - Wrap yaml.safe_load() in try/except yaml.YAMLError → RuntimeError so malformed YAML gives a clean error consistent with the rest of load_config() - Add type-dependent model_validator to InverterConfig: fronius_gen24 requires address/user/password; mqtt requires capacity — prevents KeyError at runtime - Make PvInstallationConfig.name required (no default) to prevent empty-string cache key collisions across PV installations - Add 5 new tests covering all new validation paths Co-Authored-By: Claude Sonnet 4.6 --- src/batcontrol/config_model.py | 17 ++++++++++++++++- src/batcontrol/setup.py | 5 ++++- tests/batcontrol/test_config_model.py | 25 +++++++++++++++++++++++++ 3 files changed, 45 insertions(+), 2 deletions(-) diff --git a/src/batcontrol/config_model.py b/src/batcontrol/config_model.py index e8365195..3868df39 100644 --- a/src/batcontrol/config_model.py +++ b/src/batcontrol/config_model.py @@ -79,6 +79,21 @@ def handle_max_charge_rate_rename(cls, data): data['max_grid_charge_rate'] = data.pop('max_charge_rate') return data + @model_validator(mode='after') + def validate_type_required_fields(self): + """Enforce type-dependent required fields so KeyErrors can't occur at runtime.""" + inverter_type = (self.type or '').lower() + if inverter_type == 'fronius_gen24': + missing = [f for f in ('address', 'user', 'password') if getattr(self, f) is None] + if missing: + raise ValueError( + f"fronius_gen24 inverter requires: {', '.join(missing)}" + ) + elif inverter_type == 'mqtt': + if self.capacity is None: + raise ValueError("mqtt inverter requires: capacity") + return self + class UtilityConfig(BaseModel): """Dynamic tariff provider configuration.""" @@ -148,7 +163,7 @@ class PvInstallationConfig(BaseModel): """Single PV installation configuration.""" model_config = ConfigDict(extra='allow') - name: str = '' + name: str type: Optional[str] = None lat: Optional[float] = None lon: Optional[float] = None diff --git a/src/batcontrol/setup.py b/src/batcontrol/setup.py index 349dcac4..17db7364 100644 --- a/src/batcontrol/setup.py +++ b/src/batcontrol/setup.py @@ -69,7 +69,10 @@ def load_config(configfile:str) -> dict: with open(configfile, 'r', encoding='UTF-8') as f: config_str = f.read() - config = yaml.safe_load(config_str) + try: + config = yaml.safe_load(config_str) + except yaml.YAMLError as exc: + raise RuntimeError(f'Configfile {configfile} is not valid YAML: {exc}') from exc if not isinstance(config, dict): raise RuntimeError(f'Configfile {configfile} is empty or not a valid YAML mapping') diff --git a/tests/batcontrol/test_config_model.py b/tests/batcontrol/test_config_model.py index 6a041517..5e7bea67 100644 --- a/tests/batcontrol/test_config_model.py +++ b/tests/batcontrol/test_config_model.py @@ -168,6 +168,26 @@ def test_legacy_rename_does_not_mutate_input(self): InverterConfig(**data) assert 'max_charge_rate' in data + def test_fronius_gen24_requires_address_user_password(self): + """Test that fronius_gen24 type requires address, user, password.""" + with pytest.raises(ValidationError, match='fronius_gen24'): + InverterConfig(type='fronius_gen24') + + def test_fronius_gen24_valid_with_required_fields(self): + """Test that fronius_gen24 validates OK when required fields are present.""" + cfg = InverterConfig(type='fronius_gen24', address='192.168.1.1', user='admin', password='secret') + assert cfg.address == '192.168.1.1' + + def test_mqtt_requires_capacity(self): + """Test that mqtt type requires capacity.""" + with pytest.raises(ValidationError, match='mqtt inverter requires: capacity'): + InverterConfig(type='mqtt') + + def test_mqtt_valid_with_capacity(self): + """Test that mqtt validates OK when capacity is present.""" + cfg = InverterConfig(type='mqtt', capacity=10000) + assert cfg.capacity == 10000 + class TestUtilityConfig: """Tests for UtilityConfig.""" @@ -252,6 +272,11 @@ def test_loadpoint_topic_list(self): class TestPvInstallationConfig: """Tests for PvInstallationConfig.""" + def test_name_required(self): + """Test that name is required (no default) to avoid empty-string cache key collisions.""" + with pytest.raises(ValidationError, match='name'): + PvInstallationConfig(kWp=10.0) + def test_float_coercion(self): """Test that numeric PV fields are coerced from strings.""" cfg = PvInstallationConfig( From 0c4f049773c4f07987d3cfbd47eb9664718a7657 Mon Sep 17 00:00:00 2001 From: Matthias Strubel Date: Thu, 2 Apr 2026 16:35:04 +0200 Subject: [PATCH 13/19] Address PR review round 8: int power rates, sanitize error messages, fix comment MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - Change max_grid_charge_rate/max_pv_charge_rate/min_pv_charge_rate to int in InverterConfig — they represent watts and are used with %d formatting in logs - Sanitize ValidationError in load_config(): use exc.errors() with only field path and message (no input_value) to avoid leaking secrets like inverter passwords - Fix misleading comment in consumption.py: clarify it's an explicit None check (not 'or default') so it's clear why 0 is handled correctly as a valid value - Update test asserting max_grid_charge_rate is float → int Co-Authored-By: Claude Sonnet 4.6 --- src/batcontrol/config_model.py | 6 +++--- src/batcontrol/forecastconsumption/consumption.py | 3 ++- src/batcontrol/setup.py | 8 +++++++- tests/batcontrol/test_config_model.py | 2 +- 4 files changed, 13 insertions(+), 6 deletions(-) diff --git a/src/batcontrol/config_model.py b/src/batcontrol/config_model.py index 3868df39..96da92ac 100644 --- a/src/batcontrol/config_model.py +++ b/src/batcontrol/config_model.py @@ -54,9 +54,9 @@ class InverterConfig(BaseModel): address: Optional[str] = None user: Optional[str] = None password: Optional[str] = None - max_grid_charge_rate: float = 5000 - max_pv_charge_rate: float = 0 - min_pv_charge_rate: float = 0 + max_grid_charge_rate: int = 5000 + max_pv_charge_rate: int = 0 + min_pv_charge_rate: int = 0 fronius_inverter_id: Optional[str] = None fronius_controller_id: Optional[str] = None enable_resilient_wrapper: bool = False diff --git a/src/batcontrol/forecastconsumption/consumption.py b/src/batcontrol/forecastconsumption/consumption.py index 2bccff6c..dee4df4f 100644 --- a/src/batcontrol/forecastconsumption/consumption.py +++ b/src/batcontrol/forecastconsumption/consumption.py @@ -89,7 +89,8 @@ def _create_homeassistant_forecast( history_weights = [int(x) for x in history_weights] # Coerce numeric fields that may arrive as strings from nested HA config. - # Use 'or default' to guard against null/None values in YAML config. + # Explicit is-None check guards against null/None YAML values (key present, value None) + # without treating 0 as falsy. _cache_ttl_hours = ha_config.get('cache_ttl_hours') cache_ttl_hours = float(_cache_ttl_hours if _cache_ttl_hours is not None else 48.0) _multiplier = ha_config.get('multiplier') diff --git a/src/batcontrol/setup.py b/src/batcontrol/setup.py index 17db7364..719d55e4 100644 --- a/src/batcontrol/setup.py +++ b/src/batcontrol/setup.py @@ -82,7 +82,13 @@ def load_config(configfile:str) -> dict: try: config = validate_config(config) except ValidationError as exc: - raise RuntimeError(f'Config validation failed: {exc}') from exc + # Build a sanitized error: include field path and error type but NOT + # the raw input values (which can contain secrets like passwords). + details = '; '.join( + f"{'.'.join(str(loc) for loc in e['loc'])}: {e['msg']}" + for e in exc.errors() + ) + raise RuntimeError(f'Config validation failed: {details}') from exc if not config.get('pvinstallations'): raise RuntimeError('No PV Installation found') diff --git a/tests/batcontrol/test_config_model.py b/tests/batcontrol/test_config_model.py index 5e7bea67..3aaf8a88 100644 --- a/tests/batcontrol/test_config_model.py +++ b/tests/batcontrol/test_config_model.py @@ -479,7 +479,7 @@ def test_ha_addon_string_config(self): assert isinstance(result['max_logfile_size'], int) assert isinstance( result['battery_control']['min_price_difference'], float) - assert isinstance(result['inverter']['max_grid_charge_rate'], float) + assert isinstance(result['inverter']['max_grid_charge_rate'], int) assert isinstance(result['mqtt']['port'], int) assert isinstance(result['mqtt']['retry_attempts'], int) assert isinstance(result['evcc']['port'], int) From b584ba08a681cec34f2af8d83f56e9b6e8289ea9 Mon Sep 17 00:00:00 2001 From: Matthias Strubel Date: Thu, 2 Apr 2026 16:42:46 +0200 Subject: [PATCH 14/19] =?UTF-8?q?Fix=20docs:=20align=20power=20rate=20type?= =?UTF-8?q?s=20with=20InverterConfig=20(float=20=E2=86=92=20int)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit max_grid_charge_rate/max_pv_charge_rate/min_pv_charge_rate are int in the model (watts); update the field reference tree and coercion table accordingly. Co-Authored-By: Claude Sonnet 4.6 --- docs/pydantic-config-migration.md | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/docs/pydantic-config-migration.md b/docs/pydantic-config-migration.md index 86214dc5..6e8df83e 100644 --- a/docs/pydantic-config-migration.md +++ b/docs/pydantic-config-migration.md @@ -87,9 +87,9 @@ BatcontrolConfig (top-level) ├── inverter: InverterConfig │ ├── type: str = 'dummy' [fronius_gen24, mqtt, dummy] │ ├── address, user, password: Optional[str] - │ ├── max_grid_charge_rate: float = 5000 [alias: max_charge_rate] - │ ├── max_pv_charge_rate: float = 0 - │ ├── min_pv_charge_rate: float = 0 + │ ├── max_grid_charge_rate: int = 5000 [alias: max_charge_rate] + │ ├── max_pv_charge_rate: int = 0 + │ ├── min_pv_charge_rate: int = 0 │ ├── enable_resilient_wrapper: bool = False │ ├── outage_tolerance_minutes: float = 24 │ ├── retry_backoff_seconds: float = 60 @@ -139,7 +139,7 @@ BatcontrolConfig (top-level) | `mqtt.port` | `str` | `int` | `"1883"` -> `1883` | | `evcc.port` | `str` | `int` | `"1883"` -> `1883` | | `utility.vat` | `str` | `float` | `"0.19"` -> `0.19` | -| `inverter.max_grid_charge_rate` | `str` | `float` | `"5000"` -> `5000.0` | +| `inverter.max_grid_charge_rate` | `str` | `int` | `"5000"` -> `5000` | | `battery_control.*` | `str` | `float` | `"0.05"` -> `0.05` | | `consumption_forecast.history_days` | `str` | `List[int]` | `"-7;-14;-21"` -> `[-7,-14,-21]` | From cb1ac27d33e6d4d1c30ec02764c197d3f7c748b8 Mon Sep 17 00:00:00 2001 From: Matthias Strubel Date: Thu, 2 Apr 2026 16:55:20 +0200 Subject: [PATCH 15/19] Fix test assertions: max_grid_charge_rate is int, not float After changing InverterConfig power rate fields to int in round 8, three test assertions still compared against float literals (3000.0, 4000.0). Update them to int and add isinstance(_, int) checks to catch regressions. Co-Authored-By: Claude Sonnet 4.6 --- tests/batcontrol/test_config_model.py | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-) diff --git a/tests/batcontrol/test_config_model.py b/tests/batcontrol/test_config_model.py index 3aaf8a88..f12204c6 100644 --- a/tests/batcontrol/test_config_model.py +++ b/tests/batcontrol/test_config_model.py @@ -135,13 +135,15 @@ def test_string_numeric_coercion(self): max_grid_charge_rate='3000', outage_tolerance_minutes='30', ) - assert cfg.max_grid_charge_rate == 3000.0 + assert cfg.max_grid_charge_rate == 3000 + assert isinstance(cfg.max_grid_charge_rate, int) assert cfg.outage_tolerance_minutes == 30.0 def test_legacy_max_charge_rate_rename(self): """Test backward compat: max_charge_rate -> max_grid_charge_rate.""" cfg = InverterConfig(max_charge_rate=4000) - assert cfg.max_grid_charge_rate == 4000.0 + assert cfg.max_grid_charge_rate == 4000 + assert isinstance(cfg.max_grid_charge_rate, int) def test_max_grid_charge_rate_takes_precedence(self): """Test that max_grid_charge_rate is used when both are present.""" @@ -149,7 +151,8 @@ def test_max_grid_charge_rate_takes_precedence(self): max_charge_rate=4000, max_grid_charge_rate=3000, ) - assert cfg.max_grid_charge_rate == 3000.0 + assert cfg.max_grid_charge_rate == 3000 + assert isinstance(cfg.max_grid_charge_rate, int) def test_cache_ttl_coercion(self): """Test that cache_ttl string is coerced to int (MQTT inverter).""" From 855f89b9476257e3eb42e1352215ca57da1c432c Mon Sep 17 00:00:00 2001 From: Matthias Strubel Date: Thu, 2 Apr 2026 17:03:17 +0200 Subject: [PATCH 16/19] Address PR review round 11: empty-string validation, capacity > 0, annual_consumption None MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - InverterConfig.validate_type_required_fields: reject empty/whitespace strings for fronius_gen24 address/user/password (not just None), and reject capacity <= 0 for mqtt (0 or negative is invalid for downstream calculations) - consumption.py CSV factory: guard annual_consumption against None value (YAML null) with 'or 0' pattern — consistent with HA config guard added earlier - Add 2 new tests: mqtt capacity=0 rejected; fronius empty-string fields rejected - Update test match string to align with new error message wording Co-Authored-By: Claude Sonnet 4.6 --- src/batcontrol/config_model.py | 11 +++++++---- src/batcontrol/forecastconsumption/consumption.py | 2 +- tests/batcontrol/test_config_model.py | 14 ++++++++++++-- 3 files changed, 20 insertions(+), 7 deletions(-) diff --git a/src/batcontrol/config_model.py b/src/batcontrol/config_model.py index 96da92ac..c055f0d5 100644 --- a/src/batcontrol/config_model.py +++ b/src/batcontrol/config_model.py @@ -84,14 +84,17 @@ def validate_type_required_fields(self): """Enforce type-dependent required fields so KeyErrors can't occur at runtime.""" inverter_type = (self.type or '').lower() if inverter_type == 'fronius_gen24': - missing = [f for f in ('address', 'user', 'password') if getattr(self, f) is None] + missing = [ + f for f in ('address', 'user', 'password') + if getattr(self, f) is None or not str(getattr(self, f)).strip() + ] if missing: raise ValueError( - f"fronius_gen24 inverter requires: {', '.join(missing)}" + f"fronius_gen24 inverter requires non-empty values for: {', '.join(missing)}" ) elif inverter_type == 'mqtt': - if self.capacity is None: - raise ValueError("mqtt inverter requires: capacity") + if self.capacity is None or self.capacity <= 0: + raise ValueError("mqtt inverter requires positive capacity (> 0)") return self diff --git a/src/batcontrol/forecastconsumption/consumption.py b/src/batcontrol/forecastconsumption/consumption.py index dee4df4f..5861f520 100644 --- a/src/batcontrol/forecastconsumption/consumption.py +++ b/src/batcontrol/forecastconsumption/consumption.py @@ -145,7 +145,7 @@ def _create_csv_forecast( consumption = ForecastConsumptionCsv( 'config/' + csv_config['load_profile'], tz, - float(csv_config.get('annual_consumption', 0)), + float(csv_config.get('annual_consumption') or 0), target_resolution=target_resolution ) return consumption diff --git a/tests/batcontrol/test_config_model.py b/tests/batcontrol/test_config_model.py index f12204c6..1cd38f86 100644 --- a/tests/batcontrol/test_config_model.py +++ b/tests/batcontrol/test_config_model.py @@ -182,10 +182,20 @@ def test_fronius_gen24_valid_with_required_fields(self): assert cfg.address == '192.168.1.1' def test_mqtt_requires_capacity(self): - """Test that mqtt type requires capacity.""" - with pytest.raises(ValidationError, match='mqtt inverter requires: capacity'): + """Test that mqtt type requires positive capacity.""" + with pytest.raises(ValidationError, match='mqtt inverter requires positive capacity'): InverterConfig(type='mqtt') + def test_mqtt_rejects_zero_capacity(self): + """Test that mqtt type rejects capacity=0 (invalid for downstream calculations).""" + with pytest.raises(ValidationError, match='mqtt inverter requires positive capacity'): + InverterConfig(type='mqtt', capacity=0) + + def test_fronius_rejects_empty_string_fields(self): + """Test that fronius_gen24 rejects empty-string address/user/password.""" + with pytest.raises(ValidationError, match='fronius_gen24 inverter requires non-empty'): + InverterConfig(type='fronius_gen24', address='', user='admin', password='secret') + def test_mqtt_valid_with_capacity(self): """Test that mqtt validates OK when capacity is present.""" cfg = InverterConfig(type='mqtt', capacity=10000) From 61449b4604424430c4f3f144e2f391b2caea7be6 Mon Sep 17 00:00:00 2001 From: Matthias Strubel Date: Thu, 2 Apr 2026 17:16:48 +0200 Subject: [PATCH 17/19] Address PR review round 12: PV name whitespace validation, provider required fields MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - PvInstallationConfig: add field_validator for name to reject empty/whitespace strings (used as cache keys in forecastsolar — empty would cause collisions); strip leading/trailing whitespace from valid names - UtilityConfig: add model_validator(mode='after') to enforce provider-specific required fields at config load time instead of at DynamicTariff runtime: * awattar_at/awattar_de: require vat, markup, fees * energyforecast/energyforecast_96: require vat, markup, fees, apikey * tibber: require apikey * evcc: require url * tariff_zones: require tariff_zone_1, zone_1_hours, tariff_zone_2, zone_2_hours - Update all affected tests to provide required fields for their provider type - Add 6 new tests covering name whitespace rejection, name stripping, and provider-specific validation errors (awattar, tibber, evcc) Co-Authored-By: Claude Sonnet 4.6 --- src/batcontrol/config_model.py | 41 ++++++++++++++++++ .../test_config_load_integration.py | 4 +- tests/batcontrol/test_config_model.py | 42 +++++++++++++++---- 3 files changed, 77 insertions(+), 10 deletions(-) diff --git a/src/batcontrol/config_model.py b/src/batcontrol/config_model.py index c055f0d5..7e234bd3 100644 --- a/src/batcontrol/config_model.py +++ b/src/batcontrol/config_model.py @@ -119,6 +119,39 @@ class UtilityConfig(BaseModel): tariff_zone_3: Optional[float] = None zone_3_hours: Optional[str] = None + @model_validator(mode='after') + def validate_provider_required_fields(self): + """Enforce provider-specific required fields at config load time.""" + provider = (self.type or '').lower() + if provider in ('awattar_at', 'awattar_de'): + missing = [f for f in ('vat', 'markup', 'fees') if getattr(self, f) is None] + if missing: + raise ValueError( + f"{provider} requires: {', '.join(missing)}" + ) + elif provider in ('energyforecast', 'energyforecast_96'): + missing = [f for f in ('vat', 'markup', 'fees', 'apikey') if getattr(self, f) is None] + if missing: + raise ValueError( + f"{provider} requires: {', '.join(missing)}" + ) + elif provider == 'tibber': + if self.apikey is None: + raise ValueError("tibber requires: apikey") + elif provider == 'evcc': + if self.url is None: + raise ValueError("evcc utility requires: url") + elif provider == 'tariff_zones': + missing = [ + f for f in ('tariff_zone_1', 'zone_1_hours', 'tariff_zone_2', 'zone_2_hours') + if getattr(self, f) is None + ] + if missing: + raise ValueError( + f"tariff_zones requires: {', '.join(missing)}" + ) + return self + class MqttConfig(BaseModel): """MQTT API configuration.""" @@ -186,6 +219,14 @@ class PvInstallationConfig(BaseModel): sensor_unit: Optional[str] = None cache_ttl_hours: Optional[float] = None + @field_validator('name') + @classmethod + def validate_name_nonempty(cls, v): + """Reject empty/whitespace names — used as cache keys in forecastsolar.""" + if not v.strip(): + raise ValueError("PV installation name must not be empty or whitespace") + return v.strip() + class ConsumptionForecastConfig(BaseModel): """Consumption forecast configuration.""" diff --git a/tests/batcontrol/test_config_load_integration.py b/tests/batcontrol/test_config_load_integration.py index 47a06c1c..714b5d72 100644 --- a/tests/batcontrol/test_config_load_integration.py +++ b/tests/batcontrol/test_config_load_integration.py @@ -19,7 +19,7 @@ def _minimal_config(self): """Return a minimal valid config.""" return { 'timezone': 'Europe/Berlin', - 'utility': {'type': 'awattar_de'}, + 'utility': {'type': 'awattar_de', 'vat': 0.19, 'markup': 0.03, 'fees': 0.01}, 'pvinstallations': [{'name': 'Test', 'kWp': 10.0}], } @@ -64,7 +64,7 @@ def test_load_config_missing_pvinstallations_key(self, tmp_path): """Test that missing pvinstallations key fails Pydantic validation.""" config = { 'timezone': 'Europe/Berlin', - 'utility': {'type': 'awattar_de'}, + 'utility': {'type': 'awattar_de', 'vat': 0.19, 'markup': 0.03, 'fees': 0.01}, } path = self._write_config(config, str(tmp_path)) with pytest.raises(RuntimeError, match='pvinstallations'): diff --git a/tests/batcontrol/test_config_model.py b/tests/batcontrol/test_config_model.py index 1cd38f86..f09fbd94 100644 --- a/tests/batcontrol/test_config_model.py +++ b/tests/batcontrol/test_config_model.py @@ -21,7 +21,7 @@ def _minimal_config(self): """Return a minimal valid config dict.""" return { 'timezone': 'Europe/Berlin', - 'utility': {'type': 'awattar_de'}, + 'utility': {'type': 'awattar_de', 'vat': 0.19, 'markup': 0.03, 'fees': 0.01}, 'pvinstallations': [{'name': 'Test PV', 'kWp': 10.0}], } @@ -96,7 +96,7 @@ def test_pvinstallations_required(self): """Test that pvinstallations is required (no default).""" data = { 'timezone': 'Europe/Berlin', - 'utility': {'type': 'awattar_de'}, + 'utility': {'type': 'awattar_de', 'vat': 0.19, 'markup': 0.03, 'fees': 0.01}, } with pytest.raises(ValidationError, match='pvinstallations'): BatcontrolConfig(**data) @@ -222,7 +222,7 @@ def test_string_float_coercion(self): def test_vat_fees_markup_absent_when_not_set(self): """Test that vat/fees/markup are None when not set, so downstream required_fields checks detect missing config.""" - cfg = UtilityConfig(type='tibber') + cfg = UtilityConfig(type='tibber', apikey='test_key') assert cfg.vat is None assert cfg.fees is None assert cfg.markup is None @@ -231,11 +231,28 @@ def test_tariff_zone_coercion(self): cfg = UtilityConfig( type='tariff_zones', tariff_zone_1='0.2733', + zone_1_hours='0-8', tariff_zone_2='0.1734', + zone_2_hours='8-24', ) assert cfg.tariff_zone_1 == 0.2733 assert isinstance(cfg.tariff_zone_1, float) + def test_awattar_requires_vat_fees_markup(self): + """Test that awattar providers require vat/fees/markup at validation time.""" + with pytest.raises(ValidationError, match='awattar_de requires'): + UtilityConfig(type='awattar_de') + + def test_tibber_requires_apikey(self): + """Test that tibber requires apikey at validation time.""" + with pytest.raises(ValidationError, match='tibber requires: apikey'): + UtilityConfig(type='tibber') + + def test_evcc_requires_url(self): + """Test that evcc utility requires url at validation time.""" + with pytest.raises(ValidationError, match='evcc utility requires: url'): + UtilityConfig(type='evcc') + class TestMqttConfig: """Tests for MqttConfig.""" @@ -290,6 +307,16 @@ def test_name_required(self): with pytest.raises(ValidationError, match='name'): PvInstallationConfig(kWp=10.0) + def test_name_empty_string_rejected(self): + """Test that empty/whitespace name is rejected (would cause cache key collisions).""" + with pytest.raises(ValidationError, match='must not be empty'): + PvInstallationConfig(name=' ', kWp=10.0) + + def test_name_stripped(self): + """Test that leading/trailing whitespace is stripped from name.""" + cfg = PvInstallationConfig(name=' My PV ', kWp=10.0) + assert cfg.name == 'My PV' + def test_float_coercion(self): """Test that numeric PV fields are coerced from strings.""" cfg = PvInstallationConfig( @@ -461,12 +488,11 @@ def test_validate_excludes_none_fields(self): def test_validate_utility_vat_excluded_when_not_set(self): """Test that vat/fees/markup are absent when not configured, - so dynamictariff required_fields checks still catch misconfig.""" + so downstream checks still catch misconfig (for providers that need them).""" config = self._full_config() - # Remove vat/fees/markup from utility (simulating tibber config) - del config['utility']['vat'] - del config['utility']['fees'] - del config['utility']['markup'] + # Switch to tibber (which doesn't need vat/fees/markup) to test + # that these optional fields are absent when not provided. + config['utility'] = {'type': 'tibber', 'apikey': 'test_key'} result = validate_config(config) assert 'vat' not in result['utility'] assert 'fees' not in result['utility'] From eb9c5258af8f82273bdc6699db58cba2a6d67d89 Mon Sep 17 00:00:00 2001 From: Matthias Strubel Date: Thu, 2 Apr 2026 17:24:54 +0200 Subject: [PATCH 18/19] Make timezone required in BatcontrolConfig (no default) The dummy config documents timezone as "not optional" and core logic uses it for scheduling/price-window alignment. Silently defaulting to 'Europe/Berlin' masks misconfigured setups. Now fails fast at config load with a clear validation error when timezone is absent. Co-Authored-By: Claude Sonnet 4.6 --- src/batcontrol/config_model.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/batcontrol/config_model.py b/src/batcontrol/config_model.py index 7e234bd3..8409d030 100644 --- a/src/batcontrol/config_model.py +++ b/src/batcontrol/config_model.py @@ -269,7 +269,7 @@ class BatcontrolConfig(BaseModel): """ model_config = ConfigDict(extra='allow') - timezone: str = 'Europe/Berlin' + timezone: str time_resolution_minutes: int = 60 loglevel: str = 'info' logfile_enabled: bool = True From db81c4a30588428e5bf4e8084076638059fd6852 Mon Sep 17 00:00:00 2001 From: Matthias Strubel Date: Thu, 2 Apr 2026 19:18:11 +0200 Subject: [PATCH 19/19] Housekeeping: update stale comments after rebase on main MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - UtilityConfig: update comment to reflect that vat/fees/markup are now validated by validate_provider_required_fields, not by downstream code - dynamictariff.py: add note that required_fields checks are secondary safety guards — primary validation is now in Pydantic at load time Co-Authored-By: Claude Sonnet 4.6 --- src/batcontrol/config_model.py | 6 +++--- src/batcontrol/dynamictariff/dynamictariff.py | 2 ++ 2 files changed, 5 insertions(+), 3 deletions(-) diff --git a/src/batcontrol/config_model.py b/src/batcontrol/config_model.py index 8409d030..8a4452b5 100644 --- a/src/batcontrol/config_model.py +++ b/src/batcontrol/config_model.py @@ -105,9 +105,9 @@ class UtilityConfig(BaseModel): type: str apikey: Optional[str] = None url: Optional[str] = None - # vat/fees/markup are Optional so that downstream required_fields checks - # (in dynamictariff.py) can detect missing config for providers that need them. - # With exclude_none=True, absent keys won't appear in the output dict. + # vat/fees/markup are Optional; validate_provider_required_fields below + # enforces them for known providers at config load time. + # With exclude_none=True, unset fields won't appear in the output dict. vat: Optional[float] = None fees: Optional[float] = None markup: Optional[float] = None diff --git a/src/batcontrol/dynamictariff/dynamictariff.py b/src/batcontrol/dynamictariff/dynamictariff.py index 45e6fcc5..317b55fe 100644 --- a/src/batcontrol/dynamictariff/dynamictariff.py +++ b/src/batcontrol/dynamictariff/dynamictariff.py @@ -42,6 +42,8 @@ def create_tarif_provider(config: dict, timezone, """ selected_tariff = None provider = config.get('type') + # Note: required-field checks below are secondary safety guards. + # Primary validation happens at config load time via Pydantic (config_model.py). if provider.lower() == 'awattar_at': required_fields = ['vat', 'markup', 'fees']