From c3d25e1e616625ff853b396e1392b1a6d0bb1698 Mon Sep 17 00:00:00 2001 From: Calen Pennington Date: Fri, 25 Oct 2013 11:53:35 -0400 Subject: [PATCH] Prevent unbounded nesting of lms field_datas Previously, whenever a XModule was created from a XDescriptor, we created another level of nesting of FieldData objects. This change prevents that nesting. [TKTS-393] --- cms/djangoapps/contentstore/views/preview.py | 4 +-- lms/djangoapps/courseware/module_render.py | 4 +-- lms/djangoapps/courseware/tests/__init__.py | 4 +-- lms/djangoapps/courseware/tests/tests.py | 24 ++++++++++++++ lms/xblock/field_data.py | 35 ++++++++++++-------- 5 files changed, 52 insertions(+), 19 deletions(-) diff --git a/cms/djangoapps/contentstore/views/preview.py b/cms/djangoapps/contentstore/views/preview.py index 2d9a66be1e..b188036492 100644 --- a/cms/djangoapps/contentstore/views/preview.py +++ b/cms/djangoapps/contentstore/views/preview.py @@ -16,7 +16,7 @@ from xmodule.modulestore.django import modulestore from xmodule.x_module import ModuleSystem from xblock.runtime import DbModel -from lms.xblock.field_data import lms_field_data +from lms.xblock.field_data import LmsFieldData from util.sandboxing import can_execute_unsafe_code @@ -149,7 +149,7 @@ def load_preview_module(request, preview_id, descriptor): student_data = DbModel(SessionKeyValueStore(request)) descriptor.bind_for_student( preview_module_system(request, preview_id, descriptor), - lms_field_data(descriptor._field_data, student_data), # pylint: disable=protected-access + LmsFieldData(descriptor._field_data, student_data), # pylint: disable=protected-access ) return descriptor diff --git a/lms/djangoapps/courseware/module_render.py b/lms/djangoapps/courseware/module_render.py index f4b1c64a9f..65c72cdbe1 100644 --- a/lms/djangoapps/courseware/module_render.py +++ b/lms/djangoapps/courseware/module_render.py @@ -38,7 +38,7 @@ from xblock.runtime import KeyValueStore from xblock.fields import Scope from util.sandboxing import can_execute_unsafe_code from util.json_request import JsonResponse -from lms.xblock.field_data import lms_field_data +from lms.xblock.field_data import LmsFieldData log = logging.getLogger(__name__) @@ -220,7 +220,7 @@ def get_module_for_descriptor_internal(user, descriptor, field_data_cache, cours return None student_data = DbModel(DjangoKeyValueStore(field_data_cache)) - descriptor._field_data = lms_field_data(descriptor._field_data, student_data) + descriptor._field_data = LmsFieldData(descriptor._field_data, student_data) # Setup system context for module instance ajax_url = reverse( diff --git a/lms/djangoapps/courseware/tests/__init__.py b/lms/djangoapps/courseware/tests/__init__.py index 91be45516d..3fcfd42324 100644 --- a/lms/djangoapps/courseware/tests/__init__.py +++ b/lms/djangoapps/courseware/tests/__init__.py @@ -21,7 +21,7 @@ from xmodule.modulestore import Location from xmodule.modulestore.django import modulestore from xmodule.modulestore.tests.factories import CourseFactory, ItemFactory from xmodule.modulestore.tests.django_utils import ModuleStoreTestCase -from lms.xblock.field_data import lms_field_data +from lms.xblock.field_data import LmsFieldData @override_settings(MODULESTORE=TEST_DATA_MIXED_MODULESTORE) @@ -107,7 +107,7 @@ class BaseTestXmodule(ModuleStoreTestCase): field_data = {} field_data.update(self.MODEL_DATA) student_data = DictFieldData(field_data) - self.item_descriptor._field_data = lms_field_data(self.item_descriptor._field_data, student_data) + self.item_descriptor._field_data = LmsFieldData(self.item_descriptor._field_data, student_data) self.item_descriptor.xmodule_runtime = self.new_module_runtime() self.item_module = self.item_descriptor diff --git a/lms/djangoapps/courseware/tests/tests.py b/lms/djangoapps/courseware/tests/tests.py index 2cb80b6eda..1ab22060ce 100644 --- a/lms/djangoapps/courseware/tests/tests.py +++ b/lms/djangoapps/courseware/tests/tests.py @@ -2,6 +2,8 @@ Test for LMS courseware app. """ import mock +from mock import Mock +from unittest import TestCase from django.core.urlresolvers import reverse from django.test.utils import override_settings @@ -18,6 +20,7 @@ from courseware.tests.modulestore_config import TEST_DATA_DIR, \ TEST_DATA_MONGO_MODULESTORE, \ TEST_DATA_DRAFT_MONGO_MODULESTORE, \ TEST_DATA_MIXED_MODULESTORE +from lms.xblock.field_data import LmsFieldData class ActivateLoginTest(LoginEnrollmentTestCase): @@ -183,3 +186,24 @@ class TestDraftModuleStore(ModuleStoreTestCase): # test success is just getting through the above statement. # The bug was that 'course_id' argument was # not allowed to be passed in (i.e. was throwing exception) + + +class TestLmsFieldData(TestCase): + """ + Tests of the LmsFieldData class + """ + def test_lms_field_data_wont_nest(self): + # Verify that if an LmsFieldData is passed into LmsFieldData as the + # authored_data, that it doesn't produced a nested field data. + # + # This fixes a bug where re-use of the same descriptor for many modules + # would cause more and more nesting, until the recursion depth would be + # reached on any attribute access + + # pylint: disable=protected-access + base_authored = Mock() + base_student = Mock() + first_level = LmsFieldData(base_authored, base_student) + second_level = LmsFieldData(first_level, base_student) + self.assertEquals(second_level._authored_data, first_level._authored_data) + self.assertNotIsInstance(second_level._authored_data, LmsFieldData) diff --git a/lms/xblock/field_data.py b/lms/xblock/field_data.py index 6073a86863..03b3deafac 100644 --- a/lms/xblock/field_data.py +++ b/lms/xblock/field_data.py @@ -6,21 +6,30 @@ from xblock.field_data import ReadOnlyFieldData, SplitFieldData from xblock.fields import Scope -def lms_field_data(authored_data, student_data): +class LmsFieldData(SplitFieldData): """ - Returns a new :class:`~xblock.field_data.FieldData` that + A :class:`~xblock.field_data.FieldData` that reads all UserScope.ONE and UserScope.ALL fields from `student_data` and all UserScope.NONE fields from `authored_data`. It also prevents writing to `authored_data`. """ - authored_data = ReadOnlyFieldData(authored_data) - return SplitFieldData({ - Scope.content: authored_data, - Scope.settings: authored_data, - Scope.parent: authored_data, - Scope.children: authored_data, - Scope.user_state_summary: student_data, - Scope.user_state: student_data, - Scope.user_info: student_data, - Scope.preferences: student_data, - }) + def __init__(self, authored_data, student_data): + # Make sure that we don't repeatedly nest LmsFieldData instances + if isinstance(authored_data, LmsFieldData): + authored_data = authored_data._authored_data + else: + authored_data = ReadOnlyFieldData(authored_data) + + self._authored_data = authored_data + self._student_data = student_data + + super(LmsFieldData, self).__init__({ + Scope.content: authored_data, + Scope.settings: authored_data, + Scope.parent: authored_data, + Scope.children: authored_data, + Scope.user_state_summary: student_data, + Scope.user_state: student_data, + Scope.user_info: student_data, + Scope.preferences: student_data, + })