From 9a65a89aa468d15fc3fcef45fd6587ddbc16ec07 Mon Sep 17 00:00:00 2001 From: rikroe <42204099+rikroe@users.noreply.github.com> Date: Sat, 6 May 2023 10:00:37 +0200 Subject: [PATCH] Improve internet/API error handling for BMW (#90274) * Improve internet/API error handling * Switch to library exceptions for HTTP status errors * Remove duplicate logging after reconnect * Raise UpdateFailed instead of custom log handling --------- Co-authored-by: rikroe Co-authored-by: G Johansson --- .../bmw_connected_drive/config_flow.py | 13 ++++- .../bmw_connected_drive/coordinator.py | 27 ++++------- .../bmw_connected_drive/test_config_flow.py | 48 ++++++++++++++++--- 3 files changed, 60 insertions(+), 28 deletions(-) diff --git a/homeassistant/components/bmw_connected_drive/config_flow.py b/homeassistant/components/bmw_connected_drive/config_flow.py index 0cde37ba6b3..eb58a6c7f13 100644 --- a/homeassistant/components/bmw_connected_drive/config_flow.py +++ b/homeassistant/components/bmw_connected_drive/config_flow.py @@ -6,7 +6,8 @@ from typing import Any from bimmer_connected.api.authentication import MyBMWAuthentication from bimmer_connected.api.regions import get_region_from_name -from httpx import HTTPError +from bimmer_connected.models import MyBMWAPIError, MyBMWAuthError +from httpx import RequestError import voluptuous as vol from homeassistant import config_entries, core, exceptions @@ -41,7 +42,9 @@ async def validate_input( try: await auth.login() - except HTTPError as ex: + except MyBMWAuthError as ex: + raise InvalidAuth from ex + except (MyBMWAPIError, RequestError) as ex: raise CannotConnect from ex # Return info that you want to store in the config entry. @@ -80,6 +83,8 @@ class BMWConfigFlow(config_entries.ConfigFlow, domain=DOMAIN): } except CannotConnect: errors["base"] = "cannot_connect" + except InvalidAuth: + errors["base"] = "invalid_auth" if info: if self._reauth_entry: @@ -160,3 +165,7 @@ class BMWOptionsFlow(config_entries.OptionsFlowWithConfigEntry): class CannotConnect(exceptions.HomeAssistantError): """Error to indicate we cannot connect.""" + + +class InvalidAuth(exceptions.HomeAssistantError): + """Error to indicate there is invalid auth.""" diff --git a/homeassistant/components/bmw_connected_drive/coordinator.py b/homeassistant/components/bmw_connected_drive/coordinator.py index ae139d4c64a..9c3e15a808b 100644 --- a/homeassistant/components/bmw_connected_drive/coordinator.py +++ b/homeassistant/components/bmw_connected_drive/coordinator.py @@ -6,8 +6,8 @@ import logging from bimmer_connected.account import MyBMWAccount from bimmer_connected.api.regions import get_region_from_name -from bimmer_connected.models import GPSPosition -from httpx import HTTPError, HTTPStatusError, TimeoutException +from bimmer_connected.models import GPSPosition, MyBMWAPIError, MyBMWAuthError +from httpx import RequestError from homeassistant.config_entries import ConfigEntry from homeassistant.const import CONF_PASSWORD, CONF_REGION, CONF_USERNAME @@ -56,20 +56,12 @@ class BMWDataUpdateCoordinator(DataUpdateCoordinator[None]): try: await self.account.get_vehicles() - except (HTTPError, HTTPStatusError, TimeoutException) as err: - if isinstance(err, HTTPStatusError) and err.response.status_code == 429: - # Increase scan interval to not jump to not bring up the issue next time - self.update_interval = timedelta( - seconds=DEFAULT_SCAN_INTERVAL_SECONDS * 3 - ) - if isinstance(err, HTTPStatusError) and err.response.status_code in ( - 401, - 403, - ): - # Clear refresh token only and trigger reauth - self._update_config_entry_refresh_token(None) - raise ConfigEntryAuthFailed(str(err)) from err - raise UpdateFailed(f"Error communicating with BMW API: {err}") from err + except MyBMWAuthError as err: + # Clear refresh token and trigger reauth + self._update_config_entry_refresh_token(None) + raise ConfigEntryAuthFailed(err) from err + except (MyBMWAPIError, RequestError) as err: + raise UpdateFailed(err) from err if self.account.refresh_token != old_refresh_token: self._update_config_entry_refresh_token(self.account.refresh_token) @@ -79,9 +71,6 @@ class BMWDataUpdateCoordinator(DataUpdateCoordinator[None]): self.account.refresh_token, ) - # Reset scan interval after successful update - self.update_interval = timedelta(seconds=DEFAULT_SCAN_INTERVAL_SECONDS) - def _update_config_entry_refresh_token(self, refresh_token: str | None) -> None: """Update or delete the refresh_token in the Config Entry.""" data = { diff --git a/tests/components/bmw_connected_drive/test_config_flow.py b/tests/components/bmw_connected_drive/test_config_flow.py index 4db57ad3022..e471a61d027 100644 --- a/tests/components/bmw_connected_drive/test_config_flow.py +++ b/tests/components/bmw_connected_drive/test_config_flow.py @@ -3,7 +3,8 @@ from copy import deepcopy from unittest.mock import patch from bimmer_connected.api.authentication import MyBMWAuthentication -from httpx import HTTPError +from bimmer_connected.models import MyBMWAPIError, MyBMWAuthError +from httpx import RequestError from homeassistant import config_entries, data_entry_flow from homeassistant.components.bmw_connected_drive.config_flow import DOMAIN @@ -37,15 +38,48 @@ async def test_show_form(hass: HomeAssistant) -> None: assert result["step_id"] == "user" -async def test_connection_error(hass: HomeAssistant) -> None: - """Test we show user form on BMW connected drive connection error.""" - - def _mock_get_oauth_token(*args, **kwargs): - pass +async def test_authentication_error(hass: HomeAssistant) -> None: + """Test we show user form on MyBMW authentication error.""" with patch( "bimmer_connected.api.authentication.MyBMWAuthentication.login", - side_effect=HTTPError("login failure"), + side_effect=MyBMWAuthError("Login failed"), + ): + result = await hass.config_entries.flow.async_init( + DOMAIN, + context={"source": config_entries.SOURCE_USER}, + data=FIXTURE_USER_INPUT, + ) + + assert result["type"] == data_entry_flow.FlowResultType.FORM + assert result["step_id"] == "user" + assert result["errors"] == {"base": "invalid_auth"} + + +async def test_connection_error(hass: HomeAssistant) -> None: + """Test we show user form on MyBMW API error.""" + + with patch( + "bimmer_connected.api.authentication.MyBMWAuthentication.login", + side_effect=RequestError("Connection reset"), + ): + result = await hass.config_entries.flow.async_init( + DOMAIN, + context={"source": config_entries.SOURCE_USER}, + data=FIXTURE_USER_INPUT, + ) + + assert result["type"] == data_entry_flow.FlowResultType.FORM + assert result["step_id"] == "user" + assert result["errors"] == {"base": "cannot_connect"} + + +async def test_api_error(hass: HomeAssistant) -> None: + """Test we show user form on general connection error.""" + + with patch( + "bimmer_connected.api.authentication.MyBMWAuthentication.login", + side_effect=MyBMWAPIError("400 Bad Request"), ): result = await hass.config_entries.flow.async_init( DOMAIN,