Refactor: Add api client class #51
Conversation
WalkthroughThis pull request introduces a comprehensive refactoring of the Trakt library's authentication and API interaction mechanisms. The changes include the introduction of new authentication adapters, an HTTP client, configuration management, and decorators for HTTP methods. The modifications aim to improve the library's structure, enhance authentication flows, and provide more robust error handling. The version has been incremented to 4.0.0.dev0, signaling a significant architectural update. Changes
Sequence DiagramsequenceDiagram
participant User
participant AuthInit as init_auth()
participant Adapter as AuthAdapter
participant HttpClient
participant TraktAPI
User->>AuthInit: Specify auth method
AuthInit->>Adapter: Initialize adapter
Adapter->>TraktAPI: Request authorization
TraktAPI-->>Adapter: Return authorization details
Adapter->>User: Prompt for interaction
User->>Adapter: Provide authorization info
Adapter->>HttpClient: Request access token
HttpClient->>TraktAPI: Exchange credentials
TraktAPI-->>HttpClient: Return tokens
HttpClient->>Adapter: Tokens received
Adapter->>AuthInit: Update configuration
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (24)
trakt/auth/oauth.py (2)
10-21: Ensure consistency in constructor docstring parametersWhile the docstring explains
usernameandoauth_cb, it doesn't explicitly documentclientorconfig. Including brief parameter descriptions for all arguments can help maintain clarity, especially for contributors new to this file.
48-60: Consider adding minimal logging or error handling for user inputPrompting the user for the OAuth PIN is suitable for command-line flows, but consider adding a simple check for empty input or a canceled prompt to give better feedback or default behavior when encountering unexpected user input.
trakt/core.py (5)
64-68: Add error handling or fallback for invalidAUTH_METHODThe
initfunction callsinit_auth(AUTH_METHOD, ...)but does not handle any potential error ifAUTH_METHODis invalid or unsupported. Consider adding logic to raise a descriptive exception or fallback to a default method to improve error resilience.
71-82: Evaluate the impact of@lru_cache(maxsize=None)onconfigUsing
@lru_cache(maxsize=None)on a function returning a mutable configuration object can lead to unbounded caching in long-running processes. This is acceptable if usage is small or ephemeral, but it may be worth exploring a bounded cache size or an explicit cache invalidation step.
85-93: Evaluate the impact of@lru_cache(maxsize=None)onapiSimilar to
config(), the@lru_cache(maxsize=None)decoratesapi(), potentially caching theHttpClientobject indefinitely. If your use case requires repeated re-initialization, you may need a more explicit cache policy to avoid unintended memory usage.
126-130: Maintain explanatory docstrings for backward compatibility wrappersThe
deletefunction referencestrakt.decorators.delete(f)and provides backward compatibility for version 3.x. While this is straightforward, consider adding a short docstring or comment indicating the purpose of the wrapper to guide new contributors.
132-135: Consolidate repetitive decorator wrappersThese lines wrap
get,post, andputsimilarly. If these wrappers remain identical except for the decorator they import, consider consolidating them into a shared internal function to reduce duplication.Also applies to: 137-140, 142-144
trakt/auth/device.py (3)
10-17: Avoid overshadowing local variables
responseis reused as both the dictionary fromget_device_codeand, within theexceptclause, it may refer to the HTTP response object. This can lead to confusion or errors ifresponseis misinterpreted. Consider renaming or scoping variables more distinctly.
63-82: Validate user awareness foruser_codeandverification_urlThe
get_device_codemethod only prints the user code and verification URL to the console. This works well for a CLI scenario, but consider logging or capturing further diagnostic details in case the user later reports a problem with device-based authentication.
83-115: Enhance error clarity while polling for tokensIf
get_device_tokenraises an exception, only generic messages fromerror_messagesare printed. Consider offering more specific context such as the final HTTP response to help diagnose issues more rapidly.trakt/api.py (3)
47-72: Elevate API exception chaining for clarityWithin
decode_responseat line 78, an exception is raised usingraise BadResponseException(...)in anexcept JSONDecodeError as e:block. Consider chaining the original exception withraise BadResponseException(...) from eto aid debugging by preserving the original stack trace.
73-79: Potential memory leak via@lru_cacheon methodsThe
@lru_cache(maxsize=None)annotation onerror_mapcan sometimes lead to increased memory usage over time, especially if the set of error codes or scenario contexts grows. Evaluate if a standard dictionary or a bounded cache would suffice.Also applies to: 80-95
🧰 Tools
🪛 Ruff (0.8.2)
78-78: Within an
exceptclause, raise exceptions withraise ... from errorraise ... from Noneto distinguish them from errors in exception handling(B904)
124-137: Consider concurrency impact ofvalidate_token()orrefresh_token()When multiple threads or processes might share the same
configobject and token state, race conditions can occur. If concurrency is expected, add synchronization or a robust check to avoid race conditions during token refresh.trakt/auth/pin.py (2)
11-19: Clarify parameter documentation forpinWhile the docstring references the
pinparameter, explaining that it is optional, it would be helpful to explicitly note howAPPLICATION_IDfactors into authentication, especially if a user attempts to call this adapter without either a validpinorAPPLICATION_ID.
20-45: Consider an alternative tosys.exit(1)in library codeFor library usage in non-interactive contexts, calling
sys.exit(1)can be disruptive. Instead, raising a descriptive exception enables the calling application to gracefully handle or report the error.trakt/config.py (4)
23-24: Clarify return type forhave_refresh_token.Currently, it returns the value of
self.OAUTH_EXPIRES_AT and self.OAUTH_REFRESH, which evaluates to the latter's value orNone. Consider explicitly returning a boolean for clarity—e.g.return bool(self.OAUTH_EXPIRES_AT and self.OAUTH_REFRESH)—to avoid potential confusion.
26-31: Use safer built-in functiongetattrinstead of__getattribute__.While
__getattribute__works,getattr(self, name, default)is more common and less error-prone.- return self.__getattribute__(name) + return getattr(self, name, default)
41-44: Replacefor key in self.__annotations__.keys()withfor key in self.__annotations__.As per the static analysis hint, iterating over
.keys()is unnecessary.- for key in self.__annotations__.keys(): + for key in self.__annotations__:🧰 Tools
🪛 Ruff (0.8.2)
43-43: Use
key in dictinstead ofkey in dict.keys()Remove
.keys()(SIM118)
70-72: Consider usingindent=2for JSON dump.Using indentation can make the generated config more human-readable without affecting functionality.
- json.dump(config, config_file) + json.dump(config, config_file, indent=2)trakt/auth/__init__.py (3)
11-14: Add docstring forpin_auth.Each authentication function would benefit from a clear docstring or a short comment describing return values and error handling.
29-45: Validate user input inget_client_infofunction.Currently, the
input()calls run without checks. Consider validating that the user does not enter empty strings for critical fields, or handle potential exceptions if input is not provided.
68-69: Clarify interactive vs. non-interactive usage.
get_client_infousesinput(). If you plan to run this library in headless environments (e.g., CI/CD), consider adding a non-interactive fallback or raise an error.trakt/decorators.py (1)
72-93: Encourage consistent docstring style for all decorators.While each decorator (
post,put, etc.) has docstrings, ensure consistent parameter/return descriptions and formatting across them for uniformity.tests/conftest.py (1)
29-46: Avoid printing “No mock” in production tests.Currently, missing mocks trigger a print statement rather than an exception, which might mask real test coverage issues. Consider using warnings or raising an error to ensure all expected endpoints have test coverage.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
tests/conftest.py(1 hunks)trakt/__version__.py(1 hunks)trakt/api.py(1 hunks)trakt/auth/__init__.py(1 hunks)trakt/auth/base.py(1 hunks)trakt/auth/device.py(1 hunks)trakt/auth/oauth.py(1 hunks)trakt/auth/pin.py(1 hunks)trakt/config.py(1 hunks)trakt/core.py(3 hunks)trakt/decorators.py(1 hunks)
✅ Files skipped from review due to trivial changes (2)
- trakt/version.py
- trakt/auth/base.py
🧰 Additional context used
🪛 Ruff (0.8.2)
trakt/api.py
78-78: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
85-85: Use of functools.lru_cache or functools.cache on methods can lead to memory leaks
(B019)
trakt/config.py
43-43: Use key in dict instead of key in dict.keys()
Remove .keys()
(SIM118)
58-58: Use key in dict instead of key in dict.keys()
Remove .keys()
(SIM118)
🔇 Additional comments (11)
trakt/auth/oauth.py (1)
22-47:⚠️ Potential issueVerify definition of
REDIRECT_URIattributeThe
authenticatemethod referencesself.REDIRECT_URIat line 32, but this property does not appear to be defined inOAuthAdapteror its base class in the code snippet provided. IfREDIRECT_URIis intended to come from a shared base or an external import, ensure that the code references the correct source to preventAttributeError.Run the following script to locate
REDIRECT_URIreferences in the repository and confirm they are declared or imported:✅ Verification successful
REDIRECT_URIis properly defined in the base classThe
REDIRECT_URIattribute is correctly defined intrakt/auth/base.pyas a class variable in theBaseAdapterclass with the value'urn:ietf:wg:oauth:2.0:oob'. SinceOAuthAdapterinherits fromBaseAdapter, theself.REDIRECT_URIreference in theauthenticatemethod is valid and will not raise anAttributeError.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash rg --heading --context 5 "REDIRECT_URI"Length of output: 2098
trakt/api.py (1)
97-123: Confirm offset between local time and token expiration calculationThe
TokenAuthclass usesdatetime.now(tz=timezone.utc)and compares it against the timestamp fromdatetime.fromtimestamp(self.config.OAUTH_EXPIRES_AT, tz=timezone.utc). Ensure that token expiration times are fully in sync with Trakt’s server times and that no edge case arises for time drift.trakt/config.py (2)
11-16: Well-structured dataclass usage.Good job leveraging
dataclassforAuthConfigto define optional authentication attributes. This structure improves clarity and enforces type hints.
48-54: Check existence first to prevent accidental overrides.You're correctly checking
(self.CLIENT_ID and self.CLIENT_SECRET) or not exists(self.config_path)to early-return. Double-check that you indeed want to skip loading config if eitherCLIENT_IDorCLIENT_SECRETis set. This might bypass loading additional fields from the config file in some scenarios.trakt/auth/__init__.py (2)
48-60: Check fallback for unknown method.Defaulting to
PIN_AUTHifmethodis missing or invalid is convenient, but ensure this is intentional. Consider logging a warning or raising an error if the method is not recognized.
74-75: Confirm config security before storing.Ensure that storing credentials locally is acceptable from a security perspective. Provide a warning or documentation for users about safeguarding the config file.
trakt/decorators.py (4)
10-29: Clean separation of concerns with_get_first.The
_get_firsthelper function provides a neat mechanism for extracting the first yielded value. This approach keeps your decorators clean and focused on request/response logic.
41-53: Graceful handling of cached data or partial yields inget.Your fallback for non-tuple yields helps handle instances where the generator yields only cached data or no URL. It's a good safeguard for partial yields without forcing strict structure on the generator.
57-70: Explicitly handle missing responses for thedeletedecorator.Currently, you delete unconditionally after fetching the URL. If there's a scenario where the generator yields a non-string or no URL, it might lead to runtime issues. Consider a similar fallback or guard.
95-115: Kudos for uniform generator-based HTTP patterns.The generator approach for PUT requests closely mirrors the patterns for POST, DELETE, etc. The consistent design across decorators improves maintainability.
tests/conftest.py (1)
52-52: Verify test collisions with a singleMockCoreinstance.As
MockCoreis instantiated once, ensure that multiple tests concurrently accessing or modifying it do not cause unintended side effects. If concurrency is possible, consider a fresh instance per test.
| def authenticate(self): | ||
| """Process for authenticating using device authentication. | ||
|
|
||
| The function will attempt getting the device_id, and provide | ||
| the user with a url and code. After getting the device | ||
| id, a timer is started to poll periodic for a successful authentication. | ||
| This is a blocking action, meaning you | ||
| will not be able to run any other code, while waiting for an access token. | ||
|
|
||
| If you want more control over the authentication flow, use the functions | ||
| get_device_code and get_device_token. | ||
| Where poll_for_device_token will check if the "offline" | ||
| authentication was successful. | ||
| """ | ||
|
|
||
| response = self.get_device_code() | ||
| device_code = response['device_code'] | ||
| interval = response['interval'] | ||
|
|
||
| # No need to check for expiration, the API will notify us. | ||
| while True: | ||
| try: | ||
| response = self.get_device_token(device_code) | ||
| print(self.success_message.format_map(response)) | ||
| break | ||
| except RateLimitException: | ||
| # slow down | ||
| interval *= 2 | ||
| except BadRequestException: | ||
| # not pending | ||
| pass | ||
| except TraktException as e: | ||
| print(self.error_messages.get(e.http_code, response.response)) | ||
|
|
||
| sleep(interval) | ||
|
|
||
| def get_device_code(self): |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider a maximum retry or cancel mechanism
The authenticate method enters a loop that attempts to retrieve a device token indefinitely. In cases where a user does not complete authentication or if the device code is permanently invalid, this loop can persist indefinitely. Introducing a maximum number of attempts or a timeout could enhance user experience and prevent runaway processes.
| def validate_token(self): | ||
| """Check if current OAuth token has not expired""" | ||
|
|
||
| current = datetime.now(tz=timezone.utc) | ||
| expires_at = datetime.fromtimestamp(self.config.OAUTH_EXPIRES_AT, tz=timezone.utc) | ||
| if expires_at - current > timedelta(days=2): | ||
| self.OAUTH_TOKEN_VALID = True | ||
| else: | ||
| self.refresh_token() | ||
|
|
||
| def refresh_token(self): | ||
| """Request Trakt API for a new valid OAuth token using refresh_token""" | ||
|
|
||
| self.logger.info("OAuth token has expired, refreshing now...") | ||
| data = { | ||
| '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' | ||
| } | ||
|
|
||
| try: | ||
| response = self.client.post('/oauth/token', data) | ||
| except OAuthException: | ||
| self.logger.debug( | ||
| "Rejected - Unable to refresh expired OAuth token, " | ||
| "refresh_token is invalid" | ||
| ) | ||
| return | ||
|
|
||
| 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"), | ||
| ) | ||
| self.OAUTH_TOKEN_VALID = True | ||
|
|
||
| self.logger.info( | ||
| "OAuth token successfully refreshed, valid until {}".format( | ||
| datetime.fromtimestamp(self.config.OAUTH_EXPIRES_AT, tz=timezone.utc) | ||
| ) | ||
| ) | ||
| self.config.store() |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve exception handling on OAuth refresh failures
If the refresh token is invalid or expired, the refresh_token method logs a debug message but continues without raising an error. This could cause the calling code to proceed incorrectly. Consider raising a specific exception to alert the caller that the refresh sequence failed.
Somehow left out when rebasing #51
Carrying:
Refactors bunch of different logic stuffed to
core.pyinto more specific classes followingSOLIDprinciples.New classes:
HttpClient- trakt api client that takesbase_urland implementsget/post/put/deletemethods, additionally can use requestsauthTokenAuth- class implementingrequestsmodule custom auth to supply tokens as request headersTraktApiTokenAuth- usesHttpClientand returnsclient_id,client_tokenTraktApi- integratesTraktApiTokenAuthtoHttpClient, providesget/post/put/deletemethodsAuthConfig- class to deal with loading/storing auth tokens and carry their state runtimeDeviceAuthAdapter,OAuthAdapter,PinAuthAdapter- dealing with specifictrakt.init()handlingThe
HttpClientcould be used directly.if someone wants to make raw queries without the abstraction of the object they can use just the API client class. this is not documented, so we can leave it as an internal detail.
reasons of using
HttpClientdirectly could be any of:Some implementation comments:
api()andconfig()in core module are functions, so they are evaluated after module load, to be able to set constants incore.pybefore the use of these constants@lru_cache(maxsize=None)is used to memorize function result. whilemaxsize=1would work toomaxsize=Noneuses simpler code internally (disables lru, never expires)@lru_cachestacked with@property, could be replaced with@static_propertyif python is bumped to 3.8AuthConfigwas needed to carry the tokens, as they are updated in memory at runtimebumps python version to 3.7 for@dataclass: https://docs.python.org/3/library/dataclasses.html#module-dataclasses