#269 Add a way for plugins to register their own routes
Merged 7 years ago by abompard. Opened 7 years ago by abompard.
abompard/fedora-hubs plugin-views  into  develop

file modified
+37
@@ -381,6 +381,43 @@ 

  

  Destroy your database, rebuild it, and re-run the app.  Your widget should show up.

  

+ Widget-specific views

+ ---------------------

+ A widget may also register additional routes by using the

+ ``hubs.widgets.base.widget_route`` decorator. There are some differences from

+ the usual ``route`` decorator or the ``add_url_rule`` function:

+ 

+ - The view function will be passed the database ``session`` and ``widget``

+   instances as first arguments, and then the URL kwargs.

+ - The endpoint will be prefixed with ``<widget_name>_``, for example

+   ``meetings_``. Remember that when you want to reverse the URL with

+   ``url_for``.

+ - When reversing the URL, you need to pass the ``hub`` and ``idx`` kwargs.

+ 

+ Example: consider this additional method that needs to be exported as a view

+ in the ``meetings`` plugin::

+ 

+     from hubs.widgets.base import widget_route

+     @widget_route(rule="search/<requester>/", methods=["GET", "POST"])

+     def search(session, widget, requester):

+         # Do something, probably using the "requester" variable.

+         return flask.jsonify({"hope_sources": ["Obi-Wan Kenobi"]})

+ 

+ Then, a valid call to ``url_for`` to reverse this URL would look like::

+ 

+     url_for("meetings_search", hub=widget.hub.name, idx=widget.idx, requester="Leia")

+ 

+ Behind the scenes, the ``widget_route`` decorator adds a ``ROUTES`` global

+ variable in the widget module, that contains dictionaries representing the

+ arguments passed to Flask's `add_url_rule

+ <http://flask.pocoo.org/docs/latest/api/#flask.Flask.add_url_rule>`_ function.

+ 

+ If you want more control, you can edit this ``ROUTES`` list directly, but there

+ are a difference from the basic ``add_url_rule`` function: the view function

+ name must be given with the dict key ``view_func_name``, and not ``view_func``

+ as in the ``add_url_rule`` function.

+ 

+ 

  A proposal, client-side templates

  =================================

  

file modified
+32
@@ -797,3 +797,35 @@ 

          return flask.jsonify(req.json())

      else:

          return req.text, req.status_code

+ 

+ 

+ #

+ # Add widget-specific routes

+ #

+ 

+ def _widget_view_decorator(func):

+     """

+     This internal decorator will edit the view function arguments.

+ 

+     It will:

+     - remove the hub name and the widget primary key

+     - add the database session and the widget instance

+     """

+     @functools.wraps(func)

+     def inner(*args, **kwargs):

+         hubname = kwargs.pop("hub")

+         widgetidx = kwargs.pop("idx")

+         widget = get_widget(session, hubname, widgetidx)

+         return func(session, widget, *args, **kwargs)

+     return inner

+ 

+ for widget in session.query(hubs.models.Widget):

+     for params in getattr(widget.module, 'ROUTES', []):

+         params["rule"] = "/<hub>/<int:idx>/widget/" \

+                          + params["rule"].lstrip("/")

+         if not params.get("endpoint"):

+             params["endpoint"] = params["view_func_name"]

+         params["endpoint"] = "%s_%s" % (widget.plugin, params["endpoint"])

+         params["view_func"] = _widget_view_decorator(

+             getattr(widget.module, params.pop("view_func_name")))

+         app.add_url_rule(**params)

file modified
+17
@@ -3,6 +3,7 @@ 

  import functools

  import hashlib

  import json

+ import sys

  

  import dogpile.cache

  import flask
@@ -96,3 +97,19 @@ 

          subsequent.widget_arguments = getattr(original, 'widget_arguments', [])

          return subsequent

      return decorator

+ 

+ 

+ def widget_route(**options):

+     """Register a view for the current widget.

+ 

+     This decorator can be used to expose a specific function below a widget's

+     URL endpoint. Refer to the "Widget-specific views" section in the

+     documentation to learn how to construct the corresponding URL.

+     """

+     def decorator(func):

+         options["view_func_name"] = func.__name__

+         mdict = sys.modules[func.__module__].__dict__

+         mroutes = mdict.setdefault('ROUTES', [])

+         mroutes.append(options)

+         return func

+     return decorator

This commit adds a way for plugins to declare their own routes to Flask, should they need additional endpoints.

s/dictionnaries/dictionaries/

1 new commit added

  • Fix typo
7 years ago

1 new commit added

  • Switch to a decorator system for the widget routes
7 years ago

rebased

7 years ago

i am sorry but, @abompard wouldn't it be a little better if we had the doc strings for the functions? Please don't hate me for this. (you can consider me crazy, its fine since i like harley quinn)

I think these are an SQLAlchemy database session and an instance of hubs.models.Widget respectively, is that correct? It would be good if the documentation be explicit about it? I'm never sure when I see the word session if it means database session, request session, etc.

I moved the widget documentation to be a module-level docblock in https://pagure.io/fedora-hubs/pull-request/302#_5,23 and it's inserted into the Sphinx project and linked to in the dev-guide. Assuming people like that, after this gets merged I'll just rebase and move this block in with the rest of the documentation. I just wanted to let you know so you're not surprised if this moved in the near future :)

It would be good to add a docblock here so it shows up in auto-generated API documentation. I've been using the so-called Google style Sphinx supports since I find it the most readable, but we should probably just agree on one of the thee styles Sphinx supports (the others are plain RST and NumPy style) and be consistent. I'm happy to change my existing PRs to match.

Same note about adding a docblock as above

Having this execute at the bottom of this module makes me nervous. The problem I see in the future is some of the routes in this file get factored out (into one or more API modules, for example) and then import order can break whether or not the url rules are registered.

Long term I think the best thing to do is to make this app.py strictly about configuring the Flask app object and handle route definitions in a different module or modules. We should then use the package's __init__.py to strictly control import order and this initialization to ensure people can't import things in different orders and have weird things happening.

In the short term it might be best to move this to the __init__.py after a set of import statements that ensures all widget modules have been imported and initialized. Thinking about it, this might include importing the widget package. Does that make sense?

I've added the docstrings (except for one method which is actually internal-only, so I've prefixed it with an underscore like the convention recommends).

Having this execute at the bottom of this module makes me nervous.

I agree that we may be having a problem with the import order in the future. But I would argue that in that regard our problem comes from the way we register widgets: they are all imported in hubs/widgets/__init__.py, and that module is imported at the top of hubs/app.py.

I would totally agree to split views in a separate file, and then we could have a more "sane" way of registering widget views (importing views first, then widgets, then widget views).

I would also add that it kinda is "the Flask way" to register routes as the views are imported, since most of the time it's done via decorators, or module-level function calls (http://flask.pocoo.org/docs/0.12/views/), so the way I'm proposing here isn't fundamentally different, at least not from an import-order point of view.

But I totally agree it's messy. In fact, I'd be very happy to suggest a PR to change the way widgets are registered, which would be more easily customizable by the admin and wouldn't have to happen so early in the init process. But I'm afraid it would change quite a few things for people who are currently working on a widget.

Do you think it's worth doing anyway?

rebased

7 years ago

In fact, I'd be very happy to suggest a PR to change the way widgets are registered, which would be more easily customizable by the admin and wouldn't have to happen so early in the init process. But I'm afraid it would change quite a few things for people who are currently working on a widget.

Do you think it's worth doing anyway?

I certainly wouldn't object to a PR that improves the widget registration process. Although it might disrupt current on-going widget work a little bit, in my experience adjusting designs earlier rather than later causes less overall pain.

Also, I think this is okay to merge this PR as-is, but it is a "gotcha" we'll hit down the road when we decide to refactor. I've noticed a lot of the flask projects we have are precariously close to tons of circular imports because views are implemented in the same module the flask application object is created and configured. Views make use of functionality in other modules, and those modules in turn want to use Flask configuration values so they need to import the app object. Taking advantage of the init.py to configure the package and ensure a certain import order and that everything with views that require registration are, in fact, imported should solve that problem.

All that being said, it sounds like we're in agreement and I don't think that's a problem we need to solve in this PR. How about I make an issue where we can flesh out the details? This PR looks good to me so merge away!

Yep, looks like we agree :-)

Pull-Request has been merged by abompard

7 years ago