-
Notifications
You must be signed in to change notification settings - Fork 432
Add middleware support #482
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
Conversation
@eirnym
eirnym
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you also add a decorator to write the same functions simpler? e.g remove factory boilerplate when it's not needed?
f7ad73d
to
f423316
Compare
Could you also add a decorator to write the same functions simpler? e.g remove factory boilerplate when it's not needed?
I like this idea. Do you have any suggestions where the decorator should live? In other words, what is the ideal import path for the decorator?
After supplying both parameters to the middleware explicitly, the solution could looks like this:
async def middleware(query, args, limit, timeout, return_status, *, handler, conn): print('do something before') result, stmt = await handler(query, args, limit, timeout, return_status) print('do something after') return result, stmt
and middleware processing could be like this:
for m in reversed(app._middlewares): wrapped = partial(m, handler=wrapped, conn=self) result, _ = await wrapped(query, args, limit, timeout, return_status=return_status)
The same has been done in similar framework like aiohttp
victoraugustolls
commented
Dec 30, 2019
Hi! Any news on this PR?
Sorry, i have been busy due to the holidays, I will try to finish this up soon
Glad to hear it! If you need any help, just ask ;) !
4444edd
to
2ff5dad
Compare
Please let me know if anything else is needed.
victoraugustolls
commented
Jan 16, 2020
eirnym
commented
Jan 17, 2020
@nhumrich Looks fine by me
2c19388
to
a227a3a
Compare
Hi @nhumrich. Sorry for the delay in reviewing this and thanks for working on this.
To start, I have a few of notes on the approach:
-
I don't like the name "middleware". I know it's somewhat commonly used, but usually in the context of apps and service APIs. Here we are working with a fairly low-level API, so the term "hook" seems more approprite to me.
-
I don't think that exposing the internal interface of
_execute
and making it a public API is a good idea.limit
,timeout
,andreturn_status
are internal and should not be fiddled with by the callbacks, otherwise bad things will happen. To that extent, I think it would be enough if the hooks are called onquery
andargs
and nothing else. -
I'm not sure why you need a closure over the callback, why not pass callables directly like all other hook APIs in asyncpg?
eirnym
commented
Jan 18, 2020
- I disagree with you on terminology:
Term "hook" is used as an event handler, not something in between http server and your handler, like "keypress event hook" or "on connect"
Term "middleware" is commonly used as a layer between http server and my handler and this is used almost everywhere like this: Django, Flask, aiohttp, Spring, and many others. Any middleware handlers could return response before and/or do something after, e.g. convertdict
(or other objects) to JSON/YAML/XML/Pro tobuf response based on settings or headers. - Agreed, it makes sense not to expose anything to a handler if not needed
- You can have many independent middleware functions do something and you can register them in modular way, changing order if needed, etc. I'd recommend to see documentation on aiohttp's middleware usage as it's short and easy, you can also read docs for http servers I mentioned above
Term "hook" is used as an event handler,
You are confusing hooks with, well, event handlers or callbacks. The term "hook" is well established as a mechanism to enable pluggable behavior modification. PostgreSQL itself has hooks.
Term "middleware" is commonly used as a layer between http server and my handler
Exactly, and asyncpg has nothing to do with HTTP servers, WSGI and other things.
You can have many independent middleware functions
I still don't see why you'd need a "factory" to achieve any of this, just pass a list of callables.
register them in modular way, changing order if needed
Relying on order and hook stacking is usually a bad idea that leads to fragile code that is hard to follow and reason about. Please don't bring app frameworks as a reason why this complexity needs to exist in a low-level driver.
eirnym
commented
Jan 18, 2020
I'm sorry, my bad. I was confused with repo names. Now I realised it and I agree with you. Please, ignore my comment above
a227a3a
to
faad8de
Compare
@elprans I am fine calling this hooks instead of middleware.
why do you need a closure
This is mainly needed so that the "hooks" can call eachother in the correct order, and are able to modify the contents on either side of the database query. For example, say I have an application performance monitoring hook, I want it to log and time how long every query takes. I need to not only do something before the request is made, but also once the request finishing.
Anyways, I am up for changing anything, if you just want to offer some guidance of how you would like it done. I am not sure why you are so hesitant to closures, but if you dont like them, could you offer an example of how you think this could work with standard callables?
why not pass callables directly like all other hook APIs in asyncpg?
I dont see the word hook at all in the asyncpg docs, can you give me an example of some hook api's that already exist in this library?
This is my initial stab at #152. Feedback would be appreciated, and I am sure I am not doing things the way you would prefer.
A couple notes:
result = await handler(query)
you doresult = yield query
. But I am willing to accept any ideas on middleware syntax.Pretty much everything in this PR is a proof of concept, and open to discussion. This issue has just sat here way to long, and I wanted to get the ball rolling. So, can you help me proceed to a better solution?