Fix: Attachments on Custom Objects — load order and removed-setting warning (#110)#111
Open
Kani999 wants to merge 9 commits into
Open
Fix: Attachments on Custom Objects — load order and removed-setting warning (#110)#111Kani999 wants to merge 9 commits into
Kani999 wants to merge 9 commits into
Conversation
added 5 commits
July 17, 2026 10:12
- Add a Django system check for `apps`, `allowed_models` and `mode`, which were replaced by `scope_filter` and `applied_scope` in v7.1.0 (607d5e1). - NetBox's PluginConfig.validate() only fills in *missing* keys, so a stale setting raises nothing: it sits unread in PLUGINS_CONFIG while its replacement silently falls back to a default. That default covers dcim, ipam and four more apps, so part of the config keeps working and the rest does not -- which reads as a plugin bug rather than a config one (#110). - Hints name the replacement and spell out that neither is a plain rename: `apps` and `allowed_models` were merged into one list, and `mode` changed its values as well as its name. - Register via a new ready() on the PluginConfig, mirroring the idiom in netbox_custom_objects/checks.py.
- Pull the "netbox_custom_objects.<type name>" identifier out into a reusable helper. Custom object models are named after their backing table's primary key (table134model), which is opaque and differs between installs, so settings key off the Custom Object Type's name instead. - Only validate_object_type resolved that identifier, inline; the display path needs the same key and had been deriving its own. One helper gives the two a single definition to share. - Use Django's model._meta.label_lower for the standard-model identifier. NetBox keys its template-extension registry on label_lower, so building that string by hand invites drift from the value it must match. - No behaviour change: both identifiers resolve exactly as before.
Attachments never appeared on Custom Object detail pages when
netbox_attachments was listed before netbox_custom_objects in PLUGINS.
template_extensions is built at module import, i.e. inside this plugin's
ready(). netbox_custom_objects withholds its dynamically generated models
until its own ready() has finished (`if not _app_ready: return` in its
AppConfig.get_models()), and Django runs ready() in PLUGINS order -- so
discovery found zero models and registered no panel at all, whatever
scope_filter said. A correct config was not enough to work around it.
- Replace discover_custom_object_models() and the import-time enumeration
with one globally registered extension (models = None) that resolves the
object at render time, when the models reliably exist.
- Skip custom objects in the startup loop: when netbox_custom_objects loads
first its models *are* in apps.get_models(), and a per-model extension
would then render a second panel beside the global one.
- Register the panel before the try block; it needs no models and no
database, so it should not be lost to an OperationalError below.
- Key display_setting by custom_object_identifier(), so the identifier
documented for scope_filter also drives per-type display overrides. It
previously keyed off table{pk}model, so the documented key never matched.
Resolved only when an override exists, since it costs a query.
- Guard objects without _meta: NetBox offers global extensions whatever is
in context, and it need not be a model instance.
Side effects: PLUGINS ordering no longer matters, this plugin no longer
touches the database during app loading, and Custom Object Types created
after startup get a panel without restarting NetBox.
- Add a "Removed settings" table mapping `apps`, `allowed_models` and `mode` to their replacements, and note the new startup warning. - Replace the terse "Custom Objects Limitation" note with a section that explains what does and does not work, and why. The Custom Objects detail template hardcodes its tab bar and its button row and exposes only the left/right/full-width panel hooks, so `additional_tab` and `create_add_button` cannot work there -- both are properties of that template, not of this plugin, and need an upstream change to lift. - Document that display_setting keys custom objects by type name, matching scope_filter, and that this had no effect before 11.3.0. - Bump to 11.3.0.
- `make format` reformatted views.py, tables.py and two migrations, which had drifted from ruff's style; the package is now format-clean, so the command no longer produces unrelated diffs alongside real work. - Reformatting views.py broke test_views_py_assignment_list_prefetches_tags, which asserted on the literal source text of the queryset chain. Joining `.prefetch_related(` onto the previous line changed nothing about what the code does, but the substring stopped matching -- so `make format` and `make test` could not both pass. - Parse the chain with ast instead, as the default_columns test in the same module already does, and assert on the calls and their arguments. The test still fails if select_related/prefetch_related lose their guarantee; it no longer fails on where the formatter breaks a line.
Contributor
|
Important Review skippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches✨ Simplify code
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
netbox_attachments/checks.py (1)
12-12: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueAvoid shadowing the Python builtin
Warning.As highlighted by static analysis, importing
Warningdirectly shadows the Python built-in exception. Consider aliasing it to prevent linter warnings and avoid potential confusion.♻️ Proposed fix to alias the import
-from django.core.checks import Warning, register +from django.core.checks import Warning as CheckWarning, registerEnsure that you also update the instantiation on line 49 from
Warning(toCheckWarning(.🤖 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_attachments/checks.py` at line 12, Alias the django.core.checks Warning import as CheckWarning in checks.py, then update the warning instantiation in the checks registration logic from Warning( to CheckWarning(.Source: Linters/SAST tools
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@netbox_attachments/checks.py`:
- Line 12: Alias the django.core.checks Warning import as CheckWarning in
checks.py, then update the warning instantiation in the checks registration
logic from Warning( to CheckWarning(.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: e0f49648-a074-4776-b138-5b2cc5cb91ce
📒 Files selected for processing (15)
CHANGELOG.mddocs/configuration.mdnetbox_attachments/__init__.pynetbox_attachments/checks.pynetbox_attachments/migrations/0012_netboxattachment_owner.pynetbox_attachments/migrations/0013_alter_netboxattachmentassignment_custom_field_data.pynetbox_attachments/tables.pynetbox_attachments/template_content.pynetbox_attachments/tests/test_checks.pynetbox_attachments/tests/test_configuration.pynetbox_attachments/tests/test_new_features.pynetbox_attachments/tests/test_template_extensions.pynetbox_attachments/utils.pynetbox_attachments/version.pynetbox_attachments/views.py
added 4 commits
July 17, 2026 12:57
- Check IDs were derived from REMOVED_SETTINGS dict order via enumerate(), so editing the table would silently renumber W001-W003 -- IDs users pin in SILENCED_SYSTEM_CHECKS and the CHANGELOG advertises. Each setting now carries its ID explicitly; a regression test asserts a mode-only config still yields W003, which the positional scheme got wrong. - The message claimed the replacement setting "is being used ... with its default value" even when a half-migrated config sets both keys, telling the user their configured scope_filter was not in effect when it was. The wording now reflects whether the replacement is configured. - The identical merged-settings hint for 'apps' and 'allowed_models' is one _MERGED_HINT constant instead of two literals to keep in sync. - The hint's docs link is pinned to this release's tag instead of the main branch, so it describes the settings the warned install actually has and survives a docs reorganization.
- render_custom_object_panel checked hasattr(obj, "_meta") but everything downstream reads type(obj)._meta. A model CLASS in context passes the old guard (classes have _meta) and then crashes on the metaclass -- ModelBase has no _meta -- which NetBox surfaces as an exception box on the page. Reproduced with obj=Device (the class); core templates pass instances, but the extension is global, so any third-party template could hit it. - Checking the type also covers None and non-model values in one condition.
- The scope check (validate_object_type under applied_scope="model") and the display-key resolution could each issue the same CustomObjectType query during one custom object page render, and the render path runs per template hook -- up to four identical queries per page in a model-scoped config with display_setting overrides. - Cached with lru_cache keyed on the dynamic model class: netbox_custom_objects regenerates that class whenever the type is saved, so a rename produces a new class object and therefore a fresh cache entry. A queryset .update() bypasses the regeneration signal and would serve a stale name until restart -- the same tradeoff the CO plugin's own model cache makes. - Only custom object classes enter the cache (the public wrapper filters first), so the install's standard models cannot churn the LRU.
- Add resolve_display_preference_for_model(): the single model -> effective display position answer. The startup loop and the render-time panel both call it, so display-key selection (type name vs label_lower), the query-avoidance optimization, and the additional_tab -> full_width_page fallback for custom objects have exactly one definition. Previously the key dance lived only in the render path, and the loop dropped the is_custom_object flag entirely -- leaving the documented no-tab/no-button guarantee resting on loop statement order alone. A test now pins the fallback at the resolver layer; consolidation of the two registration mechanisms is tracked in issue #112 and referenced at the guard. - Remove get_display_preference(): zero production callers, and for custom objects it answered with the wrong fallback under the wrong key. Its two tests now exercise resolve_effective_display_preference directly. - Extract _render_or_empty() for the try/render/log/return-'' tail that existed in three drifting copies (panel, button, custom object panel). - Finish the label_lower sweep this branch started: the dedup key, the registry-facing app_model_name, and both keys in get_enabled_object_type_queryset now use model._meta.label_lower instead of hand-rolled f-strings. - Move the _meta fake to a shared tests/conftest.py factory; adding label_lower earlier in this branch required synchronized edits to two per-file copies, which is the failure mode this removes.
Owner
Author
Post-review remediationA 10-angle review of this branch surfaced 13 findings (no merge blockers). Four follow-up commits address them:
The two-mechanism consolidation the review flagged at altitude is deliberately not done here — filed as #112 and referenced from the guard comment it will eventually delete. Verified live after the changes: panel renders exactly once on a custom object page, 🤖 Generated with Claude Code |
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.
Fixes #110.
The reporter saw no Attachments UI on Custom Object detail pages, and no custom object types in the object-type picker. There turned out to be two independent causes, either of which alone produces that report — so fixing only the obvious one would have left them broken.
Cause 1 — a removed setting, silently ignored
Their config used
apps, which was merged intoscope_filterin v7.1.0 (607d5e1, Feb 2025). They are on 11.2.3. NetBox'sPluginConfig.validate()only fills in missing keys, soappssat unread inPLUGINS_CONFIGwhilescope_filterfell back to its default — which coversdcimbut notnetbox_custom_objects. Their DCIM attachments kept working, which made the config look honoured and pointed suspicion at custom objects.Measured with their exact config:
apps: [...]scope_filter: [...]validate_object_type(<custom object>)FalseTrueThat alone explains the missing picker entries.
Cause 2 — a load-order bug (breaks even a correct config)
template_extensionsis built at module import, i.e. inside this plugin'sready().netbox_custom_objectswithholds its dynamically generated models until its ownready()has finished:Django runs
ready()inPLUGINSorder, so whenevernetbox_attachmentsis listed first, discovery found zero models and registered no panel at all — whateverscope_filtersaid. Reproduced on a box with a correct config:It also meant object types created after startup never got a panel until a restart.
Fix: stop enumerating custom objects at startup. They are now served by a single globally registered extension (
models = None) that resolves the object at render time, when the models reliably exist. NetBox supports this directly —_get_registered_content()renders key-Noneextensions for every object alongside model-specific ones.What's in here
Feat:apps/allowed_models/modeRefactor:custom_object_identifier(); use Django'slabel_lowerFix:Docs:Chore:ruff formatthe package; de-brittle one testBeyond the two causes, review found that
display_settingnever worked for custom objects:scope_filterkeys off the type name (netbox_custom_objects.cotab2_asset) while the display path keyed off the internal, PK-derived model name (netbox_custom_objects.table134model) — one object, two identifiers. Pre-existing, but this PR's docs newly recommenddisplay_settingfor custom objects, so it's fixed here rather than left as a false promise.Deliberately not done:
additional_taband the top buttonThe reporter asked for the gray Attachments button. That isn't reachable, and not because of this plugin. The released Custom Objects
customobject.htmloverrides{% block tabs %}(hardcoding Primary/Journal/Changelog) and overrides{% block controls %}without{% plugin_buttons %}. It renders only:So no plugin can add a tab or a top button there. The existing
additional_tab→full_width_pagefallback is therefore correct and is kept; the docs now explain why instead of just stating it. Lifting this needs an upstream change in netbox-custom-objects. ({% plugin_extra_tabs %}exists only on unreleased feature branches.)Verification
Unit tests stub their collaborators, so the fix was also exercised against live NetBox 4.6.4 + Custom Objects 0.6.0 (96 dynamic types):
/plugins/custom-objects/cotab2-asset/1/), exactly once, with working Add/Link controls — previously nothing rendered.applied_scope: "model"scoping renders for the in-scope type and stays hidden for an out-of-scope one.netbox_custom_objectsloading first): zero per-model extensions bound to them, exactly one global panel.dcimkeeps its 49 extensions and its Attachments tab; no stray panel on device pages.manage.py checkclean on a correct config; firesW001on the reporter's.Notes for review
make formatwas booby-trapped. It reformattedviews.py/tables.py/two migrations that had drifted, which broketest_views_py_assignment_list_prefetches_tags— it asserted on the literal source text of a queryset chain.make formatandmake testcould not both pass. The package is now format-clean and that test parses the chain withast(as thedefault_columnstest in the same module already does) instead of matching formatting.is_custom_object_model → continueguard exists only to stop them colliding, and thedisplay_settingbug was a symptom of them diverging. Letting the global extension serve all panel-mode models, keeping startup registration only foradditional_tab(which genuinely needsregister_model_viewbefore the URLconf freezes), would delete that whole risk class.netbox_custom_objectsinscope_filterunderapplied_scope: "app", the plugin's own metadata models (CustomObjectType,CustomObjectTypeField,CustomObjectObjectType) are also in scope, soCustomObjectTypegets an Attachments tab. Attaching a spec doc to a type definition may be legitimate; attaching to a field definition looks like a mistake. Worth a deliberate decision, out of scope here.🤖 Generated with Claude Code
Summary by CodeRabbit