Repository navigation
Closes #23426: Add a plugin API for allocating primary keys - #23443
jeremystretch wants to merge 9 commits into
Conversation
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…has_default() Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Allocators now receive the alias of the database being written to, as a third argument (`using`), and allocate_pks() accepts it too, defaulting to the router's choice for writing the model. An allocator no longer has to guess from its own context where an object is going. bulk_create() now requests all of the primary keys it needs in a single call. Allocation for it moves from NetBoxAutoField.get_pk_value_on_save(), which is called once per object and never learns the target database, to a wrapper around QuerySet._prepare_for_bulk_create(), which has both. Allocators are now consulted for any model with an auto field primary key. Previously only NetBoxAutoField keys qualified, so data migrations (historical models carry a plain BigAutoField) and models of other apps were never covered. That makes NetBoxAutoField unnecessary, so it is removed, along with the DEFAULT_AUTO_FIELD change, Tag.id's use of it and its strawberry-django type mappings. Both patches now live in utilities.pk_allocation and are installed at import time in core/apps.py, like custom_deconstruct(). Registering an allocator which is already registered now has no effect.
A failed save() or bulk_create() left the allocated primary key on each object, so saving it again attempted an UPDATE of whichever row had that key. bulk_create(ignore_conflicts=True) likewise left a key on objects whose rows were skipped. Restore such objects to the state Django would leave them in, and consult allocators during bulk_create() only once the objects have been validated, so that a failed validation consumes no keys. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Cover multi-table inheritance and proxy models, and verify that each replacement's signature matches the stock Django method it replaces, so that a Django upgrade which changes one fails loudly. Pass arguments to the stock _save_table() by keyword, and skip allocation entirely when no allocators are registered. Document the bulk_create() conflict options which bypass the guarantee against overwriting existing rows. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…ls to insert A primary key allocated for the parent of a multi-table inheritance child was withdrawn only if the parent's own INSERT failed. If the child's INSERT failed instead, the parent's row was rolled back but the object kept the allocated key and the parent links set from it, so saving it again attempted an UPDATE of the stale key. Carry the values to restore from the parent's _save_table() call to the child's, and restore them if any INSERT in the chain fails. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
The bulk_create() wrapper copied every object without a primary key and recorded its state, even when every allocator declined. Instead, the _prepare_for_bulk_create() wrapper now records the state of the objects to which it allocates primary keys, on the queryset, for bulk_create() to restore or discard. Consolidate the check for whether a model's primary keys may be allocated, previously repeated in each wrapper. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…on any exception allocate_pks() passed allocators whatever model its caller supplied, so code inserting rows outside the ORM could present a proxy or multi-table inheritance child, or a model without an auto field primary key. It now follows parent links to the model which owns the primary key column and returns None unless that is an auto field, as the ORM wrappers already ensured. Withdraw allocated primary keys on any BaseException rather than only on Exception, and allocate within the try block, so that an allocator failing after a parent's insert still restores the parent's values. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
@pheus this functionality is a bit esoteric; it's being driven by a need surfaced in netbox-branching specifically. Happy to walk through it with you if you'd prefer. |
pheus
left a comment
There was a problem hiding this comment.
Thanks! I have one routing fix to request, plus a couple of suggestions to trim the repeated comments and docstrings.
| model = pk.model | ||
| if using is None: | ||
| using = router.db_for_write(model) |
There was a problem hiding this comment.
Could we resolve using from the original model first? A proxy or inherited child can route differently from its parent, so the allocator may receive the wrong database and incorrectly decline.
| Imitate the behavior of the stock QuerySet._prepare_for_bulk_create() method, but then consult any registered | ||
| primary key allocators for the objects without a primary key, requesting all of their primary keys in a single | ||
| call. Objects which receive one are inserted along with any whose primary key was already set. Allocators are | ||
| consulted only once the objects have been validated, so that a failed validation doesn't consume primary keys. | ||
| The prior state of each object which receives one is recorded on the queryset, for bulk_create_with_pk_allocation() | ||
| to restore. |
There was a problem hiding this comment.
Could we shorten the three wrapper docstrings? The allocator contract is already covered in the plugin guide. Keeping the explanations for forced inserts and restoring allocated keys would make these easier to follow.
| Imitate the behavior of the stock QuerySet._prepare_for_bulk_create() method, but then consult any registered | |
| primary key allocators for the objects without a primary key, requesting all of their primary keys in a single | |
| call. Objects which receive one are inserted along with any whose primary key was already set. Allocators are | |
| consulted only once the objects have been validated, so that a failed validation doesn't consume primary keys. | |
| The prior state of each object which receives one is recorded on the queryset, for bulk_create_with_pk_allocation() | |
| to restore. | |
| """ | |
| Allocate missing primary keys in one batch after Django prepares the objects. | |
| Record their prior state on the queryset for bulk_create_with_pk_allocation() to restore. | |
| """ |
| manufacturer = Manufacturer(name='Manufacturer 1', slug='manufacturer-1') | ||
| manufacturer.save() | ||
|
|
||
| # The allocator is consulted only once |
There was a problem hiding this comment.
Could we trim comments that repeat the test names or assertions throughout this module? This one restates the assertion below it. The notes explaining the collision and inheritance setup are useful.
| """ | ||
| Imitate the behavior of the stock Model._save_table() method, but first consult any registered primary key | ||
| allocators for a new object without a primary key. If an allocator supplies one, insert the object directly: | ||
| Django would otherwise attempt an UPDATE using the allocated primary key first, overwriting any existing row which | ||
| shares it. Objects saved with a primary key already set, and forced updates (which require one), are unaffected. | ||
|
|
||
| If the INSERT fails, or that of a multi-table inheritance child following it, the allocated primary key is withdrawn | ||
| from the object, so that saving it again consults the allocators again, rather than attempting an UPDATE of | ||
| whichever row has the allocated primary key. | ||
|
|
||
| Allocators receive the model which owns the primary key column (the concrete parent, for a proxy or multi-table | ||
| inheritance child), and the alias of the database being written to. | ||
| """ |
There was a problem hiding this comment.
| """ | |
| Imitate the behavior of the stock Model._save_table() method, but first consult any registered primary key | |
| allocators for a new object without a primary key. If an allocator supplies one, insert the object directly: | |
| Django would otherwise attempt an UPDATE using the allocated primary key first, overwriting any existing row which | |
| shares it. Objects saved with a primary key already set, and forced updates (which require one), are unaffected. | |
| If the INSERT fails, or that of a multi-table inheritance child following it, the allocated primary key is withdrawn | |
| from the object, so that saving it again consults the allocators again, rather than attempting an UPDATE of | |
| whichever row has the allocated primary key. | |
| Allocators receive the model which owns the primary key column (the concrete parent, for a proxy or multi-table | |
| inheritance child), and the alias of the database being written to. | |
| """ | |
| """ | |
| Force an INSERT when allocating a primary key to avoid overwriting an existing row. | |
| Restore the original keys and parent links if this table's INSERT or a child INSERT raises. | |
| """ |
| """ | ||
| Wrap the stock QuerySet.bulk_create() method to withdraw allocated primary keys from objects which might not have | ||
| been inserted, leaving them as bulk_create() would without an allocator: | ||
|
|
||
| * If the operation fails, so that saving the objects again consults the allocators again, rather than attempting | ||
| an UPDATE of whichever rows have the allocated primary keys. | ||
| * If conflicting rows are ignored, since there's no telling which objects were inserted. (bulk_create() never | ||
| sets primary keys in this case.) | ||
| """ |
There was a problem hiding this comment.
| """ | |
| Wrap the stock QuerySet.bulk_create() method to withdraw allocated primary keys from objects which might not have | |
| been inserted, leaving them as bulk_create() would without an allocator: | |
| * If the operation fails, so that saving the objects again consults the allocators again, rather than attempting | |
| an UPDATE of whichever rows have the allocated primary keys. | |
| * If conflicting rows are ignored, since there's no telling which objects were inserted. (bulk_create() never | |
| sets primary keys in this case.) | |
| """ | |
| """ | |
| Withdraw allocated keys and restore object state on failure or when conflicts are ignored. | |
| Ignored conflicts prevent us from identifying which objects were inserted. | |
| """ |
Closes: #23426
This adds a plugin hook for supplying the primary key of a new object before it is inserted. The motivating case is a clone-based netbox-branching backend, which needs every object written to a branch database to draw its ID from main's sequences. Each allocator is told which database the object is being written to. When no allocator is registered, or every allocator returns
None, nothing changes: PostgreSQL assigns the ID from the table's sequence as it does today. Objects saved with a primary key already set behave exactly as before, whether or not an allocator is registered.Changes
register_pk_allocator()andallocate_pks()(netbox/utils.py), backed by a newpk_allocatorsregistry store.(model, count, using), whereusingis the alias of the database the new objects will be written to. Each returns a list ofcountkeys, orNoneto defer to the next.allocate_pks(model, count=1, using=None)is public, so code that inserts with raw SQL orCOPYcan reserve IDs the same way the ORM does. Ifusingis omitted, it assumes the router's choice for writing the model.allocate_pks()itself works out which model the allocators receive: the one that owns the primary key column, following multi-table inheritance parent links. It returnsNonewithout consulting them unless that column is an auto field. So raw-SQL callers and the ORM wrappers present allocators with the same model.save_table_with_pk_allocation()(utilities/pk_allocation.py) wraps Django'sModel._save_table().using. If one supplies a key, it sets the key and passesforce_insert=True.UPDATE ... WHERE id=<allocated>before theINSERT. That costs a wasted query, and if the key were already in use it would silently overwrite the existing row instead of raisingIntegrityError.INSERTfails for any reason, including an interruption such asKeyboardInterrupt, the allocated key is withdrawn from the object, so saving it again consults the allocators again instead of attempting anUPDATEof the allocated key. Under multi-table inheritance, a failedINSERTof the child also withdraws the key allocated for its parent (whose row is rolled back), along with the parent links Django copied it into.force_update/update_fields), pass straight through to Django.prepare_for_bulk_create_with_pk_allocation()(utilities/pk_allocation.py) wrapsQuerySet._prepare_for_bulk_create().add()andset()create through rows withbulk_create(), so they're covered too.bulk_create_with_pk_allocation()(utilities/pk_allocation.py) wrapsQuerySet.bulk_create(), only to withdraw allocated keys from objects which might not have been inserted.save().ignore_conflicts=True, there's no telling which rows were inserted, so the keys are withdrawn from every object. This matches Django, which doesn't set primary keys in this mode._prepare_for_bulk_create()wrapper records their prior state on the queryset, so abulk_create()for which every allocator declines does no extra work.ContentType, and the historical models used byRunPythondata migrations. A proxy model's allocator call receives the concrete model, and a multi-table inheritance child's receives the parent. No model fields change, so there are no migrations and no changes to the REST API or GraphQL schema.core/apps.pyinstalls all three wrappers at import time, next to the existingcustom_deconstruct()patch.bulk_create()'signore_conflictsandupdate_conflictsoptions change thatsave()requests one key at a time, whilebulk_create()requests all of its keys at onceallocate_pks()Departures from the proposal in the issue
No
NetBoxAutoField. The issue proposed aBigAutoFieldsubclass, set asDEFAULT_AUTO_FIELD, whoseget_pk_value_on_save()consults the allocators. That hook has three limits, so this wraps_save_table()and_prepare_for_bulk_create()instead:bulk_create()calls it once per object.BigAutoField, and models of apps that set their owndefault_auto_field.Dropping the subclass also removes the
DEFAULT_AUTO_FIELDchange, theTag.idchange, the strawberry-django type map registration, and the plugin guidance about which field to use.Allocators are told the target database. The issue's signature was
(model, count), leaving allocators to infer the database from their own context. That context doesn't always answer the question. In netbox-branching, for example, an active branch doesn't mean a row is going to the branch: change records and many-to-many rows follow their parent there, while some branch bookkeeping is written to main. The wrappers already knowusing, so they pass it on.No
has_default()/get_default()overrides. The issue proposed makinghas_default()returnTrueso allocated keys skip theUPDATE. Buthas_default()belongs to the field and can't see the instance being saved, so it changes behavior for every save. Saving a never-saved instance whose key matches an existing row (Site(pk=5, ...).save(), including withupdate_fields) would raiseIntegrityErrorinstead of updating the row, on every install. The_save_table()wrapper forces theINSERTonly when an allocator supplied the key, which resolves the first open question in the issue.The allocator receives the model that owns the primary key column, not
instance._meta.model. With multi-table inheritance that is the parent model, and its table and sequence are where the insert goes. For a proxy, it's the concrete model.allocate_pks()does this resolution itself, so a raw-SQL caller passingObjectTypeorUserTokenpresents allocators withContentTypeorToken, just assave()does.allocate_pks()checks the allocator's output. It converts the result to a list, and raisesValueErrorif an allocator returns a different number of keys than requested. A short list would otherwise silently drop rows for raw-SQL callers that zip keys with rows.Allocated keys are withdrawn from objects which weren't inserted. The issue didn't cover failures. Without this, an object whose
INSERTfailed would keep its allocated key, and saving it again would skip the allocators and attempt anUPDATEof that key.Open questions from the issue
has_default(): neither option. See the third departure above.bulk_create(): yes. Wrapping_prepare_for_bulk_create()requests a whole batch in one call. Because the wrappers are installed on Django'sQuerySetrather than as overrides on NetBox's querysets, they also cover auto-created through models, which an override would have missed.Risks
Model._save_table(),QuerySet._prepare_for_bulk_create()and the publicQuerySet.bulk_create()are replaced at startup. A Django upgrade could change the private methods' signatures or behavior without notice. A test checks that each replacement's signature matches the stock method's, and the tests assert exactly whichINSERTandUPDATEstatements each case issues, and how often allocators are called, so such a change would fail CI rather than slip through. NetBox already patches Django at startup (core/apps.pyreplacesmodels.Field.deconstruct()).bulk_create()wrapper, such as aQuerySetsubclass that overridesbulk_create()without callingsuper(), still receives allocated keys, since they're assigned in_prepare_for_bulk_create(). Those keys aren't withdrawn if its insert fails._save_table()wrapper passes the values to restore from a multi-table inheritance parent'sINSERTto the child's on the instance (_unallocated_pks). The bulk wrappers pass them from_prepare_for_bulk_create()tobulk_create()on the queryset. Both are removed beforesave()orbulk_create()returns._save_table()call (one per table a save writes) and eachbulk_create()adds a few attribute and dictionary lookups. With an allocator registered, every new object, orbulk_create()batch, also pays a call toallocate_pks(), which loops over the registered allocators.Verification
New tests in
netbox/netbox/tests/test_pk_allocators.py(31 tests) cover:using: passed explicitly, defaulted from the router, and taken from the queryset rather than the routerallocate_pks()given a multi-table inheritance child (ObjectType), a proxy (UserToken) and a UUID-keyed model (CachedValue): allocators receiveContentTypeandToken, and aren't consulted for the UUID-keyed modelsave()with no allocator, a matching allocator, and a declining one: oneINSERT, noUPDATE, and the allocator consulted onceINSERT, noUPDATEIntegrityErrorand leaves the existing row untouched. Saving the object again obtains a new key and issues oneINSERT, noUPDATE.force_update/update_fieldson an object with no key: the allocator isn't consulted, and Django'sValueErroris raised as usualbulk_create(): one allocator call and oneINSERTfor the batch, including when some objects already have keys, and when the allocator declinesbulk_create()withdrawing keys after a failed insert and withignore_conflicts=True, consuming no keys when validation fails, and leaving the objects of an earlier call on the same queryset aloneINSERTinterrupted byKeyboardInterrupt, via bothsave()andbulk_create(), which still withdraws the allocated keysadd()ContentType)ObjectType): the allocator receives the parent, and a failed childINSERTwithdraws the parent's key and parent link, so the retry issues oneINSERTper tableUserToken): the allocator receives the concrete modelsave()andbulk_create()Removing
force_insert = Truefrom the_save_table()wrapper fails five of these tests. Not installing the_prepare_for_bulk_create()wrapper fails ten, and not installing thebulk_create()wrapper fails three.No existing tests changed.
Full test suite:
manage.py test --parallel 16ran 14,539 tests (22 skipped), using a copy ofconfiguration_testing.pywith its own test database name. The only failure wasLoadLdapConfigTest.test_configuration_dir_setting_matches_active_configuration, which fails under any configuration module other than the stock one. It passes under the stockconfiguration_testing.py.Pre-commit hooks pass:
ruff,manage.py check,makemigrations --check, andzensical build.