Add _full_clean flag to baker.make(), prepare(), and bulk_create()#591
Add _full_clean flag to baker.make(), prepare(), and bulk_create()#591benaduo wants to merge 1 commit intomodel-bakers:mainfrom
_full_clean flag to baker.make(), prepare(), and bulk_create()#591Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds an optional Changes
Sequence Diagram(s)sequenceDiagram
participant Caller as Caller
participant Baker as Baker
participant Model as DjangoModel
participant DB as Database
Caller->>Baker: baker.make(..., _full_clean=True)
Baker->>Model: construct instance (apply fields, relations)
Baker->>Model: instance.full_clean()
alt validation passes
Baker->>DB: begin transaction (if _bulk_create)
Baker->>Model: instance.save()
Baker->>DB: commit (if _bulk_create)
Baker-->>Caller: return saved instance(s)
else validation fails
Baker->>DB: rollback (if _bulk_create)
Baker-->>Caller: raise ValidationError
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
model_bakery/baker.py (2)
464-518:⚠️ Potential issue | 🟠 MajorPropagate
_full_cleaninto autogenerated FK/O2O objects.
_full_cleanonly reachesself.instance(). The relational path still goes throughgenerate_value(), and that code only forwards_create_filesfor relation generators. Sobaker.make(models.User, profile__email="not-an-email", _full_clean=True)will still save an invalid relatedProfile, even though validation was explicitly requested.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@model_bakery/baker.py` around lines 464 - 518, The _full_clean flag is only passed to self.instance() but not into related-object creation: update calls to self.generate_value(field, commit_related) in bake/create paths to include and forward the _full_clean parameter (e.g., self.generate_value(field, commit_related, _full_clean=_full_clean)), update the generate_value(...) signature to accept _full_clean and ensure any code paths that create/autogenerate related OneToOne/ForeignKey objects forward that flag into their calls to instance(...) (and into any relation generators) so related models are validated when _full_clean=True.
950-979:⚠️ Potential issue | 🟠 MajorUse an alias-bound atomic block around the entire bulk-write phase.
When
baker._usingspecifies a non-default database,transaction.atomic()(without theusingparameter) applies only to the default database, not tobaker._using. Themanager.bulk_create()call operates onbaker._usingbut runs outside any transaction on that database when_full_clean=False, and in a transaction bound to the default database when_full_clean=True—leaving related objects inserted by_save_related_objs()(which runs outside the atomic block) unprotected. Usetransaction.atomic(using=baker._using)and wrap the entire bulk-write phase including_save_related_objs()to ensure consistency across multi-database setups.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@model_bakery/baker.py` around lines 950 - 979, The bulk_create function currently calls _save_related_objs and manager.bulk_create outside a database-scoped atomic when baker._using targets a non-default DB; wrap the entire bulk-write phase (the call to _save_related_objs and the manager.bulk_create call) in a transaction.atomic using the proper database alias: use transaction.atomic(using=baker._using) when baker._using is truthy, otherwise use a default transaction.atomic(), so the related-object inserts and the bulk_create are executed in the same atomic block tied to the correct DB.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@CHANGELOG.md`:
- Around line 12-16: Update the changelog wording to reference the user-facing
call shape using the _bulk_create flag (e.g., baker.make(..., _bulk_create=True)
and baker.prepare(..., _bulk_create=True)) instead of pointing readers to the
internal baker.bulk_create(model, ...) helper; reword the bullet to mention the
_bulk_create=True API and keep the note about atomic transaction behavior when
_full_clean=True validation fails.
---
Outside diff comments:
In `@model_bakery/baker.py`:
- Around line 464-518: The _full_clean flag is only passed to self.instance()
but not into related-object creation: update calls to self.generate_value(field,
commit_related) in bake/create paths to include and forward the _full_clean
parameter (e.g., self.generate_value(field, commit_related,
_full_clean=_full_clean)), update the generate_value(...) signature to accept
_full_clean and ensure any code paths that create/autogenerate related
OneToOne/ForeignKey objects forward that flag into their calls to instance(...)
(and into any relation generators) so related models are validated when
_full_clean=True.
- Around line 950-979: The bulk_create function currently calls
_save_related_objs and manager.bulk_create outside a database-scoped atomic when
baker._using targets a non-default DB; wrap the entire bulk-write phase (the
call to _save_related_objs and the manager.bulk_create call) in a
transaction.atomic using the proper database alias: use
transaction.atomic(using=baker._using) when baker._using is truthy, otherwise
use a default transaction.atomic(), so the related-object inserts and the
bulk_create are executed in the same atomic block tied to the correct DB.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 1ef3653d-f67b-4f4f-88d3-8dad01a7be62
📒 Files selected for processing (4)
CHANGELOG.mddocs/basic_usage.mdmodel_bakery/baker.pytests/test_baker.py
CHANGELOG.md
Outdated
| - Add `_full_clean` flag to `baker.make()`, `baker.prepare()`, and `baker.bulk_create()` to run Django model validation (`False` by default) ([#523](https://github.com/model-bakers/model_bakery/issues/523)) | ||
|
|
||
| ### Changed | ||
|
|
||
| - Bulk creation now uses transaction blocks to ensure atomic rollback when `_full_clean=True` validation fails |
There was a problem hiding this comment.
Use the _bulk_create=True call shape here.
The user-facing API is baker.make(..., _bulk_create=True), not baker.bulk_create(model, ...). As written, this changelog entry points readers at an internal helper with a different signature.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@CHANGELOG.md` around lines 12 - 16, Update the changelog wording to reference
the user-facing call shape using the _bulk_create flag (e.g., baker.make(...,
_bulk_create=True) and baker.prepare(..., _bulk_create=True)) instead of
pointing readers to the internal baker.bulk_create(model, ...) helper; reword
the bullet to mention the _bulk_create=True API and keep the note about atomic
transaction behavior when _full_clean=True validation fails.
benaduo
force-pushed
the
fix/full-clean-flag
branch
3 times, most recently
from
551811b to
c6e5c36
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@model_bakery/baker.py`:
- Around line 151-157: The bug is that _save_kwargs is being forwarded into the
prepare/bulk-prepare path where Baker.prepare() doesn't expect it, causing it to
be treated as a model attribute and breaking _bulk_create; fix by stripping
_save_kwargs (and similarly _full_clean if intended only for save) out of the
kwargs before calling baker.prepare() or bulk_create() so only real model attrs
are forwarded. Concretely, in the caller that invokes
baker.prepare()/bulk_create() (the code that calls bulk_create(baker, ...) and
the code path that forwards kwargs into Baker.prepare()), remove any key named
"_save_kwargs" from the dict passed to prepare()/bulk-prepare (e.g.,
pop("_save_kwargs", None)) and ensure _save_kwargs is only consumed by the
save/bulk create code paths that actually use it (such as _bulk_create or the
save logic).
- Around line 986-990: The current atomic block uses the default DB connection
and only wraps manager.bulk_create when _full_clean is true, leaving
_save_related_objs and writes on non-default DBs unprotected; change the flow so
that both _save_related_objs(...) and manager.bulk_create(entries) are executed
inside a single transaction.atomic(using=baker._using) context (regardless of
_full_clean) so that writes use the correct database alias and related FK rows
are rolled back together on failure.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 2dca0793-5be8-432f-9068-30650f4a7188
📒 Files selected for processing (4)
CHANGELOG.mddocs/basic_usage.mdmodel_bakery/baker.pytests/test_baker.py
✅ Files skipped from review due to trivial changes (2)
- docs/basic_usage.md
- tests/test_baker.py
🚧 Files skipped from review as they are similar to previous changes (1)
- CHANGELOG.md
| result = bulk_create( | ||
| baker, | ||
| _quantity or 1, | ||
| _save_kwargs=_save_kwargs, | ||
| _full_clean=_full_clean, | ||
| **attrs, | ||
| ) |
There was a problem hiding this comment.
Don't send _save_kwargs down the bulk-prepare path.
Line 154 always passes _save_kwargs, and Lines 971-974 forward kwargs unchanged into baker.prepare(). Baker.prepare() doesn't consume _save_kwargs, so it falls through into model attrs and breaks _bulk_create before full_clean() or bulk_create() run.
Minimal fix
- result = bulk_create(
- baker,
- _quantity or 1,
- _save_kwargs=_save_kwargs,
- _full_clean=_full_clean,
- **attrs,
- )
+ result = bulk_create(
+ baker,
+ _quantity or 1,
+ _full_clean=_full_clean,
+ **attrs,
+ )Also applies to: 959-974
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@model_bakery/baker.py` around lines 151 - 157, The bug is that _save_kwargs
is being forwarded into the prepare/bulk-prepare path where Baker.prepare()
doesn't expect it, causing it to be treated as a model attribute and breaking
_bulk_create; fix by stripping _save_kwargs (and similarly _full_clean if
intended only for save) out of the kwargs before calling baker.prepare() or
bulk_create() so only real model attrs are forwarded. Concretely, in the caller
that invokes baker.prepare()/bulk_create() (the code that calls
bulk_create(baker, ...) and the code path that forwards kwargs into
Baker.prepare()), remove any key named "_save_kwargs" from the dict passed to
prepare()/bulk-prepare (e.g., pop("_save_kwargs", None)) and ensure _save_kwargs
is only consumed by the save/bulk create code paths that actually use it (such
as _bulk_create or the save logic).
| if _full_clean: | ||
| with transaction.atomic(): | ||
| created_entries = manager.bulk_create(entries) | ||
| else: | ||
| created_entries = manager.bulk_create(entries) |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
In Django, which database connection does transaction.atomic()use whenusingis omitted, and how should it be paired withManager.using(...) in multi-database code?
💡 Result:
When using is omitted in transaction.atomic, it uses the "default" database connection. This is explicitly stated in the official Django documentation across multiple versions. In multi-database code, transaction.atomic only manages transactions on the specified (or default) database connection—it does not affect other databases. Queries executed via Manager.using('alias') (e.g., Model.objects.using('other_db').create(...)) will use the specified database alias regardless of the surrounding atomic block. The transaction management for those queries depends on whether an atomic block using that same alias is active. To ensure atomicity across operations on a specific non-default database: - Use with transaction.atomic(using='alias'): - Then perform queries with Model.objects.using('alias')... Example: from django.db import transaction with transaction.atomic(using='other_db'): obj1 = MyModel.objects.using('other_db').create(...) obj2 = MyModel.objects.using('other_db').update(...) This opens a transaction on 'other_db', executes the queries there, and commits/rolls back based on success. Note: True distributed (2-phase commit) transactions across multiple databases are not supported by Django; each database has independent transactions. Use separate nested atomic(using=...) blocks for operations spanning databases, but be aware failures in one won't automatically rollback others.
Citations:
- 1: https://docs.djangoproject.com/en/6.0/topics/db/transactions/
- 2: https://docs.djangoproject.com/en/4.2/topics/db/transactions
- 3: https://docs.djangoproject.com/en/4.2/topics/db/multi-db/
🏁 Script executed:
cat -n model_bakery/baker.py | sed -n '970,995p'Repository: model-bakers/model_bakery
Length of output: 1007
🏁 Script executed:
# Check if manager.db is used elsewhere in the codebase
rg "manager\.db" model_bakery/
# Also check what attributes are available on Django managers for database selection
rg "\.using\(" model_bakery/baker.py | head -20Repository: model-bakers/model_bakery
Length of output: 132
🏁 Script executed:
# Search for Django manager database attributes in documentation or code
rg "\.db\b" model_bakery/ | head -10
# Check what variables are available in this context
cat -n model_bakery/baker.py | sed -n '975,992p'
# Look for other atomic block usages with database specification
rg "transaction\.atomic\(using" model_bakery/Repository: model-bakers/model_bakery
Length of output: 1389
🏁 Script executed:
# Find _save_related_objs function definition
rg "def _save_related_objs" model_bakery/
# Look at the function to understand what it does
rg -A 20 "def _save_related_objs" model_bakery/baker.py | head -30Repository: model-bakers/model_bakery
Length of output: 889
🏁 Script executed:
# Check if Django's QuerySet/Manager has a db attribute or method to get the database
python3 << 'EOF'
# Check Django's QuerySet and Manager API for database tracking
import inspect
# Django QuerySet typically stores the database in _db attribute
print("Looking at common QuerySet attributes related to database:")
print("- _db (private attribute)")
print("- db (property, if it exists)")
print("- get_db() or similar methods")
# The using() method returns a new QuerySet with _db set
# Let's verify the common patterns
EOF
# Also look for how baker._using is defined and used
rg "_using\s*=" model_bakery/baker.py | head -20Repository: model-bakers/model_bakery
Length of output: 931
🏁 Script executed:
# Check if Django QuerySet has a _db or db attribute
python3 << 'EOF'
# Simulate Django QuerySet behavior
# A QuerySet created with .using(alias) has _db set to that alias
# So manager.using(baker._using) creates a QuerySet with _db=baker._using
# The manager itself doesn't have .db, but the queryset from manager.using() would have _db
# However, we need the database alias passed to transaction.atomic(using=...)
# Let's check if the code accesses _db anywhere
EOF
# Search for _db usage in baker.py to see if it's ever accessed
rg "\._db\b" model_bakery/baker.pyRepository: model-bakers/model_bakery
Length of output: 51
🏁 Script executed:
# Look at the Baker class to understand _using better
rg -B 5 -A 10 "class Baker" model_bakery/baker.py | head -40
# Check if there's any code that tries to access manager.db or similar
rg "manager\." model_bakery/baker.py | grep -v "manager\.bulk_create\|manager\.using"Repository: model-bakers/model_bakery
Length of output: 780
Wrap writes in atomic block with correct database alias.
Line 987 opens transaction.atomic() on the default connection, so manager.using(baker._using) writes are not protected on non-default databases. Line 978 is also outside the block, meaning related FK rows can be committed even if bulk_create() later fails. Wrap both _save_related_objs() and bulk_create() in transaction.atomic(using=baker._using).
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@model_bakery/baker.py` around lines 986 - 990, The current atomic block uses
the default DB connection and only wraps manager.bulk_create when _full_clean is
true, leaving _save_related_objs and writes on non-default DBs unprotected;
change the flow so that both _save_related_objs(...) and
manager.bulk_create(entries) are executed inside a single
transaction.atomic(using=baker._using) context (regardless of _full_clean) so
that writes use the correct database alias and related FK rows are rolled back
together on failure.
There was a problem hiding this comment.
🧹 Nitpick comments (1)
tests/test_baker.py (1)
1569-1579:bulk_createrollback test does not currently exercise rollback during insert.At Line 1574,
email="not-an-email"fails inprepare()(full_clean) beforebulk_createis reached, so this verifies early validation, not transactional rollback of a failed bulk insert.Consider either renaming the test to reflect “validate-before-bulk-create” or adding a separate test that fails at DB insert time (e.g., unique constraint collision in a bulk batch) and then asserts zero rows persisted.
♻️ Suggested test adjustment
class TestFullClean: @@ - def test_bulk_create_with_full_clean_rolls_back_on_error(self): - """_full_clean=True with _bulk_create=True rolls back all entries on error.""" + def test_bulk_create_with_full_clean_validates_before_bulk_insert(self): + """_full_clean=True with _bulk_create=True validates entries before bulk_create.""" with pytest.raises(ValidationError): baker.make( models.Profile, email="not-an-email", _quantity=5, _bulk_create=True, _full_clean=True, ) assert models.Profile.objects.count() == 0 + + `@pytest.mark.django_db` + def test_bulk_create_with_full_clean_rolls_back_on_db_error(self): + """DB errors during bulk_create should roll back atomically.""" + baker.make(models.DummyUniqueIntegerFieldModel, value=1) + with pytest.raises(Exception): # tighten to IntegrityError if imported in this file + baker.make( + models.DummyUniqueIntegerFieldModel, + value=iter([2, 1, 3]), # includes duplicate with existing row + _quantity=3, + _bulk_create=True, + _full_clean=True, + ) + # Only the pre-existing row should remain. + assert models.DummyUniqueIntegerFieldModel.objects.count() == 1
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: d93c00a7-dec5-45e7-88da-2199390dd60d
📒 Files selected for processing (4)
CHANGELOG.mddocs/basic_usage.mdmodel_bakery/baker.pytests/test_baker.py
✅ Files skipped from review due to trivial changes (2)
- docs/basic_usage.md
- model_bakery/baker.py
🚧 Files skipped from review as they are similar to previous changes (1)
- CHANGELOG.md
benaduo
force-pushed
the
fix/full-clean-flag
branch
from
c6e5c36 to
df33303
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@model_bakery/baker.py`:
- Around line 161-165: The code is unconditionally forwarding the _full_clean
keyword to custom baker methods and breaks bakers with strict signatures; update
all call sites that invoke baker.make(...) and baker.prepare(...) (references:
baker.make, baker.prepare, and the _save_kwargs/_refresh_after_create
parameters) to only include _full_clean in the kwargs when bool(_full_clean) is
True (i.e., guard by truthiness), and apply this same guard to each of the four
sites mentioned so that _full_clean is omitted for falsey values and preserves
backward compatibility with custom bakers.
In `@tests/test_baker.py`:
- Around line 1568-1579: The test never reaches the new atomic bulk-create
branch because the invalid email causes baker.prepare(...) to raise before the
_bulk_create path; add a new happy-path test using baker.make with valid data
and _bulk_create=True and _full_clean=True (for example create 5 valid models
using baker.make(models.Profile, email="[email protected]", _quantity=5,
_bulk_create=True, _full_clean=True) and assert
models.Profile.objects.count()==5) and also add a second case using an FK-backed
model (use baker.make on the parent, then baker.make(child_model,
parent=parent_instance, _quantity=3, _bulk_create=True, _full_clean=True) and
assert child count and FK links) so the atomic branch in model_bakery.baker (the
_bulk_create/_full_clean transaction path reached via baker.make) is actually
executed.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 7034ad23-6ae6-4372-8e6e-74b49994ca46
📒 Files selected for processing (4)
CHANGELOG.mddocs/basic_usage.mdmodel_bakery/baker.pytests/test_baker.py
✅ Files skipped from review due to trivial changes (1)
- docs/basic_usage.md
🚧 Files skipped from review as they are similar to previous changes (1)
- CHANGELOG.md
| baker.make( | ||
| _save_kwargs=_save_kwargs, | ||
| _refresh_after_create=_refresh_after_create, | ||
| _full_clean=_full_clean, | ||
| **attrs, |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cd /tmp && find . -type f -name "baker.py" 2>/dev/null | head -5Repository: model-bakers/model_bakery
Length of output: 51
🏁 Script executed:
git ls-files | grep -E "baker\.py$"Repository: model-bakers/model_bakery
Length of output: 110
🏁 Script executed:
wc -l model_bakery/baker.pyRepository: model-bakers/model_bakery
Length of output: 95
🏁 Script executed:
sed -n '155,180p' model_bakery/baker.pyRepository: model-bakers/model_bakery
Length of output: 699
🏁 Script executed:
sed -n '215,235p' model_bakery/baker.pyRepository: model-bakers/model_bakery
Length of output: 753
🏁 Script executed:
sed -n '965,980p' model_bakery/baker.pyRepository: model-bakers/model_bakery
Length of output: 650
🏁 Script executed:
rg "class Baker" model_bakery/baker.py -A 5Repository: model-bakers/model_bakery
Length of output: 185
🏁 Script executed:
sed -n '1,100p' model_bakery/baker.py | grep -A 20 "def make"Repository: model-bakers/model_bakery
Length of output: 283
🏁 Script executed:
rg "def make\(" model_bakery/baker.py -A 10 | head -50Repository: model-bakers/model_bakery
Length of output: 1446
🏁 Script executed:
rg "def prepare\(" model_bakery/baker.py -A 10 | head -50Repository: model-bakers/model_bakery
Length of output: 1141
🏁 Script executed:
rg "BAKER_CUSTOM_CLASS" model_bakery/ -B 3 -A 3Repository: model-bakers/model_bakery
Length of output: 1134
🏁 Script executed:
rg "BAKER_CUSTOM_CLASS" tests/ -B 2 -A 8Repository: model-bakers/model_bakery
Length of output: 1374
🏁 Script executed:
find . -name "*.md" -o -name "*.rst" | xargs grep -l "BAKER_CUSTOM_CLASS" 2>/dev/nullRepository: model-bakers/model_bakery
Length of output: 97
🏁 Script executed:
rg "class.*Baker" tests/ -A 10 | head -80Repository: model-bakers/model_bakery
Length of output: 5037
🏁 Script executed:
rg "def make\|def prepare" tests/ -B 2 -A 5 | grep -A 5 "class" | head -50Repository: model-bakers/model_bakery
Length of output: 51
🏁 Script executed:
sed -n '1,50p' tests/test_extending_bakery.pyRepository: model-bakers/model_bakery
Length of output: 1369
🏁 Script executed:
rg "ClassWithoutMake|ClassWithoutPrepare" tests/test_extending_bakery.py -B 3 -A 5Repository: model-bakers/model_bakery
Length of output: 673
🏁 Script executed:
git log --oneline -- model_bakery/baker.py | head -20Repository: model-bakers/model_bakery
Length of output: 143
🏁 Script executed:
git log --oneline -S "_full_clean" -- model_bakery/baker.py | head -10Repository: model-bakers/model_bakery
Length of output: 143
🏁 Script executed:
git show df33303 --statRepository: model-bakers/model_bakery
Length of output: 4558
🏁 Script executed:
git show df33303 | head -200Repository: model-bakers/model_bakery
Length of output: 5683
🏁 Script executed:
rg "_full_clean" model_bakery/baker.py | wc -lRepository: model-bakers/model_bakery
Length of output: 71
🏁 Script executed:
sed -n '160,176p' model_bakery/baker.pyRepository: model-bakers/model_bakery
Length of output: 510
🏁 Script executed:
sed -n '155,180p' model_bakery/baker.py | cat -nRepository: model-bakers/model_bakery
Length of output: 881
🏁 Script executed:
sed -n '215,235p' model_bakery/baker.py | cat -nRepository: model-bakers/model_bakery
Length of output: 900
🏁 Script executed:
sed -n '965,985p' model_bakery/baker.py | cat -nRepository: model-bakers/model_bakery
Length of output: 925
🏁 Script executed:
rg "def make" model_bakery/baker.py -A 25 | grep -A 25 "def make.*self"Repository: model-bakers/model_bakery
Length of output: 51
🏁 Script executed:
rg "^\s+def make\(" model_bakery/baker.py -A 20Repository: model-bakers/model_bakery
Length of output: 815
🏁 Script executed:
rg "^\s+def prepare\(" model_bakery/baker.py -A 20Repository: model-bakers/model_bakery
Length of output: 731
Guard _full_clean behind truthiness to maintain backward compatibility with custom bakers.
Custom bakers need only implement make() and prepare() methods with no required parameters beyond self. The code unconditionally passes _full_clean to these methods at four call sites (lines 161–165, 170–174, 222–226, 971–974), which breaks existing custom bakers with strict signatures. Only pass _full_clean when it is truthy to preserve compatibility:
Compatibility-safe forwarding pattern
- return baker.prepare(_save_related=_save_related, _full_clean=_full_clean, **attrs)
+ prepare_kwargs = {"_save_related": _save_related, **attrs}
+ if _full_clean:
+ prepare_kwargs["_full_clean"] = True
+ return baker.prepare(**prepare_kwargs)Apply the same guard to all four _full_clean forwarding sites.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@model_bakery/baker.py` around lines 161 - 165, The code is unconditionally
forwarding the _full_clean keyword to custom baker methods and breaks bakers
with strict signatures; update all call sites that invoke baker.make(...) and
baker.prepare(...) (references: baker.make, baker.prepare, and the
_save_kwargs/_refresh_after_create parameters) to only include _full_clean in
the kwargs when bool(_full_clean) is True (i.e., guard by truthiness), and apply
this same guard to each of the four sites mentioned so that _full_clean is
omitted for falsey values and preserves backward compatibility with custom
bakers.
|
Hi @amureki, just wanted to flag a few things for your review:
Happy to make any changes. Thanks for your time. |
benaduo
force-pushed
the
fix/full-clean-flag
branch
from
df33303 to
a4d2134
Compare
Fixes model-bakers#523. Adds a _full_clean=False opt-in parameter across all main creation functions. When True, calls Django's full_clean() on each instance before saving. For _bulk_create=True, validation runs at prepare() time and the bulk_create call is wrapped in transaction.atomic() to ensure atomic rollback if validation fails. Based on the approach in model-bakers#547 (thanks @berinhard), incorporating feedback from Rust's review of that PR.
benaduo
force-pushed
the
fix/full-clean-flag
branch
from
a4d2134 to
d70a2b5
Compare
|
Hi @amureki, this PR is ready for review. All CI checks are passing. Quick summary of what's in here:
Happy to make any changes based on your feedback. |
1 participant
Fixes #523
Adds
_full_clean=Falsetobaker.make(),baker.prepare(), andbaker.bulk_create(). WhenTrue, calls Django'sfull_clean()on each instance before saving, enabling model validation during test object creation.For
_bulk_create=True, validation runs atprepare()time and thebulk_createcall is wrapped intransaction.atomic()to ensure atomic rollback if any validation fails.Fresh implementation based on the approach in #547 (thanks @berinhard), incorporating feedback from Rust's review of that PR.
Changes
model_bakery/baker.py—_full_cleanparam threaded through allmake()/prepare()overloads and implementations down toBaker.instance();bulk_create()wrapped intransaction.atomic()when flag is settests/test_baker.py— newTestFullCleanclass with 5 tests covering valid creation, invalid raises, no DB entry on error, prepare path, and bulk_create rollbackdocs/basic_usage.md— new "Running Model Validation" sectionCHANGELOG.md— entries under Added and ChangedSummary by CodeRabbit
New Features
Documentation
Tests