Merge pull request #627 from edx/christina/restful-url

Change locators to a restful interface.

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

 """
-Created on Mar 13, 2013
-
-@author: dmitchell
+Identifier for course resources.
 """
+
 from __future__ import absolute_import
 import logging
 import inspect
@@ -15,6 +14,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
 
 log = logging.getLogger(__name__)
 
@@ -37,9 +37,6 @@ class Locator(object):
         """
         raise InsufficientSpecificationError()
 
-    def quoted_url(self):
-        return quote(self.url(), '@;#')
-
     def __eq__(self, other):
         return self.__dict__ == other.__dict__
 
@@ -90,11 +87,12 @@ class CourseLocator(Locator):
     Examples of valid CourseLocator specifications:
      CourseLocator(version_guid=ObjectId('519665f6223ebd6980884f2b'))
      CourseLocator(course_id='mit.eecs.6002x')
-     CourseLocator(course_id='mit.eecs.6002x;published')
+     CourseLocator(course_id='mit.eecs.6002x/branch/published')
      CourseLocator(course_id='mit.eecs.6002x', branch='published')
-     CourseLocator(url='edx://@519665f6223ebd6980884f2b')
+     CourseLocator(url='edx://version/519665f6223ebd6980884f2b')
      CourseLocator(url='edx://mit.eecs.6002x')
-     CourseLocator(url='edx://mit.eecs.6002x;published')
+     CourseLocator(url='edx://mit.eecs.6002x/branch/published')
+     CourseLocator(url='edx://mit.eecs.6002x/branch/published/version/519665f6223ebd6980884f2b')
 
     Should have at lease a specific course_id (id for the course as if it were a project w/
     versions) with optional 'branch',
@@ -115,10 +113,10 @@ class CourseLocator(Locator):
         if self.course_id:
             result = self.course_id
             if self.branch:
-                result += ';' + self.branch
+                result += BRANCH_PREFIX + self.branch
             return result
         elif self.version_guid:
-            return '@' + str(self.version_guid)
+            return URL_VERSION_PREFIX + str(self.version_guid)
         else:
             # raise InsufficientSpecificationError("missing course_id or version_guid")
             return '<InsufficientSpecificationError: missing course_id or version_guid>'
@@ -223,21 +221,18 @@ class CourseLocator(Locator):
     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)
-        If a block ('#HW3') is present, it is ignored.
+        either a valid version_guid or course_id (with optional branch), or both.
         """
         if isinstance(url, Locator):
             url = url.url()
-        assert isinstance(url, basestring), \
-            '%s is not an instance of basestring' % url
+        assert isinstance(url, basestring), '%s is not an instance of basestring' % url
         parse = parse_url(url)
         assert parse, 'Could not parse "%s" as a url' % url
-        if 'version_guid' in parse:
-            new_guid = parse['version_guid']
-            self.set_version_guid(self.as_object_id(new_guid))
-        else:
-            self.set_course_id(parse['id'])
-            self.set_branch(parse['branch'])
+        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, 'branch', lambda (new_branch): self.set_branch(new_branch))
 
     def init_from_version_guid(self, version_guid):
         """
@@ -253,14 +248,14 @@ class CourseLocator(Locator):
 
     def init_from_course_id(self, course_id, explicit_branch=None):
         """
-        Course_id is a string like 'mit.eecs.6002x' or 'mit.eecs.6002x;published'.
+        Course_id is a string like 'mit.eecs.6002x' or 'mit.eecs.6002x/branch/published'.
 
         Revision (optional) is a string like 'published'.
         It may be provided explicitly (explicit_branch) or embedded into course_id.
-        If branch is part of course_id ("...;published"), parse it out separately.
+        If branch is part of course_id (".../branch/published"), parse it out separately.
         If branch is provided both ways, that's ok as long as they are the same value.
 
-        If a block ('#HW3') is a part of course_id, it is ignored.
+        If a block ('/block/HW3') is a part of course_id, it is ignored.
 
         """
 
@@ -295,6 +290,16 @@ class CourseLocator(Locator):
         """
         return self.course_id
 
+    def _set_value(self, parse, key, setter):
+        """
+        Helper method that gets a value out of the dict returned by parse,
+        and then sets the corresponding bit of information in this locator
+        (via the supplied lambda 'setter'), unless the value is None.
+        """
+        value = parse.get(key, None)
+        if value:
+            setter(value)
+
 
 class BlockUsageLocator(CourseLocator):
     """
@@ -390,9 +395,7 @@ class BlockUsageLocator(CourseLocator):
             url = url.url()
         parse = parse_url(url)
         assert parse, 'Could not parse "%s" as a url' % url
-        block = parse.get('block', None)
-        if block:
-            self.set_usage_id(block)
+        self._set_value(parse, 'block', lambda(new_block): self.set_usage_id(new_block))
 
     def init_block_ref_from_course_id(self, course_id):
         if isinstance(course_id, CourseLocator):
@@ -400,9 +403,7 @@ class BlockUsageLocator(CourseLocator):
             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
-        block = parse.get('block', None)
-        if block:
-            self.set_usage_id(block)
+        self._set_value(parse, 'block', lambda(new_block): self.set_usage_id(new_block))
 
     def __unicode__(self):
         """
@@ -411,14 +412,14 @@ class BlockUsageLocator(CourseLocator):
         rep = CourseLocator.__unicode__(self)
         if self.usage_id is None:
             # usage_id has not been initialized
-            return rep + '#NONE'
+            return rep + BLOCK_PREFIX + 'NONE'
         else:
-            return rep + '#' + self.usage_id
+            return rep + BLOCK_PREFIX + self.usage_id
 
 
 class DescriptionLocator(Locator):
     """
-    Container for how to locate a description
+    Container for how to locate a description (the course-independent content).
     """
 
     def __init__(self, definition_id):
@@ -427,14 +428,14 @@ class DescriptionLocator(Locator):
     def __unicode__(self):
         '''
         Return a string representing this location.
-        unicode(self) returns something like this: "@519665f6223ebd6980884f2b"
+        unicode(self) returns something like this: "version/519665f6223ebd6980884f2b"
         '''
-        return '@' + str(self.definition_guid)
+        return URL_VERSION_PREFIX + str(self.definition_id)
 
     def url(self):
         """
         Return a string containing the URL for this location.
-        url(self) returns something like this: 'edx://@519665f6223ebd6980884f2b'
+        url(self) returns something like this: 'edx://version/519665f6223ebd6980884f2b'
         """
         return 'edx://' + unicode(self)
 
@@ -442,7 +443,7 @@ class DescriptionLocator(Locator):
         """
         Returns the ObjectId referencing this specific location.
         """
-        return self.definition_guid
+        return self.definition_id
 
 
 class VersionTree(object):

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

 import re
 
+# Prefix for the branch portion of a locator URL
+BRANCH_PREFIX = "/branch/"
+# Prefix for the block portion of a locator URL
+BLOCK_PREFIX = "/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/'
+
 URL_RE = re.compile(r'^edx://(.+)$', re.IGNORECASE)
 
 
@@ -9,26 +18,27 @@ def parse_url(string):
     followed by either a version_guid or a course_id.
 
     Examples:
-        'edx://@0123FFFF'
+        'edx://version/0123FFFF'
         'edx://edu.mit.eecs.6002x'
-        'edx://edu.mit.eecs.6002x;published'
-        'edx://edu.mit.eecs.6002x;published#HW3'
+        'edx://edu.mit.eecs.6002x/branch/published'
+        'edx://edu.mit.eecs.6002x/branch/published/version/519665f6223ebd6980884f2b/block/HW3'
+        'edx://edu.mit.eecs.6002x/branch/published/block/HW3'
 
     This returns None if string cannot be parsed.
 
-    If it can be parsed as a version_guid, returns a dict
+    If it can be parsed as a version_guid with no preceding course_id, returns a dict
     with key 'version_guid' and the value,
 
     If it can be parsed as a course_id, returns a dict
-    with keys 'id' and 'branch' (value of 'branch' may be None),
+    with key 'id' and optional keys 'branch' and 'version_guid'.
 
     """
     match = URL_RE.match(string)
     if not match:
         return None
     path = match.group(1)
-    if path[0] == '@':
-        return parse_guid(path[1:])
+    if path.startswith(URL_VERSION_PREFIX):
+        return parse_guid(path[len(URL_VERSION_PREFIX):])
     return parse_course_id(path)
 
 
@@ -52,7 +62,7 @@ def parse_block_ref(string):
     return None
 
 
-GUID_RE = re.compile(r'^(?P<version_guid>[A-F0-9]+)(#(?P<block>\w+))?$', re.IGNORECASE)
+GUID_RE = re.compile(r'^(?P<version_guid>[A-F0-9]+)(' + BLOCK_PREFIX + '(?P<block>\w+))?$', re.IGNORECASE)
 
 
 def parse_guid(string):
@@ -69,27 +79,34 @@ def parse_guid(string):
         return None
 
 
-COURSE_ID_RE = re.compile(r'^(?P<id>(\w+)(\.\w+\w*)*)(;(?P<branch>\w+))?(#(?P<block>\w+))?$', re.IGNORECASE)
+COURSE_ID_RE = re.compile(
+    r'^(?P<id>(\w+)(\.\w+\w*)*)(' +
+    BRANCH_PREFIX + '(?P<branch>\w+))?(' +
+    VERSION_PREFIX + '(?P<version_guid>[A-F0-9]+))?(' +
+    BLOCK_PREFIX + '(?P<block>\w+))?$', re.IGNORECASE
+)
 
 
 def parse_course_id(string):
     r"""
 
     A course_id has a main id component.
-    There may also be an optional branch (;published or ;draft).
-    There may also be an optional block (#HW3 or #Quiz2).
+    There may also be an optional branch (/branch/published or /branch/draft).
+    There may also be an optional version (/version/519665f6223ebd6980884f2b).
+    There may also be an optional block (/block/HW3 or /block/Quiz2).
 
     Examples of valid course_ids:
 
       'edu.mit.eecs.6002x'
-      'edu.mit.eecs.6002x;published'
-      'edu.mit.eecs.6002x#HW3'
-      'edu.mit.eecs.6002x;published#HW3'
+      'edu.mit.eecs.6002x/branch/published'
+      'edu.mit.eecs.6002x/block/HW3'
+      'edu.mit.eecs.6002x/branch/published/block/HW3'
+      'edu.mit.eecs.6002x/branch/published/version/519665f6223ebd6980884f2b/block/HW3'
 
 
     Syntax:
 
-      course_id = main_id [; branch] [# block]
+      course_id = main_id [/branch/ branch] [/version/ version ] [/block/ block]
 
       main_id = name [. name]*
 

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

-'''
-Created on Mar 14, 2013
-
-@author: dmitchell
-'''
+"""
+Tests for xmodule.modulestore.locator.
+"""
 from unittest import TestCase
 
 from bson.objectid import ObjectId
-from xmodule.modulestore.locator import Locator, CourseLocator, BlockUsageLocator
+from xmodule.modulestore.locator import Locator, CourseLocator, BlockUsageLocator, DescriptionLocator
+from xmodule.modulestore.parsers import BRANCH_PREFIX, BLOCK_PREFIX, VERSION_PREFIX, URL_VERSION_PREFIX
 from xmodule.modulestore.exceptions import InsufficientSpecificationError, OverSpecificationError
 
 
 class LocatorTest(TestCase):
+    """
+    Tests for subclasses of Locator.
+    """
 
     def test_cant_instantiate_abstract_class(self):
         self.assertRaises(TypeError, Locator)
@@ -32,12 +34,12 @@ class LocatorTest(TestCase):
         self.assertRaises(
             OverSpecificationError,
             CourseLocator,
-            url='edx://mit.eecs.6002x;published',
+            url='edx://mit.eecs.6002x' + BRANCH_PREFIX + 'published',
             branch='draft')
         self.assertRaises(
             OverSpecificationError,
             CourseLocator,
-            course_id='mit.eecs.6002x;published',
+            course_id='mit.eecs.6002x' + BRANCH_PREFIX + 'published',
             branch='draft')
 
     def test_course_constructor_underspecified(self):
@@ -55,8 +57,8 @@ class LocatorTest(TestCase):
         testobj_1 = CourseLocator(version_guid=test_id_1)
         self.check_course_locn_fields(testobj_1, 'version_guid', version_guid=test_id_1)
         self.assertEqual(str(testobj_1.version_guid), test_id_1_loc)
-        self.assertEqual(str(testobj_1), '@' + test_id_1_loc)
-        self.assertEqual(testobj_1.url(), 'edx://@' + test_id_1_loc)
+        self.assertEqual(str(testobj_1), URL_VERSION_PREFIX + test_id_1_loc)
+        self.assertEqual(testobj_1.url(), 'edx://' + URL_VERSION_PREFIX + test_id_1_loc)
 
         # Test using a given string
         test_id_2_loc = '519665f6223ebd6980884f2b'
@@ -64,8 +66,8 @@ class LocatorTest(TestCase):
         testobj_2 = CourseLocator(version_guid=test_id_2)
         self.check_course_locn_fields(testobj_2, 'version_guid', version_guid=test_id_2)
         self.assertEqual(str(testobj_2.version_guid), test_id_2_loc)
-        self.assertEqual(str(testobj_2), '@' + test_id_2_loc)
-        self.assertEqual(testobj_2.url(), 'edx://@' + test_id_2_loc)
+        self.assertEqual(str(testobj_2), URL_VERSION_PREFIX + test_id_2_loc)
+        self.assertEqual(testobj_2.url(), 'edx://' + URL_VERSION_PREFIX + test_id_2_loc)
 
     def test_course_constructor_bad_course_id(self):
         """
@@ -74,20 +76,20 @@ class LocatorTest(TestCase):
         for bad_id in ('mit.',
                        ' mit.eecs',
                        'mit.eecs ',
-                       '@mit.eecs',
-                       '#mit.eecs',
+                       URL_VERSION_PREFIX + 'mit.eecs',
+                       BLOCK_PREFIX + 'block/mit.eecs',
                        'mit.ee cs',
                        'mit.ee,cs',
                        'mit.ee/cs',
                        'mit.ee$cs',
                        'mit.ee&cs',
                        'mit.ee()cs',
-                       ';this',
-                       'mit.eecs;',
-                       'mit.eecs;this;that',
-                       'mit.eecs;this;',
-                       'mit.eecs;this ',
-                       'mit.eecs;th%is ',
+                       BRANCH_PREFIX + 'this',
+                       'mit.eecs' + BRANCH_PREFIX,
+                       'mit.eecs' + BRANCH_PREFIX + 'this' + BRANCH_PREFIX + 'that',
+                       'mit.eecs' + BRANCH_PREFIX + 'this' + BRANCH_PREFIX,
+                       'mit.eecs' + BRANCH_PREFIX + 'this ',
+                       'mit.eecs' + BRANCH_PREFIX + 'th%is ',
                        ):
             self.assertRaises(AssertionError, CourseLocator, course_id=bad_id)
             self.assertRaises(AssertionError, CourseLocator, url='edx://' + bad_id)
@@ -106,7 +108,7 @@ class LocatorTest(TestCase):
         self.check_course_locn_fields(testobj, 'course_id', course_id=testurn)
 
     def test_course_constructor_redundant_002(self):
-        testurn = 'mit.eecs.6002x;published'
+        testurn = 'mit.eecs.6002x' + BRANCH_PREFIX + 'published'
         expected_urn = 'mit.eecs.6002x'
         expected_rev = 'published'
         testobj = CourseLocator(course_id=testurn, url='edx://' + testurn)
@@ -114,6 +116,32 @@ class LocatorTest(TestCase):
                                       course_id=expected_urn,
                                       branch=expected_rev)
 
+    def test_course_constructor_url(self):
+        # Test parsing a url when it starts with a version ID and there is also a block ID.
+        # This hits the parsers parse_guid method.
+        test_id_loc = '519665f6223ebd6980884f2b'
+        testobj = CourseLocator(url="edx://" + URL_VERSION_PREFIX + test_id_loc + BLOCK_PREFIX + "hw3")
+        self.check_course_locn_fields(
+            testobj,
+            'test_block constructor',
+            version_guid=ObjectId(test_id_loc)
+        )
+
+    def test_course_constructor_url_course_id_and_version_guid(self):
+        test_id_loc = '519665f6223ebd6980884f2b'
+        testobj = CourseLocator(url='edx://mit.eecs.6002x' + VERSION_PREFIX + test_id_loc)
+        self.check_course_locn_fields(testobj, 'error parsing url with both course ID and version GUID',
+                                      course_id='mit.eecs.6002x',
+                                      version_guid=ObjectId(test_id_loc))
+
+    def test_course_constructor_url_course_id_branch_and_version_guid(self):
+        test_id_loc = '519665f6223ebd6980884f2b'
+        testobj = CourseLocator(url='edx://mit.eecs.6002x' + BRANCH_PREFIX + 'draft' + VERSION_PREFIX + test_id_loc)
+        self.check_course_locn_fields(testobj, 'error parsing url with both course ID branch, and version GUID',
+                                      course_id='mit.eecs.6002x',
+                                      branch='draft',
+                                      version_guid=ObjectId(test_id_loc))
+
     def test_course_constructor_course_id_no_branch(self):
         testurn = 'mit.eecs.6002x'
         testobj = CourseLocator(course_id=testurn)
@@ -123,7 +151,7 @@ class LocatorTest(TestCase):
         self.assertEqual(testobj.url(), 'edx://' + testurn)
 
     def test_course_constructor_course_id_with_branch(self):
-        testurn = 'mit.eecs.6002x;published'
+        testurn = 'mit.eecs.6002x' + BRANCH_PREFIX + 'published'
         expected_id = 'mit.eecs.6002x'
         expected_branch = 'published'
         testobj = CourseLocator(course_id=testurn)
@@ -139,7 +167,7 @@ class LocatorTest(TestCase):
     def test_course_constructor_course_id_separate_branch(self):
         test_id = 'mit.eecs.6002x'
         test_branch = 'published'
-        expected_urn = 'mit.eecs.6002x;published'
+        expected_urn = 'mit.eecs.6002x' + BRANCH_PREFIX + 'published'
         testobj = CourseLocator(course_id=test_id, branch=test_branch)
         self.check_course_locn_fields(testobj, 'course_id with separate branch',
                                       course_id=test_id,
@@ -154,10 +182,10 @@ class LocatorTest(TestCase):
         """
         The same branch appears in the course_id and the branch field.
         """
-        test_id = 'mit.eecs.6002x;published'
+        test_id = 'mit.eecs.6002x' + BRANCH_PREFIX + 'published'
         test_branch = 'published'
         expected_id = 'mit.eecs.6002x'
-        expected_urn = 'mit.eecs.6002x;published'
+        expected_urn = test_id
         testobj = CourseLocator(course_id=test_id, branch=test_branch)
         self.check_course_locn_fields(testobj, 'course_id with repeated branch',
                                       course_id=expected_id,
@@ -169,7 +197,7 @@ class LocatorTest(TestCase):
         self.assertEqual(testobj.url(), 'edx://' + expected_urn)
 
     def test_block_constructor(self):
-        testurn = 'mit.eecs.6002x;published#HW3'
+        testurn = 'mit.eecs.6002x' + BRANCH_PREFIX + 'published' + BLOCK_PREFIX + 'HW3'
         expected_id = 'mit.eecs.6002x'
         expected_branch = 'published'
         expected_block_ref = 'HW3'
@@ -181,6 +209,43 @@ class LocatorTest(TestCase):
         self.assertEqual(str(testobj), testurn)
         self.assertEqual(testobj.url(), 'edx://' + testurn)
 
+    def test_block_constructor_url_version_prefix(self):
+        test_id_loc = '519665f6223ebd6980884f2b'
+        testobj = BlockUsageLocator(
+            url='edx://mit.eecs.6002x' + VERSION_PREFIX + test_id_loc + BLOCK_PREFIX + 'lab2'
+        )
+        self.check_block_locn_fields(
+            testobj, 'error parsing URL with version and block',
+            course_id='mit.eecs.6002x',
+            block='lab2',
+            version_guid=ObjectId(test_id_loc)
+        )
+
+    def test_block_constructor_url_kitchen_sink(self):
+        test_id_loc = '519665f6223ebd6980884f2b'
+        testobj = BlockUsageLocator(
+            url='edx://mit.eecs.6002x' + BRANCH_PREFIX + 'draft' + VERSION_PREFIX + test_id_loc + BLOCK_PREFIX + 'lab2'
+        )
+        self.check_block_locn_fields(
+            testobj, 'error parsing URL with branch, version, and block',
+            course_id='mit.eecs.6002x',
+            branch='draft',
+            block='lab2',
+            version_guid=ObjectId(test_id_loc)
+        )
+
+    def test_repr(self):
+        testurn = 'mit.eecs.6002x' + BRANCH_PREFIX + 'published' + BLOCK_PREFIX + 'HW3'
+        testobj = BlockUsageLocator(course_id=testurn)
+        self.assertEqual('BlockUsageLocator("mit.eecs.6002x/branch/published/block/HW3")', repr(testobj))
+
+    def test_description_locator_url(self):
+        definition_locator = DescriptionLocator("chapter12345_2")
+        self.assertEqual('edx://' + URL_VERSION_PREFIX + 'chapter12345_2', definition_locator.url())
+
+    def test_description_locator_version(self):
+        definition_locator = DescriptionLocator("chapter12345_2")
+        self.assertEqual("chapter12345_2", definition_locator.version())
 
     # ------------------------------------------------------------------
     # Utilities