fix: update tests, update interface name conflict, support isolated test db#47
Open
marcinpsk wants to merge 5 commits into
Open
fix: update tests, update interface name conflict, support isolated test db#47marcinpsk wants to merge 5 commits into
marcinpsk wants to merge 5 commits into
Conversation
…runs Two 'manage.py test' runs in the shared devcontainer collide on the single test_netbox database — they corrupt each other's migrations, and a crashed run can leave an 'idle in transaction' connection holding locks that wedges every later run. Add netbox-test-isolated: runs the suite on a per-session test DB (name derived from the app arg, or TEST_DB_NAME=...) via a small isolated_test_settings shim (--settings + PYTHONPATH). Default falls back to test_netbox, so existing workflows are unchanged. Documented in README.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (3)
WalkthroughAdds collision detection to the interface renaming engine: new helper functions pre-check name uniqueness, record conflicts, and skip colliding renames without aborting the batch. Public API functions ( ChangesCollision-safe interface renaming
Isolated test database devcontainer infrastructure
Sequence Diagram(s)sequenceDiagram
rect rgba(173, 216, 230, 0.5)
Note over RuleApplyDetailView,apply_rule_to_existing: Foreground apply (view)
RuleApplyDetailView->>apply_rule_to_existing: call(rule, interface_ids=ids, conflicts=conflicts)
apply_rule_to_existing->>_apply_rule_to_interface: per interface, conflicts=conflicts
_apply_rule_to_interface->>_name_exists_on_device: check target name
alt collision
_name_exists_on_device-->>_apply_rule_to_interface: True
_apply_rule_to_interface->>_record_conflict: append iface, log WARNING
else free
_apply_rule_to_interface->>_rename_in_place: save renamed interface
end
apply_rule_to_existing-->>RuleApplyDetailView: renamed count
RuleApplyDetailView->>RuleApplyDetailView: messages.success(renamed count)
RuleApplyDetailView->>RuleApplyDetailView: messages.warning(skipped count) if conflicts
end
rect rgba(255, 200, 150, 0.5)
Note over ApplyRuleJob,apply_rule_to_existing: Background apply (job)
ApplyRuleJob->>apply_rule_to_existing: call(rule, conflicts=conflicts)
apply_rule_to_existing-->>ApplyRuleJob: renamed count
ApplyRuleJob->>ApplyRuleJob: log.info(renamed count)
ApplyRuleJob->>ApplyRuleJob: log.warning(skipped count) if conflicts
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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 |
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
…ising
Renaming could fail when a rule's computed name was already taken on the
device. Because renames run mostly non-interactively (module install,
API/module-type change, VC-position change via signals, and the background
ApplyRuleJob), a collision must skip-and-log, never raise or partially abort
the batch.
Engine:
- Add a device-scope pre-check (_name_exists_on_device) so a colliding
rename/breakout-channel is skipped and logged at WARNING via _record_conflict;
full_clean() stays the authoritative backstop for the rarer VC cross-member
case. Idempotent breakout re-apply and atomic rollback on unexpected errors
are preserved.
- Apply the same pre-check to the device-level path (_try_rename_device_interface)
so collisions are a clean WARNING rather than an ERROR + traceback; downgrade
its full_clean backstop log from exception to warning.
- Thread an optional conflicts collector through apply_rule_to_existing /
_apply_rule_to_interface so interactive callers can report a skipped count.
Surfacing:
- RuleApplyDetailView POST -> messages.warning with the skipped count.
- ApplyRuleJob -> logs the skipped count (non-interactive: log only).
- A collision-driven 0-rename count no longer flags the rule
potentially-deprecated (that flag means "no-op rule", a different reason).
Tests:
- Add real-object collision tests (install path, apply_rule_to_existing,
breakout single-channel skip, idempotent re-apply).
- Convert mock-based tests to exercise real behavior: view apply renames a real
interface; unsafe-AST guard via real {2**3}; invalid-regex rule for
has_applicable_interfaces; undefined-var template for _find_channel_base;
real ORM objects for _process_channel_module paths. Drop the now-unused
MagicMock import. Keep genuine fault-injection mocks (loop/rollback
resilience, save-IntegrityError race, tag-write failures).
![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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
netbox_interface_name_rules/engine.py (1)
842-927: 🛠️ Refactor suggestion | 🟠 Major | 🏗️ Heavy liftRefactor
apply_rule_to_existing()to reduce complexity before it grows further.This method is now carrying branching, filtering, conflict propagation, and per-mode exception handling in one place (Sonar reports cognitive complexity 34). Extracting the channel/plain per-module branches into dedicated helpers would make this path safer to evolve and easier to test.
🤖 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 `@netbox_interface_name_rules/engine.py` around lines 842 - 927, The apply_rule_to_existing() function has high cognitive complexity due to inline branching between channel and non-channel rule handling, filtering, and exception management. Extract the two main processing paths into dedicated helper functions: one for the channel rule branch (the if rule.channel_count > 0 block that calls _find_channel_base and processes a single base interface) and one for the plain rule branch (the else block that iterates over all interfaces). Each helper should encapsulate its id_set filtering, vars_copy construction, _apply_rule_to_interface invocation, and exception handling. Replace the branched logic in apply_rule_to_existing() with calls to these helpers, leaving the module loop, interface batch-loading, and count accumulation in the main function.Source: Linters/SAST tools
pyproject.toml (1)
27-27:⚠️ Potential issue | 🟠 MajorDeclare the NetBox minimum version in
pyproject.toml.The
dependenciesarray at line 27 is empty and does not encode a NetBox runtime floor. As per coding guidelines,pyproject.tomlmust require NetBox ≥ 4.2.0 and Python ≥ 3.12. While Python is correctly specified (line 12), the NetBox constraint must be added to the[project]dependencies to enforce the packaging metadata contract.🤖 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 `@pyproject.toml` at line 27, The dependencies array in the [project] section of pyproject.toml is empty and must include the NetBox minimum version constraint. Add "netbox>=4.2.0" to the dependencies list to enforce the required NetBox runtime version, matching the Python version constraint already correctly specified elsewhere in the configuration file.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 `@netbox_interface_name_rules/engine.py`:
- Around line 564-574: Both the `_rename_in_place()` and `_create_channel()`
functions perform a pre-check for name existence but do not handle race
conditions where a concurrent operation creates the same name between the check
and the save. Wrap the save operation (iface.full_clean() and iface.save() in
`_rename_in_place()`, and the corresponding save calls in `_create_channel()`)
in a try-except block to catch IntegrityError and ValidationError exceptions.
When these exceptions are caught, treat them as collision scenarios by recording
the conflict and returning 0, ensuring the batch is not aborted despite the
concurrent collision.
In `@README.md`:
- Around line 108-109: The README.md documentation at lines 108-109 currently
states that netbox-test-isolated falls back to test_netbox with no override, but
this contradicts the actual implementation in
.devcontainer/scripts/load-aliases.sh (line 135) which always sets TEST_DB_NAME
by default and does not fall back to test_netbox. Update the sentence in
README.md to clarify that netbox-test-isolated always sets TEST_DB_NAME by
default and only falls back to test_netbox if invoked through a different path,
ensuring the documentation accurately reflects the actual behavior of the
helper.
---
Outside diff comments:
In `@netbox_interface_name_rules/engine.py`:
- Around line 842-927: The apply_rule_to_existing() function has high cognitive
complexity due to inline branching between channel and non-channel rule
handling, filtering, and exception management. Extract the two main processing
paths into dedicated helper functions: one for the channel rule branch (the if
rule.channel_count > 0 block that calls _find_channel_base and processes a
single base interface) and one for the plain rule branch (the else block that
iterates over all interfaces). Each helper should encapsulate its id_set
filtering, vars_copy construction, _apply_rule_to_interface invocation, and
exception handling. Replace the branched logic in apply_rule_to_existing() with
calls to these helpers, leaving the module loop, interface batch-loading, and
count accumulation in the main function.
In `@pyproject.toml`:
- Line 27: The dependencies array in the [project] section of pyproject.toml is
empty and must include the NetBox minimum version constraint. Add
"netbox>=4.2.0" to the dependencies list to enforce the required NetBox runtime
version, matching the Python version constraint already correctly specified
elsewhere in the configuration file.
🪄 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: 93e3ebdc-a2be-421b-83f7-ce6b30fba063
📒 Files selected for processing (9)
.devcontainer/config/isolated_test_settings.py.devcontainer/scripts/load-aliases.shREADME.mdnetbox_interface_name_rules/engine.pynetbox_interface_name_rules/jobs.pynetbox_interface_name_rules/tests/test_engine_advanced.pynetbox_interface_name_rules/tests/test_views.pynetbox_interface_name_rules/views.pypyproject.toml
Address CodeRabbit review on PR #47. - engine: apply_interface_name_rules() now wraps each _apply_rule_to_interface call in try/except (ValueError, ValidationError, IntegrityError), mirroring apply_rule_to_existing(). The collision pre-check closes the common case, but a concurrent insert can still win between the check and the save; the resulting exception exits the per-interface atomic block (already rolled back cleanly), so catching it at the caller — not inside the helpers, which run inside the still-open transaction — lets one racing interface be skipped without aborting the whole install batch. (CR I1) - engine: extract _apply_channel_rule_to_module / _apply_plain_rule_to_module from apply_rule_to_existing() to cut its cognitive complexity (Sonar 34); no behaviour change, covered by the existing apply_rule_to_existing suite. (CR B1) - README: correct the netbox-test-isolated default — it derives a per-app test DB name (test_<app>), it does not fall back to test_netbox. (CR I2) Test: new test_install_path_save_race_skips_one_interface_without_aborting_batch injects an IntegrityError on the first save (a genuine concurrency race we can't reproduce deterministically) and asserts the batch continues; confirmed it errors against the unfixed code and passes against the fix. 312 tests pass; ruff clean.
marcinpsk
commented
Jun 14, 2026
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit