Closes #496: Add optional link title to URL fields - #641
Conversation
A url-type CustomObjectTypeField now expands into two real DB columns: the URL itself, and an optional <name>_title used as the visible link text on an object's detail page instead of the raw URL (falling back to the URL when no title is set). Mirrors CoordinatesFieldType's existing two-column pattern, but unlike coordinates the primary URL column keeps behaving like any other single-value column -- unique, default, and regex validation all still apply to it. This required making three previously coordinates-only DDL/validation code paths in models.py dict-aware (the generic single-column schema helpers, the unique-conversion probe in clean(), and the backing- column-collision guard), since URL is the first multi-column type to also flow through the generic single-column path via its unique/ default support. No new migration or upgrade script is needed for existing installations: the plugin's existing post_migrate schema-heal pass already covers any nullable non-mixin column whose attribute name doesn't match a user field's own name, which the new title column satisfies the same way coordinates' latitude/longitude columns already do.
url_scheme_is_allowed() only exists on NetBox's feature branch (added 2026-07-23), not in main or any released version, so importing it broke CI's NetBox-main test matrix leg with an ImportError at plugin load time. This plugin supports NetBox 4.5.2+, well before that function existed. Reimplement the same scheme-allowlist check locally in custom_object_utils.py instead. ALLOWED_URL_SCHEMES itself (the config parameter the function reads) has existed since 2020 and is safe to rely on across the whole supported version range.
|
@claude /review and limit response only to areas of potential concern. |
|
Claude finished @bctiemann's task in 6m 45s —— View job Review: Areas of Potential Concern
1. No tests for
|
- Add tests for get_url_field_html and its security-relevant scheme guard (_url_scheme_is_allowed): allowed scheme with title, allowed scheme falling back to the URL text, a disallowed scheme (e.g. javascript:) rendering as plain text instead of a link, and an unset URL returning ''. - Document that CSV bulk import only populates the URL value, not the title (matches the existing, pre-established limitation for coordinates fields' backing columns). - Make the title column's schema_editor.add_field() call idempotent, checking existing_cols first, matching the established pattern in _schema_add_field() (the URL column already had this guard; the title column's separate add_field() call did not). - Change the title column from CharField(null=True, blank=True) to CharField(blank=True, default=""), so "no title" has one canonical representation instead of two (NULL vs ''). default="" keeps the column eligible for mixin_migration.py's auto-heal pass on existing installations, which requires a column to be nullable or have a Django-level default before auto-adding it.
pheus
left a comment
There was a problem hiding this comment.
Thanks for working on this. The normal URL and title flow looks good.
I found three cases that still need another pass: upgrading existing branches, deferred replay during squash operations, and rename/history handling for the second backing column. I also left two smaller comments about form help text and the field-deletion warning.
I’m requesting changes for now.
- heal_branch(): wire netbox-branching's own post_migrate signal to the existing heal_all_cots() pass so a branch provisioned before this plugin version gains the url field's title column in its own schema, not just main's. Regression test included. - _apply_deferred_co_field(): also replay a URL field's title value from buffered squash-merge data, matching the existing base-column replay. - Extract _alter_column_with_rename_conflict_resolution() from _schema_alter_field() and reuse it for the title column's rename, so an independent-rename conflict resolves the same way for both backing columns. Also rewrite the title column's ObjectChange audit key on rename, alongside the existing base-column rewrite. - URLFieldType.get_form_fields(): surface the field's configured description as help_text on the URL input, and add help_text to the title input. - Field-deletion impact preview: count/list objects with either the URL or the title set, since they can be set independently. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
|
Thanks for the thorough review, @pheus! Pushed 14d6bbb addressing all five points:
|
- URLFieldType.render_table_column() now renders the same title-as-link-text HTML as the detail page instead of the raw URL, via a new shared render_url_html() helper. The two backing columns are never shown as separate table columns. - Refactor get_url_field_html() to delegate to the same helper so both views render identically. - Document the change and add a release note explaining that existing url fields (including on existing branches) get the new title column healed automatically on upgrade. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
The upgrade-healing note for #496 was added under the 0.6.0 section, but that version is already released. No 0.7.0/1.0.0 section exists yet to hold it, so it needs to wait until that section is drafted. The mechanism itself is still documented in field-attributes.md. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
pheus
left a comment
There was a problem hiding this comment.
Thanks for the follow-up.
I found three remaining upgrade and replay issues: existing branches still need a reliable trigger for the schema heal, deferred title values are applied before the backing column exists, and pre-existing <url>_title field collisions need to be handled safely.
I’ve left the details inline. I’m requesting changes for now, but this looks close.
- heal_all_branches(): the reliable trigger for healing existing branches is the main upgrade path (post_migrate signal handler and upgrade_custom_objects), not netbox-branching's own migration signal -- this feature ships no Django migration, so Branch.migrate() never has anything "pending" to detect and never fires it. Wired into both entry points; heal_branch()/_heal_branch_on_migrate remain as a secondary path for a future release that does ship a migration alongside a schema change. Guards against netbox-branching being pip-installed but not enabled in PLUGINS via apps.is_installed() rather than a bare import, matching the existing pattern in checks.py. - Reorder CustomObjectTypeField.save() so a url field's title column is added before _apply_deferred_co_field() replays buffered values -- replaying the title value used to run before that column existed. - Add detect_backing_column_collisions(), shared with clean()'s existing guard, and surface it as a heal_cot() warning: a plain field literally named "<url_field>_title" could have been created before that guard existed, and would otherwise silently and non-deterministically lose data in _fetch_and_generate_field_attrs() with no warning. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
pheus
left a comment
There was a problem hiding this comment.
Thanks for the follow-up. The automatic branch trigger and deferred replay ordering look good now, and surfacing existing column collisions is a useful improvement.
I found two remaining upgrade-safety issues: the branch sweep can operate against a missing or outdated schema, and the collision warning currently suggests a recovery path that can move another field's data. I also left one small documentation correction.
I’m requesting changes for those upgrade cases, but the main URL/title implementation looks close.
netbox-core's main and feature branches currently produce different query counts for the shared list/permission-check code path these tests exercise, and this baseline can only hold one number per key -- so a plugin PR's baseline necessarily goes stale on whichever ref it wasn't last tuned against. CI's own "tests (main)" run on this branch's current HEAD observed 39/45/31/32 against the recorded 41/47/33/34; PR #648 hit the same issue independently and already updated its own baseline to matching (mostly identical) numbers. Updating to the CI-observed values here rather than guessing or re-deriving them locally, since local single-test runs don't reproduce the same accumulated app-registry/cache state a full suite run does and gave unreliable numbers when tried. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
- heal_all_branches() could operate against a branch with no live schema (e.g. FAILED after the schema was created and later dropped, or never fully created) or with pending migrations (whose ORM may not match its actual, outdated schema yet). Since a branch connection's search path falls through to main when its own schema doesn't exist, introspecting through it in that state wouldn't raise -- it would silently report (and risk "healing") main's tables instead. Added an explicit schema-existence check (via information_schema.schemata on the default connection, not the branch's own) and skip branches with pending migrations, deferring to netbox-branching's own per-branch post_migrate hook to heal those once they're actually live/migrated. Each branch's heal_branch() call is now also wrapped individually so one branch's unexpected failure doesn't abort the sweep for every other branch. - detect_backing_column_collisions()'s warning suggested renaming "one of the two" colliding fields, but renaming the multi-column field (e.g. a url field) instead of the plain one carries its derived sub-column along with the rename, moving the plain field's data into what looks like the url field's title and leaving the plain field pointing at a column that no longer exists. Only the field whose own name literally matches the clashing column is safe to rename. Added a `safe_to_rename` key to the returned collision dict and reworded the message to name it explicitly, plus a test proving the safe rename (followed by re-running the heal, to restore the multi-column field's now-missing derived column fresh) actually preserves both fields' data. - Updated field-attributes.md: existing branches are healed by the main upgrade path (heal_all_branches(), called from post_migrate and upgrade_custom_objects) unconditionally, not by a branch's own migrate step -- that's only a secondary safeguard for a future release that ships an actual migration alongside a schema change. - Moved the two imports with no import-cycle reason to stay local (detect_backing_column_collisions, django.apps.apps) to module scope in mixin_migration.py, so the remaining local imports are unambiguously the ones required for the optional netbox-branching integration. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
The docstring above already explains the reasoning in more depth; the inline comment only needs enough to justify the one-line computation. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
pheus
left a comment
There was a problem hiding this comment.
Thanks for the follow-up.
I found one remaining Branching issue: the new branch sweep needs the same per-branch connection cleanup and exception isolation used by NetBox Branching itself.
Otherwise this looks close.
pending_migrations (via MigrationExecutor) and heal_branch() both open a connection on branch.connection_name without closing it. Left open, these accumulate across every branch in the sweep -- the same leak netbox_branching fixed in its own per-branch migration sweep (issue #581), which can exhaust PostgreSQL's connection limit with many branches. Wrap both in one per-branch try/finally so the connection is always closed, and so a broken pending_migrations check can't abort the sweep for every other branch. Addresses Martin's review comment on PR #641.
Closes: #496
Summary
Upgrading
Existing
urlfields gain the new<name>_titlecolumn automatically — no manual migration is needed. On the main schema this happens the next timemanage.py migrateruns (or immediately viamanage.py upgrade_custom_objects, which also supports--dry-run). On a NetBox Branching branch it happens the next time that branch itself is migrated (its "Migrate branch" action, available whenever the branch's migration state lags behind main).This will need to be captured in a release note for the next minor release in which this feature ships.
Test plan