From ec324fbad9ffb200712b22f1eb5c0502a1ea40ce Mon Sep 17 00:00:00 2001 From: Claude Date: Tue, 17 Mar 2026 18:26:03 +0000 Subject: [PATCH 1/9] 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 ab51044623ded3254e0d8b78dca0ac4c843a68de Mon Sep 17 00:00:00 2001 From: Claude Date: Tue, 17 Mar 2026 18:34:58 +0000 Subject: [PATCH 2/9] 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 69de7c239ac5812ae0862472b28622ddfa5f5290 Mon Sep 17 00:00:00 2001 From: Claude Date: Tue, 17 Mar 2026 18:39:57 +0000 Subject: [PATCH 3/9] 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 134c125a6d5720e01df0306007fbc25ee25b7074 Mon Sep 17 00:00:00 2001 From: Claude Date: Tue, 17 Mar 2026 18:59:36 +0000 Subject: [PATCH 4/9] 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 9f82f47d325060a23a6b2434aca98cc401937012 Mon Sep 17 00:00:00 2001 From: Matthias Strubel Date: Tue, 17 Mar 2026 20:13:39 +0100 Subject: [PATCH 5/9] 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 ec85b819d0f25374d7064a9ed3fa6156da1a43a7 Mon Sep 17 00:00:00 2001 From: Matthias Strubel Date: Tue, 17 Mar 2026 20:23:23 +0100 Subject: [PATCH 6/9] 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 a045d6341e9ed06a4a88f9bc31ec306f9637f0ce Mon Sep 17 00:00:00 2001 From: Matthias Strubel Date: Tue, 17 Mar 2026 20:35:03 +0100 Subject: [PATCH 7/9] 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 6903b32bc9433b0d858c36957debb0354a0fc9ac Mon Sep 17 00:00:00 2001 From: Matthias Strubel Date: Tue, 17 Mar 2026 21:07:45 +0100 Subject: [PATCH 8/9] 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 6307d1d0c5265e829b6fdece49aba10df03d50c4 Mon Sep 17 00:00:00 2001 From: Matthias Strubel Date: Tue, 17 Mar 2026 21:28:07 +0100 Subject: [PATCH 9/9] 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. """