raise OAuthRefreshException when refresh token fails#102
Open
simonc56 wants to merge 3 commits into
Open
Conversation
WalkthroughThis PR enhances OAuth token refresh failure handling by rewriting ChangesOAuth Token Refresh Error Handling
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 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
Comment on lines
+79
to
+81
| self.data = self._load_data() | ||
| self._error = error or self.data.get("error") | ||
| self._error_description = error_description or self.data.get("error_description") |
There was a problem hiding this comment.
Guard against non-object JSON payloads in OAuthRefreshException.
Line 80 and Line 81 assume self.data is a dict. If response.json() returns a JSON array/string/number, this path raises AttributeError and hides the actual refresh failure context.
Proposed fix
def _load_data(self):
if self.response is None:
return {}
try:
- return self.response.json()
- except (AttributeError, ValueError):
+ data = self.response.json()
+ return data if isinstance(data, dict) else {}
+ except (AttributeError, ValueError, TypeError):
return {}Also applies to: 83-90
🤖 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/errors.py` around lines 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.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
No description provided.