diff --git a/CHANGELOG.md b/CHANGELOG.md index 1c70624..2e5cda7 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -2,6 +2,10 @@ Generated from the README compatibility table by `scripts/gen_changelog.py`. Do not edit by hand. +## v2.4.5 + +Fix: sync no longer crashes on netbox-branching **1.1.1** (`SquashMergeStrategy has no attribute '_split_bidirectional_cycles'` — 1.1.1 removed that internal helper; the bidirectional-cycle split is now built into the plugin and the dependency is bounded to `<1.2`). Also fixes SNMP-endpoint rows failing validation: the bundled endpoint query branches now clamp sysDescr-derived `device_type` to NetBox's 100-char limit (`substring`) and guard empty slugs — the fix lives in the NQE queries (the source of truth), so **Publish Bundled Queries** again after upgrading (fixes the `Ensure this value has at most 100 characters` rejects and the `At least one coalesce lookup must be provided` error). + ## v2.4.4 Fix: SNMP-endpoint import now works on **tag-scoped** syncs — the device-tag include scope silently excluded every endpoint, both query-side and in the plugin's local scope filter (whose scoped-device set was built from modeled devices only, so endpoint rows were always dropped; with prune enabled they would even be deleted). Endpoint import now ignores the include scope (exclude tags still apply) and endpoint names join the scoped set (validated live: 355 Avocent endpoints import under a tag-scoped sync). Also fixes the merge-phase `Tag with this Name already exists` issues: a same-named/same-slug tag already on main is now treated as merged instead of failing the branch's tag create. diff --git a/README.md b/README.md index f2d525d..f5abe31 100644 --- a/README.md +++ b/README.md @@ -66,7 +66,8 @@ Latest release requires NetBox `4.6.4` and `netbox-branching` `1.1.0+`. Expand f | Plugin Release | NetBox Version | Status | | --- | --- | --- | -| `v2.4.4` | `4.6.4` required; needs netbox-branching `1.1.0+` | Current release; Fix: SNMP-endpoint import now works on **tag-scoped** syncs — the device-tag include scope silently excluded every endpoint, both query-side and in the plugin's local scope filter (whose scoped-device set was built from modeled devices only, so endpoint rows were always dropped; with prune enabled they would even be deleted). Endpoint import now ignores the include scope (exclude tags still apply) and endpoint names join the scoped set (validated live: 355 Avocent endpoints import under a tag-scoped sync). Also fixes the merge-phase `Tag with this Name already exists` issues: a same-named/same-slug tag already on main is now treated as merged instead of failing the branch's tag create. | +| `v2.4.5` | `4.6.4` required; needs netbox-branching `1.1.0` – `1.1.x` | Current release; Fix: sync no longer crashes on netbox-branching **1.1.1** (`SquashMergeStrategy has no attribute '_split_bidirectional_cycles'` — 1.1.1 removed that internal helper; the bidirectional-cycle split is now built into the plugin and the dependency is bounded to `<1.2`). Also fixes SNMP-endpoint rows failing validation: the bundled endpoint query branches now clamp sysDescr-derived `device_type` to NetBox's 100-char limit (`substring`) and guard empty slugs — the fix lives in the NQE queries (the source of truth), so **Publish Bundled Queries** again after upgrading (fixes the `Ensure this value has at most 100 characters` rejects and the `At least one coalesce lookup must be provided` error). | +| `v2.4.4` | `4.6.4` required; needs netbox-branching `1.1.0+` | Superseded by `v2.4.5`; Fix: SNMP-endpoint import now works on **tag-scoped** syncs — the device-tag include scope silently excluded every endpoint, both query-side and in the plugin's local scope filter (whose scoped-device set was built from modeled devices only, so endpoint rows were always dropped; with prune enabled they would even be deleted). Endpoint import now ignores the include scope (exclude tags still apply) and endpoint names join the scoped set (validated live: 355 Avocent endpoints import under a tag-scoped sync). Also fixes the merge-phase `Tag with this Name already exists` issues: a same-named/same-slug tag already on main is now treated as merged instead of failing the branch's tag create. | | `v2.4.3` | `4.6.4` required; needs netbox-branching `1.1.0+` | Superseded by `v2.4.4`; Fix: the pinned-query opt-in Health warning (2.4.1) over-claimed failure — it reads "nothing new syncs" and fires on any pinned map with the feature on, even after the query is fixed, because the Health page can't read a pinned query's contents. Reworded to a "Pinned — can't verify locally" heads-up that points at **Export Live Query Drift** to confirm (`source_matches_bundled`), instead of asserting failure. No behavior change. | | `v2.4.2` | `4.6.4` required; needs netbox-branching `1.1.0+` | Superseded by `v2.4.3`; Fix: endpoint import (`sync_endpoints`) and device-tag sync (`sync_device_tags`) now work with the alias-aware and rules-aware query variants (`forward_devices_with_netbox_aliases`, `forward_device_feature_tags_with_rules`), not just the base queries — operators running the variants saw the toggles silently do nothing (validated live: 355 Avocent endpoints import; `Mgmt_*` tags sync). Adds a **Publish Bundled Queries** button on the sync Health page (beside Refresh Query IDs) and two Health warnings: when an opt-in feature is enabled but no enabled map provides it, and when a base query and its opt-in variant are both enabled (they double-apply rows for the same model and churn — enable one). The alias-aware device query now emits the clean role name (e.g. `ROUTER`) to match the base query — expect a one-time role update on alias-mapped devices. | | `v2.4.1` | `4.6.4` required; needs netbox-branching `1.1.0+` | Superseded by `v2.4.2`; Fix: opt-in features (SNMP endpoint import, device-tag sync) silently did nothing on sources that run org-managed **pinned** Forward query IDs predating the feature — the sync Health page now raises an actionable warning instead of a silent badge. Remediation: publish the bundled queries to your Forward org folder (Overwrite on), then use Refresh Query IDs, then re-sync. | diff --git a/docs/01_User_Guide/README.md b/docs/01_User_Guide/README.md index c2756a6..5329587 100644 --- a/docs/01_User_Guide/README.md +++ b/docs/01_User_Guide/README.md @@ -20,14 +20,14 @@ pip install forward-netbox Alternatively, install a specific wheel or source archive from GitHub Releases: ```bash -pip install /path/to/forward_netbox-2.4.4-py3-none-any.whl -pip install /path/to/forward_netbox-2.4.4.tar.gz +pip install /path/to/forward_netbox-2.4.5-py3-none-any.whl +pip install /path/to/forward_netbox-2.4.5.tar.gz ``` If you mirror the package into a private Python index, pin the same release version: ```bash -pip install forward-netbox==2.4.4 +pip install forward-netbox==2.4.5 ``` ## Release Compatibility @@ -39,7 +39,8 @@ Latest release requires NetBox `4.6.4` and `netbox-branching` `1.1.0+`. Expand f | Plugin Release | NetBox Version | Status | | --- | --- | --- | -| `v2.4.4` | `4.6.4` required (4.5.x dropped); needs netbox-branching `1.1.0+` | Current release; Fix: SNMP-endpoint import now works on **tag-scoped** syncs — the device-tag include scope silently excluded every endpoint, both query-side and in the plugin's local scope filter (whose scoped-device set was built from modeled devices only, so endpoint rows were always dropped; with prune enabled they would even be deleted). Endpoint import now ignores the include scope (exclude tags still apply) and endpoint names join the scoped set. Also fixes the merge-phase `Tag with this Name already exists` issues: a same-named/same-slug tag already on main is now treated as merged instead of failing the branch's tag create. | +| `v2.4.5` | `4.6.4` required (4.5.x dropped); needs netbox-branching `1.1.0` – `1.1.x` | Current release; Fix: sync no longer crashes on netbox-branching **1.1.1** (`SquashMergeStrategy has no attribute '_split_bidirectional_cycles'` — 1.1.1 removed that internal helper; the bidirectional-cycle split is now built into the plugin and the dependency is bounded to `<1.2`). Also fixes SNMP-endpoint rows failing validation: the bundled endpoint query branches now clamp sysDescr-derived `device_type` to NetBox's 100-char limit (`substring`) and guard empty slugs — the fix lives in the NQE queries (the source of truth), so **Publish Bundled Queries** again after upgrading (fixes the `Ensure this value has at most 100 characters` rejects and the `At least one coalesce lookup must be provided` error). | +| `v2.4.4` | `4.6.4` required (4.5.x dropped); needs netbox-branching `1.1.0+` | Superseded by `v2.4.5`; Fix: SNMP-endpoint import now works on **tag-scoped** syncs — the device-tag include scope silently excluded every endpoint, both query-side and in the plugin's local scope filter (whose scoped-device set was built from modeled devices only, so endpoint rows were always dropped; with prune enabled they would even be deleted). Endpoint import now ignores the include scope (exclude tags still apply) and endpoint names join the scoped set. Also fixes the merge-phase `Tag with this Name already exists` issues: a same-named/same-slug tag already on main is now treated as merged instead of failing the branch's tag create. | | `v2.4.3` | `4.6.4` required (4.5.x dropped); needs netbox-branching `1.1.0+` | Superseded by `v2.4.4`; Fix: the pinned-query opt-in Health warning (2.4.1) over-claimed failure and fired even after the query was fixed (the Health page can't read a pinned query's contents). Reworded to a "Pinned — can't verify locally" heads-up pointing at Export Live Query Drift to confirm. No behavior change. | | `v2.4.2` | `4.6.4` required (4.5.x dropped); needs netbox-branching `1.1.0+` | Superseded by `v2.4.3`; Fix: endpoint import (`sync_endpoints`) and device-tag sync (`sync_device_tags`) now work with the alias-aware and rules-aware query variants (`forward_devices_with_netbox_aliases`, `forward_device_feature_tags_with_rules`), not just the base queries — operators running the variants saw the toggles silently do nothing. Adds a **Publish Bundled Queries** button on the sync Health page and two Health warnings: when an opt-in feature is enabled but no enabled map provides it, and when a base query and its opt-in variant are both enabled (duplicate-emission churn). The alias-aware device query now emits the clean role name (e.g. `ROUTER`) to match the base query — expect a one-time role update on alias-mapped devices. | | `v2.4.1` | `4.6.4` required (4.5.x dropped); needs netbox-branching `1.1.0+` | Superseded by `v2.4.2`; Fix: opt-in features (SNMP endpoint import, device-tag sync) silently did nothing on sources that run org-managed **pinned** Forward query IDs predating the feature — the sync Health page now raises an actionable warning instead of a silent badge. Remediation: publish the bundled queries to your Forward org folder (Overwrite on), then use Refresh Query IDs, then re-sync. | diff --git a/docs/03_Plans/active/2026-07-09-branching-111-compat-endpoint-identity.md b/docs/03_Plans/active/2026-07-09-branching-111-compat-endpoint-identity.md new file mode 100644 index 0000000..6b076f3 --- /dev/null +++ b/docs/03_Plans/active/2026-07-09-branching-111-compat-endpoint-identity.md @@ -0,0 +1,74 @@ +# Fix: netbox-branching 1.1.1 compat + endpoint identity clamping (2.4.5) + +## Goal + +Stop the sync-fatal `type object 'SquashMergeStrategy' has no attribute +'_split_bidirectional_cycles'` on netbox-branching 1.1.1, and stop SNMP-endpoint +rows failing validation on long/degenerate sysDescr-derived identity fields. + +## Constraints + +- No schema or migration changes; drop-in from 2.4.4. +- On netbox-branching 1.1.0 behavior must stay bit-identical (prefer the + framework helper when present). +- Identity clamping must not alter endpoint names (row identity / scope + keys) and must live in the queries — NQE is the source of truth; the plugin + must not normalize or mutate rows. +- Never commit the ADP network id / token. + +## Touched Surfaces + +- `forward_netbox/utilities/bulk_merge.py` (vendored + `_split_bidirectional_create_cycles` + `_create_has_fk_to`, guarded + `_log_cycle_details`) +- `forward_netbox/queries/forward_devices.nqe` and + `forward_devices_with_netbox_aliases.nqe` (endpoint identity clamping) +- `pyproject.toml` (`netboxlabs-netbox-branching >=1.1.0,<1.2.0`) +- Tests: `test_bulk_merge.py`, `test_endpoints_import.py` + +## Approach + +1. netbox-branching **1.1.1 removed** `SquashMergeStrategy._split_bidirectional_cycles` + (and `_has_fk_to`) — its own ordering now breaks cycles inline, which this + plugin's fast ordering does not use. A fresh `pip install` resolves the + unbounded `>=1.1.0` dependency to 1.1.1 and the first merge dies. Fix: + vendor the 2-node bidirectional-cycle splitter (faithful mirror of the 1.1.0 + idiom: NULL the nullable FK on one CREATE, append a synthetic UPDATE), + prefer the framework helper when it exists, guard `_log_cycle_details`, and + bound the dependency to `<1.2.0` so future internal churn fails at install + time, not mid-merge. +2. Endpoint rows emit `device_type` from the raw SNMP sysDescr, which exceeds + NetBox's 100-char `DeviceType.model`/`slug` limits (observed 251 chars → 18 + per-row rejects) and can slugify to `""` (the + `At least one coalesce lookup must be provided` error). Fix: clamp identity + **in the bundled NQE queries** — the endpoint branches derive + `ep_model = substring(sysDescr, 0, 100)`, slugify the clamped value, and + guard empty slugs/manufacturers with explicit fallbacks. NQE stays the + source of truth; the plugin does not normalize or mutate rows. Because the + queries changed, operators must Publish Bundled Queries after upgrading. + +## Validation + +Unit tests: the vendored splitter breaks a real Device↔VirtualChassis +bidirectional CREATE pair; ordering succeeds with the framework attribute +deleted (simulated 1.1.1); clamping covers long values, slug fallbacks, +symbol-only values, and leaves other models/normal rows untouched. Full suite + +lint + harness green. + +## Rollback + +Revert the branch — pure Python + dependency-range change; no data migration. + +## Decision Log + +- Vendor the splitter rather than adopt 1.1.1's inline cycle breaking: the + plugin runs its own topological sort for bulk merge, so it needs the + pre-pass regardless of framework version; preferring the framework helper + when present keeps 1.1.0 unchanged. +- Clamp in the NQE queries rather than the plugin: NQE's `substring` + builtin self-clamps indices, the transformation is visible in the query text + operators publish and review, and the plugin never silently rewrites what + the query emitted (maintainer decision — NQE is the source of truth). +- Upper-bound the branching dependency (`<1.2.0`): the bulk merge borrows + framework internals by design; an install-time conflict is a clearer failure + than a mid-merge AttributeError. diff --git a/docs/README.md b/docs/README.md index 0416d8a..f9d1c4d 100644 --- a/docs/README.md +++ b/docs/README.md @@ -13,7 +13,8 @@ Latest release requires NetBox `4.6.4` and `netbox-branching` `1.1.0+`. Expand f | Plugin Release | NetBox Version | Status | | --- | --- | --- | -| `v2.4.4` | `4.6.4` required; needs netbox-branching `1.1.0+` | Current release; Fix: SNMP-endpoint import now works on **tag-scoped** syncs — the device-tag include scope silently excluded every endpoint, both query-side and in the plugin's local scope filter (whose scoped-device set was built from modeled devices only, so endpoint rows were always dropped; with prune enabled they would even be deleted). Endpoint import now ignores the include scope (exclude tags still apply) and endpoint names join the scoped set. Also fixes the merge-phase `Tag with this Name already exists` issues: a same-named/same-slug tag already on main is now treated as merged instead of failing the branch's tag create. | +| `v2.4.5` | `4.6.4` required; needs netbox-branching `1.1.0` – `1.1.x` | Current release; Fix: sync no longer crashes on netbox-branching **1.1.1** (`SquashMergeStrategy has no attribute '_split_bidirectional_cycles'` — 1.1.1 removed that internal helper; the bidirectional-cycle split is now built into the plugin and the dependency is bounded to `<1.2`). Also fixes SNMP-endpoint rows failing validation: the bundled endpoint query branches now clamp sysDescr-derived `device_type` to NetBox's 100-char limit (`substring`) and guard empty slugs — the fix lives in the NQE queries (the source of truth), so **Publish Bundled Queries** again after upgrading (fixes the `Ensure this value has at most 100 characters` rejects and the `At least one coalesce lookup must be provided` error). | +| `v2.4.4` | `4.6.4` required; needs netbox-branching `1.1.0+` | Superseded by `v2.4.5`; Fix: SNMP-endpoint import now works on **tag-scoped** syncs — the device-tag include scope silently excluded every endpoint, both query-side and in the plugin's local scope filter (whose scoped-device set was built from modeled devices only, so endpoint rows were always dropped; with prune enabled they would even be deleted). Endpoint import now ignores the include scope (exclude tags still apply) and endpoint names join the scoped set. Also fixes the merge-phase `Tag with this Name already exists` issues: a same-named/same-slug tag already on main is now treated as merged instead of failing the branch's tag create. | | `v2.4.3` | `4.6.4` required; needs netbox-branching `1.1.0+` | Superseded by `v2.4.4`; Fix: the pinned-query opt-in Health warning (2.4.1) over-claimed failure and fired even after the query was fixed (the Health page can't read a pinned query's contents). Reworded to a "Pinned — can't verify locally" heads-up pointing at Export Live Query Drift to confirm. No behavior change. | | `v2.4.2` | `4.6.4` required; needs netbox-branching `1.1.0+` | Superseded by `v2.4.3`; Fix: endpoint import (`sync_endpoints`) and device-tag sync (`sync_device_tags`) now work with the alias-aware and rules-aware query variants (`forward_devices_with_netbox_aliases`, `forward_device_feature_tags_with_rules`), not just the base queries — operators running the variants saw the toggles silently do nothing. Adds a **Publish Bundled Queries** button on the sync Health page and two Health warnings: when an opt-in feature is enabled but no enabled map provides it, and when a base query and its opt-in variant are both enabled (duplicate-emission churn). The alias-aware device query now emits the clean role name (e.g. `ROUTER`) to match the base query — expect a one-time role update on alias-mapped devices. | | `v2.4.1` | `4.6.4` required; needs netbox-branching `1.1.0+` | Superseded by `v2.4.2`; Fix: opt-in features (SNMP endpoint import, device-tag sync) silently did nothing on sources that run org-managed **pinned** Forward query IDs predating the feature — the sync Health page now raises an actionable warning instead of a silent badge. Remediation: publish the bundled queries to your Forward org folder (Overwrite on), then use Refresh Query IDs, then re-sync. | diff --git a/forward_netbox/__init__.py b/forward_netbox/__init__.py index 46e6a48..fd2479d 100644 --- a/forward_netbox/__init__.py +++ b/forward_netbox/__init__.py @@ -8,7 +8,7 @@ class NetboxForwardConfig(PluginConfig): name = "forward_netbox" verbose_name = "Forward" description = "Sync Forward data into NetBox using built-in NQE queries." - version = "2.4.4" + version = "2.4.5" base_url = "forward" min_version = "4.6.4" diff --git a/forward_netbox/queries/forward_devices.nqe b/forward_netbox/queries/forward_devices.nqe index 1ca4680..7135b6e 100644 --- a/forward_netbox/queries/forward_devices.nqe +++ b/forward_netbox/queries/forward_devices.nqe @@ -55,24 +55,33 @@ foreach row in ( let sysDescrOpt = max(foreach o in endpoint.snmpOutputs where o.requestedOid == "1.3.6.1.2.1.1.1" select max(foreach e in o.rawOidEntries select e.rawValue)) let sysObjIdOpt = max(foreach o in endpoint.snmpOutputs where o.requestedOid == "1.3.6.1.2.1.1.2" select max(foreach e in o.rawOidEntries select e.rawValue)) let sysDescr = if isPresent(sysDescrOpt) then sysDescrOpt else "SNMP Endpoint" + // NetBox caps DeviceType.model/slug and Manufacturer.name at 100 chars, and + // sysDescr is free-form text (observed >250 chars, or symbol-only strings + // that slugify to nothing) — clamp identity here so rows arrive valid. + let ep_model = substring(sysDescr, 0, 100) + let ep_model_slug_raw = substring(slugifyNetboxModel(ep_model), 0, 100) + let ep_model_slug = if ep_model_slug_raw == "" then "unknown" else ep_model_slug_raw let isAvocent = matches(if isPresent(sysObjIdOpt) then sysObjIdOpt else "", "*10418*") - let ep_manuf = if isAvocent then "Avocent" else replaceRegexMatches(sysDescr, re` .*`, "") + let ep_manuf_raw = if isAvocent then "Avocent" else substring(replaceRegexMatches(sysDescr, re` .*`, ""), 0, 100) + let ep_manuf = if ep_manuf_raw == "" then "Unknown" else ep_manuf_raw + let ep_manuf_slug_raw = slugify(ep_manuf) + let ep_manuf_slug = if ep_manuf_slug_raw == "" then "unknown" else ep_manuf_slug_raw let ep_role = if isAvocent then "Console Server" else "SNMP Endpoint" let ep_site = if isPresent(endpoint.locationName) then toLowerCase(endpoint.locationName) else "unknown" select { name: endpoint.name, manufacturer: ep_manuf, - device_type: sysDescr, - device_type_slug: slugifyNetboxModel(sysDescr), + device_type: ep_model, + device_type_slug: ep_model_slug, site: ep_site, site_slug: slugify(ep_site), role: ep_role, role_slug: slugify(ep_role), role_color: "9e9e9e", platform: ep_manuf, - platform_slug: slugify(ep_manuf), + platform_slug: ep_manuf_slug, status: "active", - manufacturer_slug: slugify(ep_manuf) + manufacturer_slug: ep_manuf_slug } ) ) diff --git a/forward_netbox/queries/forward_devices_with_netbox_aliases.nqe b/forward_netbox/queries/forward_devices_with_netbox_aliases.nqe index d9c441f..c06fc72 100644 --- a/forward_netbox/queries/forward_devices_with_netbox_aliases.nqe +++ b/forward_netbox/queries/forward_devices_with_netbox_aliases.nqe @@ -98,24 +98,33 @@ foreach row in ( let sysDescrOpt = max(foreach o in endpoint.snmpOutputs where o.requestedOid == "1.3.6.1.2.1.1.1" select max(foreach e in o.rawOidEntries select e.rawValue)) let sysObjIdOpt = max(foreach o in endpoint.snmpOutputs where o.requestedOid == "1.3.6.1.2.1.1.2" select max(foreach e in o.rawOidEntries select e.rawValue)) let sysDescr = if isPresent(sysDescrOpt) then sysDescrOpt else "SNMP Endpoint" + // NetBox caps DeviceType.model/slug and Manufacturer.name at 100 chars, and + // sysDescr is free-form text (observed >250 chars, or symbol-only strings + // that slugify to nothing) — clamp identity here so rows arrive valid. + let ep_model = substring(sysDescr, 0, 100) + let ep_model_slug_raw = substring(slugifyNetboxModel(ep_model), 0, 100) + let ep_model_slug = if ep_model_slug_raw == "" then "unknown" else ep_model_slug_raw let isAvocent = matches(if isPresent(sysObjIdOpt) then sysObjIdOpt else "", "*10418*") - let ep_manuf = if isAvocent then "Avocent" else replaceRegexMatches(sysDescr, re` .*`, "") + let ep_manuf_raw = if isAvocent then "Avocent" else substring(replaceRegexMatches(sysDescr, re` .*`, ""), 0, 100) + let ep_manuf = if ep_manuf_raw == "" then "Unknown" else ep_manuf_raw + let ep_manuf_slug_raw = slugify(ep_manuf) + let ep_manuf_slug = if ep_manuf_slug_raw == "" then "unknown" else ep_manuf_slug_raw let ep_role = if isAvocent then "Console Server" else "SNMP Endpoint" let ep_site = if isPresent(endpoint.locationName) then toLowerCase(endpoint.locationName) else "unknown" select { name: endpoint.name, manufacturer: ep_manuf, - device_type: sysDescr, - device_type_slug: slugifyNetboxModel(sysDescr), + device_type: ep_model, + device_type_slug: ep_model_slug, site: ep_site, site_slug: slugify(ep_site), role: ep_role, role_slug: slugify(ep_role), role_color: "9e9e9e", platform: ep_manuf, - platform_slug: slugify(ep_manuf), + platform_slug: ep_manuf_slug, status: "active", - manufacturer_slug: slugify(ep_manuf) + manufacturer_slug: ep_manuf_slug } ) ) diff --git a/forward_netbox/tests/test_bulk_merge.py b/forward_netbox/tests/test_bulk_merge.py index e8b56a8..5e2befa 100644 --- a/forward_netbox/tests/test_bulk_merge.py +++ b/forward_netbox/tests/test_bulk_merge.py @@ -267,7 +267,9 @@ def _run_order(self, n, *, chain): objs = self._stub_changes(n, chain=chain) logger = logging.getLogger("forward_netbox.tests.ordering") with ( - patch.object(SquashMergeStrategy, "_split_bidirectional_cycles"), + patch.object( + SquashMergeStrategy, "_split_bidirectional_cycles", create=True + ), patch.object( SquashMergeStrategy, "_build_fk_dependency_graph", @@ -327,9 +329,11 @@ def test_unbreakable_cycle_raises(self): objs = {0: a, 1: b} logger = logging.getLogger("forward_netbox.tests.ordering") with ( - patch.object(SquashMergeStrategy, "_split_bidirectional_cycles"), + patch.object( + SquashMergeStrategy, "_split_bidirectional_cycles", create=True + ), patch.object(SquashMergeStrategy, "_build_fk_dependency_graph"), - patch.object(SquashMergeStrategy, "_log_cycle_details"), + patch.object(SquashMergeStrategy, "_log_cycle_details", create=True), patch.object(bulk_merge.squash_dependency_graph_built, "send"), ): with self.assertRaises(Exception): @@ -620,3 +624,80 @@ def test_tag_create_name_collision_skips_instead_of_failing(self): collapsed = call.args[0] self.assertNotEqual(collapsed.key[0], "extras.tag") self.assertEqual(Tag.objects.filter(name="Mgmt_Coll").count(), 1) + + +class BranchingCompatCycleSplitTest(TestCase): + """Ordering must survive netbox_branching 1.1.1, which removed + SquashMergeStrategy._split_bidirectional_cycles (sync died with + AttributeError). The vendored splitter must break a bidirectional CREATE + pair (Device.virtual_chassis <-> VirtualChassis.master) the same way.""" + + def _bidirectional_pair(self): + from types import SimpleNamespace + + from dcim.models import Device + from dcim.models import VirtualChassis + from netbox_branching.merge_strategies.squash import CollapsedChange + + from forward_netbox.utilities.bulk_merge import ActionType + + dev = CollapsedChange(("dcim.device", 1), Device) + dev.final_action = ActionType.CREATE + dev.postchange_data = {"name": "vc-member", "virtual_chassis": 5} + dev.last_change = SimpleNamespace(time=1, pk=1, request_id=uuid.uuid4()) + + vc = CollapsedChange(("dcim.virtualchassis", 5), VirtualChassis) + vc.final_action = ActionType.CREATE + vc.postchange_data = {"name": "vc-1", "master": 1} + vc.last_change = SimpleNamespace(time=2, pk=2, request_id=uuid.uuid4()) + return {dev.key: dev, vc.key: vc} + + def test_vendored_splitter_breaks_bidirectional_create_pair(self): + from forward_netbox.utilities.bulk_merge import ( + _split_bidirectional_create_cycles, + ) + + to_process = self._bidirectional_pair() + _split_bidirectional_create_cycles(to_process, logging.getLogger("compat-test")) + + synthetic = [k for k in to_process if len(k) == 3] + self.assertEqual(len(synthetic), 1) + nulled = [ + c + for c in to_process.values() + if c.final_action.value == "create" + and c.postchange_data.get("virtual_chassis") is None + or c.final_action.value == "create" + and c.postchange_data.get("master") is None + ] + self.assertTrue(nulled, "one CREATE of the pair must have its FK NULLed") + + def test_ordering_succeeds_when_framework_splitter_is_absent(self): + # Simulate netbox_branching 1.1.1, which removed the class attribute + # (delete it for the duration of the ordering call, then restore). + from netbox_branching.merge_strategies.squash import SquashMergeStrategy + + from forward_netbox.utilities.bulk_merge import ( + _order_collapsed_changes_fast, + ) + + to_process = self._bidirectional_pair() + original = SquashMergeStrategy.__dict__.get("_split_bidirectional_cycles") + if original is not None: + delattr(SquashMergeStrategy, "_split_bidirectional_cycles") + try: + self.assertFalse( + hasattr(SquashMergeStrategy, "_split_bidirectional_cycles") + ) + ordered = _order_collapsed_changes_fast( + to_process, logging.getLogger("compat-test"), operation="merge" + ) + finally: + if original is not None: + setattr(SquashMergeStrategy, "_split_bidirectional_cycles", original) + # The function orders an internal copy: 2 CREATEs plus the synthetic + # UPDATEs added by the cycle splitter(s). Success = no cycle exception + # and both original keys present in the ordering. + ordered_keys = {c.key[:2] for c in ordered} + for key in to_process: + self.assertIn(key[:2], ordered_keys) diff --git a/forward_netbox/tests/test_endpoints_import.py b/forward_netbox/tests/test_endpoints_import.py index 5e2b113..d4ebd8b 100644 --- a/forward_netbox/tests/test_endpoints_import.py +++ b/forward_netbox/tests/test_endpoints_import.py @@ -261,3 +261,31 @@ def test_endpoint_branches_do_not_filter_include_tags(self): endpoint_branch = src.split("network.endpoints", 1)[1] self.assertIn("device_tag_exclude_tags", endpoint_branch, filename) self.assertNotIn("device_tag_include_tags", endpoint_branch, filename) + + +class EndpointIdentityClampTest(SimpleTestCase): + """Endpoint identity must be clamped in the QUERY, not mutated in Python. + + NQE is the source of truth: sysDescr is free-form (observed >250 chars, or + symbol-only strings that slugify to nothing), so the endpoint branches clamp + device_type to 100 chars via substring() and guard empty slugs — NetBox + receives exactly what the query emitted. + """ + + def test_endpoint_branches_clamp_model_and_guard_slugs(self): + for filename in ( + "forward_devices.nqe", + "forward_devices_with_netbox_aliases.nqe", + ): + src = _read_query(filename) + endpoint_branch = src.split("network.endpoints", 1)[1] + self.assertIn("substring(sysDescr, 0, 100)", endpoint_branch, filename) + self.assertIn( + 'if ep_model_slug_raw == "" then "unknown"', endpoint_branch, filename + ) + self.assertIn( + 'if ep_manuf_raw == "" then "Unknown"', endpoint_branch, filename + ) + # The select must emit the clamped values, never raw sysDescr. + self.assertIn("device_type: ep_model,", endpoint_branch, filename) + self.assertNotIn("device_type: sysDescr", endpoint_branch, filename) diff --git a/forward_netbox/utilities/bulk_merge.py b/forward_netbox/utilities/bulk_merge.py index 8ab4c67..242c48c 100644 --- a/forward_netbox/utilities/bulk_merge.py +++ b/forward_netbox/utilities/bulk_merge.py @@ -25,8 +25,11 @@ import logging from collections import defaultdict +from django.contrib.contenttypes.fields import GenericForeignKey +from django.contrib.contenttypes.models import ContentType from django.db import DEFAULT_DB_ALIAS from django.db import IntegrityError +from django.db import models from django.db import transaction from django.db.models import Q from mptt.models import MPTTModel @@ -144,6 +147,92 @@ def _defer_self_referential_create_fks(to_process, change_logger): ) +def _create_has_fk_to(collapsed, target_model_class, target_obj_id): + """True when ``collapsed``'s postchange data holds an FK to the target object. + + Local mirror of netbox_branching 1.1.0's ``SquashMergeStrategy._has_fk_to`` + (concrete FKs plus GenericForeignKeys), which 1.1.1 removed along with + ``_split_bidirectional_cycles``. + """ + if not collapsed.postchange_data: + return False + for field in collapsed.model_class._meta.get_fields(): + if not isinstance(field, models.ForeignKey): + continue + fk_value = collapsed.postchange_data.get(field.name) + if not fk_value: + continue + if field.related_model == target_model_class and fk_value == target_obj_id: + return True + target_ct_id = ContentType.objects.get_for_model(target_model_class).pk + for field in collapsed.model_class._meta.private_fields: + if not isinstance(field, GenericForeignKey): + continue + # ObjectChange data may store the CT FK as either the field name or + # its *_id column. + ct_value = collapsed.postchange_data.get( + field.ct_field + ) or collapsed.postchange_data.get(f"{field.ct_field}_id") + fk_value = collapsed.postchange_data.get(field.fk_field) + if ct_value == target_ct_id and fk_value == target_obj_id: + return True + return False + + +def _split_bidirectional_create_cycles(collapsed_changes, change_logger): + """Split CREATE pairs joined by bidirectional FKs (A -> B and B -> A). + + netbox_branching 1.1.0 shipped this as + ``SquashMergeStrategy._split_bidirectional_cycles``; 1.1.1 removed it in + favour of cycle breaking inside its own ordering pass — which this module + does not use (we run our own topological sort), so the 2-node pre-pass must + exist locally. Mirrors the 1.1.0 idiom: NULL the nullable FK on one CREATE + and append a synthetic UPDATE that restores it after both rows exist. + """ + creates = { + key: c + for key, c in collapsed_changes.items() + if c.final_action == ActionType.CREATE + } + for key_a, create_a in list(creates.items()): + if not create_a.postchange_data: + continue + for field in create_a.model_class._meta.get_fields(): + if not (isinstance(field, models.ForeignKey) and field.null): + continue + fk_value = create_a.postchange_data.get(field.name) + if not fk_value: + continue + related_model = field.related_model + target_ct = ContentType.objects.get_for_model(related_model) + app_label, model = target_ct.natural_key() + key_b = (f"{app_label}.{model}", fk_value) + create_b = creates.get(key_b) + if create_b is None: + continue + if not _create_has_fk_to(create_b, create_a.model_class, key_a[1]): + continue + change_logger.info( + " Detected bidirectional cycle: %s:%s <-> %s:%s (via %s)", + create_a.model_class.__name__, + key_a[1], + create_b.model_class.__name__, + key_b[1], + field.name, + ) + original_postchange = dict(create_a.postchange_data) + create_a.postchange_data[field.name] = None + update_key = (key_a[0], key_a[1], f"update_{field.name}") + update_collapsed = CollapsedChange(update_key, create_a.model_class) + update_collapsed.change_count = 1 + update_collapsed.final_action = ActionType.UPDATE + update_collapsed.prechange_data = dict(create_a.postchange_data) + update_collapsed.postchange_data = original_postchange + update_collapsed.last_change = create_a.last_change + collapsed_changes[update_key] = update_collapsed + break + + def _is_bulk_safe(model_class) -> bool: """A model may be bulk_create'd on merge only if it is not an MPTT tree. @@ -236,7 +325,15 @@ def _order_collapsed_changes_fast(collapsed_changes, change_logger, operation): # Build the FK dependency graph exactly as the framework does (populates each # CollapsedChange.depends_on). Cheap O(V * fields); reused verbatim. - SquashMergeStrategy._split_bidirectional_cycles(to_process, change_logger) + # netbox_branching 1.1.1 removed _split_bidirectional_cycles (its own + # ordering now breaks cycles inline, which our sort does not use) — prefer + # the framework helper when present, else the local mirror. + _split_cycles = getattr( + SquashMergeStrategy, + "_split_bidirectional_cycles", + _split_bidirectional_create_cycles, + ) + _split_cycles(to_process, change_logger) # Then split deferred self-referential FKs (primary_ip4/6, oob_ip, VC master) # out of CREATEs — these form 3-node cycles the framework's 2-node split misses # (device -> primary_ip -> assigned interface -> device). Must run before the @@ -311,7 +408,16 @@ def _order_collapsed_changes_fast(collapsed_changes, change_logger, operation): remaining = { k: set(v.depends_on) for k, v in to_process.items() if indeg.get(k, 0) > 0 } - SquashMergeStrategy._log_cycle_details(remaining, to_process, change_logger) + _log_cycles = getattr(SquashMergeStrategy, "_log_cycle_details", None) + if _log_cycles is not None: + _log_cycles(remaining, to_process, change_logger) + else: + for cycle_key in list(remaining)[:5]: + change_logger.error( + " Unresolvable dependency node: %s -> %s", + cycle_key, + sorted(remaining[cycle_key]), + ) raise Exception( f"Cycle detected in dependency graph. {len(remaining)} changes are " f"involved in circular dependencies and cannot be ordered." diff --git a/pyproject.toml b/pyproject.toml index d0143a9..1f23d5a 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -1,6 +1,6 @@ [tool.poetry] name = "forward-netbox" -version = "2.4.4" +version = "2.4.5" description = "NetBox plugin to sync Forward data into NetBox via built-in NQE queries" authors = ["Craig Johnson "] license = "MIT" @@ -30,7 +30,7 @@ Changelog = "https://github.com/forwardnetworks/forward-netbox/blob/main/CHANGEL [tool.poetry.dependencies] httpx = ">0.26,<0.29" -netboxlabs-netbox-branching = ">=1.1.0" +netboxlabs-netbox-branching = ">=1.1.0,<1.2.0" pyzipper = ">=0.3.6,<0.5.0" python = "^3.10"