From 7c1438034be1e0d587c31db4c6bb9746ae0db2c2 Mon Sep 17 00:00:00 2001 From: Calen Pennington Date: Fri, 26 Sep 2014 17:15:38 -0400 Subject: [PATCH 01/20] Fix BulkAssertionMixin so that it actually asserts the truth/falsity of the contained assertions --- common/lib/xmodule/xmodule/tests/__init__.py | 3 +- .../xmodule/tests/test_bulk_assertions.py | 77 +++++++++++++++++++ 2 files changed, 79 insertions(+), 1 deletion(-) create mode 100644 common/lib/xmodule/xmodule/tests/test_bulk_assertions.py diff --git a/common/lib/xmodule/xmodule/tests/__init__.py b/common/lib/xmodule/xmodule/tests/__init__.py index a0107a5476..9581d4451d 100644 --- a/common/lib/xmodule/xmodule/tests/__init__.py +++ b/common/lib/xmodule/xmodule/tests/__init__.py @@ -193,7 +193,7 @@ class BulkAssertionManager(object): self._equal_actual.append((description, actual)) def run_assertions(self): - self._test_case.assertEqual(self._equal_expected, self._equal_actual) + super(BulkAssertionTest, self._test_case).assertEqual(self._equal_expected, self._equal_actual) class BulkAssertionTest(unittest.TestCase): @@ -224,6 +224,7 @@ class BulkAssertionTest(unittest.TestCase): self._manager.assertEqual(expected, actual, message) else: super(BulkAssertionTest, self).assertEqual(expected, actual, message) + assertEquals = assertEqual class CourseComparisonTest(BulkAssertionTest): diff --git a/common/lib/xmodule/xmodule/tests/test_bulk_assertions.py b/common/lib/xmodule/xmodule/tests/test_bulk_assertions.py new file mode 100644 index 0000000000..d796b6b546 --- /dev/null +++ b/common/lib/xmodule/xmodule/tests/test_bulk_assertions.py @@ -0,0 +1,77 @@ +import ddt +from xmodule.tests import BulkAssertionTest + +@ddt.ddt +class TestBulkAssertionTestCase(BulkAssertionTest): + + @ddt.data( + ('assertTrue', True), + ('assertFalse', False), + ('assertIs', 1, 1), + ('assertIsNot', 1, 2), + ('assertIsNone', None), + ('assertIsNotNone', 1), + ('assertIn', 1, (1, 2, 3)), + ('assertNotIn', 5, (1, 2, 3)), + ('assertIsInstance', 1, int), + ('assertNotIsInstance', '1', int), + ('assertRaises', KeyError, {}.__getitem__, '1'), + ) + @ddt.unpack + def test_passing_asserts_passthrough(self, assertion, *args): + getattr(self, assertion)(*args) + + + @ddt.data( + ('assertTrue', False), + ('assertFalse', True), + ('assertIs', 1, 2), + ('assertIsNot', 1, 1), + ('assertIsNone', 1), + ('assertIsNotNone', None), + ('assertIn', 5, (1, 2, 3)), + ('assertNotIn', 1, (1, 2, 3)), + ('assertIsInstance', '1', int), + ('assertNotIsInstance', 1, int), + ('assertRaises', ValueError, lambda: None), + ) + @ddt.unpack + def test_failing_asserts_passthrough(self, assertion, *args): + # Use super(BulkAssertionTest) to make sure we get un-adulturated assertions + with super(BulkAssertionTest, self).assertRaises(AssertionError): + getattr(self, assertion)(*args) + + def test_no_bulk_assert_equals(self): + # Use super(BulkAssertionTest) to make sure we get un-adulturated assertions + with super(BulkAssertionTest, self).assertRaises(AssertionError): + self.assertEquals(1, 2) + + @ddt.data( + 'assertEqual', 'assertEquals' + ) + def test_bulk_assert_equals(self, asserterFn): + asserter = getattr(self, asserterFn) + contextmanager = self.bulk_assertions() + + contextmanager.__enter__() + super(BulkAssertionTest, self).assertIsNotNone(self._manager) + asserter(1, 2) + asserter(3, 4) + + # Use super(BulkAssertionTest) to make sure we get un-adulturated assertions + with super(BulkAssertionTest, self).assertRaises(AssertionError): + contextmanager.__exit__(None, None, None) + + @ddt.data( + 'assertEqual', 'assertEquals' + ) + def test_bulk_assert_closed(self, asserterFn): + asserter = getattr(self, asserterFn) + + with self.bulk_assertions(): + asserter(1, 1) + asserter(2, 2) + + # Use super(BulkAssertionTest) to make sure we get un-adulturated assertions + with super(BulkAssertionTest, self).assertRaises(AssertionError): + asserter(1, 2) From 46c6a5de8a4f8bf433f969cf91ad79b37fccfe07 Mon Sep 17 00:00:00 2001 From: Calen Pennington Date: Fri, 12 Sep 2014 09:40:42 -0400 Subject: [PATCH 02/20] Bump version of PyContracts so that it will disable the 'check' function --- requirements/edx/base.txt | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/requirements/edx/base.txt b/requirements/edx/base.txt index 1ed769c6c6..87eed9125c 100644 --- a/requirements/edx/base.txt +++ b/requirements/edx/base.txt @@ -131,7 +131,7 @@ rednose==0.3 selenium==2.42.1 splinter==0.5.4 testtools==0.9.34 -PyContracts==1.6.4 +PyContracts==1.6.5 # Used for Segment.io analytics analytics-python==0.4.4 From ab75ef996367080d3476957a2c58f428ec0354f4 Mon Sep 17 00:00:00 2001 From: Calen Pennington Date: Fri, 12 Sep 2014 14:02:46 -0400 Subject: [PATCH 03/20] Teach mongo_connection to retry read operations in the face of AutoReconnect errors --- .../split_mongo/mongo_connection.py | 37 +++++++++++++++++++ 1 file changed, 37 insertions(+) diff --git a/common/lib/xmodule/xmodule/modulestore/split_mongo/mongo_connection.py b/common/lib/xmodule/xmodule/modulestore/split_mongo/mongo_connection.py index 73363b7cf0..0d4e1e09dc 100644 --- a/common/lib/xmodule/xmodule/modulestore/split_mongo/mongo_connection.py +++ b/common/lib/xmodule/xmodule/modulestore/split_mongo/mongo_connection.py @@ -3,7 +3,10 @@ Segregation of pymongo functions from the data modeling mechanisms for split mod """ import re import pymongo +import time from contracts import check +from functools import wraps +from pymongo.errors import AutoReconnect from xmodule.exceptions import HeartbeatFailure from xmodule.modulestore.split_mongo import BlockKey from datetime import tzinfo @@ -62,6 +65,32 @@ def structure_to_mongo(structure): return new_structure +def autoretry_read(wait=0.1, retries=5): + """ + Automatically retry a read-only method in the case of a pymongo + AutoReconnect exception. + + See http://emptysqua.re/blog/save-the-monkey-reliably-writing-to-mongodb/ + for a discussion of this technique. + """ + def decorate(fn): + @wraps(fn) + def wrapper(*args, **kwargs): + for attempt in xrange(retries): + try: + return fn(*args, **kwargs) + break + except AutoReconnect: + # Reraise if we failed on our last attempt + if attempt == retries - 1: + raise + + if wait: + time.sleep(wait) + return wrapper + return decorate + + class MongoConnection(object): """ Segregation of pymongo functions from the data modeling mechanisms for split modulestore. @@ -106,12 +135,14 @@ class MongoConnection(object): else: raise HeartbeatFailure("Can't connect to {}".format(self.database.name)) + @autoretry_read() def get_structure(self, key): """ Get the structure from the persistence mechanism whose id is the given key """ return structure_from_mongo(self.structures.find_one({'_id': key})) + @autoretry_read() def find_structures_by_id(self, ids): """ Return all structures that specified in ``ids``. @@ -121,6 +152,7 @@ class MongoConnection(object): """ return [structure_from_mongo(structure) for structure in self.structures.find({'_id': {'$in': ids}})] + @autoretry_read() def find_structures_derived_from(self, ids): """ Return all structures that were immediately derived from a structure listed in ``ids``. @@ -130,6 +162,7 @@ class MongoConnection(object): """ return [structure_from_mongo(structure) for structure in self.structures.find({'previous_version': {'$in': ids}})] + @autoretry_read() def find_ancestor_structures(self, original_version, block_key): """ Find all structures that originated from ``original_version`` that contain ``block_key``. @@ -155,6 +188,7 @@ class MongoConnection(object): """ self.structures.update({'_id': structure['_id']}, structure_to_mongo(structure), upsert=True) + @autoretry_read() def get_course_index(self, key, ignore_case=False): """ Get the course_index from the persistence mechanism whose id is the given key @@ -167,6 +201,7 @@ class MongoConnection(object): } ) + @autoretry_read() def find_matching_course_indexes(self, branch=None, search_targets=None): """ Find the course_index matching particular conditions. @@ -225,12 +260,14 @@ class MongoConnection(object): 'run': course_index['run'], }) + @autoretry_read() def get_definition(self, key): """ Get the definition from the persistence mechanism whose id is the given key """ return self.definitions.find_one({'_id': key}) + @autoretry_read() def find_matching_definitions(self, query): """ Find the definitions matching the query. Right now the query must be a legal mongo query From 8acef4c63795b6fce0905bd79b7bf898261daea0 Mon Sep 17 00:00:00 2001 From: Calen Pennington Date: Wed, 17 Sep 2014 14:08:45 -0400 Subject: [PATCH 04/20] Allow Timedelta and RelativeTime fields to handle from_json being passed timedelta objects --- common/lib/xmodule/xmodule/fields.py | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/common/lib/xmodule/xmodule/fields.py b/common/lib/xmodule/xmodule/fields.py index a1b7496bf8..514e58e28e 100644 --- a/common/lib/xmodule/xmodule/fields.py +++ b/common/lib/xmodule/xmodule/fields.py @@ -101,6 +101,10 @@ class Timedelta(Field): """ if time_str is None: return None + + if isinstance(time_str, datetime.timedelta): + return time_str + parts = TIMEDELTA_REGEX.match(time_str) if not parts: return @@ -182,6 +186,9 @@ class RelativeTime(Field): if not value: return datetime.timedelta(seconds=0) + if isinstance(value, datetime.timedelta): + return value + # We've seen serialized versions of float in this field if isinstance(value, float): return datetime.timedelta(seconds=value) From b57a5a6007d1527e87adfe17c3d924bd4ce368e3 Mon Sep 17 00:00:00 2001 From: Calen Pennington Date: Fri, 26 Sep 2014 16:43:09 -0400 Subject: [PATCH 05/20] Make xml exports more comparable by sorting and indenting json --- common/lib/xmodule/xmodule/contentstore/mongo.py | 2 +- common/lib/xmodule/xmodule/modulestore/xml_exporter.py | 6 +++--- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/common/lib/xmodule/xmodule/contentstore/mongo.py b/common/lib/xmodule/xmodule/contentstore/mongo.py index 44406c74a8..aad0c57d19 100644 --- a/common/lib/xmodule/xmodule/contentstore/mongo.py +++ b/common/lib/xmodule/xmodule/contentstore/mongo.py @@ -167,7 +167,7 @@ class MongoContentStore(ContentStore): policy.setdefault(asset['asset_key'].name, {})[attr] = value with open(assets_policy_file, 'w') as f: - json.dump(policy, f) + json.dump(policy, f, sort_keys=True, indent=4) def get_all_content_thumbnails_for_course(self, course_key): return self._get_all_content_for_course(course_key, get_thumbnails=True)[0] diff --git a/common/lib/xmodule/xmodule/modulestore/xml_exporter.py b/common/lib/xmodule/xmodule/modulestore/xml_exporter.py index bc31250ef9..2ff1f1da6d 100644 --- a/common/lib/xmodule/xmodule/modulestore/xml_exporter.py +++ b/common/lib/xmodule/xmodule/modulestore/xml_exporter.py @@ -97,12 +97,12 @@ def export_to_xml(modulestore, contentstore, course_key, root_dir, course_dir): # export the grading policy course_run_policy_dir = policies_dir.makeopendir(course.location.name) with course_run_policy_dir.open('grading_policy.json', 'w') as grading_policy: - grading_policy.write(dumps(course.grading_policy, cls=EdxJSONEncoder)) + grading_policy.write(dumps(course.grading_policy, cls=EdxJSONEncoder, sort_keys=True, indent=4)) # export all of the course metadata in policy.json with course_run_policy_dir.open('policy.json', 'w') as course_policy: policy = {'course/' + course.location.name: own_metadata(course)} - course_policy.write(dumps(policy, cls=EdxJSONEncoder)) + course_policy.write(dumps(policy, cls=EdxJSONEncoder, sort_keys=True, indent=4)) #### DRAFTS #### # xml backed courses don't support drafts! @@ -178,7 +178,7 @@ def _export_field_content(xblock_item, item_dir): # filename format: {dirname}.{field_name}.json with item_dir.open('{0}.{1}.{2}'.format(xblock_item.location.name, field_name, 'json'), 'w') as field_content_file: - field_content_file.write(dumps(module_data.get(field_name, {}), cls=EdxJSONEncoder)) + field_content_file.write(dumps(module_data.get(field_name, {}), cls=EdxJSONEncoder, sort_keys=True, indent=4)) def export_extra_content(export_fs, modulestore, course_key, category_type, dirname, file_suffix=''): From 3f065a5fefb709ee41ed6174d9b4388641e22b22 Mon Sep 17 00:00:00 2001 From: Calen Pennington Date: Fri, 26 Sep 2014 16:56:46 -0400 Subject: [PATCH 06/20] Export xml from the destination store during cross-modulestore import/export tests --- .../tests/test_cross_modulestore_import_export.py | 15 ++++++++++++--- 1 file changed, 12 insertions(+), 3 deletions(-) diff --git a/common/lib/xmodule/xmodule/modulestore/tests/test_cross_modulestore_import_export.py b/common/lib/xmodule/xmodule/modulestore/tests/test_cross_modulestore_import_export.py index 226fff8a1e..9245a62169 100644 --- a/common/lib/xmodule/xmodule/modulestore/tests/test_cross_modulestore_import_export.py +++ b/common/lib/xmodule/xmodule/modulestore/tests/test_cross_modulestore_import_export.py @@ -269,8 +269,8 @@ class CrossStoreXMLRoundtrip(CourseComparisonTest): with dest_content_builder.build() as dest_content: # Construct the modulestore for storing the second import (using the second contentstore) with dest_builder.build(dest_content) as dest_store: - source_course_key = source_store.make_course_key('source', 'course', 'key') - dest_course_key = dest_store.make_course_key('dest', 'course', 'key') + source_course_key = source_store.make_course_key('a', 'course', 'course') + dest_course_key = dest_store.make_course_key('a', 'course', 'course') import_from_xml( source_store, @@ -287,18 +287,27 @@ class CrossStoreXMLRoundtrip(CourseComparisonTest): source_content, source_course_key, self.export_dir, - 'exported_course', + 'exported_source_course', ) import_from_xml( dest_store, 'test_user', self.export_dir, + course_dirs=['exported_source_course'], static_content_store=dest_content, target_course_id=dest_course_key, create_new_course_if_not_present=True, ) + export_to_xml( + dest_store, + dest_content, + dest_course_key, + self.export_dir, + 'exported_dest_course', + ) + self.exclude_field(None, 'wiki_slug') self.exclude_field(None, 'xml_attributes') self.ignore_asset_key('_id') From d60117d7c25738ca22bc9bb0cbde82876cf58f5c Mon Sep 17 00:00:00 2001 From: Calen Pennington Date: Tue, 23 Sep 2014 16:24:51 -0400 Subject: [PATCH 07/20] Make Studio CoursePage objects generate the correct CourseLocator based on whether the DEFAULT_STORE is set or not --- common/test/acceptance/pages/studio/course_page.py | 13 +++++++++++-- 1 file changed, 11 insertions(+), 2 deletions(-) diff --git a/common/test/acceptance/pages/studio/course_page.py b/common/test/acceptance/pages/studio/course_page.py index fd64b77f26..2c9ffc1288 100644 --- a/common/test/acceptance/pages/studio/course_page.py +++ b/common/test/acceptance/pages/studio/course_page.py @@ -2,6 +2,8 @@ Base class for pages specific to a course in Studio. """ +import os +from opaque_keys.edx.locator import CourseLocator from bok_choy.page_object import PageObject from . import BASE_URL @@ -34,5 +36,12 @@ class CoursePage(PageObject): """ Construct a URL to the page within the course. """ - course_key = "{course_org}/{course_num}/{course_run}".format(**self.course_info) - return "/".join([BASE_URL, self.url_path, course_key]) + # TODO - is there a better way to make this agnostic to the underlying default module store? + default_store = os.environ.get('DEFAULT_STORE', 'draft') + course_key = CourseLocator( + self.course_info['course_org'], + self.course_info['course_num'], + self.course_info['course_run'], + deprecated=(default_store == 'draft') + ) + return "/".join([BASE_URL, self.url_path, unicode(course_key)]) From 234c18053d2bb098ea390ada393767e2f0e3df1a Mon Sep 17 00:00:00 2001 From: Calen Pennington Date: Wed, 17 Sep 2014 14:08:14 -0400 Subject: [PATCH 08/20] Inherit from JSONField rather than Field for json-based fields --- common/lib/xmodule/xmodule/fields.py | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/common/lib/xmodule/xmodule/fields.py b/common/lib/xmodule/xmodule/fields.py index 514e58e28e..1f75170075 100644 --- a/common/lib/xmodule/xmodule/fields.py +++ b/common/lib/xmodule/xmodule/fields.py @@ -2,7 +2,7 @@ import time import logging import re -from xblock.fields import Field +from xblock.fields import JSONField import datetime import dateutil.parser @@ -11,7 +11,7 @@ from pytz import UTC log = logging.getLogger(__name__) -class Date(Field): +class Date(JSONField): ''' Date fields know how to parse and produce json (iso) compatible formats. Converts to tz aware datetimes. ''' @@ -85,7 +85,7 @@ class Date(Field): TIMEDELTA_REGEX = re.compile(r'^((?P\d+?) day(?:s?))?(\s)?((?P\d+?) hour(?:s?))?(\s)?((?P\d+?) minute(?:s)?)?(\s)?((?P\d+?) second(?:s)?)?$') -class Timedelta(Field): +class Timedelta(JSONField): # Timedeltas are immutable, see http://docs.python.org/2/library/datetime.html#available-types MUTABLE = False @@ -133,7 +133,7 @@ class Timedelta(Field): return self.from_json(value) -class RelativeTime(Field): +class RelativeTime(JSONField): """ Field for start_time and end_time video module properties. From 687708c3d0a080a660ee7ba14a1b541c18751dbe Mon Sep 17 00:00:00 2001 From: Calen Pennington Date: Wed, 17 Sep 2014 14:13:20 -0400 Subject: [PATCH 09/20] Use InheritanceMixin in more modulestore tests --- .../tests/test_cross_modulestore_import_export.py | 15 ++++++++++++++- .../modulestore/tests/test_mixed_modulestore.py | 3 ++- 2 files changed, 16 insertions(+), 2 deletions(-) diff --git a/common/lib/xmodule/xmodule/modulestore/tests/test_cross_modulestore_import_export.py b/common/lib/xmodule/xmodule/modulestore/tests/test_cross_modulestore_import_export.py index 9245a62169..f3cdd87694 100644 --- a/common/lib/xmodule/xmodule/modulestore/tests/test_cross_modulestore_import_export.py +++ b/common/lib/xmodule/xmodule/modulestore/tests/test_cross_modulestore_import_export.py @@ -28,6 +28,8 @@ from xmodule.modulestore.xml_importer import import_from_xml from xmodule.modulestore.xml_exporter import export_to_xml from xmodule.modulestore.split_mongo.split_draft import DraftVersioningModuleStore from xmodule.modulestore.tests.mongo_connection import MONGO_PORT_NUM, MONGO_HOST +from xmodule.modulestore.inheritance import InheritanceMixin +from xmodule.x_module import XModuleMixin COMMON_DOCSTORE_CONFIG = { @@ -36,6 +38,9 @@ COMMON_DOCSTORE_CONFIG = { } +XBLOCK_MIXINS = (InheritanceMixin, XModuleMixin) + + class MemoryCache(object): """ This fits the metadata_inheritance_cache_subsystem interface used by @@ -95,6 +100,7 @@ class MongoModulestoreBuilder(object): render_template=repr, branch_setting_func=lambda: ModuleStoreEnum.Branch.draft_preferred, metadata_inheritance_cache_subsystem=MemoryCache(), + xblock_mixins=XBLOCK_MIXINS, ) modulestore.ensure_indexes() @@ -139,6 +145,7 @@ class VersioningModulestoreBuilder(object): doc_store_config, fs_root, render_template=repr, + xblock_mixins=XBLOCK_MIXINS, ) modulestore.ensure_indexes() @@ -189,7 +196,13 @@ class MixedModulestoreBuilder(object): # Generate a fake list of stores to give the already generated stores appropriate names stores = [{'NAME': name, 'ENGINE': 'This space deliberately left blank'} for name in names] - modulestore = MixedModuleStore(contentstore, self.mappings, stores, create_modulestore_instance=create_modulestore_instance) + modulestore = MixedModuleStore( + contentstore, + self.mappings, + stores, + create_modulestore_instance=create_modulestore_instance, + xblock_mixins=XBLOCK_MIXINS, + ) yield modulestore diff --git a/common/lib/xmodule/xmodule/modulestore/tests/test_mixed_modulestore.py b/common/lib/xmodule/xmodule/modulestore/tests/test_mixed_modulestore.py index 8b6f6e32c1..a355068b69 100644 --- a/common/lib/xmodule/xmodule/modulestore/tests/test_mixed_modulestore.py +++ b/common/lib/xmodule/xmodule/modulestore/tests/test_mixed_modulestore.py @@ -18,6 +18,7 @@ from uuid import uuid4 # TODO remove this import and the configuration -- xmodule should not depend on django! from django.conf import settings from xmodule.modulestore.edit_info import EditInfoMixin +from xmodule.modulestore.inheritance import InheritanceMixin if not settings.configured: settings.configure() @@ -58,7 +59,7 @@ class TestMixedModuleStore(unittest.TestCase): 'default_class': DEFAULT_CLASS, 'fs_root': DATA_DIR, 'render_template': RENDER_TEMPLATE, - 'xblock_mixins': (EditInfoMixin,) + 'xblock_mixins': (EditInfoMixin, InheritanceMixin), } DOC_STORE_CONFIG = { 'host': HOST, From 52beec887841b3b5aa132c9c14d967f7fb1d27f6 Mon Sep 17 00:00:00 2001 From: Calen Pennington Date: Fri, 12 Sep 2014 14:09:21 -0400 Subject: [PATCH 10/20] Switch inheritance in split-mongo over to using InheritingFieldData. --- .../contentstore/tests/test_import.py | 2 +- common/lib/xmodule/xmodule/fields.py | 3 + .../xmodule/modulestore/inheritance.py | 18 +++- .../split_mongo/caching_descriptor_system.py | 53 +++++---- .../xmodule/modulestore/split_mongo/split.py | 1 - .../split_mongo/split_mongo_kvs.py | 21 ++-- .../test_cross_modulestore_import_export.py | 1 + .../tests/test_mixed_modulestore.py | 9 +- .../tests/test_split_modulestore.py | 15 +++ .../xmodule/xmodule/partitions/partitions.py | 6 ++ common/lib/xmodule/xmodule/tests/__init__.py | 102 ++++++++++++++---- 11 files changed, 171 insertions(+), 60 deletions(-) diff --git a/cms/djangoapps/contentstore/tests/test_import.py b/cms/djangoapps/contentstore/tests/test_import.py index ac994880fc..d8f4dfe95c 100644 --- a/cms/djangoapps/contentstore/tests/test_import.py +++ b/cms/djangoapps/contentstore/tests/test_import.py @@ -56,7 +56,7 @@ class ContentStoreImportTest(ModuleStoreTestCase): target_course_id=target_course_id, create_new_course_if_not_present=create_new_course_if_not_present, ) - course_id = SlashSeparatedCourseKey('edX', 'test_import_course', '2012_Fall') + course_id = module_store.make_course_key('edX', 'test_import_course', '2012_Fall') course = module_store.get_course(course_id) self.assertIsNotNone(course) diff --git a/common/lib/xmodule/xmodule/fields.py b/common/lib/xmodule/xmodule/fields.py index 1f75170075..93618be63c 100644 --- a/common/lib/xmodule/xmodule/fields.py +++ b/common/lib/xmodule/xmodule/fields.py @@ -116,6 +116,9 @@ class Timedelta(JSONField): return datetime.timedelta(**time_params) def to_json(self, value): + if value is None: + return None + values = [] for attr in ('days', 'hours', 'minutes', 'seconds'): cur_value = getattr(value, attr, 0) diff --git a/common/lib/xmodule/xmodule/modulestore/inheritance.py b/common/lib/xmodule/xmodule/modulestore/inheritance.py index 1f6d3add8b..ecab07facd 100644 --- a/common/lib/xmodule/xmodule/modulestore/inheritance.py +++ b/common/lib/xmodule/xmodule/modulestore/inheritance.py @@ -214,11 +214,19 @@ class InheritingFieldData(KvsFieldData): """ The default for an inheritable name is found on a parent. """ - if name in self.inheritable_names and block.parent is not None: - parent = block.get_parent() - if parent: - return getattr(parent, name) - super(InheritingFieldData, self).default(block, name) + if name in self.inheritable_names: + # Walk up the content tree to find the first ancestor + # that this field is set on. Use the field from the current + # block so that if it has a different default than the root + # node of the tree, the block's default will be used. + field = block.fields[name] + ancestor = block.get_parent() + while ancestor is not None: + if field.is_set_on(ancestor): + return field.read_json(ancestor) + else: + ancestor = ancestor.get_parent() + return super(InheritingFieldData, self).default(block, name) def inheriting_field_data(kvs): diff --git a/common/lib/xmodule/xmodule/modulestore/split_mongo/caching_descriptor_system.py b/common/lib/xmodule/xmodule/modulestore/split_mongo/caching_descriptor_system.py index a1526c0d13..ae92cd476b 100644 --- a/common/lib/xmodule/xmodule/modulestore/split_mongo/caching_descriptor_system.py +++ b/common/lib/xmodule/xmodule/modulestore/split_mongo/caching_descriptor_system.py @@ -1,6 +1,7 @@ import sys import logging from contracts import contract, new_contract +from lazy import lazy from xblock.runtime import KvsFieldData from xblock.fields import ScopeIds from opaque_keys.edx.locator import BlockUsageLocator, LocalId, CourseLocator, DefinitionLocator @@ -12,6 +13,7 @@ from .split_mongo_kvs import SplitMongoKVS from fs.osfs import OSFS from .definition_lazy_loader import DefinitionLazyLoader from xmodule.modulestore.edit_info import EditInfoRuntimeMixin +from xmodule.modulestore.inheritance import inheriting_field_data, InheritanceMixin from xmodule.modulestore.split_mongo import BlockKey log = logging.getLogger(__name__) @@ -34,8 +36,7 @@ class CachingDescriptorSystem(MakoDescriptorSystem, EditInfoRuntimeMixin): modulestore: the module store that can be used to retrieve additional modules - course_entry: the originally fetched enveloped course_structure w/ branch and course id info - plus a dictionary of cached inherited_settings indexed by (block_type, block_id) tuple. + course_entry: the originally fetched enveloped course_structure w/ branch and course id info. Callers to _load_item provide an override but that function ignores the provided structure and only looks at the branch and course id @@ -59,15 +60,18 @@ class CachingDescriptorSystem(MakoDescriptorSystem, EditInfoRuntimeMixin): self.course_entry = course_entry self.lazy = lazy self.module_data = module_data - # Compute inheritance - modulestore.inherit_settings( - course_entry['structure'].get('blocks', {}), - course_entry['structure'].get('root'), - course_entry.setdefault('inherited_settings', {}), - ) self.default_class = default_class self.local_modules = {} + @lazy + @contract(returns="dict(BlockKey: BlockKey)") + def _parent_map(self): + parent_map = {} + for block_key, block in self.course_entry['structure']['blocks'].iteritems(): + for child in block['fields'].get('children', []): + parent_map[child] = block_key + return parent_map + @contract(usage_key="BlockUsageLocator | BlockKey") def _load_item(self, usage_key, course_entry_override=None, **kwargs): # usage_key is either a UsageKey or just the block_key. if a usage_key, @@ -96,12 +100,15 @@ class CachingDescriptorSystem(MakoDescriptorSystem, EditInfoRuntimeMixin): branch=course_info.get('branch'), ) + if course_entry_override: + structure_id = course_entry_override.get('_id') + else: + structure_id = self.course_entry.get('_id') + json_data = self.get_module_data(block_key, course_key) class_ = self.load_block_type(json_data.get('block_type')) - # pass None for inherited_settings to signal that it should get the settings from cache - new_item = self.xblock_from_json(class_, course_key, block_key, json_data, None, course_entry_override, **kwargs) - return new_item + return self.xblock_from_json(class_, course_key, block_key, json_data, course_entry_override, **kwargs) @contract(block_key=BlockKey, course_key=CourseLocator) def get_module_data(self, block_key, course_key): @@ -134,7 +141,7 @@ class CachingDescriptorSystem(MakoDescriptorSystem, EditInfoRuntimeMixin): # is the intended one when not given a course_entry_override; thus, the caching of the last branch/course id. @contract(block_key="BlockKey | None") def xblock_from_json( - self, class_, course_key, block_key, json_data, inherited_settings, course_entry_override=None, **kwargs + self, class_, course_key, block_key, json_data, course_entry_override=None, **kwargs ): if course_entry_override is None: course_entry_override = self.course_entry @@ -150,13 +157,6 @@ class CachingDescriptorSystem(MakoDescriptorSystem, EditInfoRuntimeMixin): # If no usage id is provided, generate an in-memory id if block_key is None: block_key = BlockKey(json_data['block_type'], LocalId()) - else: - if inherited_settings is None: - # see if there's a value in course_entry - if block_key in self.course_entry['inherited_settings']: - inherited_settings = self.course_entry['inherited_settings'][block_key] - elif block_key not in self.course_entry['inherited_settings']: - self.course_entry['inherited_settings'][block_key] = inherited_settings if definition_id is not None and not json_data.get('definition_loaded', False): definition_loader = DefinitionLazyLoader( @@ -182,13 +182,22 @@ class CachingDescriptorSystem(MakoDescriptorSystem, EditInfoRuntimeMixin): converted_fields = self.modulestore.convert_references_to_keys( block_locator.course_key, class_, json_data.get('fields', {}), self.course_entry['structure']['blocks'], ) + if block_key in self._parent_map: + parent_key = self._parent_map[block_key] + parent = course_key.make_usage_key(parent_key.type, parent_key.id) + else: + parent = None kvs = SplitMongoKVS( definition_loader, converted_fields, - inherited_settings, - **kwargs + parent=parent, + field_decorator=kwargs.get('field_decorator') ) - field_data = KvsFieldData(kvs) + + if InheritanceMixin in self.modulestore.xblock_mixins: + field_data = inheriting_field_data(kvs) + else: + field_data = KvsFieldData(kvs) try: module = self.construct_xblock_from_class( diff --git a/common/lib/xmodule/xmodule/modulestore/split_mongo/split.py b/common/lib/xmodule/xmodule/modulestore/split_mongo/split.py index e2de2fde59..e67de8473e 100644 --- a/common/lib/xmodule/xmodule/modulestore/split_mongo/split.py +++ b/common/lib/xmodule/xmodule/modulestore/split_mongo/split.py @@ -1582,7 +1582,6 @@ class SplitMongoModuleStore(SplitBulkWriteMixin, ModuleStoreWriteBase): course_key, BlockKey(block_type, block_id) if block_id else None, json_data, - inherited_settings, **kwargs ) for field_name, value in (fields or {}).iteritems(): diff --git a/common/lib/xmodule/xmodule/modulestore/split_mongo/split_mongo_kvs.py b/common/lib/xmodule/xmodule/modulestore/split_mongo/split_mongo_kvs.py index dae355972e..0eb4e822f5 100644 --- a/common/lib/xmodule/xmodule/modulestore/split_mongo/split_mongo_kvs.py +++ b/common/lib/xmodule/xmodule/modulestore/split_mongo/split_mongo_kvs.py @@ -1,12 +1,15 @@ import copy +from contracts import contract, new_contract from xblock.fields import Scope from collections import namedtuple from xblock.exceptions import InvalidScopeError from .definition_lazy_loader import DefinitionLazyLoader from xmodule.modulestore.inheritance import InheritanceKeyValueStore +from opaque_keys.edx.locator import BlockUsageLocator # id is a BlockUsageLocator, def_id is the definition's guid SplitMongoKVSid = namedtuple('SplitMongoKVSid', 'id, def_id') +new_contract('BlockUsageLocator', BlockUsageLocator) class SplitMongoKVS(InheritanceKeyValueStore): @@ -15,22 +18,25 @@ class SplitMongoKVS(InheritanceKeyValueStore): known to the MongoModuleStore (data, children, and metadata) """ - def __init__(self, definition, initial_values, inherited_settings, **kwargs): + @contract(parent="BlockUsageLocator | None") + def __init__(self, definition, initial_values, parent, field_decorator=None): """ :param definition: either a lazyloader or definition id for the definition :param initial_values: a dictionary of the locally set values - :param inherited_settings: the json value of each inheritable field from above this. - Note, local fields may override and disagree w/ this b/c this says what the value - should be if the field is undefined. """ # deepcopy so that manipulations of fields does not pollute the source - super(SplitMongoKVS, self).__init__(copy.deepcopy(initial_values), inherited_settings) + super(SplitMongoKVS, self).__init__(copy.deepcopy(initial_values)) self._definition = definition # either a DefinitionLazyLoader or the db id of the definition. # if the db id, then the definition is presumed to be loaded into _fields # a decorator function for field values (to be called when a field is accessed) - self.field_decorator = kwargs.get('field_decorator', lambda x: x) + if field_decorator is None: + self.field_decorator = lambda x: x + else: + self.field_decorator = field_decorator + + self.parent = parent def get(self, key): @@ -38,8 +44,7 @@ class SplitMongoKVS(InheritanceKeyValueStore): if key.field_name not in self._fields: # parent undefined in editing runtime (I think) if key.scope == Scope.parent: - # see STUD-624. Right now copies MongoKeyValueStore.get's behavior of returning None - return None + return self.parent if key.scope == Scope.children: # didn't find children in _fields; so, see if there's a default raise KeyError() diff --git a/common/lib/xmodule/xmodule/modulestore/tests/test_cross_modulestore_import_export.py b/common/lib/xmodule/xmodule/modulestore/tests/test_cross_modulestore_import_export.py index f3cdd87694..92125eefe3 100644 --- a/common/lib/xmodule/xmodule/modulestore/tests/test_cross_modulestore_import_export.py +++ b/common/lib/xmodule/xmodule/modulestore/tests/test_cross_modulestore_import_export.py @@ -323,6 +323,7 @@ class CrossStoreXMLRoundtrip(CourseComparisonTest): self.exclude_field(None, 'wiki_slug') self.exclude_field(None, 'xml_attributes') + self.exclude_field(None, 'parent') self.ignore_asset_key('_id') self.ignore_asset_key('uploadDate') self.ignore_asset_key('content_son') diff --git a/common/lib/xmodule/xmodule/modulestore/tests/test_mixed_modulestore.py b/common/lib/xmodule/xmodule/modulestore/tests/test_mixed_modulestore.py index a355068b69..c4092f3355 100644 --- a/common/lib/xmodule/xmodule/modulestore/tests/test_mixed_modulestore.py +++ b/common/lib/xmodule/xmodule/modulestore/tests/test_mixed_modulestore.py @@ -33,11 +33,11 @@ from xmodule.modulestore.mixed import MixedModuleStore from xmodule.modulestore.search import path_to_location from xmodule.modulestore.tests.factories import check_mongo_calls from xmodule.modulestore.tests.mongo_connection import MONGO_PORT_NUM, MONGO_HOST -from xmodule.tests import DATA_DIR +from xmodule.tests import DATA_DIR, CourseComparisonTest @ddt.ddt -class TestMixedModuleStore(unittest.TestCase): +class TestMixedModuleStore(CourseComparisonTest): """ Quasi-superclass which tests Location based apps against both split and mongo dbs (Locator and Location-based dbs) @@ -1047,7 +1047,7 @@ class TestMixedModuleStore(unittest.TestCase): self.store.revert_to_published(self.vertical_x1a, self.user_id) reverted_parent = self.store.get_item(self.vertical_x1a) self.assertEqual(vertical_children_num, len(published_parent.children)) - self.assertEqual(reverted_parent, published_parent) + self.assertBlocksEqualByFields(reverted_parent, published_parent) self.assertFalse(self._has_changes(self.vertical_x1a)) @ddt.data('draft', 'split') @@ -1082,7 +1082,8 @@ class TestMixedModuleStore(unittest.TestCase): orig_vertical = self.store.get_item(self.vertical_x1a) self.store.revert_to_published(self.vertical_x1a, self.user_id) reverted_vertical = self.store.get_item(self.vertical_x1a) - self.assertEqual(orig_vertical, reverted_vertical) + + self.assertBlocksEqualByFields(orig_vertical, reverted_vertical) @ddt.data('draft', 'split') def test_revert_to_published_no_published(self, default_ms): diff --git a/common/lib/xmodule/xmodule/modulestore/tests/test_split_modulestore.py b/common/lib/xmodule/xmodule/modulestore/tests/test_split_modulestore.py index e465a97a4d..0d63264a3a 100644 --- a/common/lib/xmodule/xmodule/modulestore/tests/test_split_modulestore.py +++ b/common/lib/xmodule/xmodule/modulestore/tests/test_split_modulestore.py @@ -10,6 +10,7 @@ from contracts import contract from importlib import import_module from path import path +from xblock.fields import Reference, ReferenceList, ReferenceValueDict from xmodule.course_module import CourseDescriptor from xmodule.modulestore import ModuleStoreEnum from xmodule.modulestore.exceptions import ( @@ -1756,12 +1757,26 @@ class TestPublish(SplitModuleTest): for field in source.fields.values(): if field.name == 'children': self._compare_children(field.read_from(source), field.read_from(pub_copy), unexpected_blocks) + elif isinstance(field, (Reference, ReferenceList, ReferenceValueDict)): + self.assertReferenceEqual(field.read_from(source), field.read_from(pub_copy)) else: self.assertEqual(field.read_from(source), field.read_from(pub_copy)) for unexp in unexpected_blocks: with self.assertRaises(ItemNotFoundError): modulestore().get_item(dest_course_loc.make_usage_key(unexp.type, unexp.id)) + def assertReferenceEqual(self, expected, actual): + if isinstance(expected, BlockUsageLocator): + expected = BlockKey.from_usage_key(expected) + actual = BlockKey.from_usage_key(actual) + elif isinstance(expected, list): + expected = [BlockKey.from_usage_key(key) for key in expected] + actual = [BlockKey.from_usage_key(key) for key in actual] + elif isinstance(expected, dict): + expected = {key: BlockKey.from_usage_key(val) for (key, val) in expected} + actual = {key: BlockKey.from_usage_key(val) for (key, val) in actual} + self.assertEqual(expected, actual) + @contract( source_children="list(BlockUsageLocator)", dest_children="list(BlockUsageLocator)", diff --git a/common/lib/xmodule/xmodule/partitions/partitions.py b/common/lib/xmodule/xmodule/partitions/partitions.py index cd6b7cbcfa..c2752b951f 100644 --- a/common/lib/xmodule/xmodule/partitions/partitions.py +++ b/common/lib/xmodule/xmodule/partitions/partitions.py @@ -42,6 +42,9 @@ class Group(namedtuple("Group", "id name")): Raises TypeError if the value doesn't have the right keys. """ + if isinstance(value, Group): + return value + for key in ('id', 'name', 'version'): if key not in value: raise TypeError("Group dict {0} missing value key '{1}'".format( @@ -96,6 +99,9 @@ class UserPartition(namedtuple("UserPartition", "id name description groups")): Raises TypeError if the value doesn't have the right keys. """ + if isinstance(value, UserPartition): + return value + for key in ('id', 'name', 'description', 'version', 'groups'): if key not in value: raise TypeError("UserPartition dict {0} missing value key '{1}'" diff --git a/common/lib/xmodule/xmodule/tests/__init__.py b/common/lib/xmodule/xmodule/tests/__init__.py index 9581d4451d..86abf9fd43 100644 --- a/common/lib/xmodule/xmodule/tests/__init__.py +++ b/common/lib/xmodule/xmodule/tests/__init__.py @@ -13,7 +13,9 @@ import pprint import unittest from contextlib import contextmanager +from lazy import lazy from mock import Mock +from operator import attrgetter from path import path from xblock.field_data import DictFieldData @@ -227,6 +229,26 @@ class BulkAssertionTest(unittest.TestCase): assertEquals = assertEqual +class LazyFormat(object): + """ + An stringy object that delays formatting until it's put into a string context. + """ + __slots__ = ('template', 'args', 'kwargs', '_message') + + def __init__(self, template, *args, **kwargs): + self.template = template + self.args = args + self.kwargs = kwargs + self._message = None + + def __unicode__(self): + if self._message is None: + self._message = self.template.format(*self.args, **self.kwargs) + return self._message + + def __repr__(self): + return unicode(self) + class CourseComparisonTest(BulkAssertionTest): """ Mixin that has methods for comparing courses for equality. @@ -256,6 +278,65 @@ class CourseComparisonTest(BulkAssertionTest): """ self.ignored_asset_keys.add(key_name) + def assertReferenceRelativelyEqual(self, reference_field, expected_block, actual_block): + """ + Assert that the supplied reference field is identical on the expected_block and actual_block, + assoming that the references are only relative (that is, comparing only on block_type and block_id, + not course_key). + """ + def extract_key(usage_key): + if usage_key is None: + return None + else: + return (usage_key.block_type, usage_key.block_id) + expected = reference_field.read_from(expected_block) + actual = reference_field.read_from(actual_block) + if isinstance(reference_field, Reference): + expected = extract_key(expected) + actual = extract_key(actual) + elif isinstance(reference_field, ReferenceList): + expected = [extract_key(key) for key in expected] + actual = [extract_key(key) for key in actual] + elif isinstance(reference_field, ReferenceValueDict): + expected = {key: extract_key(val) for (key, val) in expected.iteritems()} + actual = {key: extract_key(val) for (key, val) in actual.iteritems()} + self.assertEqual( + expected, + actual, + LazyFormat( + "Field {} doesn't match between usages {} and {}: {!r} != {!r}", + reference_field.name, + expected_block.scope_ids.usage_id, + actual_block.scope_ids.usage_id, + expected, + actual + ) + ) + + def assertBlocksEqualByFields(self, expected_block, actual_block): + self.assertEqual(expected_block.fields, actual_block.fields) + for field in expected_block.fields.values(): + self.assertFieldEqual(field, expected_block, actual_block) + + def assertFieldEqual(self, field, expected_block, actual_block): + if isinstance(field, (Reference, ReferenceList, ReferenceValueDict)): + self.assertReferenceRelativelyEqual(field, expected_block, actual_block) + else: + expected = field.read_from(expected_block) + actual = field.read_from(actual_block) + self.assertEqual( + expected, + actual, + LazyFormat( + "Field {} doesn't match between usages {} and {}: {!r} != {!r}", + field.name, + expected_block.scope_ids.usage_id, + actual_block.scope_ids.usage_id, + expected, + actual + ) + ) + def assertCoursesEqual(self, expected_store, expected_course_key, actual_store, actual_course_key): """ Assert that the courses identified by ``expected_course_key`` in ``expected_store`` and @@ -313,11 +394,7 @@ class CourseComparisonTest(BulkAssertionTest): actual_item = actual_item_map.get(map_key(actual_item_location)) # Formatting the message slows down tests of large courses significantly, so only do it if it would be used - if actual_item is None: - msg = u'cannot find {} in {}'.format(map_key(actual_item_location), actual_item_map) - else: - msg = None - self.assertIsNotNone(actual_item, msg) + self.assertIsNotNone(actual_item, LazyFormat(u'cannot find {} in {}', map_key(actual_item_location), actual_item_map)) # compare fields self.assertEqual(expected_item.fields, actual_item.fields) @@ -333,20 +410,7 @@ class CourseComparisonTest(BulkAssertionTest): if field_name == 'children': continue - exp_value = map_references(field.read_from(expected_item), field, actual_course_key) - actual_value = field.read_from(actual_item) - # Formatting the message slows down tests of large courses significantly, so only do it if it would be used - if exp_value != actual_value: - msg = "Field {!r} doesn't match between usages {} and {}: {!r} != {!r}".format( - field_name, - expected_item.scope_ids.usage_id, - actual_item.scope_ids.usage_id, - exp_value, - actual_value, - ) - else: - msg = None - self.assertEqual(exp_value, actual_value, msg) + self.assertFieldEqual(field, expected_item, actual_item) # compare children self.assertEqual(expected_item.has_children, actual_item.has_children) From 0aaf4817826bd26708d54006a3b1ef62aed04e9c Mon Sep 17 00:00:00 2001 From: Calen Pennington Date: Fri, 12 Sep 2014 17:03:42 -0400 Subject: [PATCH 11/20] Turn on bulk operations for xml export --- .../xmodule/modulestore/xml_exporter.py | 182 +++++++++--------- 1 file changed, 92 insertions(+), 90 deletions(-) diff --git a/common/lib/xmodule/xmodule/modulestore/xml_exporter.py b/common/lib/xmodule/xmodule/modulestore/xml_exporter.py index 2ff1f1da6d..180d840781 100644 --- a/common/lib/xmodule/xmodule/modulestore/xml_exporter.py +++ b/common/lib/xmodule/xmodule/modulestore/xml_exporter.py @@ -37,106 +37,108 @@ def export_to_xml(modulestore, contentstore, course_key, root_dir, course_dir): `course_dir`: The name of the directory inside `root_dir` to write the course content to """ - course = modulestore.get_course(course_key, depth=None) # None means infinite - fsm = OSFS(root_dir) - export_fs = course.runtime.export_fs = fsm.makeopendir(course_dir) + with modulestore.bulk_operations(course_key): - root = lxml.etree.Element('unknown') + course = modulestore.get_course(course_key, depth=None) # None means infinite + fsm = OSFS(root_dir) + export_fs = course.runtime.export_fs = fsm.makeopendir(course_dir) - # export only the published content - with modulestore.branch_setting(ModuleStoreEnum.Branch.published_only, course_key): - # change all of the references inside the course to use the xml expected key type w/o version & branch - xml_centric_course_key = CourseLocator(course_key.org, course_key.course, course_key.run, deprecated=True) - adapt_references(course, xml_centric_course_key, export_fs) + root = lxml.etree.Element('unknown') - course.add_xml_to_node(root) + # export only the published content + with modulestore.branch_setting(ModuleStoreEnum.Branch.published_only, course_key): + # change all of the references inside the course to use the xml expected key type w/o version & branch + xml_centric_course_key = CourseLocator(course_key.org, course_key.course, course_key.run, deprecated=True) + adapt_references(course, xml_centric_course_key, export_fs) - with export_fs.open('course.xml', 'w') as course_xml: - lxml.etree.ElementTree(root).write(course_xml) + course.add_xml_to_node(root) - # export the static assets - policies_dir = export_fs.makeopendir('policies') - if contentstore: - contentstore.export_all_for_course( - course_key, - root_dir + '/' + course_dir + '/static/', - root_dir + '/' + course_dir + '/policies/assets.json', - ) + with export_fs.open('course.xml', 'w') as course_xml: + lxml.etree.ElementTree(root).write(course_xml) - # If we are using the default course image, export it to the - # legacy location to support backwards compatibility. - if course.course_image == course.fields['course_image'].default: - try: - course_image = contentstore.find( - StaticContent.compute_location( - course.id, - course.course_image - ), - ) - except NotFoundError: - pass - else: - output_dir = root_dir + '/' + course_dir + '/static/images/' - if not os.path.isdir(output_dir): - os.makedirs(output_dir) - with OSFS(output_dir).open('course_image.jpg', 'wb') as course_image_file: - course_image_file.write(course_image.data) - - # export the static tabs - export_extra_content(export_fs, modulestore, xml_centric_course_key, 'static_tab', 'tabs', '.html') - - # export the custom tags - export_extra_content(export_fs, modulestore, xml_centric_course_key, 'custom_tag_template', 'custom_tags') - - # export the course updates - export_extra_content(export_fs, modulestore, xml_centric_course_key, 'course_info', 'info', '.html') - - # export the 'about' data (e.g. overview, etc.) - export_extra_content(export_fs, modulestore, xml_centric_course_key, 'about', 'about', '.html') - - # export the grading policy - course_run_policy_dir = policies_dir.makeopendir(course.location.name) - with course_run_policy_dir.open('grading_policy.json', 'w') as grading_policy: - grading_policy.write(dumps(course.grading_policy, cls=EdxJSONEncoder, sort_keys=True, indent=4)) - - # export all of the course metadata in policy.json - with course_run_policy_dir.open('policy.json', 'w') as course_policy: - policy = {'course/' + course.location.name: own_metadata(course)} - course_policy.write(dumps(policy, cls=EdxJSONEncoder, sort_keys=True, indent=4)) - - #### DRAFTS #### - # xml backed courses don't support drafts! - if course.runtime.modulestore.get_modulestore_type() != ModuleStoreEnum.Type.xml: - # NOTE: this code assumes that verticals are the top most draftable container - # should we change the application, then this assumption will no longer be valid - # NOTE: we need to explicitly implement the logic for setting the vertical's parent - # and index here since the XML modulestore cannot load draft modules - with modulestore.branch_setting(ModuleStoreEnum.Branch.draft_preferred, course_key): - draft_verticals = modulestore.get_items( + # export the static assets + policies_dir = export_fs.makeopendir('policies') + if contentstore: + contentstore.export_all_for_course( course_key, - qualifiers={'category': 'vertical'}, - revision=ModuleStoreEnum.RevisionOption.draft_only + root_dir + '/' + course_dir + '/static/', + root_dir + '/' + course_dir + '/policies/assets.json', ) - if len(draft_verticals) > 0: - draft_course_dir = export_fs.makeopendir(DRAFT_DIR) - for draft_vertical in draft_verticals: - parent_loc = modulestore.get_parent_location( - draft_vertical.location, - revision=ModuleStoreEnum.RevisionOption.draft_preferred + # If we are using the default course image, export it to the + # legacy location to support backwards compatibility. + if course.course_image == course.fields['course_image'].default: + try: + course_image = contentstore.find( + StaticContent.compute_location( + course.id, + course.course_image + ), ) - # Don't try to export orphaned items. - if parent_loc is not None: - logging.debug('parent_loc = {0}'.format(parent_loc)) - if parent_loc.category in DIRECT_ONLY_CATEGORIES: - draft_vertical.xml_attributes['parent_sequential_url'] = parent_loc.to_deprecated_string() - sequential = modulestore.get_item(parent_loc) - index = sequential.children.index(draft_vertical.location) - draft_vertical.xml_attributes['index_in_children_list'] = str(index) - draft_vertical.runtime.export_fs = draft_course_dir - adapt_references(draft_vertical, xml_centric_course_key, draft_course_dir) - node = lxml.etree.Element('unknown') - draft_vertical.add_xml_to_node(node) + except NotFoundError: + pass + else: + output_dir = root_dir + '/' + course_dir + '/static/images/' + if not os.path.isdir(output_dir): + os.makedirs(output_dir) + with OSFS(output_dir).open('course_image.jpg', 'wb') as course_image_file: + course_image_file.write(course_image.data) + + # export the static tabs + export_extra_content(export_fs, modulestore, xml_centric_course_key, 'static_tab', 'tabs', '.html') + + # export the custom tags + export_extra_content(export_fs, modulestore, xml_centric_course_key, 'custom_tag_template', 'custom_tags') + + # export the course updates + export_extra_content(export_fs, modulestore, xml_centric_course_key, 'course_info', 'info', '.html') + + # export the 'about' data (e.g. overview, etc.) + export_extra_content(export_fs, modulestore, xml_centric_course_key, 'about', 'about', '.html') + + # export the grading policy + course_run_policy_dir = policies_dir.makeopendir(course.location.name) + with course_run_policy_dir.open('grading_policy.json', 'w') as grading_policy: + grading_policy.write(dumps(course.grading_policy, cls=EdxJSONEncoder, sort_keys=True, indent=4)) + + # export all of the course metadata in policy.json + with course_run_policy_dir.open('policy.json', 'w') as course_policy: + policy = {'course/' + course.location.name: own_metadata(course)} + course_policy.write(dumps(policy, cls=EdxJSONEncoder, sort_keys=True, indent=4)) + + #### DRAFTS #### + # xml backed courses don't support drafts! + if course.runtime.modulestore.get_modulestore_type() != ModuleStoreEnum.Type.xml: + # NOTE: this code assumes that verticals are the top most draftable container + # should we change the application, then this assumption will no longer be valid + # NOTE: we need to explicitly implement the logic for setting the vertical's parent + # and index here since the XML modulestore cannot load draft modules + with modulestore.branch_setting(ModuleStoreEnum.Branch.draft_preferred, course_key): + draft_verticals = modulestore.get_items( + course_key, + qualifiers={'category': 'vertical'}, + revision=ModuleStoreEnum.RevisionOption.draft_only + ) + + if len(draft_verticals) > 0: + draft_course_dir = export_fs.makeopendir(DRAFT_DIR) + for draft_vertical in draft_verticals: + parent_loc = modulestore.get_parent_location( + draft_vertical.location, + revision=ModuleStoreEnum.RevisionOption.draft_preferred + ) + # Don't try to export orphaned items. + if parent_loc is not None: + logging.debug('parent_loc = {0}'.format(parent_loc)) + if parent_loc.category in DIRECT_ONLY_CATEGORIES: + draft_vertical.xml_attributes['parent_sequential_url'] = parent_loc.to_deprecated_string() + sequential = modulestore.get_item(parent_loc) + index = sequential.children.index(draft_vertical.location) + draft_vertical.xml_attributes['index_in_children_list'] = str(index) + draft_vertical.runtime.export_fs = draft_course_dir + adapt_references(draft_vertical, xml_centric_course_key, draft_course_dir) + node = lxml.etree.Element('unknown') + draft_vertical.add_xml_to_node(node) def adapt_references(subtree, destination_course_key, export_fs): From eea1552e0bab58a24d5a2fe28ce89cee419cc9c8 Mon Sep 17 00:00:00 2001 From: Calen Pennington Date: Tue, 16 Sep 2014 10:35:39 -0400 Subject: [PATCH 12/20] Upgrade to a more performant version of opaque-keys --- requirements/edx/github.txt | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/requirements/edx/github.txt b/requirements/edx/github.txt index 032ff643fd..7977dd679d 100644 --- a/requirements/edx/github.txt +++ b/requirements/edx/github.txt @@ -29,7 +29,7 @@ -e git+https://github.com/edx-solutions/django-splash.git@7579d052afcf474ece1239153cffe1c89935bc4f#egg=django-splash -e git+https://github.com/edx/acid-block.git@459aff7b63db8f2c5decd1755706c1a64fb4ebb1#egg=acid-xblock -e git+https://github.com/edx/edx-ora2.git@release-2014-09-18T16.00#egg=edx-ora2 --e git+https://github.com/edx/opaque-keys.git@d45d0bd8d64c69531be69178b9505b5d38806ce0#egg=opaque-keys +-e git+https://github.com/edx/opaque-keys.git@013e30b4c4909b55e03c409a90ec6ef805903e04#egg=opaque-keys -e git+https://github.com/edx/ease.git@97de68448e5495385ba043d3091f570a699d5b5f#egg=ease -e git+https://github.com/edx/i18n-tools.git@56f048af9b6868613c14aeae760548834c495011#egg=i18n-tools -e git+https://github.com/edx/edx-oauth2-provider.git@0.2.1#egg=oauth2-provider From f9c78e07a788a1b3bf6960214ba1c28bf74beead Mon Sep 17 00:00:00 2001 From: Calen Pennington Date: Tue, 16 Sep 2014 13:41:01 -0400 Subject: [PATCH 13/20] Turn the course envelope into an actual object that just stores an existing course_key --- .../modulestore/split_mongo/__init__.py | 2 + .../split_mongo/caching_descriptor_system.py | 38 ++++---- .../xmodule/modulestore/split_mongo/split.py | 90 +++++++++---------- .../modulestore/split_mongo/split_draft.py | 8 +- 4 files changed, 61 insertions(+), 77 deletions(-) diff --git a/common/lib/xmodule/xmodule/modulestore/split_mongo/__init__.py b/common/lib/xmodule/xmodule/modulestore/split_mongo/__init__.py index aed77fa000..9582fb88c6 100644 --- a/common/lib/xmodule/xmodule/modulestore/split_mongo/__init__.py +++ b/common/lib/xmodule/xmodule/modulestore/split_mongo/__init__.py @@ -20,3 +20,5 @@ class BlockKey(namedtuple('BlockKey', 'type id')): def from_usage_key(cls, usage_key): return cls(usage_key.block_type, usage_key.block_id) + +CourseEnvelope = namedtuple('CourseEnvelope', 'course_key structure') diff --git a/common/lib/xmodule/xmodule/modulestore/split_mongo/caching_descriptor_system.py b/common/lib/xmodule/xmodule/modulestore/split_mongo/caching_descriptor_system.py index ae92cd476b..8f3221ef4e 100644 --- a/common/lib/xmodule/xmodule/modulestore/split_mongo/caching_descriptor_system.py +++ b/common/lib/xmodule/xmodule/modulestore/split_mongo/caching_descriptor_system.py @@ -14,12 +14,13 @@ from fs.osfs import OSFS from .definition_lazy_loader import DefinitionLazyLoader from xmodule.modulestore.edit_info import EditInfoRuntimeMixin from xmodule.modulestore.inheritance import inheriting_field_data, InheritanceMixin -from xmodule.modulestore.split_mongo import BlockKey +from xmodule.modulestore.split_mongo import BlockKey, CourseEnvelope log = logging.getLogger(__name__) new_contract('BlockUsageLocator', BlockUsageLocator) new_contract('BlockKey', BlockKey) +new_contract('CourseEnvelope', CourseEnvelope) class CachingDescriptorSystem(MakoDescriptorSystem, EditInfoRuntimeMixin): @@ -29,6 +30,7 @@ class CachingDescriptorSystem(MakoDescriptorSystem, EditInfoRuntimeMixin): Computes the settings (nee 'metadata') inheritance upon creation. """ + @contract(course_entry=CourseEnvelope) def __init__(self, modulestore, course_entry, default_class, module_data, lazy, **kwargs): """ Computes the settings inheritance and sets up the cache. @@ -44,10 +46,10 @@ class CachingDescriptorSystem(MakoDescriptorSystem, EditInfoRuntimeMixin): underlying modulestore """ # needed by capa_problem (as runtime.filestore via this.resources_fs) - if 'course' in course_entry: - root = modulestore.fs_root / course_entry['org'] / course_entry['course'] / course_entry['run'] + if course_entry.course_key.course: + root = modulestore.fs_root / course_entry.course_key.org / course_entry.course_key.course / course_entry.course_key.run else: - root = modulestore.fs_root / course_entry['structure']['_id'] + root = modulestore.fs_root / course_entry.structure['_id'] root.makedirs_p() # create directory if it doesn't exist super(CachingDescriptorSystem, self).__init__( @@ -67,12 +69,12 @@ class CachingDescriptorSystem(MakoDescriptorSystem, EditInfoRuntimeMixin): @contract(returns="dict(BlockKey: BlockKey)") def _parent_map(self): parent_map = {} - for block_key, block in self.course_entry['structure']['blocks'].iteritems(): + for block_key, block in self.course_entry.structure['blocks'].iteritems(): for child in block['fields'].get('children', []): parent_map[child] = block_key return parent_map - @contract(usage_key="BlockUsageLocator | BlockKey") + @contract(usage_key="BlockUsageLocator | BlockKey", course_entry_override="CourseEnvelope | None") def _load_item(self, usage_key, course_entry_override=None, **kwargs): # usage_key is either a UsageKey or just the block_key. if a usage_key, if isinstance(usage_key, BlockUsageLocator): @@ -92,18 +94,12 @@ class CachingDescriptorSystem(MakoDescriptorSystem, EditInfoRuntimeMixin): block_key = usage_key course_info = course_entry_override or self.course_entry - course_key = CourseLocator( - version_guid=course_info['structure']['_id'], - org=course_info.get('org'), - course=course_info.get('course'), - run=course_info.get('run'), - branch=course_info.get('branch'), - ) + course_key = course_info.course_key if course_entry_override: - structure_id = course_entry_override.get('_id') + structure_id = course_entry_override.structure.get('_id') else: - structure_id = self.course_entry.get('_id') + structure_id = self.course_entry.structure.get('_id') json_data = self.get_module_data(block_key, course_key) @@ -147,10 +143,7 @@ class CachingDescriptorSystem(MakoDescriptorSystem, EditInfoRuntimeMixin): course_entry_override = self.course_entry else: # most recent retrieval is most likely the right one for next caller (see comment above fn) - self.course_entry['branch'] = course_entry_override['branch'] - self.course_entry['org'] = course_entry_override['org'] - self.course_entry['course'] = course_entry_override['course'] - self.course_entry['run'] = course_entry_override['run'] + self.course_entry = CourseEnvelope(course_entry_override.course_key, self.course_entry.structure) definition_id = json_data.get('definition') @@ -163,7 +156,7 @@ class CachingDescriptorSystem(MakoDescriptorSystem, EditInfoRuntimeMixin): self.modulestore, block_key.type, definition_id, lambda fields: self.modulestore.convert_references_to_keys( course_key, self.load_block_type(block_key.type), - fields, self.course_entry['structure']['blocks'], + fields, self.course_entry.structure['blocks'], ) ) else: @@ -180,7 +173,7 @@ class CachingDescriptorSystem(MakoDescriptorSystem, EditInfoRuntimeMixin): ) converted_fields = self.modulestore.convert_references_to_keys( - block_locator.course_key, class_, json_data.get('fields', {}), self.course_entry['structure']['blocks'], + block_locator.course_key, class_, json_data.get('fields', {}), self.course_entry.structure['blocks'], ) if block_key in self._parent_map: parent_key = self._parent_map[block_key] @@ -210,8 +203,7 @@ class CachingDescriptorSystem(MakoDescriptorSystem, EditInfoRuntimeMixin): return ErrorDescriptor.from_json( json_data, self, - BlockUsageLocator( - CourseLocator(version_guid=course_entry_override['structure']['_id']), + course_entry_override.course_key.make_usage_key( block_type='error', block_id=block_key.id ), diff --git a/common/lib/xmodule/xmodule/modulestore/split_mongo/split.py b/common/lib/xmodule/xmodule/modulestore/split_mongo/split.py index e67de8473e..cbf7db0a55 100644 --- a/common/lib/xmodule/xmodule/modulestore/split_mongo/split.py +++ b/common/lib/xmodule/xmodule/modulestore/split_mongo/split.py @@ -75,9 +75,10 @@ from xmodule.modulestore import ( from ..exceptions import ItemNotFoundError from .caching_descriptor_system import CachingDescriptorSystem -from xmodule.modulestore.split_mongo.mongo_connection import MongoConnection, BlockKey +from xmodule.modulestore.split_mongo.mongo_connection import MongoConnection +from xmodule.modulestore.split_mongo import BlockKey, CourseEnvelope from xmodule.error_module import ErrorDescriptor -from _collections import defaultdict +from collections import defaultdict from types import NoneType @@ -521,7 +522,7 @@ class SplitMongoModuleStore(SplitBulkWriteMixin, ModuleStoreWriteBase): new_module_data = {} for block_id in base_block_ids: new_module_data = self.descendants( - system.course_entry['structure']['blocks'], + system.course_entry.structure['blocks'], block_id, depth, new_module_data @@ -541,7 +542,7 @@ class SplitMongoModuleStore(SplitBulkWriteMixin, ModuleStoreWriteBase): converted_fields = self.convert_references_to_keys( course_key, system.load_block_type(block['block_type']), definitions[block['definition']].get('fields'), - system.course_entry['structure']['blocks'], + system.course_entry.structure['blocks'], ) block['fields'].update(converted_fields) block['definition_loaded'] = True @@ -549,24 +550,18 @@ class SplitMongoModuleStore(SplitBulkWriteMixin, ModuleStoreWriteBase): system.module_data.update(new_module_data) return system.module_data + @contract(course_entry=CourseEnvelope, block_keys="list(BlockKey)", depth="int | None") def _load_items(self, course_entry, block_keys, depth=0, lazy=True, **kwargs): ''' Load & cache the given blocks from the course. Prefetch down to the given depth. Load the definitions into each block if lazy is False; otherwise, use the lazy definition placeholder. ''' - runtime = self._get_cache(course_entry['structure']['_id']) + runtime = self._get_cache(course_entry.structure['_id']) if runtime is None: runtime = self.create_runtime(course_entry, lazy) - self._add_cache(course_entry['structure']['_id'], runtime) - course_key = CourseLocator( - version_guid=course_entry['structure']['_id'], - org=course_entry.get('org'), - course=course_entry.get('course'), - run=course_entry.get('run'), - branch=course_entry.get('branch'), - ) - self.cache_items(runtime, block_keys, course_key, depth, lazy) + self._add_cache(course_entry.structure['_id'], runtime) + self.cache_items(runtime, block_keys, course_entry.course_key, depth, lazy) return [runtime.load_item(block_key, course_entry, **kwargs) for block_key in block_keys] def _get_cache(self, course_version_guid): @@ -650,14 +645,7 @@ class SplitMongoModuleStore(SplitBulkWriteMixin, ModuleStoreWriteBase): # 'run', and 'branch' are not intrinsic to structure # and the one assoc'd w/ it by another fetch may not be the one relevant to this fetch; so, # add it in the envelope for the structure. - envelope = { - 'org': course_key.org, - 'course': course_key.course, - 'run': course_key.run, - 'branch': course_key.branch, - 'structure': entry, - } - return envelope + return CourseEnvelope(course_key.replace(version_guid=version_guid), entry) def get_courses(self, branch, **kwargs): ''' @@ -690,13 +678,15 @@ class SplitMongoModuleStore(SplitBulkWriteMixin, ModuleStoreWriteBase): result = [] for entry in matching_structures: course_info = id_version_map[entry['_id']] - envelope = { - 'org': course_info['org'], - 'course': course_info['course'], - 'run': course_info['run'], - 'branch': branch, - 'structure': entry, - } + envelope = CourseEnvelope( + CourseLocator( + org=course_info['org'], + course=course_info['course'], + run=course_info['run'], + branch=branch, + ), + entry + ) root = entry['root'] course_list = self._load_items(envelope, [root], 0, lazy=True, **kwargs) if not isinstance(course_list[0], ErrorDescriptor): @@ -721,7 +711,7 @@ class SplitMongoModuleStore(SplitBulkWriteMixin, ModuleStoreWriteBase): raise ItemNotFoundError(course_id) course_entry = self._lookup_course(course_id) - root = course_entry['structure']['root'] + root = course_entry.structure['root'] result = self._load_items(course_entry, [root], depth, lazy=True, **kwargs) return result[0] @@ -750,7 +740,7 @@ class SplitMongoModuleStore(SplitBulkWriteMixin, ModuleStoreWriteBase): if usage_key.block_id is None: raise InsufficientSpecificationError(usage_key) try: - course_structure = self._lookup_course(usage_key.course_key)['structure'] + course_structure = self._lookup_course(usage_key.course_key).structure except ItemNotFoundError: # this error only occurs if the course does not exist return False @@ -824,7 +814,7 @@ class SplitMongoModuleStore(SplitBulkWriteMixin, ModuleStoreWriteBase): # odd case where we don't search just confirm block_name = qualifiers.pop('name') block_ids = [] - for block_id, block in course['structure']['blocks'].iteritems(): + for block_id, block in course.structure['blocks'].iteritems(): if block_name == block_id.id and _block_matches_all(block): block_ids.append(block_id) @@ -836,7 +826,7 @@ class SplitMongoModuleStore(SplitBulkWriteMixin, ModuleStoreWriteBase): # don't expect caller to know that children are in fields if 'children' in qualifiers: settings['children'] = qualifiers.pop('children') - for block_id, value in course['structure']['blocks'].iteritems(): + for block_id, value in course.structure['blocks'].iteritems(): if _block_matches_all(value): items.append(block_id) @@ -854,7 +844,7 @@ class SplitMongoModuleStore(SplitBulkWriteMixin, ModuleStoreWriteBase): :param locator: BlockUsageLocator restricting search scope ''' course = self._lookup_course(locator.course_key) - parent_id = self._get_parent_from_structure(BlockKey.from_usage_key(locator), course['structure']) + parent_id = self._get_parent_from_structure(BlockKey.from_usage_key(locator), course.structure) if parent_id is None: return None return BlockUsageLocator.make_relative( @@ -869,9 +859,9 @@ class SplitMongoModuleStore(SplitBulkWriteMixin, ModuleStoreWriteBase): """ detached_categories = [name for name, __ in XBlock.load_tagged_classes("detached")] course = self._lookup_course(course_key) - items = set(course['structure']['blocks'].keys()) - items.remove(course['structure']['root']) - blocks = course['structure']['blocks'] + items = set(course.structure['blocks'].keys()) + items.remove(course.structure['root']) + blocks = course.structure['blocks'] for block_id, block_data in blocks.iteritems(): items.difference_update(BlockKey(*child) for child in block_data.get('fields', {}).get('children', [])) if block_data['block_type'] in detached_categories: @@ -912,7 +902,7 @@ class SplitMongoModuleStore(SplitBulkWriteMixin, ModuleStoreWriteBase): 'edited_on': when the change was made } """ - course = self._lookup_course(course_key)['structure'] + course = self._lookup_course(course_key).structure return { 'original_version': course['original_version'], 'previous_version': course['previous_version'], @@ -946,7 +936,7 @@ class SplitMongoModuleStore(SplitBulkWriteMixin, ModuleStoreWriteBase): return None if course_locator.version_guid is None: course = self._lookup_course(course_locator) - version_guid = course['structure']['_id'] + version_guid = course.structure['_id'] course_locator = course_locator.for_version(version_guid) else: version_guid = course_locator.version_guid @@ -977,7 +967,7 @@ class SplitMongoModuleStore(SplitBulkWriteMixin, ModuleStoreWriteBase): ''' # course_agnostic means we don't care if the head and version don't align, trust the version - course_struct = self._lookup_course(block_locator.course_key.course_agnostic())['structure'] + course_struct = self._lookup_course(block_locator.course_key.course_agnostic()).structure block_key = BlockKey.from_usage_key(block_locator) all_versions_with_block = self.find_ancestor_structures( original_version=course_struct['original_version'], @@ -1164,7 +1154,7 @@ class SplitMongoModuleStore(SplitBulkWriteMixin, ModuleStoreWriteBase): # find course_index entry if applicable and structures entry index_entry = self._get_index_if_valid(course_key, force) - structure = self._lookup_course(course_key)['structure'] + structure = self._lookup_course(course_key).structure partitioned_fields = self.partition_fields_by_scope(block_type, fields) new_def_data = partitioned_fields.get(Scope.content, {}) @@ -1243,7 +1233,7 @@ class SplitMongoModuleStore(SplitBulkWriteMixin, ModuleStoreWriteBase): **kwargs) # don't version the structure as create_item handled that already. - new_structure = self._lookup_course(xblock.location.course_key)['structure'] + new_structure = self._lookup_course(xblock.location.course_key).structure # add new block as child and update parent's version block_id = BlockKey.from_usage_key(parent_usage_key) @@ -1386,7 +1376,7 @@ class SplitMongoModuleStore(SplitBulkWriteMixin, ModuleStoreWriteBase): elif block_fields or definition_fields: # pointing to existing course w/ some overrides # just get the draft_version structure draft_version = CourseLocator(version_guid=versions_dict[master_branch]) - draft_structure = self._lookup_course(draft_version)['structure'] + draft_structure = self._lookup_course(draft_version).structure draft_structure = self.version_structure(locator, draft_structure, user_id) new_id = draft_structure['_id'] root_block = draft_structure['blocks'][draft_structure['root']] @@ -1411,7 +1401,7 @@ class SplitMongoModuleStore(SplitBulkWriteMixin, ModuleStoreWriteBase): else: # Pointing to an existing course structure new_id = versions_dict[master_branch] draft_version = CourseLocator(version_guid=new_id) - draft_structure = self._lookup_course(draft_version)['structure'] + draft_structure = self._lookup_course(draft_version).structure locator = locator.replace(version_guid=new_id) with self.bulk_operations(locator): @@ -1472,7 +1462,7 @@ class SplitMongoModuleStore(SplitBulkWriteMixin, ModuleStoreWriteBase): user_id, course_key, block_key.type, fields=fields, force=force ) - original_structure = self._lookup_course(course_key)['structure'] + original_structure = self._lookup_course(course_key).structure index_entry = self._get_index_if_valid(course_key, force) original_entry = self._get_block_from_structure(original_structure, block_key) @@ -1614,7 +1604,7 @@ class SplitMongoModuleStore(SplitBulkWriteMixin, ModuleStoreWriteBase): course_key = xblock.location.course_key with self.bulk_operations(course_key): index_entry = self._get_index_if_valid(course_key, force) - structure = self._lookup_course(course_key)['structure'] + structure = self._lookup_course(course_key).structure new_structure = self.version_structure(course_key, structure, user_id) new_id = new_structure['_id'] is_updated = self._persist_subdag(xblock, user_id, new_structure['blocks'], new_id) @@ -1749,7 +1739,7 @@ class SplitMongoModuleStore(SplitBulkWriteMixin, ModuleStoreWriteBase): # get the destination's index, and source and destination structures. with self.bulk_operations(source_course): with self.bulk_operations(destination_course): - source_structure = self._lookup_course(source_course)['structure'] + source_structure = self._lookup_course(source_course).structure index_entry = self.get_course_index(destination_course) if index_entry is None: # brand new course @@ -1767,7 +1757,7 @@ class SplitMongoModuleStore(SplitBulkWriteMixin, ModuleStoreWriteBase): definition_id=root_source['definition'], ) else: - destination_structure = self._lookup_course(destination_course)['structure'] + destination_structure = self._lookup_course(destination_course).structure destination_structure = self.version_structure(destination_course, destination_structure, user_id) if blacklist != EXCLUDE_ALL: @@ -1828,7 +1818,7 @@ class SplitMongoModuleStore(SplitBulkWriteMixin, ModuleStoreWriteBase): raise ItemNotFoundError(usage_locator) with self.bulk_operations(usage_locator.course_key): - original_structure = self._lookup_course(usage_locator.course_key)['structure'] + original_structure = self._lookup_course(usage_locator.course_key).structure block_key = BlockKey.from_usage_key(usage_locator) if original_structure['root'] == block_key: raise ValueError("Cannot delete the root of a course") @@ -1972,7 +1962,7 @@ class SplitMongoModuleStore(SplitBulkWriteMixin, ModuleStoreWriteBase): :param course_locator: the course to clean """ - original_structure = self._lookup_course(course_locator)['structure'] + original_structure = self._lookup_course(course_locator).structure for block in original_structure['blocks'].itervalues(): if 'fields' in block and 'children' in block['fields']: block['fields']["children"] = [ diff --git a/common/lib/xmodule/xmodule/modulestore/split_mongo/split_draft.py b/common/lib/xmodule/xmodule/modulestore/split_mongo/split_draft.py index 3936fb7055..6d06db94c7 100644 --- a/common/lib/xmodule/xmodule/modulestore/split_mongo/split_draft.py +++ b/common/lib/xmodule/xmodule/modulestore/split_mongo/split_draft.py @@ -242,7 +242,7 @@ class DraftVersioningModuleStore(ModuleStoreDraftAndPublished, SplitMongoModuleS :return: True if the draft and published versions differ """ def get_course(branch_name): - return self._lookup_course(xblock.location.course_key.for_branch(branch_name))['structure'] + return self._lookup_course(xblock.location.course_key.for_branch(branch_name)).structure def get_block(course_structure, block_key): return self._get_block_from_structure(course_structure, block_key) @@ -318,7 +318,7 @@ class DraftVersioningModuleStore(ModuleStoreDraftAndPublished, SplitMongoModuleS # get head version of Published branch published_course_structure = self._lookup_course( location.course_key.for_branch(ModuleStoreEnum.BranchName.published) - )['structure'] + ).structure published_block = self._get_block_from_structure( published_course_structure, BlockKey.from_usage_key(location) @@ -327,7 +327,7 @@ class DraftVersioningModuleStore(ModuleStoreDraftAndPublished, SplitMongoModuleS raise InvalidVersionError(location) # create a new versioned draft structure - draft_course_structure = self._lookup_course(draft_course_key)['structure'] + draft_course_structure = self._lookup_course(draft_course_key).structure new_structure = self.version_structure(draft_course_key, draft_course_structure, user_id) # remove the block and its descendants from the new structure @@ -394,7 +394,7 @@ class DraftVersioningModuleStore(ModuleStoreDraftAndPublished, SplitMongoModuleS pass def _get_head(self, xblock, branch): - course_structure = self._lookup_course(xblock.location.course_key.for_branch(branch))['structure'] + course_structure = self._lookup_course(xblock.location.course_key.for_branch(branch)).structure return self._get_block_from_structure(course_structure, BlockKey.from_usage_key(xblock.location)) def _get_version(self, block): From a14fbe4c6b5c51ef97ed71a306fce4289b906375 Mon Sep 17 00:00:00 2001 From: Calen Pennington Date: Tue, 16 Sep 2014 13:59:39 -0400 Subject: [PATCH 14/20] Clean up coverage counting a little bit --- common/lib/xmodule/.coveragerc | 3 +++ common/lib/xmodule/xmodule/modulestore/draft_and_published.py | 3 --- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/common/lib/xmodule/.coveragerc b/common/lib/xmodule/.coveragerc index baadd30829..5bd4aa4f65 100644 --- a/common/lib/xmodule/.coveragerc +++ b/common/lib/xmodule/.coveragerc @@ -5,6 +5,9 @@ source = common/lib/xmodule [report] ignore_errors = True +exclude_lines = + pragma: no cover + raise NotImplementedError [html] title = XModule Python Test Coverage Report diff --git a/common/lib/xmodule/xmodule/modulestore/draft_and_published.py b/common/lib/xmodule/xmodule/modulestore/draft_and_published.py index 5f36e11ceb..c5b6a86585 100644 --- a/common/lib/xmodule/xmodule/modulestore/draft_and_published.py +++ b/common/lib/xmodule/xmodule/modulestore/draft_and_published.py @@ -69,9 +69,6 @@ class ModuleStoreDraftAndPublished(BranchSettingMixin): """ __metaclass__ = ABCMeta - def __init__(self, *args, **kwargs): - super(ModuleStoreDraftAndPublished, self).__init__(*args, **kwargs) - @abstractmethod def delete_item(self, location, user_id, revision=None, **kwargs): raise NotImplementedError From 4b0686836b971127e8b754d256be9c0c14189dd2 Mon Sep 17 00:00:00 2001 From: Calen Pennington Date: Tue, 16 Sep 2014 16:47:35 -0400 Subject: [PATCH 15/20] Make modulestores properly propogate **kwargs --- common/lib/xmodule/xmodule/modulestore/__init__.py | 2 +- common/lib/xmodule/xmodule/modulestore/django.py | 10 ++++++++-- .../xmodule/xmodule/modulestore/draft_and_published.py | 2 +- .../modulestore/tests/test_mixed_modulestore.py | 6 ++++-- common/lib/xmodule/xmodule/modulestore/xml.py | 8 ++++---- 5 files changed, 18 insertions(+), 10 deletions(-) diff --git a/common/lib/xmodule/xmodule/modulestore/__init__.py b/common/lib/xmodule/xmodule/modulestore/__init__.py index 72bcf2e169..92f78b94fd 100644 --- a/common/lib/xmodule/xmodule/modulestore/__init__.py +++ b/common/lib/xmodule/xmodule/modulestore/__init__.py @@ -634,7 +634,7 @@ class ModuleStoreReadBase(BulkOperationsMixin, ModuleStoreRead): ''' Set up the error-tracking logic. ''' - super(ModuleStoreReadBase, self).__init__() + super(ModuleStoreReadBase, self).__init__(**kwargs) self._course_errors = defaultdict(make_error_tracker) # location -> ErrorLog # TODO move the inheritance_cache_subsystem to classes which use it self.metadata_inheritance_cache_subsystem = metadata_inheritance_cache_subsystem diff --git a/common/lib/xmodule/xmodule/modulestore/django.py b/common/lib/xmodule/xmodule/modulestore/django.py index 67aebc14ad..60b2dd8844 100644 --- a/common/lib/xmodule/xmodule/modulestore/django.py +++ b/common/lib/xmodule/xmodule/modulestore/django.py @@ -18,6 +18,8 @@ import threading from xmodule.util.django import get_current_request_hostname import xmodule.modulestore # pylint: disable=unused-import +from xmodule.modulestore.mixed import MixedModuleStore +from xmodule.modulestore.draft_and_published import BranchSettingMixin from xmodule.contentstore.django import contentstore import xblock.reference.plugins @@ -66,6 +68,12 @@ def create_modulestore_instance(engine, content_store, doc_store_config, options except InvalidCacheBackendError: metadata_inheritance_cache = get_cache('default') + if issubclass(class_, MixedModuleStore): + _options['create_modulestore_instance'] = create_modulestore_instance + + if issubclass(class_, BranchSettingMixin): + _options['branch_setting_func'] = _get_modulestore_branch_setting + return class_( contentstore=content_store, metadata_inheritance_cache_subsystem=metadata_inheritance_cache, @@ -75,8 +83,6 @@ def create_modulestore_instance(engine, content_store, doc_store_config, options doc_store_config=doc_store_config, i18n_service=i18n_service or ModuleI18nService(), fs_service=fs_service or xblock.reference.plugins.FSService(), - branch_setting_func=_get_modulestore_branch_setting, - create_modulestore_instance=create_modulestore_instance, **_options ) diff --git a/common/lib/xmodule/xmodule/modulestore/draft_and_published.py b/common/lib/xmodule/xmodule/modulestore/draft_and_published.py index c5b6a86585..1f8f521758 100644 --- a/common/lib/xmodule/xmodule/modulestore/draft_and_published.py +++ b/common/lib/xmodule/xmodule/modulestore/draft_and_published.py @@ -25,11 +25,11 @@ class BranchSettingMixin(object): :param branch_setting_func: a function that returns the default branch setting for this object. If not specified, ModuleStoreEnum.Branch.published_only is used as the default setting. """ - super(BranchSettingMixin, self).__init__(*args, **kwargs) self.default_branch_setting_func = kwargs.pop( 'branch_setting_func', lambda: ModuleStoreEnum.Branch.published_only ) + super(BranchSettingMixin, self).__init__(*args, **kwargs) # cache the branch setting on a local thread to support a multi-threaded environment self.thread_cache = threading.local() diff --git a/common/lib/xmodule/xmodule/modulestore/tests/test_mixed_modulestore.py b/common/lib/xmodule/xmodule/modulestore/tests/test_mixed_modulestore.py index c4092f3355..7dc26c4b8c 100644 --- a/common/lib/xmodule/xmodule/modulestore/tests/test_mixed_modulestore.py +++ b/common/lib/xmodule/xmodule/modulestore/tests/test_mixed_modulestore.py @@ -27,7 +27,7 @@ from opaque_keys.edx.locations import SlashSeparatedCourseKey from opaque_keys.edx.locator import BlockUsageLocator, CourseLocator from xmodule.exceptions import InvalidVersionError from xmodule.modulestore import ModuleStoreEnum -from xmodule.modulestore.draft_and_published import UnsupportedRevisionError +from xmodule.modulestore.draft_and_published import UnsupportedRevisionError, ModuleStoreDraftAndPublished from xmodule.modulestore.exceptions import ItemNotFoundError, DuplicateCourseError, ReferentialIntegrityError, NoPathToItem from xmodule.modulestore.mixed import MixedModuleStore from xmodule.modulestore.search import path_to_location @@ -1789,9 +1789,11 @@ def create_modulestore_instance(engine, contentstore, doc_store_config, options, """ class_ = load_function(engine) + if issubclass(class_, ModuleStoreDraftAndPublished): + options['branch_setting_func'] = lambda: ModuleStoreEnum.Branch.draft_preferred + return class_( doc_store_config=doc_store_config, contentstore=contentstore, - branch_setting_func=lambda: ModuleStoreEnum.Branch.draft_preferred, **options ) diff --git a/common/lib/xmodule/xmodule/modulestore/xml.py b/common/lib/xmodule/xmodule/modulestore/xml.py index 907e8e42ab..aade361934 100644 --- a/common/lib/xmodule/xmodule/modulestore/xml.py +++ b/common/lib/xmodule/xmodule/modulestore/xml.py @@ -370,7 +370,7 @@ class XMLModuleStore(ModuleStoreReadBase): """ def __init__( self, data_dir, default_class=None, course_dirs=None, course_ids=None, - load_error_modules=True, i18n_service=None, pyfs_service=None, **kwargs + load_error_modules=True, i18n_service=None, fs_service=None, **kwargs ): """ Initialize an XMLModuleStore from data_dir @@ -409,7 +409,7 @@ class XMLModuleStore(ModuleStoreReadBase): self.field_data = inheriting_field_data(kvs=DictKeyValueStore()) self.i18n_service = i18n_service - self.pyfs_service = pyfs_service + self.fs_service = fs_service # If we are specifically asked for missing courses, that should # be an error. If we are asked for "all" courses, find the ones @@ -555,8 +555,8 @@ class XMLModuleStore(ModuleStoreReadBase): if self.i18n_service: services['i18n'] = self.i18n_service - if self.pyfs_service: - services['fs'] = self.pyfs_service + if self.fs_service: + services['fs'] = self.fs_service system = ImportSystem( xmlstore=self, From 57f9701ab23050cddedbe058b4390d0630d1fd4f Mon Sep 17 00:00:00 2001 From: Calen Pennington Date: Tue, 16 Sep 2014 16:48:40 -0400 Subject: [PATCH 16/20] Clean up exception definitions --- .../xmodule/xmodule/modulestore/draft_and_published.py | 2 +- common/lib/xmodule/xmodule/modulestore/exceptions.py | 9 ++++----- 2 files changed, 5 insertions(+), 6 deletions(-) diff --git a/common/lib/xmodule/xmodule/modulestore/draft_and_published.py b/common/lib/xmodule/xmodule/modulestore/draft_and_published.py index 1f8f521758..27895ce06d 100644 --- a/common/lib/xmodule/xmodule/modulestore/draft_and_published.py +++ b/common/lib/xmodule/xmodule/modulestore/draft_and_published.py @@ -113,7 +113,7 @@ class ModuleStoreDraftAndPublished(BranchSettingMixin): raise NotImplementedError -class UnsupportedRevisionError(ValueError): +class UnsupportedRevisionError(ValueError): """ This error is raised if a method is called with an unsupported revision parameter. """ diff --git a/common/lib/xmodule/xmodule/modulestore/exceptions.py b/common/lib/xmodule/xmodule/modulestore/exceptions.py index e6fa9edc0f..d98c744009 100644 --- a/common/lib/xmodule/xmodule/modulestore/exceptions.py +++ b/common/lib/xmodule/xmodule/modulestore/exceptions.py @@ -74,7 +74,9 @@ class DuplicateCourseError(Exception): """ existing_entry will have the who, when, and other properties of the existing entry """ - super(DuplicateCourseError, self).__init__() + super(DuplicateCourseError, self).__init__( + u'Cannot create course {}, which duplicates {}'.format(course_id, existing_entry) + ) self.course_id = course_id self.existing_entry = existing_entry @@ -84,9 +86,6 @@ class InvalidBranchSetting(Exception): Raised when the process' branch setting did not match the required setting for the attempted operation on a store. """ def __init__(self, expected_setting, actual_setting): - super(InvalidBranchSetting, self).__init__() + super(InvalidBranchSetting, self).__init__(u"Invalid branch: expected {} but got {}".format(expected_setting, actual_setting)) self.expected_setting = expected_setting self.actual_setting = actual_setting - - def __unicode__(self, *args, **kwargs): - return u"Invalid branch: expected {} but got {}".format(self.expected_setting, self.actual_setting) From 40c3253d815ce4a160b27511d9b45273e2018596 Mon Sep 17 00:00:00 2001 From: Calen Pennington Date: Wed, 17 Sep 2014 14:14:00 -0400 Subject: [PATCH 17/20] Use more idiomatic python for deleting an attribute --- .../xmodule/xmodule/modulestore/tests/test_split_modulestore.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/common/lib/xmodule/xmodule/modulestore/tests/test_split_modulestore.py b/common/lib/xmodule/xmodule/modulestore/tests/test_split_modulestore.py index 0d63264a3a..22c1afd0df 100644 --- a/common/lib/xmodule/xmodule/modulestore/tests/test_split_modulestore.py +++ b/common/lib/xmodule/xmodule/modulestore/tests/test_split_modulestore.py @@ -1593,7 +1593,7 @@ class TestInheritance(SplitModuleTest): # unset on parent, retrieve child, verify unset chapter = modulestore().get_item(chapter.location.version_agnostic()) - chapter.fields['visible_to_staff_only'].delete_from(chapter) + del chapter.visible_to_staff_only modulestore().update_item(chapter, self.user_id) problem = modulestore().get_item(problem.location.version_agnostic()) From 1a682dacd1deeb2305f591c26f38cbfdf7b82a7e Mon Sep 17 00:00:00 2001 From: Calen Pennington Date: Tue, 23 Sep 2014 13:18:16 -0400 Subject: [PATCH 18/20] Include caching definitions in split bulk operations --- .../split_mongo/caching_descriptor_system.py | 5 +- .../split_mongo/definition_lazy_loader.py | 5 +- .../split_mongo/mongo_connection.py | 11 +- .../xmodule/modulestore/split_mongo/split.py | 109 ++++++++++--- .../test_split_modulestore_bulk_operations.py | 148 ++++++++++++++++++ lms/djangoapps/courseware/module_render.py | 49 +++--- .../courseware/tests/test_module_render.py | 54 +++++-- 7 files changed, 311 insertions(+), 70 deletions(-) diff --git a/common/lib/xmodule/xmodule/modulestore/split_mongo/caching_descriptor_system.py b/common/lib/xmodule/xmodule/modulestore/split_mongo/caching_descriptor_system.py index 8f3221ef4e..34455a068d 100644 --- a/common/lib/xmodule/xmodule/modulestore/split_mongo/caching_descriptor_system.py +++ b/common/lib/xmodule/xmodule/modulestore/split_mongo/caching_descriptor_system.py @@ -153,7 +153,10 @@ class CachingDescriptorSystem(MakoDescriptorSystem, EditInfoRuntimeMixin): if definition_id is not None and not json_data.get('definition_loaded', False): definition_loader = DefinitionLazyLoader( - self.modulestore, block_key.type, definition_id, + self.modulestore, + course_key, + block_key.type, + definition_id, lambda fields: self.modulestore.convert_references_to_keys( course_key, self.load_block_type(block_key.type), fields, self.course_entry.structure['blocks'], diff --git a/common/lib/xmodule/xmodule/modulestore/split_mongo/definition_lazy_loader.py b/common/lib/xmodule/xmodule/modulestore/split_mongo/definition_lazy_loader.py index a9b91fe4a5..226d36c606 100644 --- a/common/lib/xmodule/xmodule/modulestore/split_mongo/definition_lazy_loader.py +++ b/common/lib/xmodule/xmodule/modulestore/split_mongo/definition_lazy_loader.py @@ -8,13 +8,14 @@ class DefinitionLazyLoader(object): object doesn't force access during init but waits until client wants the definition. Only works if the modulestore is a split mongo store. """ - def __init__(self, modulestore, block_type, definition_id, field_converter): + def __init__(self, modulestore, course_key, block_type, definition_id, field_converter): """ Simple placeholder for yet-to-be-fetched data :param modulestore: the pymongo db connection with the definitions :param definition_locator: the id of the record in the above to fetch """ self.modulestore = modulestore + self.course_key = course_key self.definition_locator = DefinitionLocator(block_type, definition_id) self.field_converter = field_converter @@ -23,4 +24,4 @@ class DefinitionLazyLoader(object): Fetch the definition. Note, the caller should replace this lazy loader pointer with the result so as not to fetch more than once """ - return self.modulestore.db_connection.get_definition(self.definition_locator.definition_id) + return self.modulestore.get_definition(self.course_key, self.definition_locator.definition_id) diff --git a/common/lib/xmodule/xmodule/modulestore/split_mongo/mongo_connection.py b/common/lib/xmodule/xmodule/modulestore/split_mongo/mongo_connection.py index 0d4e1e09dc..1f04a263c8 100644 --- a/common/lib/xmodule/xmodule/modulestore/split_mongo/mongo_connection.py +++ b/common/lib/xmodule/xmodule/modulestore/split_mongo/mongo_connection.py @@ -268,18 +268,17 @@ class MongoConnection(object): return self.definitions.find_one({'_id': key}) @autoretry_read() - def find_matching_definitions(self, query): + def get_definitions(self, definitions): """ - Find the definitions matching the query. Right now the query must be a legal mongo query - :param query: a mongo-style query of {key: [value|{$in ..}|..], ..} + Retrieve all definitions listed in `definitions`. """ - return self.definitions.find(query) + return self.definitions.find({'$in': {'_id': definitions}}) - def insert_definition(self, definition): + def upsert_definition(self, definition): """ Create the definition in the db """ - self.definitions.insert(definition) + self.definitions.update({'_id': definition['_id']}, definition, upsert=True) def ensure_indexes(self): """ diff --git a/common/lib/xmodule/xmodule/modulestore/split_mongo/split.py b/common/lib/xmodule/xmodule/modulestore/split_mongo/split.py index cbf7db0a55..94d8380e23 100644 --- a/common/lib/xmodule/xmodule/modulestore/split_mongo/split.py +++ b/common/lib/xmodule/xmodule/modulestore/split_mongo/split.py @@ -117,6 +117,8 @@ class SplitBulkWriteRecord(BulkOpsRecord): self.index = None self.structures = {} self.structures_in_db = set() + self.definitions = {} + self.definitions_in_db = set() # TODO: This needs to track which branches have actually been modified/versioned, # so that copying one branch to another doesn't update the original branch. @@ -226,6 +228,9 @@ class SplitBulkWriteMixin(BulkOperationsMixin): for _id in bulk_write_record.structures.viewkeys() - bulk_write_record.structures_in_db: self.db_connection.upsert_structure(bulk_write_record.structures[_id]) + for _id in bulk_write_record.definitions.viewkeys() - bulk_write_record.definitions_in_db: + self.db_connection.upsert_definition(bulk_write_record.definitions[_id]) + if bulk_write_record.index is not None and bulk_write_record.index != bulk_write_record.initial_index: if bulk_write_record.initial_index is None: self.db_connection.insert_course_index(bulk_write_record.index) @@ -292,6 +297,67 @@ class SplitBulkWriteMixin(BulkOperationsMixin): else: self.db_connection.upsert_structure(structure) + def get_definition(self, course_key, definition_guid): + """ + Retrieve a single definition by id, respecting the active bulk operation + on course_key. + + Args: + course_key (:class:`.CourseKey`): The course being operated on + definition_guid (str or ObjectID): The id of the definition to load + """ + bulk_write_record = self._get_bulk_ops_record(course_key) + if bulk_write_record.active: + definition = bulk_write_record.definitions.get(definition_guid) + + # The definition hasn't been loaded from the db yet, so load it + if definition is None: + definition = self.db_connection.get_definition(definition_guid) + bulk_write_record.definitions[definition_guid] = definition + if definition is not None: + bulk_write_record.definitions_in_db.add(definition_guid) + + return definition + else: + # cast string to ObjectId if necessary + definition_guid = course_key.as_object_id(definition_guid) + return self.db_connection.get_definition(definition_guid) + + def get_definitions(self, ids): + """ + Return all definitions that specified in ``ids``. + + If a definition with the same id is in both the cache and the database, + the cached version will be preferred. + + Arguments: + ids (list): A list of definition ids + """ + definitions = [] + ids = set(ids) + + for _, record in self._active_records: + for definition in record.definitions.values(): + definition_id = definition.get('_id') + if definition_id in ids: + ids.remove(definition_id) + definitions.append(definition) + + definitions.extend(self.db_connection.get_definitions(list(ids))) + return definitions + + + def update_definition(self, course_key, definition): + """ + Update a definition, respecting the current bulk operation status + (no data will be written to the database if a bulk operation is active.) + """ + bulk_write_record = self._get_bulk_ops_record(course_key) + if bulk_write_record.active: + bulk_write_record.definitions[definition['_id']] = definition + else: + self.db_connection.upsert_definition(definition) + def version_structure(self, course_key, structure, user_id): """ Copy the structure and update the history info (edited_by, edited_on, previous_version) @@ -530,9 +596,10 @@ class SplitMongoModuleStore(SplitBulkWriteMixin, ModuleStoreWriteBase): if not lazy: # Load all descendants by id - descendent_definitions = self.db_connection.find_matching_definitions({ - '_id': {'$in': [block['definition'] - for block in new_module_data.itervalues()]}}) + descendent_definitions = self.get_definitions([ + block['definition'] + for block in new_module_data.itervalues() + ]) # turn into a map definitions = {definition['_id']: definition for definition in descendent_definitions} @@ -803,7 +870,7 @@ class SplitMongoModuleStore(SplitBulkWriteMixin, ModuleStoreWriteBase): self._block_matches(block_json.get('fields', {}), settings) ): if content: - definition_block = self.db_connection.get_definition(block_json['definition']) + definition_block = self.get_definition(course_locator, block_json['definition']) return self._block_matches(definition_block.get('fields', {}), content) else: return True @@ -1016,7 +1083,7 @@ class SplitMongoModuleStore(SplitBulkWriteMixin, ModuleStoreWriteBase): # TODO implement raise NotImplementedError() - def create_definition_from_data(self, new_def_data, category, user_id): + def create_definition_from_data(self, course_key, new_def_data, category, user_id): """ Pull the definition fields out of descriptor and save to the db as a new definition w/o a predecessor and return the new id. @@ -1037,11 +1104,11 @@ class SplitMongoModuleStore(SplitBulkWriteMixin, ModuleStoreWriteBase): }, 'schema_version': self.SCHEMA_VERSION, } - self.db_connection.insert_definition(document) + self.update_definition(course_key, document) definition_locator = DefinitionLocator(category, new_id) return definition_locator - def update_definition_from_data(self, definition_locator, new_def_data, user_id): + def update_definition_from_data(self, course_key, definition_locator, new_def_data, user_id): """ See if new_def_data differs from the persisted version. If so, update the persisted version and return the new id. @@ -1058,7 +1125,7 @@ class SplitMongoModuleStore(SplitBulkWriteMixin, ModuleStoreWriteBase): # if this looks in cache rather than fresh fetches, then it will probably not detect # actual change b/c the descriptor and cache probably point to the same objects - old_definition = self.db_connection.get_definition(definition_locator.definition_id) + old_definition = self.get_definition(course_key, definition_locator.definition_id) if old_definition is None: raise ItemNotFoundError(definition_locator) @@ -1072,7 +1139,7 @@ class SplitMongoModuleStore(SplitBulkWriteMixin, ModuleStoreWriteBase): # previous version id old_definition['edit_info']['previous_version'] = definition_locator.definition_id old_definition['schema_version'] = self.SCHEMA_VERSION - self.db_connection.insert_definition(old_definition) + self.update_definition(course_key, old_definition) return DefinitionLocator(old_definition['block_type'], old_definition['_id']), True else: return definition_locator, False @@ -1160,9 +1227,9 @@ class SplitMongoModuleStore(SplitBulkWriteMixin, ModuleStoreWriteBase): new_def_data = partitioned_fields.get(Scope.content, {}) # persist the definition if persisted != passed if (definition_locator is None or isinstance(definition_locator.definition_id, LocalId)): - definition_locator = self.create_definition_from_data(new_def_data, block_type, user_id) + definition_locator = self.create_definition_from_data(course_key, new_def_data, block_type, user_id) elif new_def_data is not None: - definition_locator, _ = self.update_definition_from_data(definition_locator, new_def_data, user_id) + definition_locator, _ = self.update_definition_from_data(course_key, definition_locator, new_def_data, user_id) # copy the structure and modify the new one new_structure = self.version_structure(course_key, structure, user_id) @@ -1355,7 +1422,7 @@ class SplitMongoModuleStore(SplitBulkWriteMixin, ModuleStoreWriteBase): }, 'schema_version': self.SCHEMA_VERSION, } - self.db_connection.insert_definition(definition_entry) + self.update_definition(locator, definition_entry) draft_structure = self._new_structure( user_id, @@ -1383,14 +1450,14 @@ class SplitMongoModuleStore(SplitBulkWriteMixin, ModuleStoreWriteBase): if block_fields is not None: root_block['fields'].update(self._serialize_fields(root_category, block_fields)) if definition_fields is not None: - definition = self.db_connection.get_definition(root_block['definition']) + definition = self.get_definition(locator, root_block['definition']) definition['fields'].update(definition_fields) definition['edit_info']['previous_version'] = definition['_id'] definition['edit_info']['edited_by'] = user_id definition['edit_info']['edited_on'] = datetime.datetime.now(UTC) definition['_id'] = ObjectId() definition['schema_version'] = self.SCHEMA_VERSION - self.db_connection.insert_definition(definition) + self.update_definition(locator, definition) root_block['definition'] = definition['_id'] root_block['edit_info']['edited_on'] = datetime.datetime.now(UTC) root_block['edit_info']['edited_by'] = user_id @@ -1483,7 +1550,7 @@ class SplitMongoModuleStore(SplitBulkWriteMixin, ModuleStoreWriteBase): definition_locator = DefinitionLocator(original_entry['block_type'], original_entry['definition']) if definition_fields: definition_locator, is_updated = self.update_definition_from_data( - definition_locator, definition_fields, user_id + course_key, definition_locator, definition_fields, user_id ) # check metadata @@ -1607,7 +1674,7 @@ class SplitMongoModuleStore(SplitBulkWriteMixin, ModuleStoreWriteBase): structure = self._lookup_course(course_key).structure new_structure = self.version_structure(course_key, structure, user_id) new_id = new_structure['_id'] - is_updated = self._persist_subdag(xblock, user_id, new_structure['blocks'], new_id) + is_updated = self._persist_subdag(course_key, xblock, user_id, new_structure['blocks'], new_id) if is_updated: self.update_structure(course_key, new_structure) @@ -1621,18 +1688,20 @@ class SplitMongoModuleStore(SplitBulkWriteMixin, ModuleStoreWriteBase): else: return xblock - def _persist_subdag(self, xblock, user_id, structure_blocks, new_id): + def _persist_subdag(self, course_key, xblock, user_id, structure_blocks, new_id): # persist the definition if persisted != passed partitioned_fields = self.partition_xblock_fields_by_scope(xblock) new_def_data = self._serialize_fields(xblock.category, partitioned_fields[Scope.content]) is_updated = False if xblock.definition_locator is None or isinstance(xblock.definition_locator.definition_id, LocalId): xblock.definition_locator = self.create_definition_from_data( - new_def_data, xblock.category, user_id) + course_key, new_def_data, xblock.category, user_id + ) is_updated = True elif new_def_data: xblock.definition_locator, is_updated = self.update_definition_from_data( - xblock.definition_locator, new_def_data, user_id) + course_key, xblock.definition_locator, new_def_data, user_id + ) if isinstance(xblock.scope_ids.usage_id.block_id, LocalId): # generate an id @@ -1654,7 +1723,7 @@ class SplitMongoModuleStore(SplitBulkWriteMixin, ModuleStoreWriteBase): for child in xblock.children: if isinstance(child.block_id, LocalId): child_block = xblock.system.get_block(child) - is_updated = self._persist_subdag(child_block, user_id, structure_blocks, new_id) or is_updated + is_updated = self._persist_subdag(course_key, child_block, user_id, structure_blocks, new_id) or is_updated children.append(BlockKey.from_usage_key(child_block.location)) else: children.append(BlockKey.from_usage_key(child)) diff --git a/common/lib/xmodule/xmodule/modulestore/tests/test_split_modulestore_bulk_operations.py b/common/lib/xmodule/xmodule/modulestore/tests/test_split_modulestore_bulk_operations.py index 61f9b93076..f5e6af6e8c 100644 --- a/common/lib/xmodule/xmodule/modulestore/tests/test_split_modulestore_bulk_operations.py +++ b/common/lib/xmodule/xmodule/modulestore/tests/test_split_modulestore_bulk_operations.py @@ -21,6 +21,7 @@ class TestBulkWriteMixin(unittest.TestCase): self.course_key = CourseLocator('org', 'course', 'run-a', branch='test') self.course_key_b = CourseLocator('org', 'course', 'run-b', branch='test') self.structure = {'this': 'is', 'a': 'structure', '_id': ObjectId()} + self.definition = {'this': 'is', 'a': 'definition', '_id': ObjectId()} self.index_entry = {'this': 'is', 'an': 'index'} def assertConnCalls(self, *calls): @@ -66,6 +67,20 @@ class TestBulkWriteMixinClosed(TestBulkWriteMixin): self.assertConnCalls(call.upsert_structure(self.structure)) self.clear_cache.assert_called_once_with(self.structure['_id']) + @ddt.data('deadbeef1234' * 2, u'deadbeef1234' * 2, ObjectId()) + def test_no_bulk_read_definition(self, version_guid): + # Reading a definition when no bulk operation is active should just call + # through to the db_connection + result = self.bulk.get_definition(self.course_key, version_guid) + self.assertConnCalls(call.get_definition(self.course_key.as_object_id(version_guid))) + self.assertEqual(result, self.conn.get_definition.return_value) + + def test_no_bulk_write_definition(self): + # Writing a definition when no bulk operation is active should just + # call through to the db_connection. + self.bulk.update_definition(self.course_key, self.definition) + self.assertConnCalls(call.upsert_definition(self.definition)) + @ddt.data(True, False) def test_no_bulk_read_index(self, ignore_case): # Reading a course index when no bulk operation is active should just call @@ -129,6 +144,68 @@ class TestBulkWriteMixinClosed(TestBulkWriteMixin): self.conn.mock_calls ) + def test_write_index_and_definition_on_close(self): + original_index = {'versions': {}} + self.conn.get_course_index.return_value = copy.deepcopy(original_index) + self.bulk._begin_bulk_operation(self.course_key) + self.conn.reset_mock() + self.bulk.update_definition(self.course_key, self.definition) + self.bulk.insert_course_index(self.course_key, {'versions': {self.course_key.branch: self.definition['_id']}}) + self.assertConnCalls() + self.bulk._end_bulk_operation(self.course_key) + self.assertConnCalls( + call.upsert_definition(self.definition), + call.update_course_index( + {'versions': {self.course_key.branch: self.definition['_id']}}, + from_index=original_index + ) + ) + + def test_write_index_and_multiple_definitions_on_close(self): + original_index = {'versions': {'a': ObjectId(), 'b': ObjectId()}} + self.conn.get_course_index.return_value = copy.deepcopy(original_index) + self.bulk._begin_bulk_operation(self.course_key) + self.conn.reset_mock() + self.bulk.update_definition(self.course_key.replace(branch='a'), self.definition) + other_definition = {'another': 'definition', '_id': ObjectId()} + self.bulk.update_definition(self.course_key.replace(branch='b'), other_definition) + self.bulk.insert_course_index(self.course_key, {'versions': {'a': self.definition['_id'], 'b': other_definition['_id']}}) + self.bulk._end_bulk_operation(self.course_key) + self.assertItemsEqual( + [ + call.upsert_definition(self.definition), + call.upsert_definition(other_definition), + call.update_course_index( + {'versions': {'a': self.definition['_id'], 'b': other_definition['_id']}}, + from_index=original_index + ) + ], + self.conn.mock_calls + ) + + def test_write_definition_on_close(self): + self.conn.get_course_index.return_value = None + self.bulk._begin_bulk_operation(self.course_key) + self.conn.reset_mock() + self.bulk.update_definition(self.course_key, self.definition) + self.assertConnCalls() + self.bulk._end_bulk_operation(self.course_key) + self.assertConnCalls(call.upsert_definition(self.definition)) + + def test_write_multiple_definitions_on_close(self): + self.conn.get_course_index.return_value = None + self.bulk._begin_bulk_operation(self.course_key) + self.conn.reset_mock() + self.bulk.update_definition(self.course_key.replace(branch='a'), self.definition) + other_definition = {'another': 'definition', '_id': ObjectId()} + self.bulk.update_definition(self.course_key.replace(branch='b'), other_definition) + self.assertConnCalls() + self.bulk._end_bulk_operation(self.course_key) + self.assertItemsEqual( + [call.upsert_definition(self.definition), call.upsert_definition(other_definition)], + self.conn.mock_calls + ) + def test_write_index_and_structure_on_close(self): original_index = {'versions': {}} self.conn.get_course_index.return_value = copy.deepcopy(original_index) @@ -181,6 +258,7 @@ class TestBulkWriteMixinClosed(TestBulkWriteMixin): get_result = self.bulk.get_structure(self.course_key, version_result['_id']) self.assertEquals(version_result, get_result) + class TestBulkWriteMixinClosedAfterPrevTransaction(TestBulkWriteMixinClosed, TestBulkWriteMixinPreviousTransaction): """ Test that operations on with a closed transaction aren't affected by a previously executed transaction @@ -307,6 +385,37 @@ class TestBulkWriteMixinFindMethods(TestBulkWriteMixin): else: self.assertNotIn(db_structure(_id), results) + @ddt.data( + ([], [], []), + ([1, 2, 3], [1, 2], [1, 2]), + ([1, 2, 3], [1], [1, 2]), + ([1, 2, 3], [], [1, 2]), + ) + @ddt.unpack + def test_get_definitions(self, search_ids, active_ids, db_ids): + db_definition = lambda _id: {'db': 'definition', '_id': _id} + active_definition = lambda _id: {'active': 'definition', '_id': _id} + + db_definitions = [db_definition(_id) for _id in db_ids if _id not in active_ids] + for n, _id in enumerate(active_ids): + course_key = CourseLocator('org', 'course', 'run{}'.format(n)) + self.bulk._begin_bulk_operation(course_key) + self.bulk.update_definition(course_key, active_definition(_id)) + + self.conn.get_definitions.return_value = db_definitions + results = self.bulk.get_definitions(search_ids) + self.conn.get_definitions.assert_called_once_with(list(set(search_ids) - set(active_ids))) + for _id in active_ids: + if _id in search_ids: + self.assertIn(active_definition(_id), results) + else: + self.assertNotIn(active_definition(_id), results) + for _id in db_ids: + if _id in search_ids and _id not in active_ids: + self.assertIn(db_definition(_id), results) + else: + self.assertNotIn(db_definition(_id), results) + def test_no_bulk_find_structures_derived_from(self): ids = [Mock(name='id')] self.conn.find_structures_derived_from.return_value = [MagicMock(name='result')] @@ -456,6 +565,45 @@ class TestBulkWriteMixinOpen(TestBulkWriteMixin): self.assertEquals(self.conn.get_structure.call_count, 1) self.assertEqual(result, self.structure) + @ddt.data('deadbeef1234' * 2, u'deadbeef1234' * 2, ObjectId()) + def test_read_definition_without_write_from_db(self, version_guid): + # Reading a definition before it's been written (while in bulk operation mode) + # returns the definition from the database + result = self.bulk.get_definition(self.course_key, version_guid) + self.assertEquals(self.conn.get_definition.call_count, 1) + self.assertEqual(result, self.conn.get_definition.return_value) + self.assertCacheNotCleared() + + @ddt.data('deadbeef1234' * 2, u'deadbeef1234' * 2, ObjectId()) + def test_read_definition_without_write_only_reads_once(self, version_guid): + # Reading the same definition multiple times shouldn't hit the database + # more than once + for _ in xrange(2): + result = self.bulk.get_definition(self.course_key, version_guid) + self.assertEquals(self.conn.get_definition.call_count, 1) + self.assertEqual(result, self.conn.get_definition.return_value) + self.assertCacheNotCleared() + + @ddt.data('deadbeef1234' * 2, u'deadbeef1234' * 2, ObjectId()) + def test_read_definition_after_write_no_db(self, version_guid): + # Reading a definition that's already been written shouldn't hit the db at all + self.definition['_id'] = version_guid + self.bulk.update_definition(self.course_key, self.definition) + result = self.bulk.get_definition(self.course_key, version_guid) + self.assertEquals(self.conn.get_definition.call_count, 0) + self.assertEqual(result, self.definition) + + @ddt.data('deadbeef1234' * 2, u'deadbeef1234' * 2, ObjectId()) + def test_read_definition_after_write_after_read(self, version_guid): + # Reading a definition that's been updated after being pulled from the db should + # still get the updated value + self.definition['_id'] = version_guid + self.bulk.get_definition(self.course_key, version_guid) + self.bulk.update_definition(self.course_key, self.definition) + result = self.bulk.get_definition(self.course_key, version_guid) + self.assertEquals(self.conn.get_definition.call_count, 1) + self.assertEqual(result, self.definition) + @ddt.data(True, False) def test_read_index_without_write_from_db(self, ignore_case): # Reading the index without writing to it should pull from the database diff --git a/lms/djangoapps/courseware/module_render.py b/lms/djangoapps/courseware/module_render.py index de45e8beed..f80943f729 100644 --- a/lms/djangoapps/courseware/module_render.py +++ b/lms/djangoapps/courseware/module_render.py @@ -115,35 +115,36 @@ def toc_for_course(user, request, course, active_chapter, active_section, field_ field_data_cache must include data from the course module and 2 levels of its descendents ''' - course_module = get_module_for_descriptor(user, request, course, field_data_cache, course.id) - if course_module is None: - return None + with modulestore().bulk_operations(course.id): + course_module = get_module_for_descriptor(user, request, course, field_data_cache, course.id) + if course_module is None: + return None - chapters = list() - for chapter in course_module.get_display_items(): - if chapter.hide_from_toc: - continue + chapters = list() + for chapter in course_module.get_display_items(): + if chapter.hide_from_toc: + continue - sections = list() - for section in chapter.get_display_items(): + sections = list() + for section in chapter.get_display_items(): - active = (chapter.url_name == active_chapter and - section.url_name == active_section) + active = (chapter.url_name == active_chapter and + section.url_name == active_section) - if not section.hide_from_toc: - sections.append({'display_name': section.display_name_with_default, - 'url_name': section.url_name, - 'format': section.format if section.format is not None else '', - 'due': get_extended_due_date(section), - 'active': active, - 'graded': section.graded, - }) + if not section.hide_from_toc: + sections.append({'display_name': section.display_name_with_default, + 'url_name': section.url_name, + 'format': section.format if section.format is not None else '', + 'due': get_extended_due_date(section), + 'active': active, + 'graded': section.graded, + }) - chapters.append({'display_name': chapter.display_name_with_default, - 'url_name': chapter.url_name, - 'sections': sections, - 'active': chapter.url_name == active_chapter}) - return chapters + chapters.append({'display_name': chapter.display_name_with_default, + 'url_name': chapter.url_name, + 'sections': sections, + 'active': chapter.url_name == active_chapter}) + return chapters def get_module(user, request, usage_key, field_data_cache, diff --git a/lms/djangoapps/courseware/tests/test_module_render.py b/lms/djangoapps/courseware/tests/test_module_render.py index 4498bea38b..09a53761c6 100644 --- a/lms/djangoapps/courseware/tests/test_module_render.py +++ b/lms/djangoapps/courseware/tests/test_module_render.py @@ -326,19 +326,29 @@ class TestTOC(ModuleStoreTestCase): self.request = factory.get(chapter_url) self.request.user = UserFactory() self.modulestore = self.store._get_modulestore_for_courseid(self.course_key) - with check_mongo_calls(num_finds, num_sends): - self.toy_course = self.store.get_course(self.toy_loc, depth=2) - self.field_data_cache = FieldDataCache.cache_for_descriptor_descendents( - self.toy_loc, self.request.user, self.toy_course, depth=2 - ) + with self.modulestore.bulk_operations(self.course_key): + with check_mongo_calls(num_finds, num_sends): + self.toy_course = self.store.get_course(self.toy_loc, depth=2) + self.field_data_cache = FieldDataCache.cache_for_descriptor_descendents( + self.toy_loc, self.request.user, self.toy_course, depth=2 + ) - # TODO: LMS-11220: Document why split find count is 9 - # TODO: LMS-11220: Document why mongo find count is 4 - @ddt.data((ModuleStoreEnum.Type.mongo, 3, 0), (ModuleStoreEnum.Type.split, 9, 0)) + # Mongo makes 3 queries to load the course to depth 2: + # - 1 for the course + # - 1 for its children + # - 1 for its grandchildren + # Split makes 6 queries to load the course to depth 2: + # - load the structure + # - load 5 definitions + # Split makes 2 queries to render the toc: + # - it loads the active version at the start of the bulk operation + # - it loads the course definition for inheritance, because it's outside + # the bulk-operation marker that loaded the course descriptor + @ddt.data((ModuleStoreEnum.Type.mongo, 3, 0, 0), (ModuleStoreEnum.Type.split, 6, 0, 2)) @ddt.unpack - def test_toc_toy_from_chapter(self, default_ms, num_finds, num_sends): + def test_toc_toy_from_chapter(self, default_ms, setup_finds, setup_sends, toc_finds): with self.store.default_store(default_ms): - self.setup_modulestore(default_ms, num_finds, num_sends) + self.setup_modulestore(default_ms, setup_finds, setup_sends) expected = ([{'active': True, 'sections': [{'url_name': 'Toy_Videos', 'display_name': u'Toy Videos', 'graded': True, 'format': u'Lecture Sequence', 'due': None, 'active': False}, @@ -354,20 +364,29 @@ class TestTOC(ModuleStoreTestCase): 'format': '', 'due': None, 'active': False}], 'url_name': 'secret:magic', 'display_name': 'secret:magic'}]) - with check_mongo_calls(0, 0): + with check_mongo_calls(toc_finds, 0): actual = render.toc_for_course( self.request.user, self.request, self.toy_course, self.chapter, None, self.field_data_cache ) for toc_section in expected: self.assertIn(toc_section, actual) - # TODO: LMS-11220: Document why split find count is 9 - # TODO: LMS-11220: Document why mongo find count is 4 - @ddt.data((ModuleStoreEnum.Type.mongo, 3, 0), (ModuleStoreEnum.Type.split, 9, 0)) + # Mongo makes 3 queries to load the course to depth 2: + # - 1 for the course + # - 1 for its children + # - 1 for its grandchildren + # Split makes 6 queries to load the course to depth 2: + # - load the structure + # - load 5 definitions + # Split makes 2 queries to render the toc: + # - it loads the active version at the start of the bulk operation + # - it loads the course definition for inheritance, because it's outside + # the bulk-operation marker that loaded the course descriptor + @ddt.data((ModuleStoreEnum.Type.mongo, 3, 0, 0), (ModuleStoreEnum.Type.split, 6, 0, 2)) @ddt.unpack - def test_toc_toy_from_section(self, default_ms, num_finds, num_sends): + def test_toc_toy_from_section(self, default_ms, setup_finds, setup_sends, toc_finds): with self.store.default_store(default_ms): - self.setup_modulestore(default_ms, num_finds, num_sends) + self.setup_modulestore(default_ms, setup_finds, setup_sends) section = 'Welcome' expected = ([{'active': True, 'sections': [{'url_name': 'Toy_Videos', 'display_name': u'Toy Videos', 'graded': True, @@ -384,7 +403,8 @@ class TestTOC(ModuleStoreTestCase): 'format': '', 'due': None, 'active': False}], 'url_name': 'secret:magic', 'display_name': 'secret:magic'}]) - actual = render.toc_for_course(self.request.user, self.request, self.toy_course, self.chapter, section, self.field_data_cache) + with check_mongo_calls(toc_finds, 0): + actual = render.toc_for_course(self.request.user, self.request, self.toy_course, self.chapter, section, self.field_data_cache) for toc_section in expected: self.assertIn(toc_section, actual) From ef23c19bf4195f9c70032cc13c2d88a408ad699c Mon Sep 17 00:00:00 2001 From: Calen Pennington Date: Tue, 23 Sep 2014 15:02:27 -0400 Subject: [PATCH 19/20] Prevent queries into split using deprecated CourseLocators, BlockUsageLocators, or DefinitionLocators --- .../xmodule/modulestore/split_mongo/split.py | 40 +++++++++++++++++-- .../tests/test_mixed_modulestore.py | 3 +- .../xmodule/modulestore/xml_exporter.py | 14 +++---- .../xmodule/modulestore/xml_importer.py | 2 + requirements/edx/github.txt | 2 +- 5 files changed, 48 insertions(+), 13 deletions(-) diff --git a/common/lib/xmodule/xmodule/modulestore/split_mongo/split.py b/common/lib/xmodule/xmodule/modulestore/split_mongo/split.py index 94d8380e23..544415da04 100644 --- a/common/lib/xmodule/xmodule/modulestore/split_mongo/split.py +++ b/common/lib/xmodule/xmodule/modulestore/split_mongo/split.py @@ -773,7 +773,7 @@ class SplitMongoModuleStore(SplitBulkWriteMixin, ModuleStoreWriteBase): ''' Gets the course descriptor for the course identified by the locator ''' - if not isinstance(course_id, CourseLocator): + if not isinstance(course_id, CourseLocator) or course_id.deprecated: # The supplied CourseKey is of the wrong type, so it can't possibly be stored in this modulestore. raise ItemNotFoundError(course_id) @@ -791,7 +791,7 @@ class SplitMongoModuleStore(SplitBulkWriteMixin, ModuleStoreWriteBase): Note: we return the course_id instead of a boolean here since the found course may have a different id than the given course_id when ignore_case is True. ''' - if not isinstance(course_id, CourseLocator): + if not isinstance(course_id, CourseLocator) or course_id.deprecated: # The supplied CourseKey is of the wrong type, so it can't possibly be stored in this modulestore. return False @@ -804,6 +804,10 @@ class SplitMongoModuleStore(SplitBulkWriteMixin, ModuleStoreWriteBase): the course or the block w/in the course do not exist for the given version. raises InsufficientSpecificationError if the usage_key does not id a block """ + if not isinstance(usage_key, BlockUsageLocator) or usage_key.deprecated: + # The supplied UsageKey is of the wrong type, so it can't possibly be stored in this modulestore. + return False + if usage_key.block_id is None: raise InsufficientSpecificationError(usage_key) try: @@ -823,7 +827,7 @@ class SplitMongoModuleStore(SplitBulkWriteMixin, ModuleStoreWriteBase): descendants. raises InsufficientSpecificationError or ItemNotFoundError """ - if not isinstance(usage_key, BlockUsageLocator): + if not isinstance(usage_key, BlockUsageLocator) or usage_key.deprecated: # The supplied UsageKey is of the wrong type, so it can't possibly be stored in this modulestore. raise ItemNotFoundError(usage_key) @@ -856,6 +860,10 @@ class SplitMongoModuleStore(SplitBulkWriteMixin, ModuleStoreWriteBase): For split, you can search by ``edited_by``, ``edited_on`` providing a function testing limits. """ + if not isinstance(course_locator, CourseLocator) or course_locator.deprecated: + # The supplied CourseKey is of the wrong type, so it can't possibly be stored in this modulestore. + return [] + course = self._lookup_course(course_locator) items = [] qualifiers = qualifiers.copy() if qualifiers else {} # copy the qualifiers (destructively manipulated here) @@ -910,6 +918,10 @@ class SplitMongoModuleStore(SplitBulkWriteMixin, ModuleStoreWriteBase): :param locator: BlockUsageLocator restricting search scope ''' + if not isinstance(locator, BlockUsageLocator) or locator.deprecated: + # The supplied locator is of the wrong type, so it can't possibly be stored in this modulestore. + raise ItemNotFoundError(locator) + course = self._lookup_course(locator.course_key) parent_id = self._get_parent_from_structure(BlockKey.from_usage_key(locator), course.structure) if parent_id is None: @@ -924,6 +936,10 @@ class SplitMongoModuleStore(SplitBulkWriteMixin, ModuleStoreWriteBase): """ Return an array of all of the orphans in the course. """ + if not isinstance(course_key, CourseLocator) or course_key.deprecated: + # The supplied CourseKey is of the wrong type, so it can't possibly be stored in this modulestore. + raise ItemNotFoundError(course_key) + detached_categories = [name for name, __ in XBlock.load_tagged_classes("detached")] course = self._lookup_course(course_key) items = set(course.structure['blocks'].keys()) @@ -952,6 +968,10 @@ class SplitMongoModuleStore(SplitBulkWriteMixin, ModuleStoreWriteBase): 'edited_on': when the course was originally created } """ + if not isinstance(course_key, CourseLocator) or course_key.deprecated: + # The supplied CourseKey is of the wrong type, so it can't possibly be stored in this modulestore. + raise ItemNotFoundError(course_key) + if not (course_key.course and course_key.run and course_key.org): return None index = self.get_course_index(course_key) @@ -969,6 +989,10 @@ class SplitMongoModuleStore(SplitBulkWriteMixin, ModuleStoreWriteBase): 'edited_on': when the change was made } """ + if not isinstance(course_key, CourseLocator) or course_key.deprecated: + # The supplied CourseKey is of the wrong type, so it can't possibly be stored in this modulestore. + raise ItemNotFoundError(course_key) + course = self._lookup_course(course_key).structure return { 'original_version': course['original_version'], @@ -987,6 +1011,10 @@ class SplitMongoModuleStore(SplitBulkWriteMixin, ModuleStoreWriteBase): 'edited_on': when the change was made } """ + if not isinstance(definition_locator, DefinitionLocator) or definition_locator.deprecated: + # The supplied locator is of the wrong type, so it can't possibly be stored in this modulestore. + raise ItemNotFoundError(definition_locator) + definition = self.db_connection.get_definition(definition_locator.definition_id) if definition is None: return None @@ -999,6 +1027,10 @@ class SplitMongoModuleStore(SplitBulkWriteMixin, ModuleStoreWriteBase): next versions, these do include those created for other courses. :param course_locator: ''' + if not isinstance(course_locator, CourseLocator) or course_locator.deprecated: + # The supplied CourseKey is of the wrong type, so it can't possibly be stored in this modulestore. + raise ItemNotFoundError(course_locator) + if version_history_depth < 1: return None if course_locator.version_guid is None: @@ -1882,7 +1914,7 @@ class SplitMongoModuleStore(SplitBulkWriteMixin, ModuleStoreWriteBase): change to this item, it raises a VersionConflictError unless force is True. In the force case, it forks the course but leaves the head pointer where it is (this change will not be in the course head). """ - if not isinstance(usage_locator, BlockUsageLocator): + if not isinstance(usage_locator, BlockUsageLocator) or usage_locator.deprecated: # The supplied UsageKey is of the wrong type, so it can't possibly be stored in this modulestore. raise ItemNotFoundError(usage_locator) diff --git a/common/lib/xmodule/xmodule/modulestore/tests/test_mixed_modulestore.py b/common/lib/xmodule/xmodule/modulestore/tests/test_mixed_modulestore.py index 7dc26c4b8c..d3b1469519 100644 --- a/common/lib/xmodule/xmodule/modulestore/tests/test_mixed_modulestore.py +++ b/common/lib/xmodule/xmodule/modulestore/tests/test_mixed_modulestore.py @@ -245,7 +245,8 @@ class TestMixedModuleStore(CourseComparisonTest): for course_id, course_key in self.course_locations.iteritems() # pylint: disable=maybe-no-member } - self.fake_location = self.course_locations[self.MONGO_COURSEID].course_key.make_usage_key('vertical', 'fake') + mongo_course_key = self.course_locations[self.MONGO_COURSEID].course_key + self.fake_location = self.store.make_course_key(mongo_course_key.org, mongo_course_key.course, mongo_course_key.run).make_usage_key('vertical', 'fake') self.xml_chapter_location = self.course_locations[self.XML_COURSEID1].replace( category='chapter', name='Overview' diff --git a/common/lib/xmodule/xmodule/modulestore/xml_exporter.py b/common/lib/xmodule/xmodule/modulestore/xml_exporter.py index 180d840781..6b1bb275a4 100644 --- a/common/lib/xmodule/xmodule/modulestore/xml_exporter.py +++ b/common/lib/xmodule/xmodule/modulestore/xml_exporter.py @@ -85,16 +85,16 @@ def export_to_xml(modulestore, contentstore, course_key, root_dir, course_dir): course_image_file.write(course_image.data) # export the static tabs - export_extra_content(export_fs, modulestore, xml_centric_course_key, 'static_tab', 'tabs', '.html') + export_extra_content(export_fs, modulestore, course_key, xml_centric_course_key, 'static_tab', 'tabs', '.html') # export the custom tags - export_extra_content(export_fs, modulestore, xml_centric_course_key, 'custom_tag_template', 'custom_tags') + export_extra_content(export_fs, modulestore, course_key, xml_centric_course_key, 'custom_tag_template', 'custom_tags') # export the course updates - export_extra_content(export_fs, modulestore, xml_centric_course_key, 'course_info', 'info', '.html') + export_extra_content(export_fs, modulestore, course_key, xml_centric_course_key, 'course_info', 'info', '.html') # export the 'about' data (e.g. overview, etc.) - export_extra_content(export_fs, modulestore, xml_centric_course_key, 'about', 'about', '.html') + export_extra_content(export_fs, modulestore, course_key, xml_centric_course_key, 'about', 'about', '.html') # export the grading policy course_run_policy_dir = policies_dir.makeopendir(course.location.name) @@ -183,13 +183,13 @@ def _export_field_content(xblock_item, item_dir): field_content_file.write(dumps(module_data.get(field_name, {}), cls=EdxJSONEncoder, sort_keys=True, indent=4)) -def export_extra_content(export_fs, modulestore, course_key, category_type, dirname, file_suffix=''): - items = modulestore.get_items(course_key, qualifiers={'category': category_type}) +def export_extra_content(export_fs, modulestore, source_course_key, dest_course_key, category_type, dirname, file_suffix=''): + items = modulestore.get_items(source_course_key, qualifiers={'category': category_type}) if len(items) > 0: item_dir = export_fs.makeopendir(dirname) for item in items: - adapt_references(item, course_key, export_fs) + adapt_references(item, dest_course_key, export_fs) with item_dir.open(item.location.name + file_suffix, 'w') as item_file: item_file.write(item.data.encode('utf8')) diff --git a/common/lib/xmodule/xmodule/modulestore/xml_importer.py b/common/lib/xmodule/xmodule/modulestore/xml_importer.py index 092fb08696..79304da535 100644 --- a/common/lib/xmodule/xmodule/modulestore/xml_importer.py +++ b/common/lib/xmodule/xmodule/modulestore/xml_importer.py @@ -42,6 +42,7 @@ from xmodule.modulestore.django import ASSET_IGNORE_REGEX from xmodule.modulestore.exceptions import DuplicateCourseError from xmodule.modulestore.mongo.base import MongoRevisionKey from xmodule.modulestore import ModuleStoreEnum +from xmodule.modulestore.exceptions import ItemNotFoundError log = logging.getLogger(__name__) @@ -588,6 +589,7 @@ def _import_course_draft( # IMPORTANT: Be sure to update the sequential in the NEW namespace seq_location = seq_location.map_into_course(target_course_id) + sequential = store.get_item(seq_location, depth=0) non_draft_location = module.location.map_into_course(target_course_id) diff --git a/requirements/edx/github.txt b/requirements/edx/github.txt index 7977dd679d..6fc67a9c07 100644 --- a/requirements/edx/github.txt +++ b/requirements/edx/github.txt @@ -29,7 +29,7 @@ -e git+https://github.com/edx-solutions/django-splash.git@7579d052afcf474ece1239153cffe1c89935bc4f#egg=django-splash -e git+https://github.com/edx/acid-block.git@459aff7b63db8f2c5decd1755706c1a64fb4ebb1#egg=acid-xblock -e git+https://github.com/edx/edx-ora2.git@release-2014-09-18T16.00#egg=edx-ora2 --e git+https://github.com/edx/opaque-keys.git@013e30b4c4909b55e03c409a90ec6ef805903e04#egg=opaque-keys +-e git+https://github.com/edx/opaque-keys.git@295d93170b2f6e57e3a2b9ba0a52087a4e8712c5#egg=opaque-keys -e git+https://github.com/edx/ease.git@97de68448e5495385ba043d3091f570a699d5b5f#egg=ease -e git+https://github.com/edx/i18n-tools.git@56f048af9b6868613c14aeae760548834c495011#egg=i18n-tools -e git+https://github.com/edx/edx-oauth2-provider.git@0.2.1#egg=oauth2-provider From 1a17b31a8ba4c3aa33b1591b81bcbe114a76a548 Mon Sep 17 00:00:00 2001 From: Calen Pennington Date: Thu, 25 Sep 2014 11:17:23 -0400 Subject: [PATCH 20/20] Make definitions and structures truly append-only --- .../split_mongo/mongo_connection.py | 14 +- .../xmodule/modulestore/split_mongo/split.py | 172 ++++++++++-------- .../test_split_modulestore_bulk_operations.py | 33 ++-- 3 files changed, 121 insertions(+), 98 deletions(-) diff --git a/common/lib/xmodule/xmodule/modulestore/split_mongo/mongo_connection.py b/common/lib/xmodule/xmodule/modulestore/split_mongo/mongo_connection.py index 1f04a263c8..a48b2f3ea7 100644 --- a/common/lib/xmodule/xmodule/modulestore/split_mongo/mongo_connection.py +++ b/common/lib/xmodule/xmodule/modulestore/split_mongo/mongo_connection.py @@ -4,6 +4,10 @@ Segregation of pymongo functions from the data modeling mechanisms for split mod import re import pymongo import time + +# Import this just to export it +from pymongo.errors import DuplicateKeyError # pylint: disable=unused-import + from contracts import check from functools import wraps from pymongo.errors import AutoReconnect @@ -182,11 +186,11 @@ class MongoConnection(object): } })] - def upsert_structure(self, structure): + def insert_structure(self, structure): """ - Update the db record for structure, creating that record if it doesn't already exist + Insert a new structure into the database. """ - self.structures.update({'_id': structure['_id']}, structure_to_mongo(structure), upsert=True) + self.structures.insert(structure_to_mongo(structure)) @autoretry_read() def get_course_index(self, key, ignore_case=False): @@ -274,11 +278,11 @@ class MongoConnection(object): """ return self.definitions.find({'$in': {'_id': definitions}}) - def upsert_definition(self, definition): + def insert_definition(self, definition): """ Create the definition in the db """ - self.definitions.update({'_id': definition['_id']}, definition, upsert=True) + self.definitions.insert(definition) def ensure_indexes(self): """ diff --git a/common/lib/xmodule/xmodule/modulestore/split_mongo/split.py b/common/lib/xmodule/xmodule/modulestore/split_mongo/split.py index 544415da04..563daafdd1 100644 --- a/common/lib/xmodule/xmodule/modulestore/split_mongo/split.py +++ b/common/lib/xmodule/xmodule/modulestore/split_mongo/split.py @@ -75,7 +75,7 @@ from xmodule.modulestore import ( from ..exceptions import ItemNotFoundError from .caching_descriptor_system import CachingDescriptorSystem -from xmodule.modulestore.split_mongo.mongo_connection import MongoConnection +from xmodule.modulestore.split_mongo.mongo_connection import MongoConnection, DuplicateKeyError from xmodule.modulestore.split_mongo import BlockKey, CourseEnvelope from xmodule.error_module import ErrorDescriptor from collections import defaultdict @@ -226,10 +226,22 @@ class SplitBulkWriteMixin(BulkOperationsMixin): """ # If the content is dirty, then update the database for _id in bulk_write_record.structures.viewkeys() - bulk_write_record.structures_in_db: - self.db_connection.upsert_structure(bulk_write_record.structures[_id]) + try: + self.db_connection.insert_structure(bulk_write_record.structures[_id]) + except DuplicateKeyError: + # We may not have looked up this structure inside this bulk operation, and thus + # didn't realize that it was already in the database. That's OK, the store is + # append only, so if it's already been written, we can just keep going. + log.debug("Attempted to insert duplicate structure %s", _id) for _id in bulk_write_record.definitions.viewkeys() - bulk_write_record.definitions_in_db: - self.db_connection.upsert_definition(bulk_write_record.definitions[_id]) + try: + self.db_connection.insert_definition(bulk_write_record.definitions[_id]) + except DuplicateKeyError: + # We may not have looked up this definition inside this bulk operation, and thus + # didn't realize that it was already in the database. That's OK, the store is + # append only, so if it's already been written, we can just keep going. + log.debug("Attempted to insert duplicate definition %s", _id) if bulk_write_record.index is not None and bulk_write_record.index != bulk_write_record.initial_index: if bulk_write_record.initial_index is None: @@ -295,7 +307,7 @@ class SplitBulkWriteMixin(BulkOperationsMixin): if bulk_write_record.active: bulk_write_record.structures[structure['_id']] = structure else: - self.db_connection.upsert_structure(structure) + self.db_connection.insert_structure(structure) def get_definition(self, course_key, definition_guid): """ @@ -323,7 +335,7 @@ class SplitBulkWriteMixin(BulkOperationsMixin): definition_guid = course_key.as_object_id(definition_guid) return self.db_connection.get_definition(definition_guid) - def get_definitions(self, ids): + def get_definitions(self, course_key, ids): """ Return all definitions that specified in ``ids``. @@ -331,13 +343,16 @@ class SplitBulkWriteMixin(BulkOperationsMixin): the cached version will be preferred. Arguments: + course_key (:class:`.CourseKey`): The course that these definitions are being loaded + for (to respect bulk operations). ids (list): A list of definition ids """ definitions = [] ids = set(ids) - for _, record in self._active_records: - for definition in record.definitions.values(): + bulk_write_record = self._get_bulk_ops_record(course_key) + if bulk_write_record.active: + for definition in bulk_write_record.definitions.values(): definition_id = definition.get('_id') if definition_id in ids: ids.remove(definition_id) @@ -356,7 +371,7 @@ class SplitBulkWriteMixin(BulkOperationsMixin): if bulk_write_record.active: bulk_write_record.definitions[definition['_id']] = definition else: - self.db_connection.upsert_definition(definition) + self.db_connection.insert_definition(definition) def version_structure(self, course_key, structure, user_id): """ @@ -596,10 +611,13 @@ class SplitMongoModuleStore(SplitBulkWriteMixin, ModuleStoreWriteBase): if not lazy: # Load all descendants by id - descendent_definitions = self.get_definitions([ - block['definition'] - for block in new_module_data.itervalues() - ]) + descendent_definitions = self.get_definitions( + course_key, + [ + block['definition'] + for block in new_module_data.itervalues() + ] + ) # turn into a map definitions = {definition['_id']: definition for definition in descendent_definitions} @@ -1163,16 +1181,17 @@ class SplitMongoModuleStore(SplitBulkWriteMixin, ModuleStoreWriteBase): new_def_data = self._serialize_fields(old_definition['block_type'], new_def_data) if needs_saved(): - # new id to create new version - old_definition['_id'] = ObjectId() - old_definition['fields'] = new_def_data - old_definition['edit_info']['edited_by'] = user_id - old_definition['edit_info']['edited_on'] = datetime.datetime.now(UTC) + # Do a deep copy so that we don't corrupt the cached version of the definition + new_definition = copy.deepcopy(old_definition) + new_definition['_id'] = ObjectId() + new_definition['fields'] = new_def_data + new_definition['edit_info']['edited_by'] = user_id + new_definition['edit_info']['edited_on'] = datetime.datetime.now(UTC) # previous version id - old_definition['edit_info']['previous_version'] = definition_locator.definition_id - old_definition['schema_version'] = self.SCHEMA_VERSION - self.update_definition(course_key, old_definition) - return DefinitionLocator(old_definition['block_type'], old_definition['_id']), True + new_definition['edit_info']['previous_version'] = definition_locator.definition_id + new_definition['schema_version'] = self.SCHEMA_VERSION + self.update_definition(course_key, new_definition) + return DefinitionLocator(new_definition['block_type'], new_definition['_id']), True else: return definition_locator, False @@ -1482,7 +1501,7 @@ class SplitMongoModuleStore(SplitBulkWriteMixin, ModuleStoreWriteBase): if block_fields is not None: root_block['fields'].update(self._serialize_fields(root_category, block_fields)) if definition_fields is not None: - definition = self.get_definition(locator, root_block['definition']) + definition = copy.deepcopy(self.get_definition(locator, root_block['definition'])) definition['fields'].update(definition_fields) definition['edit_info']['previous_version'] = definition['_id'] definition['edit_info']['edited_by'] = user_id @@ -1839,65 +1858,66 @@ class SplitMongoModuleStore(SplitBulkWriteMixin, ModuleStoreWriteBase): """ # get the destination's index, and source and destination structures. with self.bulk_operations(source_course): - with self.bulk_operations(destination_course): - source_structure = self._lookup_course(source_course).structure - index_entry = self.get_course_index(destination_course) - if index_entry is None: - # brand new course - raise ItemNotFoundError(destination_course) - if destination_course.branch not in index_entry['versions']: - # must be copying the dag root if there's no current dag - root_block_key = source_structure['root'] - if not any(root_block_key == BlockKey.from_usage_key(subtree) for subtree in subtree_list): - raise ItemNotFoundError(u'Must publish course root {}'.format(root_block_key)) - root_source = source_structure['blocks'][root_block_key] - # create branch - destination_structure = self._new_structure( - user_id, root_block_key, - # leave off the fields b/c the children must be filtered - definition_id=root_source['definition'], - ) - else: - destination_structure = self._lookup_course(destination_course).structure - destination_structure = self.version_structure(destination_course, destination_structure, user_id) + source_structure = self._lookup_course(source_course).structure - if blacklist != EXCLUDE_ALL: - blacklist = [BlockKey.from_usage_key(shunned) for shunned in blacklist or []] - # iterate over subtree list filtering out blacklist. - orphans = set() - destination_blocks = destination_structure['blocks'] - for subtree_root in subtree_list: - if BlockKey.from_usage_key(subtree_root) != source_structure['root']: - # find the parents and put root in the right sequence - parent = self._get_parent_from_structure(BlockKey.from_usage_key(subtree_root), source_structure) - if parent is not None: # may be a detached category xblock - if not parent in destination_blocks: - raise ItemNotFoundError(parent) - orphans.update( - self._sync_children( - source_structure['blocks'][parent], - destination_blocks[parent], - BlockKey.from_usage_key(subtree_root) - ) + with self.bulk_operations(destination_course): + index_entry = self.get_course_index(destination_course) + if index_entry is None: + # brand new course + raise ItemNotFoundError(destination_course) + if destination_course.branch not in index_entry['versions']: + # must be copying the dag root if there's no current dag + root_block_key = source_structure['root'] + if not any(root_block_key == BlockKey.from_usage_key(subtree) for subtree in subtree_list): + raise ItemNotFoundError(u'Must publish course root {}'.format(root_block_key)) + root_source = source_structure['blocks'][root_block_key] + # create branch + destination_structure = self._new_structure( + user_id, root_block_key, + # leave off the fields b/c the children must be filtered + definition_id=root_source['definition'], + ) + else: + destination_structure = self._lookup_course(destination_course).structure + destination_structure = self.version_structure(destination_course, destination_structure, user_id) + + if blacklist != EXCLUDE_ALL: + blacklist = [BlockKey.from_usage_key(shunned) for shunned in blacklist or []] + # iterate over subtree list filtering out blacklist. + orphans = set() + destination_blocks = destination_structure['blocks'] + for subtree_root in subtree_list: + if BlockKey.from_usage_key(subtree_root) != source_structure['root']: + # find the parents and put root in the right sequence + parent = self._get_parent_from_structure(BlockKey.from_usage_key(subtree_root), source_structure) + if parent is not None: # may be a detached category xblock + if not parent in destination_blocks: + raise ItemNotFoundError(parent) + orphans.update( + self._sync_children( + source_structure['blocks'][parent], + destination_blocks[parent], + BlockKey.from_usage_key(subtree_root) ) - # update/create the subtree and its children in destination (skipping blacklist) - orphans.update( - self._copy_subdag( - user_id, destination_structure['_id'], - BlockKey.from_usage_key(subtree_root), - source_structure['blocks'], - destination_blocks, - blacklist ) + # update/create the subtree and its children in destination (skipping blacklist) + orphans.update( + self._copy_subdag( + user_id, destination_structure['_id'], + BlockKey.from_usage_key(subtree_root), + source_structure['blocks'], + destination_blocks, + blacklist ) - # remove any remaining orphans - for orphan in orphans: - # orphans will include moved as well as deleted xblocks. Only delete the deleted ones. - self._delete_if_true_orphan(orphan, destination_structure) + ) + # remove any remaining orphans + for orphan in orphans: + # orphans will include moved as well as deleted xblocks. Only delete the deleted ones. + self._delete_if_true_orphan(orphan, destination_structure) - # update the db - self.update_structure(destination_course, destination_structure) - self._update_head(destination_course, index_entry, destination_course.branch, destination_structure['_id']) + # update the db + self.update_structure(destination_course, destination_structure) + self._update_head(destination_course, index_entry, destination_course.branch, destination_structure['_id']) def delete_item(self, usage_locator, user_id, force=False): """ diff --git a/common/lib/xmodule/xmodule/modulestore/tests/test_split_modulestore_bulk_operations.py b/common/lib/xmodule/xmodule/modulestore/tests/test_split_modulestore_bulk_operations.py index f5e6af6e8c..5ff112c961 100644 --- a/common/lib/xmodule/xmodule/modulestore/tests/test_split_modulestore_bulk_operations.py +++ b/common/lib/xmodule/xmodule/modulestore/tests/test_split_modulestore_bulk_operations.py @@ -64,7 +64,7 @@ class TestBulkWriteMixinClosed(TestBulkWriteMixin): # call through to the db_connection. It should also clear the # system cache self.bulk.update_structure(self.course_key, self.structure) - self.assertConnCalls(call.upsert_structure(self.structure)) + self.assertConnCalls(call.insert_structure(self.structure)) self.clear_cache.assert_called_once_with(self.structure['_id']) @ddt.data('deadbeef1234' * 2, u'deadbeef1234' * 2, ObjectId()) @@ -79,7 +79,7 @@ class TestBulkWriteMixinClosed(TestBulkWriteMixin): # Writing a definition when no bulk operation is active should just # call through to the db_connection. self.bulk.update_definition(self.course_key, self.definition) - self.assertConnCalls(call.upsert_definition(self.definition)) + self.assertConnCalls(call.insert_definition(self.definition)) @ddt.data(True, False) def test_no_bulk_read_index(self, ignore_case): @@ -128,7 +128,7 @@ class TestBulkWriteMixinClosed(TestBulkWriteMixin): self.bulk.update_structure(self.course_key, self.structure) self.assertConnCalls() self.bulk._end_bulk_operation(self.course_key) - self.assertConnCalls(call.upsert_structure(self.structure)) + self.assertConnCalls(call.insert_structure(self.structure)) def test_write_multiple_structures_on_close(self): self.conn.get_course_index.return_value = None @@ -140,7 +140,7 @@ class TestBulkWriteMixinClosed(TestBulkWriteMixin): self.assertConnCalls() self.bulk._end_bulk_operation(self.course_key) self.assertItemsEqual( - [call.upsert_structure(self.structure), call.upsert_structure(other_structure)], + [call.insert_structure(self.structure), call.insert_structure(other_structure)], self.conn.mock_calls ) @@ -154,7 +154,7 @@ class TestBulkWriteMixinClosed(TestBulkWriteMixin): self.assertConnCalls() self.bulk._end_bulk_operation(self.course_key) self.assertConnCalls( - call.upsert_definition(self.definition), + call.insert_definition(self.definition), call.update_course_index( {'versions': {self.course_key.branch: self.definition['_id']}}, from_index=original_index @@ -173,8 +173,8 @@ class TestBulkWriteMixinClosed(TestBulkWriteMixin): self.bulk._end_bulk_operation(self.course_key) self.assertItemsEqual( [ - call.upsert_definition(self.definition), - call.upsert_definition(other_definition), + call.insert_definition(self.definition), + call.insert_definition(other_definition), call.update_course_index( {'versions': {'a': self.definition['_id'], 'b': other_definition['_id']}}, from_index=original_index @@ -190,7 +190,7 @@ class TestBulkWriteMixinClosed(TestBulkWriteMixin): self.bulk.update_definition(self.course_key, self.definition) self.assertConnCalls() self.bulk._end_bulk_operation(self.course_key) - self.assertConnCalls(call.upsert_definition(self.definition)) + self.assertConnCalls(call.insert_definition(self.definition)) def test_write_multiple_definitions_on_close(self): self.conn.get_course_index.return_value = None @@ -202,7 +202,7 @@ class TestBulkWriteMixinClosed(TestBulkWriteMixin): self.assertConnCalls() self.bulk._end_bulk_operation(self.course_key) self.assertItemsEqual( - [call.upsert_definition(self.definition), call.upsert_definition(other_definition)], + [call.insert_definition(self.definition), call.insert_definition(other_definition)], self.conn.mock_calls ) @@ -216,7 +216,7 @@ class TestBulkWriteMixinClosed(TestBulkWriteMixin): self.assertConnCalls() self.bulk._end_bulk_operation(self.course_key) self.assertConnCalls( - call.upsert_structure(self.structure), + call.insert_structure(self.structure), call.update_course_index( {'versions': {self.course_key.branch: self.structure['_id']}}, from_index=original_index @@ -235,8 +235,8 @@ class TestBulkWriteMixinClosed(TestBulkWriteMixin): self.bulk._end_bulk_operation(self.course_key) self.assertItemsEqual( [ - call.upsert_structure(self.structure), - call.upsert_structure(other_structure), + call.insert_structure(self.structure), + call.insert_structure(other_structure), call.update_course_index( {'versions': {'a': self.structure['_id'], 'b': other_structure['_id']}}, from_index=original_index @@ -397,13 +397,12 @@ class TestBulkWriteMixinFindMethods(TestBulkWriteMixin): active_definition = lambda _id: {'active': 'definition', '_id': _id} db_definitions = [db_definition(_id) for _id in db_ids if _id not in active_ids] + self.bulk._begin_bulk_operation(self.course_key) for n, _id in enumerate(active_ids): - course_key = CourseLocator('org', 'course', 'run{}'.format(n)) - self.bulk._begin_bulk_operation(course_key) - self.bulk.update_definition(course_key, active_definition(_id)) + self.bulk.update_definition(self.course_key, active_definition(_id)) self.conn.get_definitions.return_value = db_definitions - results = self.bulk.get_definitions(search_ids) + results = self.bulk.get_definitions(self.course_key, search_ids) self.conn.get_definitions.assert_called_once_with(list(set(search_ids) - set(active_ids))) for _id in active_ids: if _id in search_ids: @@ -669,7 +668,7 @@ class TestBulkWriteMixinOpen(TestBulkWriteMixin): index_copy['versions']['draft'] = index['versions']['published'] self.bulk.update_course_index(self.course_key, index_copy) self.bulk._end_bulk_operation(self.course_key) - self.conn.upsert_structure.assert_called_once_with(published_structure) + self.conn.insert_structure.assert_called_once_with(published_structure) self.conn.update_course_index.assert_called_once_with(index_copy, from_index=self.conn.get_course_index.return_value) self.conn.get_course_index.assert_called_once_with(self.course_key)