fix: api manual scraping fixes #915
Conversation
WalkthroughThe changes in this pull request involve modifications to the Changes
Possibly related PRs
Suggested reviewers
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: 3
🧹 Outside diff range and nitpick comments (6)
src/routers/secure/scrape.py (3)
279-282: Handle exceptions more specifically for better error tracingCatching a general
Exceptionmay mask underlying issues. Consider catching specific exceptions related to downloader operations to provide more precise error handling and messaging.
310-310: Potential inconsistency in HTTP status codeRaising a
500 Internal Server Errorwhen no torrent ID is found may not accurately reflect the nature of the error. Consider using a404 Not Foundor400 Bad Requeststatus code to provide a more precise indication.Apply this fix:
- raise HTTPException(status_code=500, detail="No torrent ID found") + raise HTTPException(status_code=404, detail="No torrent ID found")
339-339: Clarify error status when item ID is missingSimilar to the previous comment, using a
500 Internal Server Errormay not accurately represent the issue whenitem_idis not found. A404 Not Foundwould be more appropriate.Apply this fix:
- raise HTTPException(status_code=500, detail="No item ID found") + raise HTTPException(status_code=404, detail="No item ID found")src/program/services/downloaders/shared.py (1)
44-55: Clarify return type and document possible values inadd_torrentSince
add_torrentcan return eitherintorstr, consider documenting the specific scenarios or downloader implementations that return each type to improve clarity for users of the method.src/program/services/downloaders/models.py (2)
60-63: Usenot infor clearer string membership testingThe condition
not "sample" in filename.lower()is more idiomatically expressed usingnot in.Apply this diff:
- if not any(filename.endswith(ext) for ext in VIDEO_EXTENSIONS) and not "sample" in filename.lower(): + if not any(filename.endswith(ext) for ext in VIDEO_EXTENSIONS) and "sample" not in filename.lower():🧰 Tools
🪛 Ruff (0.8.0)
60-60: Test for membership should be
not inConvert to
not in(E713)
68-70: Combine conditional statements to simplify logicYou can merge the
elifand nestedifconditions to streamline the code.Apply this diff:
- elif filetype in ["show", "season", "episode"]: - if not (FILESIZE_EPISODE_CONSTRAINT[0] <= filesize_mb <= FILESIZE_EPISODE_CONSTRAINT[1]): - return None + elif filetype in ["show", "season", "episode"] and not (FILESIZE_EPISODE_CONSTRAINT[0] <= filesize_mb <= FILESIZE_EPISODE_CONSTRAINT[1]): + return None🧰 Tools
🪛 Ruff (0.8.0)
68-69: Use a single
ifstatement instead of nestedifstatements(SIM102)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
src/program/services/downloaders/models.py(1 hunks)src/program/services/downloaders/shared.py(3 hunks)src/routers/secure/scrape.py(9 hunks)
🧰 Additional context used
🪛 Ruff (0.8.0)
src/program/services/downloaders/models.py
60-60: Test for membership should be not in
Convert to not in
(E713)
68-69: Use a single if statement instead of nested if statements
(SIM102)
src/routers/secure/scrape.py
285-285: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
🔇 Additional comments (6)
src/routers/secure/scrape.py (3)
42-43: Good use of explicit type annotations for torrent_info and containers
Defining torrent_info as TorrentInfo and containers as Optional[List[TorrentContainer]] in StartSessionResponse enhances type safety and code clarity.
106-109: Improved type safety in ScrapingSession attributes
Adding explicit type annotations for torrent_id, torrent_info, containers, and selected_files increases code maintainability and helps prevent type-related errors.
224-225: Ensure is_cached flag accurately reflects availability
After setting stream.is_cached = bool(container and container.cached), verify that get_instant_availability correctly determines the cached status to prevent any false positives or negatives.
src/program/services/downloaders/shared.py (3)
60-66: Consistent type annotations enhance method usability
Updating select_files to accept torrent_id: Union[int, str] aligns with changes in other methods and supports flexibility across different downloader services.
71-71: Ensure consistent torrent_id types across methods
By accepting torrent_id: Union[int, str] in get_torrent_info, you maintain consistency and handle varying identifier types from different downloaders.
84-84: Consistent handling of torrent_id in delete_torrent
Accepting torrent_id as Union[int, str] ensures that the method can handle identifiers from various downloader implementations.
| torrent_id: str = downloader.add_torrent(info_hash) | ||
| torrent_info: TorrentInfo = downloader.get_torrent_info(torrent_id) | ||
| container: Optional[TorrentContainer] = downloader.get_instant_availability(info_hash, item.type) | ||
| session_manager.update_session(session.id, torrent_id=torrent_id, torrent_info=torrent_info, containers=container) | ||
| except Exception as e: | ||
| background_tasks.add_task(session_manager.abort_session, session.id) | ||
| raise HTTPException(status_code=500, detail=str(e)) |
There was a problem hiding this comment.
Use exception chaining to preserve traceback
When re-raising an exception within an except block, use exception chaining to maintain the original traceback information.
Apply this fix:
- raise HTTPException(status_code=500, detail=str(e))
+ raise HTTPException(status_code=500, detail=str(e)) from e📝 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.
| raise HTTPException(status_code=500, detail=str(e)) | |
| raise HTTPException(status_code=500, detail=str(e)) from e |
🧰 Tools
🪛 Ruff (0.8.0)
285-285: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
| item.alternative_folder = session.torrent_info.alternative_filename | ||
| item.active_stream = {"infohash": session.magnet, "id": session.torrent_info.id} |
There was a problem hiding this comment.
Ensure session.torrent_info is not None before accessing attributes
Accessing session.torrent_info.alternative_filename assumes that session.torrent_info is not None. Add a check to prevent potential AttributeError.
Apply this fix:
+ if session.torrent_info:
item.alternative_folder = session.torrent_info.alternative_filename
item.active_stream = {"infohash": session.magnet, "id": session.torrent_info.id}
+ else:
+ raise HTTPException(status_code=500, detail="Torrent info is missing in the session")📝 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.
| item.alternative_folder = session.torrent_info.alternative_filename | |
| item.active_stream = {"infohash": session.magnet, "id": session.torrent_info.id} | |
| if session.torrent_info: | |
| item.alternative_folder = session.torrent_info.alternative_filename | |
| item.active_stream = {"infohash": session.magnet, "id": session.torrent_info.id} | |
| else: | |
| raise HTTPException(status_code=500, detail="Torrent info is missing in the session") |
| item_episode.alternative_folder = session.torrent_info.alternative_filename | ||
| item_episode.active_stream = {"infohash": session.magnet, "id": session.torrent_info.id} |
There was a problem hiding this comment.
Add null check for session.torrent_info before accessing
Same as previous comment, ensure session.torrent_info is not None when accessing its attributes in the loop for episodes.
Apply this fix:
+ if session.torrent_info:
item_episode.alternative_folder = session.torrent_info.alternative_filename
item_episode.active_stream = {"infohash": session.magnet, "id": session.torrent_info.id}
+ else:
+ raise HTTPException(status_code=500, detail="Torrent info is missing in the session")📝 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.
| item_episode.alternative_folder = session.torrent_info.alternative_filename | |
| item_episode.active_stream = {"infohash": session.magnet, "id": session.torrent_info.id} | |
| if session.torrent_info: | |
| item_episode.alternative_folder = session.torrent_info.alternative_filename | |
| item_episode.active_stream = {"infohash": session.magnet, "id": session.torrent_info.id} | |
| else: | |
| raise HTTPException(status_code=500, detail="Torrent info is missing in the session") |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
src/program/services/scrapers/mediafusion.py (1)
Line range hint
73-109: Consider enhancing downloader configuration validation.The current implementation validates downloader settings individually. Consider implementing a more robust validation approach:
- Create an enum or constant for supported downloaders
- Add version compatibility checks
- Validate API key format before encryption
Example enhancement:
from enum import Enum class SupportedDownloader(Enum): REAL_DEBRID = "realdebrid" TORBOX = "torbox" ALL_DEBRID = "alldebrid" def _validate_downloader_config(self) -> tuple[str, str]: """Validate and return (api_key, downloader_name)""" for downloader in SupportedDownloader: config = getattr(self.app_settings.downloaders, downloader.name.lower()) if config.enabled: if not self._validate_api_key_format(config.api_key): raise ValueError(f"Invalid API key format for {downloader.name}") return config.api_key, downloader.value raise ValueError("No supported downloader enabled")
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
src/program/services/scrapers/mediafusion.py(1 hunks)
🔇 Additional comments (1)
src/program/services/scrapers/mediafusion.py (1)
69-71: LGTM! Verify API compatibility.
The addition of All-Debrid support follows the established pattern and is implemented correctly. However, let's verify that "alldebrid" is the expected identifier in the API.
✅ Verification successful
"alldebrid" identifier is correctly used and consistent across the codebase
The verification confirms that:
- The identifier "alldebrid" is consistently used in the downloader implementation (
AllDebridDownloader.key = "alldebrid") - The API base URL is correctly defined as "api.alldebrid.com"
- The identifier matches the test data and API response formats
- The implementation is well-tested with comprehensive test cases
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the "alldebrid" identifier usage in the codebase
# Check for any existing usage of the "alldebrid" identifier
rg -i "alldebrid" --type py
# Check for any API endpoint definitions or documentation
fd -e yaml -e json . | xargs rg -i "alldebrid"
Length of output: 6104
|
TorrentContainer is a new structure, it's support need to be added in UI layer as well, currently with this change only, manual scrapping does not fix. |
Pull Request Check List
Resolves: #issue-number-here
Description:
Summary by CodeRabbit