prevent overly nesting _field_data (TNL-2050)

cms/djangoapps/contentstore/views/preview.py

@@ -214,13 +214,18 @@ def _load_preview_module(request, descriptor):
     """
     student_data = KvsFieldData(SessionKeyValueStore(request))
     if _has_author_view(descriptor):
-        field_data = CmsFieldData(descriptor._field_data, student_data)  # pylint: disable=protected-access
+        wrapper = partial(CmsFieldData, student_data=student_data)
     else:
-        field_data = LmsFieldData(descriptor._field_data, student_data)  # pylint: disable=protected-access
+        wrapper = partial(LmsFieldData, student_data=student_data)
+
+    # wrap the _field_data upfront to pass to _preview_module_system
+    wrapped_field_data = wrapper(descriptor._field_data)  # pylint: disable=protected-access
+    preview_runtime = _preview_module_system(request, descriptor, wrapped_field_data)
+
     descriptor.bind_for_student(
-        _preview_module_system(request, descriptor, field_data),
-        field_data,
-        request.user.id
+        preview_runtime,
+        request.user.id,
+        [wrapper]
     )
     return descriptor
 

common/lib/xmodule/xmodule/modulestore/tests/django_utils.py

@@ -15,6 +15,7 @@ from django.test import TestCase
 from django.test.utils import override_settings
 from request_cache.middleware import RequestCache
 
+from courseware.field_overrides import OverrideFieldData  # pylint: disable=import-error
 from xmodule.contentstore.django import _CONTENTSTORE
 from xmodule.modulestore import ModuleStoreEnum
 from xmodule.modulestore.django import modulestore, clear_existing_modulestores
@@ -261,6 +262,11 @@ class ModuleStoreTestCase(TestCase):
         self.addCleanup(XMODULE_FACTORY_LOCK.disable)
         XMODULE_FACTORY_LOCK.enable()
 
+        # When testing CCX, we should make sure that
+        # OverrideFieldData.provider_classes is always reset to `None` so
+        # that they're recalculated for every test
+        OverrideFieldData.provider_classes = None
+
         super(ModuleStoreTestCase, self).setUp()
 
         self.store = modulestore()

common/lib/xmodule/xmodule/modulestore/xml.py

@@ -14,6 +14,7 @@ from importlib import import_module
 from lxml import etree
 from path import path
 from contextlib import contextmanager
+from lazy import lazy
 
 from xmodule.error_module import ErrorDescriptor
 from xmodule.errortracker import make_error_tracker, exc_info_to_str
@@ -933,6 +934,9 @@ class LibraryXMLModuleStore(XMLModuleStore):
         here manually.
         """
         init_dict = {key: getattr(library_descriptor, key) for key in library_descriptor.fields.keys()}
+        # if set, invalidate '_unwrapped_field_data' so it will be reset
+        # the next time it will be called
+        lazy.invalidate(library_descriptor, '_unwrapped_field_data')
         # pylint: disable=protected-access
         library_descriptor._field_data = inheriting_field_data(InheritanceKeyValueStore(init_dict))
 

common/lib/xmodule/xmodule/tests/__init__.py

@@ -126,7 +126,7 @@ def get_test_system(course_id=SlashSeparatedCourseKey('org', 'course', 'run')):
         # So, bind to the same one as the current descriptor.
         module_system.descriptor_runtime = descriptor._runtime  # pylint: disable=protected-access
 
-        descriptor.bind_for_student(module_system, descriptor._field_data, user.id)
+        descriptor.bind_for_student(module_system, user.id)
 
         return descriptor
 

common/lib/xmodule/xmodule/tests/test_import.py

@@ -281,7 +281,13 @@ class ImportTestCase(BaseCourseTestCase):
             due=from_date_string, org=ORG, course=COURSE, url_name=url_name, unicorn_color=unicorn_color
         )
         descriptor = system.process_xml(start_xml)
+
+        # pylint: disable=protected-access
+        original_unwrapped = descriptor._unwrapped_field_data
         LibraryXMLModuleStore.patch_descriptor_kvs(descriptor)
+        # '_unwrapped_field_data' is reset in `patch_descriptor_kvs`
+        # pylint: disable=protected-access
+        self.assertIsNot(original_unwrapped, descriptor._unwrapped_field_data)
         compute_inherited_metadata(descriptor)
         # Check the course module, since it has inheritance
         descriptor = descriptor.get_children()[0]

common/lib/xmodule/xmodule/tests/test_library_content.py

@@ -60,7 +60,7 @@ class LibraryContentTest(MixedSplitTestCase):
             sub_module_system = get_test_system(course_id=module.location.course_key)
             sub_module_system.get_module = get_module
             sub_module_system.descriptor_runtime = descriptor._runtime  # pylint: disable=protected-access
-            descriptor.bind_for_student(sub_module_system, descriptor._field_data, self.user_id)  # pylint: disable=protected-access
+            descriptor.bind_for_student(sub_module_system, self.user_id)
             return descriptor
 
         module_system.get_module = get_module

common/lib/xmodule/xmodule/tests/test_split_test_module.py

@@ -100,7 +100,6 @@ class SplitTestModuleTest(XModuleXmlImportTest, PartitionTestCase):
         self.split_test_module = self.course_sequence.get_children()[0]
         self.split_test_module.bind_for_student(
             self.module_system,
-            self.split_test_module._field_data,  # pylint: disable=protected-access
             user.id
         )
 

common/lib/xmodule/xmodule/x_module.py

@@ -16,6 +16,7 @@ from pkg_resources import (
 )
 from webob import Response
 from webob.multidict import MultiDict
+from lazy import lazy
 
 from xblock.core import XBlock, XBlockAside
 from xblock.fields import (
@@ -361,6 +362,14 @@ class XModuleMixin(XModuleFields, XBlockMixin):
         self.save()
         return self._field_data._kvs  # pylint: disable=protected-access
 
+    @lazy
+    def _unwrapped_field_data(self):
+        """
+        This property hold the value _field_data here before we wrap it in
+        the LmsFieldData or OverrideFieldData classes.
+        """
+        return self._field_data
+
     def get_explicitly_set_fields_by_scope(self, scope=Scope.content):
         """
         Get a dictionary of the fields for the given scope which are set explicitly on this xblock. (Including
@@ -542,14 +551,17 @@ class XModuleMixin(XModuleFields, XBlockMixin):
         """
         return None
 
-    def bind_for_student(self, xmodule_runtime, field_data, user_id):
+    def bind_for_student(self, xmodule_runtime, user_id, wrappers=None):
         """
         Set up this XBlock to act as an XModule instead of an XModuleDescriptor.
 
         Arguments:
             xmodule_runtime (:class:`ModuleSystem'): the runtime to use when accessing student facing methods
-            field_data (:class:`FieldData`): The :class:`FieldData` to use for all subsequent data access
             user_id: The user_id to set in scope_ids
+            wrappers: These are a list functions that put a wrapper, such as
+                      LmsFieldData or OverrideFieldData, around the field_data.
+                      Note that the functions will be applied in the order in
+                      which they're listed. So [f1, f2] -> f2(f1(field_data))
         """
         # pylint: disable=attribute-defined-outside-init
 
@@ -578,7 +590,15 @@ class XModuleMixin(XModuleFields, XBlockMixin):
 
         # Set the new xmodule_runtime and field_data (which are user-specific)
         self.xmodule_runtime = xmodule_runtime
-        self._field_data = field_data
+
+        if wrappers is None:
+            wrappers = []
+
+        wrapped_field_data = self._unwrapped_field_data
+        for wrapper in wrappers:
+            wrapped_field_data = wrapper(wrapped_field_data)
+
+        self._field_data = wrapped_field_data
 
     @property
     def non_editable_metadata_fields(self):
@@ -1237,7 +1257,7 @@ class MetricsMixin(object):
             )
 
 
-class DescriptorSystem(MetricsMixin, ConfigurableFragmentWrapper, Runtime):  # pylint: disable=abstract-method
+class DescriptorSystem(ConfigurableFragmentWrapper, Runtime):  # pylint: disable=abstract-method
     """
     Base class for :class:`Runtime`s to be used with :class:`XModuleDescriptor`s
     """
@@ -1483,7 +1503,7 @@ class XMLParsingSystem(DescriptorSystem):
                     setattr(xblock, field.name, field_value)
 
 
-class ModuleSystem(MetricsMixin, ConfigurableFragmentWrapper, Runtime):  # pylint: disable=abstract-method
+class ModuleSystem(ConfigurableFragmentWrapper, Runtime):  # pylint: disable=abstract-method
     """
     This is an abstraction such that x_modules can function independent
     of the courseware (e.g. import into other types of courseware, LMS,

lms/djangoapps/courseware/module_render.py

@@ -497,15 +497,16 @@ def get_module_system_for_user(user, field_data_cache,
             user_location=user_location,
             request_token=request_token
         )
-        # rebinds module to a different student.  We'll change system, student_data, and scope_ids
-        authored_data = OverrideFieldData.wrap(
-            real_user, module.descriptor._field_data  # pylint: disable=protected-access
-        )
+
         module.descriptor.bind_for_student(
             inner_system,
-            LmsFieldData(authored_data, inner_student_data),
             real_user.id,
+            [
+                partial(OverrideFieldData.wrap, real_user),
+                partial(LmsFieldData, student_data=inner_student_data),
+            ],
         )
+
         module.descriptor.scope_ids = (
             module.descriptor.scope_ids._replace(user_id=real_user.id)  # pylint: disable=protected-access
         )
@@ -692,8 +693,15 @@ def get_module_for_descriptor_internal(user, descriptor, field_data_cache, cours
         request_token=request_token
     )
 
-    authored_data = OverrideFieldData.wrap(user, descriptor._field_data)  # pylint: disable=protected-access
-    descriptor.bind_for_student(system, LmsFieldData(authored_data, student_data), user.id)
+    descriptor.bind_for_student(
+        system,
+        user.id,
+        [
+            partial(OverrideFieldData.wrap, user),
+            partial(LmsFieldData, student_data=student_data),
+        ],
+    )
+
     descriptor.scope_ids = descriptor.scope_ids._replace(user_id=user.id)  # pylint: disable=protected-access
 
     # Do not check access when it's a noauth request.

lms/djangoapps/courseware/tests/test_module_render.py

@@ -12,6 +12,7 @@ from django.http import Http404, HttpResponse
 from django.core.urlresolvers import reverse
 from django.conf import settings
 from django.test.client import RequestFactory
+from django.test.utils import override_settings
 from django.contrib.auth.models import AnonymousUser
 from mock import MagicMock, patch, Mock
 from opaque_keys.edx.keys import UsageKey, CourseKey
@@ -27,6 +28,7 @@ from xblock.fragment import Fragment
 from capa.tests.response_xml_factory import OptionResponseXMLFactory
 from courseware import module_render as render
 from courseware.courses import get_course_with_access, course_image_url, get_course_info_section
+from courseware.field_overrides import OverrideFieldData
 from courseware.model_data import FieldDataCache
 from courseware.module_render import hash_resource, get_module_for_descriptor
 from courseware.models import StudentModule
@@ -34,6 +36,7 @@ from courseware.tests.factories import StudentModuleFactory, UserFactory, Global
 from courseware.tests.tests import LoginEnrollmentTestCase
 from courseware.tests.test_submitting_problems import TestSubmittingProblems
 from lms.djangoapps.lms_xblock.runtime import quote_slashes
+from lms.djangoapps.lms_xblock.field_data import LmsFieldData
 from student.models import anonymous_id_for_user
 from xmodule.modulestore.tests.django_utils import (
     TEST_DATA_MIXED_TOY_MODULESTORE,
@@ -99,10 +102,15 @@ class ModuleRenderTestCase(ModuleStoreTestCase, LoginEnrollmentTestCase):
         self.dispatch = 'score_update'
 
         # Construct a 'standard' xqueue_callback url
-        self.callback_url = reverse('xqueue_callback', kwargs=dict(course_id=self.course_key.to_deprecated_string(),
-                                                                   userid=str(self.mock_user.id),
-                                                                   mod_id=self.mock_module.id,
-                                                                   dispatch=self.dispatch))
+        self.callback_url = reverse(
+            'xqueue_callback',
+            kwargs=dict(
+                course_id=self.course_key.to_deprecated_string(),
+                userid=str(self.mock_user.id),
+                mod_id=self.mock_module.id,
+                dispatch=self.dispatch
+            )
+        )
 
     def test_get_module(self):
         self.assertEqual(
@@ -249,6 +257,63 @@ class ModuleRenderTestCase(ModuleStoreTestCase, LoginEnrollmentTestCase):
         render.get_module_for_descriptor(self.mock_user, request, descriptor, field_data_cache, self.toy_course.id)
         render.get_module_for_descriptor(self.mock_user, request, descriptor, field_data_cache, self.toy_course.id)
 
+    @override_settings(FIELD_OVERRIDE_PROVIDERS=(
+        'ccx.overrides.CustomCoursesForEdxOverrideProvider',))
+    def test_rebind_different_users_ccx(self):
+        """
+        This tests the rebinding a descriptor to a student does not result
+        in overly nested _field_data when CCX is enabled.
+        """
+        request = self.request_factory.get('')
+        request.user = self.mock_user
+        course = CourseFactory()
+
+        descriptor = ItemFactory(category='html', parent=course)
+        field_data_cache = FieldDataCache(
+            [self.toy_course, descriptor], self.toy_course.id, self.mock_user
+        )
+
+        # grab what _field_data was originally set to
+        original_field_data = descriptor._field_data  # pylint: disable=protected-access, no-member
+
+        render.get_module_for_descriptor(
+            self.mock_user, request, descriptor, field_data_cache, self.toy_course.id
+        )
+
+        # check that _unwrapped_field_data is the same as the original
+        # _field_data, but now _field_data as been reset.
+        # pylint: disable=protected-access, no-member
+        self.assertIs(descriptor._unwrapped_field_data, original_field_data)
+        self.assertIsNot(descriptor._unwrapped_field_data, descriptor._field_data)
+
+        # now bind this module to a few other students
+        for user in [UserFactory(), UserFactory(), UserFactory()]:
+            render.get_module_for_descriptor(
+                user,
+                request,
+                descriptor,
+                field_data_cache,
+                self.toy_course.id
+            )
+
+        # _field_data should now be wrapped by LmsFieldData
+        # pylint: disable=protected-access, no-member
+        self.assertIsInstance(descriptor._field_data, LmsFieldData)
+
+        # the LmsFieldData should now wrap OverrideFieldData
+        self.assertIsInstance(
+            # pylint: disable=protected-access, no-member
+            descriptor._field_data._authored_data._source,
+            OverrideFieldData
+        )
+
+        # the OverrideFieldData should point to the original unwrapped field_data
+        self.assertIs(
+            # pylint: disable=protected-access, no-member
+            descriptor._field_data._authored_data._source.fallback,
+            descriptor._unwrapped_field_data
+        )
+
     def test_hash_resource(self):
         """
         Ensure that the resource hasher works and does not fail on unicode,