Enhance plugin manager to autodetect installed plugs #3355
Conversation
📝 WalkthroughWalkthroughThe internal storage mechanism for plugs was refactored from a list to a dictionary indexed by plug name. Corresponding methods— Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Pre-merge checks and finishing touches❌ Failed checks (1 inconclusive)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
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: 2
🧹 Nitpick comments (1)
plugs/manager.py (1)
52-55: Consider using TypeError for type validation.The type check at line 52-54 currently raises
ValueError, but as the static analysis hint suggests,TypeErrorwould be more semantically appropriate for invalid type issues. That said, the dict assignment at line 55 looks correct.As per static analysis hints.
if not isinstance(plug, Plug): - msg = "plug must be an instance of Plug" - raise ValueError(msg) + msg = "plug must be an instance of Plug" + raise TypeError(msg)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
plugs/manager.py(3 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.py
📄 CodeRabbit inference engine (.cursorrules)
**/*.py: Prioritize readability and maintainability; follow Django's coding style guide (PEP 8 compliance).
Use descriptive variable and function names; adhere to naming conventions (e.g., lowercase with underscores for functions and variables).
Files:
plugs/manager.py
🧠 Learnings (2)
📓 Common learnings
Learnt from: rithviknishad
Repo: ohcnetwork/care PR: 2610
File: plug_config.py:18-23
Timestamp: 2024-11-25T11:38:30.225Z
Learning: In the `care` project, when camera-related code is moved from the main backend to be handled by a plugin, adding a camera plugin configuration (e.g., in `plug_config.py`) is expected, even if the PR title mentions removing camera code.
Learnt from: rithviknishad
Repo: ohcnetwork/care PR: 2610
File: plug_config.py:25-25
Timestamp: 2024-11-25T11:39:05.352Z
Learning: In `plug_config.py`, when moving camera-related code from the main backend to a plugin, adding `camera_plugin` to the plugs list is appropriate because the plugin will handle the camera functionality.
📚 Learning: 2024-11-25T11:39:05.352Z
Learnt from: rithviknishad
Repo: ohcnetwork/care PR: 2610
File: plug_config.py:25-25
Timestamp: 2024-11-25T11:39:05.352Z
Learning: In `plug_config.py`, when moving camera-related code from the main backend to a plugin, adding `camera_plugin` to the plugs list is appropriate because the plugin will handle the camera functionality.
Applied to files:
plugs/manager.py
🧬 Code graph analysis (1)
plugs/manager.py (1)
plugs/plug.py (1)
Plug(5-30)
🪛 Ruff (0.14.4)
plugs/manager.py
54-54: Prefer TypeError exception for invalid type
(TRY004)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Test / Test
🔇 Additional comments (4)
plugs/manager.py (4)
20-20: Nice refactor to dict-based storage.The dict comprehension provides O(1) lookups by plug name and simplifies the add/get operations throughout the class.
31-36: LGTM!The use of a set for deduplication and the conditional check for
plug.package_nameare both solid improvements.
57-61: Clean set-based deduplication.The use of a set to combine autodetected and explicit plugs, followed by conversion to a list, handles potential duplicates nicely. Just ensure the names from both sources are compatible (addressed in the previous comment about autodetection).
63-70: LGTM!The iteration over
values()correctly adapts to the new dict structure while maintaining the existing config aggregation logic.
| def autodetect_installed_plugs(self) -> list[str]: | ||
| # autodetect installed plugs starting with 'care_' | ||
| # these plugins will be using environment variables for configs | ||
| installed_plugins = [] | ||
| for dist in distributions(): | ||
| if ( | ||
| dist.name.startswith("care_") | ||
| and dist.requires | ||
| and any("django" in req.lower() for req in dist.requires) | ||
| ): | ||
| installed_plugins.append(dist.name) | ||
| return installed_plugins |
There was a problem hiding this comment.
🧩 Analysis chain
Verify that distribution names match Django app names.
This method returns distribution names (e.g., care_camera), which are then used in get_apps(). Ensure that these distribution names match the expected Django app configuration names, since there's no guarantee that a package's distribution name is identical to its Django app name.
Run the following script to check if autodetected distributions have corresponding Django apps:
🏁 Script executed:
#!/bin/bash
# Check for care_ packages and inspect their app configurations
# Find all installed care_ packages
pip list | grep -i "^care-" || echo "No care_ packages found with pip list"
# Search for Django app configs in installed packages
python -c "
from importlib.metadata import distributions
for dist in distributions():
if dist.name.startswith('care_'):
print(f'Distribution: {dist.name}')
try:
# Try to find AppConfig
if dist.files:
for file in dist.files:
if 'apps.py' in str(file):
print(f' Found apps.py: {file}')
except Exception as e:
print(f' Error: {e}')
"Length of output: 37
I need to examine the codebase to understand how these distribution names are used. The sandbox doesn't have the packages installed, so let me look at the actual implementation context.
---
🏁 Script executed:
```shell
#!/bin/bash
# Find how autodetect_installed_plugs is used and get_apps method
rg -n "get_apps" --type=py -B 3 -A 3
# Find Django settings or app configuration
fd -type f -name "settings.py" -o -name "apps.py" -o -name "*.txt" | head -20
# Look for references to care_ packages
rg "care_" --type=py -n
Length of output: 40205
🏁 Script executed:
#!/bin/bash
# Check the manager.py file context around the method
cat plugs/manager.pyLength of output: 2376
Let me examine the Plug class and look for any tests that show how these packages work:
Let me search for the Plug class and look for any tests:Perfect. The web search confirms the concern is valid. Most of the time, a distribution package provides one single import package with a matching name, but this is only a convention and is not enforced by PyPI or other package indices. Classic examples include Pillow (distribution) vs PIL (import package) and mysql-python (distribution) vs MySQLdb (import).
Now let me search the codebase to check if there's any validation or if the code attempts to import these names:
---
🏁 Script executed:
```shell
#!/bin/bash
# Look for how PLUGIN_APPS is used in Django settings
rg "PLUGIN_APPS" --type=py -B 2 -A 5
# Check if there's any validation/import attempt
rg "INSTALLED_APPS" --type=py -B 3 -A 3 | head -50
# Look for plugs/plug.py to understand Plug.name
fd -type f -name "plug.py" -o -name "*plug*.py" | grep -v __pycache__ | head -10
Length of output: 3919
Perfect confirmation. Now let me search for the plugs/plug.py file and any tests:
---
🏁 Script executed:
```shell
#!/bin/bash
# Find and display plugs/plug.py
cat plugs/plug.py
# Look for tests using PlugManager
rg "PlugManager\|autodetect_installed_plugs" --type=py -l
Length of output: 1010
Verify that distribution names match Django app names — confirmed as a critical issue.
The review comment is accurate. The autodetect_installed_plugs() method returns distribution names (e.g., care_camera) directly without validating they're importable as Django apps. Since the Plug class distinguishes between name (Django app name) and package_name (pip package), but autodetection has no such mapping, there's a real risk:
- Distribution name normalization (hyphens → underscores) is convention, not law
- Nothing guarantees
dist.namematches the actual Python module or app configuration name - The returned names are used directly in
config/urls.py(include(f"{plug}.urls")) andINSTALLED_APPS, which will fail at runtime if the names don't resolve - Manually registered plugs allow
namecustomization, but autodetected ones bypass this entirely
There's no validation that these distribution names are actually importable Django apps before they're added to configuration.
🤖 Prompt for AI Agents
In plugs/manager.py around lines 38 to 49, the autodetection currently returns
dist.name directly which may not be an importable Django app; change it to
resolve and validate the actual importable module/app name before adding: for
each distribution, read its top-level package(s) (top_level.txt or
distribution.read_text('top_level.txt')), try importing those module names and,
for the first importable one, verify it looks like a Django app (has an
AppConfig subclass or a urls module) and append that importable app/module name
(not the raw distribution name) to installed_plugins; if top_level.txt is absent
fall back to attempting normalized variants (replace hyphens with underscores)
but only include a plug when an import succeeds, and store the original
distribution/package name separately if you need package_name metadata.
| if ( | ||
| dist.name.startswith("care_") | ||
| and dist.requires | ||
| and any("django" in req.lower() for req in dist.requires) | ||
| ): |
There was a problem hiding this comment.
The "django" requirement check could match unintended packages.
The condition "django" in req.lower() is quite permissive and will match any requirement containing "django" as a substring, such as "djangorestframework" or even "django-cors-headers". While this might be acceptable, it could lead to false positives if a package happens to have a dependency that merely mentions django in its description.
Consider using a more precise check, perhaps parsing the requirement name more carefully or checking against known django package names.
- and any("django" in req.lower() for req in dist.requires)
+ and any(req.lower().split()[0].startswith("django") for req in dist.requires)🤖 Prompt for AI Agents
In plugs/manager.py around lines 43 to 47, the check `"django" in req.lower()`
is too permissive and can match packages like "djangorestframework" or
"django-cors-headers"; replace it by parsing each requirement string into a
Requirement object (from packaging.requirements import Requirement) and compare
the requirement's .name (lowercased) to "django" (or to a small explicit
allowlist if you intend to accept related packages), e.g. parse req into
Requirement(req) and use req_obj.name.lower() == "django" to decide inclusion.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop #3355 +/- ##
===========================================
- Coverage 53.88% 53.87% -0.01%
===========================================
Files 434 434
Lines 19686 19699 +13
Branches 2129 2134 +5
===========================================
+ Hits 10607 10613 +6
- Misses 9051 9056 +5
- Partials 28 30 +2 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Proposed Changes
Merge Checklist
/docsOnly PR's with test cases included and passing lint and test pipelines will be reviewed
@ohcnetwork/care-backend-maintainers @ohcnetwork/care-backend-admins
Summary by CodeRabbit
New Features
Improvements