Skip to content
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

Merged

Conversation

ashiazed
Copy link
Contributor

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,

Copy link
Collaborator

@timjklein36 timjklein36 left a 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
Copy link
Collaborator

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).

@@ -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(
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Extra unneeded "and":

Suggested change
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()]):
Copy link
Collaborator

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)
Copy link
Collaborator

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:

Suggested change
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.

@timjklein36
Copy link
Collaborator

Looks like some CI test failures and a missing CHANGELOG entry.

I will try to look into the test failures when I get a chance.

@timjklein36
Copy link
Collaborator

I am getting a consistent failure when running tests with PostgreSQL: TestBakerCreatesAssociatedModels.test_create_many_to_many_if_flagged.

The actual failure is rather cryptic:

______________________________________________________________________ TestBakerCreatesAssociatedModels.test_create_many_to_many_if_flagged _______________________________________________________________________
.tox/py37-django32-postgresql/lib/python3.7/site-packages/django/db/backends/utils.py:84: in _execute
    return self.cursor.execute(sql, params)
E   psycopg2.errors.InFailedSqlTransaction: current transaction is aborted, commands ignored until end of transaction block

The above exception was the direct cause of the following exception:
tests/test_baker.py:394: in test_create_many_to_many_if_flagged
    store = baker.make(models.Store, make_m2m=True)
model_bakery/baker.py:99: in make
    _save_kwargs=_save_kwargs, _refresh_after_create=_refresh_after_create, **attrs
model_bakery/baker.py:324: in make
    return self._make(**params)
model_bakery/baker.py:358: in _make
    self.m2m_dict[field.name] = self.m2m_value(field)
model_bakery/baker.py:404: in m2m_value
    return self.generate_value(field)
model_bakery/baker.py:594: in generate_value
    return generator(**generator_attrs)
model_bakery/random_gen.py:258: in gen_m2m
    return make(model, _quantity=MAX_MANY_QUANTITY, **attrs)
model_bakery/baker.py:95: in make
    for _ in range(_quantity)
model_bakery/baker.py:95: in <listcomp>
    for _ in range(_quantity)
model_bakery/baker.py:324: in make
    return self._make(**params)
model_bakery/baker.py:388: in _make
    _save_kwargs=_save_kwargs,
model_bakery/baker.py:418: in instance
    instance.save(**_save_kwargs)
.tox/py37-django32-postgresql/lib/python3.7/site-packages/django/db/models/base.py:727: in save
    force_update=force_update, update_fields=update_fields)
.tox/py37-django32-postgresql/lib/python3.7/site-packages/django/db/models/base.py:765: in save_base
    force_update, using, update_fields,
.tox/py37-django32-postgresql/lib/python3.7/site-packages/django/db/models/base.py:868: in _save_table
    results = self._do_insert(cls._base_manager, using, fields, returning_fields, raw)
.tox/py37-django32-postgresql/lib/python3.7/site-packages/django/db/models/base.py:908: in _do_insert
    using=using, raw=raw,
.tox/py37-django32-postgresql/lib/python3.7/site-packages/django/db/models/manager.py:85: in manager_method
    return getattr(self.get_queryset(), name)(*args, **kwargs)
.tox/py37-django32-postgresql/lib/python3.7/site-packages/django/db/models/query.py:1270: in _insert
    return query.get_compiler(using=using).execute_sql(returning_fields)
.tox/py37-django32-postgresql/lib/python3.7/site-packages/django/db/models/sql/compiler.py:1416: in execute_sql
    cursor.execute(sql, params)
.tox/py37-django32-postgresql/lib/python3.7/site-packages/django/db/backends/utils.py:66: in execute
    return self._execute_with_wrappers(sql, params, many=False, executor=self._execute)
.tox/py37-django32-postgresql/lib/python3.7/site-packages/django/db/backends/utils.py:75: in _execute_with_wrappers
    return executor(sql, params, many, context)
.tox/py37-django32-postgresql/lib/python3.7/site-packages/django/db/backends/utils.py:84: in _execute
    return self.cursor.execute(sql, params)
.tox/py37-django32-postgresql/lib/python3.7/site-packages/django/db/utils.py:90: in __exit__
    raise dj_exc_value.with_traceback(traceback) from exc_value
.tox/py37-django32-postgresql/lib/python3.7/site-packages/django/db/backends/utils.py:84: in _execute
    return self.cursor.execute(sql, params)
E   django.db.utils.InternalError: current transaction is aborted, commands ignored until end of transaction block
---------------------------------------------------------------------------------------------- Captured stderr call -----------------------------------------------------------------------------------------------
Exception ignored in: <generator object cursor_iter at 0x7f4fdcbdd5d0>
Traceback (most recent call last):
  File "/home/tim/projects/model_bakery/.tox/py37-django32-postgresql/lib/python3.7/site-packages/django/db/models/sql/compiler.py", line 1649, in cursor_iter
    cursor.close()
psycopg2.errors.InvalidCursorName: cursor "_django_curs_139981442959168_sync_1" does not exist

This does not occur when running against SQLite.

@timjklein36 timjklein36 self-requested a review October 27, 2021 20:21
@ashiazed
Copy link
Contributor Author

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
@ashiazed
Copy link
Contributor Author

@timjklein36 that was such a sneaky failure, I had to move the _save_kwargs in bulk_create outside of the recursive function. I don't entirely know why but it was causing cascading transaction failures.

Copy link
Member

@berinhard berinhard left a 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!

@berinhard berinhard merged commit b764f36 into model-bakers:main Oct 29, 2021
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.

None yet

4 participants