raise OAuthRefreshException when refresh token fails#102
Conversation
|
Warning Review limit reached
More reviews will be available in 55 minutes and 9 seconds. Learn how PR review limits work. Your organization has run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (4)
WalkthroughRewrites OAuthRefreshException for safe JSON handling, adds retry-and-structured-exception logic to TokenAuth.refresh_token (with expiry validation and config persistence), and adds tests verifying exception detail extraction and refresh-failure behavior. ChangesOAuth Token Refresh Error Handling
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
trakt/api.py (1)
284-295:⚠️ Potential issue | 🟠 Major | ⚡ Quick winValidate required refresh fields before computing
OAUTH_EXPIRES_AT.Line 284 only guards
None. An empty dict or missingcreated_at/expires_inreaches Line 294 and can raiseTypeError, escaping the dedicatedOAuthRefreshExceptionflow.Proposed fix
if response is None: self.OAUTH_TOKEN_VALID = False raise OAuthRefreshException( error='empty_refresh_response', error_description='OAuth token refresh completed without a response payload.', ) + + required = ("access_token", "refresh_token", "created_at", "expires_in") + if any(response.get(k) is None for k in required): + self.OAUTH_TOKEN_VALID = False + raise OAuthRefreshException( + error='invalid_refresh_payload', + error_description='OAuth token refresh response is missing required fields.', + ) + + try: + expires_at = int(response.get("created_at")) + int(response.get("expires_in")) + except (TypeError, ValueError): + self.OAUTH_TOKEN_VALID = False + raise OAuthRefreshException( + error='invalid_refresh_payload', + error_description='OAuth token refresh response contains invalid expiry values.', + ) self.config.update( OAUTH_TOKEN=response.get("access_token"), OAUTH_REFRESH=response.get("refresh_token"), - OAUTH_EXPIRES_AT=response.get("created_at") + response.get("expires_in"), + OAUTH_EXPIRES_AT=expires_at, )🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@trakt/api.py` around lines 284 - 295, The code only checks response is not None but then assumes response contains "created_at" and "expires_in"; update the refresh handling in the method that sets self.config (use the same response variable and the config.update block) to validate required keys ("access_token", "refresh_token", "created_at", "expires_in") and their types before computing OAUTH_EXPIRES_AT; if any are missing or invalid, set self.OAUTH_TOKEN_VALID = False and raise OAuthRefreshException with an informative error and error_description instead of letting a TypeError propagate, otherwise compute OAUTH_EXPIRES_AT as created_at + expires_in and proceed to update OAUTH_TOKEN and OAUTH_REFRESH.
🧹 Nitpick comments (1)
tests/test_errors.py (1)
33-46: ⚡ Quick winAdd a fallback-path test for non-JSON refresh error responses.
This test validates the happy path, but not the safe-parse fallback. Add one case where
response.json()fails to ensure the exception still formats safely.Proposed test addition
+def test_oauth_refresh_exception_handles_non_json_response(): + response = Mock() + response.json.side_effect = ValueError("invalid json") + + texc = OAuthRefreshException(response=response) + + assert texc.http_code == 401 + assert texc.error is None + assert texc.error_description is None + assert str(texc) == 'Unauthorized - OAuth token refresh failed'🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/test_errors.py` around lines 33 - 46, Add a new unit test that verifies OAuthRefreshException correctly handles non-JSON error responses by making the mocked response.json() raise (e.g., ValueError or json.JSONDecodeError) and asserting the exception still sets a safe http_code (401), uses fallback/default values for error and error_description (or empty strings), and produces a safe str() message like "Unauthorized - OAuth token refresh failed" without crashing; target the OAuthRefreshException constructor and __str__ behavior in tests/test_errors.py to ensure the safe-parse fallback path is covered.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@trakt/errors.py`:
- Around line 79-81: OAuthRefreshException assumes self.data is a dict and calls
.get, which will raise AttributeError for non-object JSON payloads; change the
initialization in the constructor that calls self._load_data() to defensively
coerce non-dict JSON into an empty dict (e.g., data = self._load_data(); if not
isinstance(data, dict): data = {}), then use that safe `data` for assigning
self._error, self._error_description and any other attributes set via data.get
in the block around OAuthRefreshException (lines setting
_error/_error_description and the subsequent .get uses between ~80-90), so all
.get calls operate on a dict fallback rather than potentially non-object JSON.
---
Outside diff comments:
In `@trakt/api.py`:
- Around line 284-295: The code only checks response is not None but then
assumes response contains "created_at" and "expires_in"; update the refresh
handling in the method that sets self.config (use the same response variable and
the config.update block) to validate required keys ("access_token",
"refresh_token", "created_at", "expires_in") and their types before computing
OAUTH_EXPIRES_AT; if any are missing or invalid, set self.OAUTH_TOKEN_VALID =
False and raise OAuthRefreshException with an informative error and
error_description instead of letting a TypeError propagate, otherwise compute
OAUTH_EXPIRES_AT as created_at + expires_in and proceed to update OAUTH_TOKEN
and OAUTH_REFRESH.
---
Nitpick comments:
In `@tests/test_errors.py`:
- Around line 33-46: Add a new unit test that verifies OAuthRefreshException
correctly handles non-JSON error responses by making the mocked response.json()
raise (e.g., ValueError or json.JSONDecodeError) and asserting the exception
still sets a safe http_code (401), uses fallback/default values for error and
error_description (or empty strings), and produces a safe str() message like
"Unauthorized - OAuth token refresh failed" without crashing; target the
OAuthRefreshException constructor and __str__ behavior in tests/test_errors.py
to ensure the safe-parse fallback path is covered.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 51991ce0-2bf3-4dad-af96-fd3e64806234
📒 Files selected for processing (4)
tests/test_api.pytests/test_errors.pytrakt/api.pytrakt/errors.py
|
@simonc56 please fill pr body, add references. also resolve discussions with coderabbit before requesting a review. currently the pr contains too many changes i don't think i want to waste my personal time reviewing such llm produced noise. |
|
Actionable comments posted: 0 |
simonc56
force-pushed
the
feat/raise-oauth-refresh-exception
branch
from
c577875 to
37c4a69
Compare
It contains only changes needed to fix issue described in first post. |
2 participants
OAuthRefreshExceptionexists in codebase but is never raised.This PR makes the
OAuthRefreshExceptionbe raised when refreshing OAuth token fails, instead of silently continuing with an expired token.Needed in :