fix: moved downloader proxy settings to parent instead of per debrid #914
Conversation
WalkthroughThe changes in this pull request primarily involve modifications to the downloader configuration and related classes in the Riven application. Key alterations include the removal of several proxy-related variables from the Changes
Possibly related PRs
Suggested reviewers
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
🧹 Outside diff range and nitpick comments (14)
src/program/services/downloaders/shared.py (1)
17-17: Add documentation and validation for the PROXY_URL class variableThe new proxy URL configuration would benefit from:
- Type hints and docstring explaining the expected format and usage
- Validation to ensure the URL is properly formatted when provided
- Default value handling if the setting is not configured
Consider updating like this:
- PROXY_URL: str = settings_manager.settings.downloaders.proxy_url + PROXY_URL: Optional[str] = None + """Optional proxy URL for downloader connections. + + Expected format: 'protocol://host:port' + Example: 'http://proxy.example.com:8080' + """ + + def __init__(self): + proxy_url = settings_manager.settings.downloaders.proxy_url + if proxy_url and not proxy_url.startswith(('http://', 'https://', 'socks5://')): + raise ValueError("Invalid proxy URL format. Must start with http://, https://, or socks5://") + self.PROXY_URL = proxy_urlsrc/program/services/downloaders/models.py (2)
32-32: LGTM! Consider adding tests for the new file size behavior.The change maintains consistency with the movie constraint logic, making the behavior uniform across both movies and episodes.
The PR checklist indicates tests need to be added. Would you like me to help generate test cases for the new file size constraint behavior? The tests should cover:
- Setting max filesize to 0 (unlimited)
- Setting max filesize to positive values
- Edge cases around the constraints
Line range hint
1-116: Update documentation to reflect the new file size behavior.While the code changes look good, the documentation should be updated to clearly explain:
- The new meaning of zero max file size (unlimited)
- The valid ranges for file size constraints
- How these constraints interact with the downloader settings
This is especially important since this file contains core models that affect download validation.
Would you like me to help draft the documentation updates?
src/program/services/downloaders/torbox.py (1)
Line range hint
1-200: Add tests and update documentation for proxy configuration changes.The PR checklist indicates missing tests and documentation updates. Given the critical nature of the downloader functionality, please ensure:
Add tests covering:
- Proxy configuration validation
- API error handling with and without proxy
- Retry mechanisms with proxy scenarios
Update documentation to reflect:
- The new centralized proxy configuration
- Migration guide for users updating from per-debrid proxy settings
Would you like me to help generate:
- Test cases for the proxy configuration scenarios?
- Documentation updates explaining the new proxy setup?
src/program/services/downloaders/alldebrid.py (3)
82-84: Add proxy URL validation and error handling.Consider adding validation for the proxy URL format and proper error handling for proxy connection issues.
self.api = AllDebridAPI( api_key=self.settings.api_key, - proxy_url=self.PROXY_URL if self.PROXY_URL else None + proxy_url=self._validate_proxy_url(self.PROXY_URL) if self.PROXY_URL else None ) +def _validate_proxy_url(self, url: str) -> Optional[str]: + """Validate proxy URL format and connectivity""" + try: + # Add URL format validation + if not url.startswith(('http://', 'https://')): + logger.warning(f"Invalid proxy URL format: {url}") + return None + return url + except Exception as e: + logger.error(f"Failed to validate proxy URL: {e}") + return None
Line range hint
246-259: Fix return type mismatch in get_files_and_links.The method signature promises
List[DebridFile]but returns a raw dictionary from the API response. This could lead to type errors downstream.Apply this fix to process the API response into the correct return type:
def get_files_and_links(self, torrent_id: str) -> List[DebridFile]: """ Get torrent files and links by id """ try: response = self.api.request_handler.execute( HttpMethod.GET, "magnet/files", params={"id[]": torrent_id} ) magnet_info = next((info for info in response.get("magnets") if info["id"] == torrent_id), {}) - return magnet_info.get("files", {}) + files = magnet_info.get("files", []) + return [ + DebridFile( + filename=file["filename"], + filesize_bytes=file["size"], + download_link=file.get("download", "") + ) + for file in files + ] except Exception as e: logger.error(f"Failed to get files for {torrent_id}: {e}") - raise + return []
Line range hint
1-259: Address uncompleted checklist items.The PR checklist indicates that tests and documentation updates are needed. Please ensure:
- Add tests for the new proxy URL handling
- Update documentation to reflect the proxy configuration changes
Would you like help with:
- Generating unit tests for the proxy URL handling?
- Updating the documentation to reflect these changes?
src/program/services/downloaders/realdebrid.py (1)
101-101: Document the inherited proxy configuration.The code now uses
self.PROXY_URLinherited fromDownloaderBaseinstead of per-debrid proxy settings, which aligns with the PR objectives. However, this inheritance isn't immediately clear from the code.Consider adding a docstring or comment to clarify the proxy configuration inheritance:
self.api = RealDebridAPI( api_key=self.settings.api_key, + # PROXY_URL is inherited from DownloaderBase and configured globally for all downloaders proxy_url=self.PROXY_URL if self.PROXY_URL else None )src/program/settings/models.py (3)
Line range hint
64-68: Consider adding migration logic for existing configurationsThe removal of proxy settings from individual debrid models and consolidation into DownloadersModel may break existing configurations.
Consider:
- Adding migration logic in the Observable base class to handle the transition
- Adding deprecation warnings for the old proxy settings if they're still present in config files
- Documenting the migration path in the changelog
Would you like me to provide an example implementation for the migration logic?
Line range hint
64-68: Update documentation and add testsAs noted in the PR description, tests and documentation updates are pending.
Please ensure to:
- Update the documentation to reflect the new proxy configuration location
- Add tests to verify proxy settings behavior
- Update configuration examples
Would you like me to:
- Generate test cases for the proxy configuration?
- Create a documentation PR with the necessary updates?
- Update the configuration examples?
Proxy settings need to be updated in multiple locations
Several files still contain references to the old proxy settings structure that need to be updated to match the new model:
src/routers/secure/default.py: Usessettings_manager.settings.downloaders.proxy_urlsrc/program/services/downloaders/shared.py: DefinesPROXY_URLusing old settings pathsrc/tests/test_settings_migration.py: Contains old proxy settings structure withproxy_enabledandproxy_urlin test data- Multiple downloader classes (
TorBoxAPI,RealDebridAPI,AllDebridAPI) use the proxy settings through their base classThe code changes should:
- Update the settings path in
default.pyandshared.pyto use the new proxy model- Update test data in
test_settings_migration.pyto reflect the new settings structure- Verify the proxy configuration in downloader classes still works with the new model
🔗 Analysis chain
Line range hint
64-68: Verify removal of old proxy settingsLet's ensure all references to the old proxy settings have been updated.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for any remaining references to the old proxy settings # Check for old proxy settings in Python files echo "Checking for old proxy settings references..." rg -l "proxy_enabled|proxy_url" --type py # Check for old proxy settings in configuration examples echo "Checking configuration examples..." fd -e yaml -e json -e toml -e ini -e conf -x rg -l "proxy_enabled|PROXY_ENABLED|proxy_url|PROXY_URL" {} # Check for old proxy settings in documentation echo "Checking documentation..." fd -e md -e rst -x rg -l "proxy_enabled|proxy_url" {}Length of output: 727
Script:
#!/bin/bash # Let's examine the actual usage of proxy settings in these files # Check the content of files that reference proxy settings echo "=== Checking src/routers/secure/default.py ===" rg -A 5 -B 5 "proxy_enabled|proxy_url" src/routers/secure/default.py echo "=== Checking src/program/services/downloaders/shared.py ===" rg -A 5 -B 5 "proxy_enabled|proxy_url" src/program/services/downloaders/shared.py echo "=== Checking src/program/services/downloaders/torbox.py ===" rg -A 5 -B 5 "proxy_enabled|proxy_url" src/program/services/downloaders/torbox.py echo "=== Checking src/program/services/downloaders/realdebrid.py ===" rg -A 5 -B 5 "proxy_enabled|proxy_url" src/program/services/downloaders/realdebrid.py echo "=== Checking src/program/services/downloaders/alldebrid.py ===" rg -A 5 -B 5 "proxy_enabled|proxy_url" src/program/services/downloaders/alldebrid.py echo "=== Checking src/tests/test_settings_migration.py ===" rg -A 5 -B 5 "proxy_enabled|proxy_url" src/tests/test_settings_migration.pyLength of output: 6424
src/routers/secure/default.py (2)
Line range hint
58-60: Consider improving error handling.The current error handling returns a wrapped error response that's inconsistent with the success case. Consider standardizing the response format:
- if response.status_code != 200: - return {"success": False, "message": response.json()} - - return response.json() + response.raise_for_status() + return response.json()This change would:
- Use FastAPI's built-in error handling
- Maintain consistent response types
- Provide better error details to clients
Tests and documentation needed for proxy configuration changes
The codebase inspection reveals:
- Proxy configuration has moved to a centralized location in
DownloadersModel- Existing tests only cover proxy settings migration but not the actual proxy functionality
- No dedicated documentation for proxy configuration was found
Required actions:
- Add unit tests in
src/tests/test_requests.pyto verify proxy behavior with the new centralized configuration- Document the proxy configuration in README.md or a dedicated configuration guide
🔗 Analysis chain
Line range hint
49-60: Address unchecked items from PR checklist.The PR checklist indicates that tests and documentation updates are pending. Please ensure:
- Add tests to verify the proxy configuration behavior
- Update documentation to reflect the centralized proxy configuration approach
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check for existing tests related to proxy configuration rg -l "test.*proxy" "tests/" # Check for documentation files that might need updates fd -e md -e rst . docs/Length of output: 194
Script:
#!/bin/bash # Let's first check if there are any test directories or files in the repository fd "test" # Check for any documentation files in the repository root or common doc locations fd -e md -e rst # Look for proxy-related code to understand testing scope rg -l "proxy"Length of output: 832
Script:
#!/bin/bash # Check if there are any existing proxy-related tests rg "proxy" "src/tests/" # Look at the settings migration test to understand proxy configuration testing cat "src/tests/test_settings_migration.py" # Check proxy-related code in the settings model cat "src/program/settings/models.py"Length of output: 12483
.env.example (1)
69-69: Add documentation for the new proxy URL variable.The new
RIVEN_DOWNLOADERS_PROXY_URLvariable lacks documentation about its purpose, format, and usage. Consider adding comments to explain:
- The expected proxy URL format
- Whether this is a global setting affecting all downloaders
- If this replaces the previous per-debrid proxy settings
Add documentation above the variable:
+# Global proxy URL for all downloaders. Format: protocol://host:port RIVEN_DOWNLOADERS_PROXY_URL=
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (9)
.env.example(1 hunks)src/program/services/downloaders/__init__.py(0 hunks)src/program/services/downloaders/alldebrid.py(1 hunks)src/program/services/downloaders/models.py(1 hunks)src/program/services/downloaders/realdebrid.py(1 hunks)src/program/services/downloaders/shared.py(2 hunks)src/program/services/downloaders/torbox.py(1 hunks)src/program/settings/models.py(1 hunks)src/routers/secure/default.py(1 hunks)
💤 Files with no reviewable changes (1)
- src/program/services/downloaders/init.py
🔇 Additional comments (9)
src/program/services/downloaders/shared.py (2)
Line range hint 14-108: LGTM! Well-structured base class with clear abstractions
The abstract base class provides a solid foundation for downloader implementations with:
- Clear method contracts through abstract methods
- Comprehensive docstrings
- Good separation of concerns
17-17: Verify proxy URL usage across downloader implementations
The centralization of proxy settings may require updates in child classes that previously managed their own proxy settings.
✅ Verification successful
Let's examine the actual proxy usage in these files to understand if there are any inconsistencies or independent proxy implementations.
Proxy URL implementation is correctly centralized and consistently used
The verification shows that the proxy URL is properly implemented:
- Base class
DownloaderBasedefines the centralizedPROXY_URLfrom settings - All downloader implementations (RealDebrid, TorBox, AllDebrid) consistently:
- Accept
proxy_urlin their API class constructors - Pass the centralized
PROXY_URLfrom the base class to their respective API instances - Apply the proxy configuration uniformly using session proxies
- Accept
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for existing proxy-related code in downloader implementations
rg -l "proxy" --type py src/program/services/downloaders/
Length of output: 236
Script:
#!/bin/bash
# Check the actual proxy usage in the identified files
echo "=== shared.py ==="
rg "proxy" -B 2 -A 2 src/program/services/downloaders/shared.py
echo -e "\n=== realdebrid.py ==="
rg "proxy" -B 2 -A 2 src/program/services/downloaders/realdebrid.py
echo -e "\n=== torbox.py ==="
rg "proxy" -B 2 -A 2 src/program/services/downloaders/torbox.py
echo -e "\n=== alldebrid.py ==="
rg "proxy" -B 2 -A 2 src/program/services/downloaders/alldebrid.py
Length of output: 2698
src/program/services/downloaders/models.py (1)
28-28: Improved handling of movie file size limits.
The change to use > 0 makes the behavior more intuitive - setting max filesize to 0 now means "unlimited" instead of enforcing a 0 MB limit.
Let's verify existing configurations to ensure this change doesn't unexpectedly affect any environments:
✅ Verification successful
Change in file size limit behavior is safe and well-documented
The verification shows this change is safe and properly implemented:
- The settings model (
settings/models.py) documents that-1is the standard value for "no limit", withmovie_filesize_mb_max: int = -1 # MB (-1 is no limit) - The test file shows the filesize settings were already commented out in the old configuration, suggesting they weren't widely used
- The change aligns with the codebase's existing pattern where negative values and zero are handled similarly for "no limit" cases
- The implementation in
downloaders/models.pyis consistent, applying the same logic to both movie and episode constraints
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for any environment files or configuration files that might set movie_max_filesize
rg -l "movie.*filesize.*max|movie.*max.*filesize"
# Search for any documentation mentioning file size limits
fd -e md -e rst -e txt | xargs rg -l "movie.*filesize|file.*size.*limit"
Length of output: 237
Script:
#!/bin/bash
# Let's examine the actual usage and configuration of movie_max_filesize
# First, check the test file to understand expected behavior
cat src/tests/test_settings_migration.py
# Then check the settings model to see how it's configured
cat src/program/settings/models.py
# Finally, let's see the full context of the downloader models
cat src/program/services/downloaders/models.py
Length of output: 16418
src/program/services/downloaders/torbox.py (1)
87-87: Verify parent class proxy configuration.
The change aligns with moving proxy settings to the parent class, but let's verify the implementation:
- Ensure
PROXY_URLis properly defined in the parentDownloaderBaseclass - Confirm that all downloader implementations consistently use this new approach
✅ Verification successful
Proxy configuration is correctly implemented in parent class
The verification confirms that:
PROXY_URLis properly defined inDownloaderBaseclass and sourced from settings- All downloader implementations (TorBox, RealDebrid, AllDebrid) consistently use the inherited
self.PROXY_URL - The implementation aligns with the PR objective of moving proxy settings to the parent class
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify proxy configuration consistency across downloader implementations
# Check if PROXY_URL is defined in DownloaderBase
echo "Checking DownloaderBase for PROXY_URL definition:"
ast-grep --pattern 'class DownloaderBase:
$$$
PROXY_URL = $_
$$$'
# Check other downloader implementations for consistent proxy usage
echo -e "\nChecking other downloaders for proxy configuration:"
rg -A 3 'proxy_url.*PROXY_URL' src/program/services/downloaders/
Length of output: 1267
Script:
#!/bin/bash
# Let's check the DownloaderBase class implementation with a broader pattern
echo "Checking DownloaderBase class implementation:"
fd -e py -x rg -l "class DownloaderBase" {} \; | xargs rg -A 20 "class DownloaderBase"
# Also check if there are any proxy-related configurations in __init__
echo -e "\nChecking proxy initialization:"
ast-grep --pattern 'def __init__($$$):
$$$
PROXY_URL = $_
$$$'
Length of output: 1018
src/program/services/downloaders/alldebrid.py (1)
83-83: Verify parent class proxy URL attribute.
The code assumes self.PROXY_URL is available from a parent class. Please ensure this attribute is properly defined and documented in the parent class.
Let's verify the parent class implementation:
✅ Verification successful
PROXY_URL attribute is correctly defined in parent class
The PROXY_URL attribute is properly defined in the DownloaderBase class and is sourced from settings_manager.settings.downloaders.proxy_url. The AllDebridDownloader inherits this attribute correctly through class inheritance, making the code change valid and properly implemented.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for PROXY_URL definition in parent classes
ast-grep --pattern 'class $CLASS extends DownloaderBase { $$$ }' | rg 'PROXY_URL'
# Search for the base class definition
ast-grep --pattern 'class DownloaderBase { $$$ }'
Length of output: 133
Script:
#!/bin/bash
# Let's try different approaches to find the parent class and PROXY_URL
# Search for any class definitions containing PROXY_URL
ast-grep --pattern 'class $CLASS { $$$ PROXY_URL $$$ }'
# Search for files containing PROXY_URL
rg -l "PROXY_URL"
# Search for base downloader class definition
rg -l "class.*DownloaderBase"
# Search for inheritance patterns in Python files
rg "class.*\(.*\):" --type py
Length of output: 10409
Script:
#!/bin/bash
# Let's examine the DownloaderBase class in shared.py and the implementation in alldebrid.py
# Check DownloaderBase implementation
rg -A 20 "class DownloaderBase" src/program/services/downloaders/shared.py
# Check PROXY_URL usage in shared.py
rg "PROXY_URL" src/program/services/downloaders/shared.py -A 5 -B 5
# Check AllDebridDownloader implementation
rg -A 10 "class AllDebridDownloader" src/program/services/downloaders/alldebrid.py
Length of output: 1613
src/program/services/downloaders/realdebrid.py (2)
Line range hint 114-122: LGTM: Validation logic correctly simplified.
The validation logic has been appropriately simplified to focus on essential checks (enabled status and API key) while removing redundant proxy validation that's now handled at the parent level.
Line range hint 1-277: LGTM: Well-structured implementation with good error handling.
The overall implementation is solid with:
- Clear separation of concerns
- Comprehensive error handling
- Effective logging
- Well-documented classes and methods
src/routers/secure/default.py (1)
49-50: LGTM! Proxy configuration change aligns with PR objectives.
The simplified proxy URL assignment correctly implements the centralized proxy configuration approach.
.env.example (1)
69-69: Verify migration path for existing configurations.
The consolidation of proxy settings might affect existing configurations using the old per-debrid proxy settings.
Let's check for any existing documentation or migration guides:
Would you like me to help create a migration guide for users upgrading from the old proxy configuration?
| @@ -98,7 +98,7 @@ def validate(self) -> bool: | |||
|
|
|||
| self.api = RealDebridAPI( | |||
| api_key=self.settings.api_key, | |||
| proxy_url=self.settings.proxy_url if self.settings.proxy_enabled else None | |||
| proxy_url=self.PROXY_URL if self.PROXY_URL else None | |||
There was a problem hiding this comment.
💡 Codebase verification
Migration path is already handled, but documentation needs attention
Based on the verification results:
-
The code already has proper migration support:
test_settings_migration.pyshows that old proxy settings are correctly migrated- The shared downloader base class properly reads from the centralized proxy settings
- Settings model has been updated to support the new structure
-
However, there's a documentation gap:
- No proxy configuration documentation in README.md
- No proxy-related changes documented in CHANGELOG.md
- No environment variable documentation for proxy settings
Please add documentation for:
- The new centralized proxy configuration in README.md
- Migration notes in CHANGELOG.md
- Any relevant environment variables
🔗 Analysis chain
Consider migration path and documentation updates.
The centralization of proxy settings is a good architectural improvement. However, there are a few considerations to ensure a smooth transition:
- Migration Guide: Users with existing per-debrid proxy configurations will need guidance on migrating to the new centralized configuration.
- Documentation: The PR checklist indicates documentation updates are pending. Please ensure the following are documented:
- New proxy configuration approach
- Updated environment variables
- Migration steps for existing users
- Testing: The PR checklist also indicates tests are pending. Please ensure test coverage for:
- Proxy configuration inheritance
- Backward compatibility (if applicable)
- Error cases
Let's verify the documentation and test requirements:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for documentation and test updates
# Check for documentation updates
echo "Checking for documentation files..."
fd -e md -e rst
# Check for test files
echo "Checking for test files..."
fd -g "*test*.py"
# Check for environment variable documentation
echo "Checking for environment variable documentation..."
fd -g ".env*"
Length of output: 944
Script:
#!/bin/bash
# Let's check for proxy-related changes and documentation
# Check for proxy-related changes in Python files
echo "Checking for proxy-related code changes..."
rg -l "proxy" --type py
# Check README.md content for proxy configuration
echo "Checking README for proxy documentation..."
rg -i "proxy" README.md
# Check for proxy-related test cases
echo "Checking for proxy-related tests..."
rg -i "proxy" src/tests/
# Check changelog for proxy-related updates
echo "Checking CHANGELOG.md for proxy updates..."
rg -i "proxy" CHANGELOG.md
Length of output: 1175
Script:
#!/bin/bash
# Let's examine the proxy-related code changes in more detail
# Check the shared downloader code for proxy implementation
echo "Checking shared downloader implementation..."
rg -A 5 "proxy" src/program/services/downloaders/shared.py
# Check settings model for proxy configuration
echo "Checking settings model..."
rg -A 5 "proxy" src/program/settings/models.py
# Check settings migration test details
echo "Checking settings migration test..."
cat src/tests/test_settings_migration.py
Length of output: 3194
Pull Request Check List
Resolves: #issue-number-here
Description:
Summary by CodeRabbit
Release Notes
New Features
Bug Fixes
Refactor
Chores