Conversation
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?
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
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 |
4444edd
to 2ff5dad
Compare @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:
|
|
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.
Exactly, and asyncpg has nothing to do with HTTP servers, WSGI and other things.
I still don't see why you'd need a "factory" to achieve any of this, just pass a list of callables.
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. |
@elprans I am fine calling this hooks instead of middleware.
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?
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?