Repository: hasgeek/coaster

Issue:
Turn url_for from a callable to a dynamic dictionary
It should be possible to get URLs in all these ways:

1. [x] `obj.url_for()` / `obj.url_for('edit')` (current)
2. [x] ~`obj.url_for['view']`~ `obj.urls['view']` (new)
3. [x] ~`dict(obj.url_for)`~ `dict(obj.urls)` to get all URLs
4. [x] ~`obj.current_access()['url_for']['view']`~ `obj.current_access()['urls']['view']` for JSON serialisation. Ideally, this should limit the listed URLs to the ones that the current role has access to, except we don't define roles when declaring view endpoints. That will require retrofitting.
5. [ ] `obj.current_access(profiles=('first', 'second'))` to limit JSON serialisation to the views defined in the 'first' profile. Once again, there is no definition for views available in a given profile.

Discussion:

Patch:
diff --git a/coaster/sqlalchemy/mixins.py b/coaster/sqlalchemy/mixins.py
index d2858c7..cfcbae8 100644
--- a/coaster/sqlalchemy/mixins.py
+++ b/coaster/sqlalchemy/mixins.py
@@ -22,7 +22,9 @@ Mixin classes must always appear *before* ``db.Model`` in your model's base clas
 
 from __future__ import absolute_import
 import six
+import six.moves.collections_abc as abc
 
+from collections import namedtuple
 import uuid as uuid_
 
 from sqlalchemy import (
@@ -71,6 +73,7 @@ __all__ = [
     'IdMixin',
     'TimestampMixin',
     'PermissionMixin',
+    'UrlDict',
     'UrlForMixin',
     'NoIdMixin',
     'BaseMixin',
@@ -276,34 +279,94 @@ class PermissionMixin(object):
         )
 
 
+UrlEndpointData = namedtuple(
+    'UrlEndpointData',
+    ['endpoint', 'paramattrs', 'external', 'roles', 'requires_kwargs'],
+)
+
+
+class UrlDictStub(object):
+    """
+    Dictionary-based access to URLs for a model instance, used by :class:`UrlForMixin`.
+    Proxies to :meth:`UrlForMixin.url_for` for keyword-based lookup. Uses
+    :attr:`UrlForMixin.url_for_endpoints` for enumeration, but with URLs limited to
+    those available under current roles.
+    """
+
+    def __get__(self, obj, cls=None):
+        if obj is None:
+            return self  # pragma: no cover
+        return UrlDict(obj)
+
+
+class UrlDict(abc.Mapping):
+    """
+    Provides dictionary access to an object's URLs.
+    """
+
+    def __init__(self, obj):
+        self.obj = obj
+
+    def __getitem__(self, key):
+        try:
+            return self.obj.url_for(key, _external=True)
+        except BuildError:
+            raise KeyError(key)
+
+    def __len__(self):
+        return len(self.obj.url_for_endpoints[None]) + (
+            len(self.obj.url_for_endpoints.get(current_app._get_current_object(), {}))
+            if current_app
+            else 0
+        )
+
+    def __iter__(self):
+        # 1. Iterate through all actions available to the None app and to current_app
+        # 2. If the action requires specific roles, confirm overlap with current_roles
+        # 3. Confirm the action does not require additional parameters
+        # 4. Yield whatever passes the tests
+        current_roles = self.obj.current_roles
+        for app, app_actions in self.obj.url_for_endpoints.items():
+            if app is None or (
+                current_app and app is current_app._get_current_object()
+            ):
+                for action, epdata in app_actions.items():
+                    if not epdata.requires_kwargs and (
+                        epdata.roles is None or epdata.roles.intersection(current_roles)
+                    ):
+                        yield action
+
+
 class UrlForMixin(object):
     """
     Provides a :meth:`url_for` method used by BaseMixin-derived classes
     """
 
-    #: Mapping of {app: {action: (endpoint, {param: attr}, _external)}},
-    #: where attr is a string or tuple of strings. The same action can point to
-    #: different endpoints in different apps. The app may also be None as fallback.
-    #: Each subclass will get its own dictionary. This particular dictionary is only
-    #: used as an inherited fallback.
+    #: Mapping of {app: {action: UrlEndpointData}}, where attr is a string or tuple of
+    #: strings. The same action can point to different endpoints in different apps. The
+    #: app may also be None as fallback. Each subclass will get its own dictionary.
+    #: This particular dictionary is only used as an inherited fallback.
     url_for_endpoints = {None: {}}
     #: Mapping of {app: {action: (classview, attr)}}
     view_for_endpoints = {}
 
+    #: Dictionary of URLs available on this object
+    urls = UrlDictStub()
+
     def url_for(self, action='view', **kwargs):
         """
         Return public URL to this instance for a given action (default 'view')
         """
         app = current_app._get_current_object() if current_app else None
         if app is not None and action in self.url_for_endpoints.get(app, {}):
-            endpoint, paramattrs, _external = self.url_for_endpoints[app][action]
+            epdata = self.url_for_endpoints[app][action]
         else:
             try:
-                endpoint, paramattrs, _external = self.url_for_endpoints[None][action]
+                epdata = self.url_for_endpoints[None][action]
             except KeyError:
                 raise BuildError(action, kwargs, 'GET')
         params = {}
-        for param, attr in list(paramattrs.items()):
+        for param, attr in list(epdata.paramattrs.items()):
             if isinstance(attr, tuple):
                 # attr is a tuple containing:
                 # 1. ('parent', 'name') --> self.parent.name
@@ -320,12 +383,12 @@ class UrlForMixin(object):
                 params[param] = attr(self)
             else:
                 params[param] = getattr(self, attr)
-        if _external is not None:
-            params['_external'] = _external
+        if epdata.external is not None:
+            params['_external'] = epdata.external
         params.update(kwargs)  # Let kwargs override params
 
         # url_for from flask
-        return url_for(endpoint, **params)
+        return url_for(epdata.endpoint, **params)
 
     @property
     def absolute_url(self):
@@ -340,30 +403,67 @@ class UrlForMixin(object):
     ):
         """
         View decorator that registers the view as a :meth:`url_for` target.
+
+        :param str _action: Action to register a URL under
+        :param str _endpoint: View endpoint name to pass to Flask's ``url_for``
+        :param _app: The app to register this action on (if your repo has multiple apps)
+        :param _external: If `True`, URLs are assumed to be external-facing by default
+        :param dict paramattrs: Mapping of URL parameter to attribute on the object
         """
 
         def decorator(f):
-            if 'url_for_endpoints' not in cls.__dict__:
-                cls.url_for_endpoints = {
-                    None: {}
-                }  # Stick it into the class with the first endpoint
-            cls.url_for_endpoints.setdefault(_app, {})
-
-            for keyword in paramattrs:
-                if (
-                    isinstance(paramattrs[keyword], six.string_types)
-                    and '.' in paramattrs[keyword]
-                ):
-                    paramattrs[keyword] = tuple(paramattrs[keyword].split('.'))
-            cls.url_for_endpoints[_app][_action] = (
-                _endpoint or f.__name__,
-                paramattrs,
-                _external,
+            cls.register_endpoint(
+                action=_action,
+                endpoint=_endpoint or f.__name__,
+                app=_app,
+                external=_external,
+                paramattrs=paramattrs,
             )
             return f
 
         return decorator
 
+    @classmethod
+    def register_endpoint(
+        cls, action, endpoint, app=None, external=None, roles=None, paramattrs=None
+    ):
+        """
+        Helper method for registering an endopint to a :meth:`url_for` action.
+
+        :param view_func: View handler to be registered
+        :param str action: Action to register a URL under
+        :param str endpoint: View endpoint name to pass to Flask's ``url_for``
+        :param app: Flask app (default: `None`)
+        :param external: If `True`, URLs are assumed to be external-facing by default
+        :param roles: Roles to which this URL is available, required by :class:`UrlDict`
+        :param dict paramattrs: Mapping of URL parameter to attribute on the object
+        """
+
+        if 'url_for_endpoints' not in cls.__dict__:
+            cls.url_for_endpoints = {
+                None: {}
+            }  # Stick it into the class with the first endpoint
+        cls.url_for_endpoints.setdefault(app, {})
+
+        for keyword in paramattrs:
+            if (
+                isinstance(paramattrs[keyword], six.string_types)
+                and '.' in paramattrs[keyword]
+            ):
+                paramattrs[keyword] = tuple(paramattrs[keyword].split('.'))
+        requires_kwargs = False
+        for attrs in paramattrs.values():
+            if isinstance(attrs, tuple) and attrs[0].startswith('**'):
+                requires_kwargs = True
+                break
+        cls.url_for_endpoints[app][action] = UrlEndpointData(
+            endpoint=endpoint,
+            paramattrs=paramattrs if paramattrs is not None else {},
+            external=external,
+            roles=roles,
+            requires_kwargs=requires_kwargs,
+        )
+
     @classmethod
     def register_view_for(cls, app, action, classview, attr):
         """
diff --git a/coaster/views/classview.py b/coaster/views/classview.py
index a5de3b5..39af1c9 100644
--- a/coaster/views/classview.py
+++ b/coaster/views/classview.py
@@ -669,7 +669,7 @@ class UrlForView(object):
             params = {
                 v: cls.route_model_map[v] for v in rulevars if v in cls.route_model_map
             }
-            # Hook up is_url_for with the view function's name, endpoint name and
+            # Register endpoint with the view function's name, endpoint name and
             # parameters. Register the view for a specific app, unless we're in a
             # Blueprint, in which case it's not an app.
             # FIXME: The behaviour of a Blueprint + multi-app combo is unknown and needs
@@ -680,9 +680,13 @@ class UrlForView(object):
             else:
                 prefix = ''
                 reg_app = app
-            cls.model.is_url_for(
-                view_func.__name__, prefix + endpoint, _app=reg_app, **params
-            )(view_func)
+            cls.model.register_endpoint(
+                action=view_func.__name__,
+                endpoint=prefix + endpoint,
+                app=reg_app,
+                roles=getattr(view_func, 'requires_roles', None),
+                paramattrs=params,
+            )
             cls.model.register_view_for(
                 app=reg_app,
                 action=view_func.__name__,


Test patch:
diff --git a/tests/test_annotations.py b/tests/test_sqlalchemy_annotations.py
similarity index 100%
rename from tests/test_annotations.py
rename to tests/test_sqlalchemy_annotations.py
diff --git a/tests/test_sqlalchemy_coordinates.py b/tests/test_sqlalchemy_coordinates.py
index 50c145a..a3d8bde 100644
--- a/tests/test_sqlalchemy_coordinates.py
+++ b/tests/test_sqlalchemy_coordinates.py
@@ -7,7 +7,7 @@ import unittest
 from coaster.db import db
 from coaster.sqlalchemy import BaseMixin, CoordinatesMixin
 
-from .test_models import app2
+from .test_sqlalchemy_models import app2
 
 
 class CoordinatesData(BaseMixin, CoordinatesMixin, db.Model):
diff --git a/tests/test_markdowncolumn.py b/tests/test_sqlalchemy_markdowncolumn.py
similarity index 98%
rename from tests/test_markdowncolumn.py
rename to tests/test_sqlalchemy_markdowncolumn.py
index cc1f41a..0af5f25 100644
--- a/tests/test_markdowncolumn.py
+++ b/tests/test_sqlalchemy_markdowncolumn.py
@@ -8,7 +8,7 @@ from coaster.db import db
 from coaster.gfm import markdown
 from coaster.sqlalchemy import BaseMixin, MarkdownColumn
 
-from .test_models import app1, app2
+from .test_sqlalchemy_models import app1, app2
 
 
 class MarkdownData(BaseMixin, db.Model):
diff --git a/tests/test_models.py b/tests/test_sqlalchemy_models.py
similarity index 100%
rename from tests/test_models.py
rename to tests/test_sqlalchemy_models.py
diff --git a/tests/test_roles.py b/tests/test_sqlalchemy_roles.py
similarity index 100%
rename from tests/test_roles.py
rename to tests/test_sqlalchemy_roles.py
diff --git a/tests/test_url_for.py b/tests/test_url_for.py
index 0f3b916..ffdc3e2 100644
--- a/tests/test_url_for.py
+++ b/tests/test_url_for.py
@@ -9,7 +9,7 @@ from werkzeug.routing import BuildError
 
 from coaster.db import db
 
-from .test_models import Container, NamedDocument, ScopedNamedDocument
+from .test_sqlalchemy_models import Container, NamedDocument, ScopedNamedDocument
 
 # --- Test setup --------------------------------------------------------------
 
@@ -42,7 +42,7 @@ def doc_upper(doc):
     return u'{} {}'.format('upper', doc)
 
 
-# The unusual parameter `other='@other.name'` requires an explanation.
+# The unusual parameter `other='**other.name'` requires an explanation.
 # The first `other` refers to `<other>` in the URL. The second refers
 # to the parameter given to `NamedDocument.url_for` in the test below.
 @app1.route('/<doc>/with/<other>')
@@ -202,6 +202,29 @@ class TestUrlFor(TestUrlForBase):
         # url_for is given an object and extracts an attribute from it
         assert doc1.url_for('with', other=doc2) == '/document1/with/document2'
 
+    def test_url_dict(self):
+        """URLs to views are available from a .urls dictionary-like object."""
+        doc1 = NamedDocument(name=u'document1', title=u"Document")
+        self.session.add(doc1)
+        self.session.commit()
+
+        assert doc1.urls != {}
+        assert doc1.urls['view'] == 'http://localhost/document1'
+        with self.assertRaises(KeyError):
+            doc1.urls['random']
+
+        # The len() count includes the doc_with view, but it is excluded from actual
+        # enumeration because it requires additional keyword parameters, which cannot
+        # be passed in with dictionary access.
+        assert len(doc1.urls) == 6
+        assert dict(doc1.urls) == {
+            'app_only': 'http://localhost/document1/app_only',
+            'edit': 'http://localhost/document1/edit',
+            'per_app': 'http://localhost/document1/app1',
+            'upper': 'http://localhost/DOCUMENT1/upper',
+            'view': 'http://localhost/document1',
+        }
+
 
 class TestUrlFor2(TestUrlForBase):
     app = app2
@@ -224,3 +247,20 @@ class TestUrlFor2(TestUrlForBase):
         # This action is not available in this app
         with self.assertRaises(BuildError):
             doc1.url_for('app_only')
+
+    def test_url_dict_app_only(self):
+        """The urls dictionary only includes available URLs."""
+        doc1 = NamedDocument(name=u'document1', title=u"Document 1")
+        self.session.add(doc1)
+        self.session.commit()
+
+        assert 'app_only' not in doc1.urls
+        assert 'per_app' in doc1.urls
+        assert doc1.urls['per_app'] == 'http://localhost/document1/app2'
+
+        # Note: attempting to cast `urls` into a dict will break as there are
+        # URLs registered with `is_url_for` with no app specified (meaning they are
+        # supposedly available to all apps) but with no @app.route registration. This
+        # will cause BuildError while attempting to retrieve the value. However, casting
+        # to a list/set of just the keys will work.
+        assert set(doc1.urls) == {'edit', 'upper', 'per_app', 'view'}
diff --git a/tests/test_utils_markdown.py b/tests/test_utils_markdown.py
index 0d2c9b2..11c4984 100644
--- a/tests/test_utils_markdown.py
+++ b/tests/test_utils_markdown.py
@@ -70,7 +70,7 @@ It has a newline break here.</p>
 </ol>
 <p>A naked link: <a href="https://www.example.com/" rel="nofollow">https://www.example.com/</a></p>
 <p>An <a href="https://www.example.com/" rel="nofollow">inline link</a></p>
-<div class="codehilite"><pre><span></span><code><span class="hll"><span class="k">def</span> <span class="nf">foo</span><span class="p">():</span>
+<div class="highlight"><pre><span></span><code><span class="hll"><span class="k">def</span> <span class="nf">foo</span><span class="p">():</span>
 </span>    <span class="k">return</span> <span class="s1">'https://www.example.com/'</span>
 </code></pre></div>
 
@@ -189,11 +189,13 @@ if six.PY2:
         sample_output.replace('<pre><code>', '<pre>')
         .replace('</code></pre>', '</pre>')
         .replace('<pre><span></span><code>', '<pre><span></span>')
+        .replace('<div class="highlight">', '<div class="codehilite">')
     )
     sample_output_html = (
         sample_output_html.replace('<pre><code>', '<pre>')
         .replace('</code></pre>', '</pre>')
         .replace('<pre><span></span><code>', '<pre><span></span>')
+        .replace('<div class="highlight">', '<div class="codehilite">')
     )
 
 
diff --git a/tests/test_views_classview.py b/tests/test_views_classview.py
index a0bc53f..83602a7 100644
--- a/tests/test_views_classview.py
+++ b/tests/test_views_classview.py
@@ -1,6 +1,7 @@
 # -*- coding: utf-8 -*-
 
 from __future__ import absolute_import, unicode_literals
+import six.moves.collections_abc as abc
 
 import unittest
 
@@ -27,7 +28,16 @@ from coaster.views import (
     viewdata,
 )
 
+
+class JsonEncoder(json.JSONEncoder):
+    def default(self, o):
+        if isinstance(o, abc.Mapping):
+            return dict(o)
+        return super(JsonEncoder, self).default(o)
+
+
 app = Flask(__name__)
+app.json_encoder = JsonEncoder
 app.testing = True
 app.config['SQLALCHEMY_DATABASE_URI'] = 'sqlite://'
 app.config['SQLALCHEMY_TRACK_MODIFICATIONS'] = False
@@ -750,3 +760,23 @@ class TestClassView(unittest.TestCase):
         # safety checks, and will need amending then.
         assert ModelDocumentView(obj=doc1) != ScopedDocumentView(obj=doc1)
         assert ScopedDocumentView(obj=doc1) != ModelDocumentView(obj=doc1)
+
+    def test_url_dict_current_roles(self):
+        doc1 = ViewDocument(name='test1', title="Test 1")
+        assert set(doc1.urls) == {
+            'view',  # From ModelDocumentView
+            'edit',  # From ModelDocumentView
+            'by_perm',  # From GatedDocumentView (`urls` can't handle permission gating)
+        }
+        # Adding acess permissions changes the URLs available
+        add_auth_attribute('user', 'this-is-the-owner')  # See ViewDocument.permissions
+        assert set(doc1.urls) == {
+            # From ModelDocumentView
+            'view',
+            'edit',
+            # From GatedDocumentView
+            'by_perm',
+            'by_role_perm',
+            'by_perm_role',
+            'by_role',
+        }
diff --git a/tests/test_views_loadmodels.py b/tests/test_views_loadmodels.py
index 2b05783..2f4902c 100644
--- a/tests/test_views_loadmodels.py
+++ b/tests/test_views_loadmodels.py
@@ -14,7 +14,7 @@ from coaster.db import db
 from coaster.sqlalchemy import BaseMixin, BaseNameMixin, BaseScopedIdMixin
 from coaster.views import load_model, load_models
 
-from .test_models import (
+from .test_sqlalchemy_models import (
     Container,
     IdNamedDocument,
     NamedDocument,