Make split_mongo assert block identity uniqueness only over (block_type,…

Make split_mongo assert block identity uniqueness only over (block_type, block_id) tuples [LMS-11364]

cms/djangoapps/contentstore/tests/test_crud.py

@@ -198,9 +198,7 @@ class TemplateTests(unittest.TestCase):
 
         second_problem = persistent_factories.ItemFactory.create(
             display_name='problem 2',
-            parent_location=BlockUsageLocator.make_relative(
-                test_course.location.version_agnostic(), block_type='problem', block_id=sub.location.block_id
-            ),
+            parent_location=sub.location.version_agnostic(),
             user_id='testbot', category='problem',
             data="<problem></problem>"
         )
@@ -208,6 +206,7 @@ class TemplateTests(unittest.TestCase):
         # The draft course root has 2 revisions: the published revision, and then the subsequent
         # changes to the draft revision
         version_history = self.split_store.get_block_generations(test_course.location)
+        self.assertIsNotNone(version_history)
         self.assertEqual(version_history.locator.version_guid, test_course.location.version_guid)
         self.assertEqual(len(version_history.children), 1)
         self.assertEqual(version_history.children[0].children, [])

cms/wsgi.py

+# Disable PyContract contract checking when running as a webserver
+import contracts
+contracts.disable_all()
+
 import os
 
 os.environ.setdefault("DJANGO_SETTINGS_MODULE", "cms.envs.aws")

common/lib/xmodule/xmodule/modulestore/split_mongo/__init__.py

 """
 General utilities
 """
-import urllib
 
+from collections import namedtuple
+from contracts import contract, check
+from opaque_keys.edx.locator import BlockUsageLocator
 
-def encode_key_for_mongo(fieldname):
-    """
-    Fieldnames in mongo cannot have periods nor dollar signs. So encode them.
-    :param fieldname: an atomic field name. Note, don't pass structured paths as it will flatten them
-    """
-    for char in [".", "$"]:
-        fieldname = fieldname.replace(char, '%{:02x}'.format(ord(char)))
-    return fieldname
 
+class BlockKey(namedtuple('BlockKey', 'type id')):
+    __slots__ = ()
+
+    @contract(type="string[>0]")
+    def __new__(cls, type, id):
+        return super(BlockKey, cls).__new__(cls, type, id)
+
+
+    @classmethod
+    @contract(usage_key=BlockUsageLocator)
+    def from_usage_key(cls, usage_key):
+        return cls(usage_key.block_type, usage_key.block_id)
 
-def decode_key_from_mongo(fieldname):
-    """
-    The inverse of encode_key_for_mongo
-    :param fieldname: with period and dollar escaped
-    """
-    return urllib.unquote(fieldname)

common/lib/xmodule/xmodule/modulestore/split_mongo/definition_lazy_loader.py

@@ -28,6 +28,6 @@ class DefinitionLazyLoader(object):
 
     def as_son(self):
         return SON((
-            ('category', self.definition_locator.block_type),
+            ('block_type', self.definition_locator.block_type),
             ('definition', self.definition_locator.definition_id)
         ))

common/lib/xmodule/xmodule/modulestore/split_mongo/mongo_connection.py

@@ -4,7 +4,68 @@ Segregation of pymongo functions from the data modeling mechanisms for split mod
 import re
 import pymongo
 from bson import son
+from contracts import check
 from xmodule.exceptions import HeartbeatFailure
+from xmodule.modulestore.split_mongo import BlockKey
+
+
+def structure_from_mongo(structure):
+    """
+    Converts the 'blocks' key from a list [block_data] to a map
+        {BlockKey: block_data}.
+    Converts 'root' from [block_type, block_id] to BlockKey.
+    Converts 'blocks.*.fields.children' from [[block_type, block_id]] to [BlockKey].
+    N.B. Does not convert any other ReferenceFields (because we don't know which fields they are at this level).
+    """
+    check('seq[2]', structure['root'])
+    check('list(dict)', structure['blocks'])
+    for block in structure['blocks']:
+        if 'children' in block['fields']:
+            check('list(list[2])', block['fields']['children'])
+
+    structure['root'] = BlockKey(*structure['root'])
+    new_blocks = {}
+    for block in structure['blocks']:
+        if 'children' in block['fields']:
+            block['fields']['children'] = [BlockKey(*child) for child in block['fields']['children']]
+        new_blocks[BlockKey(block['block_type'], block.pop('block_id'))] = block
+    structure['blocks'] = new_blocks
+
+    return structure
+
+
+def structure_to_mongo(structure):
+    """
+    Converts the 'blocks' key from a map {BlockKey: block_data} to
+        a list [block_data], inserting BlockKey.type as 'block_type'
+        and BlockKey.id as 'block_id'.
+    Doesn't convert 'root', since namedtuple's can be inserted
+        directly into mongo.
+    """
+    check('BlockKey', structure['root'])
+    check('dict(BlockKey: dict)', structure['blocks'])
+    for block in structure['blocks'].itervalues():
+        if 'children' in block['fields']:
+            check('list(BlockKey)', block['fields']['children'])
+
+    new_structure = dict(structure)
+    new_structure['blocks'] = []
+
+    for block_key, block in structure['blocks'].iteritems():
+        new_block = dict(block)
+        new_block.setdefault('block_type', block_key.type)
+        new_block['block_id'] = block_key.id
+        new_structure['blocks'].append(new_block)
+
+    return new_structure
+
+
+def definition_from_mongo(definition):
+    """
+    Converts 'fields.children' from a list [[block_type, block_id]] to [BlockKey]
+    """
+    new_def = dict(definition)
+
 
 class MongoConnection(object):
     """
@@ -55,7 +116,7 @@ class MongoConnection(object):
         """
         Get the structure from the persistence mechanism whose id is the given key
         """
-        return self.structures.find_one({'_id': key})
+        return structure_from_mongo(self.structures.find_one({'_id': key}))
 
     def find_structures_by_id(self, ids):
         """
@@ -64,7 +125,7 @@ class MongoConnection(object):
         Arguments:
             ids (list): A list of structure ids
         """
-        return self.structures.find({'_id': {'$in': ids}})
+        return [structure_from_mongo(structure) for structure in self.structures.find({'_id': {'$in': ids}})]
 
     def find_structures_derived_from(self, ids):
         """
@@ -73,26 +134,32 @@ class MongoConnection(object):
         Arguments:
             ids (list): A list of structure ids
         """
-        return self.structures.find({'previous_version': {'$in': ids}})
+        return [structure_from_mongo(structure) for structure in self.structures.find({'previous_version': {'$in': ids}})]
 
-    def find_ancestor_structures(self, original_version, block_id):
+    def find_ancestor_structures(self, original_version, block_key):
         """
-        Find all structures that originated from ``original_version`` that contain ``block_id``.
+        Find all structures that originated from ``original_version`` that contain ``block_key``.
 
         Arguments:
             original_version (str or ObjectID): The id of a structure
-            block_id (str): The id of the block in question
+            block_key (BlockKey): The id of the block in question
         """
-        return self.structures.find({
+        return [structure_from_mongo(structure) for structure in self.structures.find({
             'original_version': original_version,
-            'blocks.{}.edit_info.update_version'.format(block_id): {'$exists': True}
-        })
+            'blocks': {
+                '$elemMatch': {
+                    'block_id': block_key.id,
+                    'block_type': block_key.type,
+                    'edit_info.update_version': {'$exists': True},
+                }
+            }
+        })]
 
     def upsert_structure(self, structure):
         """
         Update the db record for structure, creating that record if it doesn't already exist
         """
-        self.structures.update({'_id': structure['_id']}, structure, upsert=True)
+        self.structures.update({'_id': structure['_id']}, structure_to_mongo(structure), upsert=True)
 
     def get_course_index(self, key, ignore_case=False):
         """

common/lib/xmodule/xmodule/modulestore/split_mongo/split_draft.py

@@ -10,6 +10,7 @@ from xmodule.modulestore.draft_and_published import (
     ModuleStoreDraftAndPublished, DIRECT_ONLY_CATEGORIES, UnsupportedRevisionError
 )
 from opaque_keys.edx.locator import CourseLocator
+from xmodule.modulestore.split_mongo import BlockKey
 
 
 class DraftVersioningModuleStore(ModuleStoreDraftAndPublished, SplitMongoModuleStore):
@@ -241,15 +242,15 @@ class DraftVersioningModuleStore(ModuleStoreDraftAndPublished, SplitMongoModuleS
         def get_course(branch_name):
             return self._lookup_course(xblock.location.course_key.for_branch(branch_name))['structure']
 
-        def get_block(course_structure, block_id):
-            return self._get_block_from_structure(course_structure, block_id)
+        def get_block(course_structure, block_key):
+            return self._get_block_from_structure(course_structure, block_key)
 
         draft_course = get_course(ModuleStoreEnum.BranchName.draft)
         published_course = get_course(ModuleStoreEnum.BranchName.published)
 
-        def has_changes_subtree(block_id):
-            draft_block = get_block(draft_course, block_id)
-            published_block = get_block(published_course, block_id)
+        def has_changes_subtree(block_key):
+            draft_block = get_block(draft_course, block_key)
+            published_block = get_block(published_course, block_key)
             if not published_block:
                 return True
 
@@ -265,7 +266,7 @@ class DraftVersioningModuleStore(ModuleStoreDraftAndPublished, SplitMongoModuleS
 
             return False
 
-        return has_changes_subtree(xblock.location.block_id)
+        return has_changes_subtree(BlockKey.from_usage_key(xblock.location))
 
     def publish(self, location, user_id, blacklist=None, **kwargs):
         """
@@ -316,7 +317,10 @@ class DraftVersioningModuleStore(ModuleStoreDraftAndPublished, SplitMongoModuleS
             published_course_structure = self._lookup_course(
                 location.course_key.for_branch(ModuleStoreEnum.BranchName.published)
             )['structure']
-            published_block = self._get_block_from_structure(published_course_structure, location.block_id)
+            published_block = self._get_block_from_structure(
+                published_course_structure,
+                BlockKey.from_usage_key(location)
+            )
             if published_block is None:
                 raise InvalidVersionError(location)
 
@@ -325,7 +329,7 @@ class DraftVersioningModuleStore(ModuleStoreDraftAndPublished, SplitMongoModuleS
             new_structure = self.version_structure(draft_course_key, draft_course_structure, user_id)
 
             # remove the block and its descendants from the new structure
-            self._remove_subtree(location.block_id, new_structure['blocks'])
+            self._remove_subtree(BlockKey.from_usage_key(location), new_structure['blocks'])
 
             # copy over the block and its descendants from the published branch
             def copy_from_published(root_block_id):
@@ -341,7 +345,7 @@ class DraftVersioningModuleStore(ModuleStoreDraftAndPublished, SplitMongoModuleS
                 for child_block_id in block.setdefault('fields', {}).get('children', []):
                     copy_from_published(child_block_id)
 
-            copy_from_published(location.block_id)
+            copy_from_published(BlockKey.from_usage_key(location))
 
             # update course structure and index
             self.update_structure(draft_course_key, new_structure)
@@ -389,7 +393,7 @@ class DraftVersioningModuleStore(ModuleStoreDraftAndPublished, SplitMongoModuleS
 
     def _get_head(self, xblock, branch):
         course_structure = self._lookup_course(xblock.location.course_key.for_branch(branch))['structure']
-        return self._get_block_from_structure(course_structure, xblock.location.block_id)
+        return self._get_block_from_structure(course_structure, BlockKey.from_usage_key(xblock.location))
 
     def _get_version(self, block):
         """
@@ -429,7 +433,7 @@ class DraftVersioningModuleStore(ModuleStoreDraftAndPublished, SplitMongoModuleS
             partitioned_fields = self.partition_fields_by_scope(block_type, fields)
             course_key = self._map_revision_to_branch(course_key)  # cast to branch_setting
             return self._update_item_from_fields(
-                user_id, course_key, block_type, block_id, partitioned_fields, None, allow_not_found=True, force=True
+                user_id, course_key, BlockKey(block_type, block_id), partitioned_fields, None, allow_not_found=True, force=True
             ) or self.get_item(new_usage_key)
 
     def compute_published_info_internal(self, xblock):

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

@@ -187,7 +187,7 @@ class BulkAssertionManager(object):
 
     def assertEqual(self, expected, actual, description=None):
         if description is None:
-            description = "{!r} does not equal {!r}".format(expected, actual)
+            description = u"{!r} does not equal {!r}".format(expected, actual)
         if expected != actual:
             self._equal_expected.append((description, expected))
             self._equal_actual.append((description, actual))
@@ -291,8 +291,11 @@ class CourseComparisonTest(BulkAssertionTest):
         with self.bulk_assertions():
             self.assertEqual(len(expected_items), len(actual_items))
 
+            def map_key(usage_key):
+                return (usage_key.block_type, usage_key.block_id)
+
             actual_item_map = {
-                item.location.block_id: item
+                map_key(item.location): item
                 for item in actual_items
             }
 
@@ -302,12 +305,12 @@ class CourseComparisonTest(BulkAssertionTest):
                 # modulestore actual's come from here; so, assume old mongo and if that fails, assume split
                 if expected_item.location.category == 'course':
                     actual_item_location = actual_item_location.replace(name=actual_item_location.run)
-                actual_item = actual_item_map.get(actual_item_location.block_id)
+                actual_item = actual_item_map.get(map_key(actual_item_location))
                 # must be split
                 if actual_item is None and expected_item.location.category == 'course':
                     actual_item_location = actual_item_location.replace(name='course')
-                    actual_item = actual_item_map.get(actual_item_location.block_id)
-                self.assertIsNotNone(actual_item, u'cannot find {} in {}'.format(actual_item_location, actual_item_map))
+                    actual_item = actual_item_map.get(map_key(actual_item_location))
+                self.assertIsNotNone(actual_item, u'cannot find {} in {}'.format(map_key(actual_item_location), actual_item_map))
 
                 # compare fields
                 self.assertEqual(expected_item.fields, actual_item.fields)

common/test/data/manual-testing-complete/html/Duplicate_URL_Name.xml

 <html url_name="Duplicate_URL_Name">
     This is test html.
-</html>
\ No newline at end of file
+</html>

common/test/data/manual-testing-complete/vertical/Duplicate_URL_Name.xml

 <vertical display_name="Duplicate URL Name">
     <html url_name="Duplicate_URL_Name"/>
-</vertical>
\ No newline at end of file
+</vertical>

lms/wsgi.py

+# Disable PyContract contract checking when running as a webserver
+import contracts
+contracts.disable_all()
+
 import os
 
 os.environ.setdefault("DJANGO_SETTINGS_MODULE", "lms.envs.aws")

requirements/edx/base.txt

@@ -127,6 +127,7 @@ rednose==0.3
 selenium==2.42.1
 splinter==0.5.4
 testtools==0.9.34
+PyContracts==1.6.4
 
 # Used for Segment.io analytics
 analytics-python==0.4.4