Merge pull request #2167 from edx/dhm/split_block_id

Encode periods and $ in block_id map

common/lib/xmodule/xmodule/modulestore/loc_mapper_store.py

@@ -156,7 +156,7 @@ class LocMapperStore(object):
                     entry = item
                     break
 
-        block_id = entry['block_map'].get(self._encode_for_mongo(location.name))
+        block_id = entry['block_map'].get(self.encode_key_for_mongo(location.name))
         if block_id is None:
             if add_entry_if_missing:
                 block_id = self._add_to_block_map(location, location_id, entry['block_map'])
@@ -231,7 +231,7 @@ class LocMapperStore(object):
                         candidate['_id']['org'],
                         candidate['_id']['course'],
                         category,
-                        self._decode_from_mongo(old_name),
+                        self.decode_key_from_mongo(old_name),
                         None)
                     published_locator = BlockUsageLocator(
                         candidate['course_id'], branch=candidate['prod_branch'], block_id=block_id
@@ -261,7 +261,7 @@ class LocMapperStore(object):
             # if 2 different category locations had same name, then they'll collide. Make the later
             # mapped ones unique
             block_id = self._verify_uniqueness(location.name, block_map)
-        encoded_location_name = self._encode_for_mongo(location.name)
+        encoded_location_name = self.encode_key_for_mongo(location.name)
         block_map.setdefault(encoded_location_name, {})[location.category] = block_id
         self.location_map.update(location_id, {'$set': {'block_map': block_map}})
         return block_id
@@ -331,7 +331,8 @@ class LocMapperStore(object):
                 return self._verify_uniqueness(name, block_map)
         return name
 
-    def _encode_for_mongo(self, fieldname):
+    @staticmethod
+    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
@@ -340,9 +341,10 @@ class LocMapperStore(object):
             fieldname = fieldname.replace(char, '%{:02x}'.format(ord(char)))
         return fieldname
 
-    def _decode_from_mongo(self, fieldname):
+    @staticmethod
+    def decode_key_from_mongo(fieldname):
         """
-        The inverse of _encode_for_mongo
+        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/caching_descriptor_system.py

@@ -8,6 +8,7 @@ from xblock.runtime import KvsFieldData, IdReader
 from ..exceptions import ItemNotFoundError
 from .split_mongo_kvs import SplitMongoKVS
 from xblock.fields import ScopeIds
+from xmodule.modulestore.loc_mapper_store import LocMapperStore
 
 log = logging.getLogger(__name__)
 
@@ -63,7 +64,9 @@ class CachingDescriptorSystem(MakoDescriptorSystem):
         # Compute inheritance
         modulestore.inherit_settings(
             course_entry['structure'].get('blocks', {}),
-            course_entry['structure'].get('blocks', {}).get(course_entry['structure'].get('root'))
+            course_entry['structure'].get('blocks', {}).get(
+                LocMapperStore.encode_key_for_mongo(course_entry['structure'].get('root'))
+            )
         )
         self.default_class = default_class
         self.local_modules = {}

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

@@ -56,7 +56,7 @@ import copy
 from pytz import UTC
 
 from xmodule.errortracker import null_error_tracker
-from xmodule.x_module import XModuleDescriptor, prefer_xmodules
+from xmodule.x_module import prefer_xmodules
 from xmodule.modulestore.locator import BlockUsageLocator, DefinitionLocator, CourseLocator, VersionTree, LocalId
 from xmodule.modulestore.exceptions import InsufficientSpecificationError, VersionConflictError, DuplicateItemError
 from xmodule.modulestore import inheritance, ModuleStoreWriteBase, Location, SPLIT_MONGO_MODULESTORE_TYPE
@@ -69,6 +69,7 @@ from xblock.runtime import Mixologist
 from bson.objectid import ObjectId
 from xmodule.modulestore.split_mongo.mongo_connection import MongoConnection
 from xblock.core import XBlock
+from xmodule.modulestore.loc_mapper_store import LocMapperStore
 
 log = logging.getLogger(__name__)
 #==============================================================================
@@ -343,7 +344,7 @@ class SplitMongoModuleStore(ModuleStoreWriteBase):
             # this error only occurs if the course does not exist
             return False
 
-        return course_structure['blocks'].get(block_location.block_id) is not None
+        return self._get_block_from_structure(course_structure, block_location.block_id) is not None
 
     def get_item(self, location, depth=0):
         """
@@ -432,7 +433,10 @@ class SplitMongoModuleStore(ModuleStoreWriteBase):
         '''
         course = self._lookup_course(locator)
         items = self._get_parents_from_structure(locator.block_id, course['structure'])
-        return [BlockUsageLocator(url=locator.as_course_locator(), block_id=parent_id)
+        return [BlockUsageLocator(
+                    url=locator.as_course_locator(),
+                    block_id=LocMapperStore.decode_key_from_mongo(parent_id),
+                )
                 for parent_id in items]
 
     def get_orphans(self, package_id, detached_categories, branch):
@@ -442,12 +446,12 @@ class SplitMongoModuleStore(ModuleStoreWriteBase):
         :param package_id:
         """
         course = self._lookup_course(CourseLocator(package_id=package_id, branch=branch))
-        items = set(course['structure']['blocks'].keys())
+        items = {LocMapperStore.decode_key_from_mongo(block_id) for block_id in course['structure']['blocks'].keys()}
         items.remove(course['structure']['root'])
         for block_id, block_data in course['structure']['blocks'].iteritems():
             items.difference_update(block_data.get('fields', {}).get('children', []))
             if block_data['category'] in detached_categories:
-                items.discard(block_id)
+                items.discard(LocMapperStore.decode_key_from_mongo(block_id))
         return [
             BlockUsageLocator(package_id=package_id, branch=branch, block_id=block_id)
             for block_id in items
@@ -554,21 +558,22 @@ class SplitMongoModuleStore(ModuleStoreWriteBase):
         update_version_field = 'blocks.{}.edit_info.update_version'.format(block_id)
         all_versions_with_block = self.db_connection.find_matching_structures({'original_version': course_struct['original_version'],
             update_version_field: {'$exists': True}})
-        # find (all) root versions and build map previous: [successors]
+        # find (all) root versions and build map {previous: {successors}..}
         possible_roots = []
         result = {}
         for version in all_versions_with_block:
-            if version['_id'] == version['blocks'][block_id]['edit_info']['update_version']:
-                if version['blocks'][block_id]['edit_info'].get('previous_version') is None:
-                    possible_roots.append(version['blocks'][block_id]['edit_info']['update_version'])
-                else:
-                    result.setdefault(version['blocks'][block_id]['edit_info']['previous_version'], set()).add(
-                        version['blocks'][block_id]['edit_info']['update_version'])
+            block_payload = self._get_block_from_structure(version, block_id)
+            if version['_id'] == block_payload['edit_info']['update_version']:
+                if block_payload['edit_info'].get('previous_version') is None:
+                    possible_roots.append(block_payload['edit_info']['update_version'])
+                else:  # map previous to {update..}
+                    result.setdefault(block_payload['edit_info']['previous_version'], set()).add(
+                        block_payload['edit_info']['update_version'])
 
         # more than one possible_root means usage was added and deleted > 1x.
         if len(possible_roots) > 1:
             # find the history segment including block_locator's version
-            element_to_find = course_struct['blocks'][block_id]['edit_info']['update_version']
+            element_to_find = self._get_block_from_structure(course_struct, block_id)['edit_info']['update_version']
             if element_to_find in possible_roots:
                 possible_roots = [element_to_find]
             for possibility in possible_roots:
@@ -660,6 +665,7 @@ class SplitMongoModuleStore(ModuleStoreWriteBase):
         # {category: last_serial...}
         # A potential confusion is if the name incorporates the parent's name, then if the child
         # moves, its id won't change and will be confusing
+        # NOTE2: this assumes category will never contain a $ nor a period.
         serial = 1
         while category + str(serial) in course_blocks:
             serial += 1
@@ -760,7 +766,7 @@ class SplitMongoModuleStore(ModuleStoreWriteBase):
 
         # generate usage id
         if block_id is not None:
-            if block_id in new_structure['blocks']:
+            if LocMapperStore.encode_key_for_mongo(block_id) in new_structure['blocks']:
                 raise DuplicateItemError(block_id, self, 'structures')
             else:
                 new_block_id = block_id
@@ -770,7 +776,7 @@ class SplitMongoModuleStore(ModuleStoreWriteBase):
         block_fields = partitioned_fields.get(Scope.settings, {})
         if Scope.children in partitioned_fields:
             block_fields.update(partitioned_fields[Scope.children])
-        new_structure['blocks'][new_block_id] = {
+        self._update_block_in_structure(new_structure, new_block_id, {
             "category": category,
             "definition": definition_locator.definition_id,
             "fields": block_fields,
@@ -780,12 +786,13 @@ class SplitMongoModuleStore(ModuleStoreWriteBase):
                 'previous_version': None,
                 'update_version': new_id,
             }
-        }
+        })
 
         # if given parent, add new block as child and update parent's version
         parent = None
         if isinstance(course_or_parent_locator, BlockUsageLocator) and course_or_parent_locator.block_id is not None:
-            parent = new_structure['blocks'][course_or_parent_locator.block_id]
+            encoded_block_id = LocMapperStore.encode_key_for_mongo(course_or_parent_locator.block_id)
+            parent = new_structure['blocks'][encoded_block_id]
             parent['fields'].setdefault('children', []).append(new_block_id)
             if not continue_version or parent['edit_info']['update_version'] != structure['_id']:
                 parent['edit_info']['edited_on'] = datetime.datetime.now(UTC)
@@ -892,7 +899,8 @@ class SplitMongoModuleStore(ModuleStoreWriteBase):
             if definition_fields or block_fields:
                 draft_structure = self._version_structure(draft_structure, user_id)
                 new_id = draft_structure['_id']
-                root_block = draft_structure['blocks'][draft_structure['root']]
+                encoded_block_id = LocMapperStore.encode_key_for_mongo(draft_structure['root'])
+                root_block = draft_structure['blocks'][encoded_block_id]
                 if block_fields is not None:
                     root_block['fields'].update(block_fields)
                 if definition_fields is not None:
@@ -948,7 +956,7 @@ class SplitMongoModuleStore(ModuleStoreWriteBase):
         descriptor.definition_locator, is_updated = self.update_definition_from_data(
             descriptor.definition_locator, descriptor.get_explicitly_set_fields_by_scope(Scope.content), user_id)
         # check children
-        original_entry = original_structure['blocks'][descriptor.location.block_id]
+        original_entry = self._get_block_from_structure(original_structure, descriptor.location.block_id)
         is_updated = is_updated or (
             descriptor.has_children and original_entry['fields']['children'] != descriptor.children
         )
@@ -962,7 +970,7 @@ class SplitMongoModuleStore(ModuleStoreWriteBase):
         # if updated, rev the structure
         if is_updated:
             new_structure = self._version_structure(original_structure, user_id)
-            block_data = new_structure['blocks'][descriptor.location.block_id]
+            block_data = self._get_block_from_structure(new_structure, descriptor.location.block_id)
 
             block_data["definition"] = descriptor.definition_locator.definition_id
             block_data["fields"] = descriptor.get_explicitly_set_fields_by_scope(Scope.settings)
@@ -1048,12 +1056,13 @@ class SplitMongoModuleStore(ModuleStoreWriteBase):
             is_new = True
             is_updated = True
             block_id = self._generate_block_id(structure_blocks, xblock.category)
+            encoded_block_id = block_id
             xblock.scope_ids.usage_id.block_id = block_id
         else:
             is_new = False
-            block_id = xblock.location.block_id
+            encoded_block_id = LocMapperStore.encode_key_for_mongo(xblock.location.block_id)
             is_updated = is_updated or (
-                xblock.has_children and structure_blocks[block_id]['fields']['children'] != xblock.children
+                xblock.has_children and structure_blocks[encoded_block_id]['fields']['children'] != xblock.children
             )
 
         children = []
@@ -1068,13 +1077,13 @@ class SplitMongoModuleStore(ModuleStoreWriteBase):
 
         block_fields = xblock.get_explicitly_set_fields_by_scope(Scope.settings)
         if not is_new and not is_updated:
-            is_updated = self._compare_settings(block_fields, structure_blocks[block_id]['fields'])
+            is_updated = self._compare_settings(block_fields, structure_blocks[encoded_block_id]['fields'])
         if children:
             block_fields['children'] = children
 
         if is_updated:
-            previous_version = None if is_new else structure_blocks[block_id]['edit_info'].get('update_version')
-            structure_blocks[block_id] = {
+            previous_version = None if is_new else structure_blocks[encoded_block_id]['edit_info'].get('update_version')
+            structure_blocks[encoded_block_id] = {
                 "category": xblock.category,
                 "definition": xblock.definition_locator.definition_id,
                 "fields": block_fields,
@@ -1226,7 +1235,8 @@ class SplitMongoModuleStore(ModuleStoreWriteBase):
         new_id = new_structure['_id']
         parents = self.get_parent_locations(usage_locator)
         for parent in parents:
-            parent_block = new_blocks[parent.block_id]
+            encoded_block_id = LocMapperStore.encode_key_for_mongo(parent.block_id)
+            parent_block = new_blocks[encoded_block_id]
             parent_block['fields']['children'].remove(usage_locator.block_id)
             parent_block['edit_info']['edited_on'] = datetime.datetime.now(UTC)
             parent_block['edit_info']['edited_by'] = user_id
@@ -1237,13 +1247,14 @@ class SplitMongoModuleStore(ModuleStoreWriteBase):
             """
             Remove the subtree rooted at block_id
             """
-            for child in new_blocks[block_id]['fields'].get('children', []):
+            encoded_block_id = LocMapperStore.encode_key_for_mongo(block_id)
+            for child in new_blocks[encoded_block_id]['fields'].get('children', []):
                 remove_subtree(child)
-            del new_blocks[block_id]
+            del new_blocks[encoded_block_id]
         if delete_children:
             remove_subtree(usage_locator.block_id)
         else:
-            del new_blocks[usage_locator.block_id]
+            del new_blocks[LocMapperStore.encode_key_for_mongo(usage_locator.block_id)]
 
         # update index if appropriate and structures
         self.db_connection.insert_structure(new_structure)
@@ -1306,6 +1317,7 @@ class SplitMongoModuleStore(ModuleStoreWriteBase):
 
         for child in block_fields.get('children', []):
             try:
+                child = LocMapperStore.encode_key_for_mongo(child)
                 self.inherit_settings(block_map, block_map[child], inheriting_settings)
             except KeyError:
                 # here's where we need logic for looking up in other structures when we allow cross pointers
@@ -1320,16 +1332,18 @@ class SplitMongoModuleStore(ModuleStoreWriteBase):
         (0 => this usage only, 1 => this usage and its children, etc...)
         A depth of None returns all descendants
         """
-        if block_id not in block_map:
+        encoded_block_id = LocMapperStore.encode_key_for_mongo(block_id)
+        if encoded_block_id not in block_map:
             return descendent_map
 
         if block_id not in descendent_map:
-            descendent_map[block_id] = block_map[block_id]
+            descendent_map[block_id] = block_map[encoded_block_id]
 
         if depth is None or depth > 0:
             depth = depth - 1 if depth is not None else None
-            for child in block_map[block_id]['fields'].get('children', []):
-                descendent_map = self.descendants(block_map, child, depth, descendent_map)
+            for child in descendent_map[block_id]['fields'].get('children', []):
+                descendent_map = self.descendants(block_map, child, depth,
+                    descendent_map)
 
         return descendent_map
 
@@ -1357,7 +1371,8 @@ class SplitMongoModuleStore(ModuleStoreWriteBase):
         for block in original_structure['blocks'].itervalues():
             if 'fields' in block and 'children' in block['fields']:
                 block['fields']["children"] = [
-                    block_id for block_id in block['fields']["children"] if block_id in original_structure['blocks']
+                    block_id for block_id in block['fields']["children"]
+                    if LocMapperStore.encode_key_for_mongo(block_id) in original_structure['blocks']
                 ]
         self.db_connection.update_structure(original_structure)
         # clear cache again b/c inheritance may be wrong over orphans
@@ -1511,8 +1526,11 @@ class SplitMongoModuleStore(ModuleStoreWriteBase):
         """
         new_id = ObjectId()
         if root_category is not None:
+            encoded_root = LocMapperStore.encode_key_for_mongo(root_block_id)
             blocks = {
-                root_block_id: self._new_block(user_id, root_category, block_fields, definition_id, new_id)
+                encoded_root: self._new_block(
+                    user_id, root_category, block_fields, definition_id, new_id
+                )
             }
         else:
             blocks = {}
@@ -1528,7 +1546,8 @@ class SplitMongoModuleStore(ModuleStoreWriteBase):
 
     def _get_parents_from_structure(self, block_id, structure):
         """
-        Given a structure, find all of block_id's parents in that structure
+        Given a structure, find all of block_id's parents in that structure. Note returns
+        the encoded format for parent
         """
         items = []
         for parent_id, value in structure['blocks'].iteritems():
@@ -1566,8 +1585,9 @@ class SplitMongoModuleStore(ModuleStoreWriteBase):
         Return any newly discovered orphans (as a set)
         """
         orphans = set()
-        destination_block = destination_blocks.get(block_id)
-        new_block = source_blocks[block_id]
+        encoded_block_id = LocMapperStore.encode_key_for_mongo(block_id)
+        destination_block = destination_blocks.get(encoded_block_id)
+        new_block = source_blocks[encoded_block_id]
         if destination_block:
             if destination_block['edit_info']['update_version'] != new_block['edit_info']['update_version']:
                 source_children = new_block['fields']['children']
@@ -1591,7 +1611,7 @@ class SplitMongoModuleStore(ModuleStoreWriteBase):
         for child in destination_block['fields'].get('children', []):
             if child not in blacklist:
                 orphans.update(self._publish_subdag(user_id, child, source_blocks, destination_blocks, blacklist))
-        destination_blocks[block_id] = destination_block
+        destination_blocks[encoded_block_id] = destination_block
         return orphans
 
     def _filter_blacklist(self, fields, blacklist):
@@ -1607,9 +1627,10 @@ class SplitMongoModuleStore(ModuleStoreWriteBase):
         Delete the orphan and any of its descendants which no longer have parents.
         """
         if not self._get_parents_from_structure(orphan, structure):
-            for child in structure['blocks'][orphan]['fields'].get('children', []):
+            encoded_block_id = LocMapperStore.encode_key_for_mongo(orphan)
+            for child in structure['blocks'][encoded_block_id]['fields'].get('children', []):
                 self._delete_if_true_orphan(child, structure)
-            del structure['blocks'][orphan]
+            del structure['blocks'][encoded_block_id]
 
     def _new_block(self, user_id, category, block_fields, definition_id, new_id):
         return {
@@ -1623,3 +1644,17 @@ class SplitMongoModuleStore(ModuleStoreWriteBase):
                 'update_version': new_id
             }
         }
+
+    def _get_block_from_structure(self, structure, block_id):
+        """
+        Encodes the block id before retrieving it from the structure to ensure it can
+        be a json dict key.
+        """
+        return structure['blocks'].get(LocMapperStore.encode_key_for_mongo(block_id))
+
+    def _update_block_in_structure(self, structure, block_id, content):
+        """
+        Encodes the block id before accessing it in the structure to ensure it can
+        be a json dict key.
+        """
+        structure['blocks'][LocMapperStore.encode_key_for_mongo(block_id)] = content

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

@@ -620,6 +620,34 @@ class TestItemCrud(SplitModuleTest):
         another_history = modulestore().get_definition_history_info(another_module.definition_locator)
         self.assertEqual(str(another_history['previous_version']), '0d00000040000000dddd0031')
 
+    def test_encoded_naming(self):
+        """
+        Check that using odd characters in block id don't break ability to add and retrieve block.
+        """
+        parent_locator = BlockUsageLocator(package_id="contender", block_id="head345679", branch='draft')
+        chapter_locator = BlockUsageLocator(package_id="contender", block_id="foo.bar_-~:0", branch='draft')
+        modulestore().create_item(
+            parent_locator, 'chapter', 'anotheruser',
+            block_id=chapter_locator.block_id,
+            fields={'display_name': 'chapter 99'},
+        )
+        # check that course version changed and course's previous is the other one
+        new_module = modulestore().get_item(chapter_locator)
+        self.assertEqual(new_module.location.block_id, "foo.bar_-~:0")  # hardcode to ensure BUL init didn't change
+        # now try making that a parent of something
+        new_payload = "<problem>empty</problem>"
+        problem_locator = BlockUsageLocator(package_id="contender", block_id="prob.bar_-~:99a", branch='draft')
+        modulestore().create_item(
+            chapter_locator, 'problem', 'anotheruser',
+            block_id=problem_locator.block_id,
+            fields={'display_name': 'chapter 99', 'data': new_payload},
+        )
+        # check that course version changed and course's previous is the other one
+        new_module = modulestore().get_item(problem_locator)
+        self.assertEqual(new_module.location.block_id, problem_locator.block_id)
+        chapter = modulestore().get_item(chapter_locator)
+        self.assertIn(problem_locator.block_id, chapter.children)
+
     def test_create_continue_version(self):
         """
         Test create_item using the continue_version flag