Fix URL-quoting issues (fixes #7).

src/pyneric/rest_requests.py

@@ -10,7 +10,7 @@
 
 import inspect
 import functools
-from urllib.parse import urljoin, urlsplit, urlunsplit
+from urllib.parse import quote, unquote, urljoin, urlsplit, urlunsplit
 
 from pyneric.meta import Metaclass
 from pyneric import util
@@ -20,20 +20,29 @@
 __all__ = []
 
 _ENCODING = 'UTF-8'
+"""Assumed string encoding (matches quote/unquote default)"""
 
 SEPARATOR = '/'
 """URL path separator"""
 
 
-def _url_join(base, url):
-    base = ensure_text(base)
-    url = ensure_text(url)
+def _ensure_text(value, coerce=True):
+    return ensure_text(value, _ENCODING, coerce=coerce)
+
+
+def _unquote(string):
+    return unquote(string, errors='strict')
+
+
+def _url_join(base, path, safe=''):
+    base = _ensure_text(base)
+    path = quote(_ensure_text(path), safe=safe)
     if base.endswith(SEPARATOR):
-        if not url.endswith(SEPARATOR):
-            url += SEPARATOR
+        if not path.endswith(SEPARATOR):
+            path += SEPARATOR
     else:
         base += SEPARATOR
-    return urljoin(base, url)
+    return urljoin(base, path)
 
 
 def _url_split(url):
@@ -55,7 +64,7 @@ def validate_url_path(value):
         if value is None:
             return  # Resource is abstract; no further validation is necessary.
         try:
-            ensure_text(value, _ENCODING)
+            _ensure_text(value, coerce=False)
         except TypeError:
             raise TypeError(
                 "invalid url_path attribute (not string): {!r}"
@@ -116,7 +125,8 @@ class RestResource(future.with_metaclass(_RestMetaclass, object)):
 
     This may be `None` to signify that this is an abstract resource; otherwise,
     it is the path under the base (API root or containing resource) identifying
-    this resource.
+    this resource, which will be automatically URL-quoted (except for path
+    separators) when it is included in a URL produced by the library.
 
     This may contain path separator(s) ("/") if there is no need to access the
     path segments as distinct REST resources.
@@ -197,7 +207,7 @@ def invalid_for_type(reason=None):
             container = container.url
         elif not isinstance(container, basestring):
             invalid_for_type("It must be a string (URL).")
-        self._url = _url_join(container, self.url_path)
+        self._url = _url_join(container, self.url_path, safe=SEPARATOR)
 
     @classmethod
     def from_url(cls, url):
@@ -218,11 +228,11 @@ def _from_url(cls, url, **kwargs):
 
     @classmethod
     def _get_container_from_url(cls, url):
-        original_url, url = url, ensure_text(url, _ENCODING)
+        original_url, url = url, _ensure_text(url)
         url_split = _url_split(url)
-        segments = url_split[2].split(SEPARATOR)
-        resource_segments = (ensure_text(cls.url_path).rstrip(SEPARATOR)
-                             .split(SEPARATOR))
+        segments = [quote(_unquote(x)) for x in url_split[2].split(SEPARATOR)]
+        resource_segments = (quote(_ensure_text(cls.url_path))
+                             .rstrip(SEPARATOR).split(SEPARATOR))
         size = len(resource_segments)
         if segments[-size:] != resource_segments:
             multiple = size != 1
@@ -256,7 +266,7 @@ def container(self):
         """The container of this resource.
 
         This is an instance of :attr:`container_class` if that is not `None`;
-        otherwise, this is the REST URL under which this resource resides.
+        otherwise, this is the URL under which this resource resides.
 
         Whether the container has a trailing slash determines whether the
         resource's URL includes a trailing slash.
@@ -315,19 +325,20 @@ class RestCollection(future.with_metaclass(_RestCollectionMetaclass,
     """
 
     def __init__(self, container, id=None):
-        """Initialize an instance of this REST collection.
+        """Initialize an instance of this REST collection or a member.
 
-        `container` is interpreted the same as for `RestResource`.
+        :param str/RestResource container: See :attr:`RestResource.container`.
+        :param id: See :attr:`id`.
 
-        If `id` is None, the instance represents the collection rather than
-        one of its members.
+        The instance represents the collection when `id` is `None`; otherwise,
+        it represents one of its members.
 
         """
         super().__init__(container)
         self._id = id = self.validate_id(id)
         if id is None:
             return
-        self._url = _url_join(self._url, str(id))
+        self._url = _url_join(self._url, id)
 
     @classmethod
     def _from_url(cls, url, **kwargs):
@@ -338,39 +349,42 @@ def _from_url(cls, url, **kwargs):
             result = super_method(url)
         except ValueError:
             result = None
-        url_split = _url_split(ensure_text(url, _ENCODING))
-        segments = url_split[2].split(SEPARATOR)
+        url_split = _url_split(_ensure_text(url))
+        url_split[2], id = url_split[2].rsplit(SEPARATOR, 1)
         try:
-            id = cls.validate_id(segments.pop(-1))
+            id = cls.validate_id(_unquote(id))
         except ValueError:
             if result:
                 return result
             raise
-        url_split[2] = SEPARATOR.join(segments)
         collection_url = urlunsplit(url_split)
-        member_result = tryf(super_method, collection_url, id=id)
-        if result and member_result:
+        try:
+            member_result = super_method(collection_url, id=id)
+        except ValueError:
+            if result:
+                return result
+            raise
+        if result:
             raise ValueError(
                 "The URL {!r} is ambiguous for {} as to whether "
                 "it is for the collection or one of its members."
                 .format(url, cls.__name__))
-        return member_result or result
-        # return cls(cls._get_container_from_url(collection_url), id=id)
+        return member_result
 
     @classmethod
     def validate_id(cls, value):
         """Validate the given value as a valid identifier for this resource."""
+        if value is None:
+            return
         id = value
-        if id is None:
-            return id
         if not isinstance(value, cls.id_type):
             try:
                 id = cls.id_type(id)
             except Exception as exc:
                 raise ValueError(
                     "The id {!r} cannot be cast to {!r}.  {}"
                     .format(value, cls.id_type, str(exc)))
-        if not str(id):
+        if not tryf(str, id):
             raise ValueError(
                 "The id {!r} has no string representation."
                 .format(value))
@@ -382,5 +396,18 @@ def id(self):
 
         This is `None` if this instance represents the entire collection.
 
+        The value provided to the constructor must be one of:
+
+        * `None`
+        * an instance of :attr:`id_type`
+        * a value that can be passed alone to :attr:`id_type`
+
+        In the last case, the object that results from the instantiation
+        becomes the value of this property.
+
+        Like :attr:`url_path`, when this value is included in a URL produced
+        by the library, it is automatically cast to a string and URL-quoted,
+        except that path separators (slashes) in :attr:`id` are also quoted.
+
         """
         return self._id

tests/non_django/test_rest_requests.py

@@ -1,9 +1,14 @@
 # -*- coding: utf-8 -*-
 """Tests for pyneric.rest_requests"""
 
+from future.utils import PY2
+
 from unittest import TestCase
+if PY2:
+    from urllib import quote
+else:
+    from urllib.parse import quote
 
-from future.utils import PY2
 from pyneric import rest_requests
 
 
@@ -236,12 +241,8 @@ class Resource(rest_requests.RestResource):
         self.assertEqual(obj.container_.container, ROOT)
 
     def test_from_url_invalid(self):
-        class Container(rest_requests.RestResource):
-            url_path = 'container'
-
         class Resource(rest_requests.RestResource):
             url_path = 'resource'
-            container_class = Container
 
         self.assertRaises(ValueError, Resource.from_url, 'invalid://url')
 
@@ -283,15 +284,24 @@ class Collection(rest_requests.RestCollection):
             url_path = 'things'
 
         id_ = 'important thing'
-        url = '{}/{}/{}'.format(ROOT, Collection.url_path, id_)
+        url = '{}/{}/{}'.format(ROOT, Collection.url_path, quote(id_))
         self.assertEqual(url, Collection(ROOT, id_).url)
 
     def test_init_member_unicode(self):
         class Collection(rest_requests.RestCollection):
             url_path = 'things'
 
         id_ = u'impörtant thing'
-        url = u'{}/{}/{}'.format(ROOT, Collection.url_path, id_)
+        url = u'{}/{}/{}'.format(ROOT, Collection.url_path,
+                                 quote(id_.encode('utf-8') if PY2 else id_))
+        self.assertEqual(url, Collection(ROOT, id_).url)
+
+    def test_init_member_slash(self):
+        class Collection(rest_requests.RestCollection):
+            url_path = 'things'
+
+        id_ = 'important/thing'
+        url = '{}/{}/{}'.format(ROOT, Collection.url_path, quote(id_, safe=''))
         self.assertEqual(url, Collection(ROOT, id_).url)
 
     def test_init_invalid_id_type(self):
@@ -321,15 +331,23 @@ class Resource(rest_requests.RestCollection):
             url_path = 'others'
             container_class = Container
 
+        def inner_test(obj):
+            self.assertIsInstance(obj.container_, Container)
+            self.assertEqual(container_id, obj.container_.id)
+            self.assertEqual(ROOT, obj.container_.container)
+            self.assertEqual(resource_id, obj.id)
+
         container_id = 'a thing'
-        resource_id = 'another'
+        resource_id = 'an other'
         url = '{}/{}/{}/{}/{}'.format(ROOT, Container.url_path, container_id,
                                       Resource.url_path, resource_id)
-        obj = Resource.from_url(url)
-        self.assertIsInstance(obj.container_, Container)
-        self.assertEqual(container_id, obj.container_.id)
-        self.assertEqual(ROOT, obj.container_.container)
-        self.assertEqual(resource_id, obj.id)
+        inner_test(Resource.from_url(url))
+
+        # Test the same thing with a properly quoted URL.
+        url = '{}/{}/{}/{}/{}'.format(ROOT, Container.url_path,
+                                      quote(container_id), Resource.url_path,
+                                      quote(resource_id))
+        inner_test(Resource.from_url(url))
 
     def test_from_url_member_unicode(self):
         class Container(rest_requests.RestCollection):
@@ -339,15 +357,43 @@ class Resource(rest_requests.RestCollection):
             url_path = 'others'
             container_class = Container
 
+        def inner_test(obj):
+            self.assertIsInstance(obj.container_, Container)
+            self.assertEqual(container_id, obj.container_.id)
+            self.assertEqual(ROOT, obj.container_.container)
+            self.assertEqual(resource_id, obj.id)
+
         container_id = u'a thĩng'
         resource_id = u'anöther'
         url = u'{}/{}/{}/{}/{}'.format(ROOT, Container.url_path, container_id,
                                        Resource.url_path, resource_id)
-        obj = Resource.from_url(url)
-        self.assertIsInstance(obj.container_, Container)
-        self.assertEqual(container_id, obj.container_.id)
-        self.assertEqual(ROOT, obj.container_.container)
-        self.assertEqual(resource_id, obj.id)
+        inner_test(Resource.from_url(url))
+
+        # Test the same thing with a properly quoted URL.
+        url = u'{}/{}/{}/{}/{}'.format(ROOT, Container.url_path,
+                                       quote(container_id.encode('utf-8')
+                                             if PY2 else container_id),
+                                       Resource.url_path,
+                                       quote(resource_id.encode('utf-8')
+                                             if PY2 else resource_id))
+        inner_test(Resource.from_url(url))
+
+    def test_from_url_member_slash(self):
+        class Collection(rest_requests.RestCollection):
+            url_path = 'things'
+
+        id_ = 'important/thing'
+        url = '{}/{}/{}'.format(ROOT, Collection.url_path, quote(id_, safe=''))
+        obj = Collection.from_url(url)
+        self.assertEqual(ROOT, obj.container)
+        self.assertEqual(id_, obj.id)
+
+    def test_from_url_member_invalid_collection(self):
+        class Collection(rest_requests.RestCollection):
+            url_path = 'things'
+
+        url = '{}/x/x'.format(ROOT)
+        self.assertRaises(ValueError, Collection.from_url, url)
 
     def test_id_type_int(self):
         class Collection(rest_requests.RestCollection):