simplifying middleware #2252

samuelcolvin · 2017-09-07T18:17:35Z

if you've got here following the the depreciation warning comment: see middleware docs for details on upgrading

What do these changes do?

This simplifies middleware so middlewares are simple coroutines rather than coroutine factories returning coroutines.

Simply:

from aiohttp.web import middleware

@middleware
async def my_middleware(request, handler):
    return await handler(request)

Old style middleware:

async def middleware_factory(app, handler):
    async def middleware_handler(request):
        return await handler(request)
    return middleware_handler

Continues to work with a depreciation warning.

Are there changes in behavior for the user?

They can simplify their middleware if they wish

Rationale

The new style is cleaner, less code, quicker to read, quicker to write.
Old style leaves a big bad bear trap for developers to fall into: assuming the outer coroutine is only called once and using it for setup code.

Related issue number

#2225

Checklist

I think the code is well written
Unit tests for the changes exist
Documentation reflects the changes
Add a new news fragment into the changes folder
- name it <issue_id>.<type> for example (588.bug)
- if you don't have an issue_id change it to the pr id after creating the pr
- ensure type is one of the following:
  - .feature: Signifying a new feature.
  - .bugfix: Signifying a bug fix.
  - .doc: Signifying a documentation improvement.
  - .removal: Signifying a deprecation or removal of public API.
  - .misc: A ticket has been closed, but it is not of interest to users.
- Make sure to use full sentences with correct case and punctuation, for example: "Fix issue with non-ascii contents in doctest text files."

samuelcolvin · 2017-09-07T18:22:44Z

@asvetlov

Then we'll open a kind of voting for collecting feedback from other devs.

How do you propose to vote? Using github reactions?

I've delayed updating docs until we agree this is required. Also if we are changing to this, perhaps we should give some kind of indication how long old-style middleware will continue to work.

codecov-io · 2017-09-08T09:01:05Z

Codecov Report

Merging #2252 into master will increase coverage by <.01%.
The diff coverage is 97.22%.

@@            Coverage Diff             @@
##           master    #2252      +/-   ##
==========================================
+ Coverage   97.23%   97.24%   +<.01%     
==========================================
  Files          39       39              
  Lines        8179     8190      +11     
  Branches     1430     1433       +3     
==========================================
+ Hits         7953     7964      +11     
  Misses         98       98              
  Partials      128      128

Impacted Files	Coverage Δ
aiohttp/web.py	`99.66% <100%> (+0.01%)`	⬆️
aiohttp/web_middlewares.py	`97.36% <95.83%> (+0.14%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 20854d6...c7df4b4. Read the comment docs.

asvetlov · 2017-09-08T10:12:31Z

Hmm, in your proposal all middlewares are either new-styled or old ones.
It's impossible to combine new-style middleware from some third-party library with old-fashioned one.
Therefore it looks like very hard backward compatibility breaker.

samuelcolvin · 2017-09-08T10:13:54Z

ok, I'll extend it to check every middleware.

samuelcolvin · 2017-09-09T18:45:44Z

ok, now old and new middleware can be mixed.

asvetlov · 2017-10-11T17:17:54Z

aiohttp/web.py

@@ -272,6 +274,20 @@ def _make_request(self, message, payload, protocol, writer, task,
            self._loop,
            client_max_size=self._client_max_size)

+    def _prepare_middleware(self):
+        for m in reversed(self._middlewares):
+            sig = inspect.signature(m)


I don't like signature inspecting. The check is too weak.
In my mind we should mark a new style middleware very explicitly.
Let's wrap it by @newstylemiddleware decorator.
The decorator will add __middleware_version__ attr to decorated object with value 1.

@samuelcolvin what do you think?

Seems ugly, but I agree the signature approach is brittle.

I'll add it unless anyone can think of a third, less cleaner approach?

instead of the decorator we could use a type annotation for new style middleware? So if you want to use new style middleware you have to use

async def my_middleware(request, app) -> Request: ...

After all, this is what type annotations are for.

We could then add a depeciation warning in future if middleware doesn't include a type annotation declaring its return type.

Yep, type annotations is even better than decorator!

why not?

It's there as a language feature, we can use it if we want.

We can implement the decorator solution too in case some people can't (eg. 3.4) or don't want to use type annotations.

@samuelcolvin yes, new and old styles are different by return type, now I see it.

The language feature's usage is static type checking only. Technically you could use it for different purposes but this is discouraged.
Strings as type annotations are supported now by typing and will become mandatory in next Pythons. Stub files make situation even more complex.
Let's use decorator approach.
@kxepal could you explain your idea?

Genuine question: "but this is discouraged", can you point me to where that's stated? I wasn't aware of it.

I really don't see that stub files have have relevance here, AFAIK they're particular to mypy. I was just suggesting using function.__annotations__, same as another application might use function.__name__. Stub files and strings as type annotations have no bearing on this.

However, I see a real point to not using type annotations: developers assume they're either advisory or for static type analysis; therefore it doesn't make much sense to use them for business logic in one place in the code.

I'll implement the decorator solution now.

@samuelcolvin I don't remember exact blogpost or article but Guido told many times about his vision for annotations: previously it was not utilized by python stdlib etc., people used to apply them on they own.
But now annotations are for type checking only. Other approaches are still supported to not break existing code (and will be supported at least for decade) though.

@asvetlov

def old_style_middleware(app, handler): async def middleware_handler(request): ... return middleware_handler @middleware def new_style_middleware(request, handler): ...

where middleware is something like

def middleware(func): def old_style_middleware(app, handler): async def middleware_handler(request): return await func(request, handler) return middleware_handler return old_style_middleware

Or whatever it should be to create new style arguments. But, since aiohttp implements middleware decorator, it may dictate what it should decorate and what to pass for decorated function.

Thought, I still not quite follow what's wrong with current approach. Yes, it's a bit verbose, but quite simple and explicit.

asvetlov

Looks almost perfect!
Would you update documentation as well?

asvetlov · 2017-10-12T13:13:21Z

aiohttp/web.py

+    def _prepare_middleware(self):
+        for m in reversed(self._middlewares):
+            if getattr(m, '__middleware_version__', None) == 1:
+                # first argument is request or r and this is


The comment is irrelevant

asvetlov · 2017-10-12T13:15:02Z

aiohttp/web_middlewares.py

@@ -23,6 +24,11 @@ def _check_request_resolves(request, path):
    return False, request


+def new_middleware(f):


Hmm.
I don't like the name. We have new_middleware and have no old_middleware.
Mybe just @middleware is enough?

I agree that I don't like new_middleware, I'll switch to middleware.

asvetlov · 2017-10-12T14:50:20Z

docs/web.rst

-*middleware*. For example, here's a trivial *middleware factory*::
+A *middleware* is just a coroutine that can modify either the request or
+response. For example, here's a simple *middleware* which appends
+``' wink'`` to the response::


The example doesn't work with streamed responses and websockets. Maybe we should mention it (in brackets).

asvetlov · 2017-10-12T15:40:09Z

Perfect!
Feel free to merge after making spell checker happy.
Hint: use does not instead of doesn't

samuelcolvin · 2017-10-12T16:15:56Z

Passing except for codecov. I can't merge.

Can I suggest configuring codecov so it's not this brittle?

asvetlov · 2017-10-12T17:44:28Z

That's because line https://github.com/aio-libs/aiohttp/pull/2252/files#diff-724d27c86f7c189969e6112c6976feacR62 is only partially covered but I could live with it.

@samuelcolvin if you'll cover the line -- I very appreciate it.
If not -- just say "don't want", I'll merge the PR as is.

Hmm, maybe I'm wrong -- feel free to make a PR with codecov config improvement.

samuelcolvin · 2017-10-12T17:46:11Z

I don't want to here, I thought about it but it's not really related to this PR (the line was there before, with just the indent changed).

asvetlov · 2017-10-12T17:49:31Z

Accepted

asvetlov · 2017-10-12T17:49:57Z

Thanks for such valuable improvement, @samuelcolvin

See aio-libs/aiohttp#2252

lock · 2019-10-28T14:03:59Z

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a [new issue] for related bugs.
If you feel like there's important points made in this discussion, please include those exceprts into that [new issue].
[new issue]: https://github.com/aio-libs/aiohttp/issues/new

simplifying middleware, fix #2225

376c31c

samuelcolvin requested a review from asvetlov September 7, 2017 18:17

set depeciation message

d5bc052

Update test_web_middleware.py

7dfb2f7

allow mixed middleware styles

5b7fdba

Merge branch 'master' into new-middleware

4952e10

asvetlov reviewed Oct 11, 2017

View reviewed changes

asvetlov mentioned this pull request Oct 11, 2017

aiohttp 2.3 release schedule #2250

Closed

samuelcolvin and others added 2 commits October 12, 2017 13:30

use decorator for new middleware

cc3287a

Merge branch 'master' into new-middleware

8dc0701

asvetlov requested changes Oct 12, 2017

View reviewed changes

samuelcolvin and others added 2 commits October 12, 2017 15:41

rename decorator to 'middleware' and add docs

f6396fe

Merge branch 'master' into new-middleware

9f12f8f

asvetlov reviewed Oct 12, 2017

View reviewed changes

tweaking docs

e08ca47

asvetlov previously approved these changes Oct 12, 2017

View reviewed changes

fix spelling

11e857d

samuelcolvin dismissed asvetlov’s stale review via 11e857d October 12, 2017 15:45

'streamed response' not 'steamed response' :-)

c7df4b4

asvetlov merged commit 872bea1 into aio-libs:master Oct 12, 2017

Pajinek mentioned this pull request Oct 11, 2018

New style of middleware underyx/aiohttp-sentry#15

Closed

labbati mentioned this pull request Oct 16, 2018

Update outdated aiohttp middleware DataDog/dd-trace-py#651

Closed

jettify mentioned this pull request Nov 22, 2018

Resolve warning from aiohttp related to new middleware API aio-libs/aiozipkin#179

Closed

jcox-dev mentioned this pull request Dec 11, 2018

TBL Ignore deprecation warnings from unittests and elevate others to error ONSdigital/respondent-home-ui#92

Merged

Ranc58 mentioned this pull request Apr 5, 2019

fix middleware for actual aiohttp version hzlmn/aiohttp-jwt#94

Merged

sersorrel added a commit to wsi-cogs/backend that referenced this pull request Jul 22, 2019

Update auth middleware to use new style

d40cdfa

See aio-libs/aiohttp#2252

lock bot added the outdated label Oct 28, 2019

lock bot locked as resolved and limited conversation to collaborators Oct 28, 2019

psf-chronographer bot added the bot:chronographer:provided There is a change note present in this PR label Oct 28, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

simplifying middleware #2252

simplifying middleware #2252

samuelcolvin commented Sep 7, 2017 •

edited

Loading

samuelcolvin commented Sep 7, 2017

codecov-io commented Sep 8, 2017 •

edited

Loading

asvetlov commented Sep 8, 2017

samuelcolvin commented Sep 8, 2017

samuelcolvin commented Sep 9, 2017

asvetlov Oct 11, 2017

samuelcolvin Oct 11, 2017

samuelcolvin Oct 11, 2017 •

edited

Loading

samuelcolvin Oct 11, 2017

asvetlov Oct 11, 2017

samuelcolvin Oct 12, 2017

asvetlov Oct 12, 2017

samuelcolvin Oct 12, 2017

asvetlov Oct 12, 2017

kxepal Oct 12, 2017

asvetlov left a comment

asvetlov Oct 12, 2017

asvetlov Oct 12, 2017

samuelcolvin Oct 12, 2017

kxepal Oct 12, 2017

asvetlov Oct 12, 2017

samuelcolvin Oct 12, 2017

asvetlov commented Oct 12, 2017

samuelcolvin commented Oct 12, 2017

asvetlov commented Oct 12, 2017

samuelcolvin commented Oct 12, 2017

asvetlov commented Oct 12, 2017

asvetlov commented Oct 12, 2017

lock bot commented Oct 28, 2019

		@@ -23,6 +24,11 @@ def _check_request_resolves(request, path):
		return False, request


		def new_middleware(f):

simplifying middleware #2252

simplifying middleware #2252

Conversation

samuelcolvin commented Sep 7, 2017 • edited Loading

if you've got here following the the depreciation warning comment: see middleware docs for details on upgrading

What do these changes do?

Are there changes in behavior for the user?

Rationale

Related issue number

Checklist

samuelcolvin commented Sep 7, 2017

codecov-io commented Sep 8, 2017 • edited Loading

Codecov Report

asvetlov commented Sep 8, 2017

samuelcolvin commented Sep 8, 2017

samuelcolvin commented Sep 9, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

samuelcolvin Oct 11, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

asvetlov left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

asvetlov commented Oct 12, 2017

samuelcolvin commented Oct 12, 2017

asvetlov commented Oct 12, 2017

samuelcolvin commented Oct 12, 2017

asvetlov commented Oct 12, 2017

asvetlov commented Oct 12, 2017

lock bot commented Oct 28, 2019

samuelcolvin commented Sep 7, 2017 •

edited

Loading

codecov-io commented Sep 8, 2017 •

edited

Loading

samuelcolvin Oct 11, 2017 •

edited

Loading