[arthanson]

Fixes: #705

Migration 0006 failed for open branches on installs that had already applied NetBox's extras.0131. By then the column in extras_tag_object_types is named contenttype_id, so the indexes copied into the branch schema no longer match the hard-coded names the migration tries to rename.

This updates 0006 in place; no new migration is added.

• Build the copied index names from whichever column the copied table has (objecttype_id or contenttype_id); the target names are unchanged, since 0131 renames the column but not main's indexes
• Rename the copied column back to objecttype_id when the branch hasn't applied extras.0131, so the branch's own migrate step can apply it later
• Add tests in test_upgrade.py for branches created before and after 0131, including applying 0131 afterward, checking that the copied indexes and rows match main

[arthanson]

@claude /review Only report on issues of potential concern; we don't need commentary on anything that doesn't require further consideration.

[github-actions]

Claude finished @arthanson's task in 3m 55s —— View job

---

Review

• Read changed files
• Analyze migration logic
• Analyze test coverage
• Post review findings

---

Migration (0006_tag_object_types.py)

Index lookup inconsistency with 0003_rename_indexes

0003 matches by LIKE: AND indexdef LIKE %s with f'% {definition}'. 0006 uses split_part / starts_with. Both are semantically equivalent for well-formed pg_indexes entries, but diverging from the established pattern makes the two harder to reason about together. Not a bug, but worth aligning for future maintainers.

Unquoted identifiers in DDL statements (0006_tag_object_types.py:58, :72)

cursor.execute(f"ALTER INDEX {schema_name}.{index.indexname} RENAME TO {result[0]}")
cursor.execute(f"ALTER TABLE {schema_table} RENAME COLUMN contenttype_id TO objecttype_id")

schema_name is derived from plugin config + a UUID-based schema ID, and index.indexname / result[0] come from pg_indexes. Practically low-risk, but neither schema_name nor the index names are wrapped in psycopg2.sql.Identifier / psycopg2.sql.SQL. The existing 0003 migration has the same pattern, so this isn't new, but it's worth noting if there's ever a path for schema_prefix to contain special characters.

schema_name queried with an f-string on line 26–28

The table-existence check at the top of the loop uses an f-string rather than parameterized SQL, while the column-existence check below correctly uses %s. Minor inconsistency that pre-dates this PR but could be tidied up.

---

Tests (test_upgrade.py)

test_branch_predating_0131_can_apply_0131 has no skip guard for environments without extras.0131 (test_upgrade.py:338–356)

migration = loader.get_migration(*self.MIGRATION)

If this test ever runs against a NetBox version that doesn't include extras.0131 (whether during local development or a future NetBox version), get_migration raises KeyError with an opaque error. Consider:

loader = MigrationLoader(connection)
if self.MIGRATION not in loader.migrated_apps.get(self.MIGRATION[0], {}):
    self.skipTest(f"{self.MIGRATION} not present in this NetBox version")

Fix this →

test_branch_predating_0131_can_apply_0131 hardcodes operation selection (test_upgrade.py:346)

operation = next(op for op in migration.operations if op.model_name == 'tag' and op.name == 'object_types')

If NetBox ever restructures extras.0131, this raises StopIteration. Wrapping it in a try/except StopIteration with a self.skipTest(...) or self.fail(...) would make failures more debuggable.

_create_branch leaves orphaned connection aliases open

_create_branch creates a Branch ORM record (rolled back by TestCase's transaction) and a PostgreSQL schema (rolled back by transactional DDL). No branch connection is ever opened, so this is fine — but unlike the FastTeardownTransactionTestCase-based tests, there's no tearDown closing branch connections. If any path in copy_table or the subsequent operation.database_forwards call opens a branch connection, it would leak. Worth a quick check or a defensive tearDown.

test_copy_table_for_branch_predating_0131 branch count check (test_upgrade.py:333–336)

cursor.execute(f'SELECT COUNT(*) FROM "{schema}".{self.TABLE}')
branch_count = cursor.fetchone()[0]
cursor.execute(f'SELECT COUNT(*) FROM public.{self.TABLE}')
self.assertEqual(branch_count, cursor.fetchone()[0])

This is correct but only verifies row count parity. If the migration were to silently truncate the copied table but still write the same number of rows, this would pass. A stronger assertion (e.g., checksum or spot-checking a specific row) isn't strictly necessary here, but it's worth noting the check's limits.

[arthanson]

Per Claude review:

1. Index lookup differs from 0003 (LIKE): Not fixed. LIKE treats _ as a wildcard and doesn't tell unique indexes from non-unique ones, so the exact match is safer.
2. Unquoted identifiers in DDL: Not fixed. This pattern predates the PR and is used across the plugin; the schema name comes from plugin config and the index names from pg_indexes.
3. f-string in the table-existence check: Fixed. The query now passes the schema and table names as parameters.
4. No skip guard for extras.0131: Not fixed. That migration shipped in NetBox 4.4.0 and the plugin requires 4.7 or later, and a skip would hide a real break.
5. StopIteration from the hardcoded operation lookup: Not fixed. The test error already points at that exact line.
6. _create_branch leaks connections: Not valid. The tests only use the default connection, and the search_path change is undone in a finally.
7. Row-count check is weak: Fixed. The test now seeds a tag (the table was empty before) and compares the branch's rows exactly with main's in both copy tests.