[fix] Avoid mutating device.name in _get_common_name#1311
[fix] Avoid mutating device.name in _get_common_name#1311AYUSH676-JARVIS wants to merge 3 commits intoopenwisp:masterfrom
Conversation
📝 WalkthroughWalkthrough
Sequence Diagram(s)mermaid Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related issues
Caution Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional.
❌ Failed checks (1 error, 1 warning)
✅ Passed checks (1 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 |
Code Style and Test FailuresHello @AYUSH676-JARVIS, There are multiple
Additionally, Black reported formatting errors. Please fix these issues manually and then run |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@openwisp_controller/config/base/vpn.py`:
- Around line 895-903: Fix the inconsistent indentation around the CN formatting
block in the common name generation so the module doesn't raise
IndentationError: align the if-statement and subsequent lines to the same
indentation level as the surrounding code (use the same 8-space indent), e.g.,
ensure the conditional "if cn_format == \"{mac_address}-{name}\" and name ==
d.mac_address:" and the following "cn_format = \"{mac_address}\"" and "data =
{**d.__dict__, \"name\": name}" are indented consistently with the other
statements (name, unique_slug, cn_format, common_name) in that function so
variables name, unique_slug, cn_format, data, and common_name are processed
correctly without mutating d.name.
In `@openwisp_controller/connection/tasks.py`:
- Around line 63-79: Fix the malformed finally block and use the proper teardown
hook: move the connection cleanup code inside the finally block, ensure
indentation is correct, remove the redundant calls to close(), and instead
detect and call the actual teardown method (check getattr(device_conn,
"disconnect", None) and if callable(disconnect_method) call it). Ensure the
callable check and single invocation happen inside the finally of the try/except
around device_conn.update_config(), referencing device_conn.update_config, the
finally block, the close_method/disconnect_method variable, and removing any
extra close() invocation.
🪄 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: d62de3a5-c1fe-44a7-926e-cce0594b93dc
📒 Files selected for processing (2)
openwisp_controller/config/base/vpn.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.2.0
- GitHub Check: Python==3.13 | django~=5.1.0
- GitHub Check: Python==3.12 | django~=4.2.0
- GitHub Check: Python==3.13 | django~=5.2.0
- GitHub Check: Python==3.10 | django~=5.1.0
- GitHub Check: Python==3.10 | django~=4.2.0
- GitHub Check: Python==3.12 | django~=5.1.0
- GitHub Check: Python==3.11 | django~=5.2.0
- GitHub Check: Python==3.11 | django~=4.2.0
- GitHub Check: Python==3.10 | django~=5.2.0
- GitHub Check: Python==3.11 | django~=5.1.0
🧰 Additional context used
📓 Path-based instructions (4)
**/*.{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/config/base/vpn.pyopenwisp_controller/connection/tasks.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/config/base/vpn.pyopenwisp_controller/connection/tasks.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/config/base/vpn.pyopenwisp_controller/connection/tasks.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/config/base/vpn.pyopenwisp_controller/connection/tasks.py
🧠 Learnings (4)
📚 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/config/base/vpn.pyopenwisp_controller/connection/tasks.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/config/base/vpn.pyopenwisp_controller/connection/tasks.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/config/base/vpn.pyopenwisp_controller/connection/tasks.py
📚 Learning: 2026-03-17T09:20:10.456Z
Learnt from: pandafy
Repo: openwisp/openwisp-controller PR: 1292
File: openwisp_controller/connection/tasks.py:27-31
Timestamp: 2026-03-17T09:20:10.456Z
Learning: In `openwisp_controller/connection/tasks.py`, the `update_config` Celery task only accepts one argument `device_id`, which is always passed as a string (via `str(device.pk)`) from the call site in `openwisp_controller/connection/apps.py`. Do not flag `str(device_id) in task["args"]` as unreliable due to typed args — the args are always strings.
Applied to files:
openwisp_controller/connection/tasks.py
| logger.info(f"Updating {device} (pk: {device_id})") | ||
|
|
||
| try: | ||
| device_conn.update_config() | ||
| except Exception as e: | ||
| logger.error(f"update_config failed for device {device_id}: {e}") | ||
| raise | ||
| finally: | ||
| # ensure connection is closed | ||
| close_method = getattr(device_conn, "close", None) | ||
| if callable(close_method): | ||
| try: | ||
| close_method() | ||
| try: | ||
| device_conn.close() | ||
| except Exception as close_err: | ||
| logger.warning(f"Error closing connection: {close_err}") |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
python - <<'PY'
from pathlib import Path
import ast
import sys
path = Path("openwisp_controller/connection/tasks.py")
source = path.read_text()
try:
ast.parse(source, filename=str(path))
print(f"{path}: syntax OK")
except SyntaxError as exc:
print(f"{path}:{exc.lineno}:{exc.offset}: {exc.msg}")
sys.exit(1)
PY
printf '\nTeardown methods under openwisp_controller/connection:\n'
rg -n --type=py -C2 '\bdef (disconnect|close)\s*\(' openwisp_controller/connectionRepository: openwisp/openwisp-controller
Length of output: 178
🏁 Script executed:
# First, read the file around the problematic lines to see actual state
head -n 85 openwisp_controller/connection/tasks.py | tail -n 30Repository: openwisp/openwisp-controller
Length of output: 1031
🏁 Script executed:
# Search for teardown methods in the connection module
rg -n --type=py 'def (disconnect|close)\s*\(' openwisp_controller/connectionRepository: openwisp/openwisp-controller
Length of output: 226
Fix the syntax error and use the correct teardown hook.
This block has critical syntax errors that prevent the module from loading: the else: block is not indented, and the cleanup code is misplaced outside the finally block. Additionally, the code attempts to call close() which does not exist; the actual teardown hook in the codebase is disconnect() as defined in openwisp_controller/connection/connectors/ssh.py:169 and openwisp_controller/connection/base/models.py:362. The current code also redundantly calls cleanup twice, which will fail.
🐛 Proposed fix
else:
- logger.info(f"Updating {device} (pk: {device_id})")
-
- try:
- device_conn.update_config()
- except Exception as e:
- logger.error(f"update_config failed for device {device_id}: {e}")
- raise
- finally:
- # ensure connection is closed
- close_method = getattr(device_conn, "close", None)
-if callable(close_method):
- try:
- close_method()
- try:
- device_conn.close()
- except Exception as close_err:
- logger.warning(f"Error closing connection: {close_err}")
+ logger.info(f"Updating {device} (pk: {device_id})")
+ try:
+ device_conn.update_config()
+ except Exception:
+ logger.exception("update_config failed for device %s", device_id)
+ raise
+ finally:
+ cleanup = getattr(device_conn, "disconnect", None)
+ if not callable(cleanup):
+ cleanup = getattr(device_conn, "close", None)
+ if callable(cleanup):
+ try:
+ cleanup()
+ except Exception as close_err:
+ logger.warning(
+ "Error closing connection for device %s: %s",
+ device_id,
+ close_err,
+ )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@openwisp_controller/connection/tasks.py` around lines 63 - 79, Fix the
malformed finally block and use the proper teardown hook: move the connection
cleanup code inside the finally block, ensure indentation is correct, remove the
redundant calls to close(), and instead detect and call the actual teardown
method (check getattr(device_conn, "disconnect", None) and if
callable(disconnect_method) call it). Ensure the callable check and single
invocation happen inside the finally of the try/except around
device_conn.update_config(), referencing device_conn.update_config, the finally
block, the close_method/disconnect_method variable, and removing any extra
close() invocation.
AYUSH676-JARVIS
force-pushed
the
fix-device-name-mutation
branch
from
282500e to
f46fd1b
Compare
|
Kilo Code Review could not run — your account is out of credits. Add credits at app.kilo.ai to enable reviews on this change. |
Flake8 and Import ErrorsHello @AYUSH676-JARVIS, There are two distinct issues identified in the CI logs:
To fix these, please run
To resolve this, please check the import statements in |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@openwisp_controller/connection/tests/test_tasks.py`:
- Line 162: The long dotted path
"openwisp_controller.connection.tasks.DeviceConnection.get_working_connection"
exceeds the 88-char limit; fix it by splitting the string into adjacent string
literals (for example split between modules and class name), e.g.
"openwisp_controller.connection.tasks."
"DeviceConnection.get_working_connection" where that dotted path appears so
flake8 E501 no longer triggers while preserving the exact concatenated value
used by the test.
🪄 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: d3838fc6-e0a6-415e-928a-63b676bd18c5
📒 Files selected for processing (3)
openwisp_controller/config/base/vpn.pyopenwisp_controller/connection/tasks.pyopenwisp_controller/connection/tests/test_tasks.py
💤 Files with no reviewable changes (1)
- openwisp_controller/connection/tasks.py
📜 Review details
🧰 Additional context used
📓 Path-based instructions (4)
**/*.{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/tests/test_tasks.pyopenwisp_controller/config/base/vpn.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/tests/test_tasks.pyopenwisp_controller/config/base/vpn.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/tests/test_tasks.pyopenwisp_controller/config/base/vpn.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/test_tasks.pyopenwisp_controller/config/base/vpn.py
🧠 Learnings (5)
📓 Common learnings
Learnt from: pandafy
Repo: openwisp/openwisp-controller PR: 1292
File: openwisp_controller/connection/tasks.py:27-31
Timestamp: 2026-03-17T09:20:10.456Z
Learning: In `openwisp_controller/connection/tasks.py`, the `update_config` Celery task only accepts one argument `device_id`, which is always passed as a string (via `str(device.pk)`) from the call site in `openwisp_controller/connection/apps.py`. Do not flag `str(device_id) in task["args"]` as unreliable due to typed args — the args are always strings.
📚 Learning: 2026-03-17T09:20:10.456Z
Learnt from: pandafy
Repo: openwisp/openwisp-controller PR: 1292
File: openwisp_controller/connection/tasks.py:27-31
Timestamp: 2026-03-17T09:20:10.456Z
Learning: In `openwisp_controller/connection/tasks.py`, the `update_config` Celery task only accepts one argument `device_id`, which is always passed as a string (via `str(device.pk)`) from the call site in `openwisp_controller/connection/apps.py`. Do not flag `str(device_id) in task["args"]` as unreliable due to typed args — the args are always strings.
Applied to files:
openwisp_controller/connection/tests/test_tasks.py
📚 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/test_tasks.pyopenwisp_controller/config/base/vpn.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/test_tasks.pyopenwisp_controller/config/base/vpn.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/test_tasks.pyopenwisp_controller/config/base/vpn.py
🪛 GitHub Actions: OpenWISP Controller CI Build
openwisp_controller/connection/tests/test_tasks.py
[error] 162-162: Flake8 (E501) line too long (90 > 88 characters)
🔇 Additional comments (2)
openwisp_controller/connection/tests/test_tasks.py (1)
160-187: Good coverage for exception-time connection cleanup.This test correctly targets the
update_configfailure path and verifies the connection is closed (disconnectorclose) after an exception.openwisp_controller/config/base/vpn.py (1)
895-903: Correct fix: avoids mutatingdevice.namewhile preserving CN generation.Using a local truncated
nameand formatting with an overridden data mapping removes the side effect on the device instance and keeps the common-name behavior intact.
|
|
||
| def test_update_config_closes_connection_on_exception(self): | ||
| with mock.patch( | ||
| "openwisp_controller.connection.tasks.DeviceConnection.get_working_connection" |
There was a problem hiding this comment.
Fix flake8 E501 on patch target line.
Line 162 is over 88 chars and is currently failing CI. Please split the dotted path into adjacent string literals.
💡 Proposed fix
- "openwisp_controller.connection.tasks.DeviceConnection.get_working_connection"
+ "openwisp_controller.connection.tasks."
+ "DeviceConnection.get_working_connection"Based on learnings, in this repository flake8 enforces E501 (max-line-length = 88) and # noqa: E501 should be used sparingly.
📝 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.
| "openwisp_controller.connection.tasks.DeviceConnection.get_working_connection" | |
| "openwisp_controller.connection.tasks." | |
| "DeviceConnection.get_working_connection" |
🧰 Tools
🪛 GitHub Actions: OpenWISP Controller CI Build
[error] 162-162: Flake8 (E501) line too long (90 > 88 characters)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@openwisp_controller/connection/tests/test_tasks.py` at line 162, The long
dotted path
"openwisp_controller.connection.tasks.DeviceConnection.get_working_connection"
exceeds the 88-char limit; fix it by splitting the string into adjacent string
literals (for example split between modules and class name), e.g.
"openwisp_controller.connection.tasks."
"DeviceConnection.get_working_connection" where that dotted path appears so
flake8 E501 no longer triggers while preserving the exact concatenated value
used by the test.
Flake8, Black, and Import Order FailuresHello @AYUSH676-JARVIS and @stktyebo, (Analysis for commit 5967d05)
|
2 participants
Description
This PR fixes a mutation bug in
_get_common_name.Previously, the function was modifying
device.namedirectly: