Merge pull request #1504 from cpennington/field-data-nesting-hotfix

Prevent unbounded nesting of lms field_datas

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
 

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(

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

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)

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,
+        })