Skip to content

fix: turn off atomic requests for course api get views #33230

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.

Sign up for GitHub

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

Merged
jansenk merged 2 commits into from
Sep 14, 2023
Merged

fix: turn off atomic requests for course api get views #33230

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
a35ed96
fix: turn off atomic requests for course api get views
jansenk Sep 12, 2023
ff618ea
test: fix test
jansenk Sep 12, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 4 additions & 1 deletion lms/djangoapps/course_home_api/course_metadata/tests/test_views.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@
import ddt
import json
import mock
from django.db import transaction
from django.urls import reverse
from edx_toggles.toggles.testutils import override_waffle_flag
from unittest.mock import patch
Expand Down Expand Up @@ -86,7 +87,9 @@ def test_get_masqueraded_user(self):

def test_get_unknown_course(self):
url = reverse('course-home:course-metadata', args=['course-v1:unknown+course+2T2020'])
response = self.client.get(url)
# Django TestCase wraps every test in a transaction, so we must specifically wrap this when we expect an error
with transaction.atomic():
response = self.client.get(url)
Comment on lines +90 to +92
Copy link
Contributor

@ormsbee ormsbee Sep 12, 2023

Choose a reason for hiding this comment

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

Interesting. I wouldn't have expected this to cause a problem. What exception gets thrown without this?

Copy link
Contributor Author

@jansenk jansenk Sep 13, 2023

Choose a reason for hiding this comment

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

https://github.com/openedx/edx-platform/actions/runs/6162508221/job/16724140608

Copy link
Contributor Author

@jansenk jansenk Sep 13, 2023

Choose a reason for hiding this comment

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

==================================== ERRORS ====================================
_____ ERROR at teardown of CourseHomeMetadataTests.test_get_unknown_course _____
/edx/app/edxapp/venvs/edxapp/lib/python3.8/site-packages/waffle/testutils.py:41: in disable
    self.obj.delete()
/edx/app/edxapp/venvs/edxapp/lib/python3.8/site-packages/waffle/models.py:117: in delete
    ret = super().delete(*args, **kwargs)
/edx/app/edxapp/venvs/edxapp/lib/python3.8/site-packages/django/db/models/base.py:[96](https://github.com/openedx/edx-platform/actions/runs/6162508221/job/16724140608#step:7:100)7: in delete
    return collector.delete()
/edx/app/edxapp/venvs/edxapp/lib/python3.8/site-packages/django/db/models/deletion.py:410: in delete
    count = qs._raw_delete(using=self.using)
/edx/app/edxapp/venvs/edxapp/lib/python3.8/site-packages/django/db/models/query.py:762: in _raw_delete
    cursor = query.get_compiler(using).execute_sql(CURSOR)
/edx/app/edxapp/venvs/edxapp/lib/python3.8/site-packages/django/db/models/sql/compiler.py:1175: in execute_sql
    cursor.execute(sql, params)
/edx/app/edxapp/venvs/edxapp/lib/python3.8/site-packages/django/db/backends/utils.py:66: in execute
    return self._execute_with_wrappers(sql, params, many=False, executor=self._execute)
/edx/app/edxapp/venvs/edxapp/lib/python3.8/site-packages/django/db/backends/utils.py:75: in _execute_with_wrappers
    return executor(sql, params, many, context)
/edx/app/edxapp/venvs/edxapp/lib/python3.8/site-packages/django/db/backends/utils.py:78: in _execute
    self.db.validate_no_broken_transaction()
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ 

self = <django.db.backends.sqlite3.base.DatabaseWrapper object at 0x7f149c480430>

    def validate_no_broken_transaction(self):
        if self.needs_rollback:
>           raise TransactionManagementError(
                "An error occurred in the current transaction. You can't "
                "execute queries until the end of the 'atomic' block.")
E           django.db.transaction.TransactionManagementError: An error occurred in the current transaction. You can't execute queries until the end of the 'atomic' block.

/edx/app/edxapp/venvs/edxapp/lib/python3.8/site-packages/django/db/backends/base/base.py:447: TransactionManagementError

Copy link
Contributor

@ormsbee ormsbee Sep 13, 2023

Choose a reason for hiding this comment

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

Okay. Pure paranoia, but on your local machine when you're using runserver and try to access a non-existent course, it gives you the expected 404, right?

assert response.status_code == 404

def _assert_course_access_response(self, response, expect_course_access, expected_error_code):
Expand Down
3 changes: 3 additions & 0 deletions lms/djangoapps/course_home_api/course_metadata/views.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,8 @@
General view for the Course Home that contains metadata every page needs.
"""

from django.db import transaction
from django.utils.decorators import method_decorator
from opaque_keys.edx.keys import CourseKey
from rest_framework.generics import RetrieveAPIView
from rest_framework.response import Response
Expand All @@ -23,6 +25,7 @@
from lms.djangoapps.courseware.tabs import get_course_tab_list


@method_decorator(transaction.non_atomic_requests, name='dispatch')
class CourseHomeMetadataView(RetrieveAPIView):
"""
**Use Cases**
Expand Down
3 changes: 3 additions & 0 deletions openedx/core/djangoapps/courseware_api/views.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,8 @@
from completion.exceptions import UnavailableCompletionData
from completion.utilities import get_key_to_last_completed_block
from django.conf import settings
from django.db import transaction
from django.utils.decorators import method_decorator
from django.utils.functional import cached_property
from edx_django_utils.cache import TieredCache
from edx_rest_framework_extensions.auth.jwt.authentication import JwtAuthentication
Expand Down Expand Up @@ -368,6 +370,7 @@ def learning_assistant_enabled(self):
return learning_assistant_is_active(self.course_key)


@method_decorator(transaction.non_atomic_requests, name='dispatch')
class CoursewareInformation(RetrieveAPIView):
"""
**Use Cases**
Expand Down