Parse urls in one regex w/ more consistency

common/lib/xmodule/xmodule/modulestore/locator.py

@@ -13,7 +13,7 @@ from bson.errors import InvalidId
 from xmodule.modulestore.exceptions import InsufficientSpecificationError, OverSpecificationError
 
 from .parsers import parse_url, parse_course_id, parse_block_ref
-from .parsers import BRANCH_PREFIX, BLOCK_PREFIX, URL_VERSION_PREFIX
+from .parsers import BRANCH_PREFIX, BLOCK_PREFIX, VERSION_PREFIX
 import re
 from xmodule.modulestore import Location
 
@@ -190,8 +190,8 @@ class CourseLocator(Locator):
             self.init_from_version_guid(version_guid)
         if course_id or branch:
             self.init_from_course_id(course_id, branch)
-        assert self.version_guid or self.course_id, \
-            "Either version_guid or course_id should be set."
+        if self.version_guid is None and self.course_id is None:
+            raise ValueError("Either version_guid or course_id should be set: {}".format(url))
 
     def __unicode__(self):
         """
@@ -200,10 +200,10 @@ class CourseLocator(Locator):
         if self.course_id:
             result = self.course_id
             if self.branch:
-                result += BRANCH_PREFIX + self.branch
+                result += '/' + BRANCH_PREFIX + self.branch
             return result
         elif self.version_guid:
-            return URL_VERSION_PREFIX + str(self.version_guid)
+            return VERSION_PREFIX + str(self.version_guid)
         else:
             # raise InsufficientSpecificationError("missing course_id or version_guid")
             return '<InsufficientSpecificationError: missing course_id or version_guid>'
@@ -263,22 +263,46 @@ class CourseLocator(Locator):
                              version_guid=self.version_guid,
                              branch=self.branch)
 
+    def url_reverse(self, prefix, postfix):
+        """
+        Do what reverse is supposed to do but seems unable to do. Generate a url using prefix unicode(self) postfix
+        :param prefix: the beginning of the url (should begin and end with / if non-empty)
+        :param postfix: the part to append to the url (should begin w/ / if non-empty)
+        """
+        if prefix:
+            if not prefix.endswith('/'):
+                prefix += '/'
+            if not prefix.startswith('/'):
+                prefix = '/' + prefix
+        else:
+            prefix = '/'
+        if postfix and not postfix.startswith('/'):
+            postfix = '/' + postfix
+        return prefix + unicode(self) + postfix
+
+    def reverse_kwargs(self):
+        """
+        Get the kwargs list to supply to reverse (basically, a dict of the set properties)
+        """
+        return {key: value for key, value in self.__dict__.iteritems() if value is not None}
+
     def init_from_url(self, url):
         """
         url must be a string beginning with 'edx://' and containing
         either a valid version_guid or course_id (with optional branch), or both.
         """
         if isinstance(url, Locator):
-            url = url.url()
-        if not isinstance(url, basestring):
+            parse = url.__dict__
+        elif not isinstance(url, basestring):
             raise TypeError('%s is not an instance of basestring' % url)
-        parse = parse_url(url, tag_optional=True)
-        if not parse:
-            raise ValueError('Could not parse "%s" as a url' % url)
+        else:
+            parse = parse_url(url, tag_optional=True)
+            if not parse:
+                raise ValueError('Could not parse "%s" as a url' % url)
         self._set_value(
             parse, 'version_guid', lambda (new_guid): self.set_version_guid(self.as_object_id(new_guid))
         )
-        self._set_value(parse, 'id', lambda (new_id): self.set_course_id(new_id))
+        self._set_value(parse, 'course_id', lambda (new_id): self.set_course_id(new_id))
         self._set_value(parse, 'branch', lambda (new_branch): self.set_branch(new_branch))
 
     def init_from_version_guid(self, version_guid):
@@ -313,9 +337,9 @@ class CourseLocator(Locator):
                     raise ValueError("%s does not have a valid course_id" % course_id)
 
             parse = parse_course_id(course_id)
-            if not parse:
+            if not parse or parse['course_id'] is None:
                 raise ValueError('Could not parse "%s" as a course_id' % course_id)
-            self.set_course_id(parse['id'])
+            self.set_course_id(parse['course_id'])
             rev = parse['branch']
             if rev:
                 self.set_branch(rev)
@@ -396,11 +420,12 @@ class BlockUsageLocator(CourseLocator):
             self.init_block_ref_from_course_id(course_id)
         if usage_id:
             self.init_block_ref(usage_id)
-        CourseLocator.__init__(self,
-                               url=url,
-                               version_guid=version_guid,
-                               course_id=course_id,
-                               branch=branch)
+        super(BlockUsageLocator, self).__init__(
+            url=url,
+            version_guid=version_guid,
+            course_id=course_id,
+            branch=branch
+        )
 
     def is_initialized(self):
         """
@@ -427,6 +452,16 @@ class BlockUsageLocator(CourseLocator):
                                      branch=self.branch,
                                      usage_id=self.usage_id)
 
+    def reverse_kwargs(self):
+        """
+        Get the kwargs list to supply to reverse (basically, a dict of the set properties)
+        """
+        result = super(BlockUsageLocator, self).reverse_kwargs()
+        if self.usage_id:
+            del result['usage_id']
+            result['block'] = self.usage_id
+        return result
+
     def set_usage_id(self, new):
         """
         Initialize usage_id to new value.
@@ -459,10 +494,12 @@ class BlockUsageLocator(CourseLocator):
 
     def init_block_ref_from_course_id(self, course_id):
         if isinstance(course_id, CourseLocator):
+            # FIXME the parsed course_id should never contain a block ref
             course_id = course_id.course_id
             assert course_id, "%s does not have a valid course_id"
         parse = parse_course_id(course_id)
-        assert parse, 'Could not parse "%s" as a course_id' % course_id
+        if parse is None:
+            raise ValueError('Could not parse "%s" as a course_id' % course_id)
         self._set_value(parse, 'block', lambda(new_block): self.set_usage_id(new_block))
 
     def __unicode__(self):
@@ -470,7 +507,7 @@ class BlockUsageLocator(CourseLocator):
         Return a string representing this location.
         """
         rep = super(BlockUsageLocator, self).__unicode__()
-        return rep + BLOCK_PREFIX + unicode(self.usage_id)
+        return rep + '/' + BLOCK_PREFIX + unicode(self.usage_id)
 
 
 class DefinitionLocator(Locator):
@@ -478,7 +515,7 @@ class DefinitionLocator(Locator):
     Container for how to locate a description (the course-independent content).
     """
 
-    URL_RE = re.compile(r'^defx://' + URL_VERSION_PREFIX + '([^/]+)$', re.IGNORECASE)
+    URL_RE = re.compile(r'^defx://' + VERSION_PREFIX + '([^/]+)$', re.IGNORECASE)
     def __init__(self, definition_id):
         if isinstance(definition_id, basestring):
             regex_match = self.URL_RE.match(definition_id)
@@ -491,7 +528,7 @@ class DefinitionLocator(Locator):
         Return a string representing this location.
         unicode(self) returns something like this: "version/519665f6223ebd6980884f2b"
         '''
-        return URL_VERSION_PREFIX + str(self.definition_id)
+        return VERSION_PREFIX + str(self.definition_id)
 
     def url(self):
         """

common/lib/xmodule/xmodule/modulestore/parsers.py

 import re
 
 # Prefix for the branch portion of a locator URL
-BRANCH_PREFIX = "/branch/"
+BRANCH_PREFIX = r"branch/"
 # Prefix for the block portion of a locator URL
-BLOCK_PREFIX = "/block/"
+BLOCK_PREFIX = r"block/"
 # Prefix for the version portion of a locator URL, when it is preceded by a course ID
-VERSION_PREFIX = "/version/"
-# Prefix for version when it begins the URL (no course ID).
-URL_VERSION_PREFIX = 'version/'
+VERSION_PREFIX = r"version/"
 
-URL_RE = re.compile(r'^(edx://)?(.+)$', re.IGNORECASE)
 ALLOWED_ID_CHARS = r'[a-zA-Z0-9_\-~.]'
 
+URL_RE_SOURCE = r"""
+    (?P<tag>edx://)?
+    ((?P<course_id>{ALLOWED_ID_CHARS}+)/?)?
+    ({BRANCH_PREFIX}(?P<branch>{ALLOWED_ID_CHARS}+)/?)?
+    ({VERSION_PREFIX}(?P<version_guid>[A-F0-9]+)/?)?
+    ({BLOCK_PREFIX}(?P<block>{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)
+
+
 def parse_url(string, tag_optional=False):
     """
     A url usually begins with 'edx://' (case-insensitive match),
@@ -21,9 +32,9 @@ def parse_url(string, tag_optional=False):
     Examples:
         'edx://version/0123FFFF'
         'edx://mit.eecs.6002x'
-        'edx://mit.eecs.6002x;published'
-        'edx://mit.eecs.6002x;published/block/HW3'
-        'edx://mit.eecs.6002x;published/version/000eee12345/block/HW3'
+        '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.
 
@@ -37,12 +48,10 @@ def parse_url(string, tag_optional=False):
     match = URL_RE.match(string)
     if not match:
         return None
-    if match.group(1) is None and not tag_optional:
+    matched_dict = match.groupdict()
+    if matched_dict['tag'] is None and not tag_optional:
         return None
-    path = match.group(2)
-    if path.startswith(URL_VERSION_PREFIX):
-        return parse_guid(path[len(URL_VERSION_PREFIX):])
-    return parse_course_id(path)
+    return matched_dict
 
 
 BLOCK_RE = re.compile(r'^' + ALLOWED_ID_CHARS + r'+$', re.IGNORECASE)
@@ -60,34 +69,6 @@ def parse_block_ref(string):
     return None
 
 
-GUID_RE = re.compile(
-    r'^(?P<version_guid>[A-F0-9]+)(' + BLOCK_PREFIX + '(?P<block>' + ALLOWED_ID_CHARS + r'+))?$',
-    re.IGNORECASE
-)
-
-
-def parse_guid(string):
-    """
-    A version_guid is a string of hex digits (0-F).
-
-    If string is a version_guid, returns a dict with key 'version_guid' and the value,
-    otherwise returns None.
-    """
-    m = GUID_RE.match(string)
-    if m is not None:
-        return m.groupdict()
-    else:
-        return None
-
-
-COURSE_ID_RE = re.compile(
-    r'^(?P<id>' + ALLOWED_ID_CHARS + r'+)(' +
-    BRANCH_PREFIX + r'(?P<branch>' + ALLOWED_ID_CHARS + r'+))?(' +
-    VERSION_PREFIX + r'(?P<version_guid>[A-F0-9]+))?(' +
-    BLOCK_PREFIX + r'(?P<block>' + ALLOWED_ID_CHARS + r'+))?$', re.IGNORECASE
-)
-
-
 def parse_course_id(string):
     r"""
 
@@ -122,7 +103,7 @@ def parse_course_id(string):
     Block is optional: if missing returned_dict['block'] is None.
     Else returns None.
     """
-    match = COURSE_ID_RE.match(string)
+    match = URL_RE.match(string)
     if not match:
         return None
     return match.groupdict()

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

@@ -247,7 +247,8 @@ class TestLocationMapper(unittest.TestCase):
         new_style_course_id = '{}.geek_dept.{}.baz_run'.format(org, course)
         prob_locator = BlockUsageLocator(
             course_id=new_style_course_id,
-            usage_id='problem2'
+            usage_id='problem2',
+            branch='published'
         )
         prob_location = loc_mapper().translate_locator_to_location(prob_locator)
         self.assertIsNone(prob_location, 'found entry in empty map table')
@@ -265,7 +266,9 @@ class TestLocationMapper(unittest.TestCase):
         # default branch
         self.assertEqual(prob_location, Location('i4x', org, course, 'problem', 'abc123', None))
         # explicit branch
-        prob_locator = BlockUsageLocator(prob_locator, branch='draft')
+        prob_locator = BlockUsageLocator(
+            course_id=prob_locator.course_id, branch='draft', usage_id=prob_locator.usage_id
+        )
         prob_location = loc_mapper().translate_locator_to_location(prob_locator)
         self.assertEqual(prob_location, Location('i4x', org, course, 'problem', 'abc123', 'draft'))
         prob_locator = BlockUsageLocator(
@@ -276,12 +279,13 @@ class TestLocationMapper(unittest.TestCase):
         # same for chapter except chapter cannot be draft in old system
         chap_locator = BlockUsageLocator(
             course_id=new_style_course_id,
-            usage_id='chapter48f'
+            usage_id='chapter48f',
+            branch='production'
         )
         chap_location = loc_mapper().translate_locator_to_location(chap_locator)
         self.assertEqual(chap_location, Location('i4x', org, course, 'chapter', '48f23a10395384929234'))
         # explicit branch
-        chap_locator = BlockUsageLocator(chap_locator, branch='draft')
+        chap_locator.branch = 'draft'
         chap_location = loc_mapper().translate_locator_to_location(chap_locator)
         self.assertEqual(chap_location, Location('i4x', org, course, 'chapter', '48f23a10395384929234'))
         chap_locator = BlockUsageLocator(
@@ -382,7 +386,7 @@ class TestLocationMapper(unittest.TestCase):
         self.assertEqual(new_usage_id, new_usage_id2)
         # it should be in the distractor now
         new_location = loc_mapper().translate_locator_to_location(
-            BlockUsageLocator(course_id=new_style_course_id, usage_id=new_usage_id2)
+            BlockUsageLocator(course_id=new_style_course_id, usage_id=new_usage_id2, branch='published')
         )
         self.assertEqual(new_location, location)
         # add one close to the existing chapter (cause name collision)
@@ -391,11 +395,15 @@ class TestLocationMapper(unittest.TestCase):
         self.assertRegexpMatches(new_usage_id, r'^chapter48f\d')
         # retrievable from both courses
         new_location = loc_mapper().translate_locator_to_location(
-            BlockUsageLocator(course_id=new_style_course_id, usage_id=new_usage_id)
+            BlockUsageLocator(course_id=new_style_course_id, usage_id=new_usage_id, branch='published')
         )
         self.assertEqual(new_location, location)
         new_location = loc_mapper().translate_locator_to_location(
-            BlockUsageLocator(course_id='{}.{}.{}'.format(org, course, 'baz_run'), usage_id=new_usage_id)
+            BlockUsageLocator(
+                course_id='{}.{}.{}'.format(org, course, 'baz_run'),
+                usage_id=new_usage_id,
+                branch='published'
+            )
         )
         self.assertEqual(new_location, location)
 
@@ -443,11 +451,11 @@ class TestLocationMapper(unittest.TestCase):
         # change in all courses to same value
         loc_mapper().update_block_location_translator(location, 'problem1')
         trans_loc = loc_mapper().translate_locator_to_location(
-            BlockUsageLocator(course_id=new_style_course_id, usage_id='problem1')
+            BlockUsageLocator(course_id=new_style_course_id, usage_id='problem1', branch='published')
         )
         self.assertEqual(trans_loc, location)
         trans_loc = loc_mapper().translate_locator_to_location(
-            BlockUsageLocator(course_id=new_style_course_id2, usage_id='problem1')
+            BlockUsageLocator(course_id=new_style_course_id2, usage_id='problem1', branch='published')
         )
         self.assertEqual(trans_loc, location)
         # try to change to overwrite used usage_id
@@ -457,12 +465,12 @@ class TestLocationMapper(unittest.TestCase):
         # just change the one course
         loc_mapper().update_block_location_translator(location, 'chapter2', '{}/{}/{}'.format(org, course, 'baz_run'))
         trans_loc = loc_mapper().translate_locator_to_location(
-            BlockUsageLocator(course_id=new_style_course_id, usage_id='chapter2')
+            BlockUsageLocator(course_id=new_style_course_id, usage_id='chapter2', branch='published')
         )
         self.assertEqual(trans_loc.name, '48f23a10395384929234')
         # but this still points to the old
         trans_loc = loc_mapper().translate_locator_to_location(
-            BlockUsageLocator(course_id=new_style_course_id2, usage_id='chapter2')
+            BlockUsageLocator(course_id=new_style_course_id2, usage_id='chapter2', branch='published')
         )
         self.assertEqual(trans_loc.name, '1')
 
@@ -496,10 +504,10 @@ class TestLocationMapper(unittest.TestCase):
         # delete from all courses
         loc_mapper().delete_block_location_translator(location)
         self.assertIsNone(loc_mapper().translate_locator_to_location(
-            BlockUsageLocator(course_id=new_style_course_id, usage_id='problem1')
+            BlockUsageLocator(course_id=new_style_course_id, usage_id='problem1', branch='published')
         ))
         self.assertIsNone(loc_mapper().translate_locator_to_location(
-            BlockUsageLocator(course_id=new_style_course_id2, usage_id='problem2')
+            BlockUsageLocator(course_id=new_style_course_id2, usage_id='problem2', branch='published')
         ))
         # delete from one course
         location = location.replace(name='abc123')