fix: call blockstore APIs in atomic transactions. [BD-14] (#30456)

* fix: call blockstore APIs in atomic transactions.

To ensure database integry when using the blockstore APIs, the Content
Library views which invoke blockstore API methods are wrapped in
database transactions.

* fix: assert create_library is called inside an atomic transaction block

openedx/core/djangoapps/content_libraries/api.py

@@ -426,6 +426,10 @@ def create_library(
     """
     assert isinstance(collection_uuid, UUID)
     assert isinstance(org, Organization)
+    assert not transaction.get_autocommit(), (
+        "Call within a django.db.transaction.atomic block so that all created objects are rolled back on error."
+    )
+
     validate_unicode_slug(slug)
     # First, create the blockstore bundle:
     bundle = create_bundle(
@@ -436,20 +440,16 @@ def create_library(
     )
     # Now create the library reference in our database:
     try:
-        # Atomic transaction required because if this fails,
-        # we need to delete the bundle in the exception handler.
-        with transaction.atomic():
-            ref = ContentLibrary.objects.create(
-                org=org,
-                slug=slug,
-                type=library_type,
-                bundle_uuid=bundle.uuid,
-                allow_public_learning=allow_public_learning,
-                allow_public_read=allow_public_read,
-                license=library_license,
-            )
+        ref = ContentLibrary.objects.create(
+            org=org,
+            slug=slug,
+            type=library_type,
+            bundle_uuid=bundle.uuid,
+            allow_public_learning=allow_public_learning,
+            allow_public_read=allow_public_read,
+            license=library_license,
+        )
     except IntegrityError:
-        delete_bundle(bundle.uuid)
         raise LibraryAlreadyExists(slug)  # lint-amnesty, pylint: disable=raise-missing-from
     CONTENT_LIBRARY_CREATED.send(sender=None, library_key=ref.library_key)
     return ContentLibraryMetadata(

openedx/core/djangoapps/content_libraries/tests/test_content_libraries.py

@@ -30,6 +30,7 @@ from openedx.core.djangoapps.content_libraries.tests.base import (
 )
 from openedx.core.djangoapps.content_libraries.constants import VIDEO, COMPLEX, PROBLEM, CC_4_BY, ALL_RIGHTS_RESERVED
 from openedx.core.djangolib.blockstore_cache import cache
+from openedx.core.lib import blockstore_api
 from common.djangoapps.student.tests.factories import UserFactory
 
 
@@ -189,10 +190,22 @@ class ContentLibrariesTestMixin:
         You can't create a library with the same slug as an existing library,
         or an invalid slug.
         """
+        assert 0 == len(blockstore_api.get_bundles(text_search='some-slug'))
         self._create_library(slug="some-slug", title="Existing Library")
-        self._create_library(slug="some-slug", title="Duplicate Library", expect_response=400)
+        assert 1 == len(blockstore_api.get_bundles(text_search='some-slug'))
 
-        self._create_library(slug="Invalid Slug!", title="Library with Bad Slug", expect_response=400)
+        # Try to create a library+bundle with a duplicate slug
+        response = self._create_library(slug="some-slug", title="Duplicate Library", expect_response=400)
+        assert response == {
+            'slug': 'A library with that ID already exists.',
+        }
+        # The second bundle created with that slug is removed when the transaction rolls back.
+        assert 1 == len(blockstore_api.get_bundles(text_search='some-slug'))
+
+        response = self._create_library(slug="Invalid Slug!", title="Library with Bad Slug", expect_response=400)
+        assert response == {
+            'slug': ['Enter a valid “slug” consisting of Unicode letters, numbers, underscores, or hyphens.'],
+        }
 
     @ddt.data(True, False)
     @patch("openedx.core.djangoapps.content_libraries.views.LibraryApiPagination.page_size", new=2)

openedx/core/djangoapps/content_libraries/tests/test_runtime.py

@@ -5,7 +5,7 @@ import json
 from gettext import GNUTranslations
 
 from completion.test_utils import CompletionWaffleTestMixin
-from django.db import connections
+from django.db import connections, transaction
 from django.test import LiveServerTestCase, TestCase
 from django.utils.text import slugify
 from organizations.models import Organization
@@ -51,17 +51,18 @@ class ContentLibraryContentTestMixin:
             short_name="CL-TEST",
         )
         _, slug = self.id().rsplit('.', 1)
-        self.library = library_api.create_library(
-            collection_uuid=self.collection.uuid,
-            library_type=COMPLEX,
-            org=self.organization,
-            slug=slugify(slug),
-            title=(f"{slug} Test Lib"),
-            description="",
-            allow_public_learning=True,
-            allow_public_read=False,
-            library_license=ALL_RIGHTS_RESERVED,
-        )
+        with transaction.atomic():
+            self.library = library_api.create_library(
+                collection_uuid=self.collection.uuid,
+                library_type=COMPLEX,
+                org=self.organization,
+                slug=slugify(slug),
+                title=(f"{slug} Test Lib"),
+                description="",
+                allow_public_learning=True,
+                allow_public_read=False,
+                library_license=ALL_RIGHTS_RESERVED,
+            )
 
 
 class ContentLibraryRuntimeTestMixin(ContentLibraryContentTestMixin):
@@ -83,17 +84,18 @@ class ContentLibraryRuntimeTestMixin(ContentLibraryContentTestMixin):
         library_api.create_library_block_child(unit_block_key, "problem", "p1")
         library_api.publish_changes(self.library.key)
         # Now do the same in a different library:
-        library2 = library_api.create_library(
-            collection_uuid=self.collection.uuid,
-            org=self.organization,
-            slug="idolx",
-            title=("Identical OLX Test Lib 2"),
-            description="",
-            library_type=COMPLEX,
-            allow_public_learning=True,
-            allow_public_read=False,
-            library_license=CC_4_BY,
-        )
+        with transaction.atomic():
+            library2 = library_api.create_library(
+                collection_uuid=self.collection.uuid,
+                org=self.organization,
+                slug="idolx",
+                title=("Identical OLX Test Lib 2"),
+                description="",
+                library_type=COMPLEX,
+                allow_public_learning=True,
+                allow_public_read=False,
+                library_license=CC_4_BY,
+            )
         unit_block2_key = library_api.create_library_block(library2.key, "unit", "u1").usage_key
         library_api.create_library_block_child(unit_block2_key, "problem", "p1")
         library_api.publish_changes(library2.key)

openedx/core/djangoapps/content_libraries/views.py

@@ -18,6 +18,7 @@ from django.contrib.auth import authenticate
 from django.contrib.auth import get_user_model
 from django.contrib.auth import login
 from django.contrib.auth.models import Group
+from django.db.transaction import atomic
 from django.http import Http404
 from django.http import HttpResponseBadRequest
 from django.http import JsonResponse
@@ -142,6 +143,7 @@ class LibraryRootView(APIView):
     Views to list, search for, and create content libraries.
     """
 
+    @atomic
     @apidocs.schema(
         parameters=[
             *LibraryApiPagination.apidoc_params,
@@ -184,6 +186,7 @@ class LibraryRootView(APIView):
             return paginator.get_paginated_response(serializer.data)
         return Response(serializer.data)
 
+    @atomic
     def post(self, request):
         """
         Create a new content library.
@@ -223,6 +226,7 @@ class LibraryDetailsView(APIView):
     """
     Views to work with a specific content library
     """
+    @atomic
     @convert_exceptions
     def get(self, request, lib_key_str):
         """
@@ -233,6 +237,7 @@ class LibraryDetailsView(APIView):
         result = api.get_library(key)
         return Response(ContentLibraryMetadataSerializer(result).data)
 
+    @atomic
     @convert_exceptions
     def patch(self, request, lib_key_str):
         """
@@ -255,6 +260,7 @@ class LibraryDetailsView(APIView):
         result = api.get_library(key)
         return Response(ContentLibraryMetadataSerializer(result).data)
 
+    @atomic
     @convert_exceptions
     def delete(self, request, lib_key_str):  # pylint: disable=unused-argument
         """
@@ -275,6 +281,7 @@ class LibraryTeamView(APIView):
     Note also the 'allow_public_' settings which can be edited by PATCHing the
     library itself (LibraryDetailsView.patch).
     """
+    @atomic
     @convert_exceptions
     def post(self, request, lib_key_str):
         """
@@ -302,6 +309,7 @@ class LibraryTeamView(APIView):
         grant = api.get_library_user_permissions(key, user)
         return Response(ContentLibraryPermissionSerializer(grant).data)
 
+    @atomic
     @convert_exceptions
     def get(self, request, lib_key_str):
         """
@@ -320,6 +328,7 @@ class LibraryTeamUserView(APIView):
     View to add/remove/edit an individual user's permissions for a content
     library.
     """
+    @atomic
     @convert_exceptions
     def put(self, request, lib_key_str, username):
         """
@@ -338,6 +347,7 @@ class LibraryTeamUserView(APIView):
         grant = api.get_library_user_permissions(key, user)
         return Response(ContentLibraryPermissionSerializer(grant).data)
 
+    @atomic
     @convert_exceptions
     def get(self, request, lib_key_str, username):
         """
@@ -351,6 +361,7 @@ class LibraryTeamUserView(APIView):
             raise NotFound
         return Response(ContentLibraryPermissionSerializer(grant).data)
 
+    @atomic
     @convert_exceptions
     def delete(self, request, lib_key_str, username):
         """
@@ -372,6 +383,7 @@ class LibraryTeamGroupView(APIView):
     """
     View to add/remove/edit a group's permissions for a content library.
     """
+    @atomic
     @convert_exceptions
     def put(self, request, lib_key_str, group_name):
         """
@@ -386,6 +398,7 @@ class LibraryTeamGroupView(APIView):
         api.set_library_group_permissions(key, group, access_level=serializer.validated_data["access_level"])
         return Response({})
 
+    @atomic
     @convert_exceptions
     def delete(self, request, lib_key_str, username):
         """
@@ -404,6 +417,7 @@ class LibraryBlockTypesView(APIView):
     """
     View to get the list of XBlock types that can be added to this library
     """
+    @atomic
     @convert_exceptions
     def get(self, request, lib_key_str):
         """
@@ -428,6 +442,7 @@ class LibraryLinksView(APIView):
     Links always point to a specific published version of the target bundle.
     Links are identified by a slug-like ID, e.g. "link1"
     """
+    @atomic
     @convert_exceptions
     def get(self, request, lib_key_str):
         """
@@ -438,6 +453,7 @@ class LibraryLinksView(APIView):
         result = api.get_bundle_links(key)
         return Response(LibraryBundleLinkSerializer(result, many=True).data)
 
+    @atomic
     @convert_exceptions
     def post(self, request, lib_key_str):
         """
@@ -462,6 +478,7 @@ class LibraryLinkDetailView(APIView):
     """
     View to update/delete an existing library link
     """
+    @atomic
     @convert_exceptions
     def patch(self, request, lib_key_str, link_id):
         """
@@ -478,6 +495,7 @@ class LibraryLinkDetailView(APIView):
         api.update_bundle_link(key, link_id, version=serializer.validated_data['version'])
         return Response({})
 
+    @atomic
     @convert_exceptions
     def delete(self, request, lib_key_str, link_id):  # pylint: disable=unused-argument
         """
@@ -494,6 +512,7 @@ class LibraryCommitView(APIView):
     """
     Commit/publish or revert all of the draft changes made to the library.
     """
+    @atomic
     @convert_exceptions
     def post(self, request, lib_key_str):
         """
@@ -505,6 +524,7 @@ class LibraryCommitView(APIView):
         api.publish_changes(key)
         return Response({})
 
+    @atomic
     @convert_exceptions
     def delete(self, request, lib_key_str):  # pylint: disable=unused-argument
         """
@@ -522,6 +542,7 @@ class LibraryBlocksView(APIView):
     """
     Views to work with XBlocks in a specific content library.
     """
+    @atomic
     @apidocs.schema(
         parameters=[
             *LibraryApiPagination.apidoc_params,
@@ -560,6 +581,7 @@ class LibraryBlocksView(APIView):
 
         return Response(LibraryXBlockMetadataSerializer(result, many=True).data)
 
+    @atomic
     @convert_exceptions
     def post(self, request, lib_key_str):
         """
@@ -592,6 +614,7 @@ class LibraryBlockView(APIView):
     """
     Views to work with an existing XBlock in a content library.
     """
+    @atomic
     @convert_exceptions
     def get(self, request, usage_key_str):
         """
@@ -602,6 +625,7 @@ class LibraryBlockView(APIView):
         result = api.get_library_block(key)
         return Response(LibraryXBlockMetadataSerializer(result).data)
 
+    @atomic
     @convert_exceptions
     def delete(self, request, usage_key_str):  # pylint: disable=unused-argument
         """
@@ -628,6 +652,7 @@ class LibraryBlockLtiUrlView(APIView):
 
     Returns 404 in case the block not found by the given key.
     """
+    @atomic
     @convert_exceptions
     def get(self, request, usage_key_str):
         """
@@ -647,6 +672,7 @@ class LibraryBlockOlxView(APIView):
     """
     Views to work with an existing XBlock's OLX
     """
+    @atomic
     @convert_exceptions
     def get(self, request, usage_key_str):
         """
@@ -657,6 +683,7 @@ class LibraryBlockOlxView(APIView):
         xml_str = api.get_library_block_olx(key)
         return Response(LibraryXBlockOlxSerializer({"olx": xml_str}).data)
 
+    @atomic
     @convert_exceptions
     def post(self, request, usage_key_str):
         """
@@ -682,6 +709,7 @@ class LibraryBlockAssetListView(APIView):
     """
     Views to list an existing XBlock's static asset files
     """
+    @atomic
     @convert_exceptions
     def get(self, request, usage_key_str):
         """
@@ -700,6 +728,7 @@ class LibraryBlockAssetView(APIView):
     """
     parser_classes = (MultiPartParser, )
 
+    @atomic
     @convert_exceptions
     def get(self, request, usage_key_str, file_path):
         """
@@ -713,6 +742,7 @@ class LibraryBlockAssetView(APIView):
                 return Response(LibraryXBlockStaticFileSerializer(f).data)
         raise NotFound
 
+    @atomic
     @convert_exceptions
     def put(self, request, usage_key_str, file_path):
         """
@@ -735,6 +765,7 @@ class LibraryBlockAssetView(APIView):
             raise ValidationError("Invalid file path")  # lint-amnesty, pylint: disable=raise-missing-from
         return Response(LibraryXBlockStaticFileSerializer(result).data)
 
+    @atomic
     @convert_exceptions
     def delete(self, request, usage_key_str, file_path):
         """
@@ -757,6 +788,7 @@ class LibraryImportTaskViewSet(ViewSet):
     Import blocks from Courseware through modulestore.
     """
 
+    @atomic
     @convert_exceptions
     def list(self, request, lib_key_str):
         """
@@ -775,6 +807,7 @@ class LibraryImportTaskViewSet(ViewSet):
             paginator.paginate_queryset(result, request)
         )
 
+    @atomic
     @convert_exceptions
     def create(self, request, lib_key_str):
         """
@@ -795,6 +828,7 @@ class LibraryImportTaskViewSet(ViewSet):
         import_task = api.import_blocks_create_task(library_key, course_key)
         return Response(ContentLibraryBlockImportTaskSerializer(import_task).data)
 
+    @atomic
     @convert_exceptions
     def retrieve(self, request, lib_key_str, pk=None):
         """