revert: removing read_committed argument from outer_atomic function (#28161)

In the PR https://github.com/edx/edx-platform/pull/10659 the outer_atomic decorator/context manager was created to prevent nested atomic blocks. This method received a boolean parameter read_committed to enforce read-committed MySQL isolation level. From Django 2, the default isolation level Django sets is read-committed, so the aforementioned parameter for outer_atomic can be removed
This commit is contained in:
Jhony Avella
2021-08-31 15:39:35 -05:00
committed by GitHub
parent 1749235761
commit 95a6abcd1f
6 changed files with 8 additions and 95 deletions

View File

@@ -268,7 +268,7 @@ class ChooseModeView(View):
@method_decorator(transaction.non_atomic_requests)
@method_decorator(login_required)
@method_decorator(outer_atomic(read_committed=True))
@method_decorator(outer_atomic())
def post(self, request, course_id):
"""Takes the form submission from the page and parses it.

View File

@@ -278,7 +278,7 @@ def _update_email_opt_in(request, org):
@transaction.non_atomic_requests
@require_POST
@outer_atomic(read_committed=True)
@outer_atomic()
def change_enrollment(request, check_access=True):
"""
Modify the enrollment status for the logged-in user.

View File

@@ -52,8 +52,7 @@ class OuterAtomic(transaction.Atomic):
"""
ALLOW_NESTED = False
def __init__(self, using, savepoint, read_committed=False, name=None):
self.read_committed = read_committed
def __init__(self, using, savepoint, name=None):
self.name = name
super().__init__(using, savepoint)
@@ -82,16 +81,10 @@ class OuterAtomic(transaction.Atomic):
if not self.ALLOW_NESTED and connection.in_atomic_block:
raise transaction.TransactionManagementError('Cannot be inside an atomic block.')
# This will set the transaction isolation level to READ COMMITTED for the next transaction.
if self.read_committed is True:
if connection.vendor == 'mysql':
cursor = connection.cursor()
cursor.execute("SET TRANSACTION ISOLATION LEVEL READ COMMITTED")
super().__enter__()
def outer_atomic(using=None, savepoint=True, read_committed=False, name=None):
def outer_atomic(using=None, savepoint=True, name=None):
"""
A variant of Django's atomic() which cannot be nested inside another atomic.
@@ -119,23 +112,18 @@ def outer_atomic(using=None, savepoint=True, read_committed=False, name=None):
automatic transaction management disabled and other callers are not
affected.
Additionally, some views need to use READ COMMITTED isolation level.
For this add @transaction.non_atomic_requests and
@outer_atomic(read_committed=True) decorators on it.
Arguments:
using (str): the name of the database.
read_committed (bool): Whether to use read committed isolation level.
name (str): the name to give to this outer_atomic instance.
Raises:
TransactionManagementError: if already inside an atomic block.
"""
if callable(using):
return OuterAtomic(DEFAULT_DB_ALIAS, savepoint, read_committed)(using)
return OuterAtomic(DEFAULT_DB_ALIAS, savepoint)(using)
# Decorator: @outer_atomic(...) or context manager: with outer_atomic(...): ...
else:
return OuterAtomic(using, savepoint, read_committed, name)
return OuterAtomic(using, savepoint, name)
def generate_int_id(minimum=0, maximum=MYSQL_MAX_INT, used_ids=None):

View File

@@ -1,15 +1,10 @@
"""Tests for util.db module."""
import threading
import time
import unittest
from io import StringIO
import ddt
from django.contrib.auth.models import User # lint-amnesty, pylint: disable=imported-auth-user
from django.core.management import call_command
from django.db import IntegrityError, connection
from django.db.transaction import TransactionManagementError, atomic
from django.test import TestCase, TransactionTestCase
from django.test.utils import override_settings
@@ -22,83 +17,16 @@ def do_nothing():
return
@ddt.ddt
class TransactionManagersTestCase(TransactionTestCase):
"""
Tests outer_atomic.
Note: This TestCase only works with MySQL.
To test do: "./manage.py lms --settings=test_with_mysql test util.tests.test_db"
"""
DECORATORS = {
'outer_atomic': outer_atomic(),
'outer_atomic_read_committed': outer_atomic(read_committed=True),
}
@ddt.data(
('outer_atomic', IntegrityError, None, True),
('outer_atomic_read_committed', type(None), False, True),
)
@ddt.unpack
def test_concurrent_requests(self, transaction_decorator_name, exception_class, created_in_1, created_in_2):
"""
Test that when isolation level is set to READ COMMITTED get_or_create()
for the same row in concurrent requests does not raise an IntegrityError.
"""
transaction_decorator = self.DECORATORS[transaction_decorator_name]
if connection.vendor != 'mysql':
raise unittest.SkipTest('Only works on MySQL.')
class RequestThread(threading.Thread):
""" A thread which runs a dummy view."""
def __init__(self, delay, **kwargs):
super().__init__(**kwargs)
self.delay = delay
self.status = {}
@transaction_decorator
def run(self):
"""A dummy view."""
try:
try:
User.objects.get(username='student', email='student@edx.org')
except User.DoesNotExist:
pass
else:
raise AssertionError('Did not raise User.DoesNotExist.')
if self.delay > 0:
time.sleep(self.delay)
__, created = User.objects.get_or_create(username='student', email='student@edx.org')
except Exception as exception: # pylint: disable=broad-except
self.status['exception'] = exception
else:
self.status['created'] = created
thread1 = RequestThread(delay=1)
thread2 = RequestThread(delay=0)
thread1.start()
thread2.start()
thread2.join()
thread1.join()
assert isinstance(thread1.status.get('exception'), exception_class)
assert thread1.status.get('created') == created_in_1
assert thread2.status.get('exception') is None
assert thread2.status.get('created') == created_in_2
def test_outer_atomic_nesting(self):
"""
Test that outer_atomic raises an error if it is nested inside
another atomic.
"""
if connection.vendor != 'mysql':
raise unittest.SkipTest('Only works on MySQL.')
outer_atomic()(do_nothing)() # pylint: disable=not-callable
with atomic():
@@ -120,9 +48,6 @@ class TransactionManagersTestCase(TransactionTestCase):
Test that a named outer_atomic raises an error only if nested in
enable_named_outer_atomic and inside another atomic.
"""
if connection.vendor != 'mysql':
raise unittest.SkipTest('Only works on MySQL.')
outer_atomic(name='abc')(do_nothing)() # pylint: disable=not-callable
with atomic():

View File

@@ -815,7 +815,7 @@ class SubmitPhotosView(View):
return super().dispatch(request, *args, **kwargs)
@method_decorator(login_required)
@method_decorator(outer_atomic(read_committed=True))
@method_decorator(outer_atomic())
def post(self, request):
"""
Submit photos for verification.

View File

@@ -209,7 +209,7 @@ def create_account_with_params(request, params):
custom_form = get_registration_extension_form(data=params)
# Perform operations within a transaction that are critical to account creation
with outer_atomic(read_committed=True):
with outer_atomic():
# first, create the account
(user, profile, registration) = do_create_account(form, custom_form)