Add middleware support

[nhumrich]

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:

1. I currently went with the same syntax that aiohttp uses for middlewares. I had the thought that you could do something similar but use generators instead of result = await handler(query) you do result = yield query. But I am willing to accept any ideas on middleware syntax.
2. I currently cant figure out how to test locally against a docker container, so i havent run the tests locally.

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?

[eirnym]

Could you also add a decorator to write the same functions simpler? e.g remove factory boilerplate when it's not needed?

[nhumrich]

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?

[eirnym]

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]

Hi! Any news on this PR?

[nhumrich]

Sorry, i have been busy due to the holidays, I will try to finish this up soon

[victoraugustolls]

Glad to hear it! If you need any help, just ask ;) !

[nhumrich]

Please let me know if anything else is needed.

[victoraugustolls]

@nhumrich it seems to have an unexpected indentation, besides of this, is it ok to merge @eirnym ?

[eirnym]

@nhumrich Looks fine by me

[elprans]

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:

1. 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.

2. I don't think that exposing the internal interface of _execute and making it a public API is a good idea. limit, timeout,and return_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 on query and args and nothing else.

3. 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]

@elprans

1. 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. convert dict (or other objects) to JSON/YAML/XML/Pro tobuf response based on settings or headers.
2. Agreed, it makes sense not to expose anything to a handler if not needed
3. 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

[elprans]

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]

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

[nhumrich]

@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?