From 13a04927f801e23038b19923af62caa414397df2 Mon Sep 17 00:00:00 2001 From: Don Mitchell Date: Mon, 13 Jan 2014 12:02:50 -0500 Subject: [PATCH] Encode periods and $ in block_id map STUD-872 --- .../xmodule/modulestore/loc_mapper_store.py | 14 ++- .../split_mongo/caching_descriptor_system.py | 5 +- .../xmodule/modulestore/split_mongo/split.py | 117 ++++++++++++------ .../tests/test_split_modulestore.py | 28 +++++ 4 files changed, 116 insertions(+), 48 deletions(-) diff --git a/common/lib/xmodule/xmodule/modulestore/loc_mapper_store.py b/common/lib/xmodule/xmodule/modulestore/loc_mapper_store.py index 3de215dbc7..5a37c260f7 100644 --- a/common/lib/xmodule/xmodule/modulestore/loc_mapper_store.py +++ b/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) 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 f11ed49151..83d585b964 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 @@ -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 = {} diff --git a/common/lib/xmodule/xmodule/modulestore/split_mongo/split.py b/common/lib/xmodule/xmodule/modulestore/split_mongo/split.py index 04f15727a4..4d86cf14fa 100644 --- a/common/lib/xmodule/xmodule/modulestore/split_mongo/split.py +++ b/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 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 32e9d5bbdb..aa3db34f72 100644 --- a/common/lib/xmodule/xmodule/modulestore/tests/test_split_modulestore.py +++ b/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 = "empty" + 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