Add a pre-dispatch() hook for class-based views

[François Freitag]

Sharing Class-Based Views attributes across several view methods (get(), post()) is often implemented by overriding the dispatch() method. However, AFAICT subclasses cannot reuse attributes set by parent's dispatch() method, because it immediately generates an HttpResponse. Consider the following:

class UsernameStartsWithAMixin:
    def dispatch(self, request, *args, **kwargs):
        # some logic
        self.access_a = request.user.username.startswith('a')
        return super().dispatch(request, *args, **kwargs)


class LongUsernameMixin:
    def dispatch(self, request, *args, **kwargs):
        # some logic
        self.access_long = len(request.user.username) > 50
        return super().dispatch(request, *args, **kwargs)


class MyView(UsernameStartsWithAMixin, LongUsernameMixin, View):
    def dispatch(self, request, *args, **kwargs):
        # Cannot make decisions based on self.access_a nor self.access_long,
        # because they are not set yet. Calling super() generates the
        # HttpResponse.
        return super().dispatch(request, *args, **kwargs)

Not being able to read attributes set by parent classes in dispatch() results in code duplication, because some logic has to be repeated in each view method.
For example, Django's ​BaseUpdateView duplicates self.object = self.get_object() in the get() and post() methods.

A possible addition to the framework would be a hook called in dispatch(), responsible for initializing data shared by the class view methods. For example, BaseUpdateView would be implemented as:

class BaseUpdateView(ModelFormMixin, ProcessFormView):
    def initialize(self):
        super().initialize()
        self.object = self.get_object()

Maybe initialize would be clearer as pre_dispatch or initial?

[Carlton Gibson]

Hi François.

Hmmm. I'm sceptical about this to begin. For me, there's two things going on here: the framework specific job of handling the request, for which the logic belongs in dispatch(), and the application specific job of whatever-it-is-your-app-does, for which the logic belongs in the method handlers. I look at the examples here and think that you're mixing the two, just making the code harder to follow, and so creating an unnecessary maintenance burden, all for the sake a single line of duplication.

The specific get_object() call in BaseUpdateView is a key part of the view logic. That it's repeated is absolutely fine (for me): it keeps the view logic entirely on the surface, and clear as day. If that logic moves to dispatch(), the question is What's this doing here?, or more likely, I never look at it at all, because that's just not where it's meant to be, especially if spread across a number of mixins, as in your example. (If such repetition did become overwhelming, if can be factored to a helper that is a single line to call, that remains part of the view logic.)

... is often implemented by overriding the dispatch() method.

Really? (For these kinds of reasons) I just wouldn't do that. 🙂

However, as ever, there's more than one way to go about things... can I ask you to post this to django-developers to see if people are keen?

I'm always open to persuasion but my comments here would be an initial -1. (One thought: if you want this, can you not create a base view/mixin that adds the pre_dispatch() hook, then override that as needed, thus leaving Django out of it?)

For now, I'm going to close this here as wontfix. If the discussion on django-developers is positive, I'm really happy to see it re-opened then.
(Bottom-line is it's a design decision that's better off settled there than here. Hopefully that makes sense.)

[Jon Dufresne]

I'm +1 on adding this feature.

François and I work together. We discussed this feature internally within our own project before deciding to propose a Django feature.

Our project takes advantage of a large number of custom mixins for CBVs. These mixins reduce code duplication and provide consistent logic across views. Some mixins setup instance variables while others perform nuanced user authorization checks. The authorization checks depend on a lot of context of the view & mixins. Sometimes requiring the instances variables of the parent mixins. We'd like these authorization checks to happen in order -- parent class first. We often perform these checks in dispatch() as the instance variables and authorization checks apply universally to all HTTP methods.

We've found a shortcoming in CBVs when used with many intermediate mixins such that calling the parent's dispatch() (or get() or post()) returns a response object. Calling the parent's method first means the response object has already been created so it is too late to do authorization checks. Doing the authorization checks first means the parent mixin's authorization work has yet to be done, so instance variables are unavailable and the checks are out of order.

We'd like a hook so mixins could to use the parent's setup code to continue these custom nuanced authorization checks in order before preparing the response.

and the application specific job of whatever-it-is-your-app-does, for which the logic belongs in the method handlers.

The method handlers have the same problem. Even if the code were moved to post() and get(), we'd still need some pre-post() and pre-get(). In post(), calling the parent's post() first means a response object will be returned, making it too late to do additional authorization checks. Calling the parent's post() last means the parent's work isn't ready, therefore the mixin will miss out on instance variables and the authorization checks will be out of order. We discussed pre-dispatch() as our authorization checks normally apply to all HTTP methods.

I look at the examples here and think that you're mixing the two, just making the code harder to follow, and so creating an unnecessary maintenance burden, all for the sake a single line of duplication.

The example is intentionally simplified to illustrate the ordering problem in a short amount of code. Would a more complete example help?

If such repetition did become overwhelming, if can be factored to a helper that is a single line to call, that remains part of the view logic.

In the example, sometimes the "Cannot make decisions based on self.access_a nor self.access_long" exists in yet another mixin, as the pattern is repeated across a large number of views.

We find the duplication increases the maintenance burden as when writing a new new views, the developer must remember to include these repetitive calls. This can be easy to forget for new developers first learning the system. If x views inherit the mixin, then the "single line" must be repeated x times (or 2x times if for both post() and get()). As these mixins are often used for authorization, forgetting to repeat oneself could result in accidental leak of private data.

We found that DRF has implemented this pattern in its ​APIView class. Notice it has a initialize_request and initial method in its dispatch() function. This allows inherited classes and mixins to do instance variable setup and authorization checks before preparing the response. So there is some prior art and use cases out there. Beyond meeting our own project needs, I believe this feature would benefit the wider Django community by making third party reusable mixins easier.

[Simon Charette]

For what it's worth this is not a new idea.

Here's a 2012 discussion about adding such hook ​https://groups.google.com/d/msg/django-developers/7c7aI-slGNc/3gP1cLOL9usJ by Marc Tamlyn.

I remember that a few tickets were opened for adding such as feature over the years.

I think the general consensus was that it would be useful for a large number of people and it's something I support myself as I've found myself overriding dispatch() a few times in the past years just to be confronted with similar dilemmas.

[Carlton Gibson]

Fine. OK. That thread is quite long but I do see a lot of +1s there, in which case there’s no need to repeat that discussion. Let’s go for it.

Thanks for the input all.

[Carlton Gibson]

The previous discussion on django-developers (that Simon linked) ended linking to this commit ​https://github.com/ghickman/django/commit/ca2e6d68764c0f2215e43196b6e8e1daf2e8a6c4

Move the setting of args, kwargs & request on self into the nested
view function in as_view so they are set before dispatch is
called. This allows anyone wishing to put pre-dispatch code in an
overridden dispatch and still use the self versions of args, kwargs
and request.

Comments such as:

+1, this looks like a better solution than the hook method

We have that. (The relevant attributes are set in as_view()).

So, if we’re to add this now, the benefits that weren’t seen previously should be highlighted. (“Saves you creating a base view with the hook, since it’s already in place”, or such…)

[François Freitag]

​PR

[François Freitag]

Added an example in the advanced topic "Using mixins with class-based views".

[Tim Graham <timograham@…>]

In e671337:

Fixed #29750 -- Added View.setup() hook for class-based views.