Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Changes to middleware logic #764

Merged
merged 20 commits into from Jan 12, 2020
Merged

Changes to middleware logic #764

merged 20 commits into from Jan 12, 2020

Conversation

@masipcat
Copy link
Contributor

masipcat commented Dec 10, 2019

No description provided.

@masipcat masipcat requested review from bloodbare and vangheem as code owners Dec 10, 2019
1. ASGI middlewares: run before the guillotina asgi application and can be used to
catch errors or to record timing metrics
Comment on lines 166 to 167

This comment has been minimized.

Copy link
@masipcat

masipcat Dec 10, 2019

Author Contributor

can be used to catch errors

Right now this is not true because the traversal captures all unhandled exceptions and generates the error response. Ideally, the first ASGI middleware should be the ErrorMiddleware that generates the error response, like in Starlette: https://github.com/encode/starlette/blob/7f8cd041734be3b290cdb178e99c37e1d5a19b41/docs/middleware.md

Agree?

This comment has been minimized.

Copy link
@masipcat

masipcat Dec 10, 2019

Author Contributor

Also, I think that guillotina_prometheus should be rewritten as a ASGI middleware because now it runs after the traversal so the response_time/latency isn't accurate (doesn't include the time to run traversal).

This comment has been minimized.

Copy link
@jordic

jordic Dec 11, 2019

Contributor

Cool! That's the reason why our api service it's so fast :)

This comment has been minimized.

Copy link
@vangheem

vangheem Dec 12, 2019

Member

A error handling middleware seems like a nice simple way to provide an example middleware!

This comment has been minimized.

Copy link
@masipcat

masipcat Dec 12, 2019

Author Contributor

I'll write it ;) The thing is that we need to decouple this logic from the traversal to middlewares. I'd say that the error and transaction logic should be in separate middlewares.

(incoming request) 
ErrorASGIMiddleware
[ User-provided-ASGI-middlewares ... ]
TransactionASGIMiddleware
TraversalRouterMiddleware (creates Request obj)
[ User-provided-middlewares ... ]
View execution

Makes sense?

This comment has been minimized.

Copy link
@vangheem

vangheem Dec 16, 2019

Member

I think that should work.

I'm not sure a TraversalRouterMiddleware is necessary though?

This comment has been minimized.

Copy link
@masipcat

masipcat Dec 16, 2019

Author Contributor

TraversalRouterMiddleware is the AsgiApp in guillotina/asgi.py, so this would be:

(incoming request) 
ErrorASGIMiddleware
[ User-provided-ASGI-middlewares ... ]
TransactionASGIMiddleware
GuillotinaAsgiApp (creates Request obj)
[ User-provided-middlewares ... ]
View execution

To be sure, I understand that you agree on decoupling the error and transaction logic outside of the Router/Traversal/AsgiApp , right?

This comment has been minimized.

Copy link
@vangheem

vangheem Dec 16, 2019

Member

Yes, error and transaction make sense.

Maybe I just need to see an implementation to understand how I'll like it.

This comment has been minimized.

Copy link
@masipcat

masipcat Jan 7, 2020

Author Contributor

Done!

@codecov-io

This comment has been minimized.

Copy link

codecov-io commented Dec 10, 2019

Codecov Report

❗️ No coverage uploaded for pull request base (master@0a149d4). Click here to learn what that means.
The diff coverage is 98.3%.

Impacted file tree graph

@@           Coverage Diff            @@
##             master    #764   +/-   ##
========================================
  Coverage          ?   94.3%           
========================================
  Files             ?     312           
  Lines             ?   28246           
  Branches          ?       0           
========================================
  Hits              ?   26631           
  Misses            ?    1615           
  Partials          ?       0
Impacted Files Coverage Δ
guillotina/schema/interfaces.py 100% <ø> (ø)
guillotina/tests/test_content.py 100% <ø> (ø)
guillotina/behaviors/dublincore.py 100% <ø> (ø)
guillotina/tests/dbusers/test_api.py 100% <ø> (ø)
guillotina/utils/misc.py 79.5% <ø> (ø)
guillotina/interfaces/content.py 100% <100%> (ø)
guillotina/files/manager.py 100% <100%> (ø)
guillotina/blob.py 97% <100%> (ø)
guillotina/contrib/dbusers/content/groups.py 86.5% <100%> (ø)
guillotina/files/exceptions.py 100% <100%> (ø)
... and 30 more
masipcat added 4 commits Dec 10, 2019
now
@masipcat masipcat force-pushed the g6-backwards-compatbility branch from 9f93df7 to 1e762e0 Dec 11, 2019
@masipcat

This comment has been minimized.

Copy link
Contributor Author

masipcat commented Dec 11, 2019

@vangheem @bloodbare PR ready for review

1. ASGI middlewares: run before the guillotina asgi application and can be used to
catch errors or to record timing metrics

This comment has been minimized.

Copy link
@vangheem

vangheem Dec 12, 2019

Member

A error handling middleware seems like a nice simple way to provide an example middleware!

docs/source/installation/configuration.md Outdated Show resolved Hide resolved
@masipcat masipcat changed the title Added back support for Guillotina/aiohttp middlewares + other canges Added back support for Guillotina/aiohttp middlewares + other changes Dec 12, 2019
Copy link
Member

bloodbare left a comment

Can we define pre and post middlewares? Logging and error handling should be done after handling request.

@masipcat

This comment has been minimized.

Copy link
Contributor Author

masipcat commented Dec 16, 2019

Can we define pre and post middlewares?

If you mean post view execution, no. We can define ASGI middlewares before "Guillotina", in other words, before the router/traversal. These will sit between the ASGI server and the ASGI framework.
Then we have the same behaviors we already have, that sit between the router/traversal and the view to execute (like the aiohttp middlewares).

Logging and error handling should be done after handling request.

In that case, the ASGI middlewares will never see a python exception therefore we can't use middlewares like Sentry.

@vangheem

This comment has been minimized.

Copy link
Member

vangheem commented Dec 16, 2019

maybe we should allow flexibility so you could insert a middleware between the error one and do custom error handling?

Also, as for pre/post... usually the way you implement middleware should be done where pre/post is part of the middleware implementation.

dummy psuedo code following:

async def my_middleware(app, req):
  # pre code
  resp = await app(req)
  # post code
  return resp
@masipcat masipcat changed the title Added back support for Guillotina/aiohttp middlewares + other changes Added back support for Guillotina/aiohttp middlewares Dec 18, 2019
@masipcat masipcat changed the title Added back support for Guillotina/aiohttp middlewares WIP: Added back support for Guillotina/aiohttp middlewares Dec 18, 2019
@masipcat masipcat added the wip label Dec 18, 2019
masipcat added 4 commits Dec 18, 2019
masipcat added 2 commits Jan 7, 2020
@masipcat masipcat changed the title WIP: Added back support for Guillotina/aiohttp middlewares Added back support for Guillotina/aiohttp middlewares Jan 7, 2020
@masipcat masipcat removed the wip label Jan 7, 2020
@masipcat

This comment has been minimized.

Copy link
Contributor Author

masipcat commented Jan 7, 2020

@vangheem @bloodbare Ready for review!

Changes:

  • I managed to add support for asgi middlewares and aiohttp (can be mixed).
  • Added ErrorsMiddleware that catch all errors and renders the json response. Is always the first middleware
  • Moved the traversal and resolve_route to TraversalRouter
  • Moved the view execution to ServiceExecutor (so we can run other middlewares between the traversal and the view, like in G5)

So, now we can add middlewares like Sentry and Prometheus before Traversal.

@masipcat

This comment has been minimized.

Copy link
Contributor Author

masipcat commented Jan 7, 2020

Logging and error handling should be done after handling request.

@bloodbare Regarding your comment, now the request is instantiated before executing any request middleware

Copy link
Member

vangheem left a comment

Really nice job Jordi.

I'm still trying to digest how I feel about this.

I'm not sure I like how this feels with traversal and service being their own middleware.

It's a way to provide decoupling between the components which is really nice; however, I hope it doesn't provide too much indirection.

Also, I wonder if we should adjust the call signatures of the middleware or use scope instead of task_vars.

task vars are nice; however, I'd prefer they are not used to pass dependencies between middleware.

self.config_file = config_file
self.settings = settings
self.loop = loop
self.router = router
# ...

This comment has been minimized.

Copy link
@vangheem

vangheem Jan 9, 2020

Member

this comment meant to be here?

guillotina/middlewares/errors.py Show resolved Hide resolved
"""

async def __call__(self, scope, receive, send):
service_handler = task_vars.service.get()

This comment has been minimized.

Copy link
@vangheem

vangheem Jan 9, 2020

Member

What do you think about having some of this data on the scope dictionary instead of relying on task var lookups?

Otherwise, we could also change the signature of the the middleware.

This comment has been minimized.

Copy link
@masipcat

masipcat Jan 9, 2020

Author Contributor

I prefer using the scope dictionary, so we can use generic asgi middlewares that use this signature. I'll change it

guillotina/asgi.py Show resolved Hide resolved
@masipcat

This comment has been minimized.

Copy link
Contributor Author

masipcat commented Jan 9, 2020

The thing is:

  • aiohttp/G5 runs the middlewares after the traversal/router and before the service.
  • G6 runs the middlewares before or after the traversal/router. I've done it that way to maintain compatibility with G5.

However, I don't see the benefit from running middlewares after the TraversalRouter. This logic can be implemented in a custom service class or in a custom decorator.

So, if you agree, I can remove the ServiceExecutor and drop support for middlewares after the router. This would reduce the changes and complexity.

Copy link
Member

vangheem left a comment

The thing is:

  • aiohttp/G5 runs the middlewares after the traversal/router and before the service.
  • G6 runs the middlewares before or after the traversal/router. I've done it that way to maintain compatibility with G5.

However, I don't see the benefit from running middlewares after the TraversalRouter. This logic can be implemented in a custom service class or in a custom decorator.

So, if you agree, I can remove the ServiceExecutor and drop support for middlewares after the router. This would reduce the changes and complexity.

Yes, I agree!

masipcat added 3 commits Jan 9, 2020
@masipcat masipcat changed the title Added back support for Guillotina/aiohttp middlewares Changes to middleware logic Jan 9, 2020
@masipcat

This comment has been minimized.

Copy link
Contributor Author

masipcat commented Jan 9, 2020

Final changes:

Now we only support ASGI middlewares that run before the Traversal/Router and before instantiating the request.

This would require updating guillotina_prometheus (do we have more middlewares?). I'll take care.

@masipcat

This comment has been minimized.

Copy link
Contributor Author

masipcat commented Jan 9, 2020

In a future PR I'll try to move the CORS logic and Debug headers into a middleware to reduce the code in the traversal. Is it ok?

Copy link
Member

bloodbare left a comment

Its an amazing work Jordi! I’ve also some doubts about middlewares nesting of the frame for each request. I understand that error handling (sentry or gerror need to wrap the call) but for XDebug I would prefer a set of functions on a post/pre way (I dont know if something like IBeforeTraverse and IAfterCommit). I’m really strong to not move any internal G to this wrappers! Sequencial is better to wrapping imho. @vangheem if u feel confortable i’ll aprove

@vangheem

This comment has been minimized.

Copy link
Member

vangheem commented Jan 12, 2020

I prefer it be a wrapper. That's how I understand middleware is supposed to work.

Another thought I had is that maybe we could make sure our middleware is compatible with starlette middleware.

@bloodbare

This comment has been minimized.

Copy link
Member

bloodbare commented Jan 12, 2020

I prefer it be a wrapper. That's how I understand middleware is supposed to work.

Another thought I had is that maybe we could make sure our middleware is compatible with starlette middleware.

Yups, I mean defining that some functionality of G can be exported as middlewares on wrappers (traversals, .... ) I feel its going to be overengineering.

Copy link
Member

bloodbare left a comment

I agree with this middleware implementation, with error middleware and simplification of the code

@vangheem

This comment has been minimized.

Copy link
Member

vangheem commented Jan 12, 2020

ahh, gotcha. I had the same worry then kind of. @masipcat how did you feel about it after moving it? Did it seem clean or a bit odd to move those bits into middelware?

I can try to spend some more time thinking about it, reviewing and seeing how I feel about it.

@jordic

This comment has been minimized.

Copy link
Contributor

jordic commented Jan 12, 2020

From my humil point of view, thought we should go on g6 with what we have.. later when we use it for a time perhaps we can evolve a bit more (like g7?)

@masipcat

This comment has been minimized.

Copy link
Contributor Author

masipcat commented Jan 12, 2020

Thank you for reviewing!


@bloodbare I'm ok with not moving the X-Debug logic and other internals to a middleware :)


Another thought I had is that maybe we could make sure our middleware is compatible with starlette middleware.

@vangheem this would be great, but I see some problems:

  1. In Starlette the middlewares use their Request/Response objects.
  2. In guillotina, the request is created after all middlewares ran. I'd like to implement the Request as a proxy to the asgi scope and create the request before running the middlewares, but it's a bit tricky.
  3. Starlette uses https://www.starlette.io/middleware/#basehttpmiddleware in some middlewares (it's a abstraction that allows you to write middlewares easily, but it caused some problems in our projects -- each call_next() run the next middleware in another asyncio task... Maybe @jordic rembemers better the issues we faced)

@masipcat how did you feel about it after moving it? Did it seem clean or a bit odd to move those bits into middelware?

Good, it's not the first asgi middleware I write and the ErrorMiddleware is the perfect use-case for a middleware. Also, I feel it happens too much in the Traversal and IMHO we should move the "http related logic outside the "guillotina app"/traversal. Like CORS :P


Finally, I 100% agree with @jordic . Now we have the minimal implementation to support generic asgi middlewares.

Maybe we should discuss where we want to go. Do we want to be a http framework that is compatible with asgi servers? Do we want to build guillotina on top of starlette (like FastAPI)? or maybe we only want to use their Request/Response to have compatibility with their Middlewares.

@vangheem

This comment has been minimized.

Copy link
Member

vangheem commented Jan 12, 2020

Okay, I agree. I'm going to approve.

@vangheem vangheem merged commit 74fa6b4 into master Jan 12, 2020
5 checks passed
5 checks passed
Changelog verifier Entry found
Details
Plone Contributors Agreement verifier All users have signed it
Details
codecov/patch No report found to compare against
Details
codecov/project No report found to compare against
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@vangheem vangheem deleted the g6-backwards-compatbility branch Jan 12, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants
You can’t perform that action at this time.