API Improvements for asyncpgsa

[nhumrich]

I am a maintainer of asyncpgsa, which is really just asyncpg, but with support for sqlalchemy objects instead of just strings as the query.

In order for asyncpgsa to work correctly, I need to subclass (or use composition in my case) the Connection object, so that I can parse the queries before sending them to asyncpg. Right now I am doing this by subclassing the Pool and overriding the _new_connection() method. I simply take the connection object and swap it out with my version of the connection. This is what I have been doing so far. I am now currently trying to upgrade asyncpgsa to use asyncpg 0.10.1, which no longer has that method on the pool class, and also requires a bunch of required parameters to the Pool class. I am currently trying to figure out what method to replace in order to get a new connection, but all sorts of things have changed. There are now a lot more objects involved, etc. Anyways, I don't come here asking for help to solve that problem right now, I am here to ask for the api to be improved in this case.

1. No more required arguments in the Pool class that are not required in the create_pool method. This makes it significantly harder to subclass pool. Every time another parameter gets added, all subclasses are now broken. You should make the Pool class handle the defaults, then just pass them through with the create_pool method.
2. Some easy way to change what connection object is being returned by the pool. It looks like the new way (on 0.10.1) is to override the _connect method instead the _new_connection method it was previously. Changes to protected methods (_method) are expected, so im not complaining. Just asking for some method or something that can stay consistent in the future.

I am happy to implement said things and submit a PR if you would like. But before I do so, wanted to see what your thoughts on this were. Do you have a way you would prefer on how to create connection objects? Maybe just a single method that is documented now to change, that belongs to the pool, so that connections can easily be subclassed and returned?

[lelit]

FYI, I followed a different approach to build a SA adapter, somewhat cleaner and surely faster, that does not require parsing SA output nor any change on the asyncpg side.

The following is my custom dialect:

from sqlalchemy.dialects.postgresql.psycopg2 import (PGCompiler_psycopg2,
                                                     PGDialect_psycopg2)
from sqlalchemy.sql import compiler
from sqlalchemy.types import NullType


compiler.BIND_TEMPLATES['numeric'] = '$[_POSITION]'


class PGCompiler_asyncpg(PGCompiler_psycopg2):
    """Custom SA PostgreSQL compiler that produces param placeholder
    compatible with asyncpg.

    This solves https://github.com/MagicStack/asyncpg/issues/32.
    """

    def _apply_numbered_params(self):
        idx = 0
        names = self.positiontup
        binds = self.binds
        process = self.dialect.type_compiler.process

        def replace(match):
            nonlocal idx
            name = names[idx]
            idx += 1
            param = binds[name]
            type = param.type
            if isinstance(type, NullType):
                return str(idx)
            else:
                return '%s::%s' % (idx, process(type))

        self.string = re.sub(r'\[_POSITION\]', replace, self.string)


class PGDialect_asyncpg(PGDialect_psycopg2):
    """Custom SA PostgreSQL dialect compatible with asyncpg.

    In particular it uses a variant of the ``numeric`` `paramstyle`, to
    produce placeholders like ``$1::INTEGER``, ``$2::VARCHAR`` and so on.
    """

    statement_compiler = PGCompiler_asyncpg

    def __init__(self, *args, **kwargs):
        kwargs['paramstyle'] = 'numeric'
        super().__init__(*args, **kwargs)
        self.implicit_returning = True
        self.supports_native_enum = True
        self.supports_smallserial = True
        self._backslash_escapes = False
        self.supports_sane_multi_rowcount = True
        self._has_native_hstore = True

With that in place, you can just say:

compiled = stmt.compile(dialect=PGDialect_asyncpg())
params = compiled.construct_params(named_args)
print(compiled.string)
print(tuple(params[p] for p in compiled.positiontup))

[1st1]

No more required arguments in the Pool class that are not required in the create_pool method. This makes it significantly harder to subclass pool. Every time another parameter gets added, all subclasses are now broken. You should make the Pool class handle the defaults, then just pass them through with the create_pool method.

Why can't you do the below?

class MyPoolSubclass(asyncpg.Pool):
    def __init__(self, *args, **kwargs):
        super().__init__(*args, **kwargs)
        # ... do your thing ...

def create_my_pool(*args, **kwargs):
    return MyPoolSubclass(*args, **kwargs)

Some easy way to change what connection object is being returned by the pool. It looks like the new way (on 0.10.1) is to override the _connect method instead the _new_connection method it was previously.

We can rename the new _connect method to _new_connection.

Changes to protected methods (_method) are expected, so im not complaining. Just asking for some method or something that can stay consistent in the future.

Well, we can't promise _methods staying consistent :) But we can keep in mind that certain projects rely on overloading them and try to minimize the churn.

Maybe we'll provide a stable API for overloading the Connection class, but I don't think we are ready to do this right now.

[nhumrich]

@lelit this is really cool. I might take this approach in asyncpgsa. Your welcome to submit a PR if you want credit for it. I assume right now you are just using asyncpg but compiling the string manually yourself before sending it to asyncpg?

[nhumrich]

@1st1

Why can't you do the below?

What you have suggested with the pool subclass is actually what I have. The problem is that asyncpg.Pool.__init__() has required parameters. So when you call super().__init__(*a, **k) you get errors unless you add the defaults in the create_pool method. The problem is that you are handling the defaults at the create_pool level, but should be handling them at the Pool level. So I have to manually add those defaults to my version of create_pool whenever new parameters are added.

We can rename the new _connect method to _new_connection.

I honestly dont care what the name is. _connect is fine. I just want something consistent in the future.

Anyways, I understand that your not ready for a stable Connection class overloading.

On a completely different thought, going off of what @lelit said, a potentially better solution is if you could add "middleware" or pre-processing to asyncpg. For example, when you initialize the pool, you add your own middleware/preprocessing functions that can do certain things before the query runs. Maybe something like aiohttp middlewares. That would render asyncpgsa as only a library to get the processing middleware. Keeps it stupid-simple. Anyways, what are your thoughts on that?

[1st1]

So I have to manually add those defaults to my version of create_pool whenever new parameters are added.

Why don't you do this:

# in asyncpgsa

import functools

@functools.wraps(asyncpg.create_pool)
def create_pool(*args, **kwargs):
    return SubclassedPool(*args, **kwargs)

This way you'll have the asyncpg docstring, help() and Sphinx will show the correct signature etc.

That would render asyncpgsa as only a library to get the processing middleware. Keeps it stupid-simple. Anyways, what are your thoughts on that?

This is an interesting idea. We'll discuss it with @elprans.

[lelit]

I might try to adapt my solution to asyncpgsa, but I'm quite busy on getting an app out of the door these days, so I can't promise anything in the short term.

[1st1]

FWIW we plan to release a new version of asyncpg in a couple of days, with a public API to make connect and create_pool return subclassed Connections.

[1st1]

So I'll be merging #130 soon. It adds new connection_class argument to asyncpg.connect() and asyncpg.create_pool().