Skip to content

E
edx-platform
Commit ded28af8 by Don Mitchell
Options

Make Locators comply with UsageKey accessors

parent 6d7fe561
Showing with 66 additions and 118 deletions
cms/djangoapps/contentstore/tests/test_crud.py
......@@ -12,7 +12,6 @@ from xmodule.html_module import HtmlDescriptor
from xmodule.modulestore.tests.django_utils import ModuleStoreTestCase
@unittest.skip("Not fixing split until we land opaque-keys 0.9")
class TemplateTests(unittest.TestCase):
"""
Test finding and using the templates (boilerplates) for xblocks.
......@@ -55,25 +54,25 @@ class TemplateTests(unittest.TestCase):
def test_factories(self):
test_course = persistent_factories.PersistentCourseFactory.create(
course_id='testx.tempcourse', org='testx',
offering='tempcourse', org='testx',
display_name='fun test course', user_id='testbot'
)
self.assertIsInstance(test_course, CourseDescriptor)
self.assertEqual(test_course.display_name, 'fun test course')
index_info = modulestore('split').get_course_index_info(test_course.location)
index_info = modulestore('split').get_course_index_info(test_course.id)
self.assertEqual(index_info['org'], 'testx')
self.assertEqual(index_info['_id'], 'testx.tempcourse')
self.assertEqual(index_info['offering'], 'tempcourse')
test_chapter = persistent_factories.ItemFactory.create(display_name='chapter 1',
parent_location=test_course.location)
self.assertIsInstance(test_chapter, SequenceDescriptor)
# refetch parent which should now point to child
test_course = modulestore('split').get_course(test_course.id)
test_course = modulestore('split').get_course(test_course.id.version_agnostic())
self.assertIn(test_chapter.location.block_id, test_course.children)
with self.assertRaises(DuplicateCourseError):
persistent_factories.PersistentCourseFactory.create(
course_id='testx.tempcourse', org='testx',
offering='tempcourse', org='testx',
display_name='fun test course', user_id='testbot'
)
......@@ -82,7 +81,7 @@ class TemplateTests(unittest.TestCase):
Test create_xblock to create non persisted xblocks
"""
test_course = persistent_factories.PersistentCourseFactory.create(
course_id='testx.tempcourse', org='testx',
offering='tempcourse', org='testx',
display_name='fun test course', user_id='testbot'
)
......@@ -109,7 +108,7 @@ class TemplateTests(unittest.TestCase):
try saving temporary xblocks
"""
test_course = persistent_factories.PersistentCourseFactory.create(
course_id='testx.tempcourse', org='testx',
offering='tempcourse', org='testx',
display_name='fun test course', user_id='testbot'
)
test_chapter = modulestore('split').create_xblock(
......@@ -148,7 +147,7 @@ class TemplateTests(unittest.TestCase):
def test_delete_course(self):
test_course = persistent_factories.PersistentCourseFactory.create(
course_id='edu.harvard.history.doomed', org='testx',
offering='history.doomed', org='edu.harvard',
display_name='doomed test course',
user_id='testbot')
persistent_factories.ItemFactory.create(display_name='chapter 1',
......@@ -171,7 +170,7 @@ class TemplateTests(unittest.TestCase):
Test get_block_generations
"""
test_course = persistent_factories.PersistentCourseFactory.create(
course_id='edu.harvard.history.hist101', org='testx',
offering='history.hist101', org='edu.harvard',
display_name='history test course',
user_id='testbot'
)
......@@ -193,7 +192,9 @@ class TemplateTests(unittest.TestCase):
second_problem = persistent_factories.ItemFactory.create(
display_name='problem 2',
parent_location=BlockUsageLocator.make_relative(updated_loc, block_id=sub.location.block_id),
parent_location=BlockUsageLocator.make_relative(
updated_loc, block_type='problem', block_id=sub.location.block_id
),
user_id='testbot', category='problem',
data="<problem></problem>"
)
......
cms/djangoapps/contentstore/views/tests/test_course_index.py
......@@ -7,7 +7,7 @@ import lxml
from contentstore.tests.utils import CourseTestCase
from contentstore.utils import reverse_course_url
from xmodule.modulestore.tests.factories import CourseFactory, ItemFactory
from xmodule.modulestore import parsers
from xmodule.modulestore.locator import Locator
class TestCourseIndex(CourseTestCase):
......@@ -38,7 +38,7 @@ class TestCourseIndex(CourseTestCase):
for link in course_link_eles:
self.assertRegexpMatches(
link.get("href"),
'course/slashes:{0}'.format(parsers.ALLOWED_ID_CHARS)
'course/slashes:{0}'.format(Locator.ALLOWED_ID_CHARS)
)
# now test that url
outline_response = authed_client.get(link.get("href"), {}, HTTP_ACCEPT='text/html')
......
common/lib/xmodule/setup.py
......@@ -79,5 +79,8 @@ setup(
'asset-location = xmodule.modulestore.locations:AssetLocation',
'edx = xmodule.modulestore.locator:BlockUsageLocator',
],
'definition_key': [
'defx = xmodule.modulestore.locator:DefinitionLocator',
],
},
)
common/lib/xmodule/xmodule/modulestore/keys.py
......@@ -58,7 +58,7 @@ class DefinitionKey(OpaqueKey):
KEY_TYPE = 'definition_key'
__slots__ = ()
@abstractmethod
@abstractproperty
def block_type(self):
"""
The XBlock type of this definition.
......@@ -125,6 +125,10 @@ class UsageKey(CourseObjectMixin, OpaqueKey):
"""
raise NotImplementedError()
@property
def block_type(self):
return self.category
class OpaqueKeyReader(IdReader):
"""
......
common/lib/xmodule/xmodule/modulestore/loc_mapper_store.py
......@@ -150,6 +150,7 @@ class LocMapperStore(object):
entry = self._migrate_if_necessary([entry])[0]
block_id = entry['block_map'].get(self.encode_key_for_mongo(location.name))
category = location.category
if block_id is None:
if add_entry_if_missing:
block_id = self._add_to_block_map(
......@@ -159,14 +160,15 @@ class LocMapperStore(object):
raise ItemNotFoundError(location)
else:
# jump_to_id uses a None category.
if location.category is None:
if category is None:
if len(block_id) == 1:
# unique match (most common case)
category = block_id.keys()[0]
block_id = block_id.values()[0]
else:
raise InvalidLocationError()
elif location.category in block_id:
block_id = block_id[location.category]
elif category in block_id:
block_id = block_id[category]
elif add_entry_if_missing:
block_id = self._add_to_block_map(location, course_son, entry['block_map'])
else:
......@@ -179,10 +181,12 @@ class LocMapperStore(object):
)
published_usage = BlockUsageLocator(
prod_course_locator,
block_type=category,
block_id=block_id
)
draft_usage = BlockUsageLocator(
prod_course_locator.for_branch(entry['draft_branch']),
block_type=category,
block_id=block_id
)
if published:
......@@ -285,6 +289,7 @@ class LocMapperStore(object):
org=entry[entry_org], offering=entry[entry_offering],
branch=entry['prod_branch']
),
block_type=category,
block_id=block_id
)
draft_locator = BlockUsageLocator(
......@@ -292,6 +297,7 @@ class LocMapperStore(object):
org=entry[entry_org], offering=entry[entry_offering],
branch=entry['draft_branch']
),
block_type=category,
block_id=block_id
)
self._cache_location_map_entry(location, published_locator, draft_locator)
......
common/lib/xmodule/xmodule/modulestore/parsers.py deleted 100644 → 0
import re
# Prefix for the branch portion of a locator URL
BRANCH_PREFIX = r"branch/"
# Prefix for the block portion of a locator URL
BLOCK_PREFIX = r"block/"
# Prefix for the version portion of a locator URL, when it is preceded by a course ID
VERSION_PREFIX = r"version/"
ALLOWED_ID_CHARS = r'[\w\-~.:+]'
ALLOWED_ID_RE = re.compile(r'^{}+$'.format(ALLOWED_ID_CHARS), re.UNICODE)
# NOTE: if we need to support period in place of +, make it aggressive (take the first period in the string)
URL_RE_SOURCE = r"""
((?P<org>{ALLOWED_ID_CHARS}+)\+(?P<offering>{ALLOWED_ID_CHARS}+)/?)?
({BRANCH_PREFIX}(?P<branch>{ALLOWED_ID_CHARS}+)/?)?
({VERSION_PREFIX}(?P<version_guid>[A-F0-9]+)/?)?
({BLOCK_PREFIX}(?P<block_id>{ALLOWED_ID_CHARS}+))?
""".format(
ALLOWED_ID_CHARS=ALLOWED_ID_CHARS, BRANCH_PREFIX=BRANCH_PREFIX,
VERSION_PREFIX=VERSION_PREFIX, BLOCK_PREFIX=BLOCK_PREFIX
)
URL_RE = re.compile('^' + URL_RE_SOURCE + '$', re.IGNORECASE | re.VERBOSE | re.UNICODE)
def parse_url(string):
"""
followed by either a version_guid or a org + offering pair. If tag_optional, then
the url does not have to start with the tag and edx will be assumed.
Examples:
'edx:version/0123FFFF'
'edx:mit.eecs.6002x'
'edx:mit.eecs.6002x/branch/published'
'edx:mit.eecs.6002x/branch/published/block/HW3'
'edx:mit.eecs.6002x/branch/published/version/000eee12345/block/HW3'
This returns None if string cannot be parsed.
If it can be parsed as a version_guid with no preceding org + offering, returns a dict
with key 'version_guid' and the value,
If it can be parsed as a org + offering, returns a dict
with key 'id' and optional keys 'branch' and 'version_guid'.
"""
match = URL_RE.match(string)
if not match:
return None
matched_dict = match.groupdict()
return matched_dict
def parse_block_ref(string):
r"""
A block_ref is a string of url safe characters (see ALLOWED_ID_CHARS)
If string is a block_ref, returns a dict with key 'block_ref' and the value,
otherwise returns None.
"""
if ALLOWED_ID_RE.match(string):
return {'block_id': string}
return None
common/lib/xmodule/xmodule/modulestore/split_mongo/caching_descriptor_system.py
......@@ -108,6 +108,7 @@ class CachingDescriptorSystem(MakoDescriptorSystem):
offering=course_entry_override.get('offering'),
branch=course_entry_override.get('branch'),
),
block_type=json_data.get('category'),
block_id=block_id,
)
......@@ -131,6 +132,7 @@ class CachingDescriptorSystem(MakoDescriptorSystem):
self,
BlockUsageLocator(
CourseLocator(version_guid=course_entry_override['structure']['_id']),
block_type='error',
block_id=block_id
),
error_msg=exc_info_to_str(sys.exc_info())
......
common/lib/xmodule/xmodule/modulestore/split_mongo/definition_lazy_loader.py
......@@ -8,14 +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, definition_id):
def __init__(self, modulestore, block_type, definition_id):
"""
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.definition_locator = DefinitionLocator(definition_id)
self.definition_locator = DefinitionLocator(block_type, definition_id)
def fetch(self):
"""
......
common/lib/xmodule/xmodule/modulestore/split_mongo/split.py
......@@ -152,7 +152,7 @@ class SplitMongoModuleStore(ModuleStoreWriteBase):
if lazy:
for block in new_module_data.itervalues():
block['definition'] = DefinitionLazyLoader(self, block['definition'])
block['definition'] = DefinitionLazyLoader(self, block['category'], block['definition'])
else:
# Load all descendants by id
descendent_definitions = self.db_connection.find_matching_definitions({
......@@ -242,11 +242,6 @@ class SplitMongoModuleStore(ModuleStoreWriteBase):
:param course_locator: any subclass of CourseLocator
'''
# NOTE: if and when this uses cache, the update if changed logic will break if the cache
# holds the same objects as the descriptors!
if not course_locator.is_fully_specified():
raise InsufficientSpecificationError('Not fully specified: %s' % course_locator)
if course_locator.org and course_locator.offering and course_locator.branch:
# use the course id
index = self.db_connection.get_course_index(course_locator)
......@@ -258,6 +253,8 @@ class SplitMongoModuleStore(ModuleStoreWriteBase):
if course_locator.version_guid is not None and version_guid != course_locator.version_guid:
# This may be a bit too touchy but it's hard to infer intent
raise VersionConflictError(course_locator, version_guid)
elif course_locator.version_guid is None:
raise InsufficientSpecificationError(course_locator)
else:
# TODO should this raise an exception if branch was provided?
version_guid = course_locator.version_guid
......@@ -322,9 +319,6 @@ class SplitMongoModuleStore(ModuleStoreWriteBase):
def get_course(self, course_id, depth=None):
'''
Gets the course descriptor for the course identified by the locator
which may or may not be a blockLocator.
raises InsufficientSpecificationError
'''
assert(isinstance(course_id, CourseLocator))
course_entry = self._lookup_course(course_id)
......@@ -458,6 +452,7 @@ class SplitMongoModuleStore(ModuleStoreWriteBase):
return [
BlockUsageLocator.make_relative(
locator,
block_type=course['structure']['blocks'][parent_id].get('category'),
block_id=LocMapperStore.decode_key_from_mongo(parent_id),
)
for parent_id in items
......@@ -471,12 +466,13 @@ class SplitMongoModuleStore(ModuleStoreWriteBase):
course = self._lookup_course(course_key)
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():
blocks = course['structure']['blocks']
for block_id, block_data in blocks.iteritems():
items.difference_update(block_data.get('fields', {}).get('children', []))
if block_data['category'] in detached_categories:
items.discard(LocMapperStore.decode_key_from_mongo(block_id))
return [
BlockUsageLocator(course_key=course_key, block_id=block_id)
BlockUsageLocator(course_key=course_key, block_type=blocks[block_id]['category'], block_id=block_id)
for block_id in items
]
......@@ -613,11 +609,11 @@ class SplitMongoModuleStore(ModuleStoreWriteBase):
# convert the results value sets to locators
for k, versions in result.iteritems():
result[k] = [
BlockUsageLocator(CourseLocator(version_guid=version), block_id=block_id)
block_locator.for_version(version)
for version in versions
]
return VersionTree(
BlockUsageLocator(CourseLocator(version_guid=possible_roots[0]), block_id=block_id),
block_locator.for_version(possible_roots[0]),
result
)
......@@ -650,7 +646,7 @@ class SplitMongoModuleStore(ModuleStoreWriteBase):
'schema_version': self.SCHEMA_VERSION,
}
self.db_connection.insert_definition(document)
definition_locator = DefinitionLocator(new_id)
definition_locator = DefinitionLocator(category, new_id)
return definition_locator
def update_definition_from_data(self, definition_locator, new_def_data, user_id):
......@@ -685,7 +681,7 @@ class SplitMongoModuleStore(ModuleStoreWriteBase):
old_definition['edit_info']['previous_version'] = definition_locator.definition_id
old_definition['schema_version'] = self.SCHEMA_VERSION
self.db_connection.insert_definition(old_definition)
return DefinitionLocator(old_definition['_id']), True
return DefinitionLocator(old_definition['category'], old_definition['_id']), True
else:
return definition_locator, False
......@@ -829,11 +825,13 @@ class SplitMongoModuleStore(ModuleStoreWriteBase):
self._update_head(index_entry, course_or_parent_locator.branch, new_id)
item_loc = BlockUsageLocator(
course_or_parent_locator.version_agnostic(),
block_type=category,
block_id=new_block_id,
)
else:
item_loc = BlockUsageLocator(
CourseLocator(version_guid=new_id),
block_type=category,
block_id=new_block_id,
)
......@@ -1029,7 +1027,7 @@ class SplitMongoModuleStore(ModuleStoreWriteBase):
course_key = CourseLocator(version_guid=new_id)
# fetch and return the new item--fetching is unnecessary but a good qc step
new_locator = BlockUsageLocator(course_key, descriptor.location.block_id)
new_locator = descriptor.location.map_into_course(course_key)
return self.get_item(new_locator)
else:
# nothing changed, just return the one sent in
......@@ -1101,18 +1099,14 @@ class SplitMongoModuleStore(ModuleStoreWriteBase):
self._update_head(index_entry, xblock.location.branch, new_id)
# fetch and return the new item--fetching is unnecessary but a good qc step
return self.get_item(
BlockUsageLocator(
xblock.location.course_key.for_version(new_id),
block_id=xblock.location.block_id,
)
)
return self.get_item(xblock.location.for_version(new_id))
else:
return xblock
def _persist_subdag(self, xblock, user_id, structure_blocks, new_id):
# persist the definition if persisted != passed
new_def_data = self._filter_special_fields(xblock.get_explicitly_set_fields_by_scope(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)
......@@ -1134,9 +1128,6 @@ class SplitMongoModuleStore(ModuleStoreWriteBase):
else:
is_new = False
encoded_block_id = LocMapperStore.encode_key_for_mongo(xblock.location.block_id)
is_updated = is_updated or (
xblock.has_children and structure_blocks[encoded_block_id]['fields']['children'] != xblock.children
)
children = []
if xblock.has_children:
......@@ -1147,6 +1138,7 @@ class SplitMongoModuleStore(ModuleStoreWriteBase):
children.append(child_block.location.block_id)
else:
children.append(child)
is_updated = is_updated or structure_blocks[encoded_block_id]['fields']['children'] != children
block_fields = xblock.get_explicitly_set_fields_by_scope(Scope.settings)
if not is_new and not is_updated:
......@@ -1419,9 +1411,9 @@ class SplitMongoModuleStore(ModuleStoreWriteBase):
if isinstance(definition, DefinitionLazyLoader):
return definition.definition_locator
elif '_id' not in definition:
return DefinitionLocator(LocalId())
return DefinitionLocator(definition.get('category'), LocalId())
else:
return DefinitionLocator(definition['_id'])
return DefinitionLocator(definition['category'], definition['_id'])
def get_modulestore_type(self, course_id):
"""
......
common/lib/xmodule/xmodule/modulestore/tests/persistent_factories.py
......@@ -32,14 +32,14 @@ class PersistentCourseFactory(SplitFactory):
# pylint: disable=W0613
@classmethod
def _create(cls, target_class, course_id='testX.999', org='testX', user_id='test_user',
def _create(cls, target_class, offering='999', org='testX', user_id='test_user',
master_branch='draft', **kwargs):
modulestore = kwargs.pop('modulestore')
root_block_id = kwargs.pop('root_block_id', 'course')
# Write the data to the mongo datastore
new_course = modulestore.create_course(
course_id, org, user_id, fields=kwargs,
org, offering, user_id, fields=kwargs,
master_branch=master_branch, root_block_id=root_block_id
)
......
common/lib/xmodule/xmodule/modulestore/tests/test_location_mapper.py
......@@ -267,6 +267,7 @@ class TestLocationMapper(LocMapperSetupSansDjango):
)
prob_locator = BlockUsageLocator(
prob_course_key,
block_type='problem',
block_id='problem2',
)
prob_location = loc_mapper().translate_locator_to_location(prob_locator)
......@@ -289,20 +290,21 @@ class TestLocationMapper(LocMapperSetupSansDjango):
prob_location = loc_mapper().translate_locator_to_location(prob_locator, get_course=True)
self.assertEqual(prob_location, SlashSeparatedCourseKey(org, course, run))
# explicit branch
prob_locator = BlockUsageLocator(
prob_course_key.for_branch('draft'), block_id=prob_locator.block_id
)
prob_locator = prob_locator.for_branch('draft')
prob_location = loc_mapper().translate_locator_to_location(prob_locator)
# Even though the problem was set as draft, we always return revision=None to work
# with old mongo/draft modulestores.
self.assertEqual(prob_location, Location(org, course, run, 'problem', 'abc123', None))
prob_locator = BlockUsageLocator(prob_course_key.for_branch('production'), block_id='problem2')
prob_locator = BlockUsageLocator(
prob_course_key.for_branch('production'),
block_type='problem', block_id='problem2'
)
prob_location = loc_mapper().translate_locator_to_location(prob_locator)
self.assertEqual(prob_location, Location(org, course, run, 'problem', 'abc123', None))
# same for chapter except chapter cannot be draft in old system
chap_locator = BlockUsageLocator(
prob_course_key.for_branch('production'),
block_id='chapter48f',
block_type='chapter', block_id='chapter48f',
)
chap_location = loc_mapper().translate_locator_to_location(chap_locator)
self.assertEqual(chap_location, Location(org, course, run, 'chapter', '48f23a10395384929234'))
......@@ -311,7 +313,7 @@ class TestLocationMapper(LocMapperSetupSansDjango):
chap_location = loc_mapper().translate_locator_to_location(chap_locator)
self.assertEqual(chap_location, Location(org, course, run, 'chapter', '48f23a10395384929234'))
chap_locator = BlockUsageLocator(
prob_course_key.for_branch('production'), block_id='chapter48f'
prob_course_key.for_branch('production'), block_type='chapter', block_id='chapter48f'
)
chap_location = loc_mapper().translate_locator_to_location(chap_locator)
self.assertEqual(chap_location, Location(org, course, run, 'chapter', '48f23a10395384929234'))
......@@ -319,7 +321,7 @@ class TestLocationMapper(LocMapperSetupSansDjango):
# look for non-existent problem
prob_locator2 = BlockUsageLocator(
prob_course_key.for_branch('draft'),
block_id='problem3'
block_type='problem', block_id='problem3'
)
prob_location = loc_mapper().translate_locator_to_location(prob_locator2)
self.assertIsNone(prob_location, 'Found non-existent problem')
......
common/lib/xmodule/xmodule/modulestore/tests/test_split_w_old_mongo.py
......@@ -105,6 +105,7 @@ class SplitWMongoCourseBoostrapper(unittest.TestCase):
# create pointer for split
course_or_parent_locator = BlockUsageLocator(
course_key=self.split_course_key,
block_type=parent_category,
block_id=parent_name
)
else:
......
Markdown is supported
0% or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment