Refactor for clarity and document Das Uber Url_for

ckan/lib/helpers.py

@@ -178,76 +178,141 @@ def get_site_protocol_and_host():
 
 @core_helper
 def url_for(*args, **kw):
-    '''Return the URL for the given controller, action, id, etc.
+    '''Return the URL for an endpoint given some parameters.
 
-    Usage::
+     This is a wrapper for :py:func:`flask.url_for` and
+    :py:func:`routes.url_for` that adds some extra features that CKAN needs.
 
-        import ckan.plugins.toolkit as toolkit
+    To build a URL for a Flask view, pass the name of the blueprint and the
+    view function separated by a period ``.``, plus any URL parameters::
+
+        url_for('api.action', ver=3, logic_function='status_show')
+        # Returns /api/3/action/status_show
+
+    For a fully qualified URL pass the ``_external=True`` parameter. This
+    takes the ``ckan.site_url`` and ``ckan.root_path`` settings into account.
+
+        url_for('api.action', ver=3, logic_function='status_show',
+                _external=True)
+        # Returns http://example.com/api/3/action/status_show
 
-        url = toolkit.url_for(controller='package', action='read',
-                              id='my_dataset')
-        => returns '/dataset/my_dataset'
+    URLs built by Pylons use the Routes syntax::
+
+        url_for(controller='package', action='read', id='my_dataset')
+        # Returns '/dataset/my_dataset'
 
     Or, using a named route::
 
-        toolkit.url_for('dataset_read', id='changed')
+        url_for('dataset_read', id='changed')
+        # Returns '/dataset/changed'
 
-    This is a wrapper for :py:func:`routes.url_for` that adds some extra
-    features that CKAN needs.
+    Use ``qualified=True`` for a fully qualified URL when targeting a Pylons
+    endpoint.
 
+    For backwards compatibility, an effort is made to support the Pylons syntax
+    when building a Flask URL, but this support might be dropped in the future,
+    so calls should be updated.
     '''
+    # Get the actual string code for the locale
     locale = kw.pop('locale', None)
     if locale and isinstance(locale, i18n.Locale):
         locale = i18n.get_identifier_from_locale_class(locale)
+
     # remove __ckan_no_root and add after to not pollute url
     no_root = kw.pop('__ckan_no_root', False)
-    # routes will get the wrong url for APIs if the ver is not provided
+
+    # All API URLs generated should provide the version number
     if kw.get('controller') == 'api' or args and args[0].startswith('api.'):
         ver = kw.get('ver')
         if not ver:
-            raise Exception('api calls must specify the version! e.g. ver=3')
+            raise Exception('API URLs must specify the version (eg ver=3)')
+
+    try:
+        # First try to build the URL with the Flask router, making a copy of
+        # the params in case they are modified
+        flask_args = tuple(args)
+        flask_kw = kw.copy()
+
+        my_url = _url_for_flask(*flask_args, **flask_kw)
+
+    except FlaskRouteBuildError:
+        # If it doesn't succeed, fallback to the Pylons router, using the
+        # original parameters
+        my_url = _url_for_pylons(*args, **kw)
+
+    # Add back internal params
+    kw['__ckan_no_root'] = no_root
+
+    # Rewrite the URL to take the locale and root_path into account
+    return _local_url(my_url, locale=locale, **kw)
 
-    if kw.get('qualified', False) or kw.get('_external', False):
-        kw['protocol'], kw['host'] = get_site_protocol_and_host()
 
-    original_args = tuple(args)
-    original_kw = kw.copy()
+def _url_for_flask(*args, **kw):
+    '''Build a URL using the Flask router
+
+    This function should not be called directly, use ``url_for`` instead
+
+    This function tries to support the Pylons syntax for ``url_for`` and adapt
+    it to the Flask one, eg::
+
+        # Pylons
+        url_for(controller='api', action='action', ver=3, qualified=True)
+
+        # Flask
+        url_for('api.action', ver=3, _external=True)
+
+
+    Raises :py:exception:`werkzeug.routing.BuildError` if it couldn't
+    generate a URL.
+    '''
 
     if (len(args) and '_' in args[0]
             and '.' not in args[0]
             and not args[0].startswith('/')):
+        # Try to translate Python named routes to Flask endpoints
+        # eg `dataset_new` -> `dataset.new`
         args = (args[0].replace('_', '.', 1), )
     elif kw.get('controller') and kw.get('action'):
+        # If `controller` and `action` are passed, build a Flask endpoint
+        # from them
+        # eg controller='user', action='login' -> 'user.login'
         args = ('{0}.{1}'.format(kw.pop('controller'), kw.pop('action')),)
 
+    # Support Pylons' way of asking for full URLs
     if kw.pop('qualified', False):
         kw['_external'] = True
 
+    # The API routes used to require a slash on the version number, make sure
+    # we remove it
     if (args[0].startswith('api.') and
             isinstance(kw.get('ver'), basestring) and
             kw['ver'].startswith('/')):
         kw['ver'] = kw['ver'].replace('/', '')
 
-    try:
-        kw.pop('host', None)
-        kw.pop('protocol', None)
+    # Try to build the URL with flask.url_for
+    return _flask_default_url_for(*args, **kw)
 
-        my_url = _flask_default_url_for(*args, **kw)
 
-    except FlaskRouteBuildError:
-        if original_kw.get('controller') == 'api' and original_kw.get('ver'):
-            if (isinstance(original_kw['ver'], int) or
-                    not original_kw['ver'].startswith('/')):
-                # fix ver to include the slash
-                original_kw['ver'] = '/%s' % ver
+def _url_for_pylons(*args, **kw):
+    '''Build a URL using the Pylons (Routes) router
 
-        my_url = _routes_default_url_for(*original_args, **original_kw)
+    This function should not be called directly, use ``url_for`` instead
+    '''
 
-    if kw.get('_external', False) and 'qualified' not in original_kw:
-        original_kw['qualified'] = True
+    # We need to provide protocol and host to get full URLs, get them from
+    # ckan.site_url
+    if kw.get('qualified', False) or kw.get('_external', False):
+        kw['protocol'], kw['host'] = get_site_protocol_and_host()
 
-    original_kw['__ckan_no_root'] = no_root
-    return _local_url(my_url, locale=locale, **original_kw)
+    # The Pylons API routes require a slask on the version number for some
+    # reason
+    if kw.get('controller') == 'api' and kw.get('ver'):
+        if (isinstance(kw['ver'], int) or
+                not kw['ver'].startswith('/')):
+            kw['ver'] = '/%s' % kw['ver']
+
+    # Try to build the URL with routes.url_for
+    return _routes_default_url_for(*args, **kw)
 
 
 @core_helper
@@ -332,7 +397,7 @@ def _local_url(url_to_amend, **kw):
             default_locale = True
 
     root = ''
-    if kw.get('qualified', False):
+    if kw.get('qualified', False) or kw.get('_external', False):
         # if qualified is given we want the full url ie http://...
         protocol, host = get_site_protocol_and_host()
         # TODO: Use the Flask router once the home controller is migrated to a