Refactor: Avoid duplicate calls to e.response.json()#93
Conversation
WalkthroughConsolidates JSON error parsing in trakt/api.py’s token refresh error handler by parsing once into a variable and reusing it; import formatting switched to multiline. No behavioral changes to public APIs. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant C as Client
participant A as TraktAPI
participant O as OAuth Server
C->>A: refresh_token()
A->>O: POST /token (refresh_token)
alt Success
O-->>A: 200 OK (new tokens)
A-->>C: return tokens
else Error
O-->>A: Error response
A->>A: Parse JSON once into data
A->>A: Extract error / description from data (fallback if JSON invalid)
A-->>C: Propagate/handle error (logged)
end
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Poem
✨ Finishing Touches
🧪 Generate unit tests
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: 0
🧹 Nitpick comments (5)
trakt/api.py (5)
260-263: Good: single JSON parse. Also avoid shadowing and broaden decode except.Parsing once is correct. Minor: this reuses the name data, which already holds the request payload (Lines 246–252). Rename to avoid shadowing, and catch ValueError as well (some JSON decoders raise it).
Apply:
- data = { + payload = { 'client_id': self.config.CLIENT_ID, 'client_secret': self.config.CLIENT_SECRET, 'refresh_token': self.config.OAUTH_REFRESH, 'redirect_uri': self.REDIRECT_URI, 'grant_type': 'refresh_token' } @@ - response = self.client.post('oauth/token', data) + response = self.client.post('oauth/token', payload) @@ - data = e.response.json() - error = data.get("error") - error_description = data.get("error_description") - except JSONDecodeError: + err_data = e.response.json() + error = err_data.get("error") + error_description = err_data.get("error_description") + except (JSONDecodeError, ValueError): error = "Invalid JSON response" error_description = e.response.textAlso applies to: 246-252, 255-255
129-133: Avoid sending 'null' bodies; prefer requests' json= paramCurrent code will send the literal "null" when data is None. Use json= only when payload exists; otherwise omit the body.
- if method == 'get': # GETs need to pass data as params, not body - response = self.session.request(method, url, headers=self.headers, auth=self.auth, timeout=self.timeout, params=data) - else: - response = self.session.request(method, url, headers=self.headers, auth=self.auth, timeout=self.timeout, data=json.dumps(data)) + if method == 'get': # GETs need to pass data as params, not body + response = self.session.request(method, url, headers=self.headers, auth=self.auth, timeout=self.timeout, params=data) + else: + kwargs = dict(headers=self.headers, auth=self.auth, timeout=self.timeout) + if data is not None: + kwargs["json"] = data + response = self.session.request(method, url, **kwargs)
141-145: Propagate the original response in BadResponseException and use response.json()Using response.json() lets requests handle encoding; include the response in the exception for better diagnostics.
- try: - return json.loads(response.content.decode('UTF-8', 'ignore')) - except JSONDecodeError as e: - raise BadResponseException(f"Unable to parse JSON: {e}") + try: + return response.json() + except (JSONDecodeError, ValueError) as e: + raise BadResponseException(response=response, details=f"Unable to parse JSON: {e}")
151-160: Use cached_property instead of @Property + lru_cachecached_property is simpler and avoids holding references to every instance as lru_cache keys.
-from functools import lru_cache +from functools import cached_property @@ - @property - @lru_cache(maxsize=None) - def error_map(self): + @cached_property + def error_map(self):
275-279: Defensive check when computing OAUTH_EXPIRES_ATIf created_at or expires_in are missing/None, the addition will raise TypeError. Guard and log.
- 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"), - ) + created_at = response.get("created_at") + expires_in = response.get("expires_in") + if not isinstance(created_at, int) or not isinstance(expires_in, int): + self.logger.error("Invalid token response: created_at=%r expires_in=%r", created_at, expires_in) + return + self.config.update( + OAUTH_TOKEN=response.get("access_token"), + OAUTH_REFRESH=response.get("refresh_token"), + OAUTH_EXPIRES_AT=created_at + expires_in, + )
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
trakt/api.py(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
trakt/api.py (1)
trakt/errors.py (5)
BadRequestException(57-61)BadResponseException(46-54)OAuthException(64-68)error(77-78)error_description(81-82)
🔇 Additional comments (1)
trakt/api.py (1)
13-14: Import formatting nit — OK to keepParenthesized import improves readability; no semantic change.
1 participant
Code cleanup after contributor changes: ee7dd4f, d860d01