Skip to content

Closes #496: Add optional link title to URL fields - #641

Merged
pheus merged 12 commits into
featurefrom
496-url-field-link-title
Aug 14, 2026
Merged

Closes #496: Add optional link title to URL fields#641
pheus merged 12 commits into
featurefrom
496-url-field-link-title

Conversation

@bctiemann

@bctiemann bctiemann commented Aug 2, 2026

Copy link
Copy Markdown
Contributor

Closes: #496

Summary

  • A url-type CustomObjectTypeField now expands into two real DB columns: the URL itself, and an optional _title used as the visible link text on an object's detail page instead of the raw URL (falling back to the URL itself when no title is set). List/table views are unaffected -- still plain-text URL.
  • 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 exactly as before.
  • This required making three previously coordinates-only DDL/validation code paths in models.py dict-aware (the generic single-column schema helpers used by every field type, the unique-conversion probe in clean(), and the backing-column-collision guard), since URL is the first multi-column type that also needs to 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. Documented this explicitly in mixin_migration.py.
Screenshot 2026-08-06 at 6 12 51 AM Screenshot 2026-08-06 at 6 13 01 AM

Upgrading

Existing url fields gain the new <name>_title column automatically — no manual migration is needed. On the main schema this happens the next time manage.py migrate runs (or immediately via manage.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

  • New/extended tests across test_field_types.py (URLFieldTypeTestCase: model generation, title-optional, unique still enforced, type-conversion rejection both directions, backing-column collision both directions), test_schema_operations.py (rename and delete both drop/rename the title column), test_api.py (serializer exposes both columns flat, create round-trip with and without a title), test_views.py (add form renders both inputs, create with and without a title).
  • ruff check clean across the whole package.
  • Ran the full plugin test suite (1109 tests) against a NetBox 4.6.6 checkout. All new/changed tests pass. The only pre-existing failures (20, unrelated to this change) trace to an environment gap in that test setup -- netbox_branching is importable but not enabled in PLUGINS, which breaks any test touching CustomObjectTypeField/CustomObjectType rename or delete, confirmed by the identical failure occurring on the unmodified, pre-existing coordinates rename test -- plus one unrelated csv_update_data scaffolding gap in generic view tests.

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.
@bctiemann

Copy link
Copy Markdown
Contributor Author

@claude /review and limit response only to areas of potential concern.

@github-actions

github-actions Bot commented Aug 3, 2026

Copy link
Copy Markdown

Claude finished @bctiemann's task in 6m 45s —— View job


Review: Areas of Potential Concern

Limiting to concerns only, as requested.


1. No tests for get_url_field_html / _url_scheme_is_allowed — including the security-relevant scheme guard

_url_scheme_is_allowed is the only line of defense preventing a javascript: URL from rendering as a clickable link. There are no tests for this function or the get_url_field_html filter directly. The existing view tests only check HTTP 302 redirects after form submission; they don't render the detail page and verify the output HTML.

In particular, the following cases are untested:

  • A URL with a disallowed scheme (e.g., javascript:alert(1)) renders as plain text, not a link
  • A URL with a title renders <a href="...">Title</a>
  • A URL with no title falls back to the URL text
  • An object with no URL at all renders '' (the placeholder path)

This is a gap worth filling given that _url_scheme_is_allowed is a reimplemented local copy of a NetBox core function, and any divergence from NetBox's version (e.g., new scheme normalisation logic upstream) would be silent.

Fix this →%20URL%20with%20allowed%20scheme%20renders%20as%20%3Ca%20href%3D%22...%22%3E%3C%2Fa%3E%3B%202)%20javascript%3A%20URL%20renders%20as%20plain%20text%2C%20no%20link%3B%203)%20title%20is%20used%20as%20link%20text%20when%20set%3B%204)%20URL%20text%20used%20as%20fallback%20when%20title%20is%20absent.%20Also%20add%20unit%20tests%20for%20_url_scheme_is_allowed%20in%20test_field_types.py.&repo=netboxlabs/netbox-custom-objects)


2. CSV bulk import silently drops the _title column

CustomObjectBulkImportView (views.py:1452) builds its form by iterating fields and calling get_annotated_form_field(field, for_csv_import=True). URLFieldType doesn't override get_annotated_form_field, so it calls the single-field get_form_field, which returns only a LaxURLField for the URL column. The _title column is never added to the import form.

A user exporting a COT with titled URLs and re-importing the CSV will silently lose all link titles. There's no error and no documentation warning. This could be acceptable as an MVP limitation, but it should at minimum be documented in docs/field-attributes.md.


3. Title column add_field call is not idempotent (unlike the URL column)

In CustomObjectTypeField.save() (models.py:3544), when adding a new URL field:

_schema_add_field(self, model, schema_editor, schema_conn)   # idempotent — checks column exists first
_apply_deferred_co_field(self)
if self.type == CustomObjectFieldTypeChoices.TYPE_URL:
    ...
    schema_editor.add_field(model, title_field)               # NOT idempotent

_schema_add_field explicitly checks whether the column already exists before issuing ALTER TABLE. The bare schema_editor.add_field(model, title_field) call does not — it would raise DuplicateColumn if retried in the rare case where the URL column was added but the transaction for the title column was not yet committed when the process was interrupted. In practice both operations commit together, so the risk is low. However, it's worth applying the same idempotency guard (checking existing_cols before calling add_field) to match the established pattern in _schema_add_field.


4. CharField(null=True, blank=True) creates dual empty states

The title model field (field_types.py):

models.CharField(max_length=200, null=True, blank=True, ...)

Django's CharField with null=True means "no title" can be represented as either NULL (ORM create without passing title) or "" (form submission with empty field). The display logic handles both via title or url, but comparisons like website_title="" vs website_title__isnull=True will behave differently, and assertIsNone(obj.website_title) in the API test would silently pass for NULL but fail for "".

The conventional Django solution is to store NULL consistently for "not set" by overriding the model's clean() to coerce ""None. Or drop null=True and treat empty string as "no title" consistently. Neither is urgent, but the dual-state can cause subtle filtering bugs.


5. Minor: MULTI_COLUMN_TYPES extends type-conversion guard but URL → URL rename is not re-tested

The new guard in clean():

if (
    self.pk and not self._state.adding
    and self.type != self._original_type
    and (self.type in MULTI_COLUMN_TYPES or self._original_type in MULTI_COLUMN_TYPES)
):

The condition requires self.type != self._original_type, so a URL field being renamed (same type, different name) correctly skips this branch. But the test test_change_existing_field_to_url_rejected starts from a text field, not from another multi-column type. A conversion between two different multi-column types (e.g., hypothetically coordinates → url) would also hit this guard, but that path isn't tested. Not a current bug (only two multi-column types exist and they share the same guard), but worth a comment noting the intent for future field types.


- 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.
@bctiemann
bctiemann requested review from a team and pheus and removed request for a team August 3, 2026 20:18

@pheus pheus left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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.

Comment thread netbox_custom_objects/mixin_migration.py
Comment thread netbox_custom_objects/models.py Outdated
Comment thread netbox_custom_objects/models.py Outdated
Comment thread netbox_custom_objects/field_types.py
Comment thread docs/field-attributes.md
- 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>
@bctiemann

bctiemann commented Aug 6, 2026

Copy link
Copy Markdown
Contributor Author

Thanks for the thorough review, @pheus! Pushed 14d6bbb addressing all five points:

  1. Branch-upgrade heal gap — added heal_branch() in mixin_migration.py, wired to netbox-branching's own post_migrate signal (the one Branch.migrate() actually fires, since it never triggers Django's core post_migrate). This runs the existing heal_all_cots() pass against the branch's own connection inside activate_branch(), so a branch provisioned before this release now gets the _title column added to its own schema. Added a regression test (BranchUpgradeHealTestCase) that simulates a pre-upgrade branch by dropping the column from only the branch's schema and asserts heal_branch() restores it there without touching main's.

  2. Deferred replay dropping the title value_apply_deferred_co_field() now matches and applies the <name>_title key independently of the base URL key, since the two can be set independently. Added URL fields (with both URL and title values) to the shared test_comprehensive_merge_and_revert test, which runs under both the iterative and squash strategies, plus to MissingFieldTypesTestCase.

  3. Naive title-column rename + missing audit-key rewrite — extracted the conflict-aware rename logic already used by _schema_alter_field() into a shared _alter_column_with_rename_conflict_resolution() helper, and reused it for the title column, so an independent-rename conflict (branch renames A→B, main independently renames A→C) resolves the title column the same way it already resolves the primary column. Also added a second _rename_objectchange_field_key() call for the title key. Added two regression tests: a straightforward rename that replays/reverts both values, and a rename-conflict test mirroring the existing test_sequential_renames_both_sides_merge pattern but asserting the title column converges correctly too.

  4. Missing help_textURLFieldType.get_form_fields() now renders the field's configured description as help_text on the URL input, and the title input has its own help_text.

  5. Deletion-impact preview omitting title-only objects — both the count and the dependent-objects list in CustomObjectTypeFieldDeleteView now match on URL-set OR title-set, since either can hold a value independently.

bctiemann and others added 2 commits August 6, 2026 06:22
- 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>
@bctiemann
bctiemann requested a review from pheus August 6, 2026 11:35

@pheus pheus left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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.

Comment thread netbox_custom_objects/__init__.py
Comment thread netbox_custom_objects/models.py Outdated
Comment thread netbox_custom_objects/models.py
- 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>
@bctiemann
bctiemann requested a review from pheus August 6, 2026 18:27

@pheus pheus left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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.

Comment thread netbox_custom_objects/mixin_migration.py
Comment thread netbox_custom_objects/models.py Outdated
Comment thread docs/field-attributes.md Outdated
Comment thread netbox_custom_objects/mixin_migration.py Outdated
bctiemann and others added 3 commits August 11, 2026 21:05
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>
@bctiemann
bctiemann requested a review from pheus August 12, 2026 02:58

@pheus pheus left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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.

Comment thread netbox_custom_objects/mixin_migration.py Outdated
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.
@bctiemann
bctiemann requested a review from pheus August 13, 2026 22:08
@pheus
pheus merged commit b77245a into feature Aug 14, 2026
10 checks passed
@pheus
pheus deleted the 496-url-field-link-title branch August 14, 2026 08:12
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.

2 participants