From dddcc2f250b46077bc1b2708fdc9280285dbb229 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C5=81ukasz=20Stachowiak?= Date: Fri, 25 Sep 2026 17:03:56 +0200 Subject: [PATCH] fix: do not fail config entry setup silently async_setup_entry returned False when parcel locker data was missing from the config entry or when the parcel locker ID could not be scraped from inpost.pl. Home Assistant turns that into ConfigEntryState.SETUP_ERROR with reason "Unknown error", logs nothing at all and schedules no retry, so the integration stayed dead until the next restart or manual reload. Both cases now raise a proper exception: ConfigEntryError for broken entry data and ConfigEntryNotReady for problems on InPost side, so the reason is logged and transient failures are retried. The parcel locker page URL is built from data stored when the integration was configured. InPost redirects outdated URLs to /znajdz-paczkomat with HTTP 200, which silently produced no regex match - such a redirect is now detected, reported and the parcel locker data is refetched and saved back to the config entry before giving up. Additionally: - read the response body inside the request timeout - session.request() returns as soon as the headers arrive, so a stalled body left the setup hanging for aiohttp's default five minutes without any log entry - log a warning whenever a request fails or the air data cannot be found - report missing air sensors as ConfigEntryError from the coordinator instead of matching the exception message against a string - store parcel locker data in the config entry as a plain dict - cover async_setup_entry with tests --- custom_components/inpost_air/__init__.py | 95 +++++++-- custom_components/inpost_air/api.py | 110 +++++++--- custom_components/inpost_air/config_flow.py | 10 +- custom_components/inpost_air/coordinator.py | 13 +- tests/test_api.py | 115 +++++++++- tests/test_init.py | 221 ++++++++++++++++++++ 6 files changed, 507 insertions(+), 57 deletions(-) create mode 100644 tests/test_init.py diff --git a/custom_components/inpost_air/__init__.py b/custom_components/inpost_air/__init__.py index fbb9652..19b8bab 100644 --- a/custom_components/inpost_air/__init__.py +++ b/custom_components/inpost_air/__init__.py @@ -1,7 +1,7 @@ """The InPost Air integration.""" from __future__ import annotations -from dataclasses import dataclass +from dataclasses import asdict, dataclass import logging from dacite import from_dict @@ -15,7 +15,7 @@ from custom_components.inpost_air.models import ParcelLocker from custom_components.inpost_air.utils import get_device_info, get_parcel_locker_url -from .api import InPostAirPoint, InPostApi +from .api import InPostAirApiClientError, InPostAirPoint, InPostApi _LOGGER = logging.getLogger(__name__) @@ -35,34 +35,80 @@ class InPostAirData: PLATFORMS: list[Platform] = [Platform.SENSOR] +def get_configured_point(entry: InPostAirConfiEntry) -> InPostAirPoint: + """Read parcel locker data stored in the config entry.""" + entry_data = entry.data.get("parcel_locker") + + if entry_data is None: + raise ConfigEntryError( + "Config entry does not contain any parcel locker data. " + "Please remove the integration and set it up again." + ) + + if isinstance(entry_data, InPostAirPoint): + return entry_data + + try: + return from_dict(InPostAirPoint, entry_data) + except Exception as ex: + raise ConfigEntryError( + f"Parcel locker data stored in the config entry is invalid: {ex}" + ) from ex + + +async def resolve_parcel_locker( + hass: HomeAssistant, + entry: InPostAirConfiEntry, + api_client: InPostApi, + point: InPostAirPoint, +) -> tuple[InPostAirPoint, str]: + """Find the parcel locker ID, refreshing outdated config entry data if needed.""" + refreshed_point = None + + try: + if (locker_id := await api_client.find_parcel_locker_id(point)) is not None: + return point, locker_id + + _LOGGER.info( + "Refetching data of parcel locker %s - the one stored in the config entry might be outdated", + point.n, + ) + refreshed_point = await api_client.refresh_parcel_locker(point.n) + + if refreshed_point is not None and refreshed_point != point: + locker_id = await api_client.find_parcel_locker_id(refreshed_point) + except InPostAirApiClientError as err: + # Transient problem on InPost side - let Home Assistant retry the setup. + raise ConfigEntryNotReady(f"Error communicating with InPost: {err}") from err + + if locker_id is None: + raise ConfigEntryNotReady( + f"Could not find air quality data of parcel locker {point.n} on {get_parcel_locker_url(point)}" + ) + + hass.config_entries.async_update_entry( + entry, + data={**entry.data, "parcel_locker": asdict(refreshed_point)}, + ) + + return refreshed_point, locker_id + + async def async_setup_entry(hass: HomeAssistant, entry: InPostAirConfiEntry) -> bool: """Set up InPost Air from a config entry.""" api_client = InPostApi(hass) - entry_data = entry.data.get("parcel_locker") + point = get_configured_point(entry) - if ( - point := None - if entry_data is None - else entry_data - if isinstance(entry_data, InPostAirPoint) - else from_dict(InPostAirPoint, entry_data) - ) is None: - return False - - if (parcel_locker_id := await api_client.find_parcel_locker_id(point)) is None: - return False + point, parcel_locker_id = await resolve_parcel_locker( + hass, entry, api_client, point + ) parcel_locker = ParcelLocker(point.n, parcel_locker_id) coordinator = InPostAirDataCoordinator(hass, api_client, parcel_locker) entry.runtime_data = InPostAirData(parcel_locker, coordinator) - try: - await coordinator.async_config_entry_first_refresh() - except ConfigEntryNotReady as ex: - if "Air sensors are not available" in str(ex): - raise ConfigEntryError(ex) - raise ex + await coordinator.async_config_entry_first_refresh() device_registry = dr.async_get(hass) device_registry.async_get_or_create( @@ -92,12 +138,19 @@ async def async_migrate_entry(hass: HomeAssistant, config_entry: InPostAirConfiE if config_entry.version > 2: # This means the user has downgraded from a future version + _LOGGER.error( + "Cannot migrate %s from version %s - it was created by a newer version of the integration", + config_entry.title, + config_entry.version, + ) return False if config_entry.version == 1: hass.config_entries.async_update_entry( config_entry, - data={"parcel_locker": from_dict(InPostAirPoint, config_entry.data)}, + data={ + "parcel_locker": asdict(from_dict(InPostAirPoint, config_entry.data)) + }, version=2, ) diff --git a/custom_components/inpost_air/api.py b/custom_components/inpost_air/api.py index 2e58e5b..b086eea 100644 --- a/custom_components/inpost_air/api.py +++ b/custom_components/inpost_air/api.py @@ -2,9 +2,11 @@ import asyncio from dataclasses import dataclass +import json import logging import re -from aiohttp import ClientResponse, ClientResponseError +from typing import Any +from aiohttp import ClientResponseError from dacite import from_dict from homeassistant.core import HomeAssistant from homeassistant.helpers.aiohttp_client import async_create_clientsession @@ -13,6 +15,24 @@ _LOGGER = logging.getLogger(__name__) +REQUEST_TIMEOUT = 30 + +AIR_DATA_PATTERN = re.compile( + r"data-shipx-url=\"/shipx-point-data/(.*?)/(.*?)/air_index_level\"" +) + + +@dataclass +class ApiResponse: + """API response with an already read body.""" + + url: str + body: str + + def json(self) -> Any: + """Parse the body as JSON.""" + return json.loads(self.body) + @dataclass class ParcelLockerListResponse: @@ -43,10 +63,15 @@ async def _request( url: str, headers: dict | None = None, raise_client_response_error: bool = False, - ) -> ClientResponse: - """Get information from the API.""" + ) -> ApiResponse: + """Get information from the API. + + The body is read inside the timeout on purpose - `session.request` + returns as soon as the response headers arrive, so reading the body + outside of it would leave that part of the request without any limit. + """ try: - async with asyncio.timeout(30): + async with asyncio.timeout(REQUEST_TIMEOUT): response = await self.session.request( method=method, url=url, @@ -54,23 +79,25 @@ async def _request( ) response.raise_for_status() - return response + return ApiResponse(url=str(response.url), body=await response.text()) except TimeoutError as e: - _LOGGER.warning("Request timed out") + _LOGGER.warning("Request to %s timed out", url) raise InPostAirApiClientError("Request timed out") from e except ClientResponseError as e: if raise_client_response_error: raise - raise InPostAirApiClientError("Something really wrong happened!") from e + _LOGGER.warning("Request to %s failed with status %s", url, e.status) + raise InPostAirApiClientError( + f"Request to {url} failed with status {e.status}" + ) from e except Exception as exception: # pylint: disable=broad-except + _LOGGER.warning("Request to %s failed: %s", url, exception) raise InPostAirApiClientError( "Something really wrong happened!" ) from exception - async def _search_easypack24_locker( - self, locker_code: str - ) -> InPostAirPoint | None: + async def _search_easypack24_locker(self, locker_code: str) -> dict | None: """Find info about given parcel locker.""" if not locker_code or locker_code == "": return None @@ -79,7 +106,7 @@ async def _search_easypack24_locker( method="get", url="https://api-shipx-pl.easypack24.net/v1/points/" + locker_code, ) - resp = await response.json() + resp = response.json() error = resp.get("error") if error: @@ -120,11 +147,7 @@ async def search_parcel_locker(self, locker_code: str) -> InPostAirPoint | None: method="get", url="https://inpost.pl/sites/default/files/points.json" ) parcel_locker = next( - ( - x - for x in (await response.json()).get("items") - if x.get("n") == locker_code - ), + (x for x in response.json().get("items") if x.get("n") == locker_code), None, ) @@ -133,27 +156,54 @@ async def search_parcel_locker(self, locker_code: str) -> InPostAirPoint | None: return from_dict(InPostAirPoint, parcel_locker) if parcel_locker else None + async def refresh_parcel_locker(self, locker_code: str) -> InPostAirPoint | None: + """Get current data of an already known parcel locker. + + Data stored in a config entry gets outdated when InPost changes details + of a parcel locker, so it has to be refetched. The single point endpoint + is tried first - the full list is a few megabytes of JSON. + """ + parcel_locker = await self._search_easypack24_locker(locker_code) + + if parcel_locker is None: + return await self.search_parcel_locker(locker_code) + + return from_dict(InPostAirPoint, parcel_locker) + async def get_parcel_lockers_list(self) -> list[InPostAirPoint]: """Get parcel lockers list.""" response = await self._request( method="get", url="https://inpost.pl/sites/default/files/points.json" ) - response_data = from_dict(ParcelLockerListResponse, await response.json()) + response_data = from_dict(ParcelLockerListResponse, response.json()) return response_data.items async def find_parcel_locker_id(self, point: InPostAirPoint) -> str | None: """Find parcel locker ID by its code.""" - response = await self._request( - method="get", - url=get_parcel_locker_url(point), - ) - match = re.search( - r"data-shipx-url=\"/shipx-point-data/(.*?)/(.*?)/air_index_level\"", - await response.text(), - ) + url = get_parcel_locker_url(point) + response = await self._request(method="get", url=url) + + if response.url.rstrip("/") != url.rstrip("/"): + _LOGGER.warning( + "Page of parcel locker %s (%s) redirected to %s - data stored for that parcel locker is outdated", + point.n, + url, + response.url, + ) + return None + + match = AIR_DATA_PATTERN.search(response.body) + + if match is None: + _LOGGER.warning( + "Could not find air quality data of parcel locker %s on %s", + point.n, + url, + ) + return None - return None if match is None else match.group(1) + return match.group(1) async def get_parcel_locker_air_data( self, locker_code: str, locker_id: str @@ -171,11 +221,11 @@ async def get_parcel_locker_air_data( raise InPostAirApiClientSensorsMissingError( "Air sensors are not available" ) from e - raise InPostAirApiClientError("Something really wrong happened!") from e - except: - raise + raise InPostAirApiClientError( + f"Request for air data of parcel locker {locker_code} failed with status {e.status}" + ) from e - return from_dict(ParcelLockerAirDataResponse, await response.json()) + return from_dict(ParcelLockerAirDataResponse, response.json()) class InPostAirApiClientError(Exception): diff --git a/custom_components/inpost_air/config_flow.py b/custom_components/inpost_air/config_flow.py index 0836856..99c12cc 100644 --- a/custom_components/inpost_air/config_flow.py +++ b/custom_components/inpost_air/config_flow.py @@ -1,7 +1,7 @@ """Config flow for InPost Air integration.""" from __future__ import annotations -from dataclasses import dataclass +from dataclasses import asdict, dataclass import logging from typing import Any @@ -48,7 +48,13 @@ async def validate_input(hass: HomeAssistant, data: dict[str, Any]) -> InPostAir try: parcel_locker_id = await api_client.find_parcel_locker_id(parcel_locker) + + if parcel_locker_id is None: + raise ParcelLockerWithoutAirData + await api_client.get_parcel_locker_air_data(parcel_locker.n, parcel_locker_id) + except ParcelLockerWithoutAirData: + raise except Exception as exc: raise ParcelLockerWithoutAirData from exc @@ -79,7 +85,7 @@ async def async_step_user( else: return self.async_create_entry( title=f"Parcel locker {parcel_locker.n}", - data={"parcel_locker": parcel_locker}, + data={"parcel_locker": asdict(parcel_locker)}, ) parcel_lockers = [ diff --git a/custom_components/inpost_air/coordinator.py b/custom_components/inpost_air/coordinator.py index eddf4aa..3f9df01 100644 --- a/custom_components/inpost_air/coordinator.py +++ b/custom_components/inpost_air/coordinator.py @@ -7,10 +7,15 @@ import re from homeassistant.core import HomeAssistant +from homeassistant.exceptions import ConfigEntryError from homeassistant.helpers.update_coordinator import DataUpdateCoordinator, UpdateFailed from .models import ParcelLocker -from .api import InPostAirApiClientError, InPostApi +from .api import ( + InPostAirApiClientError, + InPostAirApiClientSensorsMissingError, + InPostApi, +) from .const import Entities _LOGGER = logging.getLogger(__name__) @@ -90,7 +95,11 @@ async def _async_update_data(self): return { x.name: x for line in data.air_sensors if (x := create_value(line)) } + except InPostAirApiClientSensorsMissingError as err: + # Nothing to poll for - retrying will not help, so let the config + # entry fail permanently instead of ending up in a retry loop. + raise ConfigEntryError(err) from err except InPostAirApiClientError as err: raise UpdateFailed(err) from err except Exception as err: - raise UpdateFailed("Error communicating with API") from err + raise UpdateFailed(f"Error communicating with API: {err}") from err diff --git a/tests/test_api.py b/tests/test_api.py index b6315e5..2829882 100644 --- a/tests/test_api.py +++ b/tests/test_api.py @@ -1,7 +1,15 @@ +import asyncio +import os +from unittest.mock import patch + import pytest import pytest_socket -import os -from custom_components.inpost_air.api import InPostApi + +from custom_components.inpost_air.api import ( + ApiResponse, + InPostAirApiClientError, + InPostApi, +) from custom_components.inpost_air.models import ( InPostAirPoint, InPostAirPointCoordinates, @@ -66,3 +74,106 @@ async def test_find_parcel_locker_id(hass, _allow_inpost_requests): async def test_air_data(hass, _allow_inpost_requests): response = await InPostApi(hass).get_parcel_locker_air_data("AJE01BAPP", "56311") assert response is not None + + +class _StallingResponse: + """Response that never finishes sending its body.""" + + def __init__(self, url: str) -> None: + self.url = url + self.status = 200 + + def raise_for_status(self) -> None: + pass + + async def text(self) -> str: + await asyncio.sleep(10) + return "" + + +class _StallingSession: + """Session returning responses with a stalled body.""" + + async def request(self, method: str, url: str, headers=None) -> _StallingResponse: + return _StallingResponse(url) + + +PAGE_WITH_AIR_DATA = ( + '
' +) + +mocked_point = InPostAirPoint( + "AJE01BAPP", + 1, + "Market Dino", + "", + "", + "006", + "Andrzejewo", + "andrzejewo", + "Warszawska", + "mazowieckie", + "07-305", + "62A", + "24/7", + "[]", + InPostAirPointCoordinates(52.83679, 22.20968), + 0, + 1, +) + +mocked_point_url = ( + "https://inpost.pl/paczkomat-andrzejewo-aje01bapp-warszawska-paczkomaty-mazowieckie" +) + + +async def test_request_times_out_while_reading_body(hass): + """Reading the body is covered by the request timeout.""" + api = InPostApi(hass) + api.session = _StallingSession() + + with patch("custom_components.inpost_air.api.REQUEST_TIMEOUT", 0.05): + with pytest.raises(InPostAirApiClientError, match="Request timed out"): + await api._request(method="get", url="https://inpost.pl/whatever") + + +async def test_find_parcel_locker_id_from_page(hass): + """Parcel locker ID is scraped from its page.""" + api = InPostApi(hass) + + with patch.object( + InPostApi, + "_request", + return_value=ApiResponse(url=mocked_point_url, body=PAGE_WITH_AIR_DATA), + ): + assert await api.find_parcel_locker_id(mocked_point) == "56311" + + +async def test_find_parcel_locker_id_without_air_data(hass, caplog): + """Missing air data on the page is reported instead of failing silently.""" + api = InPostApi(hass) + + with patch.object( + InPostApi, + "_request", + return_value=ApiResponse(url=mocked_point_url, body="
"), + ): + assert await api.find_parcel_locker_id(mocked_point) is None + + assert "Could not find air quality data" in caplog.text + + +async def test_find_parcel_locker_id_when_page_redirects(hass, caplog): + """Redirect away from the parcel locker page means outdated entry data.""" + api = InPostApi(hass) + + with patch.object( + InPostApi, + "_request", + return_value=ApiResponse( + url="https://inpost.pl/znajdz-paczkomat", body="
" + ), + ): + assert await api.find_parcel_locker_id(mocked_point) is None + + assert "redirected to" in caplog.text diff --git a/tests/test_init.py b/tests/test_init.py new file mode 100644 index 0000000..02387fa --- /dev/null +++ b/tests/test_init.py @@ -0,0 +1,221 @@ +"""Define tests for setting up a config entry.""" + +from dataclasses import asdict +from unittest.mock import patch + +import pytest +from homeassistant.config_entries import ConfigEntryState +from pytest_homeassistant_custom_component.common import MockConfigEntry + +from custom_components.inpost_air.api import ( + InPostAirApiClientError, + InPostAirApiClientSensorsMissingError, + InPostApi, + ParcelLockerAirDataResponse, +) +from custom_components.inpost_air.const import DOMAIN +from custom_components.inpost_air.models import ( + InPostAirPoint, + InPostAirPointCoordinates, +) + +mocked_point = InPostAirPoint( + "AJE01BAPP", + 1, + "Market Dino", + "", + "", + "006", + "Andrzejewo", + "andrzejewo", + "Warszawska", + "mazowieckie", + "07-305", + "62A", + "24/7", + "[]", + InPostAirPointCoordinates(52.83679, 22.20968), + 0, + 1, +) + +mocked_air_data = ParcelLockerAirDataResponse( + message="", + air_index_level="good", + air_sensors=["PM25:10.0:20.0", "TEMPERATURE:21.5:", "HUMIDITY:50.0:"], +) + + +def create_entry(hass, data=None) -> MockConfigEntry: + """Add a config entry to hass.""" + entry = MockConfigEntry( + domain=DOMAIN, + version=2, + minor_version=1, + unique_id=mocked_point.n, + title=f"Parcel locker {mocked_point.n}", + data={"parcel_locker": asdict(mocked_point)} if data is None else data, + ) + entry.add_to_hass(hass) + return entry + + +async def setup_entry(hass, entry: MockConfigEntry) -> None: + """Run setup of the given config entry.""" + await hass.config_entries.async_setup(entry.entry_id) + await hass.async_block_till_done() + + +async def test_setup_entry(hass): + """Entry is loaded when parcel locker data can be fetched.""" + entry = create_entry(hass) + + with ( + patch.object(InPostApi, "find_parcel_locker_id", return_value="56311"), + patch.object( + InPostApi, "get_parcel_locker_air_data", return_value=mocked_air_data + ), + ): + await setup_entry(hass, entry) + + assert entry.state is ConfigEntryState.LOADED + assert entry.runtime_data.parcel_locker.locker_id == "56311" + assert hass.states.get("sensor.parcel_locker_aje01bapp_temperature") is not None + + +async def test_setup_entry_without_parcel_locker_data(hass, caplog): + """Entry with no parcel locker data fails with a logged error, not silently.""" + entry = create_entry(hass, data={}) + + await setup_entry(hass, entry) + + assert entry.state is ConfigEntryState.SETUP_ERROR + assert entry.reason is not None and entry.reason != "Unknown error" + assert "does not contain any parcel locker data" in caplog.text + + +async def test_setup_entry_with_invalid_parcel_locker_data(hass, caplog): + """Entry with broken parcel locker data fails with a logged error.""" + entry = create_entry(hass, data={"parcel_locker": {"n": "AJE01BAPP"}}) + + await setup_entry(hass, entry) + + assert entry.state is ConfigEntryState.SETUP_ERROR + assert "is invalid" in caplog.text + + +async def test_setup_entry_retries_when_parcel_locker_id_not_found(hass): + """Missing parcel locker ID schedules a retry instead of killing the entry.""" + entry = create_entry(hass) + + with ( + patch.object(InPostApi, "find_parcel_locker_id", return_value=None), + patch.object(InPostApi, "refresh_parcel_locker", return_value=mocked_point), + ): + await setup_entry(hass, entry) + + assert entry.state is ConfigEntryState.SETUP_RETRY + assert entry.reason is not None and "AJE01BAPP" in entry.reason + + +async def test_setup_entry_retries_on_api_error(hass): + """API errors schedule a retry instead of killing the entry.""" + entry = create_entry(hass) + + with patch.object( + InPostApi, + "find_parcel_locker_id", + side_effect=InPostAirApiClientError("Request timed out"), + ): + await setup_entry(hass, entry) + + assert entry.state is ConfigEntryState.SETUP_RETRY + + +async def test_setup_entry_refreshes_outdated_parcel_locker_data(hass): + """Outdated data stored in the entry is refetched and saved.""" + entry = create_entry(hass) + refreshed_point = InPostAirPoint(**{**asdict(mocked_point), "e": "Nowa Warszawska"}) + refreshed_point.l = InPostAirPointCoordinates(**asdict(mocked_point).get("l")) + + def find_parcel_locker_id(_self, point: InPostAirPoint): + return "56311" if point.e == "Nowa Warszawska" else None + + with ( + patch.object( + InPostApi, + "find_parcel_locker_id", + autospec=True, + side_effect=find_parcel_locker_id, + ), + patch.object(InPostApi, "refresh_parcel_locker", return_value=refreshed_point), + patch.object( + InPostApi, "get_parcel_locker_air_data", return_value=mocked_air_data + ), + ): + await setup_entry(hass, entry) + + assert entry.state is ConfigEntryState.LOADED + assert entry.data["parcel_locker"] == asdict(refreshed_point) + + +async def test_setup_entry_without_air_sensors(hass): + """Parcel locker without air sensors fails permanently, without retrying.""" + entry = create_entry(hass) + + with ( + patch.object(InPostApi, "find_parcel_locker_id", return_value="56311"), + patch.object( + InPostApi, + "get_parcel_locker_air_data", + side_effect=InPostAirApiClientSensorsMissingError( + "Air sensors are not available" + ), + ), + ): + await setup_entry(hass, entry) + + assert entry.state is ConfigEntryState.SETUP_ERROR + assert entry.reason == "Air sensors are not available" + + +@pytest.mark.parametrize("expected_lingering_timers", [True]) +async def test_unload_entry(hass): + """Entry can be unloaded.""" + entry = create_entry(hass) + + with ( + patch.object(InPostApi, "find_parcel_locker_id", return_value="56311"), + patch.object( + InPostApi, "get_parcel_locker_air_data", return_value=mocked_air_data + ), + ): + await setup_entry(hass, entry) + + assert await hass.config_entries.async_unload(entry.entry_id) + await hass.async_block_till_done() + + assert entry.state is ConfigEntryState.NOT_LOADED + + +async def test_migrate_entry_from_version_1(hass): + """Version 1 entry data is migrated into a serializable dict.""" + entry = MockConfigEntry( + domain=DOMAIN, + version=1, + unique_id=mocked_point.n, + data=asdict(mocked_point), + ) + entry.add_to_hass(hass) + + with ( + patch.object(InPostApi, "find_parcel_locker_id", return_value="56311"), + patch.object( + InPostApi, "get_parcel_locker_air_data", return_value=mocked_air_data + ), + ): + await setup_entry(hass, entry) + + assert entry.version == 2 + assert entry.data == {"parcel_locker": asdict(mocked_point)} + assert entry.state is ConfigEntryState.LOADED