-
-
Notifications
You must be signed in to change notification settings - Fork 89
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix bulk create not working with multi-database setup #252
Fix bulk create not working with multi-database setup #252
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks to me like a sensible way to handle this case. Thanks for the contribution!
if baker._using: | ||
manager = baker.model._base_manager.using(baker._using) | ||
else: | ||
manager = baker.model._base_manager |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Add newline between conditional blocks and return statement (for clarity).
tests/test_baker.py
Outdated
@@ -807,6 +817,38 @@ def test_related_fk_database_speficied_via_using_kwarg_combined_with_quantity(se | |||
assert dog in dog_qs | |||
assert dog.owner in person_qs | |||
|
|||
def test_related_fk_database_specified_via_using_kwarg_combined_with_and_bulk_create( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Extra unneeded "and
":
def test_related_fk_database_specified_via_using_kwarg_combined_with_and_bulk_create( | |
def test_related_fk_database_specified_via_using_kwarg_combined_with_bulk_create( |
"""Allow all relations.""" | ||
return True | ||
|
||
with override_settings(DATABASE_ROUTERS=[AllowRelationRouter()]): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great test setup. I like the simplicity of using override_settings
as a context manager here.
return baker.model._base_manager.bulk_create(entries) | ||
|
||
if baker._using: | ||
manager = baker.model._base_manager.using(baker._using) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think a comment here would be useful. Something along these lines:
manager = baker.model._base_manager.using(baker._using) | |
# Try to use the desired DB and let Django fail if spanning | |
# relationships without the proper router setup | |
manager = baker.model._base_manager.using(baker._using) |
But the test case you added helps to alleviate any confusion here.
Looks like some CI test failures and a missing I will try to look into the test failures when I get a chance. |
I am getting a consistent failure when running tests with PostgreSQL: The actual failure is rather cryptic:
This does not occur when running against SQLite. |
iiinteresting. I'll spin up postgresql in docker and investigate. Cryptic indeed. |
Added a comment, extra line, and removed extra and in test name Moved _save_kwargs out of recursive function to fix tests
@timjklein36 that was such a sneaky failure, I had to move the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @ashiazed for such an hacky PR =) =) =)
I know the flying kwargs can be a pain to figure out how they work, but thanks for trying and fixing them!
Bulk create now optionally uses the right DB when
_using
is defined.#210
Using a combination of bulk_create and using requires some consideration by the user.
Django normally requires that two related objects are using the same DB, in a default router it can just check the state of each instance dynamically, but with bulk_create like this, that's not possible. This isn't a model bakery limitation.
I added tests to show the expected
ValueError
Django would return and a custom router that shows a successful bake.Not sure if you want me to drop any other comments or docs to make sure this is clear.
Also fixed a copy/pasted typo in the test names for this test class, I can split it up into a different PR if needed,