Add exception handling for unexpected responses (#580)

* Add exception handling for unexpected responses

* Adjust tests and error handling in account

---------

Co-authored-by: rikroe <rikroe@users.noreply.github.com>

.pre-commit-config.yaml

@@ -1,20 +1,20 @@
 repos:
   - repo: https://github.com/charliermarsh/ruff-pre-commit
-    rev: v0.0.282
+    rev: v0.1.5
     hooks:
       - id: ruff
         args:
           - --fix
   - repo: https://github.com/psf/black
-    rev: 23.7.0
+    rev: 23.11.0
     hooks:
       - id: black
         args:
           - --safe
           - --quiet
         files: ^(bimmer_connected/.+)?[^/]+\.py$
   - repo: https://github.com/codespell-project/codespell
-    rev: v2.2.5
+    rev: v2.2.6
     hooks:
       - id: codespell
         args:
@@ -24,7 +24,7 @@ repos:
         exclude_types: [csv, json]
         exclude: ^test/responses/
   - repo: https://github.com/pre-commit/mirrors-mypy
-    rev: v1.4.1
+    rev: v1.7.0
     hooks: 
       - id: mypy
         name: mypy

bimmer_connected/account.py

@@ -1,6 +1,7 @@
 """Access to a MyBMW account and all vehicles therein."""
 
 import datetime
+import json
 import logging
 from dataclasses import InitVar, dataclass, field
 from typing import List, Optional
@@ -17,7 +18,7 @@
     VEHICLES_URL,
     CarBrands,
 )
-from bimmer_connected.models import AnonymizedResponse, GPSPosition
+from bimmer_connected.models import AnonymizedResponse, GPSPosition, MyBMWAPIError, MyBMWAuthError, MyBMWQuotaError
 from bimmer_connected.vehicle import MyBMWVehicle
 
 VALID_UNTIL_OFFSET = datetime.timedelta(seconds=10)
@@ -92,43 +93,59 @@ async def get_vehicles(self, force_init: bool = False) -> None:
             await self._init_vehicles()
 
         async with MyBMWClient(self.config) as client:
+            error_count = 0
             for vehicle in self.vehicles:
                 # Get the detailed vehicle state
-                state_response = await client.get(
-                    VEHICLE_STATE_URL,
-                    params={
-                        "apptimezone": self.utcdiff,
-                        "appDateTime": int(fetched_at.timestamp() * 1000),
-                    },
-                    headers={
-                        **client.generate_default_header(vehicle.brand),
-                        "bmw-vin": vehicle.vin,
-                    },
-                )
-                vehicle_state = state_response.json()
-
-                # Get detailed charging settings if supported by vehicle
-                charging_settings = None
-                if vehicle_state[ATTR_CAPABILITIES].get("isChargingPlanSupported", False) or vehicle_state[
-                    ATTR_CAPABILITIES
-                ].get("isChargingSettingsEnabled", False):
-                    charging_settings_response = await client.get(
-                        VEHICLE_CHARGING_DETAILS_URL,
+                try:
+                    state_response = await client.get(
+                        VEHICLE_STATE_URL,
                         params={
-                            "fields": "charging-profile",
-                            "has_charging_settings_capabilities": vehicle_state[ATTR_CAPABILITIES][
-                                "isChargingSettingsEnabled"
-                            ],
+                            "apptimezone": self.utcdiff,
+                            "appDateTime": int(fetched_at.timestamp() * 1000),
                         },
                         headers={
                             **client.generate_default_header(vehicle.brand),
-                            "bmw-current-date": fetched_at.isoformat(),
                             "bmw-vin": vehicle.vin,
                         },
                     )
-                    charging_settings = charging_settings_response.json()
-
-                self.add_vehicle(vehicle.data, vehicle_state, charging_settings, fetched_at)
+                    vehicle_state = state_response.json()
+
+                    # Get detailed charging settings if supported by vehicle
+                    charging_settings = None
+                    if vehicle_state[ATTR_CAPABILITIES].get("isChargingPlanSupported", False) or vehicle_state[
+                        ATTR_CAPABILITIES
+                    ].get("isChargingSettingsEnabled", False):
+                        charging_settings_response = await client.get(
+                            VEHICLE_CHARGING_DETAILS_URL,
+                            params={
+                                "fields": "charging-profile",
+                                "has_charging_settings_capabilities": vehicle_state[ATTR_CAPABILITIES][
+                                    "isChargingSettingsEnabled"
+                                ],
+                            },
+                            headers={
+                                **client.generate_default_header(vehicle.brand),
+                                "bmw-current-date": fetched_at.isoformat(),
+                                "bmw-vin": vehicle.vin,
+                            },
+                        )
+                        charging_settings = charging_settings_response.json()
+
+                    self.add_vehicle(vehicle.data, vehicle_state, charging_settings, fetched_at)
+                except (MyBMWAPIError, json.JSONDecodeError) as ex:
+                    # We don't want to fail completely if one vehicle fails, but we want to know about it
+                    error_count += 1
+
+                    # If it's a MyBMWQuotaError or MyBMWAuthError, we want to raise it
+                    if isinstance(ex, (MyBMWQuotaError, MyBMWAuthError)):
+                        raise ex
+
+                    # Always log the error
+                    _LOGGER.error("Unable to get details for vehicle %s - (%s) %s", vehicle.vin, type(ex).__name__, ex)
+
+                    # If all vehicles fail, we want to raise an exception
+                    if error_count == len(self.vehicles):
+                        raise ex
 
     def add_vehicle(
         self,

bimmer_connected/tests/__init__.py

@@ -35,11 +35,16 @@
 REMOTE_SERVICE_RESPONSE_EVENTPOSITION = RESPONSE_DIR / "remote_services" / "eadrax_service_eventposition.json"
 
 
-def get_fingerprint_count() -> int:
+def get_fingerprint_state_count() -> int:
     """Return number of loaded vehicles."""
     return sum([len(vehicles) for vehicles in ALL_VEHICLES.values()])
 
 
+def get_fingerprint_charging_settings_count() -> int:
+    """Return number of loaded vehicles."""
+    return len(ALL_CHARGING_SETTINGS)
+
+
 def load_response(path: Union[Path, str]) -> Any:
     """Load a stored response."""
     with open(path, "rb") as file:

bimmer_connected/tests/common.py

@@ -111,7 +111,7 @@ def add_vehicle_routes(self) -> None:
         """Add routes for vehicle requests."""
 
         self.get("/eadrax-vcs/v4/vehicles").mock(side_effect=self.vehicles_sideeffect)
-        self.get("/eadrax-vcs/v4/vehicles/state").mock(side_effect=self.vehicle_state_sideeffect)
+        self.get("/eadrax-vcs/v4/vehicles/state", name="state").mock(side_effect=self.vehicle_state_sideeffect)
         self.get("/eadrax-crccs/v2/vehicles").mock(side_effect=self.vehicle_charging_settings_sideeffect)
 
     def add_remote_service_routes(self) -> None:

bimmer_connected/tests/conftest.py

@@ -1,11 +1,13 @@
 """Fixtures for BMW tests."""
-from typing import Generator, Optional
+from collections import deque
+from typing import Deque, Generator, Optional
 
 import pytest
 import respx
 
 from bimmer_connected.account import MyBMWAccount
 from bimmer_connected.const import Regions
+from bimmer_connected.models import AnonymizedResponse
 
 from . import (
     ALL_CHARGING_SETTINGS,
@@ -31,6 +33,14 @@ def bmw_fixture(request: pytest.FixtureRequest) -> Generator[respx.MockRouter, N
         yield router
 
 
+@pytest.fixture
+def bmw_log_all_responses(monkeypatch: pytest.MonkeyPatch):
+    """Increase the length of the response store to log all responses."""
+    temp_store: Deque[AnonymizedResponse] = deque(maxlen=100)
+    monkeypatch.setattr("bimmer_connected.api.client.RESPONSE_STORE", temp_store)
+    monkeypatch.setattr("bimmer_connected.account.RESPONSE_STORE", temp_store)
+
+
 async def prepare_account_with_vehicles(region: Optional[Regions] = None, metric: bool = True):
     """Initialize account and get vehicles."""
     account = MyBMWAccount(TEST_USERNAME, TEST_PASSWORD, region or TEST_REGION, use_metric_units=metric)

bimmer_connected/tests/test_account.py

@@ -23,7 +23,9 @@
     TEST_REGION_STRING,
     TEST_USERNAME,
     VIN_G26,
-    get_fingerprint_count,
+    VIN_I20,
+    get_fingerprint_charging_settings_count,
+    get_fingerprint_state_count,
     load_response,
 )
 from .conftest import prepare_account_with_vehicles
@@ -175,7 +177,7 @@ async def test_vehicles(bmw_fixture: respx.Router):
     await account.get_vehicles()
 
     assert account.config.authentication.access_token is not None
-    assert get_fingerprint_count() == len(account.vehicles)
+    assert get_fingerprint_state_count() == len(account.vehicles)
 
     vehicle = account.get_vehicle(VIN_G26)
     assert vehicle is not None
@@ -196,15 +198,15 @@ async def test_vehicle_init(bmw_fixture: respx.Router):
 
         # First call on init
         await account.get_vehicles()
-        assert len(account.vehicles) == get_fingerprint_count()
+        assert len(account.vehicles) == get_fingerprint_state_count()
 
         # No call to _init_vehicles()
         await account.get_vehicles()
-        assert len(account.vehicles) == get_fingerprint_count()
+        assert len(account.vehicles) == get_fingerprint_state_count()
 
         # Second, forced call _init_vehicles()
         await account.get_vehicles(force_init=True)
-        assert len(account.vehicles) == get_fingerprint_count()
+        assert len(account.vehicles) == get_fingerprint_state_count()
 
         assert mock_listener.call_count == 2
 
@@ -254,23 +256,42 @@ async def test_vehicle_search_case(bmw_fixture: respx.Router):
 
 
 @pytest.mark.asyncio
-async def test_get_fingerprints(bmw_fixture: respx.Router):
+async def test_get_fingerprints(monkeypatch: pytest.MonkeyPatch, bmw_fixture: respx.Router, bmw_log_all_responses):
     """Test getting fingerprints."""
+
+    # Prepare Number of good responses (vehicle states, charging settings per vehicle)
+    # and 2x vehicle list
+    json_count = get_fingerprint_state_count() + get_fingerprint_charging_settings_count() + 2
+
     account = MyBMWAccount(TEST_USERNAME, TEST_PASSWORD, TEST_REGION, log_responses=True)
     await account.get_vehicles()
 
-    bmw_fixture.get("/eadrax-vcs/v4/vehicles/state").respond(
+    # This should have been successful
+    filenames = [Path(f.filename) for f in account.get_stored_responses()]
+    json_files = [f for f in filenames if f.suffix == ".json"]
+    txt_files = [f for f in filenames if f.suffix == ".txt"]
+
+    assert len(json_files) == json_count  # all good
+    assert len(txt_files) == 0  # no errors
+
+    # Now we simulate an error for a single vehicle
+    # We need to remove the existing state route first and add it back later as otherwise our error call is never
+    # matched (respx matches by order of routes and we don't replace the existing one)
+    state_route = bmw_fixture.routes.pop("state")
+    bmw_fixture.get("/eadrax-vcs/v4/vehicles/state", headers={"bmw-vin": VIN_G26}).respond(
         500, text=load_response(RESPONSE_DIR / "auth" / "auth_error_internal_error.txt")
     )
-    with pytest.raises(MyBMWAPIError):
-        await account.get_vehicles()
+    bmw_fixture.routes.add(state_route, "state")
+
+    account = MyBMWAccount(TEST_USERNAME, TEST_PASSWORD, TEST_REGION, log_responses=True)
+    await account.get_vehicles()
 
     filenames = [Path(f.filename) for f in account.get_stored_responses()]
     json_files = [f for f in filenames if f.suffix == ".json"]
     txt_files = [f for f in filenames if f.suffix == ".txt"]
 
-    assert len(json_files) == (get_fingerprint_count() + 1)
-    assert len(txt_files) == 1
+    assert len(json_files) == json_count - 2  # missing on 1 state and 1 charging setting
+    assert len(txt_files) == 1  # error message from state, charging setting was not loaded anymore
 
 
 @pytest.mark.asyncio
@@ -473,6 +494,51 @@ async def test_403_quota_exceeded_vehicles_usa(caplog, bmw_fixture: respx.Router
     assert len(log_quota) == 1
 
 
+@pytest.mark.asyncio
+async def test_incomplete_vehicle_details(caplog, bmw_fixture: respx.Router):
+    """Test incorrect responses for vehicle details."""
+    account = MyBMWAccount(TEST_USERNAME, TEST_PASSWORD, TEST_REGION)
+    # get vehicles once
+    await account.get_vehicles()
+
+    # We need to remove the existing state route first and add it back later as otherwise our error call is never
+    # matched (respx matches by order of routes and we don't replace the existing one)
+    state_route = bmw_fixture.routes.pop("state")
+    # JSON, but error
+    bmw_fixture.get("/eadrax-vcs/v4/vehicles/state", headers={"bmw-vin": VIN_I20}).respond(
+        500, json={"statusCode": 500, "message": "Something is broken."}
+    )
+    # No JSON
+    bmw_fixture.get("/eadrax-vcs/v4/vehicles/state", headers={"bmw-vin": VIN_G26}).respond(
+        500, text=load_response(RESPONSE_DIR / "auth" / "auth_error_internal_error.txt")
+    )
+    bmw_fixture.routes.add(state_route, "state")
+
+    await account.get_vehicles()
+
+    log_error = [r for r in caplog.records if "Unable to get details" in r.message]
+    assert len(log_error) == 2
+
+
+@pytest.mark.asyncio
+async def test_no_vehicle_details(caplog, bmw_fixture: respx.Router):
+    """Test raising an exception if no responses for vehicle details are received."""
+    account = MyBMWAccount(TEST_USERNAME, TEST_PASSWORD, TEST_REGION)
+    await account.get_vehicles()
+
+    bmw_fixture.get("/eadrax-vcs/v4/vehicles/state").mock(
+        return_value=httpx.Response(
+            500,
+            json={"statusCode": 500, "message": "Something is broken."},
+        )
+    )
+    with pytest.raises(MyBMWAPIError):
+        await account.get_vehicles()
+
+    log_error = [r for r in caplog.records if "Unable to get details" in r.message]
+    assert len(log_error) == get_fingerprint_state_count()
+
+
 @pytest.mark.asyncio
 async def test_client_async_only(bmw_fixture: respx.Router):
     """Test that the Authentication providers only work async."""