Opened 5 years ago

Closed 2 years ago

Last modified 2 years ago

#12502 closed Bug (fixed)

Diagram doesn't match text in Middleware documentation

Reported by: petrilli Owned by: Aymeric Augustin <aymeric.augustin@…>
Component: Documentation Version: master
Severity: Normal Keywords:
Cc: shai, safplusplus Triage Stage: Accepted
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: yes
Easy pickings: no UI/UX: no

Description

In the [current documentation http://docs.djangoproject.com/en/dev/topics/http/middleware/], the diagram under Activate Middleware doesn't match the MIDDLEWARE_CLASSES above it.

Attachments (13)

middleware_square.png (25.7 KB) - added by petrilli 5 years ago.
Design with squares instead of circles
middleware_arrow.png (31.4 KB) - added by petrilli 5 years ago.
Request Response Middleware.graffle (73.0 KB) - added by petrilli 5 years ago.
12502_image_patch.diff (87.3 KB) - added by gabrielhurley 5 years ago.
Patch updates the middleware.png to use the middleware_arrow.png provided on this ticket.
Middleware.process_request.png (33.7 KB) - added by SafPlusPlus 5 years ago.
Diagram of middleware request processing
Middleware.process_view.png (51.1 KB) - added by SafPlusPlus 5 years ago.
Diagram of middleware view processing
Middleware.process_exception.png (41.9 KB) - added by SafPlusPlus 5 years ago.
Diagram of middleware response processing
Middleware.process_response.png (30.0 KB) - added by SafPlusPlus 5 years ago.
Diagram of middleware response processing
Middleware.zip (83.8 KB) - added by SafPlusPlus 5 years ago.
Extension of petrilli's OnmiGraffle file
django-middleware-processing.svg (31.6 KB) - added by shai 5 years ago.
SVG file -- diagram depicting middleware processing
django-middleware-processing-bg.png (62.3 KB) - added by shai 5 years ago.
PNG file -- diagram depicting middleware processing
middleware.svg (11.1 KB) - added by aaugustin 2 years ago.
middleware-2.svg (10.4 KB) - added by aaugustin 2 years ago.

Download all attachments as: .zip

Change History (34)

Changed 5 years ago by petrilli

Design with squares instead of circles

comment:1 Changed 5 years ago by petrilli

  • Needs documentation unset
  • Needs tests unset
  • Patch needs improvement unset

I have attached a couple designs, and the OmniGraffle document they're derived from. Rather than continue with the circle, I switched to squares in one of them simply because the middleware names were becoming unreadable due to their length.

comment:2 Changed 5 years ago by timo

The attached diagram is out of date, since MessageMiddleware is now included by startproject (#12795). We may want to consider an approach that doesn't require the image to be updated each time this list is changed.

comment:3 Changed 5 years ago by russellm

  • Triage Stage changed from Unreviewed to Accepted

Changed 5 years ago by petrilli

Changed 5 years ago by petrilli

comment:4 Changed 5 years ago by petrilli

I've updated the diagram, but I agree that it could become an issue long-term. Unfortunately, I can't think of a way to express it "visually" in words that will make it clear how the process works.

Changed 5 years ago by gabrielhurley

Patch updates the middleware.png to use the middleware_arrow.png provided on this ticket.

comment:5 Changed 5 years ago by gabrielhurley

  • Has patch set
  • Owner changed from nobody to gabrielhurley
  • Status changed from new to assigned

I added a patch that just does a one-for-one replacement of the existing middleware.png file with the middleware-arrow.png provided by petrilli (which is currently up-to-date).

Until a decision is reached about finding a way to avoid using an image at all, we might as well have a working patch for the docs.

comment:6 Changed 5 years ago by shai

  • Cc shai added

Hi,

The images provided, as well as the one currently in the docs, share a problem: They are misleading, because they over-simplify the process.

The points missed:

  1. There are actually two passes "going in", one using process_request and one using process_view
  2. The "go in, then out" pictures create the impression that process_response will only be called for a given middleware class if the corresponding process_request was called; the text makes it clear that this isn't the case.
  3. Exception processing is left completely out of the picture, and that is especially misleading; in particular, it should be made clear that exception processing does not replace response processing, but is done before it; and that it is only applied to the view, not to any processing in the middlewares.

The mismatch of the list of middlewares is, in comparison, completely minor IMHO. Also, I think a simple, complete solution to the list mismatch problem is to use generic names ("MiddleWareA, MiddleWareB" etc.) in the diagram.

Changed 5 years ago by SafPlusPlus

Diagram of middleware request processing

Changed 5 years ago by SafPlusPlus

Diagram of middleware view processing

Changed 5 years ago by SafPlusPlus

Diagram of middleware response processing

Changed 5 years ago by SafPlusPlus

Diagram of middleware response processing

Changed 5 years ago by SafPlusPlus

Extension of petrilli's OnmiGraffle file

comment:7 Changed 5 years ago by SafPlusPlus

  • Cc safplusplus added

I agree with shai on that the current diagram also doesn't represent correctly how the middleware processing actually works. I took petrilli diagram and extended it with diagrams for the handling of requests, views, exceptions and responses.

Two reasons though why I'm not 100% content with my diagrams. One being that they are ugly and the second being that they may appear too complex to glance over quickly.

comment:8 Changed 5 years ago by gabrielhurley

  • Owner gabrielhurley deleted
  • Status changed from assigned to new

I'm gonna unassign this to let somebody else make the final decision during the sprint since I'm mostly not available right now.

comment:9 Changed 5 years ago by russellm

  • milestone 1.2 deleted

Deferring due to the absence of a trunk-ready patch.

Changed 5 years ago by shai

SVG file -- diagram depicting middleware processing

Changed 5 years ago by shai

PNG file -- diagram depicting middleware processing

comment:10 Changed 5 years ago by shai

The diagram I've just attached shows, I believe, a true picture of the middleware processing. Black arrows show the normal flow, gray arrows are other possible (non-erroneous) paths. I think the rest is self-explanatory enough.

This is the PNG, for easy viewing; a "source" (SVG) was also attached.

PNG file -- diagram depicting middleware processing

Shai.

comment:11 Changed 4 years ago by gabrielhurley

  • Patch needs improvement set

This latest diagram is by far the most accurate, but my concern is that it's accurate at the cost of being fairly complicated. I'm not certain there's a better compromise between accuracy and complexity to be had, but I worry this will further confuse inexperienced users.

I think at the very least it should have a key in the diagram explaining what the colors mean (black = normal flow, grey = possible short-circuits, red = exception).

If anyone else can think how this could be further simplified feel free to chime in.

comment:12 Changed 4 years ago by mattmcc

  • Severity set to Normal
  • Type set to Bug

comment:13 Changed 3 years ago by aaugustin

  • UI/UX unset

Change UI/UX from NULL to False.

comment:14 Changed 3 years ago by aaugustin

  • Easy pickings unset

Change Easy pickings from NULL to False.

comment:15 Changed 2 years ago by shai

I would like to point out that with the introduction of process_template_response, the middleware documentation has become unclear and ambiguous: I couldn't fathom, by reading it, if process_template_response calls are all made before process_response calls or instead of them. If the former, than my suggested diagram is no longer correct, and the true picture is even more complex.

comment:16 Changed 2 years ago by aaugustin

We have to strike a balance between completeness and clarity. This graph is displayed at the beginning of the introduction to middleware and should be understandable at first sight. For this reason, I wouldn't attempt to show that each middleware can short-circuit the rest of the chain by returning a response in process_request/view/exception. It's clearly explained in the text and rather intuitive.

However, there's another aspect that I'd like to represent: process_template_response and process_response are applied after process_exception. The current docs describe process_exception at the end — they start with the common case; that can be confusing. They repeat that "Response middleware is always called on every response" but the concrete result is difficult to understand (at least for me) without a visual representation.

I'm attaching (yet another) SVG file that implements these ideas and tries not to be too ugly.

Version 0, edited 2 years ago by aaugustin (next)

Changed 2 years ago by aaugustin

Changed 2 years ago by aaugustin

comment:17 Changed 2 years ago by aaugustin

After getting some feedback on IRC, I'm going to commit the second version.

I'll include the OmniGraffle source file in the commit, even though it's in a proprietary format, because we often waste time looking for sources.

comment:18 follow-up: Changed 2 years ago by shai

I agree that the second version is better, but I still think it doesn't achieve the goals you set for it in comment:16. In particular, it doesn't clarify the relationship between the three middleware functions on the way out.

Here's a quick ascii-art sketch of what I think the diagram should look like. Every block should be expanded to "middleware1, middleware2, middleware3" in the correct order as in my previous suggestion.

Question: is it -- as implied by the suggested diagrams -- that an exception middleware can return a template response to be processed by the template-response middleware? The below represents my guess that this is not the case.

#                       |
#                       V
#              [ process_request ]
#                       |
#                       V
#              [  process_view   ]
#                       |
#                       V
#              [    USER VIEW     ]
#              /        |         \
# [process_exception]   |   [process_template_response]
#              \        V         /
#              [ process_response ]
#                       |
#                       V

(Of course, the arrows to process_exception and process_template_response should be formed and colored to mark their optionality etc).

comment:19 Changed 2 years ago by Aymeric Augustin <aymeric.augustin@…>

  • Owner set to Aymeric Augustin <aymeric.augustin@…>
  • Resolution set to fixed
  • Status changed from new to closed

In ae8e97384bbacaf4094b5f98c83b8599478b3fac:

Fixed #12502 -- Improved middleware diagram.

comment:20 in reply to: ↑ 18 Changed 2 years ago by aaugustin

Replying to shai:

I agree that the second version is better, but I still think it doesn't achieve the goals you set for it in comment:16.

Sorry, I committed before seeing your comment. I wanted to get it done tonight rather than let it linger for another three years :)

Question: is it -- as implied by the suggested diagrams -- that an exception middleware can return a template response to be processed by the template-response middleware?

Yes, this is possible (look at django.core.handlers.base).

There are four possibilities for the response phase:

  • exception => template_response => response
  • exception => response
  • template_response => response
  • response

The dashes on the diagram mean that exception and template_response are optional, so these four possibilities are represented.

comment:21 Changed 2 years ago by shai

Oh, Ok, then. I stand corrected. Thanks for finally taking care of this.

Note: See TracTickets for help on using tickets.
Back to Top