Repository: bimmerconnected/bimmer_connected
Pull request #586 (2023-11-30 21:38:36)

 Fix handling 401 after 429 status code - reauthenticate last
## Proposed change
<!--
  Describe the big picture of your changes here to communicate to the
  maintainers why we should accept this pull request. If it fixes a bug
  or resolves a feature request, be sure to link to that issue in the
  additional information section.
-->
Overlooked at edge case in #585: Exactly three 429 followed by a 401 will still raise MyBMWAuthError, as the existing loop would be at an end.

This PR changes the following:
- Loop only for 429 and check for 401 afterwards (this also means that 429 > 401 > 429 will throw a MyBMWQuotaError, but this doesn't trigger reauthentication in HA and will recover itself)
- Minor optimizations
  - If a request is successful, we don't even check for error handling
  - we're running the loop only if the error is a 429

## Type of change
- [x] Bugfix (non-breaking change which fixes an issue)
- [ ] New feature (which adds functionality to this library)
- [ ] Breaking change (fix/feature causing existing functionality to break)
- [ ] Code quality improvements to existing code or addition of tests

## Additional information
<!--
  Details are important, and help maintainers processing your PR.
  Please be sure to fill out additional details, if applicable.
-->

- This PR fixes or closes issue: fixes #
- This PR is related to issue: https://github.com/home-assistant/core/issues/104555

## Checklist
<!--
  Put an `x` in the boxes that apply. You can also fill these out after
  creating the PR. If you're unsure about any of them, don't hesitate to ask.
  We're here to help! This is simply a reminder of what we are going to look
  for before merging your code.
-->

- [x] The code change is tested and works locally.
- [x] Tests have been added to verify that the new code works.



Patch:
diff --git a/bimmer_connected/api/authentication.py b/bimmer_connected/api/authentication.py
index b300e8a..ceca479 100644
--- a/bimmer_connected/api/authentication.py
+++ b/bimmer_connected/api/authentication.py
@@ -84,30 +84,34 @@ class MyBMWAuthentication(httpx.Auth):
 
         # Try getting a response
         response: httpx.Response = (yield request)
-        await response.aread()
-        prev_response_code: int = 0
 
-        # Retry 3 times on 401 or 429
-        for _ in range(3):
-            # Handle "classic" 401 Unauthorized and try getting a new token
-            # We don't want to call the auth endpoint too many times, so we only do it once per 401
-            if response.status_code == 401 and response.status_code != prev_response_code:
-                prev_response_code = response.status_code
-                async with self.login_lock:
-                    _LOGGER.debug("Received unauthorized response, refreshing token.")
-                    await self.login()
-                request.headers["authorization"] = f"Bearer {self.access_token}"
-                request.headers["bmw-session-id"] = self.session_id
-                response = yield request
+        # return directly if first response was successful
+        if response.is_success:
+            return
 
+        await response.aread()
+
+        # First check against 429 Too Many Requests and 403 Quota Exceeded
+        retry_count = 0
+        while (
+            response.status_code == 429 or (response.status_code == 403 and "quota" in response.text.lower())
+        ) and retry_count < 3:
             # Quota errors can either be 429 Too Many Requests or 403 Quota Exceeded (instead of 403 Forbidden)
-            elif response.status_code == 429 or (response.status_code == 403 and "quota" in response.text.lower()):
-                prev_response_code = response.status_code
-                await response.aread()
-                wait_time = get_retry_wait_time(response)
-                _LOGGER.debug("Sleeping %s seconds due to 429 Too Many Requests", wait_time)
-                await asyncio.sleep(wait_time)
-                response = yield request
+            wait_time = get_retry_wait_time(response)
+            _LOGGER.debug("Sleeping %s seconds due to 429 Too Many Requests", wait_time)
+            await asyncio.sleep(wait_time)
+            response = yield request
+            await response.aread()
+            retry_count += 1
+
+        # Handle 401 Unauthorized and try getting a new token
+        if response.status_code == 401:
+            async with self.login_lock:
+                _LOGGER.debug("Received unauthorized response, refreshing token.")
+                await self.login()
+            request.headers["authorization"] = f"Bearer {self.access_token}"
+            request.headers["bmw-session-id"] = self.session_id
+            response = yield request
 
         # Raise if request still was not successful
         try:


Test patch:
diff --git a/bimmer_connected/tests/test_account.py b/bimmer_connected/tests/test_account.py
index 759961f..609d616 100644
--- a/bimmer_connected/tests/test_account.py
+++ b/bimmer_connected/tests/test_account.py
@@ -508,6 +508,9 @@ async def test_429_retry_with_login_raise_vehicles(bmw_fixture: respx.Router):
             httpx.Response(401),
             httpx.Response(429, json=json_429),
             httpx.Response(429, json=json_429),
+            httpx.Response(429, json=json_429),
+            httpx.Response(429, json=json_429),
+            httpx.Response(429, json=json_429),
         ]
     )
 
@@ -533,6 +536,52 @@ async def test_multiple_401(bmw_fixture: respx.Router):
             await account.get_vehicles()
 
 
+@pytest.mark.asyncio
+async def test_401_after_429_ok(bmw_fixture: respx.Router):
+    """Test the error handling, when a 401 is received after exactly 3 429."""
+    account = MyBMWAccount(TEST_USERNAME, TEST_PASSWORD, TEST_REGION)
+    await account.get_vehicles()
+
+    json_429 = {"statusCode": 429, "message": "Rate limit is exceeded. Try again in 2 seconds."}
+
+    # Recover after 3 429 and 1 401
+    bmw_fixture.get("/eadrax-vcs/v4/vehicles").mock(
+        side_effect=[
+            httpx.Response(429, json=json_429),
+            httpx.Response(429, json=json_429),
+            httpx.Response(429, json=json_429),
+            httpx.Response(401),
+            *[httpx.Response(200, json={})] * 100,  # Just simulate OK responses from now on
+        ]
+    )
+    with mock.patch("asyncio.sleep", new_callable=mock.AsyncMock):
+        await account.get_vehicles()
+    assert len(account.vehicles) == get_fingerprint_state_count()
+
+
+@pytest.mark.asyncio
+async def test_401_after_429_fail(bmw_fixture: respx.Router):
+    """Test the error handling, when a 401 is received after exactly 3 429."""
+    account = MyBMWAccount(TEST_USERNAME, TEST_PASSWORD, TEST_REGION)
+
+    json_429 = {"statusCode": 429, "message": "Rate limit is exceeded. Try again in 2 seconds."}
+
+    # Fail after 3 429 and 1 401 with another 429
+    bmw_fixture.get("/eadrax-vcs/v4/vehicles").mock(
+        side_effect=[
+            httpx.Response(429, json=json_429),
+            httpx.Response(429, json=json_429),
+            httpx.Response(429, json=json_429),
+            httpx.Response(401),
+            httpx.Response(429, json=json_429),
+        ]
+    )
+
+    with mock.patch("asyncio.sleep", new_callable=mock.AsyncMock):
+        with pytest.raises(MyBMWQuotaError):
+            await account.get_vehicles()
+
+
 @pytest.mark.asyncio
 async def test_403_quota_exceeded_vehicles_usa(caplog, bmw_fixture: respx.Router):
     """Test 403 quota issues for vehicle state and fail if it happens too often."""