[feature] Mass Command model and REST APIs for async command execution#1395
[feature] Mass Command model and REST APIs for async command execution#1395dee077 wants to merge 25 commits into
Conversation
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughAdds batch command support across the controller. Batches target devices directly or through organization, group, and location filters; validate input; create linked child commands asynchronously; track skipped devices; and aggregate execution status. Protected REST endpoints provide dry-run, execution, list, and detail operations with authorization and sensitive-input masking. Migrations, permissions, sample-app wiring, documentation, and comprehensive tests are included. Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant Client
participant BatchCommandExecuteView
participant BatchCommand
participant launch_batch_command
participant Command
Client->>BatchCommandExecuteView: submit batch execution request
BatchCommandExecuteView->>BatchCommand: validate and resolve targets
BatchCommand->>launch_batch_command: schedule asynchronous execution
launch_batch_command->>Command: create linked device commands
Command-->>BatchCommand: update aggregate status
BatchCommandExecuteView-->>Client: return batch response
Possibly related issues
Suggested reviewers: Caution Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional.
❌ Failed checks (2 errors, 1 inconclusive)
✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
CI Failures: Black Formatting and System ChecksHello @dee077,
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
.github/workflows/ci.yml (1)
4-16: 🧹 Nitpick | 🔵 Trivial | ⚡ Quick winAdd workflow concurrency to prevent duplicate CI runs.
Line 4 defines triggers, but there is no
concurrencyguard. Rapid pushes to the same PR/branch can run overlapping matrices and waste runners.Suggested patch
on: push: branches: - master - "gsoc26-*" pull_request: branches: - master - "1.1" - "1.2" - "gsoc26-*" + +concurrency: + group: ci-${{ github.workflow }}-${{ github.event.pull_request.number || github.ref }} + cancel-in-progress: true🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.github/workflows/ci.yml around lines 4 - 16, Add a top-level concurrency stanza to the workflow (next to the existing on: triggers) to prevent duplicate CI runs: add a concurrency key with a group expression that uniquely identifies the branch/PR (e.g. using github.workflow plus github.ref or github.event.pull_request.number when available) and set cancel-in-progress: true so newer pushes cancel older runs; update the workflow around the existing on: block to include this concurrency configuration.Source: Linters/SAST tools
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@openwisp_controller/connection/base/models.py`:
- Around line 817-832: The launch() method sets status="in-progress" and saves
even when resolve_devices() yields no devices, which leaves batches stuck
because no CommandOperation rows will trigger calculate_and_update_status();
update launch() to detect an empty devices queryset: after resolving devices
(resolve_devices()) and before creating CommandOperation instances, if there are
zero devices set status to a terminal state (e.g., "completed" or "no-devices"),
set total_devices=0, persist those fields via save(update_fields=[...]) and
return early so no bulk_create is attempted; keep references to
CommandOperation, total_devices, and calculate_and_update_status() unchanged.
- Around line 822-830: The loop builds a potentially huge in-memory list `ops`
before calling `CommandOperation.objects.bulk_create(ops)`, causing memory
spikes; modify the code that iterates `devices.iterator()` to create and
validate each `CommandOperation` (call `full_clean()`), accumulate them in a
small buffer, and call `CommandOperation.objects.bulk_create(buffer,
batch_size=...)` whenever the buffer reaches a chosen chunk size (then clear the
buffer) and after the loop flush any remaining items; keep references to the
same symbols (`CommandOperation`, `batch_command_operation`,
`devices.iterator()`, `full_clean`, `bulk_create`) so reviewers can find and
verify the chunking change.
- Around line 823-830: The batch launch loop in launch() only creates
CommandOperation rows via CommandOperation.objects.bulk_create(ops) leaving
their command field NULL and never invoking AbstractCommand._schedule_command,
so per-device work is never enqueued; fix by, after creating each
CommandOperation (or immediately after bulk_create), ensuring each operation
gets a scheduled command—either call the scheduling logic
(AbstractCommand._schedule_command or an equivalent method) for each
CommandOperation instance or create and assign the related Command objects and
enqueue them so operations transition out of in-progress; update the code
referencing devices.iterator(), ops, batch_command_operation and
CommandOperation to create/assign commands and invoke the scheduler for every op
so the per-device work is enqueued and can complete.
- Around line 779-805: AbstractBatchCommand.clean currently only checks org
ownership for group and location but neglects to enforce the
organization-specific command allowlist and to validate command_input against
the command schema, allowing invalid or disabled device commands to be stored;
update AbstractBatchCommand.clean to perform the same command validation that
AbstractCommand.clean does: verify that self.command is allowed for
self.organization (apply the same allowlist check), validate/deserialize
self.command_input against the command's schema (raise ValidationError on schema
errors), and ensure disabled commands are rejected — reuse or call the existing
AbstractCommand validation helper/logic rather than duplicating ad-hoc checks so
errors are consistent.
- Line 835: The code uses the hardcoded reverse accessor
self.commandoperation_set in BatchCommandOperation.calculate_and_update_status
which breaks when CommandOperation is swapped; add an explicit related_name on
AbstractCommandOperation.batch_command_operation (e.g.,
related_name="batch_command_operations") and update
BatchCommandOperation.calculate_and_update_status to use that new accessor
(replace self.commandoperation_set with the chosen related_name) ensuring
compatibility with swapper.swappable_setting("connection", "CommandOperation").
---
Outside diff comments:
In @.github/workflows/ci.yml:
- Around line 4-16: Add a top-level concurrency stanza to the workflow (next to
the existing on: triggers) to prevent duplicate CI runs: add a concurrency key
with a group expression that uniquely identifies the branch/PR (e.g. using
github.workflow plus github.ref or github.event.pull_request.number when
available) and set cancel-in-progress: true so newer pushes cancel older runs;
update the workflow around the existing on: block to include this concurrency
configuration.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 9644ffdf-bf70-4c83-809f-afee03e5b60c
📒 Files selected for processing (3)
.github/workflows/ci.ymlopenwisp_controller/connection/base/models.pyopenwisp_controller/connection/models.py
📜 Review details
⏰ 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). (11)
- GitHub Check: Python==3.12 | django~=5.2.0
- GitHub Check: Python==3.13 | django~=5.2.0
- GitHub Check: Python==3.13 | django~=5.1.0
- GitHub Check: Python==3.12 | django~=4.2.0
- GitHub Check: Python==3.11 | django~=5.2.0
- GitHub Check: Python==3.12 | django~=5.1.0
- GitHub Check: Python==3.11 | django~=4.2.0
- GitHub Check: Python==3.10 | django~=5.1.0
- GitHub Check: Python==3.10 | django~=5.2.0
- GitHub Check: Python==3.11 | django~=5.1.0
- GitHub Check: Python==3.10 | django~=4.2.0
🧰 Additional context used
📓 Path-based instructions (5)
**/*.{js,ts,tsx,jsx,py,java,go,cs,rb,php,c,cpp,h,hpp}
📄 CodeRabbit inference engine (Custom checks)
**/*.{js,ts,tsx,jsx,py,java,go,cs,rb,php,c,cpp,h,hpp}: Flag potential security vulnerabilities in code
Avoid unnecessary comments or docstrings for code that is already clear
Code formatting is compact and readable. Do not add excessive blank lines, especially inside function or method bodies
Flag unused or redundant code
Ensure variables, functions, classes, and files have descriptive and consistent names
New code must handle errors properly: log errors that cannot be resolved by the user with error level, log unusual conditions with warning level, log important background actions with info level, and provide user-facing messages for errors that the user can solve autonomously
Files:
openwisp_controller/connection/models.pyopenwisp_controller/connection/base/models.py
**/*.{js,ts,tsx,jsx,py,java,go,cs,rb,php,c,cpp,h,hpp,sql}
📄 CodeRabbit inference engine (Custom checks)
Flag obvious performance regressions, such as heavy loops, repeated I/O, or unoptimized queries
Files:
openwisp_controller/connection/models.pyopenwisp_controller/connection/base/models.py
**/*.{js,ts,tsx,jsx,py,java,go,cs,rb,php,c,cpp,h,hpp,sh,bash,sql}
📄 CodeRabbit inference engine (Custom checks)
Cryptic or non-obvious code (regex, complex bash commands, or hard-to-read code) must include a concise comment explaining why it is needed and why the complexity is acceptable
Files:
openwisp_controller/connection/models.pyopenwisp_controller/connection/base/models.py
**/*.{py,html}
📄 CodeRabbit inference engine (Custom checks)
For Django pull requests, ensure all user-facing strings are marked as translatable using the Django i18n framework
Files:
openwisp_controller/connection/models.pyopenwisp_controller/connection/base/models.py
**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
**/*.py: Mark user-facing strings for translation with Django i18n helpers in Django code
Avoid unnecessary blank lines inside function and method bodies
Be careful with authentication, authorization, queryset filtering, serializers, admin behavior, cache invalidation, signals, Celery tasks, and websocket updates in Django code
Preserve validation around templates, VPN/PKI material, SSH credentials, device commands, uploaded files, URLs, and subnet/IP data
Write comments and docstrings only when they explain why code is shaped a certain way, placing them before the relevant code block instead of scattering them inside it
Files:
openwisp_controller/connection/models.pyopenwisp_controller/connection/base/models.py
🧠 Learnings (5)
📚 Learning: 2026-02-24T16:24:55.443Z
Learnt from: nemesifier
Repo: openwisp/openwisp-controller PR: 1233
File: .github/workflows/backport.yml:22-22
Timestamp: 2026-02-24T16:24:55.443Z
Learning: In repositories within the OpenWISP organization, it is acceptable to reference reusable workflows from other OpenWISP-controlled repos using mutable refs (e.g., master) in .github/workflows. This is permissible due to the shared trust boundary within the organization. If applying this pattern, ensure the target repos are under the same organization and maintain awareness of potential breakages from upstream mutable refs; consider pinning to a tagged version for longer-term stability when appropriate.
Applied to files:
.github/workflows/ci.yml
📚 Learning: 2026-02-24T16:25:20.080Z
Learnt from: nemesifier
Repo: openwisp/openwisp-controller PR: 1233
File: .github/workflows/backport.yml:35-35
Timestamp: 2026-02-24T16:25:20.080Z
Learning: In .github/workflows/backport.yml, enforce that backport-on-comment triggers only for users with author_association MEMBE R or OWNER (COLLABORATOR excluded), reflecting maintainer feedback. Update the trigger condition to check author_association and restrict to MEMBERS/OWNERS; document rationale and PR `#1233` reference in code comments.
Applied to files:
.github/workflows/ci.yml
📚 Learning: 2026-01-15T15:05:49.557Z
Learnt from: DragnEmperor
Repo: openwisp/openwisp-controller PR: 1175
File: openwisp_controller/config/management/commands/clear_last_ip.py:38-42
Timestamp: 2026-01-15T15:05:49.557Z
Learning: In Django projects, when using select_related() to traverse relations (for example, select_related("organization__config_settings")), the traversed relation must not be deferred. If you also use .only() in the same query, include the relation name or FK field (e.g., "organization" or "organization_id") in the .only() list to avoid the error "Field X cannot be both deferred and traversed using select_related at the same time." Apply this guideline to Django code in openwisp_controller/config/management/commands/clear_last_ip.py and similar modules by ensuring any select_related with an accompanying only() includes the related field names to prevent deferred/traversed conflicts.
Applied to files:
openwisp_controller/connection/models.pyopenwisp_controller/connection/base/models.py
📚 Learning: 2026-02-17T19:13:10.088Z
Learnt from: nemesifier
Repo: openwisp/openwisp-controller PR: 1175
File: openwisp_controller/config/whois/commands.py:0-0
Timestamp: 2026-02-17T19:13:10.088Z
Learning: In reviews for the openwisp/openwisp-controller repository, do not propose changes based on Ruff warnings. The project does not use Ruff as its linter; ignore Ruff-related suggestions and follow the repository’s established linting and configuration rules. This guidance applies to all Python files under the openwisp_controller directory.
Applied to files:
openwisp_controller/connection/models.pyopenwisp_controller/connection/base/models.py
📚 Learning: 2026-01-15T15:07:17.354Z
Learnt from: DragnEmperor
Repo: openwisp/openwisp-controller PR: 1175
File: openwisp_controller/geo/estimated_location/tests/tests.py:172-175
Timestamp: 2026-01-15T15:07:17.354Z
Learning: In this repository, flake8 enforces E501 (line too long) via setup.cfg (max-line-length = 88) while ruff ignores E501 via ruff.toml. Therefore, use '# noqa: E501' on lines that intentionally exceed 88 characters to satisfy flake8 without affecting ruff checks. This applies to Python files across the project (any .py) and is relevant for tests as well. Use sparingly and only where breaking lines is not feasible without hurting readability or functionality.
Applied to files:
openwisp_controller/connection/models.pyopenwisp_controller/connection/base/models.py
🪛 zizmor (1.25.2)
.github/workflows/ci.yml
[warning] 4-14: insufficient job-level concurrency limits (concurrency-limits): workflow is missing concurrency setting
(concurrency-limits)
🔇 Additional comments (1)
.github/workflows/ci.yml (1)
8-8: LGTM!Also applies to: 14-14, 103-103
ac060dd to
cbe8ea6
Compare
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@openwisp_controller/connection/base/models.py`:
- Around line 844-845: The parent batch aggregation isn't recalculated after a
child command finishes; update the code so that whenever a child transitions out
of "in-progress" (i.e., at the end of AbstractCommand.execute() where the child
row is saved/finished) you explicitly refresh the parent by invoking its
calculate_and_update_status() (and save the parent if needed) so parent
counters/status reflect the new child state; apply the same change to the other
similar blocks referenced (around the 853-895 region) wherever child completion
only saves the child without touching the parent.
- Around line 738-741: The organization ForeignKey on the model is currently
required but must be nullable to support cross-organization batches; change the
organization field declaration (organization = models.ForeignKey(...)) to allow
null and blank (null=True, blank=True) and update any DB migration. Then update
resolve_devices (and any code that assumes organization is always present) to
load the Device model (Device = load_model("config", "Device")) and only filter
by organization when self.organization_id is truthy (e.g., if
self.organization_id: qs = qs.filter(organization=self.organization)); ensure
any callers/permissions that relied on a non-null organization handle the
nullable case safely.
- Around line 826-851: The launch path currently creates new Command records
every time launch() or launch_async() runs, causing duplicate commands; make it
idempotent by skipping devices that already have a Command tied to this batch.
In launch(), after resolving devices (resolve_devices()) load existing Commands
for this batch
(Command.objects.filter(batch_command=self).values_list('device_id', flat=True))
and only create Command instances for devices whose id is not in that set (or
use Command.objects.get_or_create(...) keyed by device + batch_command + type).
Update total_devices to reflect the count of devices with a Command for this
batch (existing + newly created) and continue to call
calculate_and_update_status(); ensure launch_async() remains unchanged except
for relying on the idempotent launch() behavior.
In `@openwisp_controller/connection/tasks.py`:
- Around line 104-109: The current except ObjectDoesNotExist wraps both the DB
lookup and batch.launch(), hiding errors raised inside BatchCommand.launch();
narrow the exception to only the lookup by enclosing only
BatchCommand.objects.get(pk=batch_id) in the try/except and handling the missing
object with the existing warning; call batch.launch() outside that except block
(and optionally add a separate try/except around batch.launch() to catch and log
unexpected exceptions including ObjectDoesNotExist at error level so failures in
BatchCommand.launch() are not misreported).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 35398c9a-4dec-42fc-b505-27427d8d6602
📒 Files selected for processing (3)
openwisp_controller/connection/base/models.pyopenwisp_controller/connection/models.pyopenwisp_controller/connection/tasks.py
📜 Review details
🧰 Additional context used
📓 Path-based instructions (5)
**/*.{js,ts,tsx,jsx,py,java,go,cs,rb,php,c,cpp,h,hpp}
📄 CodeRabbit inference engine (Custom checks)
**/*.{js,ts,tsx,jsx,py,java,go,cs,rb,php,c,cpp,h,hpp}: Flag potential security vulnerabilities in code
Avoid unnecessary comments or docstrings for code that is already clear
Code formatting is compact and readable. Do not add excessive blank lines, especially inside function or method bodies
Flag unused or redundant code
Ensure variables, functions, classes, and files have descriptive and consistent names
New code must handle errors properly: log errors that cannot be resolved by the user with error level, log unusual conditions with warning level, log important background actions with info level, and provide user-facing messages for errors that the user can solve autonomously
Files:
openwisp_controller/connection/models.pyopenwisp_controller/connection/tasks.pyopenwisp_controller/connection/base/models.py
**/*.{js,ts,tsx,jsx,py,java,go,cs,rb,php,c,cpp,h,hpp,sql}
📄 CodeRabbit inference engine (Custom checks)
Flag obvious performance regressions, such as heavy loops, repeated I/O, or unoptimized queries
Files:
openwisp_controller/connection/models.pyopenwisp_controller/connection/tasks.pyopenwisp_controller/connection/base/models.py
**/*.{js,ts,tsx,jsx,py,java,go,cs,rb,php,c,cpp,h,hpp,sh,bash,sql}
📄 CodeRabbit inference engine (Custom checks)
Cryptic or non-obvious code (regex, complex bash commands, or hard-to-read code) must include a concise comment explaining why it is needed and why the complexity is acceptable
Files:
openwisp_controller/connection/models.pyopenwisp_controller/connection/tasks.pyopenwisp_controller/connection/base/models.py
**/*.{py,html}
📄 CodeRabbit inference engine (Custom checks)
For Django pull requests, ensure all user-facing strings are marked as translatable using the Django i18n framework
Files:
openwisp_controller/connection/models.pyopenwisp_controller/connection/tasks.pyopenwisp_controller/connection/base/models.py
**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
**/*.py: Mark user-facing strings for translation with Django i18n helpers in Django code
Avoid unnecessary blank lines inside function and method bodies
Be careful with authentication, authorization, queryset filtering, serializers, admin behavior, cache invalidation, signals, Celery tasks, and websocket updates in Django code
Preserve validation around templates, VPN/PKI material, SSH credentials, device commands, uploaded files, URLs, and subnet/IP data
Write comments and docstrings only when they explain why code is shaped a certain way, placing them before the relevant code block instead of scattering them inside it
Files:
openwisp_controller/connection/models.pyopenwisp_controller/connection/tasks.pyopenwisp_controller/connection/base/models.py
🧠 Learnings (3)
📚 Learning: 2026-01-15T15:05:49.557Z
Learnt from: DragnEmperor
Repo: openwisp/openwisp-controller PR: 1175
File: openwisp_controller/config/management/commands/clear_last_ip.py:38-42
Timestamp: 2026-01-15T15:05:49.557Z
Learning: In Django projects, when using select_related() to traverse relations (for example, select_related("organization__config_settings")), the traversed relation must not be deferred. If you also use .only() in the same query, include the relation name or FK field (e.g., "organization" or "organization_id") in the .only() list to avoid the error "Field X cannot be both deferred and traversed using select_related at the same time." Apply this guideline to Django code in openwisp_controller/config/management/commands/clear_last_ip.py and similar modules by ensuring any select_related with an accompanying only() includes the related field names to prevent deferred/traversed conflicts.
Applied to files:
openwisp_controller/connection/models.pyopenwisp_controller/connection/tasks.pyopenwisp_controller/connection/base/models.py
📚 Learning: 2026-02-17T19:13:10.088Z
Learnt from: nemesifier
Repo: openwisp/openwisp-controller PR: 1175
File: openwisp_controller/config/whois/commands.py:0-0
Timestamp: 2026-02-17T19:13:10.088Z
Learning: In reviews for the openwisp/openwisp-controller repository, do not propose changes based on Ruff warnings. The project does not use Ruff as its linter; ignore Ruff-related suggestions and follow the repository’s established linting and configuration rules. This guidance applies to all Python files under the openwisp_controller directory.
Applied to files:
openwisp_controller/connection/models.pyopenwisp_controller/connection/tasks.pyopenwisp_controller/connection/base/models.py
📚 Learning: 2026-01-15T15:07:17.354Z
Learnt from: DragnEmperor
Repo: openwisp/openwisp-controller PR: 1175
File: openwisp_controller/geo/estimated_location/tests/tests.py:172-175
Timestamp: 2026-01-15T15:07:17.354Z
Learning: In this repository, flake8 enforces E501 (line too long) via setup.cfg (max-line-length = 88) while ruff ignores E501 via ruff.toml. Therefore, use '# noqa: E501' on lines that intentionally exceed 88 characters to satisfy flake8 without affecting ruff checks. This applies to Python files across the project (any .py) and is relevant for tests as well. Use sparingly and only where breaking lines is not feasible without hurting readability or functionality.
Applied to files:
openwisp_controller/connection/models.pyopenwisp_controller/connection/tasks.pyopenwisp_controller/connection/base/models.py
Migrations Check FailedHello @dee077, The CI build failed because of a migrations check. The error message indicates that model changes have not been migrated. To fix this, please run the following command in your local environment: ./manage.py makemigrationsThen, commit the generated migration files and push your changes again. |
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
openwisp_controller/connection/base/models.py (1)
733-739:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftThe batch status enum no longer matches the feature contract.
The PR objectives call out
partial successandno-target, but this enum cannot represent either. Downstream, mixed outcomes are collapsed intofailed, empty launches fall back toidle, andcancelledis unreachable because child commands never emit a cancelled state.Based on learnings from the PR objectives, batch execution must expose distinct aggregate states for partial success and no-target outcomes.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@openwisp_controller/connection/base/models.py` around lines 733 - 739, STATUS_CHOICES no longer reflects required aggregate states; add explicit choices for "partial-success" and "no-target" and update labels so they read e.g. ("partial-success", _("partial success")) and ("no-target", _("no target")) alongside existing values in STATUS_CHOICES. After adding the new constants, update any aggregation logic that computes the batch status from child command outcomes (the code that collapses mixed outcomes into "failed" and treats empty launches as "idle") to emit "partial-success" when there are mixed success/fail results and "no-target" when a launch had no target commands, and ensure "cancelled" remains reachable only if child commands can produce cancelled — adjust that aggregator function accordingly.
♻️ Duplicate comments (4)
openwisp_controller/connection/base/models.py (3)
566-567:⚠️ Potential issue | 🟠 Major | ⚡ Quick winThis hook still misses the real completion path.
calculate_and_update_status()only runs when a terminal child is persisted throughsave(), butAbstractCommand.execute()and the failure paths inopenwisp_controller/connection/tasks.pywrite terminal states via_save_without_resurrecting(). The batch therefore stays on its initial aggregate and never reflects completed children.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@openwisp_controller/connection/base/models.py` around lines 566 - 567, The post-save hook only calls batch_command.calculate_and_update_status() when a terminal child is persisted via save(), but terminal states written by AbstractCommand.execute() and the failure paths in openwisp_controller/connection/tasks.py use _save_without_resurrecting(), so the batch status never gets recalculated; update those code paths to invoke batch_command.calculate_and_update_status() after calling _save_without_resurrecting() (or modify _save_without_resurrecting() to trigger the same post-save behavior) so that when a Command model with batch_command_id transitions to a terminal state the batch aggregate is recalculated and persisted.
850-868:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy lift
launch()is still non-idempotent.A second
launch_async()delivery, manuallaunch(), or task retry will create a freshCommandrow for every target again, andCommand.save()schedules execution on create. For device commands, that turns a retry into duplicated commands sent to the same devices.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@openwisp_controller/connection/base/models.py` around lines 850 - 868, The launch() method currently creates a new Command for every device unconditionally, causing duplicate commands on retries; change it to be idempotent by checking for existing Commands tied to this batch before creating new ones: use the Command model to query existing commands with batch_command=self (and optionally matching device, type and input) and skip creation for devices that already have a Command, e.g., build a set of device ids from existing Command objects and only create Command objects for devices not in that set (or use get_or_create per device), then update total_devices to reflect the number of unique target devices (existing + newly created) and call calculate_and_update_status(); reference launch(), Command, resolve_devices(), device, batch_command, type, input, total_devices, calculate_and_update_status() when making the change.
743-744:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftCross-organization batches are only validated against the global allowlist.
When
organizationis null,clean()accepts any command enabled in__all__, butlaunch()later validates each childCommandagainst that device’s organization. A superuser batch spanning orgs with different command policies can therefore create/schedule some commands and then blow up mid-loop on the first disallowed device, leaving a partial batch behind.As per coding guidelines, "Preserve validation around templates, VPN/PKI material, SSH credentials, device commands, uploaded files, URLs, and subnet/IP data".
Also applies to: 817-823
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@openwisp_controller/connection/base/models.py` around lines 743 - 744, The Batch model's clean() currently trusts the global __all__ when organization is null, allowing cross-org batches to pass initial validation but fail in launch(), so update the clean() method to validate child Command entries against the actual device/organization policy before saving: when self.organization is None iterate through the related commands/devices (use the same checks/utility used by launch() — e.g., call Command.full_clean() or the command-permission validator used in launch) and raise ValidationError on any disallowed command so a cross-organization batch cannot be created; apply the same change to the similar validation block referenced around lines 817-823 to ensure both code paths enforce per-device organization command policies.Source: Coding guidelines
openwisp_controller/connection/tasks.py (1)
112-116:⚠️ Potential issue | 🟠 Major | ⚡ Quick winOnly catch the missing-batch lookup here.
The current
except ObjectDoesNotExistalso coversbatch.launch(), so any nestedDoesNotExistraised while resolving targets or creating child commands is misreported as “batch deleted” and the task exits cleanly with stale state.As per coding guidelines, "New code must handle errors properly: log errors that cannot be resolved by the user with error level, log unusual conditions with warning level, log important background actions with info level".
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@openwisp_controller/connection/tasks.py` around lines 112 - 116, The ObjectDoesNotExist except currently wraps both the lookup and batch.launch(), which hides DoesNotExist errors raised during launch; change the flow so only the lookup is guarded: perform BatchCommand.objects.get(pk=batch_id) inside a try/except catching ObjectDoesNotExist and call logger.warning(f"The BatchCommand object with id {batch_id} not found") in that except, then call batch.launch() outside that try block so any DoesNotExist from batch.launch() (or other errors) will propagate and be handled/logged at error level by the task runner.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@openwisp_controller/connection/api/views.py`:
- Around line 168-169: The code calls batch.resolve_devices() then builds
device_pks by instantiating full Device objects (resolved =
batch.resolve_devices(); device_pks = [str(d.pk) for d in resolved]) which loads
entire rows unnecessarily; change to obtain only primary keys (e.g., have
resolve_devices() return a queryset or iterable of pks or call values_list('pk',
flat=True) on the queryset) and build device_pks from those pks (or update
batch.resolve_devices to provide a resolve_device_pks() / return pks) so you
never instantiate full Device model instances when only IDs are needed.
- Around line 151-156: The POST flow creates a batch via serializer.save() and
then calls batch.launch_async(); if launch_async() raises, the code currently
neither logs nor returns a controlled error. Wrap the batch.launch_async() call
in a try/except, log the exception at error level with contextual details
(batch.pk, request.user or request.auth) and return a clear API error Response
(e.g., status.HTTP_500_INTERNAL_SERVER_ERROR or HTTP_503) describing that
asynchronous enqueue failed; optionally mark or update the batch state to
"failed" if a setter exists on the Batch model to avoid leaving it in an
indeterminate state. Ensure you reference the post() handler, serializer.save(),
batch.launch_async(), Response and status when making the change.
In
`@openwisp_controller/connection/migrations/0011_batchcommand_command_batch_command.py`:
- Around line 125-127: The migration's frozen model options ("Batch command
operation" / plural) no longer match the model's Meta on AbstractBatchCommand
which now uses "Batch command" / "Batch commands"; update the migration
(0011_batchcommand_command_batch_command.py) to set options to "verbose_name":
"Batch command" and "verbose_name_plural": "Batch commands" (or regenerate the
migration so the frozen Meta matches AbstractBatchCommand.Meta) to avoid an
immediate AlterModelOptions diff.
In `@openwisp_controller/connection/tasks.py`:
- Around line 84-91: Remove the demo print block that prints
command.batch_command_id/devices/status/output to stdout; instead, stop using
print() and (within the same task/function that references
command.batch_command_id) use the project's logger to record only non-sensitive,
minimal info (e.g., log an info-level message that a batch command completed and
a warning/error-level message on failures) and never dump command.output or
device credentials to stdout; reference the existing variables
command.batch_command_id, command.device.name, command.status, and
command.output when implementing the replacement and ensure you follow the
project's logging/error-handling conventions (mask or omit sensitive output and
log detailed errors at error level).
---
Outside diff comments:
In `@openwisp_controller/connection/base/models.py`:
- Around line 733-739: STATUS_CHOICES no longer reflects required aggregate
states; add explicit choices for "partial-success" and "no-target" and update
labels so they read e.g. ("partial-success", _("partial success")) and
("no-target", _("no target")) alongside existing values in STATUS_CHOICES. After
adding the new constants, update any aggregation logic that computes the batch
status from child command outcomes (the code that collapses mixed outcomes into
"failed" and treats empty launches as "idle") to emit "partial-success" when
there are mixed success/fail results and "no-target" when a launch had no target
commands, and ensure "cancelled" remains reachable only if child commands can
produce cancelled — adjust that aggregator function accordingly.
---
Duplicate comments:
In `@openwisp_controller/connection/base/models.py`:
- Around line 566-567: The post-save hook only calls
batch_command.calculate_and_update_status() when a terminal child is persisted
via save(), but terminal states written by AbstractCommand.execute() and the
failure paths in openwisp_controller/connection/tasks.py use
_save_without_resurrecting(), so the batch status never gets recalculated;
update those code paths to invoke batch_command.calculate_and_update_status()
after calling _save_without_resurrecting() (or modify
_save_without_resurrecting() to trigger the same post-save behavior) so that
when a Command model with batch_command_id transitions to a terminal state the
batch aggregate is recalculated and persisted.
- Around line 850-868: The launch() method currently creates a new Command for
every device unconditionally, causing duplicate commands on retries; change it
to be idempotent by checking for existing Commands tied to this batch before
creating new ones: use the Command model to query existing commands with
batch_command=self (and optionally matching device, type and input) and skip
creation for devices that already have a Command, e.g., build a set of device
ids from existing Command objects and only create Command objects for devices
not in that set (or use get_or_create per device), then update total_devices to
reflect the number of unique target devices (existing + newly created) and call
calculate_and_update_status(); reference launch(), Command, resolve_devices(),
device, batch_command, type, input, total_devices, calculate_and_update_status()
when making the change.
- Around line 743-744: The Batch model's clean() currently trusts the global
__all__ when organization is null, allowing cross-org batches to pass initial
validation but fail in launch(), so update the clean() method to validate child
Command entries against the actual device/organization policy before saving:
when self.organization is None iterate through the related commands/devices (use
the same checks/utility used by launch() — e.g., call Command.full_clean() or
the command-permission validator used in launch) and raise ValidationError on
any disallowed command so a cross-organization batch cannot be created; apply
the same change to the similar validation block referenced around lines 817-823
to ensure both code paths enforce per-device organization command policies.
In `@openwisp_controller/connection/tasks.py`:
- Around line 112-116: The ObjectDoesNotExist except currently wraps both the
lookup and batch.launch(), which hides DoesNotExist errors raised during launch;
change the flow so only the lookup is guarded: perform
BatchCommand.objects.get(pk=batch_id) inside a try/except catching
ObjectDoesNotExist and call logger.warning(f"The BatchCommand object with id
{batch_id} not found") in that except, then call batch.launch() outside that try
block so any DoesNotExist from batch.launch() (or other errors) will propagate
and be handled/logged at error level by the task runner.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 2a7daccb-35b2-4120-905b-86413be228fe
📒 Files selected for processing (6)
openwisp_controller/connection/api/serializers.pyopenwisp_controller/connection/api/urls.pyopenwisp_controller/connection/api/views.pyopenwisp_controller/connection/base/models.pyopenwisp_controller/connection/migrations/0011_batchcommand_command_batch_command.pyopenwisp_controller/connection/tasks.py
📜 Review details
⏰ 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). (9)
- GitHub Check: Python==3.11 | django~=4.2.0
- GitHub Check: Python==3.12 | django~=4.2.0
- GitHub Check: Python==3.10 | django~=5.1.0
- GitHub Check: Python==3.13 | django~=5.1.0
- GitHub Check: Python==3.10 | django~=5.2.0
- GitHub Check: Python==3.11 | django~=5.2.0
- GitHub Check: Python==3.12 | django~=5.2.0
- GitHub Check: Python==3.12 | django~=5.1.0
- GitHub Check: Python==3.11 | django~=5.1.0
🧰 Additional context used
📓 Path-based instructions (5)
**/*.{js,ts,tsx,jsx,py,java,go,cs,rb,php,c,cpp,h,hpp}
📄 CodeRabbit inference engine (Custom checks)
**/*.{js,ts,tsx,jsx,py,java,go,cs,rb,php,c,cpp,h,hpp}: Flag potential security vulnerabilities in code
Avoid unnecessary comments or docstrings for code that is already clear
Code formatting is compact and readable. Do not add excessive blank lines, especially inside function or method bodies
Flag unused or redundant code
Ensure variables, functions, classes, and files have descriptive and consistent names
New code must handle errors properly: log errors that cannot be resolved by the user with error level, log unusual conditions with warning level, log important background actions with info level, and provide user-facing messages for errors that the user can solve autonomously
Files:
openwisp_controller/connection/api/urls.pyopenwisp_controller/connection/api/views.pyopenwisp_controller/connection/tasks.pyopenwisp_controller/connection/migrations/0011_batchcommand_command_batch_command.pyopenwisp_controller/connection/api/serializers.pyopenwisp_controller/connection/base/models.py
**/*.{js,ts,tsx,jsx,py,java,go,cs,rb,php,c,cpp,h,hpp,sql}
📄 CodeRabbit inference engine (Custom checks)
Flag obvious performance regressions, such as heavy loops, repeated I/O, or unoptimized queries
Files:
openwisp_controller/connection/api/urls.pyopenwisp_controller/connection/api/views.pyopenwisp_controller/connection/tasks.pyopenwisp_controller/connection/migrations/0011_batchcommand_command_batch_command.pyopenwisp_controller/connection/api/serializers.pyopenwisp_controller/connection/base/models.py
**/*.{js,ts,tsx,jsx,py,java,go,cs,rb,php,c,cpp,h,hpp,sh,bash,sql}
📄 CodeRabbit inference engine (Custom checks)
Cryptic or non-obvious code (regex, complex bash commands, or hard-to-read code) must include a concise comment explaining why it is needed and why the complexity is acceptable
Files:
openwisp_controller/connection/api/urls.pyopenwisp_controller/connection/api/views.pyopenwisp_controller/connection/tasks.pyopenwisp_controller/connection/migrations/0011_batchcommand_command_batch_command.pyopenwisp_controller/connection/api/serializers.pyopenwisp_controller/connection/base/models.py
**/*.{py,html}
📄 CodeRabbit inference engine (Custom checks)
For Django pull requests, ensure all user-facing strings are marked as translatable using the Django i18n framework
Files:
openwisp_controller/connection/api/urls.pyopenwisp_controller/connection/api/views.pyopenwisp_controller/connection/tasks.pyopenwisp_controller/connection/migrations/0011_batchcommand_command_batch_command.pyopenwisp_controller/connection/api/serializers.pyopenwisp_controller/connection/base/models.py
**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
**/*.py: Mark user-facing strings for translation with Django i18n helpers in Django code
Avoid unnecessary blank lines inside function and method bodies
Be careful with authentication, authorization, queryset filtering, serializers, admin behavior, cache invalidation, signals, Celery tasks, and websocket updates in Django code
Preserve validation around templates, VPN/PKI material, SSH credentials, device commands, uploaded files, URLs, and subnet/IP data
Write comments and docstrings only when they explain why code is shaped a certain way, placing them before the relevant code block instead of scattering them inside it
Files:
openwisp_controller/connection/api/urls.pyopenwisp_controller/connection/api/views.pyopenwisp_controller/connection/tasks.pyopenwisp_controller/connection/migrations/0011_batchcommand_command_batch_command.pyopenwisp_controller/connection/api/serializers.pyopenwisp_controller/connection/base/models.py
🧠 Learnings (3)
📚 Learning: 2026-01-15T15:05:49.557Z
Learnt from: DragnEmperor
Repo: openwisp/openwisp-controller PR: 1175
File: openwisp_controller/config/management/commands/clear_last_ip.py:38-42
Timestamp: 2026-01-15T15:05:49.557Z
Learning: In Django projects, when using select_related() to traverse relations (for example, select_related("organization__config_settings")), the traversed relation must not be deferred. If you also use .only() in the same query, include the relation name or FK field (e.g., "organization" or "organization_id") in the .only() list to avoid the error "Field X cannot be both deferred and traversed using select_related at the same time." Apply this guideline to Django code in openwisp_controller/config/management/commands/clear_last_ip.py and similar modules by ensuring any select_related with an accompanying only() includes the related field names to prevent deferred/traversed conflicts.
Applied to files:
openwisp_controller/connection/api/urls.pyopenwisp_controller/connection/api/views.pyopenwisp_controller/connection/tasks.pyopenwisp_controller/connection/migrations/0011_batchcommand_command_batch_command.pyopenwisp_controller/connection/api/serializers.pyopenwisp_controller/connection/base/models.py
📚 Learning: 2026-02-17T19:13:10.088Z
Learnt from: nemesifier
Repo: openwisp/openwisp-controller PR: 1175
File: openwisp_controller/config/whois/commands.py:0-0
Timestamp: 2026-02-17T19:13:10.088Z
Learning: In reviews for the openwisp/openwisp-controller repository, do not propose changes based on Ruff warnings. The project does not use Ruff as its linter; ignore Ruff-related suggestions and follow the repository’s established linting and configuration rules. This guidance applies to all Python files under the openwisp_controller directory.
Applied to files:
openwisp_controller/connection/api/urls.pyopenwisp_controller/connection/api/views.pyopenwisp_controller/connection/tasks.pyopenwisp_controller/connection/migrations/0011_batchcommand_command_batch_command.pyopenwisp_controller/connection/api/serializers.pyopenwisp_controller/connection/base/models.py
📚 Learning: 2026-01-15T15:07:17.354Z
Learnt from: DragnEmperor
Repo: openwisp/openwisp-controller PR: 1175
File: openwisp_controller/geo/estimated_location/tests/tests.py:172-175
Timestamp: 2026-01-15T15:07:17.354Z
Learning: In this repository, flake8 enforces E501 (line too long) via setup.cfg (max-line-length = 88) while ruff ignores E501 via ruff.toml. Therefore, use '# noqa: E501' on lines that intentionally exceed 88 characters to satisfy flake8 without affecting ruff checks. This applies to Python files across the project (any .py) and is relevant for tests as well. Use sparingly and only where breaking lines is not feasible without hurting readability or functionality.
Applied to files:
openwisp_controller/connection/api/urls.pyopenwisp_controller/connection/api/views.pyopenwisp_controller/connection/tasks.pyopenwisp_controller/connection/migrations/0011_batchcommand_command_batch_command.pyopenwisp_controller/connection/api/serializers.pyopenwisp_controller/connection/base/models.py
🪛 ast-grep (0.43.0)
openwisp_controller/connection/migrations/0011_batchcommand_command_batch_command.py
[info] 55-65: use help_text to document model columns
Context: models.CharField(
choices=[
("idle", "idle"),
("in-progress", "in progress"),
("success", "completed successfully"),
("failed", "completed with some failures"),
("cancelled", "completed with some cancellations"),
],
default="idle",
max_length=12,
)
Note: Security best practice.
(model-help-text)
[info] 69-72: use help_text to document model columns
Context: models.CharField(
choices=openwisp_controller.connection.commands.get_command_choices,
max_length=16,
)
Note: Security best practice.
(model-help-text)
🔇 Additional comments (4)
openwisp_controller/connection/api/serializers.py (2)
15-15: LGTM!Also applies to: 47-50
125-127: PreserveAbstractBatchCommand.clean()auth/schema checks inBatchCommandExecuteSerializer
BatchCommandExecuteSerializerinherits from plainserializers.ModelSerializerandvalidate()returnsdatadirectly; meanwhileAbstractBatchCommand.clean()enforces org/group/location/device consistency plus org-allowedcommand_typeand JSON schema validation forcommand_input. If thoseclean()validations aren’t invoked duringserializer.save()inBatchCommandExecuteView.post(), this serializer path can bypass model-level authorization/schema enforcement (lines 125-173 inopenwisp_controller/connection/api/serializers.py).Suggested fix
class BatchCommandExecuteSerializer( - FilterSerializerByOrgManaged, serializers.ModelSerializer + FilterSerializerByOrgManaged, ValidatedModelSerializer ): @@ def validate(self, data): if ( not data.get("organization") and not self.context["request"].user.is_superuser ): raise serializers.ValidationError( _("Only superusers can execute batch commands without an organization.") ) if devices := data.get("devices"): org = data.get("organization") for device in devices: if org and device.organization_id != org.id: raise serializers.ValidationError( { "devices": _( "All devices must belong to the same organization." ) } ) - return data + return super().validate(data)openwisp_controller/connection/api/views.py (1)
194-194: LGTM!openwisp_controller/connection/api/urls.py (1)
43-47: LGTM!
Flake8, Migrations, and Test FailuresHello @dee077, There are three distinct failures in the CI logs:
|
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (3)
openwisp_controller/connection/tasks.py (2)
84-91:⚠️ Potential issue | 🟠 Major | ⚡ Quick winRemove the demo stdout dump before merge.
This prints device names and raw
command.outputfor batch runs straight to worker stdout. That can leak sensitive device data and bypasses the project's logging/error-handling conventions.As per coding guidelines, "New code must handle errors properly..." and "Preserve validation around ... SSH credentials, device commands...".
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@openwisp_controller/connection/tasks.py` around lines 84 - 91, Remove the entire debug print block that outputs sensitive device data to stdout. Delete the conditional block starting with the comment "Todo: Remove once demo is completed" that checks command.batch_command_id and all subsequent print statements that write device names and command output. This debug code bypasses proper logging conventions and leaks sensitive information, so it must be completely removed before merging.Source: Coding guidelines
112-117:⚠️ Potential issue | 🟠 Major | ⚡ Quick winOnly treat lookup failures as “batch deleted”.
except ObjectDoesNotExistcurrently wraps both Line 113 and Line 114. Ifcreate_commands()raises its ownObjectDoesNotExist, the worker logs that the batch was deleted and exits cleanly, which hides the real failure and leaves the batch state stale.As per coding guidelines, "New code must handle errors properly: log errors that cannot be resolved by the user with error level...".
In Django, is ObjectDoesNotExist the base class for model-specific DoesNotExist exceptions, and would `except ObjectDoesNotExist` also catch `DoesNotExist` raised inside a later method call in the same try block?🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@openwisp_controller/connection/tasks.py` around lines 112 - 117, The except ObjectDoesNotExist handler currently catches exceptions from both the BatchCommand.objects.get() call and the batch.create_commands() call. If create_commands() raises ObjectDoesNotExist for a reason unrelated to the batch lookup, it gets misidentified as a deleted batch and hides the real failure. Restructure the exception handling so that only the get() call on line 113 is protected by the except ObjectDoesNotExist clause, while the create_commands() call on line 114 is either moved outside the try block or handled separately with proper error-level logging according to coding guidelines. Also fix the typo "foound" to "found" in the warning message.Source: Coding guidelines
openwisp_controller/connection/base/models.py (1)
890-904:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy lift
create_commands()still duplicates work on rerun.Every invocation creates a fresh
Commandfor every resolved device, and Line 903 immediately schedules execution viaCommand.save(). A Celery redelivery or a second manual call will enqueue duplicate commands for the same(batch_command, device)pair unless existing rows are filtered out first.Based on the batch orchestration flow in this PR, retries need to be idempotent before child commands are created.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@openwisp_controller/connection/base/models.py` around lines 890 - 904, The create_commands() method creates duplicate Command objects on rerun because it does not check for existing commands before creating new ones for each device. Before creating a Command in the loop over self.resolve_devices().iterator(), query the Command model to check if a command already exists for the current (batch_command, device) pair. Only instantiate and save a new Command if one does not already exist for that combination. This ensures the method is idempotent and safe for Celery redeliveries or manual reruns without generating duplicates.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@openwisp_controller/connection/api/views.py`:
- Around line 166-173: The dry-run endpoint in the BatchCommand.dry_run response
handling is materializing and returning the entire list of resolved device UUIDs
without any bounds, which creates performance issues for mass targets like
execute_all or org-wide previews. Instead of returning all devices in the
data["devices"] field, modify the response to include a device count and a
capped or paginated sample of device UUIDs. This prevents unbounded payload
sizes while still providing sufficient preview information to the user.
- Around line 157-160: The ValidationError exception handler is only returning
the first error message by accessing e.messages[0], which loses field names and
additional validation errors from the full_clean() call. This creates an
inconsistent 400 response contract compared to serializer validation. Instead of
flattening to a single error string, return the complete validation error
payload that preserves all field names and error details. This fix should be
applied to both the exception handler in the execute method (lines 157-160) and
the corresponding exception handler in the dry_run method (lines 168-171) to
ensure consistent error responses across both code paths.
In `@openwisp_controller/connection/base/models.py`:
- Around line 894-905: The issue is that when command.full_clean() raises a
ValidationError, the command is never saved, causing the device to be dropped
from batch aggregates including total_devices, successful, failed, and
calculate_and_update_status(). This violates the requirement to preserve
validation around device commands and expose accurate batch status aggregated
from all targeted devices. Instead of skipping the device entirely, modify the
exception handling in the loop over self.resolve_devices().iterator() to persist
the command with an appropriate status (such as failed or invalid) even when
full_clean() fails, ensuring all targeted devices are represented in the batch
aggregates and the batch status accurately reflects the execution outcome across
all intended targets.
- Around line 868-888: The `execute()` and `dry_run()` classmethod
implementations have an authorization gap where explicitly provided devices
bypass organization validation. The `devices_list` parameter is validated after
`full_clean()` is called, but the organization validation in the `clean()`
method requires `self.pk` to exist, allowing mismatched devices to bypass
checks. Before calling `full_clean()` in both methods, add explicit validation
of the `devices_list` parameter (if provided) to ensure all devices belong to
the same organization as the batch. This ensures authorization checks are
performed on user-provided devices before any instance is persisted.
---
Duplicate comments:
In `@openwisp_controller/connection/base/models.py`:
- Around line 890-904: The create_commands() method creates duplicate Command
objects on rerun because it does not check for existing commands before creating
new ones for each device. Before creating a Command in the loop over
self.resolve_devices().iterator(), query the Command model to check if a command
already exists for the current (batch_command, device) pair. Only instantiate
and save a new Command if one does not already exist for that combination. This
ensures the method is idempotent and safe for Celery redeliveries or manual
reruns without generating duplicates.
In `@openwisp_controller/connection/tasks.py`:
- Around line 84-91: Remove the entire debug print block that outputs sensitive
device data to stdout. Delete the conditional block starting with the comment
"Todo: Remove once demo is completed" that checks command.batch_command_id and
all subsequent print statements that write device names and command output. This
debug code bypasses proper logging conventions and leaks sensitive information,
so it must be completely removed before merging.
- Around line 112-117: The except ObjectDoesNotExist handler currently catches
exceptions from both the BatchCommand.objects.get() call and the
batch.create_commands() call. If create_commands() raises ObjectDoesNotExist for
a reason unrelated to the batch lookup, it gets misidentified as a deleted batch
and hides the real failure. Restructure the exception handling so that only the
get() call on line 113 is protected by the except ObjectDoesNotExist clause,
while the create_commands() call on line 114 is either moved outside the try
block or handled separately with proper error-level logging according to coding
guidelines. Also fix the typo "foound" to "found" in the warning message.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 50afe5cb-bf73-45c0-b66d-723c6793776e
📒 Files selected for processing (5)
openwisp_controller/connection/api/serializers.pyopenwisp_controller/connection/api/views.pyopenwisp_controller/connection/base/models.pyopenwisp_controller/connection/migrations/0011_batchcommand_command_batch_command.pyopenwisp_controller/connection/tasks.py
📜 Review details
⏰ 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). (11)
- GitHub Check: Python==3.12 | django~=5.1.0
- GitHub Check: Python==3.13 | django~=5.2.0
- GitHub Check: Python==3.11 | django~=4.2.0
- GitHub Check: Python==3.12 | django~=5.2.0
- GitHub Check: Python==3.11 | django~=5.2.0
- GitHub Check: Python==3.10 | django~=4.2.0
- GitHub Check: Python==3.10 | django~=5.2.0
- GitHub Check: Python==3.10 | django~=5.1.0
- GitHub Check: Python==3.11 | django~=5.1.0
- GitHub Check: Python==3.12 | django~=4.2.0
- GitHub Check: Python==3.13 | django~=5.1.0
🧰 Additional context used
📓 Path-based instructions (5)
**/*.{js,ts,tsx,jsx,py,java,go,cs,rb,php,c,cpp,h,hpp}
📄 CodeRabbit inference engine (Custom checks)
**/*.{js,ts,tsx,jsx,py,java,go,cs,rb,php,c,cpp,h,hpp}: Flag potential security vulnerabilities in code
Avoid unnecessary comments or docstrings for code that is already clear
Code formatting is compact and readable. Do not add excessive blank lines, especially inside function or method bodies
Flag unused or redundant code
Ensure variables, functions, classes, and files have descriptive and consistent names
New code must handle errors properly: log errors that cannot be resolved by the user with error level, log unusual conditions with warning level, log important background actions with info level, and provide user-facing messages for errors that the user can solve autonomously
Files:
openwisp_controller/connection/tasks.pyopenwisp_controller/connection/api/views.pyopenwisp_controller/connection/migrations/0011_batchcommand_command_batch_command.pyopenwisp_controller/connection/api/serializers.pyopenwisp_controller/connection/base/models.py
**/*.{js,ts,tsx,jsx,py,java,go,cs,rb,php,c,cpp,h,hpp,sql}
📄 CodeRabbit inference engine (Custom checks)
Flag obvious performance regressions, such as heavy loops, repeated I/O, or unoptimized queries
Files:
openwisp_controller/connection/tasks.pyopenwisp_controller/connection/api/views.pyopenwisp_controller/connection/migrations/0011_batchcommand_command_batch_command.pyopenwisp_controller/connection/api/serializers.pyopenwisp_controller/connection/base/models.py
**/*.{js,ts,tsx,jsx,py,java,go,cs,rb,php,c,cpp,h,hpp,sh,bash,sql}
📄 CodeRabbit inference engine (Custom checks)
Cryptic or non-obvious code (regex, complex bash commands, or hard-to-read code) must include a concise comment explaining why it is needed and why the complexity is acceptable
Files:
openwisp_controller/connection/tasks.pyopenwisp_controller/connection/api/views.pyopenwisp_controller/connection/migrations/0011_batchcommand_command_batch_command.pyopenwisp_controller/connection/api/serializers.pyopenwisp_controller/connection/base/models.py
**/*.{py,html}
📄 CodeRabbit inference engine (Custom checks)
For Django pull requests, ensure all user-facing strings are marked as translatable using the Django i18n framework
Files:
openwisp_controller/connection/tasks.pyopenwisp_controller/connection/api/views.pyopenwisp_controller/connection/migrations/0011_batchcommand_command_batch_command.pyopenwisp_controller/connection/api/serializers.pyopenwisp_controller/connection/base/models.py
**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
**/*.py: Mark user-facing strings for translation with Django i18n helpers in Django code
Avoid unnecessary blank lines inside function and method bodies
Be careful with authentication, authorization, queryset filtering, serializers, admin behavior, cache invalidation, signals, Celery tasks, and websocket updates in Django code
Preserve validation around templates, VPN/PKI material, SSH credentials, device commands, uploaded files, URLs, and subnet/IP data
Write comments and docstrings only when they explain why code is shaped a certain way, placing them before the relevant code block instead of scattering them inside it
Files:
openwisp_controller/connection/tasks.pyopenwisp_controller/connection/api/views.pyopenwisp_controller/connection/migrations/0011_batchcommand_command_batch_command.pyopenwisp_controller/connection/api/serializers.pyopenwisp_controller/connection/base/models.py
🧠 Learnings (3)
📚 Learning: 2026-01-15T15:05:49.557Z
Learnt from: DragnEmperor
Repo: openwisp/openwisp-controller PR: 1175
File: openwisp_controller/config/management/commands/clear_last_ip.py:38-42
Timestamp: 2026-01-15T15:05:49.557Z
Learning: In Django projects, when using select_related() to traverse relations (for example, select_related("organization__config_settings")), the traversed relation must not be deferred. If you also use .only() in the same query, include the relation name or FK field (e.g., "organization" or "organization_id") in the .only() list to avoid the error "Field X cannot be both deferred and traversed using select_related at the same time." Apply this guideline to Django code in openwisp_controller/config/management/commands/clear_last_ip.py and similar modules by ensuring any select_related with an accompanying only() includes the related field names to prevent deferred/traversed conflicts.
Applied to files:
openwisp_controller/connection/tasks.pyopenwisp_controller/connection/api/views.pyopenwisp_controller/connection/migrations/0011_batchcommand_command_batch_command.pyopenwisp_controller/connection/api/serializers.pyopenwisp_controller/connection/base/models.py
📚 Learning: 2026-02-17T19:13:10.088Z
Learnt from: nemesifier
Repo: openwisp/openwisp-controller PR: 1175
File: openwisp_controller/config/whois/commands.py:0-0
Timestamp: 2026-02-17T19:13:10.088Z
Learning: In reviews for the openwisp/openwisp-controller repository, do not propose changes based on Ruff warnings. The project does not use Ruff as its linter; ignore Ruff-related suggestions and follow the repository’s established linting and configuration rules. This guidance applies to all Python files under the openwisp_controller directory.
Applied to files:
openwisp_controller/connection/tasks.pyopenwisp_controller/connection/api/views.pyopenwisp_controller/connection/migrations/0011_batchcommand_command_batch_command.pyopenwisp_controller/connection/api/serializers.pyopenwisp_controller/connection/base/models.py
📚 Learning: 2026-01-15T15:07:17.354Z
Learnt from: DragnEmperor
Repo: openwisp/openwisp-controller PR: 1175
File: openwisp_controller/geo/estimated_location/tests/tests.py:172-175
Timestamp: 2026-01-15T15:07:17.354Z
Learning: In this repository, flake8 enforces E501 (line too long) via setup.cfg (max-line-length = 88) while ruff ignores E501 via ruff.toml. Therefore, use '# noqa: E501' on lines that intentionally exceed 88 characters to satisfy flake8 without affecting ruff checks. This applies to Python files across the project (any .py) and is relevant for tests as well. Use sparingly and only where breaking lines is not feasible without hurting readability or functionality.
Applied to files:
openwisp_controller/connection/tasks.pyopenwisp_controller/connection/api/views.pyopenwisp_controller/connection/migrations/0011_batchcommand_command_batch_command.pyopenwisp_controller/connection/api/serializers.pyopenwisp_controller/connection/base/models.py
🔇 Additional comments (2)
openwisp_controller/connection/api/views.py (1)
155-161: Enqueue failures are still unhandled in the POST path.
BatchCommand.execute()persists the batch before scheduling the Celery dispatch, so a broker/on-commit failure here still bubbles as a 500 after the batch has been created.openwisp_controller/connection/api/serializers.py (1)
139-183: LGTM!
CI Failures: Code Style and TestsHello @dee077,
|
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
openwisp_controller/connection/base/models.py (1)
891-893:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftStrengthen idempotency to handle retries and partial runs safely.
Line 892 returns as soon as any child
Commandexists. That makes retries skip missing devices after partial failures, and concurrent workers can still pass this pre-check and create duplicates (TOCTOU). Deduplicate per device within a transaction/lock instead of short-circuiting the entire batch.Suggested fix
def create_commands(self): Command = load_model("connection", "Command") - if Command.objects.filter(batch_command=self).exists(): - return + with transaction.atomic(): + batch = self.__class__.objects.select_for_update().get(pk=self.pk) + existing_device_ids = set( + Command.objects.filter(batch_command=batch).values_list( + "device_id", flat=True + ) + ) - self.status = "in-progress" - self.save() - for device in self.resolve_devices().iterator(): + batch.status = "in-progress" + batch.save(update_fields=["status"]) + for device in self.resolve_devices().exclude(pk__in=existing_device_ids).iterator(): command = Command( device=device, type=self.command_type, input=self.command_input, batch_command=self, )🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@openwisp_controller/connection/base/models.py` around lines 891 - 893, The early return in the batch_command method (when checking if any Command exists) is not properly idempotent and allows race conditions where concurrent workers or retries after partial failures can create duplicates. Instead of returning early when any child Command exists, implement per-device deduplication within a transaction or lock. This ensures that if some Commands already exist for certain devices, they won't be recreated on retry, while missing Commands for other devices will be created, and concurrent workers cannot bypass the check and create duplicates.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@openwisp_controller/connection/base/models.py`:
- Around line 907-910: The code directly serializes the exception into both the
command output and log message, which can expose sensitive validation payloads.
Replace the raw exception string in the command.output assignment (the `str(e)`
part) with a generic failure message like "Command validation failed".
Similarly, modify the logger.warning call to exclude the raw exception `{e}` and
instead log only non-sensitive identifiers (such as the device.pk and batch
self.pk that are already included). Extract only field names or error types from
the ValidationError if needed for debugging, but never include the actual
submitted values or detailed validation messages that could contain sensitive
command data.
---
Duplicate comments:
In `@openwisp_controller/connection/base/models.py`:
- Around line 891-893: The early return in the batch_command method (when
checking if any Command exists) is not properly idempotent and allows race
conditions where concurrent workers or retries after partial failures can create
duplicates. Instead of returning early when any child Command exists, implement
per-device deduplication within a transaction or lock. This ensures that if some
Commands already exist for certain devices, they won't be recreated on retry,
while missing Commands for other devices will be created, and concurrent workers
cannot bypass the check and create duplicates.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 4fea12c9-1fda-4a74-94d0-e2c93a2be9aa
📒 Files selected for processing (4)
openwisp_controller/connection/api/views.pyopenwisp_controller/connection/base/models.pyopenwisp_controller/connection/migrations/0011_batchcommand_command_batch_command.pyopenwisp_controller/connection/tasks.py
📜 Review details
⏰ 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). (11)
- GitHub Check: Python==3.13 | django~=5.1.0
- GitHub Check: Python==3.12 | django~=5.1.0
- GitHub Check: Python==3.11 | django~=5.1.0
- GitHub Check: Python==3.10 | django~=5.2.0
- GitHub Check: Python==3.12 | django~=4.2.0
- GitHub Check: Python==3.12 | django~=5.2.0
- GitHub Check: Python==3.11 | django~=5.2.0
- GitHub Check: Python==3.11 | django~=4.2.0
- GitHub Check: Python==3.10 | django~=5.1.0
- GitHub Check: Python==3.10 | django~=4.2.0
- GitHub Check: Python==3.13 | django~=5.2.0
🧰 Additional context used
📓 Path-based instructions (5)
**/*.{js,ts,tsx,jsx,py,java,go,cs,rb,php,c,cpp,h,hpp}
📄 CodeRabbit inference engine (Custom checks)
**/*.{js,ts,tsx,jsx,py,java,go,cs,rb,php,c,cpp,h,hpp}: Flag potential security vulnerabilities in code
Avoid unnecessary comments or docstrings for code that is already clear
Code formatting is compact and readable. Do not add excessive blank lines, especially inside function or method bodies
Flag unused or redundant code
Ensure variables, functions, classes, and files have descriptive and consistent names
New code must handle errors properly: log errors that cannot be resolved by the user with error level, log unusual conditions with warning level, log important background actions with info level, and provide user-facing messages for errors that the user can solve autonomously
Files:
openwisp_controller/connection/tasks.pyopenwisp_controller/connection/migrations/0011_batchcommand_command_batch_command.pyopenwisp_controller/connection/base/models.pyopenwisp_controller/connection/api/views.py
**/*.{js,ts,tsx,jsx,py,java,go,cs,rb,php,c,cpp,h,hpp,sql}
📄 CodeRabbit inference engine (Custom checks)
Flag obvious performance regressions, such as heavy loops, repeated I/O, or unoptimized queries
Files:
openwisp_controller/connection/tasks.pyopenwisp_controller/connection/migrations/0011_batchcommand_command_batch_command.pyopenwisp_controller/connection/base/models.pyopenwisp_controller/connection/api/views.py
**/*.{js,ts,tsx,jsx,py,java,go,cs,rb,php,c,cpp,h,hpp,sh,bash,sql}
📄 CodeRabbit inference engine (Custom checks)
Cryptic or non-obvious code (regex, complex bash commands, or hard-to-read code) must include a concise comment explaining why it is needed and why the complexity is acceptable
Files:
openwisp_controller/connection/tasks.pyopenwisp_controller/connection/migrations/0011_batchcommand_command_batch_command.pyopenwisp_controller/connection/base/models.pyopenwisp_controller/connection/api/views.py
**/*.{py,html}
📄 CodeRabbit inference engine (Custom checks)
For Django pull requests, ensure all user-facing strings are marked as translatable using the Django i18n framework
Files:
openwisp_controller/connection/tasks.pyopenwisp_controller/connection/migrations/0011_batchcommand_command_batch_command.pyopenwisp_controller/connection/base/models.pyopenwisp_controller/connection/api/views.py
**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
**/*.py: Mark user-facing strings for translation with Django i18n helpers in Django code
Avoid unnecessary blank lines inside function and method bodies
Be careful with authentication, authorization, queryset filtering, serializers, admin behavior, cache invalidation, signals, Celery tasks, and websocket updates in Django code
Preserve validation around templates, VPN/PKI material, SSH credentials, device commands, uploaded files, URLs, and subnet/IP data
Write comments and docstrings only when they explain why code is shaped a certain way, placing them before the relevant code block instead of scattering them inside it
Files:
openwisp_controller/connection/tasks.pyopenwisp_controller/connection/migrations/0011_batchcommand_command_batch_command.pyopenwisp_controller/connection/base/models.pyopenwisp_controller/connection/api/views.py
🧠 Learnings (3)
📚 Learning: 2026-01-15T15:05:49.557Z
Learnt from: DragnEmperor
Repo: openwisp/openwisp-controller PR: 1175
File: openwisp_controller/config/management/commands/clear_last_ip.py:38-42
Timestamp: 2026-01-15T15:05:49.557Z
Learning: In Django projects, when using select_related() to traverse relations (for example, select_related("organization__config_settings")), the traversed relation must not be deferred. If you also use .only() in the same query, include the relation name or FK field (e.g., "organization" or "organization_id") in the .only() list to avoid the error "Field X cannot be both deferred and traversed using select_related at the same time." Apply this guideline to Django code in openwisp_controller/config/management/commands/clear_last_ip.py and similar modules by ensuring any select_related with an accompanying only() includes the related field names to prevent deferred/traversed conflicts.
Applied to files:
openwisp_controller/connection/tasks.pyopenwisp_controller/connection/migrations/0011_batchcommand_command_batch_command.pyopenwisp_controller/connection/base/models.pyopenwisp_controller/connection/api/views.py
📚 Learning: 2026-02-17T19:13:10.088Z
Learnt from: nemesifier
Repo: openwisp/openwisp-controller PR: 1175
File: openwisp_controller/config/whois/commands.py:0-0
Timestamp: 2026-02-17T19:13:10.088Z
Learning: In reviews for the openwisp/openwisp-controller repository, do not propose changes based on Ruff warnings. The project does not use Ruff as its linter; ignore Ruff-related suggestions and follow the repository’s established linting and configuration rules. This guidance applies to all Python files under the openwisp_controller directory.
Applied to files:
openwisp_controller/connection/tasks.pyopenwisp_controller/connection/migrations/0011_batchcommand_command_batch_command.pyopenwisp_controller/connection/base/models.pyopenwisp_controller/connection/api/views.py
📚 Learning: 2026-01-15T15:07:17.354Z
Learnt from: DragnEmperor
Repo: openwisp/openwisp-controller PR: 1175
File: openwisp_controller/geo/estimated_location/tests/tests.py:172-175
Timestamp: 2026-01-15T15:07:17.354Z
Learning: In this repository, flake8 enforces E501 (line too long) via setup.cfg (max-line-length = 88) while ruff ignores E501 via ruff.toml. Therefore, use '# noqa: E501' on lines that intentionally exceed 88 characters to satisfy flake8 without affecting ruff checks. This applies to Python files across the project (any .py) and is relevant for tests as well. Use sparingly and only where breaking lines is not feasible without hurting readability or functionality.
Applied to files:
openwisp_controller/connection/tasks.pyopenwisp_controller/connection/migrations/0011_batchcommand_command_batch_command.pyopenwisp_controller/connection/base/models.pyopenwisp_controller/connection/api/views.py
🔇 Additional comments (3)
openwisp_controller/connection/migrations/0011_batchcommand_command_batch_command.py (1)
3-4: LGTM!Also applies to: 12-13
openwisp_controller/connection/tasks.py (1)
114-117: LGTM!openwisp_controller/connection/api/views.py (1)
159-160: LGTM!Also applies to: 171-172
Flake8 Linting FailuresHello @dee077, There is a
|
|
The CI is failing due to transient infrastructure issues (not related to your code). I have restarted the failed jobs automatically (1/3). |
Test Failures in CIHello @dee077, There are test failures in the CI pipeline.
|
|
The CI is failing due to transient infrastructure issues (not related to your code). I have restarted the failed jobs automatically (1/3). |
|
The CI is failing due to transient infrastructure issues (not related to your code). I have restarted the failed jobs automatically (2/3). |
pandafy
left a comment
pandafy
left a comment
There was a problem hiding this comment.
I believe the current state of PR is no up-to-date with your development. I'd some time today so went ahead to review the model and API definitions.
Some of these comments might be already outdated, which is fine. I added them so keep a track of the decisions. Also, coderabbit will help us flag if anything gets missed when the PR becomes huge.
0d7c392 to
65f73d0
Compare
|
coverage: 98.515% (+0.04%) from 98.477% — feature/1344-mass-command-execution-pipeline into gsoc26-mass-commands |
Migrations and User API FailuresHello @dee077, There are two main issues in your CI run:
|
4d9f667 to
e9bf89f
Compare
Multiple Test Failures and Code Style IssuesHello @dee077, There are multiple test failures and potential code style issues in your commit:
|
✅ Action performedFull review finished. |
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@openwisp_controller/connection/api/serializers.py`:
- Around line 272-275: Remove the duplicate change_password password-masking
block from BatchCommandDetailSerializer.to_representation; rely on
super().to_representation(instance) inherited from BatchCommandSerializer to
apply the existing redaction.
In `@openwisp_controller/connection/base/models.py`:
- Around line 918-919: Redact the password carried by change_password batches
before execute() returns or the batch is serialized, eliminating the interval
where API responses expose plaintext before the Celery cleanup task runs. Update
the batch creation/return flow around launch_batch_command and the corresponding
locations at the additional affected call sites, while preserving existing
validation for templates, VPN/PKI material, SSH credentials, device commands,
uploaded files, URLs, and subnet/IP data.
- Around line 974-979: Remove the redundant command.full_clean() call from the
flow surrounding AbstractCommand.save(), allowing save() to perform validation
once for new child commands while preserving the transaction.atomic() and
command.save() behavior.
In `@openwisp_controller/connection/tests/test_tasks.py`:
- Around line 386-431: Update test_launch_batch_command_timeout and
test_launch_batch_command_exception to create a change_password batch with
password-containing input, exercising the persistence branch when
create_commands raises SoftTimeLimitExceeded or RuntimeError. Assert that the
stored batch input does not retain the raw password after each failure while
preserving the existing failed-status and exception-log assertions.
In `@tests/openwisp2/sample_connection/api/views.py`:
- Around line 1-9: Consolidate the three imports from
openwisp_controller.connection.api.views into one parenthesized import block,
preserving the existing aliases BaseBatchCommandDetailView,
BaseBatchCommandExecuteView, and BaseBatchCommandListView.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 9dfce40a-6630-498f-a8a3-df46dd16816e
📒 Files selected for processing (24)
docs/developer/extending.rstdocs/user/intro.rstdocs/user/rest-api.rstdocs/user/shell-commands.rstopenwisp_controller/connection/api/serializers.pyopenwisp_controller/connection/api/urls.pyopenwisp_controller/connection/api/views.pyopenwisp_controller/connection/base/models.pyopenwisp_controller/connection/migrations/0011_batchcommand_command_batch_command.pyopenwisp_controller/connection/migrations/__init__.pyopenwisp_controller/connection/models.pyopenwisp_controller/connection/tasks.pyopenwisp_controller/connection/tests/pytest.pyopenwisp_controller/connection/tests/test_api.pyopenwisp_controller/connection/tests/test_models.pyopenwisp_controller/connection/tests/test_selenium.pyopenwisp_controller/connection/tests/test_tasks.pyopenwisp_controller/connection/tests/utils.pyopenwisp_controller/geo/estimated_location/tests/tests.pyopenwisp_controller/geo/tests/test_api.pytests/openwisp2/sample_connection/api/views.pytests/openwisp2/sample_connection/migrations/0005_batchcommand_command_batch_command.pytests/openwisp2/sample_connection/models.pytests/openwisp2/settings.py
📜 Review details
⏰ Context from checks skipped due to timeout. (11)
- GitHub Check: Python==3.10 | django~=5.1.0
- GitHub Check: Python==3.13 | django~=5.2.0
- GitHub Check: Python==3.11 | django~=5.1.0
- GitHub Check: Python==3.10 | django~=4.2.0
- GitHub Check: Python==3.10 | django~=5.2.0
- GitHub Check: Python==3.11 | django~=5.2.0
- GitHub Check: Python==3.12 | django~=5.1.0
- GitHub Check: Python==3.11 | django~=4.2.0
- GitHub Check: Python==3.12 | django~=4.2.0
- GitHub Check: Python==3.12 | django~=5.2.0
- GitHub Check: Python==3.13 | django~=5.1.0
🧰 Additional context used
📓 Path-based instructions (2)
**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
**/*.py: Mark user-facing strings for translation with Django i18n helpers in Django code
Avoid unnecessary blank lines inside function and method bodies
Be careful with authentication, authorization, queryset filtering, serializers, admin behavior, cache invalidation, signals, Celery tasks, and websocket updates in Django code
Preserve validation around templates, VPN/PKI material, SSH credentials, device commands, uploaded files, URLs, and subnet/IP data
Write comments and docstrings only when they explain why code is shaped a certain way, placing them before the relevant code block instead of scattering them inside it
Files:
openwisp_controller/connection/tests/pytest.pyopenwisp_controller/geo/estimated_location/tests/tests.pytests/openwisp2/settings.pyopenwisp_controller/connection/tests/test_selenium.pyopenwisp_controller/connection/api/urls.pyopenwisp_controller/geo/tests/test_api.pyopenwisp_controller/connection/tests/utils.pytests/openwisp2/sample_connection/models.pyopenwisp_controller/connection/tasks.pyopenwisp_controller/connection/migrations/0011_batchcommand_command_batch_command.pyopenwisp_controller/connection/models.pyopenwisp_controller/connection/migrations/__init__.pyopenwisp_controller/connection/api/serializers.pyopenwisp_controller/connection/tests/test_tasks.pytests/openwisp2/sample_connection/api/views.pyopenwisp_controller/connection/tests/test_models.pytests/openwisp2/sample_connection/migrations/0005_batchcommand_command_batch_command.pyopenwisp_controller/connection/api/views.pyopenwisp_controller/connection/base/models.pyopenwisp_controller/connection/tests/test_api.py
**/*.{py,html}
📄 CodeRabbit inference engine (Custom checks)
For Django pull requests, ensure all user-facing strings are marked as translatable using the Django i18n framework.
Files:
openwisp_controller/connection/tests/pytest.pyopenwisp_controller/geo/estimated_location/tests/tests.pytests/openwisp2/settings.pyopenwisp_controller/connection/tests/test_selenium.pyopenwisp_controller/connection/api/urls.pyopenwisp_controller/geo/tests/test_api.pyopenwisp_controller/connection/tests/utils.pytests/openwisp2/sample_connection/models.pyopenwisp_controller/connection/tasks.pyopenwisp_controller/connection/migrations/0011_batchcommand_command_batch_command.pyopenwisp_controller/connection/models.pyopenwisp_controller/connection/migrations/__init__.pyopenwisp_controller/connection/api/serializers.pyopenwisp_controller/connection/tests/test_tasks.pytests/openwisp2/sample_connection/api/views.pyopenwisp_controller/connection/tests/test_models.pytests/openwisp2/sample_connection/migrations/0005_batchcommand_command_batch_command.pyopenwisp_controller/connection/api/views.pyopenwisp_controller/connection/base/models.pyopenwisp_controller/connection/tests/test_api.py
🧠 Learnings (9)
📚 Learning: 2026-01-15T15:05:49.557Z
Learnt from: DragnEmperor
Repo: openwisp/openwisp-controller PR: 1175
File: openwisp_controller/config/management/commands/clear_last_ip.py:38-42
Timestamp: 2026-01-15T15:05:49.557Z
Learning: In Django projects, when using select_related() to traverse relations (for example, select_related("organization__config_settings")), the traversed relation must not be deferred. If you also use .only() in the same query, include the relation name or FK field (e.g., "organization" or "organization_id") in the .only() list to avoid the error "Field X cannot be both deferred and traversed using select_related at the same time." Apply this guideline to Django code in openwisp_controller/config/management/commands/clear_last_ip.py and similar modules by ensuring any select_related with an accompanying only() includes the related field names to prevent deferred/traversed conflicts.
Applied to files:
openwisp_controller/connection/tests/pytest.pyopenwisp_controller/geo/estimated_location/tests/tests.pyopenwisp_controller/connection/tests/test_selenium.pyopenwisp_controller/connection/api/urls.pyopenwisp_controller/geo/tests/test_api.pyopenwisp_controller/connection/tests/utils.pyopenwisp_controller/connection/tasks.pyopenwisp_controller/connection/migrations/0011_batchcommand_command_batch_command.pyopenwisp_controller/connection/models.pyopenwisp_controller/connection/migrations/__init__.pyopenwisp_controller/connection/api/serializers.pyopenwisp_controller/connection/tests/test_tasks.pyopenwisp_controller/connection/tests/test_models.pyopenwisp_controller/connection/api/views.pyopenwisp_controller/connection/base/models.pyopenwisp_controller/connection/tests/test_api.py
📚 Learning: 2026-02-17T19:13:10.088Z
Learnt from: nemesifier
Repo: openwisp/openwisp-controller PR: 1175
File: openwisp_controller/config/whois/commands.py:0-0
Timestamp: 2026-02-17T19:13:10.088Z
Learning: In reviews for the openwisp/openwisp-controller repository, do not propose changes based on Ruff warnings. The project does not use Ruff as its linter; ignore Ruff-related suggestions and follow the repository’s established linting and configuration rules. This guidance applies to all Python files under the openwisp_controller directory.
Applied to files:
openwisp_controller/connection/tests/pytest.pyopenwisp_controller/geo/estimated_location/tests/tests.pyopenwisp_controller/connection/tests/test_selenium.pyopenwisp_controller/connection/api/urls.pyopenwisp_controller/geo/tests/test_api.pyopenwisp_controller/connection/tests/utils.pyopenwisp_controller/connection/tasks.pyopenwisp_controller/connection/migrations/0011_batchcommand_command_batch_command.pyopenwisp_controller/connection/models.pyopenwisp_controller/connection/migrations/__init__.pyopenwisp_controller/connection/api/serializers.pyopenwisp_controller/connection/tests/test_tasks.pyopenwisp_controller/connection/tests/test_models.pyopenwisp_controller/connection/api/views.pyopenwisp_controller/connection/base/models.pyopenwisp_controller/connection/tests/test_api.py
📚 Learning: 2026-01-15T15:07:17.354Z
Learnt from: DragnEmperor
Repo: openwisp/openwisp-controller PR: 1175
File: openwisp_controller/geo/estimated_location/tests/tests.py:172-175
Timestamp: 2026-01-15T15:07:17.354Z
Learning: In this repository, flake8 enforces E501 (line too long) via setup.cfg (max-line-length = 88) while ruff ignores E501 via ruff.toml. Therefore, use '# noqa: E501' on lines that intentionally exceed 88 characters to satisfy flake8 without affecting ruff checks. This applies to Python files across the project (any .py) and is relevant for tests as well. Use sparingly and only where breaking lines is not feasible without hurting readability or functionality.
Applied to files:
openwisp_controller/connection/tests/pytest.pyopenwisp_controller/geo/estimated_location/tests/tests.pytests/openwisp2/settings.pyopenwisp_controller/connection/tests/test_selenium.pyopenwisp_controller/connection/api/urls.pyopenwisp_controller/geo/tests/test_api.pyopenwisp_controller/connection/tests/utils.pytests/openwisp2/sample_connection/models.pyopenwisp_controller/connection/tasks.pyopenwisp_controller/connection/migrations/0011_batchcommand_command_batch_command.pyopenwisp_controller/connection/models.pyopenwisp_controller/connection/migrations/__init__.pyopenwisp_controller/connection/api/serializers.pyopenwisp_controller/connection/tests/test_tasks.pytests/openwisp2/sample_connection/api/views.pyopenwisp_controller/connection/tests/test_models.pytests/openwisp2/sample_connection/migrations/0005_batchcommand_command_batch_command.pyopenwisp_controller/connection/api/views.pyopenwisp_controller/connection/base/models.pyopenwisp_controller/connection/tests/test_api.py
📚 Learning: 2026-06-07T12:07:08.468Z
Learnt from: stktyagi
Repo: openwisp/openwisp-controller PR: 1378
File: openwisp_controller/config/tests/test_admin.py:2335-2335
Timestamp: 2026-06-07T12:07:08.468Z
Learning: In this project’s Python test suite (files under openwisp_controller/**/tests/), don’t require or request prose/inline comments that document the breakdown of query-count changes (e.g., assertions around template/DB query counts in helpers like _verify_template_queries). Treat query-count assertions as volatile implementation details that change frequently; review should focus on whether the test asserts the expected behavior, not on explaining the specific query-count deltas in comments.
Applied to files:
openwisp_controller/connection/tests/pytest.pyopenwisp_controller/geo/estimated_location/tests/tests.pyopenwisp_controller/connection/tests/test_selenium.pyopenwisp_controller/geo/tests/test_api.pyopenwisp_controller/connection/tests/utils.pyopenwisp_controller/connection/tests/test_tasks.pyopenwisp_controller/connection/tests/test_models.pyopenwisp_controller/connection/tests/test_api.py
📚 Learning: 2026-06-07T12:07:24.608Z
Learnt from: stktyagi
Repo: openwisp/openwisp-controller PR: 1378
File: openwisp_controller/pki/tests/test_api.py:155-155
Timestamp: 2026-06-07T12:07:24.608Z
Learning: When reviewing Python test files in this repository, avoid recommending inline comments that explain or justify `assertNumQueries` (Django query count) expectations. Query counts can change frequently as implementations evolve, and inline explanations add maintenance burden; the expected count should be understandable without added comment blocks.
Applied to files:
openwisp_controller/connection/tests/pytest.pyopenwisp_controller/geo/estimated_location/tests/tests.pytests/openwisp2/settings.pyopenwisp_controller/connection/tests/test_selenium.pyopenwisp_controller/geo/tests/test_api.pyopenwisp_controller/connection/tests/utils.pytests/openwisp2/sample_connection/models.pyopenwisp_controller/connection/tests/test_tasks.pytests/openwisp2/sample_connection/api/views.pyopenwisp_controller/connection/tests/test_models.pytests/openwisp2/sample_connection/migrations/0005_batchcommand_command_batch_command.pyopenwisp_controller/connection/tests/test_api.py
📚 Learning: 2026-06-25T12:20:18.414Z
Learnt from: dee077
Repo: openwisp/openwisp-controller PR: 1395
File: openwisp_controller/connection/base/models.py:571-572
Timestamp: 2026-06-25T12:20:18.414Z
Learning: When writing or reviewing tests that override pagination behavior via OpenWispPagination.paginate_queryset(), patch `view.pagination_page_size` (not `page_size`). The method uses `getattr(view, "pagination_page_size", self.page_size)`, so tests must set the attribute on the view to affect pagination. If the view class does not define `pagination_page_size`, using `unittest.mock.patch(..., create=True)` is intentional and correct because the attribute may not exist until patched.
Applied to files:
openwisp_controller/connection/tests/pytest.pyopenwisp_controller/geo/estimated_location/tests/tests.pytests/openwisp2/settings.pyopenwisp_controller/connection/tests/test_selenium.pyopenwisp_controller/geo/tests/test_api.pyopenwisp_controller/connection/tests/utils.pytests/openwisp2/sample_connection/models.pyopenwisp_controller/connection/tests/test_tasks.pytests/openwisp2/sample_connection/api/views.pyopenwisp_controller/connection/tests/test_models.pytests/openwisp2/sample_connection/migrations/0005_batchcommand_command_batch_command.pyopenwisp_controller/connection/tests/test_api.py
📚 Learning: 2026-06-25T12:20:45.387Z
Learnt from: dee077
Repo: openwisp/openwisp-controller PR: 1395
File: openwisp_controller/connection/tests/test_api.py:916-932
Timestamp: 2026-06-25T12:20:45.387Z
Learning: When reviewing API pagination behavior in openwisp-controller, assume `OpenWispPagination.paginate_queryset()` allows a per-view page-size override via `getattr(view, "pagination_page_size", self.page_size)` (so `view.pagination_page_size`, if present, should affect pagination). In Python tests, it is valid to patch `pagination_page_size` on a view class even if the attribute isn’t declared on the class by default, by using `unittest.mock.patch.object(..., "pagination_page_size", ..., create=True)` so the override is available for the pagination logic during the test.
Applied to files:
openwisp_controller/connection/tests/pytest.pyopenwisp_controller/geo/estimated_location/tests/tests.pyopenwisp_controller/connection/tests/test_selenium.pyopenwisp_controller/geo/tests/test_api.pyopenwisp_controller/connection/tests/utils.pyopenwisp_controller/connection/tests/test_tasks.pyopenwisp_controller/connection/tests/test_models.pyopenwisp_controller/connection/tests/test_api.py
📚 Learning: 2026-03-27T20:50:26.240Z
Learnt from: nemesifier
Repo: openwisp/openwisp-controller PR: 1315
File: openwisp_controller/geo/estimated_location/service.py:70-76
Timestamp: 2026-03-27T20:50:26.240Z
Learning: In openwisp-controller’s WHOIS and estimated-location services (openwisp_controller/config/whois/ and openwisp_controller/geo/estimated_location/), these components only process public IP addresses. When reviewing logs/error/debug messages in this area, treat logging the IP address as acceptable and do not flag it as a privacy/security concern—unless the logged value can originate from non-public/private IPs in that specific code path.
Applied to files:
openwisp_controller/geo/estimated_location/tests/tests.py
📚 Learning: 2026-06-07T12:07:25.164Z
Learnt from: stktyagi
Repo: openwisp/openwisp-controller PR: 1378
File: openwisp_controller/config/tests/test_config.py:864-865
Timestamp: 2026-06-07T12:07:25.164Z
Learning: When reviewing this repo’s Python test suite, treat changes to the *expected* query count in `assertNumQueries(...)` calls as routine test maintenance. If a PR updates the numeric argument (e.g., in `test_config.py`, `test_api.py`, `test_admin.py`, `test_pki.py`) and the test remains consistent with the feature changes, reviewers should not flag the increased number as a performance regression that requires investigation solely because the count went up; instead, focus on whether the update is intentional and the surrounding test/code changes justify the revised expectation.
Applied to files:
openwisp_controller/geo/estimated_location/tests/tests.pyopenwisp_controller/connection/tests/test_selenium.pyopenwisp_controller/geo/tests/test_api.pyopenwisp_controller/connection/tests/test_tasks.pyopenwisp_controller/connection/tests/test_models.pyopenwisp_controller/connection/tests/test_api.py
🪛 ast-grep (0.44.1)
openwisp_controller/connection/tasks.py
[warning] 102-102: Loading a Keras model from an untrusted file can execute arbitrary code via Lambda layers or custom objects. Load only trusted models and avoid deserializing custom objects from untrusted sources.
Context: load_model("connection", "BatchCommand")
Note: [CWE-502] Deserialization of Untrusted Data.
(keras-load-model-python)
openwisp_controller/connection/migrations/0011_batchcommand_command_batch_command.py
[info] 60-69: use help_text to document model columns
Context: models.CharField(
choices=[
("idle", "idle"),
("in-progress", "in progress"),
("success", "success"),
("failed", "failed"),
],
default="idle",
max_length=12,
)
Note: [CWE-710] Improper Adherence to Coding Standards.
(model-help-text)
[info] 73-80: use help_text to document model columns
Context: models.CharField(
choices=(
openwisp_controller.connection.commands.COMMAND_CHOICES
if django.VERSION < (5, 0)
else openwisp_controller.connection.commands.get_command_choices # noqa: E501
),
max_length=16,
)
Note: [CWE-710] Improper Adherence to Coding Standards.
(model-help-text)
openwisp_controller/connection/tests/test_models.py
[warning] 35-35: Loading a Keras model from an untrusted file can execute arbitrary code via Lambda layers or custom objects. Load only trusted models and avoid deserializing custom objects from untrusted sources.
Context: load_model("config", "DeviceGroup")
Note: [CWE-502] Deserialization of Untrusted Data.
(keras-load-model-python)
[warning] 36-36: Loading a Keras model from an untrusted file can execute arbitrary code via Lambda layers or custom objects. Load only trusted models and avoid deserializing custom objects from untrusted sources.
Context: load_model("geo", "Location")
Note: [CWE-502] Deserialization of Untrusted Data.
(keras-load-model-python)
[warning] 37-37: Loading a Keras model from an untrusted file can execute arbitrary code via Lambda layers or custom objects. Load only trusted models and avoid deserializing custom objects from untrusted sources.
Context: load_model("geo", "DeviceLocation")
Note: [CWE-502] Deserialization of Untrusted Data.
(keras-load-model-python)
tests/openwisp2/sample_connection/migrations/0005_batchcommand_command_batch_command.py
[info] 58-67: use help_text to document model columns
Context: models.CharField(
choices=[
("idle", "idle"),
("in-progress", "in progress"),
("success", "success"),
("failed", "failed"),
],
default="idle",
max_length=12,
)
Note: [CWE-710] Improper Adherence to Coding Standards.
(model-help-text)
[info] 71-78: use help_text to document model columns
Context: models.CharField(
max_length=16,
choices=(
connection_config.commands.COMMAND_CHOICES
if django.VERSION < (5, 0)
else connection_config.commands.get_command_choices
),
)
Note: [CWE-710] Improper Adherence to Coding Standards.
(model-help-text)
openwisp_controller/connection/base/models.py
[warning] 859-859: Loading a Keras model from an untrusted file can execute arbitrary code via Lambda layers or custom objects. Load only trusted models and avoid deserializing custom objects from untrusted sources.
Context: load_model("connection", "Command")
Note: [CWE-502] Deserialization of Untrusted Data.
(keras-load-model-python)
[warning] 886-886: Loading a Keras model from an untrusted file can execute arbitrary code via Lambda layers or custom objects. Load only trusted models and avoid deserializing custom objects from untrusted sources.
Context: load_model("config", "Device")
Note: [CWE-502] Deserialization of Untrusted Data.
(keras-load-model-python)
[warning] 961-961: Loading a Keras model from an untrusted file can execute arbitrary code via Lambda layers or custom objects. Load only trusted models and avoid deserializing custom objects from untrusted sources.
Context: load_model("connection", "Command")
Note: [CWE-502] Deserialization of Untrusted Data.
(keras-load-model-python)
[warning] 962-962: Loading a Keras model from an untrusted file can execute arbitrary code via Lambda layers or custom objects. Load only trusted models and avoid deserializing custom objects from untrusted sources.
Context: load_model("config", "Device")
Note: [CWE-502] Deserialization of Untrusted Data.
(keras-load-model-python)
[info] 751-753: use help_text to document model columns
Context: models.CharField(
max_length=12, choices=STATUS_CHOICES, default=STATUS_CHOICES[0][0]
)
Note: [CWE-710] Improper Adherence to Coding Standards.
(model-help-text)
[info] 754-757: use help_text to document model columns
Context: models.CharField(
max_length=16,
choices=(COMMAND_CHOICES if django.VERSION < (5, 0) else get_command_choices),
)
Note: [CWE-710] Improper Adherence to Coding Standards.
(model-help-text)
openwisp_controller/connection/tests/test_api.py
[warning] 21-21: Loading a Keras model from an untrusted file can execute arbitrary code via Lambda layers or custom objects. Load only trusted models and avoid deserializing custom objects from untrusted sources.
Context: load_model("connection", "Command")
Note: [CWE-502] Deserialization of Untrusted Data.
(keras-load-model-python)
[warning] 22-22: Loading a Keras model from an untrusted file can execute arbitrary code via Lambda layers or custom objects. Load only trusted models and avoid deserializing custom objects from untrusted sources.
Context: load_model("connection", "DeviceConnection")
Note: [CWE-502] Deserialization of Untrusted Data.
(keras-load-model-python)
[warning] 23-23: Loading a Keras model from an untrusted file can execute arbitrary code via Lambda layers or custom objects. Load only trusted models and avoid deserializing custom objects from untrusted sources.
Context: load_model("connection", "BatchCommand")
Note: [CWE-502] Deserialization of Untrusted Data.
(keras-load-model-python)
[warning] 25-25: Loading a Keras model from an untrusted file can execute arbitrary code via Lambda layers or custom objects. Load only trusted models and avoid deserializing custom objects from untrusted sources.
Context: load_model("openwisp_users", "OrganizationUser")
Note: [CWE-502] Deserialization of Untrusted Data.
(keras-load-model-python)
[warning] 26-26: Loading a Keras model from an untrusted file can execute arbitrary code via Lambda layers or custom objects. Load only trusted models and avoid deserializing custom objects from untrusted sources.
Context: load_model("openwisp_users", "Group")
Note: [CWE-502] Deserialization of Untrusted Data.
(keras-load-model-python)
[warning] 27-27: Loading a Keras model from an untrusted file can execute arbitrary code via Lambda layers or custom objects. Load only trusted models and avoid deserializing custom objects from untrusted sources.
Context: load_model("config", "DeviceGroup")
Note: [CWE-502] Deserialization of Untrusted Data.
(keras-load-model-python)
[warning] 28-28: Loading a Keras model from an untrusted file can execute arbitrary code via Lambda layers or custom objects. Load only trusted models and avoid deserializing custom objects from untrusted sources.
Context: load_model("geo", "Location")
Note: [CWE-502] Deserialization of Untrusted Data.
(keras-load-model-python)
[warning] 29-29: Loading a Keras model from an untrusted file can execute arbitrary code via Lambda layers or custom objects. Load only trusted models and avoid deserializing custom objects from untrusted sources.
Context: load_model("geo", "DeviceLocation")
Note: [CWE-502] Deserialization of Untrusted Data.
(keras-load-model-python)
[info] 1070-1070: use jsonify instead of json.dumps for JSON output
Context: json.dumps(payload)
Note: [CWE-116] Improper Encoding or Escaping of Output.
(use-jsonify)
[info] 1107-1107: use jsonify instead of json.dumps for JSON output
Context: json.dumps(payload)
Note: [CWE-116] Improper Encoding or Escaping of Output.
(use-jsonify)
[info] 1133-1133: use jsonify instead of json.dumps for JSON output
Context: json.dumps(payload)
Note: [CWE-116] Improper Encoding or Escaping of Output.
(use-jsonify)
[info] 1149-1149: use jsonify instead of json.dumps for JSON output
Context: json.dumps(payload)
Note: [CWE-116] Improper Encoding or Escaping of Output.
(use-jsonify)
[info] 1173-1173: use jsonify instead of json.dumps for JSON output
Context: json.dumps(payload)
Note: [CWE-116] Improper Encoding or Escaping of Output.
(use-jsonify)
[info] 1198-1198: use jsonify instead of json.dumps for JSON output
Context: json.dumps(payload)
Note: [CWE-116] Improper Encoding or Escaping of Output.
(use-jsonify)
[info] 1235-1241: use jsonify instead of json.dumps for JSON output
Context: json.dumps(
{
"type": "custom",
"input": {"command": "echo test"},
"label": "test-label",
}
)
Note: [CWE-116] Improper Encoding or Escaping of Output.
(use-jsonify)
[info] 1251-1258: use jsonify instead of json.dumps for JSON output
Context: json.dumps(
{
"type": "custom",
"input": {"command": "echo test"},
"label": "test-label",
"devices": [str(device1.pk), str(device2.pk)],
}
)
Note: [CWE-116] Improper Encoding or Escaping of Output.
(use-jsonify)
[info] 1308-1308: use jsonify instead of json.dumps for JSON output
Context: json.dumps(payload)
Note: [CWE-116] Improper Encoding or Escaping of Output.
(use-jsonify)
[info] 1331-1331: use jsonify instead of json.dumps for JSON output
Context: json.dumps(payload)
Note: [CWE-116] Improper Encoding or Escaping of Output.
(use-jsonify)
[info] 1362-1362: use jsonify instead of json.dumps for JSON output
Context: json.dumps(payload)
Note: [CWE-116] Improper Encoding or Escaping of Output.
(use-jsonify)
[info] 1385-1385: use jsonify instead of json.dumps for JSON output
Context: json.dumps(payload)
Note: [CWE-116] Improper Encoding or Escaping of Output.
(use-jsonify)
[info] 1421-1421: use jsonify instead of json.dumps for JSON output
Context: json.dumps(payload)
Note: [CWE-116] Improper Encoding or Escaping of Output.
(use-jsonify)
[info] 1442-1442: use jsonify instead of json.dumps for JSON output
Context: json.dumps(payload)
Note: [CWE-116] Improper Encoding or Escaping of Output.
(use-jsonify)
[info] 1473-1473: use jsonify instead of json.dumps for JSON output
Context: json.dumps(payload)
Note: [CWE-116] Improper Encoding or Escaping of Output.
(use-jsonify)
[info] 1494-1494: use jsonify instead of json.dumps for JSON output
Context: json.dumps(payload)
Note: [CWE-116] Improper Encoding or Escaping of Output.
(use-jsonify)
[info] 1547-1547: use jsonify instead of json.dumps for JSON output
Context: json.dumps(list_response.data)
Note: [CWE-116] Improper Encoding or Escaping of Output.
(use-jsonify)
[info] 1548-1548: use jsonify instead of json.dumps for JSON output
Context: json.dumps(list_response.data)
Note: [CWE-116] Improper Encoding or Escaping of Output.
(use-jsonify)
[info] 1549-1549: use jsonify instead of json.dumps for JSON output
Context: json.dumps(list_response.data)
Note: [CWE-116] Improper Encoding or Escaping of Output.
(use-jsonify)
[info] 1551-1551: use jsonify instead of json.dumps for JSON output
Context: json.dumps(list_response.data)
Note: [CWE-116] Improper Encoding or Escaping of Output.
(use-jsonify)
[info] 1557-1557: use jsonify instead of json.dumps for JSON output
Context: json.dumps(detail_response.data)
Note: [CWE-116] Improper Encoding or Escaping of Output.
(use-jsonify)
[info] 1558-1558: use jsonify instead of json.dumps for JSON output
Context: json.dumps(detail_response.data)
Note: [CWE-116] Improper Encoding or Escaping of Output.
(use-jsonify)
[info] 1559-1559: use jsonify instead of json.dumps for JSON output
Context: json.dumps(detail_response.data)
Note: [CWE-116] Improper Encoding or Escaping of Output.
(use-jsonify)
[info] 1561-1561: use jsonify instead of json.dumps for JSON output
Context: json.dumps(detail_response.data)
Note: [CWE-116] Improper Encoding or Escaping of Output.
(use-jsonify)
[info] 1588-1588: use jsonify instead of json.dumps for JSON output
Context: json.dumps({"type": "custom"})
Note: [CWE-116] Improper Encoding or Escaping of Output.
(use-jsonify)
[info] 1642-1642: use jsonify instead of json.dumps for JSON output
Context: json.dumps(payload)
Note: [CWE-116] Improper Encoding or Escaping of Output.
(use-jsonify)
[info] 1699-1699: use jsonify instead of json.dumps for JSON output
Context: json.dumps(payload)
Note: [CWE-116] Improper Encoding or Escaping of Output.
(use-jsonify)
[info] 1717-1725: use jsonify instead of json.dumps for JSON output
Context: json.dumps(
{
"organization": str(org.pk),
"type": "custom",
"input": {"command": "echo test"},
"label": "test-label",
"devices": [str(device_org2.pk)],
}
)
Note: [CWE-116] Improper Encoding or Escaping of Output.
(use-jsonify)
[info] 1746-1754: use jsonify instead of json.dumps for JSON output
Context: json.dumps(
{
"organization": str(org.pk),
"type": "custom",
"input": {"command": "echo test"},
"label": "test-label",
"group": str(group_org2.pk),
}
)
Note: [CWE-116] Improper Encoding or Escaping of Output.
(use-jsonify)
[info] 1778-1786: use jsonify instead of json.dumps for JSON output
Context: json.dumps(
{
"organization": str(org.pk),
"type": "custom",
"input": {"command": "echo test"},
"label": "test-label",
"location": str(location_org2.pk),
}
)
Note: [CWE-116] Improper Encoding or Escaping of Output.
(use-jsonify)
[info] 1923-1931: use jsonify instead of json.dumps for JSON output
Context: json.dumps(
{
"organization": str(org.pk),
"type": "custom",
"input": {"command": "echo test"},
"label": "test-label",
"devices": [str(device1.pk)],
}
)
Note: [CWE-116] Improper Encoding or Escaping of Output.
(use-jsonify)
[info] 1943-1951: use jsonify instead of json.dumps for JSON output
Context: json.dumps(
{
"organization": str(org.pk),
"type": "custom",
"input": {"command": "echo test"},
"label": "test-label",
"group": str(group.pk),
}
)
Note: [CWE-116] Improper Encoding or Escaping of Output.
(use-jsonify)
[info] 1963-1971: use jsonify instead of json.dumps for JSON output
Context: json.dumps(
{
"organization": str(org.pk),
"type": "custom",
"input": {"command": "echo test"},
"label": "test-label",
"location": str(location.pk),
}
)
Note: [CWE-116] Improper Encoding or Escaping of Output.
(use-jsonify)
[info] 1984-1993: use jsonify instead of json.dumps for JSON output
Context: json.dumps(
{
"organization": str(org.pk),
"type": "custom",
"input": {"command": "echo test"},
"label": "test-label",
"group": str(group.pk),
"location": str(location.pk),
}
)
Note: [CWE-116] Improper Encoding or Escaping of Output.
(use-jsonify)
[info] 2005-2012: use jsonify instead of json.dumps for JSON output
Context: json.dumps(
{
"organization": str(org.pk),
"type": "custom",
"input": {"command": "echo test"},
"label": "test-label",
}
)
Note: [CWE-116] Improper Encoding or Escaping of Output.
(use-jsonify)
[info] 2027-2035: use jsonify instead of json.dumps for JSON output
Context: json.dumps(
{
"organization": str(org.pk),
"type": "custom",
"input": {"command": "echo test"},
"label": "test-label",
"devices": [],
}
)
Note: [CWE-116] Improper Encoding or Escaping of Output.
(use-jsonify)
[info] 2047-2054: use jsonify instead of json.dumps for JSON output
Context: json.dumps(
{
"organization": str(org.pk),
"type": "custom",
"input": {"command": "echo test"},
"label": "test-label",
}
)
Note: [CWE-116] Improper Encoding or Escaping of Output.
(use-jsonify)
[info] 2068-2076: use jsonify instead of json.dumps for JSON output
Context: json.dumps(
{
"organization": str(org.pk),
"type": "custom",
"input": {"command": "echo test"},
"label": "",
"devices": [str(device1.pk)],
}
)
Note: [CWE-116] Improper Encoding or Escaping of Output.
(use-jsonify)
[info] 2084-2092: use jsonify instead of json.dumps for JSON output
Context: json.dumps(
{
"organization": str(org.pk),
"type": "custom",
"input": {"command": "echo test"},
"label": "a" * 65,
"devices": [str(device1.pk)],
}
)
Note: [CWE-116] Improper Encoding or Escaping of Output.
(use-jsonify)
[info] 2100-2108: use jsonify instead of json.dumps for JSON output
Context: json.dumps(
{
"organization": str(org.pk),
"type": "custom",
"input": {},
"label": "test-label",
"devices": [str(device1.pk)],
}
)
Note: [CWE-116] Improper Encoding or Escaping of Output.
(use-jsonify)
[info] 2116-2124: use jsonify instead of json.dumps for JSON output
Context: json.dumps(
{
"organization": str(org.pk),
"type": "nonexistent",
"input": {"command": "echo test"},
"label": "test-label",
"devices": [str(device1.pk)],
}
)
Note: [CWE-116] Improper Encoding or Escaping of Output.
(use-jsonify)
[info] 2142-2153: use jsonify instead of json.dumps for JSON output
Context: json.dumps(
{
"organization": str(org.pk),
"type": "change_password",
"input": {
"password": password,
"confirm_password": password,
},
"label": "change password",
"devices": [str(device.pk)],
}
)
Note: [CWE-116] Improper Encoding or Escaping of Output.
(use-jsonify)
[info] 2158-2158: use jsonify instead of json.dumps for JSON output
Context: json.dumps(batch.input)
Note: [CWE-116] Improper Encoding or Escaping of Output.
(use-jsonify)
[info] 2161-2161: use jsonify instead of json.dumps for JSON output
Context: json.dumps(list_response.data)
Note: [CWE-116] Improper Encoding or Escaping of Output.
(use-jsonify)
[info] 2166-2166: use jsonify instead of json.dumps for JSON output
Context: json.dumps(detail_response.data)
Note: [CWE-116] Improper Encoding or Escaping of Output.
(use-jsonify)
[info] 2199-2199: use jsonify instead of json.dumps for JSON output
Context: json.dumps(payload)
Note: [CWE-116] Improper Encoding or Escaping of Output.
(use-jsonify)
[info] 2251-2259: use jsonify instead of json.dumps for JSON output
Context: json.dumps(
{
"organization": str(org.pk),
"type": "custom",
"input": {"command": "echo test"},
"label": "test-label",
"devices": [str(device.pk)],
}
)
Note: [CWE-116] Improper Encoding or Escaping of Output.
(use-jsonify)
[info] 2293-2301: use jsonify instead of json.dumps for JSON output
Context: json.dumps(
{
"organization": str(org.pk),
"type": "custom",
"input": {"command": "echo test"},
"label": "test-label",
"devices": [str(device.pk)],
}
)
Note: [CWE-116] Improper Encoding or Escaping of Output.
(use-jsonify)
🪛 Betterleaks (1.6.1)
docs/user/rest-api.rst
[high] 466-468: Discovered a potential authorization token provided in a curl command header, which could compromise the curl accessed resource.
(curl-auth-header)
[high] 558-560: Discovered a potential authorization token provided in a curl command header, which could compromise the curl accessed resource.
(curl-auth-header)
[high] 933-935: Discovered a potential authorization token provided in a curl command header, which could compromise the curl accessed resource.
(curl-auth-header)
[high] 952-954: Discovered a potential authorization token provided in a curl command header, which could compromise the curl accessed resource.
(curl-auth-header)
🔇 Additional comments (24)
openwisp_controller/connection/models.py (1)
3-8: LGTM!Also applies to: 27-32
openwisp_controller/connection/migrations/0011_batchcommand_command_batch_command.py (1)
1-181: LGTM!openwisp_controller/connection/migrations/__init__.py (1)
65-89: LGTM!tests/openwisp2/sample_connection/migrations/0005_batchcommand_command_batch_command.py (1)
1-176: LGTM!tests/openwisp2/sample_connection/models.py (1)
4-4: LGTM!Also applies to: 31-35
tests/openwisp2/settings.py (1)
296-296: LGTM!tests/openwisp2/sample_connection/api/views.py (1)
54-74: LGTM!openwisp_controller/connection/base/models.py (2)
1043-1054: Mixed skipped batches still remainin-progress.Once all created children succeed but
skipped_devicesis non-empty, none of the terminal branches match andbatch.statusremains"in-progress". Treat skipped targets as failure after the in-progress branch.Proposed fix
- elif stats["failed"] > 0: + elif stats["failed"] > 0 or batch.skipped_devices: new_status = "failed"
18-18: LGTM!Also applies to: 34-38, 475-481, 572-573, 738-895
docs/user/shell-commands.rst (1)
199-203: Document that explicit devices override the filters.“Any combination” implies that
devices,group, andlocationare intersected, but explicit devices intentionally take precedence and group/location are ignored. Describe these as alternatives and state the precedence rule.openwisp_controller/connection/tasks.py (1)
101-120: LGTM!docs/user/intro.rst (1)
73-75: LGTM!docs/developer/extending.rst (1)
357-357: LGTM!openwisp_controller/connection/api/views.py (1)
151-197: LGTM!openwisp_controller/connection/api/urls.py (1)
43-57: LGTM!docs/user/rest-api.rst (1)
484-590: LGTM!openwisp_controller/connection/tests/pytest.py (1)
68-68: Previously raised and explicitly deferred.Positive batch-linked WebSocket coverage was already requested and deferred to the follow-up WebSocket work.
openwisp_controller/connection/tests/test_api.py (1)
6-6: LGTM!Also applies to: 18-30, 869-2319
openwisp_controller/connection/tests/test_models.py (1)
35-38: LGTM!Also applies to: 467-474, 975-1747
openwisp_controller/connection/tests/test_tasks.py (1)
19-19: LGTM!Also applies to: 319-385, 433-480
openwisp_controller/connection/tests/utils.py (1)
14-14: LGTM!Also applies to: 122-139
openwisp_controller/connection/tests/test_selenium.py (1)
1-2: LGTM!Also applies to: 58-60, 70-70
openwisp_controller/geo/estimated_location/tests/tests.py (1)
569-569: LGTM!openwisp_controller/geo/tests/test_api.py (1)
697-697: LGTM!
ac14b28 to
8efe276
Compare
|
@coderabbitai full review |
✅ Action performedFull review finished. |
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
tests/openwisp2/sample_connection/api/views.py (1)
1-9: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsolidate imports from the same module.
To improve code readability, consider combining these separate import statements into a single block since they originate from the same module.
♻️ Proposed refactor
-from openwisp_controller.connection.api.views import ( - BatchCommandDetailView as BaseBatchCommandDetailView, -) -from openwisp_controller.connection.api.views import ( - BatchCommandExecuteView as BaseBatchCommandExecuteView, -) -from openwisp_controller.connection.api.views import ( - BatchCommandListView as BaseBatchCommandListView, -) +from openwisp_controller.connection.api.views import ( + BatchCommandDetailView as BaseBatchCommandDetailView, + BatchCommandExecuteView as BaseBatchCommandExecuteView, + BatchCommandListView as BaseBatchCommandListView, +)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/openwisp2/sample_connection/api/views.py` around lines 1 - 9, Consolidate the three imports from openwisp_controller.connection.api.views into one parenthesized import block, preserving the existing aliases for BaseBatchCommandDetailView, BaseBatchCommandExecuteView, and BaseBatchCommandListView.openwisp_controller/connection/tests/test_api.py (1)
2130-2167: 🔒 Security & Privacy | 🟠 Major | 🏗️ Heavy liftDo not rely on the asynchronous worker to redact passwords.
BatchCommand.execute()commits the raw password before scheduling the worker;create_commands()masks it later. Worker delays therefore leave credentials plaintext in the database and exposed through list/detail responses. Persist a secret-free or encrypted value before commit, and patchlaunch_batch_command.delayhere to verify redaction before worker execution.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@openwisp_controller/connection/tests/test_api.py` around lines 2130 - 2167, Update the batch command password test around BatchCommand execution to patch launch_batch_command.delay, assert the persisted BatchCommand input is already secret-free before any worker runs, and verify list/detail responses likewise omit the password. Ensure the production execution path sanitizes or encrypts the password before committing the BatchCommand, rather than relying on create_commands() or asynchronous worker redaction.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@openwisp_controller/connection/tests/test_models.py`:
- Around line 1707-1719: Extend the “all success shows success” test to include
a completed batch with nonempty skipped_devices and assert its status is failed.
Update the post-running branch of calculate_and_update_status so completed
commands with any skipped devices select failed, while batches with no skipped
devices retain the existing success behavior.
---
Duplicate comments:
In `@openwisp_controller/connection/tests/test_api.py`:
- Around line 2130-2167: Update the batch command password test around
BatchCommand execution to patch launch_batch_command.delay, assert the persisted
BatchCommand input is already secret-free before any worker runs, and verify
list/detail responses likewise omit the password. Ensure the production
execution path sanitizes or encrypts the password before committing the
BatchCommand, rather than relying on create_commands() or asynchronous worker
redaction.
In `@tests/openwisp2/sample_connection/api/views.py`:
- Around line 1-9: Consolidate the three imports from
openwisp_controller.connection.api.views into one parenthesized import block,
preserving the existing aliases for BaseBatchCommandDetailView,
BaseBatchCommandExecuteView, and BaseBatchCommandListView.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 19aaf83f-2d45-45bd-9336-1edf03149de5
📒 Files selected for processing (24)
docs/developer/extending.rstdocs/user/intro.rstdocs/user/rest-api.rstdocs/user/shell-commands.rstopenwisp_controller/connection/api/serializers.pyopenwisp_controller/connection/api/urls.pyopenwisp_controller/connection/api/views.pyopenwisp_controller/connection/base/models.pyopenwisp_controller/connection/migrations/0011_batchcommand_command_batch_command.pyopenwisp_controller/connection/migrations/__init__.pyopenwisp_controller/connection/models.pyopenwisp_controller/connection/tasks.pyopenwisp_controller/connection/tests/pytest.pyopenwisp_controller/connection/tests/test_api.pyopenwisp_controller/connection/tests/test_models.pyopenwisp_controller/connection/tests/test_selenium.pyopenwisp_controller/connection/tests/test_tasks.pyopenwisp_controller/connection/tests/utils.pyopenwisp_controller/geo/estimated_location/tests/tests.pyopenwisp_controller/geo/tests/test_api.pytests/openwisp2/sample_connection/api/views.pytests/openwisp2/sample_connection/migrations/0005_batchcommand_command_batch_command.pytests/openwisp2/sample_connection/models.pytests/openwisp2/settings.py
📜 Review details
⏰ Context from checks skipped due to timeout. (9)
- GitHub Check: Python==3.13 | django~=5.2.0
- GitHub Check: Python==3.10 | django~=5.2.0
- GitHub Check: Python==3.12 | django~=5.2.0
- GitHub Check: Python==3.12 | django~=5.1.0
- GitHub Check: Python==3.10 | django~=5.1.0
- GitHub Check: Python==3.10 | django~=4.2.0
- GitHub Check: Python==3.11 | django~=5.1.0
- GitHub Check: Python==3.11 | django~=5.2.0
- GitHub Check: Kilo Code Review
🧰 Additional context used
📓 Path-based instructions (2)
**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
**/*.py: Mark user-facing strings for translation with Django i18n helpers in Django code
Avoid unnecessary blank lines inside function and method bodies
Be careful with authentication, authorization, queryset filtering, serializers, admin behavior, cache invalidation, signals, Celery tasks, and websocket updates in Django code
Preserve validation around templates, VPN/PKI material, SSH credentials, device commands, uploaded files, URLs, and subnet/IP data
Write comments and docstrings only when they explain why code is shaped a certain way, placing them before the relevant code block instead of scattering them inside it
Files:
openwisp_controller/connection/api/urls.pyopenwisp_controller/connection/tasks.pyopenwisp_controller/connection/tests/pytest.pyopenwisp_controller/geo/tests/test_api.pytests/openwisp2/settings.pyopenwisp_controller/connection/migrations/__init__.pyopenwisp_controller/connection/migrations/0011_batchcommand_command_batch_command.pyopenwisp_controller/connection/tests/test_selenium.pytests/openwisp2/sample_connection/models.pyopenwisp_controller/connection/models.pyopenwisp_controller/geo/estimated_location/tests/tests.pyopenwisp_controller/connection/base/models.pytests/openwisp2/sample_connection/migrations/0005_batchcommand_command_batch_command.pyopenwisp_controller/connection/api/views.pyopenwisp_controller/connection/tests/utils.pytests/openwisp2/sample_connection/api/views.pyopenwisp_controller/connection/tests/test_tasks.pyopenwisp_controller/connection/api/serializers.pyopenwisp_controller/connection/tests/test_models.pyopenwisp_controller/connection/tests/test_api.py
**/*.{py,html}
📄 CodeRabbit inference engine (Custom checks)
For Django pull requests, ensure all user-facing strings are marked as translatable using the Django i18n framework.
Files:
openwisp_controller/connection/api/urls.pyopenwisp_controller/connection/tasks.pyopenwisp_controller/connection/tests/pytest.pyopenwisp_controller/geo/tests/test_api.pytests/openwisp2/settings.pyopenwisp_controller/connection/migrations/__init__.pyopenwisp_controller/connection/migrations/0011_batchcommand_command_batch_command.pyopenwisp_controller/connection/tests/test_selenium.pytests/openwisp2/sample_connection/models.pyopenwisp_controller/connection/models.pyopenwisp_controller/geo/estimated_location/tests/tests.pyopenwisp_controller/connection/base/models.pytests/openwisp2/sample_connection/migrations/0005_batchcommand_command_batch_command.pyopenwisp_controller/connection/api/views.pyopenwisp_controller/connection/tests/utils.pytests/openwisp2/sample_connection/api/views.pyopenwisp_controller/connection/tests/test_tasks.pyopenwisp_controller/connection/api/serializers.pyopenwisp_controller/connection/tests/test_models.pyopenwisp_controller/connection/tests/test_api.py
🧠 Learnings (9)
📚 Learning: 2026-01-15T15:05:49.557Z
Learnt from: DragnEmperor
Repo: openwisp/openwisp-controller PR: 1175
File: openwisp_controller/config/management/commands/clear_last_ip.py:38-42
Timestamp: 2026-01-15T15:05:49.557Z
Learning: In Django projects, when using select_related() to traverse relations (for example, select_related("organization__config_settings")), the traversed relation must not be deferred. If you also use .only() in the same query, include the relation name or FK field (e.g., "organization" or "organization_id") in the .only() list to avoid the error "Field X cannot be both deferred and traversed using select_related at the same time." Apply this guideline to Django code in openwisp_controller/config/management/commands/clear_last_ip.py and similar modules by ensuring any select_related with an accompanying only() includes the related field names to prevent deferred/traversed conflicts.
Applied to files:
openwisp_controller/connection/api/urls.pyopenwisp_controller/connection/tasks.pyopenwisp_controller/connection/tests/pytest.pyopenwisp_controller/geo/tests/test_api.pyopenwisp_controller/connection/migrations/__init__.pyopenwisp_controller/connection/migrations/0011_batchcommand_command_batch_command.pyopenwisp_controller/connection/tests/test_selenium.pyopenwisp_controller/connection/models.pyopenwisp_controller/geo/estimated_location/tests/tests.pyopenwisp_controller/connection/base/models.pyopenwisp_controller/connection/api/views.pyopenwisp_controller/connection/tests/utils.pyopenwisp_controller/connection/tests/test_tasks.pyopenwisp_controller/connection/api/serializers.pyopenwisp_controller/connection/tests/test_models.pyopenwisp_controller/connection/tests/test_api.py
📚 Learning: 2026-02-17T19:13:10.088Z
Learnt from: nemesifier
Repo: openwisp/openwisp-controller PR: 1175
File: openwisp_controller/config/whois/commands.py:0-0
Timestamp: 2026-02-17T19:13:10.088Z
Learning: In reviews for the openwisp/openwisp-controller repository, do not propose changes based on Ruff warnings. The project does not use Ruff as its linter; ignore Ruff-related suggestions and follow the repository’s established linting and configuration rules. This guidance applies to all Python files under the openwisp_controller directory.
Applied to files:
openwisp_controller/connection/api/urls.pyopenwisp_controller/connection/tasks.pyopenwisp_controller/connection/tests/pytest.pyopenwisp_controller/geo/tests/test_api.pyopenwisp_controller/connection/migrations/__init__.pyopenwisp_controller/connection/migrations/0011_batchcommand_command_batch_command.pyopenwisp_controller/connection/tests/test_selenium.pyopenwisp_controller/connection/models.pyopenwisp_controller/geo/estimated_location/tests/tests.pyopenwisp_controller/connection/base/models.pyopenwisp_controller/connection/api/views.pyopenwisp_controller/connection/tests/utils.pyopenwisp_controller/connection/tests/test_tasks.pyopenwisp_controller/connection/api/serializers.pyopenwisp_controller/connection/tests/test_models.pyopenwisp_controller/connection/tests/test_api.py
📚 Learning: 2026-01-15T15:07:17.354Z
Learnt from: DragnEmperor
Repo: openwisp/openwisp-controller PR: 1175
File: openwisp_controller/geo/estimated_location/tests/tests.py:172-175
Timestamp: 2026-01-15T15:07:17.354Z
Learning: In this repository, flake8 enforces E501 (line too long) via setup.cfg (max-line-length = 88) while ruff ignores E501 via ruff.toml. Therefore, use '# noqa: E501' on lines that intentionally exceed 88 characters to satisfy flake8 without affecting ruff checks. This applies to Python files across the project (any .py) and is relevant for tests as well. Use sparingly and only where breaking lines is not feasible without hurting readability or functionality.
Applied to files:
openwisp_controller/connection/api/urls.pyopenwisp_controller/connection/tasks.pyopenwisp_controller/connection/tests/pytest.pyopenwisp_controller/geo/tests/test_api.pytests/openwisp2/settings.pyopenwisp_controller/connection/migrations/__init__.pyopenwisp_controller/connection/migrations/0011_batchcommand_command_batch_command.pyopenwisp_controller/connection/tests/test_selenium.pytests/openwisp2/sample_connection/models.pyopenwisp_controller/connection/models.pyopenwisp_controller/geo/estimated_location/tests/tests.pyopenwisp_controller/connection/base/models.pytests/openwisp2/sample_connection/migrations/0005_batchcommand_command_batch_command.pyopenwisp_controller/connection/api/views.pyopenwisp_controller/connection/tests/utils.pytests/openwisp2/sample_connection/api/views.pyopenwisp_controller/connection/tests/test_tasks.pyopenwisp_controller/connection/api/serializers.pyopenwisp_controller/connection/tests/test_models.pyopenwisp_controller/connection/tests/test_api.py
📚 Learning: 2026-06-07T12:07:08.468Z
Learnt from: stktyagi
Repo: openwisp/openwisp-controller PR: 1378
File: openwisp_controller/config/tests/test_admin.py:2335-2335
Timestamp: 2026-06-07T12:07:08.468Z
Learning: In this project’s Python test suite (files under openwisp_controller/**/tests/), don’t require or request prose/inline comments that document the breakdown of query-count changes (e.g., assertions around template/DB query counts in helpers like _verify_template_queries). Treat query-count assertions as volatile implementation details that change frequently; review should focus on whether the test asserts the expected behavior, not on explaining the specific query-count deltas in comments.
Applied to files:
openwisp_controller/connection/tests/pytest.pyopenwisp_controller/geo/tests/test_api.pyopenwisp_controller/connection/tests/test_selenium.pyopenwisp_controller/geo/estimated_location/tests/tests.pyopenwisp_controller/connection/tests/utils.pyopenwisp_controller/connection/tests/test_tasks.pyopenwisp_controller/connection/tests/test_models.pyopenwisp_controller/connection/tests/test_api.py
📚 Learning: 2026-06-07T12:07:24.608Z
Learnt from: stktyagi
Repo: openwisp/openwisp-controller PR: 1378
File: openwisp_controller/pki/tests/test_api.py:155-155
Timestamp: 2026-06-07T12:07:24.608Z
Learning: When reviewing Python test files in this repository, avoid recommending inline comments that explain or justify `assertNumQueries` (Django query count) expectations. Query counts can change frequently as implementations evolve, and inline explanations add maintenance burden; the expected count should be understandable without added comment blocks.
Applied to files:
openwisp_controller/connection/tests/pytest.pyopenwisp_controller/geo/tests/test_api.pytests/openwisp2/settings.pyopenwisp_controller/connection/tests/test_selenium.pytests/openwisp2/sample_connection/models.pyopenwisp_controller/geo/estimated_location/tests/tests.pytests/openwisp2/sample_connection/migrations/0005_batchcommand_command_batch_command.pyopenwisp_controller/connection/tests/utils.pytests/openwisp2/sample_connection/api/views.pyopenwisp_controller/connection/tests/test_tasks.pyopenwisp_controller/connection/tests/test_models.pyopenwisp_controller/connection/tests/test_api.py
📚 Learning: 2026-06-25T12:20:18.414Z
Learnt from: dee077
Repo: openwisp/openwisp-controller PR: 1395
File: openwisp_controller/connection/base/models.py:571-572
Timestamp: 2026-06-25T12:20:18.414Z
Learning: When writing or reviewing tests that override pagination behavior via OpenWispPagination.paginate_queryset(), patch `view.pagination_page_size` (not `page_size`). The method uses `getattr(view, "pagination_page_size", self.page_size)`, so tests must set the attribute on the view to affect pagination. If the view class does not define `pagination_page_size`, using `unittest.mock.patch(..., create=True)` is intentional and correct because the attribute may not exist until patched.
Applied to files:
openwisp_controller/connection/tests/pytest.pyopenwisp_controller/geo/tests/test_api.pytests/openwisp2/settings.pyopenwisp_controller/connection/tests/test_selenium.pytests/openwisp2/sample_connection/models.pyopenwisp_controller/geo/estimated_location/tests/tests.pytests/openwisp2/sample_connection/migrations/0005_batchcommand_command_batch_command.pyopenwisp_controller/connection/tests/utils.pytests/openwisp2/sample_connection/api/views.pyopenwisp_controller/connection/tests/test_tasks.pyopenwisp_controller/connection/tests/test_models.pyopenwisp_controller/connection/tests/test_api.py
📚 Learning: 2026-06-25T12:20:45.387Z
Learnt from: dee077
Repo: openwisp/openwisp-controller PR: 1395
File: openwisp_controller/connection/tests/test_api.py:916-932
Timestamp: 2026-06-25T12:20:45.387Z
Learning: When reviewing API pagination behavior in openwisp-controller, assume `OpenWispPagination.paginate_queryset()` allows a per-view page-size override via `getattr(view, "pagination_page_size", self.page_size)` (so `view.pagination_page_size`, if present, should affect pagination). In Python tests, it is valid to patch `pagination_page_size` on a view class even if the attribute isn’t declared on the class by default, by using `unittest.mock.patch.object(..., "pagination_page_size", ..., create=True)` so the override is available for the pagination logic during the test.
Applied to files:
openwisp_controller/connection/tests/pytest.pyopenwisp_controller/geo/tests/test_api.pyopenwisp_controller/connection/tests/test_selenium.pyopenwisp_controller/geo/estimated_location/tests/tests.pyopenwisp_controller/connection/tests/utils.pyopenwisp_controller/connection/tests/test_tasks.pyopenwisp_controller/connection/tests/test_models.pyopenwisp_controller/connection/tests/test_api.py
📚 Learning: 2026-06-07T12:07:25.164Z
Learnt from: stktyagi
Repo: openwisp/openwisp-controller PR: 1378
File: openwisp_controller/config/tests/test_config.py:864-865
Timestamp: 2026-06-07T12:07:25.164Z
Learning: When reviewing this repo’s Python test suite, treat changes to the *expected* query count in `assertNumQueries(...)` calls as routine test maintenance. If a PR updates the numeric argument (e.g., in `test_config.py`, `test_api.py`, `test_admin.py`, `test_pki.py`) and the test remains consistent with the feature changes, reviewers should not flag the increased number as a performance regression that requires investigation solely because the count went up; instead, focus on whether the update is intentional and the surrounding test/code changes justify the revised expectation.
Applied to files:
openwisp_controller/geo/tests/test_api.pyopenwisp_controller/connection/tests/test_selenium.pyopenwisp_controller/geo/estimated_location/tests/tests.pyopenwisp_controller/connection/tests/test_tasks.pyopenwisp_controller/connection/tests/test_models.pyopenwisp_controller/connection/tests/test_api.py
📚 Learning: 2026-03-27T20:50:26.240Z
Learnt from: nemesifier
Repo: openwisp/openwisp-controller PR: 1315
File: openwisp_controller/geo/estimated_location/service.py:70-76
Timestamp: 2026-03-27T20:50:26.240Z
Learning: In openwisp-controller’s WHOIS and estimated-location services (openwisp_controller/config/whois/ and openwisp_controller/geo/estimated_location/), these components only process public IP addresses. When reviewing logs/error/debug messages in this area, treat logging the IP address as acceptable and do not flag it as a privacy/security concern—unless the logged value can originate from non-public/private IPs in that specific code path.
Applied to files:
openwisp_controller/geo/estimated_location/tests/tests.py
🪛 ast-grep (0.44.1)
openwisp_controller/connection/tasks.py
[warning] 102-102: Loading a Keras model from an untrusted file can execute arbitrary code via Lambda layers or custom objects. Load only trusted models and avoid deserializing custom objects from untrusted sources.
Context: load_model("connection", "BatchCommand")
Note: [CWE-502] Deserialization of Untrusted Data.
(keras-load-model-python)
openwisp_controller/connection/migrations/0011_batchcommand_command_batch_command.py
[info] 60-69: use help_text to document model columns
Context: models.CharField(
choices=[
("idle", "idle"),
("in-progress", "in progress"),
("success", "success"),
("failed", "failed"),
],
default="idle",
max_length=12,
)
Note: [CWE-710] Improper Adherence to Coding Standards.
(model-help-text)
[info] 73-80: use help_text to document model columns
Context: models.CharField(
choices=(
openwisp_controller.connection.commands.COMMAND_CHOICES
if django.VERSION < (5, 0)
else openwisp_controller.connection.commands.get_command_choices # noqa: E501
),
max_length=16,
)
Note: [CWE-710] Improper Adherence to Coding Standards.
(model-help-text)
openwisp_controller/connection/base/models.py
[info] 751-753: use help_text to document model columns
Context: models.CharField(
max_length=12, choices=STATUS_CHOICES, default=STATUS_CHOICES[0][0]
)
Note: [CWE-710] Improper Adherence to Coding Standards.
(model-help-text)
[info] 754-757: use help_text to document model columns
Context: models.CharField(
max_length=16,
choices=(COMMAND_CHOICES if django.VERSION < (5, 0) else get_command_choices),
)
Note: [CWE-710] Improper Adherence to Coding Standards.
(model-help-text)
[warning] 859-859: Loading a Keras model from an untrusted file can execute arbitrary code via Lambda layers or custom objects. Load only trusted models and avoid deserializing custom objects from untrusted sources.
Context: load_model("connection", "Command")
Note: [CWE-502] Deserialization of Untrusted Data.
(keras-load-model-python)
[warning] 886-886: Loading a Keras model from an untrusted file can execute arbitrary code via Lambda layers or custom objects. Load only trusted models and avoid deserializing custom objects from untrusted sources.
Context: load_model("config", "Device")
Note: [CWE-502] Deserialization of Untrusted Data.
(keras-load-model-python)
[warning] 961-961: Loading a Keras model from an untrusted file can execute arbitrary code via Lambda layers or custom objects. Load only trusted models and avoid deserializing custom objects from untrusted sources.
Context: load_model("connection", "Command")
Note: [CWE-502] Deserialization of Untrusted Data.
(keras-load-model-python)
[warning] 962-962: Loading a Keras model from an untrusted file can execute arbitrary code via Lambda layers or custom objects. Load only trusted models and avoid deserializing custom objects from untrusted sources.
Context: load_model("config", "Device")
Note: [CWE-502] Deserialization of Untrusted Data.
(keras-load-model-python)
tests/openwisp2/sample_connection/migrations/0005_batchcommand_command_batch_command.py
[info] 58-67: use help_text to document model columns
Context: models.CharField(
choices=[
("idle", "idle"),
("in-progress", "in progress"),
("success", "success"),
("failed", "failed"),
],
default="idle",
max_length=12,
)
Note: [CWE-710] Improper Adherence to Coding Standards.
(model-help-text)
[info] 71-78: use help_text to document model columns
Context: models.CharField(
max_length=16,
choices=(
connection_config.commands.COMMAND_CHOICES
if django.VERSION < (5, 0)
else connection_config.commands.get_command_choices
),
)
Note: [CWE-710] Improper Adherence to Coding Standards.
(model-help-text)
openwisp_controller/connection/tests/test_tasks.py
[info] 448-448: use jsonify instead of json.dumps for JSON output
Context: json.dumps(batch.input)
Note: [CWE-116] Improper Encoding or Escaping of Output.
(use-jsonify)
openwisp_controller/connection/tests/test_models.py
[warning] 35-35: Loading a Keras model from an untrusted file can execute arbitrary code via Lambda layers or custom objects. Load only trusted models and avoid deserializing custom objects from untrusted sources.
Context: load_model("config", "DeviceGroup")
Note: [CWE-502] Deserialization of Untrusted Data.
(keras-load-model-python)
[warning] 36-36: Loading a Keras model from an untrusted file can execute arbitrary code via Lambda layers or custom objects. Load only trusted models and avoid deserializing custom objects from untrusted sources.
Context: load_model("geo", "Location")
Note: [CWE-502] Deserialization of Untrusted Data.
(keras-load-model-python)
[warning] 37-37: Loading a Keras model from an untrusted file can execute arbitrary code via Lambda layers or custom objects. Load only trusted models and avoid deserializing custom objects from untrusted sources.
Context: load_model("geo", "DeviceLocation")
Note: [CWE-502] Deserialization of Untrusted Data.
(keras-load-model-python)
openwisp_controller/connection/tests/test_api.py
[warning] 21-21: Loading a Keras model from an untrusted file can execute arbitrary code via Lambda layers or custom objects. Load only trusted models and avoid deserializing custom objects from untrusted sources.
Context: load_model("connection", "Command")
Note: [CWE-502] Deserialization of Untrusted Data.
(keras-load-model-python)
[warning] 22-22: Loading a Keras model from an untrusted file can execute arbitrary code via Lambda layers or custom objects. Load only trusted models and avoid deserializing custom objects from untrusted sources.
Context: load_model("connection", "DeviceConnection")
Note: [CWE-502] Deserialization of Untrusted Data.
(keras-load-model-python)
[warning] 23-23: Loading a Keras model from an untrusted file can execute arbitrary code via Lambda layers or custom objects. Load only trusted models and avoid deserializing custom objects from untrusted sources.
Context: load_model("connection", "BatchCommand")
Note: [CWE-502] Deserialization of Untrusted Data.
(keras-load-model-python)
[warning] 25-25: Loading a Keras model from an untrusted file can execute arbitrary code via Lambda layers or custom objects. Load only trusted models and avoid deserializing custom objects from untrusted sources.
Context: load_model("openwisp_users", "OrganizationUser")
Note: [CWE-502] Deserialization of Untrusted Data.
(keras-load-model-python)
[warning] 26-26: Loading a Keras model from an untrusted file can execute arbitrary code via Lambda layers or custom objects. Load only trusted models and avoid deserializing custom objects from untrusted sources.
Context: load_model("openwisp_users", "Group")
Note: [CWE-502] Deserialization of Untrusted Data.
(keras-load-model-python)
[warning] 27-27: Loading a Keras model from an untrusted file can execute arbitrary code via Lambda layers or custom objects. Load only trusted models and avoid deserializing custom objects from untrusted sources.
Context: load_model("config", "DeviceGroup")
Note: [CWE-502] Deserialization of Untrusted Data.
(keras-load-model-python)
[warning] 28-28: Loading a Keras model from an untrusted file can execute arbitrary code via Lambda layers or custom objects. Load only trusted models and avoid deserializing custom objects from untrusted sources.
Context: load_model("geo", "Location")
Note: [CWE-502] Deserialization of Untrusted Data.
(keras-load-model-python)
[warning] 29-29: Loading a Keras model from an untrusted file can execute arbitrary code via Lambda layers or custom objects. Load only trusted models and avoid deserializing custom objects from untrusted sources.
Context: load_model("geo", "DeviceLocation")
Note: [CWE-502] Deserialization of Untrusted Data.
(keras-load-model-python)
[info] 1070-1070: use jsonify instead of json.dumps for JSON output
Context: json.dumps(payload)
Note: [CWE-116] Improper Encoding or Escaping of Output.
(use-jsonify)
[info] 1107-1107: use jsonify instead of json.dumps for JSON output
Context: json.dumps(payload)
Note: [CWE-116] Improper Encoding or Escaping of Output.
(use-jsonify)
[info] 1133-1133: use jsonify instead of json.dumps for JSON output
Context: json.dumps(payload)
Note: [CWE-116] Improper Encoding or Escaping of Output.
(use-jsonify)
[info] 1149-1149: use jsonify instead of json.dumps for JSON output
Context: json.dumps(payload)
Note: [CWE-116] Improper Encoding or Escaping of Output.
(use-jsonify)
[info] 1173-1173: use jsonify instead of json.dumps for JSON output
Context: json.dumps(payload)
Note: [CWE-116] Improper Encoding or Escaping of Output.
(use-jsonify)
[info] 1198-1198: use jsonify instead of json.dumps for JSON output
Context: json.dumps(payload)
Note: [CWE-116] Improper Encoding or Escaping of Output.
(use-jsonify)
[info] 1235-1241: use jsonify instead of json.dumps for JSON output
Context: json.dumps(
{
"type": "custom",
"input": {"command": "echo test"},
"label": "test-label",
}
)
Note: [CWE-116] Improper Encoding or Escaping of Output.
(use-jsonify)
[info] 1251-1258: use jsonify instead of json.dumps for JSON output
Context: json.dumps(
{
"type": "custom",
"input": {"command": "echo test"},
"label": "test-label",
"devices": [str(device1.pk), str(device2.pk)],
}
)
Note: [CWE-116] Improper Encoding or Escaping of Output.
(use-jsonify)
[info] 1308-1308: use jsonify instead of json.dumps for JSON output
Context: json.dumps(payload)
Note: [CWE-116] Improper Encoding or Escaping of Output.
(use-jsonify)
[info] 1331-1331: use jsonify instead of json.dumps for JSON output
Context: json.dumps(payload)
Note: [CWE-116] Improper Encoding or Escaping of Output.
(use-jsonify)
[info] 1362-1362: use jsonify instead of json.dumps for JSON output
Context: json.dumps(payload)
Note: [CWE-116] Improper Encoding or Escaping of Output.
(use-jsonify)
[info] 1385-1385: use jsonify instead of json.dumps for JSON output
Context: json.dumps(payload)
Note: [CWE-116] Improper Encoding or Escaping of Output.
(use-jsonify)
[info] 1421-1421: use jsonify instead of json.dumps for JSON output
Context: json.dumps(payload)
Note: [CWE-116] Improper Encoding or Escaping of Output.
(use-jsonify)
[info] 1442-1442: use jsonify instead of json.dumps for JSON output
Context: json.dumps(payload)
Note: [CWE-116] Improper Encoding or Escaping of Output.
(use-jsonify)
[info] 1473-1473: use jsonify instead of json.dumps for JSON output
Context: json.dumps(payload)
Note: [CWE-116] Improper Encoding or Escaping of Output.
(use-jsonify)
[info] 1494-1494: use jsonify instead of json.dumps for JSON output
Context: json.dumps(payload)
Note: [CWE-116] Improper Encoding or Escaping of Output.
(use-jsonify)
[info] 1547-1547: use jsonify instead of json.dumps for JSON output
Context: json.dumps(list_response.data)
Note: [CWE-116] Improper Encoding or Escaping of Output.
(use-jsonify)
[info] 1548-1548: use jsonify instead of json.dumps for JSON output
Context: json.dumps(list_response.data)
Note: [CWE-116] Improper Encoding or Escaping of Output.
(use-jsonify)
[info] 1549-1549: use jsonify instead of json.dumps for JSON output
Context: json.dumps(list_response.data)
Note: [CWE-116] Improper Encoding or Escaping of Output.
(use-jsonify)
[info] 1551-1551: use jsonify instead of json.dumps for JSON output
Context: json.dumps(list_response.data)
Note: [CWE-116] Improper Encoding or Escaping of Output.
(use-jsonify)
[info] 1557-1557: use jsonify instead of json.dumps for JSON output
Context: json.dumps(detail_response.data)
Note: [CWE-116] Improper Encoding or Escaping of Output.
(use-jsonify)
[info] 1558-1558: use jsonify instead of json.dumps for JSON output
Context: json.dumps(detail_response.data)
Note: [CWE-116] Improper Encoding or Escaping of Output.
(use-jsonify)
[info] 1559-1559: use jsonify instead of json.dumps for JSON output
Context: json.dumps(detail_response.data)
Note: [CWE-116] Improper Encoding or Escaping of Output.
(use-jsonify)
[info] 1561-1561: use jsonify instead of json.dumps for JSON output
Context: json.dumps(detail_response.data)
Note: [CWE-116] Improper Encoding or Escaping of Output.
(use-jsonify)
[info] 1588-1588: use jsonify instead of json.dumps for JSON output
Context: json.dumps({"type": "custom"})
Note: [CWE-116] Improper Encoding or Escaping of Output.
(use-jsonify)
[info] 1642-1642: use jsonify instead of json.dumps for JSON output
Context: json.dumps(payload)
Note: [CWE-116] Improper Encoding or Escaping of Output.
(use-jsonify)
[info] 1699-1699: use jsonify instead of json.dumps for JSON output
Context: json.dumps(payload)
Note: [CWE-116] Improper Encoding or Escaping of Output.
(use-jsonify)
[info] 1717-1725: use jsonify instead of json.dumps for JSON output
Context: json.dumps(
{
"organization": str(org.pk),
"type": "custom",
"input": {"command": "echo test"},
"label": "test-label",
"devices": [str(device_org2.pk)],
}
)
Note: [CWE-116] Improper Encoding or Escaping of Output.
(use-jsonify)
[info] 1746-1754: use jsonify instead of json.dumps for JSON output
Context: json.dumps(
{
"organization": str(org.pk),
"type": "custom",
"input": {"command": "echo test"},
"label": "test-label",
"group": str(group_org2.pk),
}
)
Note: [CWE-116] Improper Encoding or Escaping of Output.
(use-jsonify)
[info] 1778-1786: use jsonify instead of json.dumps for JSON output
Context: json.dumps(
{
"organization": str(org.pk),
"type": "custom",
"input": {"command": "echo test"},
"label": "test-label",
"location": str(location_org2.pk),
}
)
Note: [CWE-116] Improper Encoding or Escaping of Output.
(use-jsonify)
[info] 1923-1931: use jsonify instead of json.dumps for JSON output
Context: json.dumps(
{
"organization": str(org.pk),
"type": "custom",
"input": {"command": "echo test"},
"label": "test-label",
"devices": [str(device1.pk)],
}
)
Note: [CWE-116] Improper Encoding or Escaping of Output.
(use-jsonify)
[info] 1943-1951: use jsonify instead of json.dumps for JSON output
Context: json.dumps(
{
"organization": str(org.pk),
"type": "custom",
"input": {"command": "echo test"},
"label": "test-label",
"group": str(group.pk),
}
)
Note: [CWE-116] Improper Encoding or Escaping of Output.
(use-jsonify)
[info] 1963-1971: use jsonify instead of json.dumps for JSON output
Context: json.dumps(
{
"organization": str(org.pk),
"type": "custom",
"input": {"command": "echo test"},
"label": "test-label",
"location": str(location.pk),
}
)
Note: [CWE-116] Improper Encoding or Escaping of Output.
(use-jsonify)
[info] 1984-1993: use jsonify instead of json.dumps for JSON output
Context: json.dumps(
{
"organization": str(org.pk),
"type": "custom",
"input": {"command": "echo test"},
"label": "test-label",
"group": str(group.pk),
"location": str(location.pk),
}
)
Note: [CWE-116] Improper Encoding or Escaping of Output.
(use-jsonify)
[info] 2005-2012: use jsonify instead of json.dumps for JSON output
Context: json.dumps(
{
"organization": str(org.pk),
"type": "custom",
"input": {"command": "echo test"},
"label": "test-label",
}
)
Note: [CWE-116] Improper Encoding or Escaping of Output.
(use-jsonify)
[info] 2027-2035: use jsonify instead of json.dumps for JSON output
Context: json.dumps(
{
"organization": str(org.pk),
"type": "custom",
"input": {"command": "echo test"},
"label": "test-label",
"devices": [],
}
)
Note: [CWE-116] Improper Encoding or Escaping of Output.
(use-jsonify)
[info] 2047-2054: use jsonify instead of json.dumps for JSON output
Context: json.dumps(
{
"organization": str(org.pk),
"type": "custom",
"input": {"command": "echo test"},
"label": "test-label",
}
)
Note: [CWE-116] Improper Encoding or Escaping of Output.
(use-jsonify)
[info] 2068-2076: use jsonify instead of json.dumps for JSON output
Context: json.dumps(
{
"organization": str(org.pk),
"type": "custom",
"input": {"command": "echo test"},
"label": "",
"devices": [str(device1.pk)],
}
)
Note: [CWE-116] Improper Encoding or Escaping of Output.
(use-jsonify)
[info] 2084-2092: use jsonify instead of json.dumps for JSON output
Context: json.dumps(
{
"organization": str(org.pk),
"type": "custom",
"input": {"command": "echo test"},
"label": "a" * 65,
"devices": [str(device1.pk)],
}
)
Note: [CWE-116] Improper Encoding or Escaping of Output.
(use-jsonify)
[info] 2100-2108: use jsonify instead of json.dumps for JSON output
Context: json.dumps(
{
"organization": str(org.pk),
"type": "custom",
"input": {},
"label": "test-label",
"devices": [str(device1.pk)],
}
)
Note: [CWE-116] Improper Encoding or Escaping of Output.
(use-jsonify)
[info] 2116-2124: use jsonify instead of json.dumps for JSON output
Context: json.dumps(
{
"organization": str(org.pk),
"type": "nonexistent",
"input": {"command": "echo test"},
"label": "test-label",
"devices": [str(device1.pk)],
}
)
Note: [CWE-116] Improper Encoding or Escaping of Output.
(use-jsonify)
[info] 2142-2153: use jsonify instead of json.dumps for JSON output
Context: json.dumps(
{
"organization": str(org.pk),
"type": "change_password",
"input": {
"password": password,
"confirm_password": password,
},
"label": "change password",
"devices": [str(device.pk)],
}
)
Note: [CWE-116] Improper Encoding or Escaping of Output.
(use-jsonify)
[info] 2158-2158: use jsonify instead of json.dumps for JSON output
Context: json.dumps(batch.input)
Note: [CWE-116] Improper Encoding or Escaping of Output.
(use-jsonify)
[info] 2161-2161: use jsonify instead of json.dumps for JSON output
Context: json.dumps(list_response.data)
Note: [CWE-116] Improper Encoding or Escaping of Output.
(use-jsonify)
[info] 2166-2166: use jsonify instead of json.dumps for JSON output
Context: json.dumps(detail_response.data)
Note: [CWE-116] Improper Encoding or Escaping of Output.
(use-jsonify)
[info] 2199-2199: use jsonify instead of json.dumps for JSON output
Context: json.dumps(payload)
Note: [CWE-116] Improper Encoding or Escaping of Output.
(use-jsonify)
[info] 2251-2259: use jsonify instead of json.dumps for JSON output
Context: json.dumps(
{
"organization": str(org.pk),
"type": "custom",
"input": {"command": "echo test"},
"label": "test-label",
"devices": [str(device.pk)],
}
)
Note: [CWE-116] Improper Encoding or Escaping of Output.
(use-jsonify)
[info] 2293-2301: use jsonify instead of json.dumps for JSON output
Context: json.dumps(
{
"organization": str(org.pk),
"type": "custom",
"input": {"command": "echo test"},
"label": "test-label",
"devices": [str(device.pk)],
}
)
Note: [CWE-116] Improper Encoding or Escaping of Output.
(use-jsonify)
🪛 Betterleaks (1.6.1)
docs/user/rest-api.rst
[high] 466-468: Discovered a potential authorization token provided in a curl command header, which could compromise the curl accessed resource.
(curl-auth-header)
[high] 558-560: Discovered a potential authorization token provided in a curl command header, which could compromise the curl accessed resource.
(curl-auth-header)
[high] 933-935: Discovered a potential authorization token provided in a curl command header, which could compromise the curl accessed resource.
(curl-auth-header)
[high] 952-954: Discovered a potential authorization token provided in a curl command header, which could compromise the curl accessed resource.
(curl-auth-header)
🔇 Additional comments (21)
openwisp_controller/connection/models.py (1)
3-8: LGTM!Also applies to: 27-32
openwisp_controller/connection/migrations/0011_batchcommand_command_batch_command.py (1)
31-181: LGTM!openwisp_controller/connection/migrations/__init__.py (1)
65-89: LGTM!tests/openwisp2/sample_connection/migrations/0005_batchcommand_command_batch_command.py (1)
29-176: LGTM!tests/openwisp2/sample_connection/models.py (1)
4-4: LGTM!Also applies to: 33-35
tests/openwisp2/settings.py (1)
296-296: LGTM!tests/openwisp2/sample_connection/api/views.py (1)
54-74: LGTM!openwisp_controller/connection/base/models.py (2)
1043-1054: Previously raised: mixed successful/skipped batches never becomefailed.Once all created commands succeed,
skipped_devicesblocks the success branch, but the fallback preservesin-progress(or a prematuresuccess). Handle non-emptyskipped_devicesin the failed branch after the in-progress check.
18-18: LGTM!Also applies to: 34-38, 475-481, 572-573, 738-1042, 1055-1057
docs/user/shell-commands.rst (2)
199-203: Previously raised: document explicit-device precedence.“Any combination” implies that
devices,group, andlocationare combined, but explicitdevicesoverride the other filters. State that precedence to prevent misleading API usage.
185-198: LGTM!Also applies to: 205-211
openwisp_controller/connection/tasks.py (1)
101-120: LGTM!docs/user/intro.rst (1)
73-75: LGTM!docs/developer/extending.rst (1)
357-357: LGTM!openwisp_controller/connection/tests/test_tasks.py (1)
1-1: LGTM!Also applies to: 20-20, 320-498
openwisp_controller/connection/api/serializers.py (1)
215-217: 📐 Maintainability & Code Quality | 💤 Low valueMark the fallback string for translation.
The fallback string
"some other organization"is injected into the API response when a device's organization is missing from the map, but it is currently not marked for translation. As per coding guidelines, user-facing strings in Django code must be marked for translation.🌐 Proposed fix
else: org_name = device_org_map.get( - uuid.UUID(device_pk), "some other organization" + uuid.UUID(device_pk), _("some other organization") )Source: Coding guidelines
openwisp_controller/connection/tests/pytest.py (1)
68-68: LGTM!openwisp_controller/connection/tests/test_api.py (1)
6-6: LGTM!Also applies to: 18-30, 869-2129, 2169-2319
openwisp_controller/connection/tests/test_models.py (1)
35-38: LGTM!Also applies to: 467-474, 975-1652, 1727-1747
openwisp_controller/connection/tests/utils.py (1)
14-14: LGTM!Also applies to: 122-139
openwisp_controller/connection/tests/test_selenium.py (1)
1-2: LGTM!Also applies to: 58-60, 70-70
|
@coderabbitai full revoew |
|
The Comment image shows a Swagger-like Batch Command Execute API page for
✅ Action performedFull review finished. |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs/user/shell-commands.rst`:
- Around line 197-207: Update the “Targeting options” documentation to state
that when devices is provided, it explicitly determines the targets and
overrides group and location; clarify that group and location are only applied
when devices is absent, replacing the current “any combination” wording.
In `@openwisp_controller/connection/base/models.py`:
- Around line 879-997: Update resolve_devices() to apply
select_related("config") to the Device queryset before returning its iterator,
including both explicitly targeted and filtered devices, so create_commands()
can validate command.full_clean() without an extra config query per device.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 84d48b9f-564a-4f12-ba46-650fbb0b3f53
📒 Files selected for processing (24)
docs/developer/extending.rstdocs/user/intro.rstdocs/user/rest-api.rstdocs/user/shell-commands.rstopenwisp_controller/connection/api/serializers.pyopenwisp_controller/connection/api/urls.pyopenwisp_controller/connection/api/views.pyopenwisp_controller/connection/base/models.pyopenwisp_controller/connection/migrations/0011_batchcommand_command_batch_command.pyopenwisp_controller/connection/migrations/__init__.pyopenwisp_controller/connection/models.pyopenwisp_controller/connection/tasks.pyopenwisp_controller/connection/tests/pytest.pyopenwisp_controller/connection/tests/test_api.pyopenwisp_controller/connection/tests/test_models.pyopenwisp_controller/connection/tests/test_selenium.pyopenwisp_controller/connection/tests/test_tasks.pyopenwisp_controller/connection/tests/utils.pyopenwisp_controller/geo/estimated_location/tests/tests.pyopenwisp_controller/geo/tests/test_api.pytests/openwisp2/sample_connection/api/views.pytests/openwisp2/sample_connection/migrations/0005_batchcommand_command_batch_command.pytests/openwisp2/sample_connection/models.pytests/openwisp2/settings.py
📜 Review details
⏰ Context from checks skipped due to timeout. (12)
- GitHub Check: Python==3.10 | django~=5.1.0
- GitHub Check: Python==3.13 | django~=5.1.0
- GitHub Check: Python==3.10 | django~=5.2.0
- GitHub Check: Python==3.11 | django~=4.2.0
- GitHub Check: Python==3.12 | django~=5.2.0
- GitHub Check: Python==3.11 | django~=5.2.0
- GitHub Check: Python==3.12 | django~=5.1.0
- GitHub Check: Python==3.12 | django~=4.2.0
- GitHub Check: Python==3.13 | django~=5.2.0
- GitHub Check: Python==3.11 | django~=5.1.0
- GitHub Check: Python==3.10 | django~=4.2.0
- GitHub Check: Kilo Code Review
🧰 Additional context used
📓 Path-based instructions (2)
**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
**/*.py: Mark user-facing strings for translation with Django i18n helpers in Django code
Avoid unnecessary blank lines inside function and method bodies
Be careful with authentication, authorization, queryset filtering, serializers, admin behavior, cache invalidation, signals, Celery tasks, and websocket updates in Django code
Preserve validation around templates, VPN/PKI material, SSH credentials, device commands, uploaded files, URLs, and subnet/IP data
Write comments and docstrings only when they explain why code is shaped a certain way, placing them before the relevant code block instead of scattering them inside it
Files:
openwisp_controller/connection/tests/pytest.pytests/openwisp2/settings.pyopenwisp_controller/connection/api/urls.pyopenwisp_controller/connection/tasks.pyopenwisp_controller/connection/tests/utils.pyopenwisp_controller/geo/tests/test_api.pyopenwisp_controller/geo/estimated_location/tests/tests.pytests/openwisp2/sample_connection/api/views.pyopenwisp_controller/connection/tests/test_selenium.pyopenwisp_controller/connection/migrations/0011_batchcommand_command_batch_command.pyopenwisp_controller/connection/models.pytests/openwisp2/sample_connection/models.pyopenwisp_controller/connection/migrations/__init__.pytests/openwisp2/sample_connection/migrations/0005_batchcommand_command_batch_command.pyopenwisp_controller/connection/api/views.pyopenwisp_controller/connection/api/serializers.pyopenwisp_controller/connection/tests/test_tasks.pyopenwisp_controller/connection/base/models.pyopenwisp_controller/connection/tests/test_models.pyopenwisp_controller/connection/tests/test_api.py
**/*.{py,html}
📄 CodeRabbit inference engine (Custom checks)
For Django pull requests, ensure all user-facing strings are marked as translatable using the Django i18n framework.
Files:
openwisp_controller/connection/tests/pytest.pytests/openwisp2/settings.pyopenwisp_controller/connection/api/urls.pyopenwisp_controller/connection/tasks.pyopenwisp_controller/connection/tests/utils.pyopenwisp_controller/geo/tests/test_api.pyopenwisp_controller/geo/estimated_location/tests/tests.pytests/openwisp2/sample_connection/api/views.pyopenwisp_controller/connection/tests/test_selenium.pyopenwisp_controller/connection/migrations/0011_batchcommand_command_batch_command.pyopenwisp_controller/connection/models.pytests/openwisp2/sample_connection/models.pyopenwisp_controller/connection/migrations/__init__.pytests/openwisp2/sample_connection/migrations/0005_batchcommand_command_batch_command.pyopenwisp_controller/connection/api/views.pyopenwisp_controller/connection/api/serializers.pyopenwisp_controller/connection/tests/test_tasks.pyopenwisp_controller/connection/base/models.pyopenwisp_controller/connection/tests/test_models.pyopenwisp_controller/connection/tests/test_api.py
🧠 Learnings (9)
📚 Learning: 2026-01-15T15:05:49.557Z
Learnt from: DragnEmperor
Repo: openwisp/openwisp-controller PR: 1175
File: openwisp_controller/config/management/commands/clear_last_ip.py:38-42
Timestamp: 2026-01-15T15:05:49.557Z
Learning: In Django projects, when using select_related() to traverse relations (for example, select_related("organization__config_settings")), the traversed relation must not be deferred. If you also use .only() in the same query, include the relation name or FK field (e.g., "organization" or "organization_id") in the .only() list to avoid the error "Field X cannot be both deferred and traversed using select_related at the same time." Apply this guideline to Django code in openwisp_controller/config/management/commands/clear_last_ip.py and similar modules by ensuring any select_related with an accompanying only() includes the related field names to prevent deferred/traversed conflicts.
Applied to files:
openwisp_controller/connection/tests/pytest.pyopenwisp_controller/connection/api/urls.pyopenwisp_controller/connection/tasks.pyopenwisp_controller/connection/tests/utils.pyopenwisp_controller/geo/tests/test_api.pyopenwisp_controller/geo/estimated_location/tests/tests.pyopenwisp_controller/connection/tests/test_selenium.pyopenwisp_controller/connection/migrations/0011_batchcommand_command_batch_command.pyopenwisp_controller/connection/models.pyopenwisp_controller/connection/migrations/__init__.pyopenwisp_controller/connection/api/views.pyopenwisp_controller/connection/api/serializers.pyopenwisp_controller/connection/tests/test_tasks.pyopenwisp_controller/connection/base/models.pyopenwisp_controller/connection/tests/test_models.pyopenwisp_controller/connection/tests/test_api.py
📚 Learning: 2026-02-17T19:13:10.088Z
Learnt from: nemesifier
Repo: openwisp/openwisp-controller PR: 1175
File: openwisp_controller/config/whois/commands.py:0-0
Timestamp: 2026-02-17T19:13:10.088Z
Learning: In reviews for the openwisp/openwisp-controller repository, do not propose changes based on Ruff warnings. The project does not use Ruff as its linter; ignore Ruff-related suggestions and follow the repository’s established linting and configuration rules. This guidance applies to all Python files under the openwisp_controller directory.
Applied to files:
openwisp_controller/connection/tests/pytest.pyopenwisp_controller/connection/api/urls.pyopenwisp_controller/connection/tasks.pyopenwisp_controller/connection/tests/utils.pyopenwisp_controller/geo/tests/test_api.pyopenwisp_controller/geo/estimated_location/tests/tests.pyopenwisp_controller/connection/tests/test_selenium.pyopenwisp_controller/connection/migrations/0011_batchcommand_command_batch_command.pyopenwisp_controller/connection/models.pyopenwisp_controller/connection/migrations/__init__.pyopenwisp_controller/connection/api/views.pyopenwisp_controller/connection/api/serializers.pyopenwisp_controller/connection/tests/test_tasks.pyopenwisp_controller/connection/base/models.pyopenwisp_controller/connection/tests/test_models.pyopenwisp_controller/connection/tests/test_api.py
📚 Learning: 2026-01-15T15:07:17.354Z
Learnt from: DragnEmperor
Repo: openwisp/openwisp-controller PR: 1175
File: openwisp_controller/geo/estimated_location/tests/tests.py:172-175
Timestamp: 2026-01-15T15:07:17.354Z
Learning: In this repository, flake8 enforces E501 (line too long) via setup.cfg (max-line-length = 88) while ruff ignores E501 via ruff.toml. Therefore, use '# noqa: E501' on lines that intentionally exceed 88 characters to satisfy flake8 without affecting ruff checks. This applies to Python files across the project (any .py) and is relevant for tests as well. Use sparingly and only where breaking lines is not feasible without hurting readability or functionality.
Applied to files:
openwisp_controller/connection/tests/pytest.pytests/openwisp2/settings.pyopenwisp_controller/connection/api/urls.pyopenwisp_controller/connection/tasks.pyopenwisp_controller/connection/tests/utils.pyopenwisp_controller/geo/tests/test_api.pyopenwisp_controller/geo/estimated_location/tests/tests.pytests/openwisp2/sample_connection/api/views.pyopenwisp_controller/connection/tests/test_selenium.pyopenwisp_controller/connection/migrations/0011_batchcommand_command_batch_command.pyopenwisp_controller/connection/models.pytests/openwisp2/sample_connection/models.pyopenwisp_controller/connection/migrations/__init__.pytests/openwisp2/sample_connection/migrations/0005_batchcommand_command_batch_command.pyopenwisp_controller/connection/api/views.pyopenwisp_controller/connection/api/serializers.pyopenwisp_controller/connection/tests/test_tasks.pyopenwisp_controller/connection/base/models.pyopenwisp_controller/connection/tests/test_models.pyopenwisp_controller/connection/tests/test_api.py
📚 Learning: 2026-06-07T12:07:08.468Z
Learnt from: stktyagi
Repo: openwisp/openwisp-controller PR: 1378
File: openwisp_controller/config/tests/test_admin.py:2335-2335
Timestamp: 2026-06-07T12:07:08.468Z
Learning: In this project’s Python test suite (files under openwisp_controller/**/tests/), don’t require or request prose/inline comments that document the breakdown of query-count changes (e.g., assertions around template/DB query counts in helpers like _verify_template_queries). Treat query-count assertions as volatile implementation details that change frequently; review should focus on whether the test asserts the expected behavior, not on explaining the specific query-count deltas in comments.
Applied to files:
openwisp_controller/connection/tests/pytest.pyopenwisp_controller/connection/tests/utils.pyopenwisp_controller/geo/tests/test_api.pyopenwisp_controller/geo/estimated_location/tests/tests.pyopenwisp_controller/connection/tests/test_selenium.pyopenwisp_controller/connection/tests/test_tasks.pyopenwisp_controller/connection/tests/test_models.pyopenwisp_controller/connection/tests/test_api.py
📚 Learning: 2026-06-07T12:07:24.608Z
Learnt from: stktyagi
Repo: openwisp/openwisp-controller PR: 1378
File: openwisp_controller/pki/tests/test_api.py:155-155
Timestamp: 2026-06-07T12:07:24.608Z
Learning: When reviewing Python test files in this repository, avoid recommending inline comments that explain or justify `assertNumQueries` (Django query count) expectations. Query counts can change frequently as implementations evolve, and inline explanations add maintenance burden; the expected count should be understandable without added comment blocks.
Applied to files:
openwisp_controller/connection/tests/pytest.pytests/openwisp2/settings.pyopenwisp_controller/connection/tests/utils.pyopenwisp_controller/geo/tests/test_api.pyopenwisp_controller/geo/estimated_location/tests/tests.pytests/openwisp2/sample_connection/api/views.pyopenwisp_controller/connection/tests/test_selenium.pytests/openwisp2/sample_connection/models.pytests/openwisp2/sample_connection/migrations/0005_batchcommand_command_batch_command.pyopenwisp_controller/connection/tests/test_tasks.pyopenwisp_controller/connection/tests/test_models.pyopenwisp_controller/connection/tests/test_api.py
📚 Learning: 2026-06-25T12:20:18.414Z
Learnt from: dee077
Repo: openwisp/openwisp-controller PR: 1395
File: openwisp_controller/connection/base/models.py:571-572
Timestamp: 2026-06-25T12:20:18.414Z
Learning: When writing or reviewing tests that override pagination behavior via OpenWispPagination.paginate_queryset(), patch `view.pagination_page_size` (not `page_size`). The method uses `getattr(view, "pagination_page_size", self.page_size)`, so tests must set the attribute on the view to affect pagination. If the view class does not define `pagination_page_size`, using `unittest.mock.patch(..., create=True)` is intentional and correct because the attribute may not exist until patched.
Applied to files:
openwisp_controller/connection/tests/pytest.pytests/openwisp2/settings.pyopenwisp_controller/connection/tests/utils.pyopenwisp_controller/geo/tests/test_api.pyopenwisp_controller/geo/estimated_location/tests/tests.pytests/openwisp2/sample_connection/api/views.pyopenwisp_controller/connection/tests/test_selenium.pytests/openwisp2/sample_connection/models.pytests/openwisp2/sample_connection/migrations/0005_batchcommand_command_batch_command.pyopenwisp_controller/connection/tests/test_tasks.pyopenwisp_controller/connection/tests/test_models.pyopenwisp_controller/connection/tests/test_api.py
📚 Learning: 2026-06-25T12:20:45.387Z
Learnt from: dee077
Repo: openwisp/openwisp-controller PR: 1395
File: openwisp_controller/connection/tests/test_api.py:916-932
Timestamp: 2026-06-25T12:20:45.387Z
Learning: When reviewing API pagination behavior in openwisp-controller, assume `OpenWispPagination.paginate_queryset()` allows a per-view page-size override via `getattr(view, "pagination_page_size", self.page_size)` (so `view.pagination_page_size`, if present, should affect pagination). In Python tests, it is valid to patch `pagination_page_size` on a view class even if the attribute isn’t declared on the class by default, by using `unittest.mock.patch.object(..., "pagination_page_size", ..., create=True)` so the override is available for the pagination logic during the test.
Applied to files:
openwisp_controller/connection/tests/pytest.pyopenwisp_controller/connection/tests/utils.pyopenwisp_controller/geo/tests/test_api.pyopenwisp_controller/geo/estimated_location/tests/tests.pyopenwisp_controller/connection/tests/test_selenium.pyopenwisp_controller/connection/tests/test_tasks.pyopenwisp_controller/connection/tests/test_models.pyopenwisp_controller/connection/tests/test_api.py
📚 Learning: 2026-06-07T12:07:25.164Z
Learnt from: stktyagi
Repo: openwisp/openwisp-controller PR: 1378
File: openwisp_controller/config/tests/test_config.py:864-865
Timestamp: 2026-06-07T12:07:25.164Z
Learning: When reviewing this repo’s Python test suite, treat changes to the *expected* query count in `assertNumQueries(...)` calls as routine test maintenance. If a PR updates the numeric argument (e.g., in `test_config.py`, `test_api.py`, `test_admin.py`, `test_pki.py`) and the test remains consistent with the feature changes, reviewers should not flag the increased number as a performance regression that requires investigation solely because the count went up; instead, focus on whether the update is intentional and the surrounding test/code changes justify the revised expectation.
Applied to files:
openwisp_controller/geo/tests/test_api.pyopenwisp_controller/geo/estimated_location/tests/tests.pyopenwisp_controller/connection/tests/test_selenium.pyopenwisp_controller/connection/tests/test_tasks.pyopenwisp_controller/connection/tests/test_models.pyopenwisp_controller/connection/tests/test_api.py
📚 Learning: 2026-03-27T20:50:26.240Z
Learnt from: nemesifier
Repo: openwisp/openwisp-controller PR: 1315
File: openwisp_controller/geo/estimated_location/service.py:70-76
Timestamp: 2026-03-27T20:50:26.240Z
Learning: In openwisp-controller’s WHOIS and estimated-location services (openwisp_controller/config/whois/ and openwisp_controller/geo/estimated_location/), these components only process public IP addresses. When reviewing logs/error/debug messages in this area, treat logging the IP address as acceptable and do not flag it as a privacy/security concern—unless the logged value can originate from non-public/private IPs in that specific code path.
Applied to files:
openwisp_controller/geo/estimated_location/tests/tests.py
🪛 ast-grep (0.44.1)
openwisp_controller/connection/tasks.py
[warning] 102-102: Loading a Keras model from an untrusted file can execute arbitrary code via Lambda layers or custom objects. Load only trusted models and avoid deserializing custom objects from untrusted sources.
Context: load_model("connection", "BatchCommand")
Note: [CWE-502] Deserialization of Untrusted Data.
(keras-load-model-python)
openwisp_controller/connection/migrations/0011_batchcommand_command_batch_command.py
[info] 60-69: use help_text to document model columns
Context: models.CharField(
choices=[
("idle", "idle"),
("in-progress", "in progress"),
("success", "success"),
("failed", "failed"),
],
default="idle",
max_length=12,
)
Note: [CWE-710] Improper Adherence to Coding Standards.
(model-help-text)
[info] 73-80: use help_text to document model columns
Context: models.CharField(
choices=(
openwisp_controller.connection.commands.COMMAND_CHOICES
if django.VERSION < (5, 0)
else openwisp_controller.connection.commands.get_command_choices # noqa: E501
),
max_length=16,
)
Note: [CWE-710] Improper Adherence to Coding Standards.
(model-help-text)
tests/openwisp2/sample_connection/migrations/0005_batchcommand_command_batch_command.py
[info] 58-67: use help_text to document model columns
Context: models.CharField(
choices=[
("idle", "idle"),
("in-progress", "in progress"),
("success", "success"),
("failed", "failed"),
],
default="idle",
max_length=12,
)
Note: [CWE-710] Improper Adherence to Coding Standards.
(model-help-text)
[info] 71-78: use help_text to document model columns
Context: models.CharField(
max_length=16,
choices=(
connection_config.commands.COMMAND_CHOICES
if django.VERSION < (5, 0)
else connection_config.commands.get_command_choices
),
)
Note: [CWE-710] Improper Adherence to Coding Standards.
(model-help-text)
openwisp_controller/connection/tests/test_tasks.py
[info] 448-448: use jsonify instead of json.dumps for JSON output
Context: json.dumps(batch.input)
Note: [CWE-116] Improper Encoding or Escaping of Output.
(use-jsonify)
openwisp_controller/connection/base/models.py
[info] 751-753: use help_text to document model columns
Context: models.CharField(
max_length=12, choices=STATUS_CHOICES, default=STATUS_CHOICES[0][0]
)
Note: [CWE-710] Improper Adherence to Coding Standards.
(model-help-text)
[info] 754-757: use help_text to document model columns
Context: models.CharField(
max_length=16,
choices=(COMMAND_CHOICES if django.VERSION < (5, 0) else get_command_choices),
)
Note: [CWE-710] Improper Adherence to Coding Standards.
(model-help-text)
[warning] 859-859: Loading a Keras model from an untrusted file can execute arbitrary code via Lambda layers or custom objects. Load only trusted models and avoid deserializing custom objects from untrusted sources.
Context: load_model("connection", "Command")
Note: [CWE-502] Deserialization of Untrusted Data.
(keras-load-model-python)
[warning] 886-886: Loading a Keras model from an untrusted file can execute arbitrary code via Lambda layers or custom objects. Load only trusted models and avoid deserializing custom objects from untrusted sources.
Context: load_model("config", "Device")
Note: [CWE-502] Deserialization of Untrusted Data.
(keras-load-model-python)
[warning] 961-961: Loading a Keras model from an untrusted file can execute arbitrary code via Lambda layers or custom objects. Load only trusted models and avoid deserializing custom objects from untrusted sources.
Context: load_model("connection", "Command")
Note: [CWE-502] Deserialization of Untrusted Data.
(keras-load-model-python)
[warning] 962-962: Loading a Keras model from an untrusted file can execute arbitrary code via Lambda layers or custom objects. Load only trusted models and avoid deserializing custom objects from untrusted sources.
Context: load_model("config", "Device")
Note: [CWE-502] Deserialization of Untrusted Data.
(keras-load-model-python)
openwisp_controller/connection/tests/test_models.py
[warning] 35-35: Loading a Keras model from an untrusted file can execute arbitrary code via Lambda layers or custom objects. Load only trusted models and avoid deserializing custom objects from untrusted sources.
Context: load_model("config", "DeviceGroup")
Note: [CWE-502] Deserialization of Untrusted Data.
(keras-load-model-python)
[warning] 36-36: Loading a Keras model from an untrusted file can execute arbitrary code via Lambda layers or custom objects. Load only trusted models and avoid deserializing custom objects from untrusted sources.
Context: load_model("geo", "Location")
Note: [CWE-502] Deserialization of Untrusted Data.
(keras-load-model-python)
[warning] 37-37: Loading a Keras model from an untrusted file can execute arbitrary code via Lambda layers or custom objects. Load only trusted models and avoid deserializing custom objects from untrusted sources.
Context: load_model("geo", "DeviceLocation")
Note: [CWE-502] Deserialization of Untrusted Data.
(keras-load-model-python)
openwisp_controller/connection/tests/test_api.py
[warning] 21-21: Loading a Keras model from an untrusted file can execute arbitrary code via Lambda layers or custom objects. Load only trusted models and avoid deserializing custom objects from untrusted sources.
Context: load_model("connection", "Command")
Note: [CWE-502] Deserialization of Untrusted Data.
(keras-load-model-python)
[warning] 22-22: Loading a Keras model from an untrusted file can execute arbitrary code via Lambda layers or custom objects. Load only trusted models and avoid deserializing custom objects from untrusted sources.
Context: load_model("connection", "DeviceConnection")
Note: [CWE-502] Deserialization of Untrusted Data.
(keras-load-model-python)
[warning] 23-23: Loading a Keras model from an untrusted file can execute arbitrary code via Lambda layers or custom objects. Load only trusted models and avoid deserializing custom objects from untrusted sources.
Context: load_model("connection", "BatchCommand")
Note: [CWE-502] Deserialization of Untrusted Data.
(keras-load-model-python)
[warning] 25-25: Loading a Keras model from an untrusted file can execute arbitrary code via Lambda layers or custom objects. Load only trusted models and avoid deserializing custom objects from untrusted sources.
Context: load_model("openwisp_users", "OrganizationUser")
Note: [CWE-502] Deserialization of Untrusted Data.
(keras-load-model-python)
[warning] 26-26: Loading a Keras model from an untrusted file can execute arbitrary code via Lambda layers or custom objects. Load only trusted models and avoid deserializing custom objects from untrusted sources.
Context: load_model("openwisp_users", "Group")
Note: [CWE-502] Deserialization of Untrusted Data.
(keras-load-model-python)
[warning] 27-27: Loading a Keras model from an untrusted file can execute arbitrary code via Lambda layers or custom objects. Load only trusted models and avoid deserializing custom objects from untrusted sources.
Context: load_model("config", "DeviceGroup")
Note: [CWE-502] Deserialization of Untrusted Data.
(keras-load-model-python)
[warning] 28-28: Loading a Keras model from an untrusted file can execute arbitrary code via Lambda layers or custom objects. Load only trusted models and avoid deserializing custom objects from untrusted sources.
Context: load_model("geo", "Location")
Note: [CWE-502] Deserialization of Untrusted Data.
(keras-load-model-python)
[warning] 29-29: Loading a Keras model from an untrusted file can execute arbitrary code via Lambda layers or custom objects. Load only trusted models and avoid deserializing custom objects from untrusted sources.
Context: load_model("geo", "DeviceLocation")
Note: [CWE-502] Deserialization of Untrusted Data.
(keras-load-model-python)
[info] 1070-1070: use jsonify instead of json.dumps for JSON output
Context: json.dumps(payload)
Note: [CWE-116] Improper Encoding or Escaping of Output.
(use-jsonify)
[info] 1107-1107: use jsonify instead of json.dumps for JSON output
Context: json.dumps(payload)
Note: [CWE-116] Improper Encoding or Escaping of Output.
(use-jsonify)
[info] 1133-1133: use jsonify instead of json.dumps for JSON output
Context: json.dumps(payload)
Note: [CWE-116] Improper Encoding or Escaping of Output.
(use-jsonify)
[info] 1149-1149: use jsonify instead of json.dumps for JSON output
Context: json.dumps(payload)
Note: [CWE-116] Improper Encoding or Escaping of Output.
(use-jsonify)
[info] 1173-1173: use jsonify instead of json.dumps for JSON output
Context: json.dumps(payload)
Note: [CWE-116] Improper Encoding or Escaping of Output.
(use-jsonify)
[info] 1198-1198: use jsonify instead of json.dumps for JSON output
Context: json.dumps(payload)
Note: [CWE-116] Improper Encoding or Escaping of Output.
(use-jsonify)
[info] 1235-1241: use jsonify instead of json.dumps for JSON output
Context: json.dumps(
{
"type": "custom",
"input": {"command": "echo test"},
"label": "test-label",
}
)
Note: [CWE-116] Improper Encoding or Escaping of Output.
(use-jsonify)
[info] 1251-1258: use jsonify instead of json.dumps for JSON output
Context: json.dumps(
{
"type": "custom",
"input": {"command": "echo test"},
"label": "test-label",
"devices": [str(device1.pk), str(device2.pk)],
}
)
Note: [CWE-116] Improper Encoding or Escaping of Output.
(use-jsonify)
[info] 1308-1308: use jsonify instead of json.dumps for JSON output
Context: json.dumps(payload)
Note: [CWE-116] Improper Encoding or Escaping of Output.
(use-jsonify)
[info] 1331-1331: use jsonify instead of json.dumps for JSON output
Context: json.dumps(payload)
Note: [CWE-116] Improper Encoding or Escaping of Output.
(use-jsonify)
[info] 1362-1362: use jsonify instead of json.dumps for JSON output
Context: json.dumps(payload)
Note: [CWE-116] Improper Encoding or Escaping of Output.
(use-jsonify)
[info] 1385-1385: use jsonify instead of json.dumps for JSON output
Context: json.dumps(payload)
Note: [CWE-116] Improper Encoding or Escaping of Output.
(use-jsonify)
[info] 1421-1421: use jsonify instead of json.dumps for JSON output
Context: json.dumps(payload)
Note: [CWE-116] Improper Encoding or Escaping of Output.
(use-jsonify)
[info] 1442-1442: use jsonify instead of json.dumps for JSON output
Context: json.dumps(payload)
Note: [CWE-116] Improper Encoding or Escaping of Output.
(use-jsonify)
[info] 1473-1473: use jsonify instead of json.dumps for JSON output
Context: json.dumps(payload)
Note: [CWE-116] Improper Encoding or Escaping of Output.
(use-jsonify)
[info] 1494-1494: use jsonify instead of json.dumps for JSON output
Context: json.dumps(payload)
Note: [CWE-116] Improper Encoding or Escaping of Output.
(use-jsonify)
[info] 1587-1587: use jsonify instead of json.dumps for JSON output
Context: json.dumps({"type": "custom"})
Note: [CWE-116] Improper Encoding or Escaping of Output.
(use-jsonify)
[info] 1641-1641: use jsonify instead of json.dumps for JSON output
Context: json.dumps(payload)
Note: [CWE-116] Improper Encoding or Escaping of Output.
(use-jsonify)
[info] 1698-1698: use jsonify instead of json.dumps for JSON output
Context: json.dumps(payload)
Note: [CWE-116] Improper Encoding or Escaping of Output.
(use-jsonify)
[info] 1716-1724: use jsonify instead of json.dumps for JSON output
Context: json.dumps(
{
"organization": str(org.pk),
"type": "custom",
"input": {"command": "echo test"},
"label": "test-label",
"devices": [str(device_org2.pk)],
}
)
Note: [CWE-116] Improper Encoding or Escaping of Output.
(use-jsonify)
[info] 1745-1753: use jsonify instead of json.dumps for JSON output
Context: json.dumps(
{
"organization": str(org.pk),
"type": "custom",
"input": {"command": "echo test"},
"label": "test-label",
"group": str(group_org2.pk),
}
)
Note: [CWE-116] Improper Encoding or Escaping of Output.
(use-jsonify)
[info] 1777-1785: use jsonify instead of json.dumps for JSON output
Context: json.dumps(
{
"organization": str(org.pk),
"type": "custom",
"input": {"command": "echo test"},
"label": "test-label",
"location": str(location_org2.pk),
}
)
Note: [CWE-116] Improper Encoding or Escaping of Output.
(use-jsonify)
[info] 1922-1930: use jsonify instead of json.dumps for JSON output
Context: json.dumps(
{
"organization": str(org.pk),
"type": "custom",
"input": {"command": "echo test"},
"label": "test-label",
"devices": [str(device1.pk)],
}
)
Note: [CWE-116] Improper Encoding or Escaping of Output.
(use-jsonify)
[info] 1942-1950: use jsonify instead of json.dumps for JSON output
Context: json.dumps(
{
"organization": str(org.pk),
"type": "custom",
"input": {"command": "echo test"},
"label": "test-label",
"group": str(group.pk),
}
)
Note: [CWE-116] Improper Encoding or Escaping of Output.
(use-jsonify)
[info] 1962-1970: use jsonify instead of json.dumps for JSON output
Context: json.dumps(
{
"organization": str(org.pk),
"type": "custom",
"input": {"command": "echo test"},
"label": "test-label",
"location": str(location.pk),
}
)
Note: [CWE-116] Improper Encoding or Escaping of Output.
(use-jsonify)
[info] 1983-1992: use jsonify instead of json.dumps for JSON output
Context: json.dumps(
{
"organization": str(org.pk),
"type": "custom",
"input": {"command": "echo test"},
"label": "test-label",
"group": str(group.pk),
"location": str(location.pk),
}
)
Note: [CWE-116] Improper Encoding or Escaping of Output.
(use-jsonify)
[info] 2004-2011: use jsonify instead of json.dumps for JSON output
Context: json.dumps(
{
"organization": str(org.pk),
"type": "custom",
"input": {"command": "echo test"},
"label": "test-label",
}
)
Note: [CWE-116] Improper Encoding or Escaping of Output.
(use-jsonify)
[info] 2026-2034: use jsonify instead of json.dumps for JSON output
Context: json.dumps(
{
"organization": str(org.pk),
"type": "custom",
"input": {"command": "echo test"},
"label": "test-label",
"devices": [],
}
)
Note: [CWE-116] Improper Encoding or Escaping of Output.
(use-jsonify)
[info] 2046-2053: use jsonify instead of json.dumps for JSON output
Context: json.dumps(
{
"organization": str(org.pk),
"type": "custom",
"input": {"command": "echo test"},
"label": "test-label",
}
)
Note: [CWE-116] Improper Encoding or Escaping of Output.
(use-jsonify)
[info] 2067-2075: use jsonify instead of json.dumps for JSON output
Context: json.dumps(
{
"organization": str(org.pk),
"type": "custom",
"input": {"command": "echo test"},
"label": "",
"devices": [str(device1.pk)],
}
)
Note: [CWE-116] Improper Encoding or Escaping of Output.
(use-jsonify)
[info] 2083-2091: use jsonify instead of json.dumps for JSON output
Context: json.dumps(
{
"organization": str(org.pk),
"type": "custom",
"input": {"command": "echo test"},
"label": "a" * 65,
"devices": [str(device1.pk)],
}
)
Note: [CWE-116] Improper Encoding or Escaping of Output.
(use-jsonify)
[info] 2099-2107: use jsonify instead of json.dumps for JSON output
Context: json.dumps(
{
"organization": str(org.pk),
"type": "custom",
"input": {},
"label": "test-label",
"devices": [str(device1.pk)],
}
)
Note: [CWE-116] Improper Encoding or Escaping of Output.
(use-jsonify)
[info] 2115-2123: use jsonify instead of json.dumps for JSON output
Context: json.dumps(
{
"organization": str(org.pk),
"type": "nonexistent",
"input": {"command": "echo test"},
"label": "test-label",
"devices": [str(device1.pk)],
}
)
Note: [CWE-116] Improper Encoding or Escaping of Output.
(use-jsonify)
[info] 2141-2152: use jsonify instead of json.dumps for JSON output
Context: json.dumps(
{
"organization": str(org.pk),
"type": "change_password",
"input": {
"password": password,
"confirm_password": password,
},
"label": "change password",
"devices": [str(device.pk)],
}
)
Note: [CWE-116] Improper Encoding or Escaping of Output.
(use-jsonify)
[info] 2157-2157: use jsonify instead of json.dumps for JSON output
Context: json.dumps(batch.input)
Note: [CWE-116] Improper Encoding or Escaping of Output.
(use-jsonify)
[info] 2160-2160: use jsonify instead of json.dumps for JSON output
Context: json.dumps(list_response.data)
Note: [CWE-116] Improper Encoding or Escaping of Output.
(use-jsonify)
[info] 2165-2165: use jsonify instead of json.dumps for JSON output
Context: json.dumps(detail_response.data)
Note: [CWE-116] Improper Encoding or Escaping of Output.
(use-jsonify)
[info] 2198-2198: use jsonify instead of json.dumps for JSON output
Context: json.dumps(payload)
Note: [CWE-116] Improper Encoding or Escaping of Output.
(use-jsonify)
[info] 2250-2258: use jsonify instead of json.dumps for JSON output
Context: json.dumps(
{
"organization": str(org.pk),
"type": "custom",
"input": {"command": "echo test"},
"label": "test-label",
"devices": [str(device.pk)],
}
)
Note: [CWE-116] Improper Encoding or Escaping of Output.
(use-jsonify)
[info] 2292-2300: use jsonify instead of json.dumps for JSON output
Context: json.dumps(
{
"organization": str(org.pk),
"type": "custom",
"input": {"command": "echo test"},
"label": "test-label",
"devices": [str(device.pk)],
}
)
Note: [CWE-116] Improper Encoding or Escaping of Output.
(use-jsonify)
🪛 Betterleaks (1.6.1)
docs/user/rest-api.rst
[high] 466-468: Discovered a potential authorization token provided in a curl command header, which could compromise the curl accessed resource.
(curl-auth-header)
[high] 558-560: Discovered a potential authorization token provided in a curl command header, which could compromise the curl accessed resource.
(curl-auth-header)
[high] 933-935: Discovered a potential authorization token provided in a curl command header, which could compromise the curl accessed resource.
(curl-auth-header)
[high] 952-954: Discovered a potential authorization token provided in a curl command header, which could compromise the curl accessed resource.
(curl-auth-header)
🔇 Additional comments (24)
openwisp_controller/connection/tests/pytest.py (1)
68-68: LGTM!openwisp_controller/connection/tests/utils.py (1)
14-14: LGTM!Also applies to: 122-139
openwisp_controller/connection/base/models.py (1)
437-736: LGTM!Also applies to: 738-878, 898-1057
openwisp_controller/connection/tasks.py (1)
101-122: LGTM!docs/user/rest-api.rst (1)
468-468: LGTM!Also applies to: 484-590, 935-935, 954-954
openwisp_controller/connection/models.py (1)
3-8: LGTM!Also applies to: 27-32
tests/openwisp2/sample_connection/models.py (1)
4-4: LGTM!Also applies to: 31-35
openwisp_controller/connection/tests/test_models.py (1)
35-38: LGTM!Also applies to: 467-474, 975-1763
openwisp_controller/connection/tests/test_selenium.py (1)
1-2: LGTM!Also applies to: 58-70
openwisp_controller/connection/tests/test_tasks.py (1)
1-1: LGTM!Also applies to: 20-20, 320-498
openwisp_controller/geo/estimated_location/tests/tests.py (1)
569-569: LGTM!openwisp_controller/geo/tests/test_api.py (1)
697-697: LGTM!openwisp_controller/connection/api/serializers.py (1)
15-15: LGTM!Also applies to: 47-50, 125-170, 173-230
openwisp_controller/connection/api/views.py (1)
1-15: LGTM!Also applies to: 26-28, 38-38, 151-215, 227-229
openwisp_controller/connection/api/urls.py (1)
43-57: LGTM!docs/user/shell-commands.rst (1)
185-196: LGTM!Also applies to: 208-211
docs/user/intro.rst (1)
73-75: LGTM!docs/developer/extending.rst (1)
357-357: LGTM!openwisp_controller/connection/migrations/0011_batchcommand_command_batch_command.py (1)
1-181: LGTM!openwisp_controller/connection/migrations/__init__.py (1)
63-89: LGTM!tests/openwisp2/sample_connection/migrations/0005_batchcommand_command_batch_command.py (1)
1-176: LGTM!tests/openwisp2/settings.py (1)
296-296: LGTM!tests/openwisp2/sample_connection/api/views.py (1)
1-9: LGTM!Also applies to: 54-74
openwisp_controller/connection/tests/test_api.py (1)
6-6: LGTM!Also applies to: 18-30, 869-2318
779abc2 to
1e4e4ee
Compare
|
@coderabbitai full review |
✅ Action performedFull review finished. |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@openwisp_controller/connection/base/models.py`:
- Around line 879-895: Update the explicit-device branch of resolve_devices() to
query self.devices with select_related("config") before returning its iterator,
rather than returning the raw M2M iterator. Preserve the existing
device-resolution filters and empty-result behavior so create_commands()
receives devices with config prefetched on every execution path.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: ad9c246d-8820-437f-a02c-870c80bdef48
📒 Files selected for processing (24)
docs/developer/extending.rstdocs/user/intro.rstdocs/user/rest-api.rstdocs/user/shell-commands.rstopenwisp_controller/connection/api/serializers.pyopenwisp_controller/connection/api/urls.pyopenwisp_controller/connection/api/views.pyopenwisp_controller/connection/base/models.pyopenwisp_controller/connection/migrations/0011_batchcommand_command_batch_command.pyopenwisp_controller/connection/migrations/__init__.pyopenwisp_controller/connection/models.pyopenwisp_controller/connection/tasks.pyopenwisp_controller/connection/tests/pytest.pyopenwisp_controller/connection/tests/test_api.pyopenwisp_controller/connection/tests/test_models.pyopenwisp_controller/connection/tests/test_selenium.pyopenwisp_controller/connection/tests/test_tasks.pyopenwisp_controller/connection/tests/utils.pyopenwisp_controller/geo/estimated_location/tests/tests.pyopenwisp_controller/geo/tests/test_api.pytests/openwisp2/sample_connection/api/views.pytests/openwisp2/sample_connection/migrations/0005_batchcommand_command_batch_command.pytests/openwisp2/sample_connection/models.pytests/openwisp2/settings.py
📜 Review details
⏰ Context from checks skipped due to timeout. (12)
- GitHub Check: Python==3.11 | django~=5.1.0
- GitHub Check: Python==3.11 | django~=5.2.0
- GitHub Check: Python==3.11 | django~=4.2.0
- GitHub Check: Python==3.12 | django~=4.2.0
- GitHub Check: Python==3.12 | django~=5.1.0
- GitHub Check: Python==3.13 | django~=5.2.0
- GitHub Check: Python==3.13 | django~=5.1.0
- GitHub Check: Python==3.12 | django~=5.2.0
- GitHub Check: Python==3.10 | django~=5.1.0
- GitHub Check: Python==3.10 | django~=5.2.0
- GitHub Check: Python==3.10 | django~=4.2.0
- GitHub Check: Kilo Code Review
🧰 Additional context used
📓 Path-based instructions (2)
**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
**/*.py: Mark user-facing strings for translation with Django i18n helpers in Django code
Avoid unnecessary blank lines inside function and method bodies
Be careful with authentication, authorization, queryset filtering, serializers, admin behavior, cache invalidation, signals, Celery tasks, and websocket updates in Django code
Preserve validation around templates, VPN/PKI material, SSH credentials, device commands, uploaded files, URLs, and subnet/IP data
Write comments and docstrings only when they explain why code is shaped a certain way, placing them before the relevant code block instead of scattering them inside it
Files:
openwisp_controller/connection/tests/pytest.pyopenwisp_controller/geo/estimated_location/tests/tests.pyopenwisp_controller/connection/tests/test_selenium.pyopenwisp_controller/connection/api/urls.pytests/openwisp2/settings.pyopenwisp_controller/connection/migrations/__init__.pyopenwisp_controller/geo/tests/test_api.pyopenwisp_controller/connection/tests/utils.pyopenwisp_controller/connection/tasks.pyopenwisp_controller/connection/models.pytests/openwisp2/sample_connection/migrations/0005_batchcommand_command_batch_command.pyopenwisp_controller/connection/tests/test_tasks.pyopenwisp_controller/connection/api/views.pyopenwisp_controller/connection/migrations/0011_batchcommand_command_batch_command.pytests/openwisp2/sample_connection/api/views.pytests/openwisp2/sample_connection/models.pyopenwisp_controller/connection/tests/test_api.pyopenwisp_controller/connection/base/models.pyopenwisp_controller/connection/api/serializers.pyopenwisp_controller/connection/tests/test_models.py
**/*.{py,html}
📄 CodeRabbit inference engine (Custom checks)
For Django pull requests, ensure all user-facing strings are marked as translatable using the Django i18n framework.
Files:
openwisp_controller/connection/tests/pytest.pyopenwisp_controller/geo/estimated_location/tests/tests.pyopenwisp_controller/connection/tests/test_selenium.pyopenwisp_controller/connection/api/urls.pytests/openwisp2/settings.pyopenwisp_controller/connection/migrations/__init__.pyopenwisp_controller/geo/tests/test_api.pyopenwisp_controller/connection/tests/utils.pyopenwisp_controller/connection/tasks.pyopenwisp_controller/connection/models.pytests/openwisp2/sample_connection/migrations/0005_batchcommand_command_batch_command.pyopenwisp_controller/connection/tests/test_tasks.pyopenwisp_controller/connection/api/views.pyopenwisp_controller/connection/migrations/0011_batchcommand_command_batch_command.pytests/openwisp2/sample_connection/api/views.pytests/openwisp2/sample_connection/models.pyopenwisp_controller/connection/tests/test_api.pyopenwisp_controller/connection/base/models.pyopenwisp_controller/connection/api/serializers.pyopenwisp_controller/connection/tests/test_models.py
🧠 Learnings (9)
📚 Learning: 2026-01-15T15:05:49.557Z
Learnt from: DragnEmperor
Repo: openwisp/openwisp-controller PR: 1175
File: openwisp_controller/config/management/commands/clear_last_ip.py:38-42
Timestamp: 2026-01-15T15:05:49.557Z
Learning: In Django projects, when using select_related() to traverse relations (for example, select_related("organization__config_settings")), the traversed relation must not be deferred. If you also use .only() in the same query, include the relation name or FK field (e.g., "organization" or "organization_id") in the .only() list to avoid the error "Field X cannot be both deferred and traversed using select_related at the same time." Apply this guideline to Django code in openwisp_controller/config/management/commands/clear_last_ip.py and similar modules by ensuring any select_related with an accompanying only() includes the related field names to prevent deferred/traversed conflicts.
Applied to files:
openwisp_controller/connection/tests/pytest.pyopenwisp_controller/geo/estimated_location/tests/tests.pyopenwisp_controller/connection/tests/test_selenium.pyopenwisp_controller/connection/api/urls.pyopenwisp_controller/connection/migrations/__init__.pyopenwisp_controller/geo/tests/test_api.pyopenwisp_controller/connection/tests/utils.pyopenwisp_controller/connection/tasks.pyopenwisp_controller/connection/models.pyopenwisp_controller/connection/tests/test_tasks.pyopenwisp_controller/connection/api/views.pyopenwisp_controller/connection/migrations/0011_batchcommand_command_batch_command.pyopenwisp_controller/connection/tests/test_api.pyopenwisp_controller/connection/base/models.pyopenwisp_controller/connection/api/serializers.pyopenwisp_controller/connection/tests/test_models.py
📚 Learning: 2026-02-17T19:13:10.088Z
Learnt from: nemesifier
Repo: openwisp/openwisp-controller PR: 1175
File: openwisp_controller/config/whois/commands.py:0-0
Timestamp: 2026-02-17T19:13:10.088Z
Learning: In reviews for the openwisp/openwisp-controller repository, do not propose changes based on Ruff warnings. The project does not use Ruff as its linter; ignore Ruff-related suggestions and follow the repository’s established linting and configuration rules. This guidance applies to all Python files under the openwisp_controller directory.
Applied to files:
openwisp_controller/connection/tests/pytest.pyopenwisp_controller/geo/estimated_location/tests/tests.pyopenwisp_controller/connection/tests/test_selenium.pyopenwisp_controller/connection/api/urls.pyopenwisp_controller/connection/migrations/__init__.pyopenwisp_controller/geo/tests/test_api.pyopenwisp_controller/connection/tests/utils.pyopenwisp_controller/connection/tasks.pyopenwisp_controller/connection/models.pyopenwisp_controller/connection/tests/test_tasks.pyopenwisp_controller/connection/api/views.pyopenwisp_controller/connection/migrations/0011_batchcommand_command_batch_command.pyopenwisp_controller/connection/tests/test_api.pyopenwisp_controller/connection/base/models.pyopenwisp_controller/connection/api/serializers.pyopenwisp_controller/connection/tests/test_models.py
📚 Learning: 2026-01-15T15:07:17.354Z
Learnt from: DragnEmperor
Repo: openwisp/openwisp-controller PR: 1175
File: openwisp_controller/geo/estimated_location/tests/tests.py:172-175
Timestamp: 2026-01-15T15:07:17.354Z
Learning: In this repository, flake8 enforces E501 (line too long) via setup.cfg (max-line-length = 88) while ruff ignores E501 via ruff.toml. Therefore, use '# noqa: E501' on lines that intentionally exceed 88 characters to satisfy flake8 without affecting ruff checks. This applies to Python files across the project (any .py) and is relevant for tests as well. Use sparingly and only where breaking lines is not feasible without hurting readability or functionality.
Applied to files:
openwisp_controller/connection/tests/pytest.pyopenwisp_controller/geo/estimated_location/tests/tests.pyopenwisp_controller/connection/tests/test_selenium.pyopenwisp_controller/connection/api/urls.pytests/openwisp2/settings.pyopenwisp_controller/connection/migrations/__init__.pyopenwisp_controller/geo/tests/test_api.pyopenwisp_controller/connection/tests/utils.pyopenwisp_controller/connection/tasks.pyopenwisp_controller/connection/models.pytests/openwisp2/sample_connection/migrations/0005_batchcommand_command_batch_command.pyopenwisp_controller/connection/tests/test_tasks.pyopenwisp_controller/connection/api/views.pyopenwisp_controller/connection/migrations/0011_batchcommand_command_batch_command.pytests/openwisp2/sample_connection/api/views.pytests/openwisp2/sample_connection/models.pyopenwisp_controller/connection/tests/test_api.pyopenwisp_controller/connection/base/models.pyopenwisp_controller/connection/api/serializers.pyopenwisp_controller/connection/tests/test_models.py
📚 Learning: 2026-06-07T12:07:08.468Z
Learnt from: stktyagi
Repo: openwisp/openwisp-controller PR: 1378
File: openwisp_controller/config/tests/test_admin.py:2335-2335
Timestamp: 2026-06-07T12:07:08.468Z
Learning: In this project’s Python test suite (files under openwisp_controller/**/tests/), don’t require or request prose/inline comments that document the breakdown of query-count changes (e.g., assertions around template/DB query counts in helpers like _verify_template_queries). Treat query-count assertions as volatile implementation details that change frequently; review should focus on whether the test asserts the expected behavior, not on explaining the specific query-count deltas in comments.
Applied to files:
openwisp_controller/connection/tests/pytest.pyopenwisp_controller/geo/estimated_location/tests/tests.pyopenwisp_controller/connection/tests/test_selenium.pyopenwisp_controller/geo/tests/test_api.pyopenwisp_controller/connection/tests/utils.pyopenwisp_controller/connection/tests/test_tasks.pyopenwisp_controller/connection/tests/test_api.pyopenwisp_controller/connection/tests/test_models.py
📚 Learning: 2026-06-07T12:07:24.608Z
Learnt from: stktyagi
Repo: openwisp/openwisp-controller PR: 1378
File: openwisp_controller/pki/tests/test_api.py:155-155
Timestamp: 2026-06-07T12:07:24.608Z
Learning: When reviewing Python test files in this repository, avoid recommending inline comments that explain or justify `assertNumQueries` (Django query count) expectations. Query counts can change frequently as implementations evolve, and inline explanations add maintenance burden; the expected count should be understandable without added comment blocks.
Applied to files:
openwisp_controller/connection/tests/pytest.pyopenwisp_controller/geo/estimated_location/tests/tests.pyopenwisp_controller/connection/tests/test_selenium.pytests/openwisp2/settings.pyopenwisp_controller/geo/tests/test_api.pyopenwisp_controller/connection/tests/utils.pytests/openwisp2/sample_connection/migrations/0005_batchcommand_command_batch_command.pyopenwisp_controller/connection/tests/test_tasks.pytests/openwisp2/sample_connection/api/views.pytests/openwisp2/sample_connection/models.pyopenwisp_controller/connection/tests/test_api.pyopenwisp_controller/connection/tests/test_models.py
📚 Learning: 2026-06-25T12:20:18.414Z
Learnt from: dee077
Repo: openwisp/openwisp-controller PR: 1395
File: openwisp_controller/connection/base/models.py:571-572
Timestamp: 2026-06-25T12:20:18.414Z
Learning: When writing or reviewing tests that override pagination behavior via OpenWispPagination.paginate_queryset(), patch `view.pagination_page_size` (not `page_size`). The method uses `getattr(view, "pagination_page_size", self.page_size)`, so tests must set the attribute on the view to affect pagination. If the view class does not define `pagination_page_size`, using `unittest.mock.patch(..., create=True)` is intentional and correct because the attribute may not exist until patched.
Applied to files:
openwisp_controller/connection/tests/pytest.pyopenwisp_controller/geo/estimated_location/tests/tests.pyopenwisp_controller/connection/tests/test_selenium.pytests/openwisp2/settings.pyopenwisp_controller/geo/tests/test_api.pyopenwisp_controller/connection/tests/utils.pytests/openwisp2/sample_connection/migrations/0005_batchcommand_command_batch_command.pyopenwisp_controller/connection/tests/test_tasks.pytests/openwisp2/sample_connection/api/views.pytests/openwisp2/sample_connection/models.pyopenwisp_controller/connection/tests/test_api.pyopenwisp_controller/connection/tests/test_models.py
📚 Learning: 2026-06-25T12:20:45.387Z
Learnt from: dee077
Repo: openwisp/openwisp-controller PR: 1395
File: openwisp_controller/connection/tests/test_api.py:916-932
Timestamp: 2026-06-25T12:20:45.387Z
Learning: When reviewing API pagination behavior in openwisp-controller, assume `OpenWispPagination.paginate_queryset()` allows a per-view page-size override via `getattr(view, "pagination_page_size", self.page_size)` (so `view.pagination_page_size`, if present, should affect pagination). In Python tests, it is valid to patch `pagination_page_size` on a view class even if the attribute isn’t declared on the class by default, by using `unittest.mock.patch.object(..., "pagination_page_size", ..., create=True)` so the override is available for the pagination logic during the test.
Applied to files:
openwisp_controller/connection/tests/pytest.pyopenwisp_controller/geo/estimated_location/tests/tests.pyopenwisp_controller/connection/tests/test_selenium.pyopenwisp_controller/geo/tests/test_api.pyopenwisp_controller/connection/tests/utils.pyopenwisp_controller/connection/tests/test_tasks.pyopenwisp_controller/connection/tests/test_api.pyopenwisp_controller/connection/tests/test_models.py
📚 Learning: 2026-03-27T20:50:26.240Z
Learnt from: nemesifier
Repo: openwisp/openwisp-controller PR: 1315
File: openwisp_controller/geo/estimated_location/service.py:70-76
Timestamp: 2026-03-27T20:50:26.240Z
Learning: In openwisp-controller’s WHOIS and estimated-location services (openwisp_controller/config/whois/ and openwisp_controller/geo/estimated_location/), these components only process public IP addresses. When reviewing logs/error/debug messages in this area, treat logging the IP address as acceptable and do not flag it as a privacy/security concern—unless the logged value can originate from non-public/private IPs in that specific code path.
Applied to files:
openwisp_controller/geo/estimated_location/tests/tests.py
📚 Learning: 2026-06-07T12:07:25.164Z
Learnt from: stktyagi
Repo: openwisp/openwisp-controller PR: 1378
File: openwisp_controller/config/tests/test_config.py:864-865
Timestamp: 2026-06-07T12:07:25.164Z
Learning: When reviewing this repo’s Python test suite, treat changes to the *expected* query count in `assertNumQueries(...)` calls as routine test maintenance. If a PR updates the numeric argument (e.g., in `test_config.py`, `test_api.py`, `test_admin.py`, `test_pki.py`) and the test remains consistent with the feature changes, reviewers should not flag the increased number as a performance regression that requires investigation solely because the count went up; instead, focus on whether the update is intentional and the surrounding test/code changes justify the revised expectation.
Applied to files:
openwisp_controller/geo/estimated_location/tests/tests.pyopenwisp_controller/connection/tests/test_selenium.pyopenwisp_controller/geo/tests/test_api.pyopenwisp_controller/connection/tests/test_tasks.pyopenwisp_controller/connection/tests/test_api.pyopenwisp_controller/connection/tests/test_models.py
🪛 ast-grep (0.44.1)
openwisp_controller/connection/tasks.py
[warning] 102-102: Loading a Keras model from an untrusted file can execute arbitrary code via Lambda layers or custom objects. Load only trusted models and avoid deserializing custom objects from untrusted sources.
Context: load_model("connection", "BatchCommand")
Note: [CWE-502] Deserialization of Untrusted Data.
(keras-load-model-python)
tests/openwisp2/sample_connection/migrations/0005_batchcommand_command_batch_command.py
[info] 58-67: use help_text to document model columns
Context: models.CharField(
choices=[
("idle", "idle"),
("in-progress", "in progress"),
("success", "success"),
("failed", "failed"),
],
default="idle",
max_length=12,
)
Note: [CWE-710] Improper Adherence to Coding Standards.
(model-help-text)
[info] 71-78: use help_text to document model columns
Context: models.CharField(
max_length=16,
choices=(
connection_config.commands.COMMAND_CHOICES
if django.VERSION < (5, 0)
else connection_config.commands.get_command_choices
),
)
Note: [CWE-710] Improper Adherence to Coding Standards.
(model-help-text)
openwisp_controller/connection/tests/test_tasks.py
[info] 448-448: use jsonify instead of json.dumps for JSON output
Context: json.dumps(batch.input)
Note: [CWE-116] Improper Encoding or Escaping of Output.
(use-jsonify)
openwisp_controller/connection/migrations/0011_batchcommand_command_batch_command.py
[info] 60-69: use help_text to document model columns
Context: models.CharField(
choices=[
("idle", "idle"),
("in-progress", "in progress"),
("success", "success"),
("failed", "failed"),
],
default="idle",
max_length=12,
)
Note: [CWE-710] Improper Adherence to Coding Standards.
(model-help-text)
[info] 73-80: use help_text to document model columns
Context: models.CharField(
choices=(
openwisp_controller.connection.commands.COMMAND_CHOICES
if django.VERSION < (5, 0)
else openwisp_controller.connection.commands.get_command_choices # noqa: E501
),
max_length=16,
)
Note: [CWE-710] Improper Adherence to Coding Standards.
(model-help-text)
openwisp_controller/connection/tests/test_api.py
[warning] 21-21: Loading a Keras model from an untrusted file can execute arbitrary code via Lambda layers or custom objects. Load only trusted models and avoid deserializing custom objects from untrusted sources.
Context: load_model("connection", "Command")
Note: [CWE-502] Deserialization of Untrusted Data.
(keras-load-model-python)
[warning] 22-22: Loading a Keras model from an untrusted file can execute arbitrary code via Lambda layers or custom objects. Load only trusted models and avoid deserializing custom objects from untrusted sources.
Context: load_model("connection", "DeviceConnection")
Note: [CWE-502] Deserialization of Untrusted Data.
(keras-load-model-python)
[warning] 23-23: Loading a Keras model from an untrusted file can execute arbitrary code via Lambda layers or custom objects. Load only trusted models and avoid deserializing custom objects from untrusted sources.
Context: load_model("connection", "BatchCommand")
Note: [CWE-502] Deserialization of Untrusted Data.
(keras-load-model-python)
[warning] 25-25: Loading a Keras model from an untrusted file can execute arbitrary code via Lambda layers or custom objects. Load only trusted models and avoid deserializing custom objects from untrusted sources.
Context: load_model("openwisp_users", "OrganizationUser")
Note: [CWE-502] Deserialization of Untrusted Data.
(keras-load-model-python)
[warning] 26-26: Loading a Keras model from an untrusted file can execute arbitrary code via Lambda layers or custom objects. Load only trusted models and avoid deserializing custom objects from untrusted sources.
Context: load_model("openwisp_users", "Group")
Note: [CWE-502] Deserialization of Untrusted Data.
(keras-load-model-python)
[warning] 27-27: Loading a Keras model from an untrusted file can execute arbitrary code via Lambda layers or custom objects. Load only trusted models and avoid deserializing custom objects from untrusted sources.
Context: load_model("config", "DeviceGroup")
Note: [CWE-502] Deserialization of Untrusted Data.
(keras-load-model-python)
[warning] 28-28: Loading a Keras model from an untrusted file can execute arbitrary code via Lambda layers or custom objects. Load only trusted models and avoid deserializing custom objects from untrusted sources.
Context: load_model("geo", "Location")
Note: [CWE-502] Deserialization of Untrusted Data.
(keras-load-model-python)
[warning] 29-29: Loading a Keras model from an untrusted file can execute arbitrary code via Lambda layers or custom objects. Load only trusted models and avoid deserializing custom objects from untrusted sources.
Context: load_model("geo", "DeviceLocation")
Note: [CWE-502] Deserialization of Untrusted Data.
(keras-load-model-python)
[info] 1070-1070: use jsonify instead of json.dumps for JSON output
Context: json.dumps(payload)
Note: [CWE-116] Improper Encoding or Escaping of Output.
(use-jsonify)
[info] 1107-1107: use jsonify instead of json.dumps for JSON output
Context: json.dumps(payload)
Note: [CWE-116] Improper Encoding or Escaping of Output.
(use-jsonify)
[info] 1133-1133: use jsonify instead of json.dumps for JSON output
Context: json.dumps(payload)
Note: [CWE-116] Improper Encoding or Escaping of Output.
(use-jsonify)
[info] 1149-1149: use jsonify instead of json.dumps for JSON output
Context: json.dumps(payload)
Note: [CWE-116] Improper Encoding or Escaping of Output.
(use-jsonify)
[info] 1173-1173: use jsonify instead of json.dumps for JSON output
Context: json.dumps(payload)
Note: [CWE-116] Improper Encoding or Escaping of Output.
(use-jsonify)
[info] 1198-1198: use jsonify instead of json.dumps for JSON output
Context: json.dumps(payload)
Note: [CWE-116] Improper Encoding or Escaping of Output.
(use-jsonify)
[info] 1235-1241: use jsonify instead of json.dumps for JSON output
Context: json.dumps(
{
"type": "custom",
"input": {"command": "echo test"},
"label": "test-label",
}
)
Note: [CWE-116] Improper Encoding or Escaping of Output.
(use-jsonify)
[info] 1251-1258: use jsonify instead of json.dumps for JSON output
Context: json.dumps(
{
"type": "custom",
"input": {"command": "echo test"},
"label": "test-label",
"devices": [str(device1.pk), str(device2.pk)],
}
)
Note: [CWE-116] Improper Encoding or Escaping of Output.
(use-jsonify)
[info] 1308-1308: use jsonify instead of json.dumps for JSON output
Context: json.dumps(payload)
Note: [CWE-116] Improper Encoding or Escaping of Output.
(use-jsonify)
[info] 1331-1331: use jsonify instead of json.dumps for JSON output
Context: json.dumps(payload)
Note: [CWE-116] Improper Encoding or Escaping of Output.
(use-jsonify)
[info] 1362-1362: use jsonify instead of json.dumps for JSON output
Context: json.dumps(payload)
Note: [CWE-116] Improper Encoding or Escaping of Output.
(use-jsonify)
[info] 1385-1385: use jsonify instead of json.dumps for JSON output
Context: json.dumps(payload)
Note: [CWE-116] Improper Encoding or Escaping of Output.
(use-jsonify)
[info] 1421-1421: use jsonify instead of json.dumps for JSON output
Context: json.dumps(payload)
Note: [CWE-116] Improper Encoding or Escaping of Output.
(use-jsonify)
[info] 1442-1442: use jsonify instead of json.dumps for JSON output
Context: json.dumps(payload)
Note: [CWE-116] Improper Encoding or Escaping of Output.
(use-jsonify)
[info] 1473-1473: use jsonify instead of json.dumps for JSON output
Context: json.dumps(payload)
Note: [CWE-116] Improper Encoding or Escaping of Output.
(use-jsonify)
[info] 1494-1494: use jsonify instead of json.dumps for JSON output
Context: json.dumps(payload)
Note: [CWE-116] Improper Encoding or Escaping of Output.
(use-jsonify)
[info] 1587-1587: use jsonify instead of json.dumps for JSON output
Context: json.dumps({"type": "custom"})
Note: [CWE-116] Improper Encoding or Escaping of Output.
(use-jsonify)
[info] 1641-1641: use jsonify instead of json.dumps for JSON output
Context: json.dumps(payload)
Note: [CWE-116] Improper Encoding or Escaping of Output.
(use-jsonify)
[info] 1698-1698: use jsonify instead of json.dumps for JSON output
Context: json.dumps(payload)
Note: [CWE-116] Improper Encoding or Escaping of Output.
(use-jsonify)
[info] 1716-1724: use jsonify instead of json.dumps for JSON output
Context: json.dumps(
{
"organization": str(org.pk),
"type": "custom",
"input": {"command": "echo test"},
"label": "test-label",
"devices": [str(device_org2.pk)],
}
)
Note: [CWE-116] Improper Encoding or Escaping of Output.
(use-jsonify)
[info] 1745-1753: use jsonify instead of json.dumps for JSON output
Context: json.dumps(
{
"organization": str(org.pk),
"type": "custom",
"input": {"command": "echo test"},
"label": "test-label",
"group": str(group_org2.pk),
}
)
Note: [CWE-116] Improper Encoding or Escaping of Output.
(use-jsonify)
[info] 1777-1785: use jsonify instead of json.dumps for JSON output
Context: json.dumps(
{
"organization": str(org.pk),
"type": "custom",
"input": {"command": "echo test"},
"label": "test-label",
"location": str(location_org2.pk),
}
)
Note: [CWE-116] Improper Encoding or Escaping of Output.
(use-jsonify)
[info] 1922-1930: use jsonify instead of json.dumps for JSON output
Context: json.dumps(
{
"organization": str(org.pk),
"type": "custom",
"input": {"command": "echo test"},
"label": "test-label",
"devices": [str(device1.pk)],
}
)
Note: [CWE-116] Improper Encoding or Escaping of Output.
(use-jsonify)
[info] 1942-1950: use jsonify instead of json.dumps for JSON output
Context: json.dumps(
{
"organization": str(org.pk),
"type": "custom",
"input": {"command": "echo test"},
"label": "test-label",
"group": str(group.pk),
}
)
Note: [CWE-116] Improper Encoding or Escaping of Output.
(use-jsonify)
[info] 1962-1970: use jsonify instead of json.dumps for JSON output
Context: json.dumps(
{
"organization": str(org.pk),
"type": "custom",
"input": {"command": "echo test"},
"label": "test-label",
"location": str(location.pk),
}
)
Note: [CWE-116] Improper Encoding or Escaping of Output.
(use-jsonify)
[info] 1983-1992: use jsonify instead of json.dumps for JSON output
Context: json.dumps(
{
"organization": str(org.pk),
"type": "custom",
"input": {"command": "echo test"},
"label": "test-label",
"group": str(group.pk),
"location": str(location.pk),
}
)
Note: [CWE-116] Improper Encoding or Escaping of Output.
(use-jsonify)
[info] 2004-2011: use jsonify instead of json.dumps for JSON output
Context: json.dumps(
{
"organization": str(org.pk),
"type": "custom",
"input": {"command": "echo test"},
"label": "test-label",
}
)
Note: [CWE-116] Improper Encoding or Escaping of Output.
(use-jsonify)
[info] 2026-2034: use jsonify instead of json.dumps for JSON output
Context: json.dumps(
{
"organization": str(org.pk),
"type": "custom",
"input": {"command": "echo test"},
"label": "test-label",
"devices": [],
}
)
Note: [CWE-116] Improper Encoding or Escaping of Output.
(use-jsonify)
[info] 2046-2053: use jsonify instead of json.dumps for JSON output
Context: json.dumps(
{
"organization": str(org.pk),
"type": "custom",
"input": {"command": "echo test"},
"label": "test-label",
}
)
Note: [CWE-116] Improper Encoding or Escaping of Output.
(use-jsonify)
[info] 2067-2075: use jsonify instead of json.dumps for JSON output
Context: json.dumps(
{
"organization": str(org.pk),
"type": "custom",
"input": {"command": "echo test"},
"label": "",
"devices": [str(device1.pk)],
}
)
Note: [CWE-116] Improper Encoding or Escaping of Output.
(use-jsonify)
[info] 2083-2091: use jsonify instead of json.dumps for JSON output
Context: json.dumps(
{
"organization": str(org.pk),
"type": "custom",
"input": {"command": "echo test"},
"label": "a" * 65,
"devices": [str(device1.pk)],
}
)
Note: [CWE-116] Improper Encoding or Escaping of Output.
(use-jsonify)
[info] 2099-2107: use jsonify instead of json.dumps for JSON output
Context: json.dumps(
{
"organization": str(org.pk),
"type": "custom",
"input": {},
"label": "test-label",
"devices": [str(device1.pk)],
}
)
Note: [CWE-116] Improper Encoding or Escaping of Output.
(use-jsonify)
[info] 2115-2123: use jsonify instead of json.dumps for JSON output
Context: json.dumps(
{
"organization": str(org.pk),
"type": "nonexistent",
"input": {"command": "echo test"},
"label": "test-label",
"devices": [str(device1.pk)],
}
)
Note: [CWE-116] Improper Encoding or Escaping of Output.
(use-jsonify)
[info] 2141-2152: use jsonify instead of json.dumps for JSON output
Context: json.dumps(
{
"organization": str(org.pk),
"type": "change_password",
"input": {
"password": password,
"confirm_password": password,
},
"label": "change password",
"devices": [str(device.pk)],
}
)
Note: [CWE-116] Improper Encoding or Escaping of Output.
(use-jsonify)
[info] 2157-2157: use jsonify instead of json.dumps for JSON output
Context: json.dumps(batch.input)
Note: [CWE-116] Improper Encoding or Escaping of Output.
(use-jsonify)
[info] 2160-2160: use jsonify instead of json.dumps for JSON output
Context: json.dumps(list_response.data)
Note: [CWE-116] Improper Encoding or Escaping of Output.
(use-jsonify)
[info] 2165-2165: use jsonify instead of json.dumps for JSON output
Context: json.dumps(detail_response.data)
Note: [CWE-116] Improper Encoding or Escaping of Output.
(use-jsonify)
[info] 2198-2198: use jsonify instead of json.dumps for JSON output
Context: json.dumps(payload)
Note: [CWE-116] Improper Encoding or Escaping of Output.
(use-jsonify)
[info] 2250-2258: use jsonify instead of json.dumps for JSON output
Context: json.dumps(
{
"organization": str(org.pk),
"type": "custom",
"input": {"command": "echo test"},
"label": "test-label",
"devices": [str(device.pk)],
}
)
Note: [CWE-116] Improper Encoding or Escaping of Output.
(use-jsonify)
[info] 2292-2300: use jsonify instead of json.dumps for JSON output
Context: json.dumps(
{
"organization": str(org.pk),
"type": "custom",
"input": {"command": "echo test"},
"label": "test-label",
"devices": [str(device.pk)],
}
)
Note: [CWE-116] Improper Encoding or Escaping of Output.
(use-jsonify)
openwisp_controller/connection/base/models.py
[warning] 859-859: Loading a Keras model from an untrusted file can execute arbitrary code via Lambda layers or custom objects. Load only trusted models and avoid deserializing custom objects from untrusted sources.
Context: load_model("connection", "Command")
Note: [CWE-502] Deserialization of Untrusted Data.
(keras-load-model-python)
[warning] 886-886: Loading a Keras model from an untrusted file can execute arbitrary code via Lambda layers or custom objects. Load only trusted models and avoid deserializing custom objects from untrusted sources.
Context: load_model("config", "Device")
Note: [CWE-502] Deserialization of Untrusted Data.
(keras-load-model-python)
[warning] 961-961: Loading a Keras model from an untrusted file can execute arbitrary code via Lambda layers or custom objects. Load only trusted models and avoid deserializing custom objects from untrusted sources.
Context: load_model("connection", "Command")
Note: [CWE-502] Deserialization of Untrusted Data.
(keras-load-model-python)
[warning] 962-962: Loading a Keras model from an untrusted file can execute arbitrary code via Lambda layers or custom objects. Load only trusted models and avoid deserializing custom objects from untrusted sources.
Context: load_model("config", "Device")
Note: [CWE-502] Deserialization of Untrusted Data.
(keras-load-model-python)
[info] 751-753: use help_text to document model columns
Context: models.CharField(
max_length=12, choices=STATUS_CHOICES, default=STATUS_CHOICES[0][0]
)
Note: [CWE-710] Improper Adherence to Coding Standards.
(model-help-text)
[info] 754-757: use help_text to document model columns
Context: models.CharField(
max_length=16,
choices=(COMMAND_CHOICES if django.VERSION < (5, 0) else get_command_choices),
)
Note: [CWE-710] Improper Adherence to Coding Standards.
(model-help-text)
openwisp_controller/connection/tests/test_models.py
[warning] 35-35: Loading a Keras model from an untrusted file can execute arbitrary code via Lambda layers or custom objects. Load only trusted models and avoid deserializing custom objects from untrusted sources.
Context: load_model("config", "DeviceGroup")
Note: [CWE-502] Deserialization of Untrusted Data.
(keras-load-model-python)
[warning] 36-36: Loading a Keras model from an untrusted file can execute arbitrary code via Lambda layers or custom objects. Load only trusted models and avoid deserializing custom objects from untrusted sources.
Context: load_model("geo", "Location")
Note: [CWE-502] Deserialization of Untrusted Data.
(keras-load-model-python)
[warning] 37-37: Loading a Keras model from an untrusted file can execute arbitrary code via Lambda layers or custom objects. Load only trusted models and avoid deserializing custom objects from untrusted sources.
Context: load_model("geo", "DeviceLocation")
Note: [CWE-502] Deserialization of Untrusted Data.
(keras-load-model-python)
🪛 Betterleaks (1.6.1)
docs/user/rest-api.rst
[high] 466-468: Discovered a potential authorization token provided in a curl command header, which could compromise the curl accessed resource.
(curl-auth-header)
[high] 560-562: Discovered a potential authorization token provided in a curl command header, which could compromise the curl accessed resource.
(curl-auth-header)
[high] 935-937: Discovered a potential authorization token provided in a curl command header, which could compromise the curl accessed resource.
(curl-auth-header)
[high] 954-956: Discovered a potential authorization token provided in a curl command header, which could compromise the curl accessed resource.
(curl-auth-header)
🔇 Additional comments (29)
openwisp_controller/connection/tests/pytest.py (1)
68-68: LGTM!openwisp_controller/connection/tests/test_api.py (1)
6-6: LGTM!Also applies to: 18-30, 869-2318
openwisp_controller/connection/tests/test_models.py (1)
35-38: LGTM!Also applies to: 467-474, 975-1763
openwisp_controller/connection/tests/test_selenium.py (1)
1-2: LGTM!Also applies to: 58-60, 70-70
openwisp_controller/connection/tests/test_tasks.py (1)
1-1: LGTM!Also applies to: 20-20, 320-498
openwisp_controller/connection/tests/utils.py (1)
14-14: LGTM!Also applies to: 122-139
openwisp_controller/geo/estimated_location/tests/tests.py (1)
569-569: LGTM!openwisp_controller/geo/tests/test_api.py (1)
697-697: LGTM!openwisp_controller/connection/base/models.py (6)
999-1057: Status aggregation logic verified — correctly resolved from earlier review rounds.Confirmed the
elifchain is exhaustive givenCommand.STATUS_CHOICESonly hasin-progress/success/failed: whenevertotal_operations>0,in_progress==0, andfailed==0,completedmust equaltotal_operationsandsuccessfulmust equalcompleted(no fourth status exists), so the finalelifalways fires andnew_statusis always assigned. Theskipped_devices-aware branching for both the "no commands created" and "all created commands succeeded" cases correctly resolves the previously-flagged "stuck in-progress with skipped devices" issue.One minor fragility for the future: if a new
Commandstatus value were ever introduced, this chain would fall through withnew_statusundefined (UnboundLocalError). Not an issue today, so no action needed unless the status enum changes.
18-18: LGTM!Also applies to: 34-38, 475-481, 572-573
738-807: LGTM!Also applies to: 821-848
897-943: LGTM!
945-947: LGTM!Also applies to: 990-998
857-877: 🩺 Stability & AvailabilityNo issue —
SchemaErroralready aliasesjsonschema.exceptions.ValidationError, so invalidinputis caught here.> Likely an incorrect or invalid review comment.openwisp_controller/connection/models.py (1)
3-8: LGTM!Also applies to: 29-32
openwisp_controller/connection/tasks.py (1)
101-120: LGTM!openwisp_controller/connection/migrations/0011_batchcommand_command_batch_command.py (1)
1-182: LGTM!openwisp_controller/connection/migrations/__init__.py (1)
65-89: LGTM!tests/openwisp2/sample_connection/migrations/0005_batchcommand_command_batch_command.py (1)
1-177: LGTM!tests/openwisp2/sample_connection/models.py (1)
4-4: LGTM!Also applies to: 33-35
tests/openwisp2/settings.py (1)
296-296: LGTM!openwisp_controller/connection/api/serializers.py (1)
15-15: LGTM!Also applies to: 47-50, 123-170, 173-221, 222-230
openwisp_controller/connection/api/views.py (1)
1-15: LGTM!Also applies to: 26-28, 38-38, 151-215, 227-229
openwisp_controller/connection/api/urls.py (1)
43-57: LGTM!docs/user/rest-api.rst (1)
468-468: LGTM!Also applies to: 484-592, 937-937, 956-956
docs/user/shell-commands.rst (1)
185-211: LGTM!docs/user/intro.rst (1)
73-75: LGTM!tests/openwisp2/sample_connection/api/views.py (1)
1-9: LGTM!Also applies to: 54-74
docs/developer/extending.rst (1)
357-357: LGTM!
1e4e4ee to
4994e34
Compare
|
@coderabbitai full review |
✅ Action performedFull review finished. |
| # websockets is added. | ||
| # Wait for the redirect triggered by command submission to complete. | ||
| # Navigating away immediately can race with the redirect | ||
| sleep(0.3) |
There was a problem hiding this comment.
I would avoid this fixed sleep. It still depends on timing, may fail on a slow CI worker, and slows down every successful run. Please replace it with an explicit wait for the redirect or for the expected page state after command submission.
There was a problem hiding this comment.
I think we can keep this for now, it keeps ci consistent, and once we add websockets, we need to change this test anyway
Let me know if we still need to remove it
|
@coderabbitai full review |
✅ Action performedFull review finished. |
✅ Action performedFull review finished. |
There was a problem hiding this comment.
♻️ Duplicate comments (2)
tests/openwisp2/sample_connection/api/views.py (1)
1-9: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsolidate imports from the same module.
This was previously raised and marked as addressed, but the imports remain separated. To improve code readability and follow standard style, consider combining these imports into a single block.
♻️ Proposed refactor
-from openwisp_controller.connection.api.views import ( - BatchCommandDetailView as BaseBatchCommandDetailView, -) -from openwisp_controller.connection.api.views import ( - BatchCommandExecuteView as BaseBatchCommandExecuteView, -) -from openwisp_controller.connection.api.views import ( - BatchCommandListView as BaseBatchCommandListView, -) +from openwisp_controller.connection.api.views import ( + BatchCommandDetailView as BaseBatchCommandDetailView, + BatchCommandExecuteView as BaseBatchCommandExecuteView, + BatchCommandListView as BaseBatchCommandListView, +)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/openwisp2/sample_connection/api/views.py` around lines 1 - 9, Consolidate the three imports from openwisp_controller.connection.api.views into one parenthesized import block, preserving the existing aliases BaseBatchCommandDetailView, BaseBatchCommandExecuteView, and BaseBatchCommandListView.docs/user/shell-commands.rst (1)
197-203: 📐 Maintainability & Code Quality | 🟡 Minor | 💤 Low valueDocument that explicit
devicesoverrides the other target filters.This was previously raised and marked as addressed, but the text still states "using any combination of the following". The code uses
devicesdirectly when present, meaninggroupandlocationdo not further filter the list. Please clarify thatdevicesoverrides the other options.📝 Proposed clarification
**Targeting options:** -You can target devices using any combination of the following: +Target devices either with an explicit ``devices`` list or with the +``group`` and ``location`` filters. The latter two may be combined. +When ``devices`` is supplied, it determines the target set. - ``devices``: Explicit list of device UUIDs.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docs/user/shell-commands.rst` around lines 197 - 203, Update the “Targeting options” documentation to state that providing explicit devices takes precedence over group and location filters; clarify that group and location are only used when devices is not provided, replacing the misleading “any combination” wording.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Duplicate comments:
In `@docs/user/shell-commands.rst`:
- Around line 197-203: Update the “Targeting options” documentation to state
that providing explicit devices takes precedence over group and location
filters; clarify that group and location are only used when devices is not
provided, replacing the misleading “any combination” wording.
In `@tests/openwisp2/sample_connection/api/views.py`:
- Around line 1-9: Consolidate the three imports from
openwisp_controller.connection.api.views into one parenthesized import block,
preserving the existing aliases BaseBatchCommandDetailView,
BaseBatchCommandExecuteView, and BaseBatchCommandListView.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 84d46ba1-5003-4525-8b99-f9ad3cf555c8
📒 Files selected for processing (24)
docs/developer/extending.rstdocs/user/intro.rstdocs/user/rest-api.rstdocs/user/shell-commands.rstopenwisp_controller/connection/api/serializers.pyopenwisp_controller/connection/api/urls.pyopenwisp_controller/connection/api/views.pyopenwisp_controller/connection/base/models.pyopenwisp_controller/connection/migrations/0011_batchcommand_command_batch_command.pyopenwisp_controller/connection/migrations/__init__.pyopenwisp_controller/connection/models.pyopenwisp_controller/connection/tasks.pyopenwisp_controller/connection/tests/pytest.pyopenwisp_controller/connection/tests/test_api.pyopenwisp_controller/connection/tests/test_models.pyopenwisp_controller/connection/tests/test_selenium.pyopenwisp_controller/connection/tests/test_tasks.pyopenwisp_controller/connection/tests/utils.pyopenwisp_controller/geo/estimated_location/tests/tests.pyopenwisp_controller/geo/tests/test_api.pytests/openwisp2/sample_connection/api/views.pytests/openwisp2/sample_connection/migrations/0005_batchcommand_command_batch_command.pytests/openwisp2/sample_connection/models.pytests/openwisp2/settings.py
📜 Review details
⏰ Context from checks skipped due to timeout. (12)
- GitHub Check: Python==3.12 | django~=5.2.0
- GitHub Check: Python==3.11 | django~=5.1.0
- GitHub Check: Python==3.12 | django~=5.1.0
- GitHub Check: Python==3.10 | django~=4.2.0
- GitHub Check: Python==3.11 | django~=4.2.0
- GitHub Check: Python==3.13 | django~=5.2.0
- GitHub Check: Python==3.10 | django~=5.1.0
- GitHub Check: Python==3.13 | django~=5.1.0
- GitHub Check: Python==3.10 | django~=5.2.0
- GitHub Check: Python==3.11 | django~=5.2.0
- GitHub Check: Python==3.12 | django~=4.2.0
- GitHub Check: Kilo Code Review
🧰 Additional context used
📓 Path-based instructions (2)
**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
**/*.py: Mark user-facing strings for translation with Django i18n helpers in Django code
Avoid unnecessary blank lines inside function and method bodies
Be careful with authentication, authorization, queryset filtering, serializers, admin behavior, cache invalidation, signals, Celery tasks, and websocket updates in Django code
Preserve validation around templates, VPN/PKI material, SSH credentials, device commands, uploaded files, URLs, and subnet/IP data
Write comments and docstrings only when they explain why code is shaped a certain way, placing them before the relevant code block instead of scattering them inside it
Files:
openwisp_controller/connection/tests/pytest.pyopenwisp_controller/connection/tests/utils.pyopenwisp_controller/geo/tests/test_api.pyopenwisp_controller/geo/estimated_location/tests/tests.pyopenwisp_controller/connection/models.pyopenwisp_controller/connection/tasks.pyopenwisp_controller/connection/api/urls.pytests/openwisp2/sample_connection/models.pyopenwisp_controller/connection/migrations/__init__.pytests/openwisp2/sample_connection/api/views.pytests/openwisp2/sample_connection/migrations/0005_batchcommand_command_batch_command.pytests/openwisp2/settings.pyopenwisp_controller/connection/migrations/0011_batchcommand_command_batch_command.pyopenwisp_controller/connection/tests/test_api.pyopenwisp_controller/connection/base/models.pyopenwisp_controller/connection/tests/test_selenium.pyopenwisp_controller/connection/tests/test_tasks.pyopenwisp_controller/connection/api/views.pyopenwisp_controller/connection/api/serializers.pyopenwisp_controller/connection/tests/test_models.py
**/*.{py,html}
📄 CodeRabbit inference engine (Custom checks)
For Django pull requests, ensure all user-facing strings are marked as translatable using the Django i18n framework.
Files:
openwisp_controller/connection/tests/pytest.pyopenwisp_controller/connection/tests/utils.pyopenwisp_controller/geo/tests/test_api.pyopenwisp_controller/geo/estimated_location/tests/tests.pyopenwisp_controller/connection/models.pyopenwisp_controller/connection/tasks.pyopenwisp_controller/connection/api/urls.pytests/openwisp2/sample_connection/models.pyopenwisp_controller/connection/migrations/__init__.pytests/openwisp2/sample_connection/api/views.pytests/openwisp2/sample_connection/migrations/0005_batchcommand_command_batch_command.pytests/openwisp2/settings.pyopenwisp_controller/connection/migrations/0011_batchcommand_command_batch_command.pyopenwisp_controller/connection/tests/test_api.pyopenwisp_controller/connection/base/models.pyopenwisp_controller/connection/tests/test_selenium.pyopenwisp_controller/connection/tests/test_tasks.pyopenwisp_controller/connection/api/views.pyopenwisp_controller/connection/api/serializers.pyopenwisp_controller/connection/tests/test_models.py
🧠 Learnings (9)
📚 Learning: 2026-01-15T15:05:49.557Z
Learnt from: DragnEmperor
Repo: openwisp/openwisp-controller PR: 1175
File: openwisp_controller/config/management/commands/clear_last_ip.py:38-42
Timestamp: 2026-01-15T15:05:49.557Z
Learning: In Django projects, when using select_related() to traverse relations (for example, select_related("organization__config_settings")), the traversed relation must not be deferred. If you also use .only() in the same query, include the relation name or FK field (e.g., "organization" or "organization_id") in the .only() list to avoid the error "Field X cannot be both deferred and traversed using select_related at the same time." Apply this guideline to Django code in openwisp_controller/config/management/commands/clear_last_ip.py and similar modules by ensuring any select_related with an accompanying only() includes the related field names to prevent deferred/traversed conflicts.
Applied to files:
openwisp_controller/connection/tests/pytest.pyopenwisp_controller/connection/tests/utils.pyopenwisp_controller/geo/tests/test_api.pyopenwisp_controller/geo/estimated_location/tests/tests.pyopenwisp_controller/connection/models.pyopenwisp_controller/connection/tasks.pyopenwisp_controller/connection/api/urls.pyopenwisp_controller/connection/migrations/__init__.pyopenwisp_controller/connection/migrations/0011_batchcommand_command_batch_command.pyopenwisp_controller/connection/tests/test_api.pyopenwisp_controller/connection/base/models.pyopenwisp_controller/connection/tests/test_selenium.pyopenwisp_controller/connection/tests/test_tasks.pyopenwisp_controller/connection/api/views.pyopenwisp_controller/connection/api/serializers.pyopenwisp_controller/connection/tests/test_models.py
📚 Learning: 2026-02-17T19:13:10.088Z
Learnt from: nemesifier
Repo: openwisp/openwisp-controller PR: 1175
File: openwisp_controller/config/whois/commands.py:0-0
Timestamp: 2026-02-17T19:13:10.088Z
Learning: In reviews for the openwisp/openwisp-controller repository, do not propose changes based on Ruff warnings. The project does not use Ruff as its linter; ignore Ruff-related suggestions and follow the repository’s established linting and configuration rules. This guidance applies to all Python files under the openwisp_controller directory.
Applied to files:
openwisp_controller/connection/tests/pytest.pyopenwisp_controller/connection/tests/utils.pyopenwisp_controller/geo/tests/test_api.pyopenwisp_controller/geo/estimated_location/tests/tests.pyopenwisp_controller/connection/models.pyopenwisp_controller/connection/tasks.pyopenwisp_controller/connection/api/urls.pyopenwisp_controller/connection/migrations/__init__.pyopenwisp_controller/connection/migrations/0011_batchcommand_command_batch_command.pyopenwisp_controller/connection/tests/test_api.pyopenwisp_controller/connection/base/models.pyopenwisp_controller/connection/tests/test_selenium.pyopenwisp_controller/connection/tests/test_tasks.pyopenwisp_controller/connection/api/views.pyopenwisp_controller/connection/api/serializers.pyopenwisp_controller/connection/tests/test_models.py
📚 Learning: 2026-01-15T15:07:17.354Z
Learnt from: DragnEmperor
Repo: openwisp/openwisp-controller PR: 1175
File: openwisp_controller/geo/estimated_location/tests/tests.py:172-175
Timestamp: 2026-01-15T15:07:17.354Z
Learning: In this repository, flake8 enforces E501 (line too long) via setup.cfg (max-line-length = 88) while ruff ignores E501 via ruff.toml. Therefore, use '# noqa: E501' on lines that intentionally exceed 88 characters to satisfy flake8 without affecting ruff checks. This applies to Python files across the project (any .py) and is relevant for tests as well. Use sparingly and only where breaking lines is not feasible without hurting readability or functionality.
Applied to files:
openwisp_controller/connection/tests/pytest.pyopenwisp_controller/connection/tests/utils.pyopenwisp_controller/geo/tests/test_api.pyopenwisp_controller/geo/estimated_location/tests/tests.pyopenwisp_controller/connection/models.pyopenwisp_controller/connection/tasks.pyopenwisp_controller/connection/api/urls.pytests/openwisp2/sample_connection/models.pyopenwisp_controller/connection/migrations/__init__.pytests/openwisp2/sample_connection/api/views.pytests/openwisp2/sample_connection/migrations/0005_batchcommand_command_batch_command.pytests/openwisp2/settings.pyopenwisp_controller/connection/migrations/0011_batchcommand_command_batch_command.pyopenwisp_controller/connection/tests/test_api.pyopenwisp_controller/connection/base/models.pyopenwisp_controller/connection/tests/test_selenium.pyopenwisp_controller/connection/tests/test_tasks.pyopenwisp_controller/connection/api/views.pyopenwisp_controller/connection/api/serializers.pyopenwisp_controller/connection/tests/test_models.py
📚 Learning: 2026-06-07T12:07:08.468Z
Learnt from: stktyagi
Repo: openwisp/openwisp-controller PR: 1378
File: openwisp_controller/config/tests/test_admin.py:2335-2335
Timestamp: 2026-06-07T12:07:08.468Z
Learning: In this project’s Python test suite (files under openwisp_controller/**/tests/), don’t require or request prose/inline comments that document the breakdown of query-count changes (e.g., assertions around template/DB query counts in helpers like _verify_template_queries). Treat query-count assertions as volatile implementation details that change frequently; review should focus on whether the test asserts the expected behavior, not on explaining the specific query-count deltas in comments.
Applied to files:
openwisp_controller/connection/tests/pytest.pyopenwisp_controller/connection/tests/utils.pyopenwisp_controller/geo/tests/test_api.pyopenwisp_controller/geo/estimated_location/tests/tests.pyopenwisp_controller/connection/tests/test_api.pyopenwisp_controller/connection/tests/test_selenium.pyopenwisp_controller/connection/tests/test_tasks.pyopenwisp_controller/connection/tests/test_models.py
📚 Learning: 2026-06-07T12:07:24.608Z
Learnt from: stktyagi
Repo: openwisp/openwisp-controller PR: 1378
File: openwisp_controller/pki/tests/test_api.py:155-155
Timestamp: 2026-06-07T12:07:24.608Z
Learning: When reviewing Python test files in this repository, avoid recommending inline comments that explain or justify `assertNumQueries` (Django query count) expectations. Query counts can change frequently as implementations evolve, and inline explanations add maintenance burden; the expected count should be understandable without added comment blocks.
Applied to files:
openwisp_controller/connection/tests/pytest.pyopenwisp_controller/connection/tests/utils.pyopenwisp_controller/geo/tests/test_api.pyopenwisp_controller/geo/estimated_location/tests/tests.pytests/openwisp2/sample_connection/models.pytests/openwisp2/sample_connection/api/views.pytests/openwisp2/sample_connection/migrations/0005_batchcommand_command_batch_command.pytests/openwisp2/settings.pyopenwisp_controller/connection/tests/test_api.pyopenwisp_controller/connection/tests/test_selenium.pyopenwisp_controller/connection/tests/test_tasks.pyopenwisp_controller/connection/tests/test_models.py
📚 Learning: 2026-06-25T12:20:18.414Z
Learnt from: dee077
Repo: openwisp/openwisp-controller PR: 1395
File: openwisp_controller/connection/base/models.py:571-572
Timestamp: 2026-06-25T12:20:18.414Z
Learning: When writing or reviewing tests that override pagination behavior via OpenWispPagination.paginate_queryset(), patch `view.pagination_page_size` (not `page_size`). The method uses `getattr(view, "pagination_page_size", self.page_size)`, so tests must set the attribute on the view to affect pagination. If the view class does not define `pagination_page_size`, using `unittest.mock.patch(..., create=True)` is intentional and correct because the attribute may not exist until patched.
Applied to files:
openwisp_controller/connection/tests/pytest.pyopenwisp_controller/connection/tests/utils.pyopenwisp_controller/geo/tests/test_api.pyopenwisp_controller/geo/estimated_location/tests/tests.pytests/openwisp2/sample_connection/models.pytests/openwisp2/sample_connection/api/views.pytests/openwisp2/sample_connection/migrations/0005_batchcommand_command_batch_command.pytests/openwisp2/settings.pyopenwisp_controller/connection/tests/test_api.pyopenwisp_controller/connection/tests/test_selenium.pyopenwisp_controller/connection/tests/test_tasks.pyopenwisp_controller/connection/tests/test_models.py
📚 Learning: 2026-06-25T12:20:45.387Z
Learnt from: dee077
Repo: openwisp/openwisp-controller PR: 1395
File: openwisp_controller/connection/tests/test_api.py:916-932
Timestamp: 2026-06-25T12:20:45.387Z
Learning: When reviewing API pagination behavior in openwisp-controller, assume `OpenWispPagination.paginate_queryset()` allows a per-view page-size override via `getattr(view, "pagination_page_size", self.page_size)` (so `view.pagination_page_size`, if present, should affect pagination). In Python tests, it is valid to patch `pagination_page_size` on a view class even if the attribute isn’t declared on the class by default, by using `unittest.mock.patch.object(..., "pagination_page_size", ..., create=True)` so the override is available for the pagination logic during the test.
Applied to files:
openwisp_controller/connection/tests/pytest.pyopenwisp_controller/connection/tests/utils.pyopenwisp_controller/geo/tests/test_api.pyopenwisp_controller/geo/estimated_location/tests/tests.pyopenwisp_controller/connection/tests/test_api.pyopenwisp_controller/connection/tests/test_selenium.pyopenwisp_controller/connection/tests/test_tasks.pyopenwisp_controller/connection/tests/test_models.py
📚 Learning: 2026-06-07T12:07:25.164Z
Learnt from: stktyagi
Repo: openwisp/openwisp-controller PR: 1378
File: openwisp_controller/config/tests/test_config.py:864-865
Timestamp: 2026-06-07T12:07:25.164Z
Learning: When reviewing this repo’s Python test suite, treat changes to the *expected* query count in `assertNumQueries(...)` calls as routine test maintenance. If a PR updates the numeric argument (e.g., in `test_config.py`, `test_api.py`, `test_admin.py`, `test_pki.py`) and the test remains consistent with the feature changes, reviewers should not flag the increased number as a performance regression that requires investigation solely because the count went up; instead, focus on whether the update is intentional and the surrounding test/code changes justify the revised expectation.
Applied to files:
openwisp_controller/geo/tests/test_api.pyopenwisp_controller/geo/estimated_location/tests/tests.pyopenwisp_controller/connection/tests/test_api.pyopenwisp_controller/connection/tests/test_selenium.pyopenwisp_controller/connection/tests/test_tasks.pyopenwisp_controller/connection/tests/test_models.py
📚 Learning: 2026-03-27T20:50:26.240Z
Learnt from: nemesifier
Repo: openwisp/openwisp-controller PR: 1315
File: openwisp_controller/geo/estimated_location/service.py:70-76
Timestamp: 2026-03-27T20:50:26.240Z
Learning: In openwisp-controller’s WHOIS and estimated-location services (openwisp_controller/config/whois/ and openwisp_controller/geo/estimated_location/), these components only process public IP addresses. When reviewing logs/error/debug messages in this area, treat logging the IP address as acceptable and do not flag it as a privacy/security concern—unless the logged value can originate from non-public/private IPs in that specific code path.
Applied to files:
openwisp_controller/geo/estimated_location/tests/tests.py
🔇 Additional comments (26)
openwisp_controller/connection/tests/test_selenium.py (2)
1-2: Replace the fixed delay with an explicit redirect or page-state wait.The 0.3-second delay remains timing-dependent and has already been flagged in the existing review.
Also applies to: 58-60
70-70: LGTM!openwisp_controller/connection/tests/test_tasks.py (3)
387-407: Cover password cleanup in the timeout branch.The earlier review requested
change_passwordcoverage for both failure branches. This timeout case still usescustominput, so timeout-specific cleanup regressions remain undetected.
1-1: LGTM!Also applies to: 20-20, 320-386, 408-473
475-498: 🩺 Stability & AvailabilityVerify concurrent idempotency guard. Serial test coverage still leaves a race window if two workers hit this at the same time; add concurrent coverage or a database-backed lock/uniqueness guarantee for one command per batch/device.
openwisp_controller/connection/tests/pytest.py (1)
68-68: LGTM!openwisp_controller/connection/tests/test_api.py (1)
6-6: LGTM!Also applies to: 18-30, 867-2371
openwisp_controller/connection/tests/test_models.py (1)
35-38: LGTM!Also applies to: 467-474, 975-1633, 1653-1762
openwisp_controller/connection/tests/utils.py (1)
14-14: LGTM!Also applies to: 122-139
openwisp_controller/geo/estimated_location/tests/tests.py (1)
569-569: LGTM!openwisp_controller/geo/tests/test_api.py (1)
697-697: LGTM!openwisp_controller/connection/base/models.py (1)
18-18: LGTM!Also applies to: 34-38, 475-481, 572-573, 736-1062
openwisp_controller/connection/tasks.py (1)
101-122: LGTM!openwisp_controller/connection/migrations/0011_batchcommand_command_batch_command.py (1)
1-182: LGTM!openwisp_controller/connection/migrations/__init__.py (1)
65-89: LGTM!openwisp_controller/connection/models.py (1)
3-8: LGTM!Also applies to: 27-32
tests/openwisp2/sample_connection/migrations/0005_batchcommand_command_batch_command.py (1)
1-177: LGTM!tests/openwisp2/sample_connection/models.py (1)
4-4: LGTM!Also applies to: 31-35
tests/openwisp2/settings.py (1)
296-296: LGTM!openwisp_controller/connection/api/serializers.py (1)
15-15: LGTM!Also applies to: 47-50, 123-170, 173-221, 222-230
openwisp_controller/connection/api/views.py (1)
1-15: LGTM!Also applies to: 26-28, 38-38, 151-215, 227-229
openwisp_controller/connection/api/urls.py (1)
43-57: LGTM!docs/user/rest-api.rst (1)
468-468: LGTM!Also applies to: 484-592, 937-937, 956-956
docs/user/intro.rst (1)
73-75: LGTM!tests/openwisp2/sample_connection/api/views.py (1)
54-74: LGTM!docs/developer/extending.rst (1)
357-357: LGTM!
✅ Action performedFull review finished. |
There was a problem hiding this comment.
♻️ Duplicate comments (2)
docs/user/shell-commands.rst (1)
199-204: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winClarify targeting options behavior.
The documentation still states that devices can be targeted using "any combination" of the options. However, as noted in previous reviews, providing
devicesexplicitly determines the targets and overridesgroupandlocation. Please update this section to accurately reflect the actual precedence behavior, matching the clearer wording used in the REST API docs.📝 Proposed doc fix
-You can target devices using any combination of the following: +Target devices either with an explicit ``devices`` list or with the +``group`` and ``location`` filters. The latter two may be combined. +When ``devices`` is supplied, it determines the target set and the other two are ignored. - ``devices``: Explicit list of device UUIDs. - ``group``: All devices belonging to a device group. - ``location``: All devices at a specific location.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docs/user/shell-commands.rst` around lines 199 - 204, The targeting options documentation should no longer claim that all options can be combined. Update the section around the devices, group, and location list to state that an explicit devices list determines the targets and takes precedence over group and location, using wording consistent with the REST API documentation.tests/openwisp2/sample_connection/api/views.py (1)
1-9: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsolidate imports from the same module.
To improve code readability, consider combining these separate import statements into a single block since they originate from the same module. This was flagged in a previous review but seems to have re-appeared.
♻️ Proposed refactor
-from openwisp_controller.connection.api.views import ( - BatchCommandDetailView as BaseBatchCommandDetailView, -) -from openwisp_controller.connection.api.views import ( - BatchCommandExecuteView as BaseBatchCommandExecuteView, -) -from openwisp_controller.connection.api.views import ( - BatchCommandListView as BaseBatchCommandListView, -) +from openwisp_controller.connection.api.views import ( + BatchCommandDetailView as BaseBatchCommandDetailView, + BatchCommandExecuteView as BaseBatchCommandExecuteView, + BatchCommandListView as BaseBatchCommandListView, +)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/openwisp2/sample_connection/api/views.py` around lines 1 - 9, Consolidate the three imports from openwisp_controller.connection.api.views into one parenthesized import block, preserving the existing aliases BaseBatchCommandDetailView, BaseBatchCommandExecuteView, and BaseBatchCommandListView.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Duplicate comments:
In `@docs/user/shell-commands.rst`:
- Around line 199-204: The targeting options documentation should no longer
claim that all options can be combined. Update the section around the devices,
group, and location list to state that an explicit devices list determines the
targets and takes precedence over group and location, using wording consistent
with the REST API documentation.
In `@tests/openwisp2/sample_connection/api/views.py`:
- Around line 1-9: Consolidate the three imports from
openwisp_controller.connection.api.views into one parenthesized import block,
preserving the existing aliases BaseBatchCommandDetailView,
BaseBatchCommandExecuteView, and BaseBatchCommandListView.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 84d46ba1-5003-4525-8b99-f9ad3cf555c8
📒 Files selected for processing (24)
docs/developer/extending.rstdocs/user/intro.rstdocs/user/rest-api.rstdocs/user/shell-commands.rstopenwisp_controller/connection/api/serializers.pyopenwisp_controller/connection/api/urls.pyopenwisp_controller/connection/api/views.pyopenwisp_controller/connection/base/models.pyopenwisp_controller/connection/migrations/0011_batchcommand_command_batch_command.pyopenwisp_controller/connection/migrations/__init__.pyopenwisp_controller/connection/models.pyopenwisp_controller/connection/tasks.pyopenwisp_controller/connection/tests/pytest.pyopenwisp_controller/connection/tests/test_api.pyopenwisp_controller/connection/tests/test_models.pyopenwisp_controller/connection/tests/test_selenium.pyopenwisp_controller/connection/tests/test_tasks.pyopenwisp_controller/connection/tests/utils.pyopenwisp_controller/geo/estimated_location/tests/tests.pyopenwisp_controller/geo/tests/test_api.pytests/openwisp2/sample_connection/api/views.pytests/openwisp2/sample_connection/migrations/0005_batchcommand_command_batch_command.pytests/openwisp2/sample_connection/models.pytests/openwisp2/settings.py
📜 Review details
⏰ Context from checks skipped due to timeout. (11)
- GitHub Check: Python==3.12 | django~=5.2.0
- GitHub Check: Python==3.11 | django~=5.1.0
- GitHub Check: Python==3.12 | django~=5.1.0
- GitHub Check: Python==3.10 | django~=4.2.0
- GitHub Check: Python==3.11 | django~=4.2.0
- GitHub Check: Python==3.13 | django~=5.2.0
- GitHub Check: Python==3.10 | django~=5.1.0
- GitHub Check: Python==3.13 | django~=5.1.0
- GitHub Check: Python==3.10 | django~=5.2.0
- GitHub Check: Python==3.11 | django~=5.2.0
- GitHub Check: Python==3.12 | django~=4.2.0
🧰 Additional context used
📓 Path-based instructions (2)
**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
**/*.py: Mark user-facing strings for translation with Django i18n helpers in Django code
Avoid unnecessary blank lines inside function and method bodies
Be careful with authentication, authorization, queryset filtering, serializers, admin behavior, cache invalidation, signals, Celery tasks, and websocket updates in Django code
Preserve validation around templates, VPN/PKI material, SSH credentials, device commands, uploaded files, URLs, and subnet/IP data
Write comments and docstrings only when they explain why code is shaped a certain way, placing them before the relevant code block instead of scattering them inside it
Files:
openwisp_controller/geo/tests/test_api.pyopenwisp_controller/connection/tests/pytest.pytests/openwisp2/settings.pyopenwisp_controller/geo/estimated_location/tests/tests.pyopenwisp_controller/connection/api/urls.pyopenwisp_controller/connection/tasks.pyopenwisp_controller/connection/migrations/__init__.pyopenwisp_controller/connection/tests/utils.pytests/openwisp2/sample_connection/models.pyopenwisp_controller/connection/models.pyopenwisp_controller/connection/tests/test_selenium.pytests/openwisp2/sample_connection/api/views.pyopenwisp_controller/connection/migrations/0011_batchcommand_command_batch_command.pyopenwisp_controller/connection/tests/test_api.pyopenwisp_controller/connection/api/serializers.pyopenwisp_controller/connection/base/models.pyopenwisp_controller/connection/api/views.pytests/openwisp2/sample_connection/migrations/0005_batchcommand_command_batch_command.pyopenwisp_controller/connection/tests/test_tasks.pyopenwisp_controller/connection/tests/test_models.py
**/*.{py,html}
📄 CodeRabbit inference engine (Custom checks)
For Django pull requests, ensure all user-facing strings are marked as translatable using the Django i18n framework.
Files:
openwisp_controller/geo/tests/test_api.pyopenwisp_controller/connection/tests/pytest.pytests/openwisp2/settings.pyopenwisp_controller/geo/estimated_location/tests/tests.pyopenwisp_controller/connection/api/urls.pyopenwisp_controller/connection/tasks.pyopenwisp_controller/connection/migrations/__init__.pyopenwisp_controller/connection/tests/utils.pytests/openwisp2/sample_connection/models.pyopenwisp_controller/connection/models.pyopenwisp_controller/connection/tests/test_selenium.pytests/openwisp2/sample_connection/api/views.pyopenwisp_controller/connection/migrations/0011_batchcommand_command_batch_command.pyopenwisp_controller/connection/tests/test_api.pyopenwisp_controller/connection/api/serializers.pyopenwisp_controller/connection/base/models.pyopenwisp_controller/connection/api/views.pytests/openwisp2/sample_connection/migrations/0005_batchcommand_command_batch_command.pyopenwisp_controller/connection/tests/test_tasks.pyopenwisp_controller/connection/tests/test_models.py
🧠 Learnings (9)
📚 Learning: 2026-01-15T15:05:49.557Z
Learnt from: DragnEmperor
Repo: openwisp/openwisp-controller PR: 1175
File: openwisp_controller/config/management/commands/clear_last_ip.py:38-42
Timestamp: 2026-01-15T15:05:49.557Z
Learning: In Django projects, when using select_related() to traverse relations (for example, select_related("organization__config_settings")), the traversed relation must not be deferred. If you also use .only() in the same query, include the relation name or FK field (e.g., "organization" or "organization_id") in the .only() list to avoid the error "Field X cannot be both deferred and traversed using select_related at the same time." Apply this guideline to Django code in openwisp_controller/config/management/commands/clear_last_ip.py and similar modules by ensuring any select_related with an accompanying only() includes the related field names to prevent deferred/traversed conflicts.
Applied to files:
openwisp_controller/geo/tests/test_api.pyopenwisp_controller/connection/tests/pytest.pyopenwisp_controller/geo/estimated_location/tests/tests.pyopenwisp_controller/connection/api/urls.pyopenwisp_controller/connection/tasks.pyopenwisp_controller/connection/migrations/__init__.pyopenwisp_controller/connection/tests/utils.pyopenwisp_controller/connection/models.pyopenwisp_controller/connection/tests/test_selenium.pyopenwisp_controller/connection/migrations/0011_batchcommand_command_batch_command.pyopenwisp_controller/connection/tests/test_api.pyopenwisp_controller/connection/api/serializers.pyopenwisp_controller/connection/base/models.pyopenwisp_controller/connection/api/views.pyopenwisp_controller/connection/tests/test_tasks.pyopenwisp_controller/connection/tests/test_models.py
📚 Learning: 2026-02-17T19:13:10.088Z
Learnt from: nemesifier
Repo: openwisp/openwisp-controller PR: 1175
File: openwisp_controller/config/whois/commands.py:0-0
Timestamp: 2026-02-17T19:13:10.088Z
Learning: In reviews for the openwisp/openwisp-controller repository, do not propose changes based on Ruff warnings. The project does not use Ruff as its linter; ignore Ruff-related suggestions and follow the repository’s established linting and configuration rules. This guidance applies to all Python files under the openwisp_controller directory.
Applied to files:
openwisp_controller/geo/tests/test_api.pyopenwisp_controller/connection/tests/pytest.pyopenwisp_controller/geo/estimated_location/tests/tests.pyopenwisp_controller/connection/api/urls.pyopenwisp_controller/connection/tasks.pyopenwisp_controller/connection/migrations/__init__.pyopenwisp_controller/connection/tests/utils.pyopenwisp_controller/connection/models.pyopenwisp_controller/connection/tests/test_selenium.pyopenwisp_controller/connection/migrations/0011_batchcommand_command_batch_command.pyopenwisp_controller/connection/tests/test_api.pyopenwisp_controller/connection/api/serializers.pyopenwisp_controller/connection/base/models.pyopenwisp_controller/connection/api/views.pyopenwisp_controller/connection/tests/test_tasks.pyopenwisp_controller/connection/tests/test_models.py
📚 Learning: 2026-01-15T15:07:17.354Z
Learnt from: DragnEmperor
Repo: openwisp/openwisp-controller PR: 1175
File: openwisp_controller/geo/estimated_location/tests/tests.py:172-175
Timestamp: 2026-01-15T15:07:17.354Z
Learning: In this repository, flake8 enforces E501 (line too long) via setup.cfg (max-line-length = 88) while ruff ignores E501 via ruff.toml. Therefore, use '# noqa: E501' on lines that intentionally exceed 88 characters to satisfy flake8 without affecting ruff checks. This applies to Python files across the project (any .py) and is relevant for tests as well. Use sparingly and only where breaking lines is not feasible without hurting readability or functionality.
Applied to files:
openwisp_controller/geo/tests/test_api.pyopenwisp_controller/connection/tests/pytest.pytests/openwisp2/settings.pyopenwisp_controller/geo/estimated_location/tests/tests.pyopenwisp_controller/connection/api/urls.pyopenwisp_controller/connection/tasks.pyopenwisp_controller/connection/migrations/__init__.pyopenwisp_controller/connection/tests/utils.pytests/openwisp2/sample_connection/models.pyopenwisp_controller/connection/models.pyopenwisp_controller/connection/tests/test_selenium.pytests/openwisp2/sample_connection/api/views.pyopenwisp_controller/connection/migrations/0011_batchcommand_command_batch_command.pyopenwisp_controller/connection/tests/test_api.pyopenwisp_controller/connection/api/serializers.pyopenwisp_controller/connection/base/models.pyopenwisp_controller/connection/api/views.pytests/openwisp2/sample_connection/migrations/0005_batchcommand_command_batch_command.pyopenwisp_controller/connection/tests/test_tasks.pyopenwisp_controller/connection/tests/test_models.py
📚 Learning: 2026-06-07T12:07:08.468Z
Learnt from: stktyagi
Repo: openwisp/openwisp-controller PR: 1378
File: openwisp_controller/config/tests/test_admin.py:2335-2335
Timestamp: 2026-06-07T12:07:08.468Z
Learning: In this project’s Python test suite (files under openwisp_controller/**/tests/), don’t require or request prose/inline comments that document the breakdown of query-count changes (e.g., assertions around template/DB query counts in helpers like _verify_template_queries). Treat query-count assertions as volatile implementation details that change frequently; review should focus on whether the test asserts the expected behavior, not on explaining the specific query-count deltas in comments.
Applied to files:
openwisp_controller/geo/tests/test_api.pyopenwisp_controller/connection/tests/pytest.pyopenwisp_controller/geo/estimated_location/tests/tests.pyopenwisp_controller/connection/tests/utils.pyopenwisp_controller/connection/tests/test_selenium.pyopenwisp_controller/connection/tests/test_api.pyopenwisp_controller/connection/tests/test_tasks.pyopenwisp_controller/connection/tests/test_models.py
📚 Learning: 2026-06-07T12:07:24.608Z
Learnt from: stktyagi
Repo: openwisp/openwisp-controller PR: 1378
File: openwisp_controller/pki/tests/test_api.py:155-155
Timestamp: 2026-06-07T12:07:24.608Z
Learning: When reviewing Python test files in this repository, avoid recommending inline comments that explain or justify `assertNumQueries` (Django query count) expectations. Query counts can change frequently as implementations evolve, and inline explanations add maintenance burden; the expected count should be understandable without added comment blocks.
Applied to files:
openwisp_controller/geo/tests/test_api.pyopenwisp_controller/connection/tests/pytest.pytests/openwisp2/settings.pyopenwisp_controller/geo/estimated_location/tests/tests.pyopenwisp_controller/connection/tests/utils.pytests/openwisp2/sample_connection/models.pyopenwisp_controller/connection/tests/test_selenium.pytests/openwisp2/sample_connection/api/views.pyopenwisp_controller/connection/tests/test_api.pytests/openwisp2/sample_connection/migrations/0005_batchcommand_command_batch_command.pyopenwisp_controller/connection/tests/test_tasks.pyopenwisp_controller/connection/tests/test_models.py
📚 Learning: 2026-06-25T12:20:18.414Z
Learnt from: dee077
Repo: openwisp/openwisp-controller PR: 1395
File: openwisp_controller/connection/base/models.py:571-572
Timestamp: 2026-06-25T12:20:18.414Z
Learning: When writing or reviewing tests that override pagination behavior via OpenWispPagination.paginate_queryset(), patch `view.pagination_page_size` (not `page_size`). The method uses `getattr(view, "pagination_page_size", self.page_size)`, so tests must set the attribute on the view to affect pagination. If the view class does not define `pagination_page_size`, using `unittest.mock.patch(..., create=True)` is intentional and correct because the attribute may not exist until patched.
Applied to files:
openwisp_controller/geo/tests/test_api.pyopenwisp_controller/connection/tests/pytest.pytests/openwisp2/settings.pyopenwisp_controller/geo/estimated_location/tests/tests.pyopenwisp_controller/connection/tests/utils.pytests/openwisp2/sample_connection/models.pyopenwisp_controller/connection/tests/test_selenium.pytests/openwisp2/sample_connection/api/views.pyopenwisp_controller/connection/tests/test_api.pytests/openwisp2/sample_connection/migrations/0005_batchcommand_command_batch_command.pyopenwisp_controller/connection/tests/test_tasks.pyopenwisp_controller/connection/tests/test_models.py
📚 Learning: 2026-06-07T12:07:25.164Z
Learnt from: stktyagi
Repo: openwisp/openwisp-controller PR: 1378
File: openwisp_controller/config/tests/test_config.py:864-865
Timestamp: 2026-06-07T12:07:25.164Z
Learning: When reviewing this repo’s Python test suite, treat changes to the *expected* query count in `assertNumQueries(...)` calls as routine test maintenance. If a PR updates the numeric argument (e.g., in `test_config.py`, `test_api.py`, `test_admin.py`, `test_pki.py`) and the test remains consistent with the feature changes, reviewers should not flag the increased number as a performance regression that requires investigation solely because the count went up; instead, focus on whether the update is intentional and the surrounding test/code changes justify the revised expectation.
Applied to files:
openwisp_controller/geo/tests/test_api.pyopenwisp_controller/geo/estimated_location/tests/tests.pyopenwisp_controller/connection/tests/test_selenium.pyopenwisp_controller/connection/tests/test_api.pyopenwisp_controller/connection/tests/test_tasks.pyopenwisp_controller/connection/tests/test_models.py
📚 Learning: 2026-06-25T12:20:45.387Z
Learnt from: dee077
Repo: openwisp/openwisp-controller PR: 1395
File: openwisp_controller/connection/tests/test_api.py:916-932
Timestamp: 2026-06-25T12:20:45.387Z
Learning: When reviewing API pagination behavior in openwisp-controller, assume `OpenWispPagination.paginate_queryset()` allows a per-view page-size override via `getattr(view, "pagination_page_size", self.page_size)` (so `view.pagination_page_size`, if present, should affect pagination). In Python tests, it is valid to patch `pagination_page_size` on a view class even if the attribute isn’t declared on the class by default, by using `unittest.mock.patch.object(..., "pagination_page_size", ..., create=True)` so the override is available for the pagination logic during the test.
Applied to files:
openwisp_controller/geo/tests/test_api.pyopenwisp_controller/connection/tests/pytest.pyopenwisp_controller/geo/estimated_location/tests/tests.pyopenwisp_controller/connection/tests/utils.pyopenwisp_controller/connection/tests/test_selenium.pyopenwisp_controller/connection/tests/test_api.pyopenwisp_controller/connection/tests/test_tasks.pyopenwisp_controller/connection/tests/test_models.py
📚 Learning: 2026-03-27T20:50:26.240Z
Learnt from: nemesifier
Repo: openwisp/openwisp-controller PR: 1315
File: openwisp_controller/geo/estimated_location/service.py:70-76
Timestamp: 2026-03-27T20:50:26.240Z
Learning: In openwisp-controller’s WHOIS and estimated-location services (openwisp_controller/config/whois/ and openwisp_controller/geo/estimated_location/), these components only process public IP addresses. When reviewing logs/error/debug messages in this area, treat logging the IP address as acceptable and do not flag it as a privacy/security concern—unless the logged value can originate from non-public/private IPs in that specific code path.
Applied to files:
openwisp_controller/geo/estimated_location/tests/tests.py
🪛 ast-grep (0.44.1)
openwisp_controller/connection/tasks.py
[warning] 102-102: Loading a Keras model from an untrusted file can execute arbitrary code via Lambda layers or custom objects. Load only trusted models and avoid deserializing custom objects from untrusted sources.
Context: load_model("connection", "BatchCommand")
Note: [CWE-502] Deserialization of Untrusted Data.
(keras-load-model-python)
openwisp_controller/connection/migrations/0011_batchcommand_command_batch_command.py
[info] 60-69: use help_text to document model columns
Context: models.CharField(
choices=[
("idle", "idle"),
("in-progress", "in progress"),
("success", "success"),
("failed", "failed"),
],
default="idle",
max_length=12,
)
Note: [CWE-710] Improper Adherence to Coding Standards.
(model-help-text)
[info] 73-80: use help_text to document model columns
Context: models.CharField(
choices=(
openwisp_controller.connection.commands.COMMAND_CHOICES
if django.VERSION < (5, 0)
else openwisp_controller.connection.commands.get_command_choices # noqa: E501
),
max_length=16,
)
Note: [CWE-710] Improper Adherence to Coding Standards.
(model-help-text)
openwisp_controller/connection/tests/test_api.py
[warning] 21-21: Loading a Keras model from an untrusted file can execute arbitrary code via Lambda layers or custom objects. Load only trusted models and avoid deserializing custom objects from untrusted sources.
Context: load_model("connection", "Command")
Note: [CWE-502] Deserialization of Untrusted Data.
(keras-load-model-python)
[warning] 22-22: Loading a Keras model from an untrusted file can execute arbitrary code via Lambda layers or custom objects. Load only trusted models and avoid deserializing custom objects from untrusted sources.
Context: load_model("connection", "DeviceConnection")
Note: [CWE-502] Deserialization of Untrusted Data.
(keras-load-model-python)
[warning] 23-23: Loading a Keras model from an untrusted file can execute arbitrary code via Lambda layers or custom objects. Load only trusted models and avoid deserializing custom objects from untrusted sources.
Context: load_model("connection", "BatchCommand")
Note: [CWE-502] Deserialization of Untrusted Data.
(keras-load-model-python)
[warning] 25-25: Loading a Keras model from an untrusted file can execute arbitrary code via Lambda layers or custom objects. Load only trusted models and avoid deserializing custom objects from untrusted sources.
Context: load_model("openwisp_users", "OrganizationUser")
Note: [CWE-502] Deserialization of Untrusted Data.
(keras-load-model-python)
[warning] 26-26: Loading a Keras model from an untrusted file can execute arbitrary code via Lambda layers or custom objects. Load only trusted models and avoid deserializing custom objects from untrusted sources.
Context: load_model("openwisp_users", "Group")
Note: [CWE-502] Deserialization of Untrusted Data.
(keras-load-model-python)
[warning] 27-27: Loading a Keras model from an untrusted file can execute arbitrary code via Lambda layers or custom objects. Load only trusted models and avoid deserializing custom objects from untrusted sources.
Context: load_model("config", "DeviceGroup")
Note: [CWE-502] Deserialization of Untrusted Data.
(keras-load-model-python)
[warning] 28-28: Loading a Keras model from an untrusted file can execute arbitrary code via Lambda layers or custom objects. Load only trusted models and avoid deserializing custom objects from untrusted sources.
Context: load_model("geo", "Location")
Note: [CWE-502] Deserialization of Untrusted Data.
(keras-load-model-python)
[warning] 29-29: Loading a Keras model from an untrusted file can execute arbitrary code via Lambda layers or custom objects. Load only trusted models and avoid deserializing custom objects from untrusted sources.
Context: load_model("geo", "DeviceLocation")
Note: [CWE-502] Deserialization of Untrusted Data.
(keras-load-model-python)
[info] 1070-1070: use jsonify instead of json.dumps for JSON output
Context: json.dumps(payload)
Note: [CWE-116] Improper Encoding or Escaping of Output.
(use-jsonify)
[info] 1107-1107: use jsonify instead of json.dumps for JSON output
Context: json.dumps(payload)
Note: [CWE-116] Improper Encoding or Escaping of Output.
(use-jsonify)
[info] 1133-1133: use jsonify instead of json.dumps for JSON output
Context: json.dumps(payload)
Note: [CWE-116] Improper Encoding or Escaping of Output.
(use-jsonify)
[info] 1149-1149: use jsonify instead of json.dumps for JSON output
Context: json.dumps(payload)
Note: [CWE-116] Improper Encoding or Escaping of Output.
(use-jsonify)
[info] 1173-1173: use jsonify instead of json.dumps for JSON output
Context: json.dumps(payload)
Note: [CWE-116] Improper Encoding or Escaping of Output.
(use-jsonify)
[info] 1198-1198: use jsonify instead of json.dumps for JSON output
Context: json.dumps(payload)
Note: [CWE-116] Improper Encoding or Escaping of Output.
(use-jsonify)
[info] 1235-1241: use jsonify instead of json.dumps for JSON output
Context: json.dumps(
{
"type": "custom",
"input": {"command": "echo test"},
"label": "test-label",
}
)
Note: [CWE-116] Improper Encoding or Escaping of Output.
(use-jsonify)
[info] 1251-1258: use jsonify instead of json.dumps for JSON output
Context: json.dumps(
{
"type": "custom",
"input": {"command": "echo test"},
"label": "test-label",
"devices": [str(device1.pk), str(device2.pk)],
}
)
Note: [CWE-116] Improper Encoding or Escaping of Output.
(use-jsonify)
[info] 1291-1298: use jsonify instead of json.dumps for JSON output
Context: json.dumps(
{
"type": "custom",
"input": {"command": "echo test"},
"label": "infer-group",
"group": str(group.pk),
}
)
Note: [CWE-116] Improper Encoding or Escaping of Output.
(use-jsonify)
[info] 1316-1323: use jsonify instead of json.dumps for JSON output
Context: json.dumps(
{
"type": "custom",
"input": {"command": "echo test"},
"label": "infer-location",
"location": str(location.pk),
}
)
Note: [CWE-116] Improper Encoding or Escaping of Output.
(use-jsonify)
[info] 1361-1361: use jsonify instead of json.dumps for JSON output
Context: json.dumps(payload)
Note: [CWE-116] Improper Encoding or Escaping of Output.
(use-jsonify)
[info] 1384-1384: use jsonify instead of json.dumps for JSON output
Context: json.dumps(payload)
Note: [CWE-116] Improper Encoding or Escaping of Output.
(use-jsonify)
[info] 1415-1415: use jsonify instead of json.dumps for JSON output
Context: json.dumps(payload)
Note: [CWE-116] Improper Encoding or Escaping of Output.
(use-jsonify)
[info] 1438-1438: use jsonify instead of json.dumps for JSON output
Context: json.dumps(payload)
Note: [CWE-116] Improper Encoding or Escaping of Output.
(use-jsonify)
[info] 1474-1474: use jsonify instead of json.dumps for JSON output
Context: json.dumps(payload)
Note: [CWE-116] Improper Encoding or Escaping of Output.
(use-jsonify)
[info] 1495-1495: use jsonify instead of json.dumps for JSON output
Context: json.dumps(payload)
Note: [CWE-116] Improper Encoding or Escaping of Output.
(use-jsonify)
[info] 1526-1526: use jsonify instead of json.dumps for JSON output
Context: json.dumps(payload)
Note: [CWE-116] Improper Encoding or Escaping of Output.
(use-jsonify)
[info] 1547-1547: use jsonify instead of json.dumps for JSON output
Context: json.dumps(payload)
Note: [CWE-116] Improper Encoding or Escaping of Output.
(use-jsonify)
[info] 1640-1640: use jsonify instead of json.dumps for JSON output
Context: json.dumps({"type": "custom"})
Note: [CWE-116] Improper Encoding or Escaping of Output.
(use-jsonify)
[info] 1694-1694: use jsonify instead of json.dumps for JSON output
Context: json.dumps(payload)
Note: [CWE-116] Improper Encoding or Escaping of Output.
(use-jsonify)
[info] 1751-1751: use jsonify instead of json.dumps for JSON output
Context: json.dumps(payload)
Note: [CWE-116] Improper Encoding or Escaping of Output.
(use-jsonify)
[info] 1769-1777: use jsonify instead of json.dumps for JSON output
Context: json.dumps(
{
"organization": str(org.pk),
"type": "custom",
"input": {"command": "echo test"},
"label": "test-label",
"devices": [str(device_org2.pk)],
}
)
Note: [CWE-116] Improper Encoding or Escaping of Output.
(use-jsonify)
[info] 1798-1806: use jsonify instead of json.dumps for JSON output
Context: json.dumps(
{
"organization": str(org.pk),
"type": "custom",
"input": {"command": "echo test"},
"label": "test-label",
"group": str(group_org2.pk),
}
)
Note: [CWE-116] Improper Encoding or Escaping of Output.
(use-jsonify)
[info] 1830-1838: use jsonify instead of json.dumps for JSON output
Context: json.dumps(
{
"organization": str(org.pk),
"type": "custom",
"input": {"command": "echo test"},
"label": "test-label",
"location": str(location_org2.pk),
}
)
Note: [CWE-116] Improper Encoding or Escaping of Output.
(use-jsonify)
[info] 1975-1983: use jsonify instead of json.dumps for JSON output
Context: json.dumps(
{
"organization": str(org.pk),
"type": "custom",
"input": {"command": "echo test"},
"label": "test-label",
"devices": [str(device1.pk)],
}
)
Note: [CWE-116] Improper Encoding or Escaping of Output.
(use-jsonify)
[info] 1995-2003: use jsonify instead of json.dumps for JSON output
Context: json.dumps(
{
"organization": str(org.pk),
"type": "custom",
"input": {"command": "echo test"},
"label": "test-label",
"group": str(group.pk),
}
)
Note: [CWE-116] Improper Encoding or Escaping of Output.
(use-jsonify)
[info] 2015-2023: use jsonify instead of json.dumps for JSON output
Context: json.dumps(
{
"organization": str(org.pk),
"type": "custom",
"input": {"command": "echo test"},
"label": "test-label",
"location": str(location.pk),
}
)
Note: [CWE-116] Improper Encoding or Escaping of Output.
(use-jsonify)
[info] 2036-2045: use jsonify instead of json.dumps for JSON output
Context: json.dumps(
{
"organization": str(org.pk),
"type": "custom",
"input": {"command": "echo test"},
"label": "test-label",
"group": str(group.pk),
"location": str(location.pk),
}
)
Note: [CWE-116] Improper Encoding or Escaping of Output.
(use-jsonify)
[info] 2057-2064: use jsonify instead of json.dumps for JSON output
Context: json.dumps(
{
"organization": str(org.pk),
"type": "custom",
"input": {"command": "echo test"},
"label": "test-label",
}
)
Note: [CWE-116] Improper Encoding or Escaping of Output.
(use-jsonify)
[info] 2079-2087: use jsonify instead of json.dumps for JSON output
Context: json.dumps(
{
"organization": str(org.pk),
"type": "custom",
"input": {"command": "echo test"},
"label": "test-label",
"devices": [],
}
)
Note: [CWE-116] Improper Encoding or Escaping of Output.
(use-jsonify)
[info] 2099-2106: use jsonify instead of json.dumps for JSON output
Context: json.dumps(
{
"organization": str(org.pk),
"type": "custom",
"input": {"command": "echo test"},
"label": "test-label",
}
)
Note: [CWE-116] Improper Encoding or Escaping of Output.
(use-jsonify)
[info] 2120-2128: use jsonify instead of json.dumps for JSON output
Context: json.dumps(
{
"organization": str(org.pk),
"type": "custom",
"input": {"command": "echo test"},
"label": "",
"devices": [str(device1.pk)],
}
)
Note: [CWE-116] Improper Encoding or Escaping of Output.
(use-jsonify)
[info] 2136-2144: use jsonify instead of json.dumps for JSON output
Context: json.dumps(
{
"organization": str(org.pk),
"type": "custom",
"input": {"command": "echo test"},
"label": "a" * 65,
"devices": [str(device1.pk)],
}
)
Note: [CWE-116] Improper Encoding or Escaping of Output.
(use-jsonify)
[info] 2152-2160: use jsonify instead of json.dumps for JSON output
Context: json.dumps(
{
"organization": str(org.pk),
"type": "custom",
"input": {},
"label": "test-label",
"devices": [str(device1.pk)],
}
)
Note: [CWE-116] Improper Encoding or Escaping of Output.
(use-jsonify)
[info] 2168-2176: use jsonify instead of json.dumps for JSON output
Context: json.dumps(
{
"organization": str(org.pk),
"type": "nonexistent",
"input": {"command": "echo test"},
"label": "test-label",
"devices": [str(device1.pk)],
}
)
Note: [CWE-116] Improper Encoding or Escaping of Output.
(use-jsonify)
[info] 2194-2205: use jsonify instead of json.dumps for JSON output
Context: json.dumps(
{
"organization": str(org.pk),
"type": "change_password",
"input": {
"password": password,
"confirm_password": password,
},
"label": "change password",
"devices": [str(device.pk)],
}
)
Note: [CWE-116] Improper Encoding or Escaping of Output.
(use-jsonify)
[info] 2210-2210: use jsonify instead of json.dumps for JSON output
Context: json.dumps(batch.input)
Note: [CWE-116] Improper Encoding or Escaping of Output.
(use-jsonify)
[info] 2213-2213: use jsonify instead of json.dumps for JSON output
Context: json.dumps(list_response.data)
Note: [CWE-116] Improper Encoding or Escaping of Output.
(use-jsonify)
[info] 2218-2218: use jsonify instead of json.dumps for JSON output
Context: json.dumps(detail_response.data)
Note: [CWE-116] Improper Encoding or Escaping of Output.
(use-jsonify)
[info] 2251-2251: use jsonify instead of json.dumps for JSON output
Context: json.dumps(payload)
Note: [CWE-116] Improper Encoding or Escaping of Output.
(use-jsonify)
[info] 2303-2311: use jsonify instead of json.dumps for JSON output
Context: json.dumps(
{
"organization": str(org.pk),
"type": "custom",
"input": {"command": "echo test"},
"label": "test-label",
"devices": [str(device.pk)],
}
)
Note: [CWE-116] Improper Encoding or Escaping of Output.
(use-jsonify)
[info] 2345-2353: use jsonify instead of json.dumps for JSON output
Context: json.dumps(
{
"organization": str(org.pk),
"type": "custom",
"input": {"command": "echo test"},
"label": "test-label",
"devices": [str(device.pk)],
}
)
Note: [CWE-116] Improper Encoding or Escaping of Output.
(use-jsonify)
openwisp_controller/connection/base/models.py
[info] 751-753: use help_text to document model columns
Context: models.CharField(
max_length=12, choices=STATUS_CHOICES, default=STATUS_CHOICES[0][0]
)
Note: [CWE-710] Improper Adherence to Coding Standards.
(model-help-text)
[info] 754-757: use help_text to document model columns
Context: models.CharField(
max_length=16,
choices=(COMMAND_CHOICES if django.VERSION < (5, 0) else get_command_choices),
)
Note: [CWE-710] Improper Adherence to Coding Standards.
(model-help-text)
[warning] 864-864: Loading a Keras model from an untrusted file can execute arbitrary code via Lambda layers or custom objects. Load only trusted models and avoid deserializing custom objects from untrusted sources.
Context: load_model("connection", "Command")
Note: [CWE-502] Deserialization of Untrusted Data.
(keras-load-model-python)
[warning] 891-891: Loading a Keras model from an untrusted file can execute arbitrary code via Lambda layers or custom objects. Load only trusted models and avoid deserializing custom objects from untrusted sources.
Context: load_model("config", "Device")
Note: [CWE-502] Deserialization of Untrusted Data.
(keras-load-model-python)
[warning] 966-966: Loading a Keras model from an untrusted file can execute arbitrary code via Lambda layers or custom objects. Load only trusted models and avoid deserializing custom objects from untrusted sources.
Context: load_model("connection", "Command")
Note: [CWE-502] Deserialization of Untrusted Data.
(keras-load-model-python)
[warning] 967-967: Loading a Keras model from an untrusted file can execute arbitrary code via Lambda layers or custom objects. Load only trusted models and avoid deserializing custom objects from untrusted sources.
Context: load_model("config", "Device")
Note: [CWE-502] Deserialization of Untrusted Data.
(keras-load-model-python)
tests/openwisp2/sample_connection/migrations/0005_batchcommand_command_batch_command.py
[info] 58-67: use help_text to document model columns
Context: models.CharField(
choices=[
("idle", "idle"),
("in-progress", "in progress"),
("success", "success"),
("failed", "failed"),
],
default="idle",
max_length=12,
)
Note: [CWE-710] Improper Adherence to Coding Standards.
(model-help-text)
[info] 71-78: use help_text to document model columns
Context: models.CharField(
max_length=16,
choices=(
connection_config.commands.COMMAND_CHOICES
if django.VERSION < (5, 0)
else connection_config.commands.get_command_choices
),
)
Note: [CWE-710] Improper Adherence to Coding Standards.
(model-help-text)
openwisp_controller/connection/tests/test_tasks.py
[info] 448-448: use jsonify instead of json.dumps for JSON output
Context: json.dumps(batch.input)
Note: [CWE-116] Improper Encoding or Escaping of Output.
(use-jsonify)
openwisp_controller/connection/tests/test_models.py
[warning] 35-35: Loading a Keras model from an untrusted file can execute arbitrary code via Lambda layers or custom objects. Load only trusted models and avoid deserializing custom objects from untrusted sources.
Context: load_model("config", "DeviceGroup")
Note: [CWE-502] Deserialization of Untrusted Data.
(keras-load-model-python)
[warning] 36-36: Loading a Keras model from an untrusted file can execute arbitrary code via Lambda layers or custom objects. Load only trusted models and avoid deserializing custom objects from untrusted sources.
Context: load_model("geo", "Location")
Note: [CWE-502] Deserialization of Untrusted Data.
(keras-load-model-python)
[warning] 37-37: Loading a Keras model from an untrusted file can execute arbitrary code via Lambda layers or custom objects. Load only trusted models and avoid deserializing custom objects from untrusted sources.
Context: load_model("geo", "DeviceLocation")
Note: [CWE-502] Deserialization of Untrusted Data.
(keras-load-model-python)
🪛 Betterleaks (1.6.1)
docs/user/rest-api.rst
[high] 466-468: Discovered a potential authorization token provided in a curl command header, which could compromise the curl accessed resource.
(curl-auth-header)
[high] 560-562: Discovered a potential authorization token provided in a curl command header, which could compromise the curl accessed resource.
(curl-auth-header)
[high] 935-937: Discovered a potential authorization token provided in a curl command header, which could compromise the curl accessed resource.
(curl-auth-header)
[high] 954-956: Discovered a potential authorization token provided in a curl command header, which could compromise the curl accessed resource.
(curl-auth-header)
🔇 Additional comments (20)
openwisp_controller/connection/base/models.py (2)
18-18: LGTM!Also applies to: 34-38, 475-481, 572-573
738-1062: LGTM!openwisp_controller/connection/tasks.py (1)
101-122: LGTM!openwisp_controller/connection/migrations/0011_batchcommand_command_batch_command.py (1)
30-181: LGTM!openwisp_controller/connection/migrations/__init__.py (1)
65-89: LGTM!openwisp_controller/connection/models.py (1)
3-8: LGTM!Also applies to: 29-32
tests/openwisp2/sample_connection/migrations/0005_batchcommand_command_batch_command.py (1)
28-176: LGTM!tests/openwisp2/sample_connection/models.py (1)
4-4: LGTM!Also applies to: 33-35
tests/openwisp2/settings.py (1)
296-296: LGTM!openwisp_controller/connection/api/serializers.py (1)
15-230: LGTM!openwisp_controller/connection/api/views.py (1)
1-229: LGTM!openwisp_controller/connection/tests/test_selenium.py (2)
1-2: Replace the fixed sleep with an explicit wait.
sleep(0.3)remains timing-dependent and can race on slower CI workers. This is already raised by the existing review on Line 60.Also applies to: 58-60
70-70: LGTM!openwisp_controller/connection/tests/pytest.py (1)
68-68: LGTM!openwisp_controller/connection/tests/test_api.py (1)
6-6: LGTM!Also applies to: 18-30, 867-2371
openwisp_controller/connection/tests/test_models.py (1)
35-38: LGTM!Also applies to: 467-474, 975-1763
openwisp_controller/connection/tests/test_tasks.py (1)
1-1: LGTM!Also applies to: 20-20, 320-498
openwisp_controller/connection/tests/utils.py (1)
14-14: LGTM!Also applies to: 122-139
openwisp_controller/geo/estimated_location/tests/tests.py (1)
569-569: LGTM!openwisp_controller/geo/tests/test_api.py (1)
697-697: LGTM!
4 participants
Checklist
Reference to Existing Issue
Closes #1344.
Description of Changes
1. Models
2. APIs:
Endpoint for executing the batch command
Payload:
Response:
Endpoint for dry run to get the device ids of affected devices if a POST request is used
Response:
Applicable filters:
organizationOrganization UUID (optional)typeCommand type (optional for dry-run)inputInput data for the command (optional for dry-run)devicesComma-separated device UUIDs (optional)groupDevice group UUID (optional)locationLocation UUID (optional)execute_allSet totrueto target all devices (optional)Endpoint for specific batch command entry
Response:
3. Flow
Screenshots
1. Api GET and POST for
/batch-command/execute/.2.Batch command workflow via POST request
Screencast.from.2026-07-10.04-45-20.webm