diff --git a/exercise/async_views.py b/exercise/async_views.py index 3125c537d..b01b87c9f 100644 --- a/exercise/async_views.py +++ b/exercise/async_views.py @@ -9,6 +9,7 @@ from notification.models import Notification from lti_tool.utils import send_lti_points +from .cache.exercise import ExerciseCache from .forms import SubmissionCallbackForm from .models import SubmissionTagging @@ -94,6 +95,15 @@ def _post_async_submission(request, exercise, submission, errors=None): # pylint # the LTI Platform. if submission.meta_data == "": submission.meta_data = {} + exercise_version = form.cleaned_data["exercise_version"] + if exercise_version: + language = submission.lang or exercise.course_instance.default_language + cached_version = ExerciseCache.cached_exercise_version(exercise, language) + if cached_version and cached_version != exercise_version: + ExerciseCache.invalidate(exercise, modifiers=[language]) + if not isinstance(submission.meta_data, dict): + submission.meta_data = {} + submission.meta_data["exercise_version"] = exercise_version if (form.cleaned_data["lti_launch_id"] and submission.meta_data.get("lti-launch-id") is None): submission.meta_data["lti-launch-id"] = form.cleaned_data["lti_launch_id"] diff --git a/exercise/cache/exercise.py b/exercise/cache/exercise.py index 03191e35e..bbbb23bdc 100644 --- a/exercise/cache/exercise.py +++ b/exercise/cache/exercise.py @@ -3,6 +3,7 @@ from typing import Any, Dict, List, Optional, TYPE_CHECKING from django.conf import settings +from django.core.cache import cache from django.http.request import HttpRequest from lib.cache import CachedAbstract @@ -29,6 +30,7 @@ def compress(data): class ExerciseCache(CachedAbstract): """ Exercise HTML content """ KEY_PREFIX = "exercisepage" + VERSION_MAX_AGE = 15 * 60 def __init__( # pylint: disable=too-many-arguments self, @@ -43,7 +45,11 @@ def __init__( # pylint: disable=too-many-arguments self.load_args = [language, request, students, url_name, ordinal] super().__init__(exercise, modifiers=[language]) - def _needs_generation(self, data: Dict[str, Any]) -> bool: + def _needs_generation(self, data: Optional[Dict[str, Any]]) -> bool: + if data and 'exercise_version' not in data: + # Cache entries created before exercise version stamping was added must be refreshed + # so update detection can work. + return True expires = data['expires'] if data else None return not expires or time.time() > expires # pylint: disable-next=arguments-differ @@ -51,7 +57,13 @@ def _generate_data(self, exercise: 'BaseExercise', data: Optional[Dict[str, Any] try: page = exercise.load_page( *self.load_args, - last_modified=data['last_modified'] if data else None + # A versionless cache entry must receive a full response. Reusing + # its timestamp could produce a 304 that can not add the version. + last_modified=( + data['last_modified'] + if data and 'exercise_version' in data + else None + ) ) content = compress(page.content.encode('utf-8')) @@ -60,6 +72,7 @@ def _generate_data(self, exercise: 'BaseExercise', data: Optional[Dict[str, Any] 'head': page.head, 'content': content, 'last_modified': page.last_modified, + 'exercise_version': page.exercise_version, 'expires': page.expires if page.is_loaded else 0, } except RemotePageNotModified as e: @@ -74,6 +87,29 @@ def content(self) -> str: content = decompress(self.data['content']).decode('utf-8') return content + def exercise_version(self) -> str: + return self.data.get('exercise_version') or '' + + @classmethod + def cached_exercise_version( + cls, + exercise: 'BaseExercise | int', + language: str, + max_age: Optional[int] = None, + ) -> str: + """Return a recently cached version without regenerating exercise HTML.""" + raw = cache.get(cls._key(exercise, modifiers=[language])) + if not isinstance(raw, tuple) or len(raw) != 2: + return '' + updated, data = raw + if updated is None or not isinstance(data, dict): + return '' + if max_age is not None: + expires = data.get('expires') or 0 + if time.time() - updated > max_age or (expires and time.time() > expires): + return '' + return data.get('exercise_version') or '' + def invalidate_instance(instance: 'CourseInstance') -> None: for module in instance.course_modules.all(): diff --git a/exercise/exercise_models.py b/exercise/exercise_models.py index e3bafa35e..a641a49d3 100644 --- a/exercise/exercise_models.py +++ b/exercise/exercise_models.py @@ -405,6 +405,7 @@ def load( cache = ExerciseCache(self, language, request, students, url_name, ordinal) page.head = cache.head() page.content = cache.content() + page.exercise_version = cache.exercise_version() page.is_loaded = True return page diff --git a/exercise/forms.py b/exercise/forms.py index 88d17b645..0a232dd78 100644 --- a/exercise/forms.py +++ b/exercise/forms.py @@ -21,6 +21,7 @@ class SubmissionCallbackForm(forms.Form): notify = forms.CharField(required=False) regrade_when_notification_seen = forms.BooleanField(required=False) grading_payload = forms.CharField(required=False) + exercise_version = forms.CharField(required=False) lti_launch_id = forms.CharField(required=False) lti_session_id = forms.CharField(required=False) error = forms.BooleanField(required=False) diff --git a/exercise/protocol/aplus.py b/exercise/protocol/aplus.py index 1d9224eba..202f0fb02 100644 --- a/exercise/protocol/aplus.py +++ b/exercise/protocol/aplus.py @@ -7,6 +7,7 @@ from lib.email_messages import email_course_error from lib.remote_page import RemotePage, RemotePageException +from ..cache.exercise import ExerciseCache from .exercise_page import ExercisePage from lti_tool.utils import send_lti_points @@ -64,6 +65,14 @@ def load_feedback_page(request, url, exercise, submission, no_penalties=False): if page.is_loaded: submission.feedback = page.clean_content + if page.exercise_version: + language = submission.lang or exercise.course_instance.default_language + cached_version = ExerciseCache.cached_exercise_version(exercise, language) + if cached_version and cached_version != page.exercise_version: + ExerciseCache.invalidate(exercise, modifiers=[language]) + if not isinstance(submission.meta_data, dict): + submission.meta_data = {} + submission.meta_data['exercise_version'] = page.exercise_version if page.is_accepted: submission.set_waiting() if page.is_graded: @@ -181,4 +190,5 @@ def parse_page_content( id_attrs_to_remove = ('exercise', 'chapter', 'aplus') page.content, page.clean_content = remote_page.element_or_body(element_selectors, id_attrs_to_remove) page.last_modified = remote_page.last_modified() + page.exercise_version = remote_page.meta("aplus-exercise-version") or "" page.expires = remote_page.expires() diff --git a/exercise/protocol/exercise_page.py b/exercise/protocol/exercise_page.py index 03adbb335..55c8178d9 100644 --- a/exercise/protocol/exercise_page.py +++ b/exercise/protocol/exercise_page.py @@ -23,6 +23,7 @@ def __init__(self, exercise): self.content = "" self.clean_content = "" self.last_modified = "" + self.exercise_version = "" # Content hash supplied by MOOC-Grader self.expires = 0 self.meta = { "title": exercise.name, diff --git a/exercise/submission_models.py b/exercise/submission_models.py index 403e8516b..2a6a2c5c4 100644 --- a/exercise/submission_models.py +++ b/exercise/submission_models.py @@ -252,6 +252,7 @@ def create_from_post(self, exercise, submitters, request): meta_data_dict = json.loads(request.POST.get('__aplus__', '{}')) except json.JSONDecodeError as exc: raise ValueError("The content of the field __aplus__ is not valid json") from exc + meta_data_dict.pop('exercise_version', None) if 'lang' not in meta_data_dict: meta_data_dict['lang'] = get_language() diff --git a/exercise/tests_versioning.py b/exercise/tests_versioning.py new file mode 100644 index 000000000..96e645d40 --- /dev/null +++ b/exercise/tests_versioning.py @@ -0,0 +1,238 @@ +from time import time +from unittest.mock import Mock, patch + +from django.core.cache import cache +from django.test import SimpleTestCase +from django.utils.translation import override + +from lib.remote_page import RemotePageNotModified +from .async_views import _post_async_submission +from .cache.exercise import ExerciseCache +from .protocol.aplus import load_feedback_page +from .views import ExerciseView + + +class ExerciseCacheVersionTest(SimpleTestCase): + def tearDown(self): + cache.clear() + + def test_versionless_cache_entry_forces_full_response(self): + exercise = Mock() + page = Mock( + content="new content", + head="", + last_modified="new timestamp", + exercise_version="new version", + expires=time() + 60, + is_loaded=True, + ) + + def load_page(*args, last_modified=None): + if last_modified: + raise RemotePageNotModified(time() + 60) + return page + + exercise.load_page.side_effect = load_page + exercise_cache = ExerciseCache.__new__(ExerciseCache) + exercise_cache.load_args = ["en", Mock(), [], "exercise", None] + old_data = { + "head": "", + "content": b"old content", + "last_modified": "old timestamp", + "expires": time() + 60, + } + + data = exercise_cache._generate_data(exercise, old_data) + + self.assertEqual(data["exercise_version"], "new version") + self.assertIsNone(exercise.load_page.call_args.kwargs["last_modified"]) + + def test_recent_randomized_version_is_reused(self): + cache.set( + ExerciseCache._key(123, modifiers=["en"]), + (time(), {"exercise_version": "version", "expires": 0}), + ) + + version = ExerciseCache.cached_exercise_version( + 123, + "en", + max_age=ExerciseCache.VERSION_MAX_AGE, + ) + + self.assertEqual(version, "version") + + def test_expired_nonrandomized_version_is_refreshed(self): + cache.set( + ExerciseCache._key(123, modifiers=["en"]), + (time(), {"exercise_version": "old", "expires": time() - 1}), + ) + + version = ExerciseCache.cached_exercise_version( + 123, + "en", + max_age=ExerciseCache.VERSION_MAX_AGE, + ) + + self.assertEqual(version, "") + + def test_old_nonrandomized_version_is_refreshed_before_long_expiry(self): + cache.set( + ExerciseCache._key(123, modifiers=["en"]), + ( + time() - ExerciseCache.VERSION_MAX_AGE - 1, + {"exercise_version": "old", "expires": time() + 60 * 60}, + ), + ) + + version = ExerciseCache.cached_exercise_version( + 123, + "en", + max_age=ExerciseCache.VERSION_MAX_AGE, + ) + + self.assertEqual(version, "") + + +class FeedbackVersionTest(SimpleTestCase): + def test_grader_response_version_is_stored_and_invalidates_old_page(self): + exercise = Mock() + exercise.course_instance.id = 1 + exercise.course_instance.default_language = "en" + exercise.course_instance.visible_to_students = False + submission = Mock() + submission.lang = "en" + submission.meta_data = {"lang": "en"} + submission.get_post_parameters.return_value = ({}, {}) + + def set_page_data(page, remote_page, parsed_exercise): + page.is_loaded = True + page.is_rejected = True + page.clean_content = "feedback" + page.exercise_version = "new version" + + with ( + patch("exercise.protocol.aplus.RemotePage"), + patch("exercise.protocol.aplus.parse_page_content", side_effect=set_page_data), + patch.object(ExerciseCache, "cached_exercise_version", return_value="old version"), + patch.object(ExerciseCache, "invalidate") as invalidate, + ): + load_feedback_page(Mock(), "https://grader.example/exercise", exercise, submission) + + self.assertEqual(submission.meta_data["exercise_version"], "new version") + invalidate.assert_called_once_with(exercise, modifiers=["en"]) + submission.save.assert_called_once_with() + + def test_async_grader_response_version_is_stored(self): + exercise = Mock() + exercise.course_instance.default_language = "en" + exercise.course_instance.visible_to_students = False + submission = Mock( + lang="en", + lti_launch_id=None, + meta_data={"lang": "en"}, + ) + request = Mock() + request.POST = { + "points": "1", + "max_points": "1", + "feedback": "feedback", + "exercise_version": "new version", + } + + with ( + patch.object(ExerciseCache, "cached_exercise_version", return_value="old version"), + patch.object(ExerciseCache, "invalidate") as invalidate, + ): + result = _post_async_submission(request, exercise, submission) + + self.assertTrue(result["success"]) + self.assertEqual(submission.meta_data["exercise_version"], "new version") + invalidate.assert_called_once_with(exercise, modifiers=["en"]) + submission.save.assert_called_once_with() + + +class SubmissionCompatibilityTest(SimpleTestCase): + def setUp(self): + self.view = ExerciseView() + self.view.exercise = Mock() + self.view.post_url_name = "exercise" + self.request = Mock() + self.students = [] + + @override("en") + def test_legacy_submission_is_kept(self): + submission = Mock(lang="en", meta_data={}) + self.view._get_current_exercise_version = Mock() + + compatible = self.view._submission_is_compatible( + self.request, + self.students, + submission, + ) + + self.assertTrue(compatible) + self.view._get_current_exercise_version.assert_not_called() + + @override("en") + def test_language_change_is_incompatible(self): + submission = Mock(lang="fi", meta_data={}) + + compatible = self.view._submission_is_compatible( + self.request, + self.students, + submission, + ) + + self.assertFalse(compatible) + + @override("en") + def test_version_change_is_incompatible(self): + submission = Mock(lang="en", meta_data={"exercise_version": "old"}) + self.view._get_current_exercise_version = Mock(return_value="new") + + compatible = self.view._submission_is_compatible( + self.request, + self.students, + submission, + ) + + self.assertFalse(compatible) + + @override("en") + def test_delayed_feedback_uses_default_form_after_change(self): + submission = Mock(lang="en", meta_data={"exercise_version": "old"}) + queryset = self.view.exercise.get_submissions_for_student.return_value + queryset.order_by.return_value.first.return_value = submission + self.view.exercise.is_submittable = True + self.view.exercise.load.return_value = default_page = Mock() + self.view.profile = Mock() + self.view.feedback_revealed = False + self.view._get_current_exercise_version = Mock(return_value="new") + self.request.GET = {"submission": "true"} + + page = self.view.get_page(self.request, self.students) + + self.assertIs(page, default_page) + submission.load.assert_not_called() + + @override("en") + def test_version_check_reuses_loaded_default_form(self): + submission = Mock(lang="en", meta_data={"exercise_version": "old"}) + queryset = self.view.exercise.get_submissions_for_student.return_value + queryset.order_by.return_value.first.return_value = submission + default_page = Mock(exercise_version="new") + self.view.exercise.is_submittable = True + self.view.exercise.load.return_value = default_page + self.view.profile = Mock() + self.view.feedback_revealed = True + self.request.GET = {"submission": "true"} + + with patch.object(ExerciseCache, "cached_exercise_version", return_value=""): + page = self.view.get_page(self.request, self.students) + + self.assertIs(page, default_page) + self.view.exercise.load.assert_called_once_with( + self.request, + self.students, + url_name="exercise", + ) diff --git a/exercise/views.py b/exercise/views.py index 69684e3d2..2a754b056 100644 --- a/exercise/views.py +++ b/exercise/views.py @@ -25,6 +25,7 @@ from lib.remote_page import RemotePageNotFound, request_for_response from lib.viewbase import BaseFormView, BaseRedirectMixin, BaseView from userprofile.models import UserProfile +from .cache.exercise import ExerciseCache from .cache.points import CachedPoints, ModulePoints, ExercisePoints from .models import BaseExercise, LearningObject, LearningObjectDisplay from .protocol.exercise_page import ExercisePage @@ -296,6 +297,53 @@ def submission_check(self, request=None): ) return submission_status, submission_allowed, issues, students + def _get_current_exercise_version(self, request: HttpRequest, students: List[UserProfile]) -> Optional[str]: + language = get_language() + version = ExerciseCache.cached_exercise_version( + self.exercise, + language, + max_age=ExerciseCache.VERSION_MAX_AGE, + ) + if version: + return version + page = self._load_default_page(request, students) + return page.exercise_version or None + + def _load_default_page(self, request: HttpRequest, students: List[UserProfile]) -> ExercisePage: + page = getattr(self, '_default_exercise_page', None) + if page is None: + page = self.exercise.load( + request, + students, + url_name=self.post_url_name, + ) + self._default_exercise_page = page + return page + + def _submission_is_compatible( + self, + request: HttpRequest, + students: List[UserProfile], + submission: Submission, + ) -> bool: + submission_language = submission.lang + if ( + submission_language is not None + and submission_language.lower() != get_language().lower() + ): + return False + + try: + submission_version = submission.meta_data.get('exercise_version') + except AttributeError: + submission_version = None + + # Submissions made before version stamping can not be compared reliably. + if not submission_version: + return True + current_version = self._get_current_exercise_version(request, students) + return current_version is None or submission_version == current_version + def get_page(self, request: HttpRequest, students: List[UserProfile]) -> ExercisePage: """ Determines which page should be displayed for this exercise: @@ -330,13 +378,11 @@ def get_page(self, request: HttpRequest, students: List[UserProfile]) -> Exercis .first() ) if submission: + if not self._submission_is_compatible(request, students, submission): + return self._load_default_page(request, students) if self.feedback_revealed and not submission.feedback: # We cannot show feedback (grader service was probably down), so load a blank exercise page - return self.exercise.load( - request, - students, - url_name=self.post_url_name, - ) + return self._load_default_page(request, students) if self.feedback_revealed: page = ExercisePage(self.exercise) page.content = submission.feedback @@ -346,11 +392,7 @@ def get_page(self, request: HttpRequest, students: List[UserProfile]) -> Exercis return submission.load(request, feedback_revealed=False) # In every other case, load a blank exercise page - return self.exercise.load( - request, - students, - url_name=self.post_url_name, - ) + return self._load_default_page(request, students) def _load_exercisecollection(self, request, submission_disabled):