Conversation
Signed-off-by: BKSteve <stephen.rieger@gmail.com>
WalkthroughThe pull request introduces an API key configuration for the OpenSubtitles provider. It adds an input field in the subtitle provider configuration form and updates related settings to load and save the API key securely. Additionally, it registers a new REST provider for OpenSubtitles, updating its configuration and communication to use HTTPS. The new provider class implements methods for authentication, searching subtitles, and downloading subtitle files via the OpenSubtitles REST API. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Provider as OpenSubtitlesRESTProvider
participant API as OpenSubtitles API
User->>Provider: Initialize provider (trigger login)
Provider->>API: POST login (credentials & API key)
API-->>Provider: Return bearer token
Note over Provider: Session headers updated
User->>Provider: Request search_subtitles(video, languages)
Provider->>API: GET search request with query parameters
API-->>Provider: Return list of subtitles
Provider->>User: Return filtered subtitle list
User->>Provider: Request download_subtitle(file_id)
Provider->>API: POST download request (file_id)
API-->>Provider: Return download link and subtitle content
Provider->>User: Return subtitle content
✨ Finishing Touches
🪧 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
Documentation and Community
|
![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 (2)
sickchill/providers/subtitle/opensubtitles.py (2)
20-39: Login exception handling is effective, but consider edge cases.
This method properly raises an error on invalid credentials. However, if the REST API supports API key-only authentication, consider accommodating flows without username/password.
46-82: Robust approach for searching subtitles, but consider defensive checks.
Parsingfiles[0]might raise an index error if no files are returned. Adding a quick length check could avoid potential runtime exceptions.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
sickchill/gui/slick/views/config_subtitles.mako(4 hunks)sickchill/oldbeard/subtitles.py(3 hunks)sickchill/providers/subtitle/opensubtitles.py(1 hunks)sickchill/settings.py(1 hunks)sickchill/start.py(2 hunks)sickchill/views/config/subtitles.py(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (6)
- GitHub Check: Test (ubuntu-latest, 3.8, false)
- GitHub Check: Test (macos-latest, 3.11, false)
- GitHub Check: Test (windows-latest, 3.11, false)
- GitHub Check: Test (ubuntu-latest, 3.11, false)
- GitHub Check: Test (ubuntu-latest, 3.10, false)
- GitHub Check: Test (ubuntu-latest, 3.9, false)
🔇 Additional comments (15)
sickchill/settings.py (1)
303-303: API key configuration added for OpenSubtitles serviceThis line adds a new configuration variable to store the OpenSubtitles API key, which is necessary for authentication with the OpenSubtitles REST API.
sickchill/views/config/subtitles.py (1)
66-66: OpenSubtitles API key retrieval from HTTP request implementedThis line captures the OpenSubtitles API key from the form submission and assigns it to the application settings, enabling the user to configure the API key through the web interface.
sickchill/gui/slick/views/config_subtitles.mako (3)
329-332: Updated provider dictionary to include API key fieldThe dictionary now includes an 'apikey' key for each subtitle provider, with OpenSubtitles correctly using the value from settings while others default to None.
341-342: Adjusted column layout for provider configurationColumn widths have been adjusted from 6 to 4 to accommodate the new API key field while maintaining a clean layout in the provider settings UI.
Also applies to: 357-358
373-388: Added API key input field to UIA new column has been added to the form to allow users to enter their OpenSubtitles API key, with appropriate styling and labels consistent with the existing username and password fields.
sickchill/start.py (2)
702-702: Load OpenSubtitles API key from configurationThis line loads the OpenSubtitles API key from the configuration file with proper security measures (censor_log=True) to ensure the API key isn't exposed in logs.
1662-1662: Save OpenSubtitles API key to configurationThis line ensures the OpenSubtitles API key is saved to the configuration file when settings are updated, properly persisting the user's API key.
sickchill/oldbeard/subtitles.py (4)
21-21: Import usage is consistent and aligns with the new provider registration.
26-28: Conditional registration logic is correct.
This ensures that the provider is only registered once, avoiding duplicate registrations.
45-51: Great move to HTTPS endpoints.
Switching to HTTPS improves security by encrypting traffic to the providers.
78-82: Consider handling missing or empty API key scenario.
Although username/password checks are in place, double-check behavior ifOPENSUBTITLES_APIKEYis empty or unset. Ensure that a missing API key won’t cause unexpected runtime issues.sickchill/providers/subtitle/opensubtitles.py (4)
1-7: Import statements are straightforward and used correctly.
No issues identified with the external libraries or modules.
8-19: Constructor logic is concise and clear.
The session headers are properly set with the API key and the token is initialized toNone. Recommend verifying behavior ifOPENSUBTITLES_APIKEYis not provided to avoid silent failures.
40-45: Initialization and termination steps are well-structured.
Automatically attempting login upon initialization and closing the session interminatefollows good resource management practices.
83-97: Download method is straightforward and well-handled.
It gracefully handles errors by logging them and returningNone. This approach is clear and consistent.
Signed-off-by: BKSteve <stephen.rieger@gmail.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🔭 Outside diff range comments (1)
sickchill/providers/subtitle/opensubtitles.py (1)
78-92: 🛠️ Refactor suggestionEnhance download_subtitle with better error handling and response validation.
The download method needs similar improvements as the search method:
- More specific exception handling
- Response validation
- Checking for token validity
def download_subtitle(self, subtitle): + if (not self.token or not self.is_token_valid()) and not self.login(): + return None + try: payload = {"file_id": subtitle["download_link"]} response = self.session.post(f"{self.base_url}/download", json=payload) response.raise_for_status() data = response.json() + + # Validate response + if "link" not in data: + logger.error("No download link in OpenSubtitles response") + return None download_response = self.session.get(data["link"]) download_response.raise_for_status() return download_response.content - except Exception as e: + except requests.RequestException as e: logger.error(f"Error downloading subtitle from OpenSubtitles: {e}") return None + except (KeyError, ValueError) as e: + logger.error(f"Error parsing OpenSubtitles download response: {e}") + return None
🧹 Nitpick comments (5)
sickchill/providers/subtitle/opensubtitles.py (5)
10-17: Consider adding API key validation.The constructor initializes well with the necessary components, but there's no validation for the API key. If
settings.OPENSUBTITLES_APIKEYis empty or None, this could cause issues during API calls.def __init__(self): self.apikey = settings.OPENSUBTITLES_APIKEY + if not self.apikey: + logger.warning("OpenSubtitles API key is not configured") self.base_url = "https://api.opensubtitles.com/api/v1" self.session = requests.Session() self.session.headers.update({"Api-Key": self.apikey, "User-Agent": "SickChill v1.0", "Content-Type": "application/json"}) self.token = None
18-34: Use more specific exception handling.The error handling is good, but catching a generic
Exceptionis too broad. Consider catching more specific exceptions likerequests.RequestExceptionto provide better error diagnostics.try: response = self.session.post(f"{self.base_url}/login", json=payload) response.raise_for_status() data = response.json() self.token = data["token"] self.session.headers.update({"Authorization": f"Bearer {self.token}"}) return True -except Exception as e: +except requests.RequestException as e: logger.error(f"Failed to login to OpenSubtitles REST API: {e}") return False +except (KeyError, ValueError) as e: + logger.error(f"Invalid response format from OpenSubtitles REST API: {e}") + return False
35-40: Consider token refresh mechanism.The initialize method calls login, but there's no mechanism to refresh the token if it expires during operation. OpenSubtitles tokens typically have an expiration time.
def initialize(self): self.login() + self.token_expiry = None # Add token expiry tracking def terminate(self): self.session.close() +def is_token_valid(self): + """Check if the current token is still valid""" + if not self.token or (self.token_expiry and self.token_expiry < time.time()): + return False + return True
1-92: Add rate limiting and retry mechanism.OpenSubtitles API has rate limits. Consider implementing a mechanism to handle rate limiting responses (HTTP 429) and retry with exponential backoff.
import time import random def _request_with_retry(self, method, url, **kwargs): """Make a request with retry logic for rate limiting""" max_retries = 3 retry_count = 0 while retry_count < max_retries: try: response = method(url, **kwargs) # If we hit a rate limit if response.status_code == 429: retry_after = int(response.headers.get('Retry-After', 10)) sleep_time = retry_after + random.uniform(0, 2) # Add jitter logger.warning(f"Rate limited by OpenSubtitles API. Retrying after {sleep_time:.2f} seconds") time.sleep(sleep_time) retry_count += 1 continue response.raise_for_status() return response except requests.RequestException as e: retry_count += 1 if retry_count < max_retries: sleep_time = 2 ** retry_count + random.uniform(0, 1) # Exponential backoff with jitter logger.warning(f"Request failed, retrying in {sleep_time:.2f} seconds: {e}") time.sleep(sleep_time) else: raise return NoneYou can then use this method in your API calls:
# Replace direct session calls with the retry method response = self._request_with_retry(self.session.get, f"{self.base_url}/subtitles", params=query)
10-17: Consider adding a logout method for proper API resource management.The OpenSubtitles API has a logout endpoint to properly terminate sessions. This helps with server resource management and security.
def logout(self): """Logout from OpenSubtitles API to free server resources""" if not self.token: return True try: response = self.session.delete(f"{self.base_url}/logout") response.raise_for_status() self.token = None return True except Exception as e: logger.error(f"Error logging out from OpenSubtitles: {e}") return FalseThen update the terminate method:
def terminate(self): + self.logout() self.session.close()
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
sickchill/oldbeard/subtitles.py(3 hunks)sickchill/providers/subtitle/opensubtitles.py(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- sickchill/oldbeard/subtitles.py
⏰ Context from checks skipped due to timeout of 90000ms (6)
- GitHub Check: Test (macos-latest, 3.11, false)
- GitHub Check: Test (ubuntu-latest, 3.10, false)
- GitHub Check: Test (windows-latest, 3.11, false)
- GitHub Check: Test (ubuntu-latest, 3.11, false)
- GitHub Check: Test (ubuntu-latest, 3.9, false)
- GitHub Check: Test (ubuntu-latest, 3.8, false)
🔇 Additional comments (1)
sickchill/providers/subtitle/opensubtitles.py (1)
1-8: Good choice of imports and dependencies.The necessary imports for HTTP requests, language handling, and provider functionality are properly included. The code correctly imports both standard libraries and application-specific modules.
| def search_subtitles(self, video, languages, hearing_impaired=False): | ||
| if not self.token and not self.login(): | ||
| return [] | ||
|
|
||
| subtitles = [] | ||
| query = { | ||
| "query": video.series if hasattr(video, "series") else os.path.splitext(os.path.basename(video.name))[0], | ||
| "languages": ",".join([lang.alpha2 for lang in languages]), | ||
| } | ||
|
|
||
| if hasattr(video, "season"): | ||
| query["season"] = video.season | ||
| if hasattr(video, "episode"): | ||
| query["episode"] = video.episode | ||
|
|
||
| try: | ||
| response = self.session.get(f"{self.base_url}/subtitles", params=query) | ||
| response.raise_for_status() | ||
| data = response.json() | ||
|
|
||
| for sub in data.get("data", []): | ||
| subtitle = { | ||
| "id": sub["attributes"]["subtitle_id"], | ||
| "language": Language.fromopensubtitles(sub["attributes"]["language"]), | ||
| "download_link": sub["attributes"]["files"][0]["file_id"], | ||
| "hearing_impaired": sub["attributes"].get("hearing_impaired", False), | ||
| "release": sub["attributes"]["release"], | ||
| } | ||
| if hearing_impaired == subtitle["hearing_impaired"]: | ||
| subtitles.append(subtitle) | ||
|
|
||
| return subtitles | ||
|
|
||
| except Exception as e: | ||
| logger.error(f"Error searching OpenSubtitles: {e}") | ||
| return [] | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve search_subtitles error handling and response validation.
The search method handles the basic flow, but there are opportunities for improvement:
- The method checks if the token exists, but not if it's expired
- The code assumes the response has a specific structure
- Consider handling API rate limits
def search_subtitles(self, video, languages, hearing_impaired=False):
- if not self.token and not self.login():
+ if (not self.token or not self.is_token_valid()) and not self.login():
return []
subtitles = []
query = {
"query": video.series if hasattr(video, "series") else os.path.splitext(os.path.basename(video.name))[0],
"languages": ",".join([lang.alpha2 for lang in languages]),
}
if hasattr(video, "season"):
query["season"] = video.season
if hasattr(video, "episode"):
query["episode"] = video.episode
try:
response = self.session.get(f"{self.base_url}/subtitles", params=query)
response.raise_for_status()
data = response.json()
+ # Validate response structure
+ if "data" not in data:
+ logger.warning("Unexpected response format from OpenSubtitles API")
+ return []
for sub in data.get("data", []):
+ # Add additional validation for required attributes
+ if not all(k in sub.get("attributes", {}) for k in ["subtitle_id", "language", "files", "release"]):
+ continue
+
+ if not sub["attributes"]["files"] or not isinstance(sub["attributes"]["files"], list):
+ continue
subtitle = {
"id": sub["attributes"]["subtitle_id"],
"language": Language.fromopensubtitles(sub["attributes"]["language"]),
"download_link": sub["attributes"]["files"][0]["file_id"],
"hearing_impaired": sub["attributes"].get("hearing_impaired", False),
"release": sub["attributes"]["release"],
}
if hearing_impaired == subtitle["hearing_impaired"]:
subtitles.append(subtitle)
return subtitles
- except Exception as e:
+ except requests.RequestException as e:
logger.error(f"Error searching OpenSubtitles: {e}")
return []
+ except (KeyError, ValueError) as e:
+ logger.error(f"Error parsing OpenSubtitles response: {e}")
+ return []📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| def search_subtitles(self, video, languages, hearing_impaired=False): | |
| if not self.token and not self.login(): | |
| return [] | |
| subtitles = [] | |
| query = { | |
| "query": video.series if hasattr(video, "series") else os.path.splitext(os.path.basename(video.name))[0], | |
| "languages": ",".join([lang.alpha2 for lang in languages]), | |
| } | |
| if hasattr(video, "season"): | |
| query["season"] = video.season | |
| if hasattr(video, "episode"): | |
| query["episode"] = video.episode | |
| try: | |
| response = self.session.get(f"{self.base_url}/subtitles", params=query) | |
| response.raise_for_status() | |
| data = response.json() | |
| for sub in data.get("data", []): | |
| subtitle = { | |
| "id": sub["attributes"]["subtitle_id"], | |
| "language": Language.fromopensubtitles(sub["attributes"]["language"]), | |
| "download_link": sub["attributes"]["files"][0]["file_id"], | |
| "hearing_impaired": sub["attributes"].get("hearing_impaired", False), | |
| "release": sub["attributes"]["release"], | |
| } | |
| if hearing_impaired == subtitle["hearing_impaired"]: | |
| subtitles.append(subtitle) | |
| return subtitles | |
| except Exception as e: | |
| logger.error(f"Error searching OpenSubtitles: {e}") | |
| return [] | |
| def search_subtitles(self, video, languages, hearing_impaired=False): | |
| if (not self.token or not self.is_token_valid()) and not self.login(): | |
| return [] | |
| subtitles = [] | |
| query = { | |
| "query": video.series if hasattr(video, "series") else os.path.splitext(os.path.basename(video.name))[0], | |
| "languages": ",".join([lang.alpha2 for lang in languages]), | |
| } | |
| if hasattr(video, "season"): | |
| query["season"] = video.season | |
| if hasattr(video, "episode"): | |
| query["episode"] = video.episode | |
| try: | |
| response = self.session.get(f"{self.base_url}/subtitles", params=query) | |
| response.raise_for_status() | |
| data = response.json() | |
| # Validate response structure | |
| if "data" not in data: | |
| logger.warning("Unexpected response format from OpenSubtitles API") | |
| return [] | |
| for sub in data.get("data", []): | |
| # Add additional validation for required attributes | |
| if not all(k in sub.get("attributes", {}) for k in ["subtitle_id", "language", "files", "release"]): | |
| continue | |
| if not sub["attributes"]["files"] or not isinstance(sub["attributes"]["files"], list): | |
| continue | |
| subtitle = { | |
| "id": sub["attributes"]["subtitle_id"], | |
| "language": Language.fromopensubtitles(sub["attributes"]["language"]), | |
| "download_link": sub["attributes"]["files"][0]["file_id"], | |
| "hearing_impaired": sub["attributes"].get("hearing_impaired", False), | |
| "release": sub["attributes"]["release"], | |
| } | |
| if hearing_impaired == subtitle["hearing_impaired"]: | |
| subtitles.append(subtitle) | |
| return subtitles | |
| except requests.RequestException as e: | |
| logger.error(f"Error searching OpenSubtitles: {e}") | |
| return [] | |
| except (KeyError, ValueError) as e: | |
| logger.error(f"Error parsing OpenSubtitles response: {e}") | |
| return [] |
|
Is this working yet? |
|
Haven't done any real testing |
| # 'legendastv' closed down | ||
| # Lines ~26-28 | ||
| if "opensubtitles" not in subliminal.provider_manager.names(): | ||
| subliminal.provider_manager.register("opensubtitles", OpenSubtitlesRESTProvider) |
There was a problem hiding this comment.
Does this work rather than using an entrypoint like the other ones? I think we wanted to use entrypoints to avoid importing all of them so that subliminal could unload ones that weren't in use and save memory.
|
This pull request is stale because it has been open 30 days with no activity. Remove stale label or comment or this will be closed in 5 days. |
github-actions
bot
added
the
Stale
2 participants
attempt at opensubtitles REST
Summary by CodeRabbit