fix: Update on_commit_changes_to of modulestore to check MySQL transaction [FC-0097] (#37485)

- `handle_update_xblock_upstream_link` is called asynchronously with celery. In `update_upstream_downstream_link_handler`, the xblock has the updated version, but when calling `handle_update_xblock_upstream_link` inside Celery, the xblock is outdated in a previous version, which is why the error occurs. This happens because `on_commit_changes_to` is executed before the MySQL transaction ends.
- Added `ImmediateOnCommitMixin` to be used in tests that need to call `on_commit_changes_to`. See https://github.com/openedx/edx-platform/pull/37485#issuecomment-3412979170  for more info
This commit is contained in:
Chris Chávez
2025-10-20 17:02:04 -05:00
committed by GitHub
parent 23295c5e18
commit 3f5ac6ddbc
10 changed files with 79 additions and 24 deletions

View File

@@ -12,6 +12,7 @@ from abc import ABCMeta, abstractmethod
from collections import defaultdict
from contextlib import contextmanager
from operator import itemgetter
from django.db import transaction
from opaque_keys.edx.keys import AssetKey, CourseKey
from opaque_keys.edx.locations import Location # For import backwards compatibility
@@ -322,6 +323,10 @@ class BulkOperationsMixin:
"""
Call some callback when the currently active bulk operation has saved
"""
# If we're in a MySQL transaction, so the new version will only be committed to the
# SplitModulestoreCourseIndex table after the MySQL transaction is closed.
def wrapped_fn():
transaction.on_commit(fn)
# Check if a bulk op is active. If so, defer fn(); otherwise call it immediately.
# Note: calling _get_bulk_ops_record() here and then checking .active can have side-effects in some cases
# because it creates an entry in the defaultdict if none exists, so we check if the record is active using
@@ -329,9 +334,9 @@ class BulkOperationsMixin:
# so we check it this way:
if course_key and course_key.for_branch(None) in self._active_bulk_ops.records:
bulk_ops_record = self._active_bulk_ops.records[course_key.for_branch(None)]
bulk_ops_record.defer_until_commit(fn)
bulk_ops_record.defer_until_commit(wrapped_fn)
else:
fn() # There is no active bulk operation - call fn() now.
wrapped_fn() # There is no active bulk operation - call wrapped_fn() now.
def _is_in_bulk_operation(self, course_key, ignore_case=False):
"""

View File

@@ -613,6 +613,35 @@ class ModuleStoreTestCase(
return updated_course
class ImmediateOnCommitMixin:
"""
Mixin for tests that want `on_commit` callbacks to run immediately,
even under TestCase (which normally wraps tests in a transaction
that never commits).
Especially useful when the test needs to execute an event that occurs after an `on_commit`
"""
@classmethod
def setUpClass(cls):
super_cls = super()
if hasattr(super_cls, 'setUpClass'):
super_cls.setUpClass()
# Patch `transaction.on_commit` so that callbacks run immediately
cls._on_commit_patcher = patch(
'django.db.transaction.on_commit',
side_effect=lambda func, **kwargs: func()
)
cls._on_commit_patcher.start()
@classmethod
def tearDownClass(cls):
# Stop patching, restore original behavior
cls._on_commit_patcher.stop()
super_cls = super()
if hasattr(super_cls, 'tearDownClass'):
super_cls.tearDownClass()
def upload_file_to_course(course_key, contentstore, source_file, target_filename):
'''
Uploads the given source file to the given course, and returns the content of the file.

View File

@@ -13,7 +13,7 @@ from contextlib import contextmanager
from functools import wraps
from unittest.mock import Mock
from django.test import TestCase
from django.test import TransactionTestCase
from opaque_keys.edx.keys import CourseKey
from path import Path as path
@@ -306,7 +306,7 @@ class LazyFormat:
return str(self)[index]
class CourseComparisonTest(TestCase):
class CourseComparisonTest(TransactionTestCase):
"""
Mixin that has methods for comparing courses for equality.
"""