routespecs are strings (again)

- no slash means host-routing
- slash means no-host

jupyterhub/apihandlers/proxy.py

@@ -9,26 +9,9 @@ from urllib.parse import urlparse
 from tornado import gen, web
 
 from .. import orm
-from ..proxy import RouteSpec
 from ..utils import admin_only
 from .base import APIHandler
 
-class _RouteSpecJSONEncoder(json.JSONEncoder):
-    """JSON encoder that handles routespecs"""
-    def encode(self, obj):
-        if isinstance(obj, RouteSpec):
-            obj = obj.host + obj.path
-        return super().encode(obj)
-
-    def iterencode(self, obj, _one_shot=True):
-        if isinstance(obj, dict):
-            obj = {
-                key.host + key.path if isinstance(key, RouteSpec) else key : value
-                for key, value in obj.items()
-            }
-        return super().iterencode(obj)
-
-_json = _RouteSpecJSONEncoder()
 
 class ProxyAPIHandler(APIHandler):
     
@@ -41,7 +24,7 @@ class ProxyAPIHandler(APIHandler):
         but without clients needing to maintain separate
         """
         routes = yield self.proxy.get_all_routes()
-        self.write(_json.encode(routes))
+        self.write(json.dumps(routes))
 
     @admin_only
     @gen.coroutine

jupyterhub/objects.py

@@ -12,6 +12,7 @@ from traitlets import (
     HasTraits, Instance, Integer, Unicode,
     default, observe,
 )
+from .traitlets import URLPrefix
 from . import orm
 from .utils import (
     url_path_join, can_connect, wait_for_server,
@@ -30,7 +31,7 @@ class Server(HasTraits):
     connect_ip = Unicode()
     proto = Unicode('http')
     port = Integer()
-    base_url = Unicode('/')
+    base_url = URLPrefix('/')
     cookie_name = Unicode('')
 
     @property

jupyterhub/proxy.py

@@ -1,4 +1,11 @@
-"""API for JupyterHub's proxy."""
+"""API for JupyterHub's proxy.
+
+Route Specification:
+
+- A routespec is a URI (excluding scheme), e.g.
+  'host.name/path/' for host-based routing or '/path/' for default routing.
+- Route paths should be normalized to always start and end with `/`
+"""
 
 # Copyright (c) IPython Development Team.
 # Distributed under the terms of the Modified BSD License.
@@ -8,7 +15,7 @@ import json
 import os
 from subprocess import Popen
 import time
-from urllib.parse import quote
+from urllib.parse import quote, urlparse
 
 from tornado import gen
 from tornado.httpclient import AsyncHTTPClient, HTTPRequest
@@ -28,54 +35,6 @@ from . import utils
 from .utils import url_path_join
 
 
-class RouteSpec(namedtuple('RouteSpec', ['path', 'host'])):
-    """Object wrapping a proxy route specification
-
-    Attributes:
-
-        path (str): The URL path prefix for this route. Required.
-        host (str): The hostname used for routing when host-based routing is enabled.
-
-    """
-    def __new__(cls, path, *, host=''):
-        """Create a route specification.
-
-        Arguments:
-
-            path (str): The path prefix. Leading and trailing slashes will be added
-                if missing. Required.
-            host (str): The hostname used for routing when host-based routing is enabled.
-                Optional, keyword-only.
-        """
-        # give host a default value
-        if isinstance(path, cls) and host == '':
-            # RouteSpec(routespec) makes a copy
-            path, host = path
-
-        # ensure leading/trailing slashes
-        if not path.startswith('/'):
-            path = '/' + path
-        if not path.endswith('/'):
-            path = path + '/'
-        return super().__new__(cls, path, host)
-
-    @classmethod
-    def as_routespec(cls, routespec):
-        """Ensure a RouteSpec or str is a RouteSpec
-        
-        Allows string arguments to be accepted anywhere RouteSpecs are accepted.
-        """
-        if isinstance(routespec, str):
-            return cls(routespec)
-        else:
-            return routespec
-
-    def __repr__(self):
-        if not self.host:
-            return '%s(path=%r)' % (self.__class__.__name__, self.path)
-        else:
-            return super().__repr__()
-
 class Proxy(LoggingConfigurable):
     """Base class for configurable proxies that JupyterHub can use."""
 
@@ -107,13 +66,31 @@ class Proxy(LoggingConfigurable):
         Will be called during teardown if should_start is True.
         """
     
+    def validate_routespec(self, routespec):
+        """Validate a routespec
+        
+        - Checks host value vs host-based routing.
+        - Ensures trailing slash on path.
+        """
+        # check host routing
+        host_route = not routespec.startswith('/')
+        if host_route and not self.host_routing:
+            raise ValueError("Cannot add host-based route %r, not using host-routing" % routespec)
+        if self.host_routing and not host_route:
+            raise ValueError("Cannot add route without host %r, using host-routing" % routespec)
+        # add trailing slash
+        if not routespec.endswith('/'):
+            return routespec + '/'
+        else:
+            return routespec
+
     @gen.coroutine
     def add_route(self, routespec, target, data):
         """Add a route to the proxy.
 
         Args:
-            routespec (RouteSpec or str): A specification for which this route will be matched.
+            routespec (str): A URI (excluding scheme) for which this route will be matched,
-                If a string, should be treated as RouteSpec(routespec).
+                e.g. host.name/path/
             target (str): A URL that will be the target of this route.
             data (dict): A JSONable dict that will be associated with this route, and will
                 be returned when retrieving information about this route.
@@ -131,13 +108,31 @@ class Proxy(LoggingConfigurable):
         """Delete a route with a given routespec if it exists."""
         pass
 
+    @gen.coroutine
+    def get_all_routes(self):
+        """Fetch and return all the routes associated by JupyterHub from the
+        proxy.
+
+        Should return a dictionary of routes, where the keys are
+        routespecs and each value is a dict of the form::
+
+          {
+            'routespec': the route specification
+            'target': the target host for this route
+            'data': the attached data dict for this route (as specified in add_route)
+          }
+        that would be returned by
+        `get_route(routespec)`.
+        """
+        pass
+
     @gen.coroutine
     def get_route(self, routespec):
         """Return the route info for a given routespec.
 
         Args:
-            routespec (RouteSpec or str): The route specification that was used to add this route.
+            routespec (str): A URI that was used to add this route,
-                If a string, should be treated as RouteSpec(routespec).
+                e.g. `host.tld/path/`
 
         Returns:
             result (dict): with the following keys:
@@ -148,21 +143,10 @@ class Proxy(LoggingConfigurable):
             None: if there are no routes matching the given routespec
         """
         # default implementation relies on get_all_routes
-        routespec = RouteSpec.as_routespec(routespec)
+        routespec = self.validate_routespec(routespec)
         routes = yield self.get_all_routes()
         return routes.get(routespec)
 
-    @gen.coroutine
-    def get_all_routes(self):
-        """Fetch and return all the routes associated by JupyterHub from the
-        proxy.
-
-        Should return a dictionary of routes, where the keys are
-        routespecs and each value is the dict that would be returned by
-        `get_route(routespec)`.
-        """
-        pass
-
     # Most basic implementers must only implement above methods
 
     @gen.coroutine
@@ -418,37 +402,39 @@ class ConfigurableHTTPProxy(Proxy):
         yield self.restore_routes()
 
     def _routespec_to_chp_path(self, routespec):
-        """Turn a RouteSpec into a CHP API path"""
+        """Turn a routespec into a CHP API path
-        path = routespec.path
+
-        if routespec.host:
+        For host-based routing, CHP uses the host as the first path segment.
-            if not self.host_routing:
+        """
-                raise RuntimeError("Adding route with a host")
+        path = self.validate_routespec(routespec)
-            path = '/' + url_path_join(routespec.host, path)
+        # CHP always wants to start with /
+        if not path.startswith('/'):
+            path = path + '/'
+        # BUG: CHP doesn't seem to like trailing slashes on some endpoints (DELETE)
         if path != '/' and path.endswith('/'):
             path = path.rstrip('/')
         return path
 
     def _routespec_from_chp_path(self, chp_path):
-        """Turn a CHP route into a RouteSpec
+        """Turn a CHP route into a route spec
         
         In the JSON API, CHP route keys are unescaped,
-        so re-escape them to raw URLs.
+        so re-escape them to raw URLs and ensure slashes are in the right places.
         """
         # chp stores routes in unescaped form.
         # restore escaped-form we created it with.
-        path = quote(chp_path, safe='@/')
+        routespec = quote(chp_path, safe='@/')
-        host = ''
         if self.host_routing:
-            host, *rest = path.lstrip('/').split('/', 1)
+            # host routes don't start with /
-            path = '/' + ''.join(rest)
+            routespec = routespec.lstrip('/')
-        return RouteSpec(path, host=host)
+        # all routes should end with /
-
+        if not routespec.endswith('/'):
+            routespec = routespec + '/'
+        return routespec
 
     def api_request(self, path, method='GET', body=None, client=None):
         """Make an authenticated API request of the proxy."""
         client = client or AsyncHTTPClient()
-        if isinstance(path, RouteSpec):
-            path = self._routespec_to_chp_path(path)
         url = url_path_join(self.api_url, 'api/routes', path)
 
         if isinstance(body, dict):
@@ -464,22 +450,20 @@ class ConfigurableHTTPProxy(Proxy):
         return client.fetch(req)
 
     def add_route(self, routespec, target, data=None):
-        # ensure RouteSpec object
-        routespec = RouteSpec.as_routespec(routespec)
         body = data or {}
         body['target'] = target
-        return self.api_request(routespec,
+        path = self._routespec_to_chp_path(routespec)
+        return self.api_request(path,
                                 method='POST',
                                 body=body,
                                 )
 
     def delete_route(self, routespec):
-        routespec = RouteSpec.as_routespec(routespec)
+        path = self._routespec_to_chp_path(routespec)
-        return self.api_request(routespec, method='DELETE')
+        return self.api_request(path, method='DELETE')
 
     def _reformat_routespec(self, routespec, chp_data):
         """Reformat CHP data format to JupyterHub's proxy API."""
-        # ensure RouteSpec object
         target = chp_data.pop('target')
         return {
             'routespec': routespec,

jupyterhub/services/service.py

@@ -52,7 +52,6 @@ from traitlets.config import LoggingConfigurable
 
 from .. import orm
 from ..objects import Server
-from ..proxy import RouteSpec
 from ..traitlets import Command
 from ..spawner import LocalProcessSpawner, set_user_setuid
 from ..utils import url_path_join
@@ -249,9 +248,9 @@ class Service(LoggingConfigurable):
         if not self.server:
             return ''
         if self.domain:
-            return RouteSpec(path=self.server.base_url, host=self.domain)
+            return self.domain + self.server.base_url
         else:
-            return RouteSpec(self.server.base_url)
+            return self.server.base_url
 
     def __repr__(self):
         return "<{cls}(name={name}{managed})>".format(

jupyterhub/tests/test_api.py

@@ -413,9 +413,8 @@ def test_spawn(app, io_loop):
     status = io_loop.run_sync(app_user.spawner.poll)
     assert status is None
 
-    assert user.server.base_url == ujoin(app.base_url, 'user/%s' % name)
+    assert user.server.base_url == ujoin(app.base_url, 'user/%s' % name) + '/'
     url = public_url(app, user)
-    print(url)
     r = requests.get(url)
     assert r.status_code == 200
     assert r.text == user.server.base_url

jupyterhub/tests/test_pages.py

@@ -97,7 +97,7 @@ def test_spawn_redirect(app, io_loop):
     r.raise_for_status()
     print(urlparse(r.url))
     path = urlparse(r.url).path
-    assert path == ujoin(app.base_url, 'user/%s' % name)
+    assert path == ujoin(app.base_url, 'user/%s/' % name)
     
     # should have started server
     status = io_loop.run_sync(u.spawner.poll)
@@ -108,7 +108,7 @@ def test_spawn_redirect(app, io_loop):
     r.raise_for_status()
     print(urlparse(r.url))
     path = urlparse(r.url).path
-    assert path == ujoin(app.base_url, '/user/%s' % name)
+    assert path == ujoin(app.base_url, '/user/%s/' % name)
 
 def test_spawn_page(app):
     with mock.patch.dict(app.users.settings, {'spawner_class': FormSpawner}):

jupyterhub/tests/test_proxy.py

@@ -15,40 +15,6 @@ from .mocking import MockHub
 from .test_api import api_request
 from ..utils import wait_for_http_server, url_path_join as ujoin
 
-from jupyterhub.proxy import RouteSpec
-
-def test_routespec():
-    with pytest.raises(TypeError):
-        RouteSpec()
-
-    spec = RouteSpec('/test/')
-    assert spec.host == ''
-    assert spec.path == '/test/'
-
-    assert 'path=%r' % spec.path in repr(spec)
-    assert 'host' not in repr(spec)
-
-    spec = RouteSpec('test2', host='myhost')
-    assert spec.path == '/test2/'
-    assert spec.host == 'myhost'
-
-    assert 'path=%r' % spec.path in repr(spec)
-    assert 'host=%r' % spec.host in repr(spec)
-
-    copyspec = RouteSpec(spec)
-    assert copyspec.path == '/test2/'
-    assert copyspec.host == 'myhost'
-    assert copyspec == spec
-
-def test_as_routespec():
-    spec = RouteSpec('/test', host='myhost')
-    as_spec = RouteSpec.as_routespec(spec)
-    assert as_spec is spec
-
-    spec2 = RouteSpec.as_routespec('/path')
-    assert isinstance(spec2, RouteSpec)
-    assert spec2.path == '/path/'
-
 
 def test_external_proxy(request, io_loop):
 
@@ -98,7 +64,7 @@ def test_external_proxy(request, io_loop):
 
     # test if api service has a root route '/'
     routes = io_loop.run_sync(app.proxy.get_all_routes)
-    assert list(routes.keys()) == [RouteSpec('/')]
+    assert list(routes.keys()) == ['/']
     
     # add user to the db and start a single user server
     name = 'river'
@@ -109,12 +75,13 @@ def test_external_proxy(request, io_loop):
     
     routes = io_loop.run_sync(app.proxy.get_all_routes)
     # sets the desired path result
-    user_path = ujoin(app.base_url, 'user/river')
+    user_path = ujoin(app.base_url, 'user/river') + '/'
+    print(app.base_url, user_path)
     host = ''
     if app.subdomain_host:
         host = '%s.%s' % (name, urlparse(app.subdomain_host).hostname)
-    user_spec = RouteSpec(user_path, host=host)
+    user_spec = host + user_path
-    assert sorted(routes.keys()) == [RouteSpec('/'), user_spec]
+    assert sorted(routes.keys()) == ['/', user_spec]
     
     # teardown the proxy and start a new one in the same place
     proxy.terminate()
@@ -123,7 +90,7 @@ def test_external_proxy(request, io_loop):
 
     routes = io_loop.run_sync(app.proxy.get_all_routes)
 
-    assert list(routes.keys()) == [RouteSpec('/')]
+    assert list(routes.keys()) == ['/']
     
     # poke the server to update the proxy
     r = api_request(app, 'proxy', method='post')
@@ -131,7 +98,7 @@ def test_external_proxy(request, io_loop):
 
     # check that the routes are correct
     routes = io_loop.run_sync(app.proxy.get_all_routes)
-    assert sorted(routes.keys()) == [RouteSpec('/'), user_spec]
+    assert sorted(routes.keys()) == ['/', user_spec]
 
     # teardown the proxy, and start a new one with different auth and port
     proxy.terminate()
@@ -170,7 +137,7 @@ def test_external_proxy(request, io_loop):
 
     # check that the routes are correct
     routes = io_loop.run_sync(app.proxy.get_all_routes)
-    assert sorted(routes.keys()) == [RouteSpec('/'), user_spec]
+    assert sorted(routes.keys()) == ['/', user_spec]
 
 
 @pytest.mark.parametrize("username, endpoints", [
@@ -207,40 +174,54 @@ def test_check_routes(app, io_loop, username, endpoints):
     # check that before and after state are the same
     assert before == after
 
+from contextlib import contextmanager
 
-@pytest.mark.now
 @pytest.mark.gen_test
-@pytest.mark.parametrize("route_str", [
+@pytest.mark.parametrize("routespec", [
-    '/has%20space/foo',
+    '/has%20space/foo/',
-    'trailing/slash/',
+    '/missing-trailing/slash',
     '/has/@/',
-    'has/' + quote('üñîçø∂é'),
+    '/has/' + quote('üñîçø∂é'),
+    'host.name/path/',
+    'other.host/path/no/slash',
 ])
-@pytest.mark.parametrize("as_str", [
+def test_add_get_delete(app, routespec):
-    True,
+    arg = routespec
-    False,
+    if not routespec.endswith('/'):
-])
+        routespec = routespec + '/'
-def test_add_get_delete(app, route_str, as_str):
+    
-    if app.subdomain_host and as_str:
+    # host-routes when not host-routing raises an error
-        # can't use simple str args when host is not empty
+    # and vice versa
-        pytest.skip()
+    expect_value_error = bool(app.subdomain_host) ^ (not routespec.startswith('/'))
-    routespec = RouteSpec(route_str, host=urlparse(app.subdomain_host).hostname or '')
+    @contextmanager
+    def context():
+        if expect_value_error:
+            with pytest.raises(ValueError):
+                yield
+        else:
+            yield
+
     proxy = app.proxy
-    arg = route_str if as_str else routespec
     target = 'https://localhost:1234'
+    with context():
         yield proxy.add_route(arg, target=target)
     routes = yield proxy.get_all_routes()
+    if not expect_value_error:
         assert routespec in routes.keys()
+    with context():
         route = yield proxy.get_route(arg)
         assert route == {
             'target': target,
             'routespec': routespec,
             'data': route.get('data'),
         }
+    with context():
         yield proxy.delete_route(arg)
+    with context():
         route = yield proxy.get_route(arg)
         assert route is None
 
+
 @pytest.mark.parametrize("test_data", [None, 'notjson', json.dumps([])])
 def test_proxy_patch_bad_request_data(app, test_data):
     r = api_request(app, 'proxy', method='patch', data=test_data)

jupyterhub/user.py

@@ -15,7 +15,6 @@ from . import orm
 from .objects import Server
 from traitlets import HasTraits, Any, Dict, observe, default
 from .spawner import LocalProcessSpawner
-from .proxy import RouteSpec
 
 class UserDict(dict):
     """Like defaultdict, but for users
@@ -120,7 +119,7 @@ class User(HasTraits):
         self.allow_named_servers = self.settings.get('allow_named_servers', False)
 
         self.base_url = url_path_join(
-            self.settings.get('base_url', '/'), 'user', self.escaped_name)
+            self.settings.get('base_url', '/'), 'user', self.escaped_name) + '/'
 
         self.spawner = self.spawner_class(
             user=self,
@@ -171,9 +170,9 @@ class User(HasTraits):
     @property
     def proxy_spec(self):
         if self.settings.get('subdomain_host'):
-            return RouteSpec(path=self.base_url, host=self.domain)
+            return self.domain + self.base_url
         else:
-            return RouteSpec(path=self.base_url)
+            return self.base_url
 
     @property
     def domain(self):
@@ -223,7 +222,7 @@ class User(HasTraits):
                 server_name = options['server_name']
             else:
                 server_name = default_server_name(self)
-            base_url = url_path_join(self.base_url, server_name)
+            base_url = url_path_join(self.base_url, server_name) + '/'
         else:
             server_name = ''
             base_url = self.base_url