Repository navigation
Fix django_get_or_create for factories built through SubFactory - #1161
Open
inchang-ing wants to merge 1 commit into
Open
inchang-ing wants to merge 1 commit into
inchang-ing wants to merge 1 commit into
Conversation
django_get_or_create relies on DjangoModelFactory._original_params to recover the user-provided lookup when the initial creation hits an IntegrityError. That attribute was only recorded by Factory._generate, which is bypassed when the factory is instantiated through a SubFactory, resulting in an IntegrityError (or an AttributeError before the guard added in FactoryBoy#981) instead of reusing the existing object. Record the params in StepBuilder.build instead: it covers both direct calls and SubFactory recursion, and the params received there contain only user-provided values (call-site overrides and declaration-level params), not values generated by declarations — preserving the distinction required for the get_or_create fallback lookup. Fixes FactoryBoy#979
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #979
Problem
django_get_or_createworks when the factory is called directly (or used throughBaseFactory), but not when it is reached through aSubFactory: on a unique-constraint collision, instead of reusing the existing object, the call fails with anIntegrityError(anAttributeErroron_original_paramsin 3.2.1, before the guard added in #981).Root cause
DjangoModelFactory._get_or_createrecovers the user-provided lookup fromcls._original_paramswhen the initialget_or_createhits anIntegrityError. That attribute is only recorded byDjangoModelFactory._generate, which is on the direct-call path (Factory.create()→_generate()→StepBuilder.build()). TheSubFactorypath enters atStepBuilder.build()viaBuildStep.recurse()and never calls_generate, so_original_paramsstaysNoneand the fallback cannot run.Approach (and the lesson from #981)
#981 added the failing testcase and a guard that re-raises the
IntegrityErrorwhen_original_paramsisNone. In the discussion on #979, @francoisfreitag explained why initializing_original_paramsto an empty dict (the alternative proposed there) was not acceptable:_original_paramsmust distinguish user-provided values (usable as aget()lookup) from values generated by the factory (usable only increate()), and no place in the library preserved that distinction for the SubFactory path.That distinction is actually available: in
StepBuilder,extrascontains exactly the user-provided params for the factory being built — call-site overrides (pair_factory(base_currency__name="Bitcoin")) and declaration-level params (SubFactory(CurrencyFactory, name="Bitcoin")) — and never generated values (those only appear after declaration resolution in the step attributes). Moreover, for a direct call,extrasis the very same dict that_generatereceives asparams.So this PR records the params in
StepBuilder.build():This covers both paths consistently (the direct-call recording in
DjangoModelFactory._generatebecomes redundant but is left untouched), applies the same fix toSQLAlchemyModelFactoryfor free, and keeps the #981 behavior intact: when the user params do not cover thedjango_get_or_createfields, theIntegrityErroris still re-raised (verified by the testcase from #981, which still passes).Testcase
Two regression tests mirroring the issue report were added to
tests/test_django.py(SubFactoryGetOrCreateTests), one for declaration-level params (SubFactory(CurrencyFactory, name="Bitcoin")style) and one for call-time nested params (multifield__slug=...style). Both fail withIntegrityErrorbefore the fix and pass after it.Full test suite: 441 tests, all passing (439 before, +2 new).