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

Do not try to create revision if current transaction is under rollback. #919

Merged
merged 1 commit into from
Sep 24, 2022
Merged

Do not try to create revision if current transaction is under rollback. #919

merged 1 commit into from
Sep 24, 2022

Conversation

proofit404
Copy link
Contributor

Reason

Good evening,

Thank you for creating awesome library!

At the moment I found one corner case that I can't workaround without monkey patching reversion library.

Let say we enabled reversion middleware for every view within the project.

Let say in a signal receiver a database exception was catched.

I'm aware that programmers should avoid catching exceptions inside atomic block.

Unfortunately, such code could be placed in a poorly-written third-party package that we are forced to use.

Expected behavior

Without reversion enabled such code block would rollback transaction and delegate execution to the view management code.

Real behavior

With reversion enabled the same code would fail with transaction management error.

You could see how written test would fail without transaction state check.

======================================================================
ERROR: testTransactionInRollbackState (test_app.tests.test_models.TransactionRollbackTest)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/home/runner/work/django-reversion/django-reversion/tests/test_app/tests/test_models.py", line 460, in testTransactionInRollbackState
    pass
  File "/home/runner/work/django-reversion/django-reversion/reversion/revisions.py", line 312, in __exit__
    return self._context.__exit__(exc_type, exc_value, traceback)
  File "/opt/hostedtoolcache/Python/3.9.13/x64/lib/python3.9/contextlib.py", line 126, in __exit__
    next(self.gen)
  File "/home/runner/work/django-reversion/django-reversion/reversion/revisions.py", line 283, in _create_revision_context
    _save_revision(
  File "/home/runner/work/django-reversion/django-reversion/reversion/revisions.py", line 219, in _save_revision
    model_db_existing_pks = {
  File "/home/runner/work/django-reversion/django-reversion/reversion/revisions.py", line 220, in <dictcomp>
    model: {
  File "/home/runner/work/django-reversion/django-reversion/reversion/revisions.py", line 221, in <dictcomp>
    db: frozenset(map(
  File "/opt/hostedtoolcache/Python/3.9.13/x64/lib/python3.9/site-packages/django/db/models/query.py", line 394, in __iter__
    self._fetch_all()
  File "/opt/hostedtoolcache/Python/3.9.13/x64/lib/python3.9/site-packages/django/db/models/query.py", line 1866, in _fetch_all
    self._result_cache = list(self._iterable_class(self))
  File "/opt/hostedtoolcache/Python/3.9.13/x64/lib/python3.9/site-packages/django/db/models/query.py", line 281, in __iter__
    for row in compiler.results_iter(
  File "/opt/hostedtoolcache/Python/3.9.13/x64/lib/python3.9/site-packages/django/db/models/sql/compiler.py", line 1346, in results_iter
    results = self.execute_sql(
  File "/opt/hostedtoolcache/Python/3.9.13/x64/lib/python3.9/site-packages/django/db/models/sql/compiler.py", line 1395, in execute_sql
    cursor.execute(sql, params)
  File "/opt/hostedtoolcache/Python/3.9.13/x64/lib/python3.9/site-packages/django/db/backends/utils.py", line 67, in execute
    return self._execute_with_wrappers(
  File "/opt/hostedtoolcache/Python/3.9.13/x64/lib/python3.9/site-packages/django/db/backends/utils.py", line 80, in _execute_with_wrappers
    return executor(sql, params, many, context)
  File "/opt/hostedtoolcache/Python/3.9.13/x64/lib/python3.9/site-packages/django/db/backends/utils.py", line 83, in _execute
    self.db.validate_no_broken_transaction()
  File "/opt/hostedtoolcache/Python/3.9.13/x64/lib/python3.9/site-packages/django/db/backends/base/base.py", line 520, in validate_no_broken_transaction
    raise TransactionManagementError(
django.db.transaction.TransactionManagementError: An error occurred in the current transaction. You can't execute queries until the end of the 'atomic' block.

----------------------------------------------------------------------

Suggested solution

After execution exists yield statement inside _create_revision_context context manager we would place additional checks for current transaction state before trying to create Revision object.

Best regards,
Josiah.

@etianen etianen merged commit 284dd51 into etianen:master Sep 24, 2022
@etianen
Copy link
Owner

etianen commented Sep 24, 2022

This looks great! Thanks for the tests and the comprehensive writeup! I'll get this into a release ASAP!

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.

2 participants