Skip to content

feat: parent child interfaces - #87

Closed
marcinpsk wants to merge 164 commits into
fix/object-scoped-lookupsfrom
feat/parent-child-interfaces
Closed

feat: parent child interfaces#87
marcinpsk wants to merge 164 commits into
fix/object-scoped-lookupsfrom
feat/parent-child-interfaces

Conversation

@marcinpsk

@marcinpsk marcinpsk commented May 29, 2026

Copy link
Copy Markdown
Owner

Summary

Stacked on #115 — review only the delta over feat/bulk-import.

Parent / LAG / child interface relationships. Resolves LAG aggregate↔member and sub-interface parent↔child pairs from LibreNMS port_stack (with a per-OS PortStackLagPattern fallback for name-based aggregates like Po1), renders a combined Parent/LAG column with a verify control, and bulk-syncs the relationships onto NetBox interfaces. Adds the PortStackLagPattern model + migration.

Motivation / Problem

Feature. Reflect LibreNMS LAG/sub-interface topology on the NetBox side.

Scope of Change

  • Sync/Import logic
  • NetBox models / ORM
  • Database migrations
  • Web UI / templates
  • Tests

How Was This Tested?

  • Unit tests: yes — relationship resolution (either-side aggregate, Junos .N sub-units, Nokia SAP skip), row-owner pinning, verify rollback; real Device/Interface rows.
  • Manual testing: yes — Interfaces tab Parent/LAG column + bulk sync.

Risk Assessment

Writes lag/parent FKs only for user-selected rows; links validated with full_clean before save.

Backwards Compatibility

  • No breaking changes

Summary by CodeRabbit

  • New Features

    • Added configurable, OS-specific port-stack LAG patterns with create, edit, import, filtering, search, YAML export, and REST API support.
    • Added LAG-member and parent-interface synchronization for devices and virtual machines.
    • Added Parent/LAG relationship columns, status indicators, related controls, and management guidance.
  • Bug Fixes

    • Improved vendor-specific interface, sub-interface, virtual chassis, and malformed-data handling.
    • Strengthened permissions, validation, concurrency, rollback, and migrated-interface safeguards.
    • Improved VLAN and interface synchronization reliability and error reporting.

@coderabbitai

coderabbitai Bot commented May 29, 2026

Copy link
Copy Markdown

Review Change Stack

📝 Walkthrough

Walkthrough

The PR adds configurable LibreNMS port-stack LAG patterns, vendor-aware relationship resolution, stable port-ID synchronization, relationship controls, permission-scoped operations, row locking, and extensive regression coverage.

Changes

Port-stack pattern management

Layer / File(s) Summary
Pattern model and persistence
netbox_librenms_plugin/models.py, netbox_librenms_plugin/migrations/*
Adds OS-scoped regex patterns, normalization, validation, caching, YAML output, seeded defaults, and case-insensitive uniqueness.
Pattern management and API wiring
netbox_librenms_plugin/forms.py, netbox_librenms_plugin/filters.py, netbox_librenms_plugin/views/mapping_views.py, netbox_librenms_plugin/api/*, netbox_librenms_plugin/urls.py
Adds CRUD, bulk import/delete, filtering, YAML export, changelog routes, and REST API support.
Pattern UI and validation
netbox_librenms_plugin/templates/netbox_librenms_plugin/*pattern*, netbox_librenms_plugin/templates/netbox_librenms_plugin/inc/_rules_patterns_tabs.html, netbox_librenms_plugin/tables/mappings.py, netbox_librenms_plugin/tests/test_port_stack_lag_pattern.py
Adds list/detail rendering, navigation, table selection, regex guidance, and ORM/API/migration tests.

Relationship resolution and synchronization

Layer / File(s) Summary
LibreNMS relationship resolution
netbox_librenms_plugin/librenms_api.py, netbox_librenms_plugin/interface_relationships.py, netbox_librenms_plugin/views/base/interfaces_view.py
Retrieves and validates port-stack data, normalizes identifiers, handles vendor-specific shapes, resolves LAG and sub-interface relationships, and records incomplete data.
Interface synchronization flow
netbox_librenms_plugin/views/sync/interfaces.py, netbox_librenms_plugin/views/object_sync/devices.py, netbox_librenms_plugin/tables/interfaces.py
Uses stable port IDs, validates ownership and permissions, resolves VC targets, applies relationships transactionally, and renders relationship status and controls.
Browser and template behavior
netbox_librenms_plugin/static/netbox_librenms_plugin/js/librenms_sync.js, netbox_librenms_plugin/templates/netbox_librenms_plugin/_interface_sync*.html
Adds relationship auto-selection, stable row keys, rollback handling, delegated sync requests, migrated-mode controls, and incomplete-data warnings.

Permission and concurrency controls

Layer / File(s) Summary
Scoped lookup and locking
netbox_librenms_plugin/views/mixins.py, netbox_librenms_plugin/views/imports/actions.py, netbox_librenms_plugin/views/sync/migrate.py, netbox_librenms_plugin/views/sync/vlans.py, netbox_librenms_plugin/views/sync/device_fields.py
Adds permission-scoped object resolution, relocking, ownership checks, dynamic VLAN permissions, collision filtering, and concurrent-write revalidation.
Shared identity and validation helpers
netbox_librenms_plugin/utils.py
Adds strict numeric parsing, normalized relationship maps, VC ownership resolution, scoped ID matching, pagination validation, preference isolation, and shared validation formatting.
Regression and concurrency coverage
netbox_librenms_plugin/tests/*
Adds database-backed tests for permissions, locking, malformed data, cache behavior, synchronization, migration state, templates, API behavior, and concurrent updates.

Estimated code review effort: 5 (Critical) | ~120 minutes

Mergeability Score: 🔵 Low · up to 47b71

Malformed cached VLAN override IDs could prevent the interfaces table from rendering; the PR is otherwise mergeable with owner awareness and follow-up to validate IDs before lookup.

Possibly related issues

  • marcinpsk/netbox-librenms-plugin#95 — Covers the same LibreNMSAPI.resolve_port_relationships() response-shape and replay-testing objectives.
  • marcinpsk/netbox-librenms-plugin#130 — Covers shared relationship-resolution helpers and integration between verification and table flows.

Possibly related PRs

Suggested reviewers: bonzo81

Poem

A rabbit checks each port with care,
Finds LAGs hiding everywhere.
Stable IDs guide every hop,
Locked rows keep the changes sharp.
Regex seeds bloom in patterns bright—
LibreNMS links now land just right.

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 61.00% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Description check ✅ Passed The description covers the main change, motivation, scope, testing, risks, and compatibility, with only the optional Other Notes section omitted.
Title check ✅ Passed The title clearly identifies the primary parent-child interface relationship feature, which matches the main objective of the changeset.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
✨ Finishing Touches 💡 1
📝 Generate docstrings 💡
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch feat/parent-child-interfaces
✨ Simplify code
  • Create PR with simplified code
  • Commit simplified code in branch feat/parent-child-interfaces

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 10

🤖 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_librenms_plugin/librenms_api.py`:
- Around line 413-414: The call returning (True, data.get("mappings", []))
should validate that "mappings" is a list of dicts before reporting success; in
librenms_api.py replace the raw return in the function that parses the response
with logic that: ensure data.get("mappings") is a list, filter out any non-dict
entries (e.g., [m for m in mappings if isinstance(m, dict)]), and if the result
is missing/empty use an empty list and still return success=False or True as
appropriate for your function (prefer True with an empty list to match
get_device_inventory()/get_inventory_filtered() behavior); update the code paths
that call resolve_port_relationships() to rely on the validated list so callers
that call .get() on each item won't hit AttributeError/TypeError.

In `@netbox_librenms_plugin/static/netbox_librenms_plugin/js/librenms_sync.js`:
- Around line 1918-1938: The fetch response is parsed unconditionally; update
the fetch chain inside the existing fetch(url, { ... }) call so you first check
the Response.ok (the returned r object) before calling r.json() — if !r.ok
attempt to parse JSON or text to pull an error message and treat it as an error
branch (set btn.disabled = false, btn.innerHTML to the alert icon and btn.title
to the backend error or status text); if r.ok continue to parse r.json() and
handle data.status === 'success' as now. Also enhance the .catch handler to
include the caught error.message in btn.title instead of the generic 'Request
failed'. Reference the existing variables btn, url, csrf, body and the current
then/catch chain when making the change.
- Around line 346-365: The delegated change handler that auto-selects LAG
members sets memberCheckbox.checked but doesn't trigger those checkboxes' own
change listeners, so bulk-action state isn't recomputed; inside the loop in the
change handler (the document.addEventListener('change', ...) block that checks
'input[name="select"]' and the 'autoSelectLagMembers' toggle and iterates
`document.querySelectorAll('tr[data-member-of-lag="' + portId + '"]')`), after
setting `memberCheckbox.checked = checkbox.checked`, dispatch a change event on
the memberCheckbox (e.g. memberCheckbox.dispatchEvent(new Event('change', {
bubbles: true }))) so those checkboxes' listeners run and the global bulk-action
state is updated.

In `@netbox_librenms_plugin/tests/test_coverage_actions.py`:
- Around line 4243-4319: Add thin view-level regression tests that exercise
AddAsOOBView and PromoteToHostView end-to-end rather than only calling
set_librenms_oob: craft a request/POST that goes through the view code paths
which would pass oob_type="oob" (simulate the same MagicMock device/CF data used
in current tests), call AddAsOOBView and PromoteToHostView (or use the test
client to POST to their endpoints) and assert the HTTP response is not 400 and
that the device custom_field_data was updated (via get_librenms_oob or
inspecting obj.custom_field_data) to contain type=="oob" or the saved oob id;
keep the existing util-level tests but add one view-level test per path
referencing AddAsOOBView, PromoteToHostView and set_librenms_oob so the
user-visible behavior is covered.

In `@netbox_librenms_plugin/tests/test_librenms_api.py`:
- Around line 1731-1781: The tests in TestGetPortStack rely on the pytest
fixture mock_librenms_api but that fixture isn't available at class scope; move
or register mock_librenms_api at module/collection scope so pytest can find it:
either place the mock_librenms_api fixture into your project's conftest.py
(preferred) or expose it via module-level pytest_plugins in the test module so
TestGetPortStack and its methods can use it; ensure the fixture name remains
mock_librenms_api and that tests like
TestGetPortStack::test_returns_mappings_list_on_success,
test_returns_false_on_404, and test_returns_empty_list_when_no_mappings_key
import/use the same fixture.

In `@netbox_librenms_plugin/urls.py`:
- Around line 936-940: The changelog route for PortStackLagPattern is missing
the model context: update the path that registers
PortStackLagPatternChangeLogView (name "portstacklagpattern_changelog") to pass
kwargs={"model": PortStackLagPattern} so the view receives the model like the
other *ChangeLogView routes; locate the path call for
PortStackLagPatternChangeLogView and add the kwargs parameter referencing the
PortStackLagPattern model.

In `@netbox_librenms_plugin/views/base/cables_view.py`:
- Around line 116-134: OOB links are appended using link.get("local_port")
directly which skips the configured interface_name_field resolution; update the
OOB branch (where get_librenms_oob(...) and
self.librenms_api.get_device_links(oob["id"]) are used) to fetch OOB ports via
self.librenms_api.get_ports(oob["id"]) and build an oob_local_ports_map keyed by
port id that resolves the display name based on the same interface_name_field
logic used for the main device (mirror how local_ports_map is built), then use
that map to populate "local_port" and "local_port_id" when appending to
links_data so OOB interface names respect the user's naming preference.

In `@netbox_librenms_plugin/views/base/interfaces_view.py`:
- Around line 436-462: The _has_lag_signals function currently re-queries
PortStackLagPattern.objects.all() and recompiles regexes on every call; change
it to reuse cached compiled patterns by retrieving a precompiled attribute if
present (e.g. PortStackLagPattern._compiled_pattern) or by compiling once and
storing it on the model instance (check for getattr(pat_obj,
"_compiled_pattern", None) and use that) so you avoid repeated DB reads and
regex.compile calls; alternatively move the pattern-compile logic into a small
helper (cached via functools.lru_cache or a module-level variable) that returns
compiled patterns from PortStackLagPattern.lag_name_pattern, and then have
_has_lag_signals iterate those compiled regexes instead of recompiling each
time.

In `@netbox_librenms_plugin/views/imports/actions.py`:
- Around line 172-217: The update_fields branch in _save_device currently skips
model validation and only relies on DB errors; before calling
device.save(update_fields=update_fields) call device.full_clean(exclude=...) to
validate the updated fields only (build exclude as the set of model field names
not present in update_fields) and handle ValidationError the same way as the
other branch (use _err with the same message formatting). Keep existing
IntegrityError handling and HTMX-aware _err behavior; use the same
ValidationError/message_dict logic used when update_fields is None.

In `@netbox_librenms_plugin/views/sync/interfaces.py`:
- Around line 512-546: _resolve_interface_by_port_id currently only searches
interfaces directly on the passed obj, so interfaces that were created on VC
member devices (via sync_interface using device_selection_*) are never found
when running sync from the VC master; update _resolve_interface_by_port_id to,
when obj is a VC master (or when a device selection is present), iterate the
actual selected member devices' Interface/VMInterface querysets in addition to
obj itself and attempt the same librenms_id/name_hint matching against each
member device's interfaces; ensure you reference the same get_librenms_device_id
matching logic and return the matching iface and None as before so the post-sync
LAG/parent update and single-item relationship endpoints succeed.
🪄 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: Repository UI (base), Organization UI (inherited)

Review profile: ASSERTIVE

Plan: Pro

Run ID: e79507fd-bbd2-4c3f-8fb9-b2ce8e5f66f4

📥 Commits

Reviewing files that changed from the base of the PR and between 3de5c61 and f9f9995.

⛔ Files ignored due to path filters (13)
  • docs/img/Netbox-librenms-plugin-device-sync-fields.png is excluded by !**/*.png
  • docs/img/Netbox-librenms-plugin-import-page.png is excluded by !**/*.png
  • docs/img/Netbox-librenms-plugin-module-sync-tab.png is excluded by !**/*.png
  • docs/img/carrier_auto_install_rules/list.png is excluded by !**/*.png
  • docs/img/device_type_mappings/list.png is excluded by !**/*.png
  • docs/img/inventory_ignore_rules/list.png is excluded by !**/*.png
  • docs/img/module_bay_mappings/list.png is excluded by !**/*.png
  • docs/img/module_type_mappings/add.png is excluded by !**/*.png
  • docs/img/module_type_mappings/list.png is excluded by !**/*.png
  • docs/img/normalization_rules/add.png is excluded by !**/*.png
  • docs/img/normalization_rules/list.png is excluded by !**/*.png
  • docs/img/platform_mappings/add.png is excluded by !**/*.png
  • docs/img/platform_mappings/list.png is excluded by !**/*.png
📒 Files selected for processing (65)
  • docs/README.md
  • docs/feature_list.md
  • docs/librenms_import/validation.md
  • docs/usage_tips/README.md
  • docs/usage_tips/mapping_rules.md
  • docs/usage_tips/module_sync.md
  • netbox_librenms_plugin/constants.py
  • netbox_librenms_plugin/filters.py
  • netbox_librenms_plugin/forms.py
  • netbox_librenms_plugin/import_utils/__init__.py
  • netbox_librenms_plugin/import_utils/bulk_import.py
  • netbox_librenms_plugin/import_utils/collisions.py
  • netbox_librenms_plugin/import_utils/device_operations.py
  • netbox_librenms_plugin/import_utils/ip_helpers.py
  • netbox_librenms_plugin/import_utils/vm_operations.py
  • netbox_librenms_plugin/import_validation_helpers.py
  • netbox_librenms_plugin/librenms_api.py
  • netbox_librenms_plugin/migrations/0011_librenmssettings_auto_create_ipam_default.py
  • netbox_librenms_plugin/migrations/0012_portstacklagpattern.py
  • netbox_librenms_plugin/migrations/0013_portstacklagpattern_data.py
  • netbox_librenms_plugin/models.py
  • netbox_librenms_plugin/navigation.py
  • netbox_librenms_plugin/static/netbox_librenms_plugin/js/librenms_import.js
  • netbox_librenms_plugin/static/netbox_librenms_plugin/js/librenms_sync.js
  • netbox_librenms_plugin/tables/cables.py
  • netbox_librenms_plugin/tables/device_status.py
  • netbox_librenms_plugin/tables/interfaces.py
  • netbox_librenms_plugin/tables/mappings.py
  • netbox_librenms_plugin/tables/modules.py
  • netbox_librenms_plugin/templates/netbox_librenms_plugin/_interface_sync_content.html
  • netbox_librenms_plugin/templates/netbox_librenms_plugin/htmx/_dt_mapping_form.html
  • netbox_librenms_plugin/templates/netbox_librenms_plugin/htmx/_platform_mapping_form.html
  • netbox_librenms_plugin/templates/netbox_librenms_plugin/htmx/bulk_import_collision.html
  • netbox_librenms_plugin/templates/netbox_librenms_plugin/htmx/device_import_row.html
  • netbox_librenms_plugin/templates/netbox_librenms_plugin/htmx/device_validation_details.html
  • netbox_librenms_plugin/templates/netbox_librenms_plugin/librenms_import.html
  • netbox_librenms_plugin/templates/netbox_librenms_plugin/librenms_sync_base.html
  • netbox_librenms_plugin/templates/netbox_librenms_plugin/portstacklagpattern.html
  • netbox_librenms_plugin/templates/netbox_librenms_plugin/portstacklagpattern_list.html
  • netbox_librenms_plugin/templates/netbox_librenms_plugin/settings.html
  • netbox_librenms_plugin/tests/test_collisions.py
  • netbox_librenms_plugin/tests/test_coverage_actions.py
  • netbox_librenms_plugin/tests/test_coverage_base_views.py
  • netbox_librenms_plugin/tests/test_coverage_device_operations.py
  • netbox_librenms_plugin/tests/test_coverage_list.py
  • netbox_librenms_plugin/tests/test_coverage_sync_interfaces.py
  • netbox_librenms_plugin/tests/test_import_utils.py
  • netbox_librenms_plugin/tests/test_import_validation_helpers.py
  • netbox_librenms_plugin/tests/test_ip_helpers.py
  • netbox_librenms_plugin/tests/test_librenms_api.py
  • netbox_librenms_plugin/tests/test_librenms_id.py
  • netbox_librenms_plugin/tests/test_migrate_views.py
  • netbox_librenms_plugin/tests/test_port_stack_lag_pattern.py
  • netbox_librenms_plugin/urls.py
  • netbox_librenms_plugin/utils.py
  • netbox_librenms_plugin/views/__init__.py
  • netbox_librenms_plugin/views/base/cables_view.py
  • netbox_librenms_plugin/views/base/interfaces_view.py
  • netbox_librenms_plugin/views/base/librenms_sync_view.py
  • netbox_librenms_plugin/views/base/modules_view.py
  • netbox_librenms_plugin/views/imports/actions.py
  • netbox_librenms_plugin/views/imports/list.py
  • netbox_librenms_plugin/views/mapping_views.py
  • netbox_librenms_plugin/views/sync/interfaces.py
  • netbox_librenms_plugin/views/sync/migrate.py

Comment thread netbox_librenms_plugin/librenms_api.py Outdated
Comment thread netbox_librenms_plugin/static/netbox_librenms_plugin/js/librenms_sync.js Outdated
Comment thread netbox_librenms_plugin/tests/test_coverage_actions.py
Comment thread netbox_librenms_plugin/tests/test_librenms_api.py Outdated
Comment thread netbox_librenms_plugin/urls.py
Comment thread netbox_librenms_plugin/views/base/cables_view.py Outdated
Comment thread netbox_librenms_plugin/views/base/interfaces_view.py Outdated
Comment thread netbox_librenms_plugin/views/imports/actions.py
Comment thread netbox_librenms_plugin/views/sync/interfaces.py Outdated
Comment thread netbox_librenms_plugin/views/sync/interfaces.py Outdated
marcinpsk pushed a commit that referenced this pull request May 29, 2026
- librenms_api: guard against non-dict entries in port_stack iteration
- views/sync/interfaces: resolve LAG/parent interfaces across VC member devices
- urls: add missing kwargs={"model": PortStackLagPattern} to changelog route
- js: check response.ok before parsing JSON in LAG/parent sync fetch handler
- js: call updateBulkActionButton() after LAG cascade uncheck clears members
marcinpsk pushed a commit that referenced this pull request May 29, 2026
- librenms_api: guard against non-dict entries in port_stack iteration
- views/sync/interfaces: resolve LAG/parent interfaces across VC member devices
- urls: add missing kwargs={"model": PortStackLagPattern} to changelog route
- js: check response.ok before parsing JSON in LAG/parent sync fetch handler
- js: call updateBulkActionButton() after LAG cascade uncheck clears members
@marcinpsk
marcinpsk force-pushed the feat/parent-child-interfaces branch from eeb6bc2 to 4986913 Compare May 29, 2026 13:23
marcinpsk pushed a commit that referenced this pull request May 29, 2026
- librenms_api: guard against non-dict entries in port_stack iteration
- views/sync/interfaces: resolve LAG/parent interfaces across VC member devices
- urls: add missing kwargs={"model": PortStackLagPattern} to changelog route
- js: check response.ok before parsing JSON in LAG/parent sync fetch handler
- js: call updateBulkActionButton() after LAG cascade uncheck clears members
@marcinpsk
marcinpsk force-pushed the feat/parent-child-interfaces branch 11 times, most recently from f0f9eef to 8ccd827 Compare June 1, 2026 11:29
@marcinpsk
marcinpsk marked this pull request as draft June 1, 2026 11:54
@marcinpsk
marcinpsk force-pushed the feat/parent-child-interfaces branch 3 times, most recently from 7a2a5af to 432b7f8 Compare June 1, 2026 13:27
@marcinpsk

Copy link
Copy Markdown
Owner Author

@coderabbitai help

@coderabbitai

coderabbitai Bot commented Jun 1, 2026

Copy link
Copy Markdown

Chat

There are 3 ways to chat with CodeRabbit:

  • Review comments: Directly reply to a review comment made by CodeRabbit. Example:
    • I pushed a fix in commit <commit_id>, please review it.
    • Open a follow-up GitHub issue for this discussion.
  • Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query.
  • PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
    • @coderabbitai gather interesting stats about this repository and render them as a table. Additionally, render a pie chart showing the language distribution in the codebase.
    • @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.

CodeRabbit commands

These commands are invoked using PR/Issue comments.

  • @coderabbitai pause to pause the reviews on a PR.
  • @coderabbitai resume to resume the paused reviews.
  • @coderabbitai review to trigger an incremental review. This is useful when automatic reviews are disabled for the repository.
  • @coderabbitai full review to do a full review from scratch and review all the files again.
  • @coderabbitai rate limit to show your current review rate limit status.
  • @coderabbitai summary to regenerate the summary of the PR.
  • @coderabbitai evaluate custom pre-merge check --instructions <custom-checks-instructions> --name <custom-checks-title> [--mode <error|warning>] to validate the custom pre-merge checks instructions. Defaults to error when --mode is omitted.
  • @coderabbitai ignore pre-merge checks to override pre-merge checks and get an approval on PR.
  • @coderabbitai run pre-merge checks to run pre-merge checks on the pull request.
  • @coderabbitai generate docstrings to generate docstrings for this PR.
  • @coderabbitai generate sequence diagram to generate a sequence diagram of the changes in this PR.
  • @coderabbitai generate unit tests to generate unit tests for this PR.
  • @coderabbitai emit path instructions to emit generated path-specific review instructions for this repository.
  • @coderabbitai resolve merge conflict to automatically resolve merge conflicts.
  • @coderabbitai autofix to automatically fix issues identified in unresolved review comments.
  • @coderabbitai resolve to resolve all the CodeRabbit review comments.
  • @coderabbitai configuration to show the current CodeRabbit configuration for the repository.
  • @coderabbitai generate configuration to create a PR that adds the current resolved configuration as .coderabbit.yaml (or show it if already present).
  • @coderabbitai help to get help.

Other keywords and placeholders

  • Add @coderabbitai ignore or @coderabbit ignore anywhere in the PR description to prevent this PR from being reviewed.
  • Add @coderabbitai summary to generate the high-level summary at a specific location in the PR description.
  • Add @coderabbitai anywhere in the PR title to generate the title automatically.

CodeRabbit configuration file (.coderabbit.yaml)

  • You can programmatically configure CodeRabbit by adding a .coderabbit.yaml file to the root of your repository.
  • Please see the configuration documentation for more information.
  • You can also validate your configuration using the online YAML validator.
  • If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: # yaml-language-server: $schema=https://coderabbit.ai/integrations/schema.v2.json

Status, support, documentation and community

  • Visit our status page to check the current availability of CodeRabbit.
  • Create a ticket on our support page for assistance with any issues or questions.
  • Visit our documentation site for detailed information on how to use CodeRabbit.
  • Join our Discord community to connect with other users and get help from the community.
  • Follow us on X/Twitter for updates and announcements.

@marcinpsk

Copy link
Copy Markdown
Owner Author

@coderabbitai pause

@marcinpsk
marcinpsk marked this pull request as ready for review June 1, 2026 17:57
@coderabbitai

coderabbitai Bot commented Jun 1, 2026

Copy link
Copy Markdown
✅ Actions performed

Reviews paused.

@marcinpsk
marcinpsk force-pushed the feat/parent-child-interfaces branch from 432b7f8 to 7033069 Compare June 2, 2026 14:52
The guard scanner required the stack element to equal the guard string
exactly, so a compound '{% if not migrated_to_marker and x %}' guard
was falsely reported unguarded. Match the guard as one conjunct of a
pure and-chain instead; any or-compound still fails closed (with and
binding tighter than or, an or operand can render the branch with the
marker set).
The LAG/parent endpoints gate on change_interface / change_vminterface, but
NetBoxObjectPermissionMixin asks has_perm without an instance, so a CONSTRAINED
grant clears the gate. The owner was then resolved with a raw get_object_or_404
and the interface index was built off the plain manager, so a user constrained to
one site could post another site's device id and set lag/parent on interfaces it
cannot see (both ends are written: the source FK, and the aggregate's type bump).

Resolve the owner through restrict_object_or_404 and build the index through
Interface/VMInterface .restrict(user, "change"), so an out-of-grant id 404s like a
nonexistent one. The POST gate now also declares the view permission the owner
lookup performs, so a missing read grant is a stated 403 rather than a puzzling
404; reaching these endpoints already requires the device sync tab, which needs
view_device anyway.

The direct post() drives in the tests now bind the request the way View.setup()
does under dispatch, instead of leaving self.request unset.
The collision-banner test stubbed views.sync.interfaces.get_object_or_404, which
the object-scoped resolution replaced with restrict_object_or_404.
_lock_selected_device_targets and _lock_relationship_scope re-locked the object's
own virtual chassis with a raw manager filter, which the raw-pk scan reported.
The id comes from obj, which the request already resolved through a scoped
queryset, so route both through relock_scoped_row: that states the provenance
rather than relying on how the expression is written.

Dropping of=("self",) changes no lock: neither queryset joins, so the row locked
is the same one.
…l contracts

Three suites still described how the code used to behave.

SingleInterfaceVerifyView resolves the row by the stable LibreNMS port_id and
fails closed when the caller posts none, so two tests that posted only a name got
a 404. Post the port_id the view asks for, and give the shared port fixture one.

test_mixed_structural_and_name_signals_warn_when_os_is_unknown created the "ios"
PortStackLagPattern that migration 0013 already seeds, so it hit the case
insensitive uniqueness constraint. Take the seeded row instead.

test_verify_response_does_not_expose_inaccessible_vc_members asserted that
str(hidden_device.pk) was absent from the whole response body. That matches any
rendered number, so the assertion tracked pk allocation rather than a leak: it
failed when the hidden device happened to get pk 1500 and the snapshot rendered
ifMtu 1500. Assert the pk cannot appear as a device reference instead. The test
passed on the branches above only because pks landed differently there.
Two test-infrastructure gaps left this branch red at its own tip while every
test passed in isolation.

Redis is not rolled back between tests and primary keys are reused, so a value
cached under one pk was read by the next test that drew the same pk. Every test
now runs in a unique key prefix on a sibling Redis database, which also keeps
the dev server's DEBUG-time cache.clear() out of the run.

A test marked django_db(transaction=True) truncates every table, including the
rows migration 0013 seeds. pytest-django sorts every ordinary database test
ahead of every transactional one, so no test observes that flush inside the same
run. The damage is carried ACROSS runs: the flush lands in the session teardown
and leaves a reused database empty, so the next run starts with LAG pattern
detection already disabled. _reseed_after_transactional_flush restores the rows
after the database fixture finalizes, and _restore_migration_seeded_rows covers
a run that begins against an already-empty reused database by seeding inside
each test's own transaction, which is rolled back afterwards.

The per-test restore gates on the django_db marker rather than on
request.fixturenames. pytest-django's _django_db_marker requests
_django_db_helper, never the public db or transactional_db fixture, so those
names are absent from the closure and a fixturenames-only check is false for
every marker-only test. The gate keeps the fixturenames branch as well, for
tests that request db directly without the marker.
Both suites post server_key "stub" and both passed alone but failed in a full
run. test_interface_vlan_sync declares pytest_plugins at module level, which
registers the API-test helpers session-wide, and that plugin carries an autouse
fixture pinning get_plugin_config to a default-only server map. Every module
collected after it inherits the mock, so "stub" became an unknown explicit key,
LibreNMSAPI raised KeyError, build_librenms_api returned None and the view
failed closed with 400 before it reached the behaviour under test. The
concurrency thread returned for the same reason and never reached target
locking.

Each module now overrides the fixture by name, which is the pattern
test_device_fields_server_scoping already uses. Load the helper plugin
explicitly to reproduce the old failures:

  pytest -p netbox_librenms_plugin.tests.test_librenms_api_helpers \
    netbox_librenms_plugin/tests/test_verify_views.py \
    netbox_librenms_plugin/tests/test_sync_interface_concurrency.py
Relationship resolution read port_id_high and port_id_low. A live LibreNMS
26.6.1 returns {id, device_id, high_ifIndex, high_port_id, low_ifIndex,
low_port_id, ifStackStatus}, so both lookups returned None, every entry was
skipped, and LAG membership and sub-interface parenting both resolved to an
empty map. Nothing was mis-paired: the loop never reached the pairing.

LibreNMS 24.07 renamed the old port_id_high and port_id_low columns to
high_ifIndex and low_ifIndex, because they held ifIndex values, and added
high_port_id and low_port_id carrying the resolved port ids. The endpoint
serves the rows verbatim, so the response uses the column names. The API docs
still describe the pre-2024 shape, which is where the wrong spelling came from.
high_port_id is the field to read here: this resolver keys by port_id from the
ports payload, so high_ifIndex would match only where an ifIndex happens to
equal some port's id.

Every fixture in the suite used the documented spelling, so production and its
tests agreed on a shape no server sends and the suite stayed green. The fixtures
now carry the real shape: with the wrong keys restored, 29 of 39 resolver tests
fail, against none before. One test pins a verbatim live entry, and an entry
carrying neither key is now logged rather than dropped in silence.
… 4.4

Two CI-only failures on every leg, plus two more on the 4.4.0 leg.

The first two named `server_key = "stub"`. Only `default` is configured in
`media/configuration.testing.py`, which is what CI runs, so the view refused the
unconfigured server and returned before the behaviour under test. Both now use
`default`.

The 4.4.0 pair hit an upstream bug: `Interface.clean()` there compares
`self.parent.device.virtual_chassis` with `self.parent.virtual_chassis`, and
`Interface` has no `virtual_chassis` attribute (4.6 compares
`self.device.virtual_chassis`). So the validation NetBox means to run raises
AttributeError, and a parent on another chassis member 500s instead of being
accepted. `min_version` is 4.4.0 and that leg gates, so `_validate_relationship`
tolerates exactly that failure: the parent edge, `exc.name == "virtual_chassis"`,
and both interfaces on members of one chassis, which is the case the comparison
exists to allow. Anything else propagates.
Reuse the concurrent platform winner directly instead of re-reading it through a
view-restricted queryset. The branch runs only when no platform existed at
preflight, so the gate asked for ("add", Platform) and never ("view", Platform);
restrict() then returns none() for an add-only user and aborts an assign they
were authorized to perform.

Handle IntegrityError at the outer transaction in the interface sync. The
relationship pass catches it around an inner savepoint, but Postgres validates
Django's DEFERRABLE INITIALLY DEFERRED foreign keys at the outermost COMMIT, so
a concurrently deleted related row escaped that handler and returned HTTP 500.
The inner comment claimed to cover this case and no longer does.

Coerce the cached VLAN id once and drop the entry when it is not numeric. The
value comes from the LibreNMS payload, which is only checked for being a dict, so
a non-numeric VID raised ValueError and aborted a sync that had already applied
other rows.

Degrade the relationship cell when no owner resolves: reverse() with an empty
object_id raises NoReverseMatch and takes down the whole table render.

Bound the port-id regex at 19 digits, the width of a PostgreSQL bigint. An
oversized string was previously rejected only by CPython's int_max_str_digits
limit, which a host may raise or disable.

Extract _lock_mapping_in_scope() so the device-type and platform mapping views
share one scope-then-lock implementation; both copies had to stay identical for
the permission guarantee to hold.

Tests: each production change has a test that fails without it. Also cover the VM
routed sync pages (they inherit the same scoped get_object), add in-grant control
cases for the mapping-scope refusals, name the locked table in the OOB IP lock
assertion, seed a LAG pattern no migration row can match, pin the verify refusal
status, rename the fallback test to match what it asserts, and drop the worker
lock_timeout below the caller's future deadline in two "must not lock" tests.
…e class

The interface table wrote the selection and name accessors into base_columns and
the row-attribute map into _meta before calling super().__init__(). Both are
class attributes: django-tables2 deep-copies type(self).base_columns only after
__init__ runs, and reads _meta.row_attrs from the class. A table built with a
non-default interface_name_field therefore retargeted the columns for every
later table in the same worker process, across requests and users.

The two columns are now copied, given their accessor and passed through
extra_columns, which Table.__init__ applies to its own copy. The accessor has to
be set before that call because BoundColumn.accessor is cached while the columns
are bound, so a later assignment is read back as the column name and renders an
empty cell. The row-attribute map is passed as row_attrs, which binds to the
instance.

Two tests fail against the previous code: the class keeps the leaked "ifName"
accessor, and _meta.row_attrs is populated on the class.
@marcinpsk

Copy link
Copy Markdown
Owner Author

Superseded by #157.

GitHub closed this PR automatically when its base branch fix/object-scoped-lookups was deleted after #127 merged. The base could not be moved to develop because this PR belongs to a GitHub stack, which rejects base changes and exposes no API to leave the stack.

#157 has the identical head commit and content, based directly on develop.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants