Class-based views

[jace]

New take on class-based views, as the foundation for model-based views and CRUD views.

[jace]

Because of the use of decorators, this take on class views creates tight coupling between methods and routes. It is not possible for a subclass to redefine only the route, or only the method.

Consider this situation:

class BaseView(ClassView):
    @route('')
    def view(self):
        return "Base view"

@route('/doc')
class SubView(BaseView):
    def view(self):
        return "Redefined view"

SubView.init_app(app)

>>> client = app.test_client()
>>> client.get('/doc').data
'Base view'

Because of the way init_app works – it searches base classes for ViewDecorator instances – the redefined view did not override the base class's route. Possible resolutions:

1. Tweak ViewDecorator.init_app to check if getattr(cls, self.name) != self.method. If so, construct the view using the freshly retrieved method, not the originally decorated method. This will make the above code sample work, but is susceptible to cause confusion since the presence of a view handler isn't obvious. (Reject)

2. Add all symbols to the processed set. Make Python subclassing work like it should. If we don't specify a route, it means we want to remove the route. Just as properties have .getter and .setter decorators for replacing portions of the property, view decorators should have a .reroute decorator that keeps the route but replaces the handler. Usage:

@route('/doc')
class SubView(BaseView):
    @BaseView.view.reroute
    def view(self):
        return "Redefined view"

Since @BaseView.view will trigger ViewDecorator.__get__ and return the actual decorated method, we need a way to add the reroute decorator to it.

1. Make __get__ return a wrapper with the reroute method, and with a __call__ that passes through to the decorated method. (Heavy)
2. Stick the reroute decorator into the method's namespace. (Light but unusual)

[jace]

Should we call this decorator reroute or replace or override or something else? Consider a possible CrudView that provides a default view handler, that needs replacement with additional routes for something like Hasjob:

@route('/<domain>/<hashid>')
def DocumentView(CrudView):
    @route('/view/<hashid>')
    @CrudView.view.reroute  # This is inheriting @route('')
    def view(self, domain, hashid):
        return JobPost.get(hashid=hashid).one_or_404().current_access()

Is there a better term than 'reroute' here?

[jace]

We now allow sub views to replace view handlers or add additional URL rules like this:

class BaseView(ClassView):
    @route('')
    def first(self):
        return 'first'

    @route('second')
    def second(self):
        return 'second'

    @route('third')
    def third(self):
        return 'third'

    @route('inherited')
    def inherited(self):
        return 'inherited'

    @route('also-inherited')
    def also_inherited(self):
        return 'also_inherited'


@route('/subclasstest')
class SubView(BaseView):
    @BaseView.first.reroute
    def first(self):
        return 'rerouted-first'

    @route('2')
    @BaseView.second.reroute
    def second(self):
        return 'rerouted-second'

    def third(self):
        return 'removed-third'

    also_inherited = route('/inherited')(
        route('inherited2')(
            BaseView.get_view('also_inherited')))

SubView.init_app(app)

However, it is not possible to:

1. Take an existing view handler from the base class and replace all URL rules on it (instead of adding to them).

2. Convert a method in the base class into a view in the subclass. Base classes therefore can't provide template view handlers for the sub class to add a URL rule to.

[jace]

Converting a method into a view can be achieved like this:

class SubView(BaseView):
    @route('my-action')
    def my_action(self):
        return super(SubView, self).my_action()

This is unnecessary boilerplate though. Maybe we need a call like ClassView.add_route_for(methodname, rule, **options):

class SubView(BaseView):
    pass

SubView.add_route_for('my_action', 'my-action')

Calling this will mutate SubView to replace the existing attribute.

[jace]

Replacing get_view with add_route_for has also made the also_inherited example cleaner in SubView:

@route('/subclasstest')
class SubView(BaseView):
    @BaseView.first.reroute
    def first(self):
        return 'rerouted-first'

    @route('2')
    @BaseView.second.reroute
    def second(self):
        return 'rerouted-second'

    def third(self):
        return 'removed-third'

SubView.add_route_for('also_inherited', '/inherited')
SubView.add_route_for('also_inherited', 'inherited2')
SubView.add_route_for('latent_route', 'latent')
SubView.init_app(app)

[jace]

The reroute attribute added to the wrapped method is problematic since it mutates the method. Consider this:

class BaseView(ClassView):
    @route('')
    def first(self):
        return 'first'

@route('/subclass')
class SubView(BaseView):
    @route('first')
    @BaseView.first.reroute
    def first(self):
        return 'rerouted-first'

# Available routes:
# /subclass
# /subclass/first

@route('/anothersubclass')
class AnotherSubView(BaseView):
    @route('1')
    @BaseView.first.reroute  # This is now actually SubView.first.reroute
    def first(self):
        return 'rerouted-first'

# Available routes:
# /anothersubclass
# /anothersubclass/first
# /anothersubclass/1

We will have to use the descriptor approach we previously rejected.

[jace]

ModelView has been introduced in 796f0dd. It makes a slight change to ClassView's semantics (view handler methods no longer receive URL rule variables), and provides a rudimentary framework for mixin classes or subclasses to add functionality to. Usage:

@route('/doc/<document>')
class DocumentView(UrlForView, InstanceLoader, ModelView):
    model = Document
    route_model_map = {
        'document': 'name'
        }

    @route('')
    @render_with(json=True)
    def view(self):
        """
        Views no longer need to bother with loading an instance.
        Views are also auto-registered to the model as url_for targets
        """
        return self.obj.current_access()

DocumentView.init_app(app)

>>> doc1 = Document(name='test1', title="Test 1")
>>> doc2 = Document(name='test2', title="Test 2")
>>> db.session.add_all([doc1, doc2])
>>> db.session.commit()
>>> client = app.test_client()
>>> rv = self.client.get('/model/test1')
>>> data = json.loads(rv.data)
>>> data['name']
'test1'
>>> doc1.url_for('view')
'/doc/test1'
>>> doc2.url_for('')  # 'view' is the default
'/doc/test2'

[jace]

This call is a little unwieldy:

SubView.add_route_for('also_inherited', '/inherited')
SubView.add_route_for('also_inherited', 'inherited2')
SubView.add_route_for('latent_route', 'latent')

Why not make it like this?

SubView.also_inherited.add_route('/inherited')
SubView.also_inherited.add_route('inherited2')
SubView.latent_route.add_route('latent')

Since we now have a ViewDecoratorWrapper, it can provide this method and call add_route_for.

Update: this is tricky because ViewDecorator won't have a name until __set_name__ is called, which in Python < 3.6 only happens at init_app. add_route_for will instead have to search base classes by attribute value to find the name. That's a lot more tedious.

[jace]

This will affect Funnel's CommentSpace and VoteSpace, since there's no backward link from them to Proposal. The loader must load the Proposal and join with CommentSpace/VoteSpace. In this case, the space goes into self.obj while the proposal goes into self.proposal.

[jace]

InstanceLoader now does joined loads:

@route('/model/<parent>/<document>')
class ScopedDocumentView(DocumentView):
    model = ScopedViewDocument
    route_model_map = {
        'document': 'name',
        'parent': 'parent.name',
        }

ScopedDocumentView.init_app(app)

With this, #78 (load_models should support joined loads) is no longer relevant, with one caveat: we have no framework by which to handle redirect models, as highlighted in that ticket.

[jace]

Still missing: access control. We have two frameworks in Coaster: permissions and roles. Roles are appropriate for controlling model attribute access, typically in a flexible framework like GraphQL (see #100), while permissions are appropriate for controlling access to views.

load_models accepted a permission parameter, but in ModelView there's no place to add this decoration. It certainly can't be in @route.

Solution:

1. InstanceLoader calls obj.permissions(current_auth.actor)
2. InstanceLoader places the result at current_auth.permissions
3. A new requires_permission decorator (in coaster.views.decorators) checks for the specified permission in current_auth.permissions.

Problem: Calling obj.permissions isn't cheap. It should not be done if the view has no access control. However, at this time we have no way to signal to the loader that the view that will follow needs a permission check.

Solution: requires_permission sticks an attribute into the wrapper (requires_permission = (permissions), basically its own parameters). Since @wraps copies wrapper namespace, this attribute should remain inspectable. ModelView's before_request looks up the view function, checks for the presence of this attribute, and copies it into self.requires_permission. The loader can in turn check for this and decide on whether to call obj.permissions.

[jace]

I foresee problems with view mixins and the before_request hook. Let's say we move to using UUIDs in URLs (in the form of ShortUUIDs). When an object is moved to a new parent, or has a new name (BaseIdNameMixin's url_id_name used in the URL), the old URL should redirect to the new URL. We can do this by:

1. Specify route_model_map to not check the parent (in case parentage change is relevant).
2. In before_request, after loader is called, check if self.obj.url_for(view, _external=True) == request.base_url. If it's not, redirect to that URL.

Problem: If this is a view mixin (named, say, UrlRewriteView), how do we ensure its before_request is called after InstanceLoader's? It's all about the difference between these:

class MyView(InstanceLoader, UrlRewriteView, ModelView):
    …

class MyView(UrlRewriteView, InstanceLoader, ModelView):
    …

In the first case, UrlRewriteView will work correctly. In the second case, it won't. One possibility is to simply expect self.obj to be present, so it throws an exception if the order was incorrect. The exception could be trapped to present an explanatory error message.

[jace]

ClassView (or a mixin) should register itself as current_view in template context. This will let templates refer back to the view for additional methods, or to check for whether some features are available.

[jace]

ClassView/ModelView should allow @with_roles and @state.transition decorators on class methods. Let these be used to define business logic without forcibly coding them into the models. Views can hold UI layers that models cannot.

Problem: state.transitions will now contain a view method. What’s the protocol for accessing it? Create a class instance first, or assume it will be created when called?

[jace]

1. Adding @with_roles is fine, but it's only an annotator. Something else – maybe a view decorator – will have to check for whether the role is available.

Problem: state.transitions will now contain a view method. What’s the protocol for accessing it? Create a class instance first, or assume it will be created when called?

2. This is a bigger problem. We don't want to pollute the model with a view's methods. We want to instead decorate a transition with a view, so the transition's availability determines the view's availability. There may be distinct GET and POST views to each transition, so it's a 1:2 mapping from transition to view.

Also, given multiple transitions that take no/similar parameters, a wildcard view should be possible (n:n mappings):

@route('/<mymodel>')
class MyModelView(InstanceLoader, ModelView):
    model = MyModel
    …
    @route('<transition>', methods=['POST'])  # Creates /<mymodel>/<transition>
    @render_with(json=True)
    @transition_view('transition')  # String parameter implies transition named in the keyword argument
    def all_transitions(self, transition):  # Loaded transition is passed as 'transition' parameter to view
        transition()
        return {'status': 'ok'}

    @route('special', methods=['POST'])  # Creates /<mymodel>/special
    @render_with(json=True)
    @transition_view(MyModel.special)  # Transition parameter implies this is the transition we want to call
    def special_transition(self, transition):  # MyModel.transition has turned into self.model.transition
        transition()
        return {'status': 'ok', 'special': True}

[jace]

This is unnecessarily verbose. Why not:

class MyModelView(InstanceLoader, TransitionView):
    model = MyModel
    transition_forms = {
        'special': SpecialForm
        }

MyModelView.add_route_for('call_transition', '<transition>', methods=['POST'])

Note that this cannot offer the GET portion which renders a form, because UI rendering is out of scope in Coaster. It should be provided as part of hasgeek/baseframe#80.

[jace]

@shreyas-satish @iambibhas I think ClassView and ModelView are now mature enough for this PR to be merged. We should move transition wrappers into a separate PR.