Skip to content

#532 - move integer BigIntegerField#600

Merged
bctiemann merged 7 commits into
featurefrom
532-integer
Jul 2, 2026
Merged

#532 - move integer BigIntegerField#600
bctiemann merged 7 commits into
featurefrom
532-integer

Conversation

@arthanson

Copy link
Copy Markdown
Contributor

Fixes: #532

Switched IntegerField to BigIntegerField for int fields and supporting migration. I didn't see a reason to keep the IntegerField, NetBox uses BitIntegerField for pretty much everything now.

@netboxlabs netboxlabs deleted a comment from github-actions Bot Jun 29, 2026
@arthanson

Copy link
Copy Markdown
Contributor Author

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

@github-actions

github-actions Bot commented Jun 29, 2026

Copy link
Copy Markdown

Claude finished @arthanson's task in 1m 56s —— View job


PR Review: #532 - move integer BigIntegerField

Two items worth considering:


1. Unquoted identifier in ALTER TABLE SQL (migrations/0015_widen_integer_columns.py:52)

cursor.execute(
    f'ALTER TABLE "{table_name}" ALTER COLUMN "{column_name}" TYPE bigint'
)

table_name is derived from an integer PK (safe), but column_name = field.name is user-supplied data interpolated directly via f-string. If a field name somehow contained a " character, the double-quote escaping would break. The information_schema check above it (lines 40–49) correctly uses parameterised %s for both values — the ALTER should be consistent. Use schema_editor.quote_name() for both identifiers:

cursor.execute(
    f'ALTER TABLE {schema_editor.quote_name(table_name)} '
    f'ALTER COLUMN {schema_editor.quote_name(column_name)} TYPE bigint'
)

Fix this →


2. Test passes current app registry instead of historical registry (tests/test_field_types.py:265)

from django.apps import apps
...
migration.widen_integer_columns(apps, schema_editor)

Django's RunPython normally passes the historical app registry (representing schema state at migration time), not django.apps.apps. This works here because widen_integer_columns only reads CustomObjectTypeField rows and doesn't depend on historical field structure — but it's a subtle inconsistency. If the function signature ever evolves to use historical model state (e.g. to introspect field definitions at migration time), this test would silently test the wrong thing. Worth adding a comment explaining why the current registry is intentionally used, or using MigrationLoader to get the historical state.


@arthanson

Copy link
Copy Markdown
Contributor Author

Claude issues resolved.

@arthanson arthanson marked this pull request as ready for review June 29, 2026 21:14
@arthanson arthanson requested review from a team and bctiemann and removed request for a team June 29, 2026 21:14

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

LGTM, but I feel like this should be into feature for a couple of reasons -- 1) it's technically an expected-behavior change, and 2) it has a migration which will clash with the 0015 that just got merged into feature and will need to be renumbered at some point anyway.

@arthanson arthanson changed the base branch from main to feature July 2, 2026 18:58
@arthanson arthanson requested a review from bctiemann July 2, 2026 19:17
@bctiemann bctiemann merged commit 87edad4 into feature Jul 2, 2026
7 checks passed
@bctiemann bctiemann added this to the v0.6.0 milestone Jul 2, 2026
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.

Field type for 64-bit integer

2 participants