Repository: hasgeek/coaster

Issue:
UrlForMixin should register views on a pre-app basis
In Funnel we have two apps with different URL structures. `UrlForMixin`'s `url_for` should know which app it is building URLs for and what attributes go into it.

Discussion:

Patch:
diff --git a/CHANGES.rst b/CHANGES.rst
index 37a6299..c8cd133 100644
--- a/CHANGES.rst
+++ b/CHANGES.rst
@@ -27,6 +27,8 @@
 * Changed: ``UuidMixin`` no longer provides ``url_id``
 * New: ``coaster.sqlalchemy.UrlForMixin`` now provides an ``absolute_url``
   property
+* Changed: ``coaster.sqlalchemy.UrlForMixin`` now recognises that the project
+  may have multiple apps with distinct URLs for the same content
 
 
 0.6.0
diff --git a/coaster/sqlalchemy/mixins.py b/coaster/sqlalchemy/mixins.py
index 8eed61a..98809d5 100644
--- a/coaster/sqlalchemy/mixins.py
+++ b/coaster/sqlalchemy/mixins.py
@@ -29,7 +29,8 @@ from sqlalchemy.ext.declarative import declared_attr
 from sqlalchemy.ext.hybrid import hybrid_property
 from sqlalchemy.orm import synonym
 from sqlalchemy_utils.types import UUIDType
-from flask import url_for
+from werkzeug.routing import BuildError
+from flask import current_app, url_for
 import six
 from ..utils import make_name, uuid2suuid, uuid2buid, buid2uuid, suuid2uuid, InspectableSet
 from ..utils.misc import _punctuation_re
@@ -218,18 +219,23 @@ class UrlForMixin(object):
     """
     Provides a :meth:`url_for` method used by BaseMixin-derived classes
     """
-    #: Mapping of {action: (endpoint, {param: attr})}, where attr is a string or tuple of strings.
-    #: This particular dictionary is only used as a fallback. Each subclass will get its own dictionary.
-    url_for_endpoints = {}
+    #: Mapping of {app: {action: (endpoint, {param: attr})}}, 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: {}}
 
     def url_for(self, action='view', **kwargs):
         """
         Return public URL to this instance for a given action (default 'view')
         """
-        if action not in self.url_for_endpoints:
-            # FIXME: Legacy behaviour, fails silently, but shouldn't. url_for itself raises a BuildError
-            return
-        endpoint, paramattrs, _external = self.url_for_endpoints[action]
+        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]
+        else:
+            try:
+                endpoint, paramattrs, _external = self.url_for_endpoints[None][action]
+            except KeyError:
+                raise BuildError(action, kwargs, 'GET')
         params = {}
         for param, attr in list(paramattrs.items()):
             if isinstance(attr, tuple):
@@ -254,18 +260,19 @@ class UrlForMixin(object):
         return self.url_for(_external=True)
 
     @classmethod
-    def is_url_for(cls, _action, _endpoint=None, _external=None, **paramattrs):
+    def is_url_for(cls, _action, _endpoint=None, _app=None, _external=None, **paramattrs):
         """
         View decorator that registers the view as a :meth:`url_for` target.
         """
         def decorator(f):
             if 'url_for_endpoints' not in cls.__dict__:
-                cls.url_for_endpoints = {}  # Stick it into the class with the first endpoint
+                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[_action] = _endpoint or f.__name__, paramattrs, _external
+            cls.url_for_endpoints[_app][_action] = _endpoint or f.__name__, paramattrs, _external
             return f
         return decorator
 
diff --git a/coaster/views/classview.py b/coaster/views/classview.py
index ff56f4d..a43794c 100644
--- a/coaster/views/classview.py
+++ b/coaster/views/classview.py
@@ -483,9 +483,17 @@ class UrlForView(object):
             rulevars = (v for c, a, v in parse_rule(rule))
             # Make a subset of cls.route_model_map with the required variables
             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 parameters
-            prefix = app.name + '.' if isinstance(app, Blueprint) else ''
-            cls.model.is_url_for(view_func.__name__, prefix + endpoint, **params)(view_func)
+            # Hook up is_url_for 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 tests.
+            if isinstance(app, Blueprint):
+                prefix = app.name + '.'
+                reg_app = None
+            else:
+                prefix = ''
+                reg_app = app
+            cls.model.is_url_for(view_func.__name__, prefix + endpoint, _app=reg_app, **params)(view_func)
             if callback:  # pragma: no cover
                 callback(rule, endpoint, view_func, **options)
 


Test patch:
diff --git a/tests/test_models.py b/tests/test_models.py
index c7ebcf5..0305457 100644
--- a/tests/test_models.py
+++ b/tests/test_models.py
@@ -13,6 +13,7 @@ from sqlalchemy import Column, Integer, Unicode, UniqueConstraint, ForeignKey, f
 from sqlalchemy.orm import relationship, synonym
 from sqlalchemy.exc import IntegrityError
 from sqlalchemy.orm.exc import MultipleResultsFound
+from werkzeug.routing import BuildError
 from coaster.sqlalchemy import (BaseMixin, BaseNameMixin, BaseScopedNameMixin,
     BaseIdNameMixin, BaseScopedIdMixin, BaseScopedIdNameMixin, JsonDict, failsafe_add,
     UuidMixin, UUIDType, add_primary_relationship, auto_init_default)
@@ -530,7 +531,8 @@ class TestCoasterModels(unittest.TestCase):
 
     def test_url_for_fail(self):
         d = UnnamedDocument(content="hello")
-        self.assertEqual(d.url_for(), None)
+        with self.assertRaises(BuildError):
+            d.url_for()
 
     def test_jsondict(self):
         m1 = MyData(data={'value': 'foo'})
diff --git a/tests/test_url_for.py b/tests/test_url_for.py
index 5ed400b..aefa257 100644
--- a/tests/test_url_for.py
+++ b/tests/test_url_for.py
@@ -3,6 +3,7 @@
 from __future__ import absolute_import
 
 import unittest
+from werkzeug.routing import BuildError
 from flask import Flask
 from coaster.db import db
 
@@ -10,47 +11,72 @@ from .test_models import Container, NamedDocument, ScopedNamedDocument
 
 # --- Test setup --------------------------------------------------------------
 
-app = Flask(__name__)
-app.config['SQLALCHEMY_DATABASE_URI'] = 'sqlite://'
-app.config['SQLALCHEMY_TRACK_MODIFICATIONS'] = False
-db.init_app(app)
+app1 = Flask(__name__)
+app1.config['SQLALCHEMY_DATABASE_URI'] = 'sqlite://'
+app1.config['SQLALCHEMY_TRACK_MODIFICATIONS'] = False
+db.init_app(app1)
 
+app2 = Flask(__name__)
+app2.config['SQLALCHEMY_DATABASE_URI'] = 'sqlite://'
+app2.config['SQLALCHEMY_TRACK_MODIFICATIONS'] = False
+db.init_app(app2)
 
-@app.route('/<doc>')
+
+@app1.route('/<doc>')
 @NamedDocument.is_url_for('view', doc='name')
 def doc_view(doc):
     return u'{} {}'.format('view', doc)
 
 
-@app.route('/<doc>/edit')
+@app1.route('/<doc>/edit')
 @NamedDocument.is_url_for('edit', doc='name')
 def doc_edit(doc):
     return u'{} {}'.format('edit', doc)
 
 
-@app.route('/<doc>/upper')
+@app1.route('/<doc>/upper')
 @NamedDocument.is_url_for('upper', doc=lambda d: d.name.upper())
 def doc_upper(doc):
     return u'{} {}'.format('upper', doc)
 
 
-@app.route('/<container>/<doc>')
+@app1.route('/<container>/<doc>')
 @ScopedNamedDocument.is_url_for('view', container='parent.id', doc='name')
 def sdoc_view(container, doc):
     return u'{} {} {}'.format('view', container, doc)
 
 
-@app.route('/<container>/<doc>/edit')
+@app1.route('/<container>/<doc>/edit')
 @ScopedNamedDocument.is_url_for('edit', _external=True, container=('parent', 'id'), doc='name')
 def sdoc_edit(container, doc):
     return u'{} {} {}'.format('edit', container, doc)
 
 
+@app1.route('/<doc>/app_only')
+@NamedDocument.is_url_for('app_only', _app=app1, doc='name')
+def doc_app_only(doc):
+    return u'{} {}'.format('app_only', doc)
+
+
+@app1.route('/<doc>/app1')
+@NamedDocument.is_url_for('per_app', _app=app1, doc='name')
+def doc_per_app1(doc):
+    return u'{} {}'.format('per_app', doc)
+
+
+@app2.route('/<doc>/app2')
+@NamedDocument.is_url_for('per_app', _app=app2, doc='name')
+def doc_per_app2(doc):
+    return u'{} {}'.format('per_app', doc)
+
+
 # --- Tests -------------------------------------------------------------------
 
-class TestUrlFor(unittest.TestCase):
+class TestUrlForBase(unittest.TestCase):
+    app = app1
+
     def setUp(self):
-        self.ctx = app.test_request_context()
+        self.ctx = self.app.test_request_context()
         self.ctx.push()
         db.create_all()
         self.session = db.session
@@ -60,11 +86,13 @@ class TestUrlFor(unittest.TestCase):
         db.drop_all()
         self.ctx.pop()
 
+
+class TestUrlFor(TestUrlForBase):
     def test_class_has_url_for(self):
         """
         Test that is_url_for declarations on one class are distinct from those on another class.
         """
-        self.assertNotEqual(NamedDocument.url_for_endpoints, ScopedNamedDocument.url_for_endpoints)
+        assert NamedDocument.url_for_endpoints is not ScopedNamedDocument.url_for_endpoints
 
     def test_url_for(self):
         """
@@ -80,24 +108,24 @@ class TestUrlFor(unittest.TestCase):
         self.session.commit()
 
         # Confirm first returns the correct paths
-        self.assertEqual(doc1.url_for(), '/document1')
-        self.assertEqual(doc1.url_for('view'), '/document1')
-        self.assertEqual(doc1.url_for('edit'), '/document1/edit')
+        assert doc1.url_for() == '/document1'
+        assert doc1.url_for('view') == '/document1'
+        assert doc1.url_for('edit') == '/document1/edit'
         # Test callable parameters
-        self.assertEqual(doc1.url_for('upper'), '/DOCUMENT1/upper')
+        assert doc1.url_for('upper') == '/DOCUMENT1/upper'
         # Insist on changing one of the parameters
-        self.assertEqual(doc1.url_for('edit', doc=doc1.name.upper()), '/DOCUMENT1/edit')
+        assert doc1.url_for('edit', doc=doc1.name.upper()) == '/DOCUMENT1/edit'
         # Confirm second returns the correct paths
-        self.assertEqual(doc2.url_for(), '/1/document2')
-        self.assertEqual(doc2.url_for('view'), '/1/document2')
+        assert doc2.url_for() == '/1/document2'
+        assert doc2.url_for('view') == '/1/document2'
         # Test _external flag
-        self.assertEqual(doc2.url_for('edit'), 'http://localhost/1/document2/edit')
-        self.assertEqual(doc2.url_for('edit', _external=False), '/1/document2/edit')
-        self.assertEqual(doc2.url_for('edit', _external=True), 'http://localhost/1/document2/edit')
+        assert doc2.url_for('edit') == 'http://localhost/1/document2/edit'
+        assert doc2.url_for('edit', _external=False) == '/1/document2/edit'
+        assert doc2.url_for('edit', _external=True) == 'http://localhost/1/document2/edit'
 
-    def test_url(self):
+    def test_absolute_url(self):
         """
-        The .url property is the same as .url_for(_external=True)
+        The .absolute_url property is the same as .url_for(_external=True)
         """
         # Make two documents
         doc1 = NamedDocument(name=u'document1', title=u"Document 1")
@@ -112,3 +140,44 @@ class TestUrlFor(unittest.TestCase):
         assert doc1.absolute_url != doc1.url_for(_external=False)
         assert doc2.absolute_url == doc2.url_for(_external=True)
         assert doc2.absolute_url != doc2.url_for(_external=False)
+
+    def test_per_app(self):
+        """Allow app-specific URLs for the same action name"""
+        doc1 = NamedDocument(name=u'document1', title=u"Document 1")
+        self.session.add(doc1)
+        self.session.commit()
+
+        # The action's URL is specific to the app
+        assert doc1.url_for('per_app') == '/document1/app1'
+
+    def test_app_only(self):
+        """Allow URLs to only be available in one app"""
+        doc1 = NamedDocument(name=u'document1', title=u"Document 1")
+        self.session.add(doc1)
+        self.session.commit()
+
+        # This action is only available in this app
+        assert doc1.url_for('app_only') == '/document1/app_only'
+
+
+class TestUrlFor2(TestUrlForBase):
+    app = app2
+
+    def test_per_app(self):
+        """Allow app-specific URLs for the same action name"""
+        doc1 = NamedDocument(name=u'document1', title=u"Document 1")
+        self.session.add(doc1)
+        self.session.commit()
+
+        # The action's URL is specific to the app
+        assert doc1.url_for('per_app') == '/document1/app2'
+
+    def test_app_only(self):
+        """Allow URLs to only be available in one app"""
+        doc1 = NamedDocument(name=u'document1', title=u"Document 1")
+        self.session.add(doc1)
+        self.session.commit()
+
+        # This action is not available in this app
+        with self.assertRaises(BuildError):
+            doc1.url_for('app_only')