- Download Diff

Summary:

Add support for middleware to extensions

Review Request #4530 — Created Sept. 5, 2013 and submitted Oct. 8, 2013, 2:11 p.m.

Information
Owner:	mcote
Repository:	Djblets
Branch:	master
Bugs:
Depends On:
Blocks:	4563
Reviewers
Groups:	djblets
People:	smacleod

Description

Add support for middleware in extensions.

Testing Done

All unit tests pass.

Issues

Description	From	Last Updated
Hm... so it sounds like we'd be generating this list pretty much every time the MiddlewareRunner was hit... Could we …	mike_conley	Sept. 9, 2013, 2:03 p.m.
Maybe "_recalculate_middleware?"	chipx86	Sept. 12, 2013, 1:39 p.m.
What is "this" and "dependencies" in theis context? Not sure this paragraph is describing the function.	chipx86	Sept. 12, 2013, 1:39 p.m.
Blank line between statements and for/if/etc. statements that form blocks.	chipx86	Sept. 12, 2013, 1:39 p.m.
Should be _get_extension_middleware.	chipx86	Sept. 12, 2013, 1:39 p.m.
I'd recommend specifying that this is a utility function for _set_middleware, and describe how it's used/what it does, since it …	chipx86	Sept. 12, 2013, 1:39 p.m.
Blank around each of htese.	chipx86	Sept. 12, 2013, 1:39 p.m.
Blank before this.	chipx86	Sept. 12, 2013, 1:39 p.m.
And before this.	chipx86	Sept. 12, 2013, 1:39 p.m.
And before this.	chipx86	Sept. 12, 2013, 1:39 p.m.
I don't see where this is used. The class should have a docstring going into detail on what this is …	chipx86	Sept. 12, 2013, 1:40 p.m.
Blank line before.	chipx86	Sept. 12, 2013, 1:40 p.m.
And here.	chipx86	Sept. 12, 2013, 1:40 p.m.
And here.	chipx86	Sept. 12, 2013, 1:40 p.m.
And here.	chipx86	Sept. 12, 2013, 1:40 p.m.
And here.	chipx86	Sept. 12, 2013, 1:40 p.m.
And here.	chipx86	Sept. 12, 2013, 1:40 p.m.
And here.	chipx86	Sept. 12, 2013, 1:40 p.m.
And here.	chipx86	Sept. 12, 2013, 1:40 p.m.
It might be worth documenting here that this class needs to be added to the settings.py MIDDLEWARE_CLASSES for the extension …	SM smacleod	Sept. 12, 2013, 2:51 p.m.
Mark should go before Micah.	mike_conley	Sept. 25, 2013, 7:06 p.m.

This is a review from Review Bot.
Tool: PEP8 Style Checker
Processed Files:
djblets/extensions/base.py
djblets/extensions/middleware.py
Ignored Files:
AUTHORS

This is a review from Review Bot.
Tool: Pyflakes
Processed Files:
djblets/extensions/base.py
djblets/extensions/middleware.py
Ignored Files:
AUTHORS

mcote Sept. 9, 2013, 9:25 a.m.

Yeah, that's a good idea. I only fully understood how extensions were loaded part-way through writing this patch, so that never occurred to me. Will fix.

djblets/extensions/middleware.py (Diff revision 1)

Show all issues

Hm... so it sounds like we'd be generating this list pretty much every time the MiddlewareRunner was hit...

Could we not have the ExtensionManager maintain this list of middleware, and add / remove to it when extensions are enabled and disabled? This way, we wouldn't need to generate it each time.

Or am I missing a case?

Change Summary:

Updated with mconley's suggestion: creation and ownership of the middleware list is now in ExtensionManager instead of in ExtensionMiddlewareRunner.

Diff:

Revision 2 (+83)

Show changes

	AUTHORS
	djblets/extensions/base.py
	djblets/extensions/middleware.py

This is a review from Review Bot.
Tool: PEP8 Style Checker
Processed Files:
djblets/extensions/base.py
djblets/extensions/middleware.py
Ignored Files:
AUTHORS

This is a review from Review Bot.
Tool: Pyflakes
Processed Files:
djblets/extensions/base.py
djblets/extensions/middleware.py
Ignored Files:
AUTHORS

Change Summary:

Cleaning up description.

Description:

~		This adds support for middleware to djblets extensions. To enable in ReviewBoard, you just have to add 'djblets.extensions.middleware.ExtensionsMiddlewareRunner' to the end of the MIDDLEWARE_CLASSES list in reviewboard/settings.py (will put that up in a separate patch once this patch is merged in).
	~	Add support for middleware in extensions.
-
-		I didn't add any tests as it seemed rather difficult to mock up something that runs all the middleware functions. Suggestions welcome if there's a decent mechanism that I don't know of. :)

djblets/extensions/base.py (Diff revision 2)
Show all issues
```
Maybe "_recalculate_middleware?"
```

djblets/extensions/base.py (Diff revision 2)

Show all issues

What is "this" and "dependencies" in theis context? Not sure this paragraph is describing the function.

mcote Sept. 12, 2013, 1:33 p.m.

I was trying to explain that, rather than attempting to insert or delete exactly the middleware (and dependencies) of the enabled/disabled extension, we just throw away the whole list and recalculate it for all extensions.  It's probably an unnecessary justification, though, especially after renaming the function as you suggested, so I'll just delete it.

djblets/extensions/base.py (Diff revision 2)

Show all issues

Blank line between statements and for/if/etc. statements that form blocks.

djblets/extensions/base.py (Diff revision 2)
Show all issues
```
Should be _get_extension_middleware.
```

djblets/extensions/base.py (Diff revision 2)

Show all issues

I'd recommend specifying that this is a utility function for _set_middleware, and describe how it's used/what it does, since it has to operate within _set_middleware.

djblets/extensions/base.py (Diff revision 2)

Show all issues

Blank around each of htese.

mcote Sept. 12, 2013, 1:40 p.m.

I assume you mean blank lines around this whole block, not around each line.

djblets/extensions/base.py (Diff revision 2)
Show all issues
```
Blank before this.
```
djblets/extensions/base.py (Diff revision 2)
Show all issues
```
And before this.
```
djblets/extensions/base.py (Diff revision 2)
Show all issues
```
And before this.
```

djblets/extensions/middleware.py (Diff revision 2)

Show all issues

I don't see where this is used.
The class should have a docstring going into detail on what this is for.
Same with all the functions within.

mcote Sept. 12, 2013, 1:33 p.m.

Heh this is the whole point of the patch. :) However I really should have added some comments, which I will do now.

djblets/extensions/middleware.py (Diff revision 2)
Show all issues
```
Blank line before.
```
djblets/extensions/middleware.py (Diff revision 2)
Show all issues
```
And here.
```
djblets/extensions/middleware.py (Diff revision 2)
Show all issues
```
And here.
```
djblets/extensions/middleware.py (Diff revision 2)
Show all issues
```
And here.
```
djblets/extensions/middleware.py (Diff revision 2)
Show all issues
```
And here.
```
djblets/extensions/middleware.py (Diff revision 2)
Show all issues
```
And here.
```
djblets/extensions/middleware.py (Diff revision 2)
Show all issues
```
And here.
```
djblets/extensions/middleware.py (Diff revision 2)
Show all issues
```
And here.
```

Change Summary:

New patch addressing issues. I also noticed that ExtensionManager._recalculate_middleware() was still being called on every page load because ExtensionManager.load() also is, so I modified the latter to only call the former if an extension is added or removed.

Diff:

Revision 3 (+116)

Show changes

	AUTHORS
	djblets/extensions/base.py
	djblets/extensions/middleware.py

This is a review from Review Bot.
Tool: PEP8 Style Checker
Processed Files:
djblets/extensions/base.py
djblets/extensions/middleware.py
Ignored Files:
AUTHORS

This is a review from Review Bot.
Tool: Pyflakes
Processed Files:
djblets/extensions/base.py
djblets/extensions/middleware.py
Ignored Files:
AUTHORS

Just a nit about some documentation, otherwise it looks good to me!

djblets/extensions/middleware.py (Diff revision 3)

Show all issues

It might be worth documenting here that this class needs to be added to the settings.py

MIDDLEWARE_CLASSES for the extension middleware to be run.

mcote Sept. 12, 2013, 2:45 p.m.

Hm well, there are other middleware classes in djblets that are loaded by ReviewBoard, and none of them have a comment like that. I was just going to add it to ReviewBoard's settings.py as a separate patch once this one is committed.

smacleod Sept. 12, 2013, 2:51 p.m.

Fair enough. You're right, the other middleware isn't documented like that.

chipx86 Sept. 15, 2013, 7:35 p.m.

It should document, however, whether placement matters, and if so, where it should go.

mcote Sept. 16, 2013, 2:25 p.m.

Hm, so, I don't see an explicit comment in, say, djblets.siteconfig.middleware.SettingsMiddleware, which has a comment above it in reviewboard/settings.py, but I will add one anyway.

Ship it!

```
Ship It!
```

Change Summary:

Added comment about positioning within middleware list.

Diff:

Revision 4 (+122)

Show changes

	AUTHORS
	djblets/extensions/base.py
	djblets/extensions/middleware.py

This is a review from Review Bot.
Tool: PEP8 Style Checker
Processed Files:
djblets/extensions/base.py
djblets/extensions/middleware.py
Ignored Files:
AUTHORS

This is a review from Review Bot.
Tool: Pyflakes
Processed Files:
djblets/extensions/base.py
djblets/extensions/middleware.py
Ignored Files:
AUTHORS

Ship it!

```
Ship It!
```

Just one minor nit. The rest of this looks fine to me.

AUTHORS (Diff revision 4)
Show all issues
```
Mark should go before Micah.
```
1. MC
  
  mcote Sept. 25, 2013, 7:06 p.m.
  Right you are.

Change Summary:

Corrected order in AUTHORS.

Diff:

Revision 5 (+122)

Show changes

	AUTHORS
	djblets/extensions/base.py
	djblets/extensions/middleware.py

This is a review from Review Bot.
Tool: PEP8 Style Checker
Processed Files:
djblets/extensions/base.py
djblets/extensions/middleware.py
Ignored Files:
AUTHORS

This is a review from Review Bot.
Tool: Pyflakes
Processed Files:
djblets/extensions/base.py
djblets/extensions/middleware.py
Ignored Files:
AUTHORS

Ship it!

```
Ship It!
```

Ship it!

```
Ship It!
```

Ship it!

```
Ship It!
```

Status: Closed (submitted)

Change Summary:

Pushed to release-0.7.x (e28f6d2). Thanks!

You have a pending review.

Review Board 5.0.2 alpha 0 (dev)

Add support for middleware to extensions

Change Summary:

Diff:

Change Summary:

Description:

Change Summary:

Diff:

Change Summary:

Diff:

Change Summary:

Diff:

Status: Closed (submitted)

Change Summary: