[feature] BatchCommand model and REST APIs for async batch command execution#1395
[feature] BatchCommand model and REST APIs for async batch command execution#1395dee077 wants to merge 19 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:
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds a swappable batch command model, links per-device Command rows to batch records, and introduces batch validation, device resolution, per-device command creation, and status aggregation. It also adds a Celery task for batch launching, batch list/detail/execute API endpoints, sample-project wiring for the new model, and tests covering the new batch command flow. Estimated code review effort: 4 (Complex) | ~60 minutes Possibly related issues
Suggested reviewers: Caution Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional.
❌ Failed checks (2 errors)
✅ Passed checks (6 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
dee077
force-pushed
the
feature/1344-mass-command-execution-pipeline
branch
from
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.
dee077
force-pushed
the
feature/1344-mass-command-execution-pipeline
branch
from
0d7c392 to
65f73d0
Compare
|
coverage: 98.525%. first build — 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:
|
dee077
force-pushed
the
feature/1344-mass-command-execution-pipeline
branch
2 times, most recently
from
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:
|
pandafy
left a comment
pandafy
left a comment
There was a problem hiding this comment.
@dee077 can you please verify if we already have test cases for the following scenarios?
If we don't, then let's add them so we can go ahead and merge this PR.
Superuser cross-organization flow
- A superuser submitting with organization=null and having resolve_devices() return devices from more than one organization
(currently only the operator-is-rejected negative case is tested). - The detail/list response for such a no-org batch (status, resolved devices, device_count).
Serializer targeting guard
- POST with execute_all=false and no organization, devices, group, or location, expecting the "Specify at least one targeting
option" 400. This branch is currently unreachable in tests because execute_all defaults to true.
Validation and input edge cases
- Empty or whitespace-only label.
- label exceeding max_length=64.
- type=custom with missing or empty input (schema rejection through the API, not just the model).
- input provided for a command type that takes none (for example reboot).
- Unknown/invalid type value returns a clean 400.
Query-count coverage
- assertNumQueries on the execute path for a group/org-wide batch, not just the explicit-devices path.
dee077
force-pushed
the
gsoc26-mass-commands
branch
from
872fd14 to
d133bbe
Compare
Introduced AbstractBatchCommand model with calculate_and_update_status() and launch() methods to support batch command execution on multiple devices, following the pattern of BatchUpgradeOperation in openwisp-firmware-upgrader. Added batch_command FK to the existing Command model to link individual commands to their parent batch. Closes #1344
…nd execution - Removed counter DB fields - Added computed properties via aggregation - Added execute_all boolean field - Renamed launch() to create_commands() - Added execute() and dry_run() classmethods - Updated calculate_and_update_status() - Made organization FK nullable - Updated views with execute/dry_run - Updated serializer with execute_all and type/input aliases - Updated migration and celery task
…dempotency guard - Added full_clean() to dry_run() for model-level validation - Create failed Command records instead of skipping on validation error - Added idempotency guard to create_commands() via Command existence check - Narrowed ObjectDoesNotExist handler in launch_batch_command task - Return full message_dict instead of first message on ValidationError
- Fixed migration swappable dependency for config/geo apps - Log only field names (not values) in create_commands error handler - Added batch_command field to expected websocket response - Added sample_connection BatchCommand model, migration, view, and settings - Updated geo test query count assertions
…o true - Make type optional on GET requests via serializer __init__ - Skip full_clean() in dry_run when command_type is not provided - Default execute_all to True for both GET and POST
- Renamed BatchCommand fields across model, serializer, migrations, and tests - Moved test fixtures from setUp into individual test methods - _create_batch_command now requires organization as positional arg - Added test_batch_command_execute_queries with assertNumQueries(13) - Added test_batch_command_cross_org_restrictions for org-level command restrictions - Added device ID assertions in execute tests - Renamed tests: list_filter_org→list_organization_scoped, execute_no_devices→execute_org_has_no_devices
…ommand tests - Added skipped_devices JSONField to track devices skipped during batch command creation, along with API, model, and task-level tests. - Added skipped_devices JSONField to AbstractBatchCommand model - Added API tests: execute, dry-run, skipped devices, org mismatch, authorization, query counts, device targeting by group/location - Added model tests: str, total_devices/successful/failed, validation, create_commands (deactivated device, no credentials), resolve_devices, dry_run, execute, org mismatch, idempotency, status calculation, permissions - Added task tests: deleted batch resilience, single device command creation, multiple device command creation
dee077
force-pushed
the
feature/1344-mass-command-execution-pipeline
branch
from
7c80797 to
9add005
Compare
|
@coderabbitai full review |
✅ Action performedFull review finished. |
| CONNECTION_CREDENTIALS_MODEL = "sample_connection.Credentials" | ||
| CONNECTION_DEVICECONNECTION_MODEL = "sample_connection.DeviceConnection" | ||
| CONNECTION_COMMAND_MODEL = "sample_connection.Command" | ||
| CONNECTION_BATCHCOMMAND_MODEL = "sample_connection.BatchCommand" |
There was a problem hiding this comment.
This need to be mentioned in extending.rst.
There was a problem hiding this comment.
Actionable comments posted: 6
🤖 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 943-956: The idempotency guard in create_commands is only a
read-then-act check, so concurrent launch_batch_command executions can both pass
self.batch_commands.exists() and create duplicate Command rows. Make the guard
atomic by moving the transition to in-progress into a conditional update on the
BatchCommand instance (or by locking the row with select_for_update) and return
early when the status change does not succeed. Keep the rest of create_commands
using the existing Command, Device, skipped_devices, and status flow, but only
proceed after the atomic guard succeeds.
- Around line 966-981: The exception handling in the batch command creation flow
is storing raw exception text in self.skipped_devices, which can leak submitted
command/input details through the batch detail API. Update the try/except around
command.full_clean() and command.save() to translate known validation failures
into generic, field-only messages before persisting them, and avoid saving
e.messages or str(e) verbatim for schema/input-related errors. Keep the existing
logger.warning in the same command-processing path, but ensure skipped_devices
only contains sanitized, non-sensitive summaries.
- Around line 809-819: Remove the unused convenience properties from the batch
model: the cached total_devices accessor and the successful/failed count
properties in the same class. These getters are inconsistent as a
cached_property versus plain properties and are not used by the current API/task
flow, so delete them from the model rather than keeping potentially stale
helpers around. Keep the change scoped to the model methods that define
total_devices, successful, and failed.
In `@openwisp_controller/connection/tests/test_api.py`:
- Around line 878-894: The _create_batch_command helper is duplicated in the
test suite and should be centralized to prevent drift. Move the shared logic
from the _create_batch_command method into a common mixin or utility in
tests/utils.py alongside CreateConnectionsMixin, then update the test classes in
test_api.py and test_models.py to use that shared helper instead of maintaining
separate copies.
In `@openwisp_controller/connection/tests/test_models.py`:
- Around line 1095-1133: The dead assertions in the BatchCommand validation
tests are unreachable because they are placed inside the assertRaises blocks
after batch.clean() raises. Update the "group org mismatch" and "location org
mismatch" subtests in test_models.py so the ctx.exception.message_dict key
checks for "group" and "location" happen after the with
self.assertRaises(ValidationError) block, matching the existing pattern used by
the nearby BatchCommand tests. This keeps the assertions effective while still
checking the expected ValidationError messages from BatchCommand.clean().
In `@openwisp_controller/connection/tests/test_selenium.py`:
- Around line 58-60: The selenium test in test_command_submission lost the
assertion that exactly one Command record was created. After the
wait_for_visibility check and the existing sleep-based redirect handling, re-add
the Command.objects.count() assertion in the same test so it still verifies a
single submission was persisted, using the existing Command model and test flow
to locate the right spot.
🪄 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: 3797f9b4-1ac6-427f-9fa0-030c44698b94
📒 Files selected for processing (20)
docs/user/rest-api.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/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
🧰 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/api/urls.pyopenwisp_controller/geo/estimated_location/tests/tests.pyopenwisp_controller/geo/tests/test_api.pytests/openwisp2/settings.pyopenwisp_controller/connection/tests/test_selenium.pyopenwisp_controller/connection/tasks.pyopenwisp_controller/connection/migrations/__init__.pyopenwisp_controller/connection/models.pytests/openwisp2/sample_connection/models.pytests/openwisp2/sample_connection/api/views.pytests/openwisp2/sample_connection/migrations/0005_batchcommand_command_batch_command.pyopenwisp_controller/connection/migrations/0011_batchcommand_command_batch_command.pyopenwisp_controller/connection/api/serializers.pyopenwisp_controller/connection/api/views.pyopenwisp_controller/connection/tests/test_tasks.pyopenwisp_controller/connection/tests/test_api.pyopenwisp_controller/connection/base/models.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/api/urls.pyopenwisp_controller/geo/estimated_location/tests/tests.pyopenwisp_controller/geo/tests/test_api.pytests/openwisp2/settings.pyopenwisp_controller/connection/tests/test_selenium.pyopenwisp_controller/connection/tasks.pyopenwisp_controller/connection/migrations/__init__.pyopenwisp_controller/connection/models.pytests/openwisp2/sample_connection/models.pytests/openwisp2/sample_connection/api/views.pytests/openwisp2/sample_connection/migrations/0005_batchcommand_command_batch_command.pyopenwisp_controller/connection/migrations/0011_batchcommand_command_batch_command.pyopenwisp_controller/connection/api/serializers.pyopenwisp_controller/connection/api/views.pyopenwisp_controller/connection/tests/test_tasks.pyopenwisp_controller/connection/tests/test_api.pyopenwisp_controller/connection/base/models.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/api/urls.pyopenwisp_controller/geo/estimated_location/tests/tests.pyopenwisp_controller/geo/tests/test_api.pyopenwisp_controller/connection/tests/test_selenium.pyopenwisp_controller/connection/tasks.pyopenwisp_controller/connection/migrations/__init__.pyopenwisp_controller/connection/models.pyopenwisp_controller/connection/migrations/0011_batchcommand_command_batch_command.pyopenwisp_controller/connection/api/serializers.pyopenwisp_controller/connection/api/views.pyopenwisp_controller/connection/tests/test_tasks.pyopenwisp_controller/connection/tests/test_api.pyopenwisp_controller/connection/base/models.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/api/urls.pyopenwisp_controller/geo/estimated_location/tests/tests.pyopenwisp_controller/geo/tests/test_api.pyopenwisp_controller/connection/tests/test_selenium.pyopenwisp_controller/connection/tasks.pyopenwisp_controller/connection/migrations/__init__.pyopenwisp_controller/connection/models.pyopenwisp_controller/connection/migrations/0011_batchcommand_command_batch_command.pyopenwisp_controller/connection/api/serializers.pyopenwisp_controller/connection/api/views.pyopenwisp_controller/connection/tests/test_tasks.pyopenwisp_controller/connection/tests/test_api.pyopenwisp_controller/connection/base/models.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/api/urls.pyopenwisp_controller/geo/estimated_location/tests/tests.pyopenwisp_controller/geo/tests/test_api.pytests/openwisp2/settings.pyopenwisp_controller/connection/tests/test_selenium.pyopenwisp_controller/connection/tasks.pyopenwisp_controller/connection/migrations/__init__.pyopenwisp_controller/connection/models.pytests/openwisp2/sample_connection/models.pytests/openwisp2/sample_connection/api/views.pytests/openwisp2/sample_connection/migrations/0005_batchcommand_command_batch_command.pyopenwisp_controller/connection/migrations/0011_batchcommand_command_batch_command.pyopenwisp_controller/connection/api/serializers.pyopenwisp_controller/connection/api/views.pyopenwisp_controller/connection/tests/test_tasks.pyopenwisp_controller/connection/tests/test_api.pyopenwisp_controller/connection/base/models.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/geo/tests/test_api.pyopenwisp_controller/connection/tests/test_selenium.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/geo/tests/test_api.pytests/openwisp2/settings.pyopenwisp_controller/connection/tests/test_selenium.pytests/openwisp2/sample_connection/models.pytests/openwisp2/sample_connection/api/views.pytests/openwisp2/sample_connection/migrations/0005_batchcommand_command_batch_command.pyopenwisp_controller/connection/tests/test_tasks.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/geo/tests/test_api.pytests/openwisp2/settings.pyopenwisp_controller/connection/tests/test_selenium.pytests/openwisp2/sample_connection/models.pytests/openwisp2/sample_connection/api/views.pytests/openwisp2/sample_connection/migrations/0005_batchcommand_command_batch_command.pyopenwisp_controller/connection/tests/test_tasks.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/geo/tests/test_api.pyopenwisp_controller/connection/tests/test_selenium.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/geo/tests/test_api.pyopenwisp_controller/connection/tests/test_selenium.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/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] 1078-1078: 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] 1115-1115: 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] 1142-1142: 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] 1159-1159: 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] 1184-1184: 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] 1210-1210: 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] 1256-1256: 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] 1280-1280: 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] 1311-1311: 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] 1335-1335: 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] 1371-1371: 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] 1393-1393: 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] 1424-1424: 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] 1446-1446: 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] 1508-1508: 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] 1562-1562: 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] 1620-1620: 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] 1638-1646: 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] 1662-1670: 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] 1694-1702: 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] 1834-1842: 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] 1854-1862: 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] 1874-1882: 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] 1895-1904: 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] 1916-1924: 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",
"execute_all": True,
}
)
Note: [CWE-116] Improper Encoding or Escaping of Output.
(use-jsonify)
[info] 1939-1945: 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] 1957-1965: 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] 1973-1981: 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] 1989-1997: 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] 2005-2013: 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] 2049-2049: 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] 2101-2109: 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] 2143-2151: 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] 852-852: 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] 879-879: 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] 951-951: 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] 952-952: 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)
🪛 Betterleaks (1.6.0)
docs/user/rest-api.rst
[high] 559-561: Discovered a potential authorization token provided in a curl command header, which could compromise the curl accessed resource.
(curl-auth-header)
🔇 Additional comments (25)
openwisp_controller/connection/base/models.py (3)
738-807: LGTM! Rest ofAbstractBatchCommand(fields,_validate_device_org,_validate_org_relations,resolve_devices,execute,dry_run,calculate_and_update_status) matches tested behavior and previously-discussed design decisions (nullable org for superuser cross-org batches, atomic rollback on validation failure inexecute(), per-device org command allow-list enforcement).Also applies to: 821-849, 872-942, 988-1042
18-18: LGTM! Import addition,batch_commandFK with explicitrelated_name, and the child→parent status recalculation hook are consistent with the rest of the batch pipeline and prior review resolutions.Also applies to: 34-38, 475-481, 572-573
850-870: 🎯 Functional CorrectnessNo issue here:
SchemaErrorisjsonschema.exceptions.ValidationError, so invalidinputvalues are already caught and wrapped.> Likely an incorrect or invalid review comment.openwisp_controller/connection/models.py (1)
3-8: LGTM!BatchCommandmirrors the existingCommand/Credentials/DeviceConnectionswappable pattern correctly.Also applies to: 27-32
openwisp_controller/connection/tasks.py (1)
101-117: LGTM!launch_batch_commandcorrectly handles the deleted-batch case and marks the batch"failed"with logging on unexpected exceptions, consistent withlaunch_command's pattern and the accompanying tests.docs/user/rest-api.rst (2)
484-572: LGTM! Dry-run/execute/detail sections correctly reflect the repeated-query-param vs. JSON-array distinction and conditionalinputrequirement noted in earlier review rounds.Also applies to: 583-592
573-582: 🎯 Functional CorrectnessThe list endpoint includes
skipped_devices. The list view usesBatchCommandSerializer, which exposes bothdevice_countandskipped_devices.> Likely an incorrect or invalid review comment.tests/openwisp2/sample_connection/models.py (1)
4-4: LGTM!BatchCommandcorrectly mirrors the siblingCommandmodel's convention of omittingswappablein the sample app's concrete Meta.Also applies to: 31-35
openwisp_controller/connection/migrations/0011_batchcommand_command_batch_command.py (1)
1-181: LGTM!openwisp_controller/connection/migrations/__init__.py (1)
65-89: LGTM!openwisp_controller/connection/api/serializers.py (1)
15-15: LGTM!Also applies to: 47-50, 123-236
openwisp_controller/connection/api/views.py (1)
1-15: LGTM!Also applies to: 26-28, 38-38, 151-199, 211-213
openwisp_controller/connection/api/urls.py (1)
43-57: LGTM!tests/openwisp2/sample_connection/api/views.py (1)
1-9: LGTM!Also applies to: 54-74
tests/openwisp2/sample_connection/migrations/0005_batchcommand_command_batch_command.py (1)
1-176: LGTM!tests/openwisp2/settings.py (1)
296-296: LGTM!openwisp_controller/connection/tests/pytest.py (1)
68-68: LGTM!Matches the downstream
CommandConsumer.send_updatebehavior, which serializes the event dict as-is, so addingbatch_commandto the expected payload is correct.openwisp_controller/connection/tests/test_api.py (3)
917-933: LGTM!Patching
pagination_page_sizewithcreate=Trueis correct here, sinceOpenWispPagination.paginate_queryset()readsgetattr(view, "pagination_page_size", self.page_size).
1029-1146: LGTM!Query-count assertions are treated as routine test maintenance here, consistent with prior guidance on this codebase.
Based on learnings, "reviewers should not flag the increased number as a performance regression that requires investigation solely because the count went up".
1148-2169: LGTM!Good coverage of permission scoping, org mismatches, dry-run/execute parity, and skipped-device scenarios.
openwisp_controller/connection/tests/test_models.py (2)
462-475: LGTM!Updated counts reflect the new
batchcommandpermissions added to theconnectionapp content types.
975-1094: LGTM!Solid coverage of
total_devices/successful/failed,create_commandsskip scenarios,resolve_devices,dry_run,execute, org-mismatch validation, idempotency, and status aggregation, matching the model contract shown in the referenced snippets.Also applies to: 1135-1765
openwisp_controller/connection/tests/test_tasks.py (1)
319-480: LGTM!Good coverage of deleted-batch handling, single/multi-device command creation with
launch_command.delayinvocation, timeout/exception failure paths, all-devices-skipped handling, and the idempotency guard.SoftTimeLimitExceededinherits fromException, so the genericexcept Exceptionhandler inlaunch_batch_commandcorrectly catches it as exercised bytest_launch_batch_command_timeout.openwisp_controller/geo/estimated_location/tests/tests.py (1)
569-569: LGTM!Based on learnings, changes to
assertNumQueriesexpected counts are treated as routine test maintenance in this repository and the extra query here was already explained in a prior review thread as coming from Django's cascade-delete discovery mechanism.openwisp_controller/geo/tests/test_api.py (1)
697-697: LGTM!Based on learnings, this query-count bump was already explained in a prior review as resulting from Django's cascade-delete relation discovery, and such changes should not be flagged as regressions requiring investigation.
| def create_commands(self): | ||
| """ | ||
| Creates individual Command instances for each device targeted by | ||
| this batch command. Returns early if commands already exist | ||
| (idempotent guard). Devices that fail validation are recorded | ||
| in skipped_devices and logged. | ||
| """ | ||
| if self.batch_commands.exists(): | ||
| return | ||
| Command = load_model("connection", "Command") | ||
| Device = load_model("config", "Device") | ||
| self.skipped_devices = {} | ||
| self.status = "in-progress" | ||
| self.save() |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Idempotency guard is a plain read-then-act check — vulnerable to a race under concurrent/duplicate task delivery.
if self.batch_commands.exists(): return and the subsequent loop aren't protected by any lock or atomic conditional update. If launch_batch_command is redelivered (e.g. broker visibility-timeout retry on a long-running batch, or a manual retrigger overlapping with the original run), two concurrent invocations can both pass the exists() check before either has created any Command rows, and both then create duplicate per-device commands — sending the same command to devices twice. A previous review thread on this PR treated a similar concern as resolved via this same guard, but that fix only covers sequential re-invocation (as tested by test_batch_command_create_commands_idempotent), not true concurrent execution.
Consider making the guard atomic, e.g. conditionally transition status first with self.__class__.objects.filter(pk=self.pk, status="idle").update(status="in-progress") and bail out if it affects 0 rows, or wrap the check+first-write in select_for_update().
🧰 Tools
🪛 ast-grep (0.44.1)
[warning] 951-951: 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] 952-952: 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)
🤖 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 943 - 956, The
idempotency guard in create_commands is only a read-then-act check, so
concurrent launch_batch_command executions can both pass
self.batch_commands.exists() and create duplicate Command rows. Make the guard
atomic by moving the transition to in-progress into a conditional update on the
BatchCommand instance (or by locking the row with select_for_update) and return
early when the status change does not succeed. Keep the rest of create_commands
using the existing Command, Device, skipped_devices, and status flow, but only
proceed after the atomic guard succeeds.
| def _create_batch_command(self, organization, **kwargs): | ||
| opts = dict( | ||
| organization=organization, | ||
| type="custom", | ||
| input={"command": "echo test"}, | ||
| label="test-label", | ||
| ) | ||
| devices = kwargs.pop("devices", None) | ||
| opts.update(kwargs) | ||
| batch = BatchCommand(**opts) | ||
| batch.full_clean() | ||
| batch.save() | ||
| if devices is not None: | ||
| if not isinstance(devices, (list, tuple)): | ||
| devices = [devices] | ||
| batch.devices.set(devices) | ||
| return batch |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
Duplicate _create_batch_command helper across test files.
The same helper is defined nearly identically here and in test_models.py (lines 975-991). Consider extracting it into a shared mixin (e.g. alongside CreateConnectionsMixin in tests/utils.py) to avoid divergence over time.
Also applies to: 975-991
🤖 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 878 - 894, The
_create_batch_command helper is duplicated in the test suite and should be
centralized to prevent drift. Move the shared logic from the
_create_batch_command method into a common mixin or utility in tests/utils.py
alongside CreateConnectionsMixin, then update the test classes in test_api.py
and test_models.py to use that shared helper instead of maintaining separate
copies.
There was a problem hiding this comment.
@dee077 let's avoid code duplication if possible please
There was a problem hiding this comment.
@nemesifier, agreed — happy to leave this open for @dee077 to consolidate the _create_batch_command helper into a shared mixin. I'll keep flagging any further duplication in this area until it's centralized.
| with self.subTest("group org mismatch"): | ||
| group = DeviceGroup.objects.create(name="test-group", organization=org2) | ||
| batch = BatchCommand( | ||
| organization=org, | ||
| type="custom", | ||
| input={"command": "echo test"}, | ||
| label="test-label", | ||
| group=group, | ||
| ) | ||
| with self.assertRaises(ValidationError) as ctx: | ||
| batch.clean() | ||
| self.assertIn("group", ctx.exception.message_dict) | ||
| self.assertIn( | ||
| "Please ensure that the organization of this Batch command " | ||
| "and the organization of the related Device Group match", | ||
| ctx.exception.message_dict["group"][0], | ||
| ) | ||
|
|
||
| with self.subTest("location org mismatch"): | ||
| location = Location.objects.create( | ||
| name="test-location", | ||
| type="indoor", | ||
| organization=org2, | ||
| ) | ||
| batch = BatchCommand( | ||
| organization=org, | ||
| type="custom", | ||
| input={"command": "echo test"}, | ||
| label="test-label", | ||
| location=location, | ||
| ) | ||
| with self.assertRaises(ValidationError) as ctx: | ||
| batch.clean() | ||
| self.assertIn("location", ctx.exception.message_dict) | ||
| self.assertIn( | ||
| "Please ensure that the organization of this Batch command " | ||
| "and the organization of the related location match", | ||
| ctx.exception.message_dict["location"][0], | ||
| ) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Dead assertions inside assertRaises blocks never execute.
In both the "group org mismatch" and "location org mismatch" subtests, the self.assertIn("group"/"location", ctx.exception.message_dict) line is nested inside the with self.assertRaises(...) block, right after the statement that raises. Since the exception propagates out of the with block immediately, this line is unreachable and never actually runs — it silently does nothing, unlike the correctly-placed pattern used in the "devices from different org" and "invalid command type for org" subtests just above (lines 1057-1063, 1076-1082), where the equivalent check sits outside the with block.
🐛 Proposed fix
with self.assertRaises(ValidationError) as ctx:
batch.clean()
- self.assertIn("group", ctx.exception.message_dict)
+ self.assertIn("group", ctx.exception.message_dict)
self.assertIn(
"Please ensure that the organization of this Batch command "
"and the organization of the related Device Group match",
ctx.exception.message_dict["group"][0],
) with self.assertRaises(ValidationError) as ctx:
batch.clean()
- self.assertIn("location", ctx.exception.message_dict)
+ self.assertIn("location", ctx.exception.message_dict)
self.assertIn(
"Please ensure that the organization of this Batch command "
"and the organization of the related location match",
ctx.exception.message_dict["location"][0],
)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| with self.subTest("group org mismatch"): | |
| group = DeviceGroup.objects.create(name="test-group", organization=org2) | |
| batch = BatchCommand( | |
| organization=org, | |
| type="custom", | |
| input={"command": "echo test"}, | |
| label="test-label", | |
| group=group, | |
| ) | |
| with self.assertRaises(ValidationError) as ctx: | |
| batch.clean() | |
| self.assertIn("group", ctx.exception.message_dict) | |
| self.assertIn( | |
| "Please ensure that the organization of this Batch command " | |
| "and the organization of the related Device Group match", | |
| ctx.exception.message_dict["group"][0], | |
| ) | |
| with self.subTest("location org mismatch"): | |
| location = Location.objects.create( | |
| name="test-location", | |
| type="indoor", | |
| organization=org2, | |
| ) | |
| batch = BatchCommand( | |
| organization=org, | |
| type="custom", | |
| input={"command": "echo test"}, | |
| label="test-label", | |
| location=location, | |
| ) | |
| with self.assertRaises(ValidationError) as ctx: | |
| batch.clean() | |
| self.assertIn("location", ctx.exception.message_dict) | |
| self.assertIn( | |
| "Please ensure that the organization of this Batch command " | |
| "and the organization of the related location match", | |
| ctx.exception.message_dict["location"][0], | |
| ) | |
| with self.subTest("group org mismatch"): | |
| group = DeviceGroup.objects.create(name="test-group", organization=org2) | |
| batch = BatchCommand( | |
| organization=org, | |
| type="custom", | |
| input={"command": "echo test"}, | |
| label="test-label", | |
| group=group, | |
| ) | |
| with self.assertRaises(ValidationError) as ctx: | |
| batch.clean() | |
| self.assertIn("group", ctx.exception.message_dict) | |
| self.assertIn( | |
| "Please ensure that the organization of this Batch command " | |
| "and the organization of the related Device Group match", | |
| ctx.exception.message_dict["group"][0], | |
| ) | |
| with self.subTest("location org mismatch"): | |
| location = Location.objects.create( | |
| name="test-location", | |
| type="indoor", | |
| organization=org2, | |
| ) | |
| batch = BatchCommand( | |
| organization=org, | |
| type="custom", | |
| input={"command": "echo test"}, | |
| label="test-label", | |
| location=location, | |
| ) | |
| with self.assertRaises(ValidationError) as ctx: | |
| batch.clean() | |
| self.assertIn("location", ctx.exception.message_dict) | |
| self.assertIn( | |
| "Please ensure that the organization of this Batch command " | |
| "and the organization of the related location match", | |
| ctx.exception.message_dict["location"][0], | |
| ) |
🤖 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_models.py` around lines 1095 -
1133, The dead assertions in the BatchCommand validation tests are unreachable
because they are placed inside the assertRaises blocks after batch.clean()
raises. Update the "group org mismatch" and "location org mismatch" subtests in
test_models.py so the ctx.exception.message_dict key checks for "group" and
"location" happen after the with self.assertRaises(ValidationError) block,
matching the existing pattern used by the nearby BatchCommand tests. This keeps
the assertions effective while still checking the expected ValidationError
messages from BatchCommand.clean().
| # 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.
🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick win
Lost Command.objects.count() assertion.
The sleep(0.3) addition to work around the redirect race is reasonable given the beforeunload constraint already discussed in prior review. However, the removed self.assertEqual(Command.objects.count(), 1) check isn't replaced — the test now only relies on the "recent commands" tab becoming visible, which doesn't verify exactly one Command row was created (e.g. it wouldn't catch an accidental duplicate submission). Consider re-adding the count assertion after the wait_for_visibility call once the page has settled.
♻️ Proposed fix
self.open(path)
self.wait_for_visibility(
By.CSS_SELECTOR,
( # selector for Django 5.2
"`#tabs-container` li.recent.commands,"
# selector for Django 4.2
" `#tabs-container` li.recent-commands"
),
)
+ self.assertEqual(Command.objects.count(), 1)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| # Wait for the redirect triggered by command submission to complete. | |
| # Navigating away immediately can race with the redirect | |
| sleep(0.3) | |
| # Wait for the redirect triggered by command submission to complete. | |
| # Navigating away immediately can race with the redirect | |
| sleep(0.3) | |
| self.assertEqual(Command.objects.count(), 1) |
🤖 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_selenium.py` around lines 58 - 60,
The selenium test in test_command_submission lost the assertion that exactly one
Command record was created. After the wait_for_visibility check and the existing
sleep-based redirect handling, re-add the Command.objects.count() assertion in
the same test so it still verifies a single submission was persisted, using the
existing Command model and test flow to locate the right spot.
1. Passwords must not be stored. 2. Shared objects can be seen by org managers but shall not leak sensitive data of other tenants
| "group", | ||
| "location", | ||
| "device_count", | ||
| "skipped_devices", |
There was a problem hiding this comment.
For shared/global batch commands (organization=None), this can expose tenant-specific data to organization managers through the list endpoint. In particular, skipped_devices keys are device UUIDs and their messages may reveal details from organizations the user does not manage. Showing general shared batch metadata may be fine, but tenant-specific fields should be filtered or redacted. I added a failing regression test: TestBatchCommandsAPI.test_shared_batch_command_does_not_expose_other_tenant_data.
There was a problem hiding this comment.
Updated!
Now it will show
"Org A device": ""restricted to {org_name} managers and users"
instead of just not showing an entry for skipped devices
| many=True, | ||
| queryset=Device.objects.all(), | ||
| required=False, | ||
| allow_empty=True, |
There was a problem hiding this comment.
Question about the API contract: are we sure that accepting devices=[] and treating it the same as omitted devices is the intended behavior? Many clients interpret an empty list as an explicit empty selection. With an organization provided, this can become an all-devices target. If that is intentional, the docs should say so clearly. If not, the code should distinguish a missing devices key from an empty list.
There was a problem hiding this comment.
Made these work for above scenerios with execute_all and other possible options
| return self.label | ||
|
|
||
| @cached_property | ||
| def total_devices(self): |
There was a problem hiding this comment.
Are these stats properties intended to be used later by the admin or API? Right now I only found tests using total_devices, successful, and failed; the API exposes only the annotated device_count.
I just want to double check with you that this is not dead code.
Black and Flake8 FailuresHello @dee077 and @nemesifier, (Analysis for commit 05e8d94)
Test Failures
|
dee077
changed the title
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