Returned rows not properly exposing a mapping interface.

[rlittlefield]

my version: 0.2.2
asyncpg version: 0.18.3
functions: database.fetch_one, database.fetch_all

dict(user_row) when user_row is a databases.backends.postgres.Record object returns an empty dict for me when using postgres through asyncpg.

dict(user_row.__dict__['_row']) works, so I know that at least in my case the record has successfully loaded and there is a dict somewhere. dict style user_row['id'] works just fine.

[rlittlefield]

It also seems that user_row will be falsey, such that bool(user_row) will be False. This makes a common check to see if a row was returned fail.

[rlittlefield]

This can be worked around by checking for if user_row is None, but when dealing with a row object it is probably expected that any row returned is truthy.

[rlittlefield]

databases/databases/backends/postgres.py

Line 96 in c008b2a

def __getitem__(self, key: typing.Any) -> typing.Any:

It looks like this would work if the postgres backend's Record class fully implemented the Mapping interface by adding a .keys() method:

    def keys(self):
        if not self._column_map:
            return self._row.keys()
        return self._column_map.keys()

It appears in local development that adding that method to the Record makes the tests skip over __getitem__ entirely, which makes me a bit concerned that things like numeric column addresses won't work. Right now you can't use my_func(**record) without a keys(), because it expands to an empty dict. With the keys above, you could probably still ask for a number column one at a time with square brackets though. We just can't have it expand a column into both a text name and an integer index.

[lovelydinosaur]

things like numeric column addresses won't work

Okay. It's not obvious to me that we ought to be supported that as part of the API in any case. Any reason we shouldn't just be explicit that it returns a mapping interface, and not have any expectation of integer-indexing. Thoughts?

[rlittlefield]

If numeric columns are needed as a feature, it might make more sense to expose in a different way, like record.index(n), or like record.tuple[1]. Overloading named/index technically works, but splitting them would be more explicit.

If it really isn't a supported or used feature, it could just be dropped. I always use names because I'm worried about accidentally shifting a column by one.

[lovelydinosaur]

If numeric columns are needed as a feature, it might make more sense to expose in a different way, like record.index(n), or like record.tuple[1]

That's a good call, yup

[lovelydinosaur]

Considering this for the API:

Mapping interface: row['title']
Attribute lookup: row.c.title
(Perhaps also row.column.title, with row.c.title being a shortcut version.)
Indexed lookup row.index[0]

Seem okay?

[gvbgduh]

But I think it might become a tricky one as there're many variables:

• as was mentioned above asyncpg uses the Record object for fetch* method - https://magicstack.github.io/asyncpg/current/api/index.html#record-objects that implements mapping as python object relying on the query select params
• sqlalchemy core can't provide details for any kind of "raw" queries even over sqlalchemy.sql.text one with bindparams, so mapping in databases.postgres.Record will be empty, but asyncpg.Record will have one
• fetchval - returns value of a particular, it might be handy for count or exist queries, but it's not wrapped to the Record object
• execute, copy_* - returns only statuses of the command (INSERT 5, COPY 300, etc) as a string
• mysql, sqllite drivers usually return a plain list/tuple of results with no information about columns names (mapping and order should be handled separately before the request).

So, it bring some complexity to have a generalised approach.
Postgres backend has already Record class, but it's specific one and coupled to the PostgresConnection and PostgresTransaction.

And to just throw some ideas:
I'm wondering if it could be possible to consider the generic Record class or/and interface that can be reused over all backends and decoupling the Record class. So the Record class can passed (having the default as the default implementation) to PostgresBackend or in general to DatabaseBackend.
It might also help to support custom user Records classes, so the user can rely not only on sqlalchemy schemas, but define something using other libraries (typesystem(?), marshmallow(?)). I can't say I'm clearly see this, but it might be not that bad for the lower level purpose of databases. If it's not a wrong direction.

[Lawouach]

It might also help to support custom user Records classes, so the user can rely not only on sqlalchemy schemas, but define something using other libraries (typesystem(?), marshmallow(?)). I can't say I'm clearly see this, but it might be not that bad for the lower level purpose of databases. If it's not a wrong direction.

On that front, and this might actually be a totally different issue in itself, I'm certainly seeing the pain of constantly mapping from Records to pydantic. So such potentiall automated mapping would be useful. Not a blocker for me at all but would be convenient.

[onecrayon]

If numeric columns are needed as a feature, it might make more sense to expose in a different way, like record.index(n), or like record.tuple[1]. Overloading named/index technically works, but splitting them would be more explicit.

Why not just enforce string indices for all column names? So record[1] gives you the second column and record['1'] gives you the column named '1'? This seems like a pretty rare edge case, so I'm not sure that adding an explicit lookup for numeric indices vs. column names would really be all that beneficial (plus I assume most database drivers would be returning the column name as a string anyway).

[rlittlefield]

Why not just enforce string indices for all column names? So record[1] gives you the second column and record['1'] gives you the column named '1'? This seems like a pretty rare edge case, so I'm not sure that adding an explicit lookup for numeric indices vs. column names would really be all that beneficial (plus I assume most database drivers would be returning the column name as a string anyway).

That appears to be the intended API at the moment. The question that comes with a combined interface is how to handle things that expect a mapping. Right now, the record provides a mapping interface that isn't populated, at least for asyncpg records. If you make it a mapping that works but do both ints and strings, you end up with redundant properties if you iterate over the keys (once for index keys, once for string keys, unless you only use one or the other). Having them separate (name or int) could be really useful.

If we know what the API should look like here, some cleanup could be done on the column maps:
https://github.com/encode/databases/blob/master/databases/backends/postgres.py#L92

It looks like someone recently switched to the iter and len dunder methods to use the same from the underlying _row property:
https://github.com/encode/databases/blob/master/databases/backends/postgres.py#L117

[gvbgduh]

To be honest I'm getting a bit confused as it goes, so I'd like to start the loop again, but from the opposite direction.

The driver response for mysql, sqlite and aiopg is a tuple, so the RowProxy is totally justified, it doesn't solve the problem with raw queries as sqlalchemy can't be fully aware of the query details, but it's another issue.

asyncpg provides a quite sophisticated row wrapper - asyncpg.Record and strictly speaking it's the C data structure that has python bindings and it has quite a bit of functionality out of box.

And the key points of this data structure I'd like to highlight are:

1. It does't matter how the query is built, be it sqlalchemy object such as

SELECT some_table.id, some_table.status, some_table.created_at, some_table.updated_at 
FROM some_table
 LIMIT :param_1 OFFSET :param_2

or be it the plain string passed to the raw_driver, such as

SELECT id, status, created_at, updated_at FROM some_table LIMIT 100 OFFSET 0

the asyncpg.Record will contain exact order and exact set of fields that were queried, so in this case it's

<Record id=UUID('37e36d2f-dffd-5b9a-a3d2-1685d432bf21') status='OK' created_at=datetime.datetime(2019, 7, 18, 0, 54, 12, 682083) updated_at=datetime.datetime(2019, 7, 18, 0, 54, 12, 682083)>

NOTE: it stays true for count and exists queries, but if there's only one field I would go straight with fetch_val, which is especially handy for quickly getting bools from exists ones.

So, you don't have to rely on the sqlalchemy schema, it's always in place provided by asyncpg.

2. It does types conversion to python types, while it's pretty rare needed some special codecs can be specified as well.

3. It supports fields access by index and by key, such as:

(Pdb) res[0]
UUID('37e36d2f-dffd-5b9a-a3d2-1685d432bf21')
(Pdb) res['id']
UUID('37e36d2f-dffd-5b9a-a3d2-1685d432bf21')

and it has iterators for items, keys and values

(Pdb) dir(res)
['__class__', '__contains__', '__delattr__', '__dir__', '__doc__', '__eq__', '__format__', '__ge__', '__getattribute__', '__getitem__', '__gt__', '__hash__', '__init__', '__init_subclass__', '__iter__', '__le__', '__len__', '__lt__', '__ne__', '__new__', '__reduce__', '__reduce_ex__', '__repr__', '__setattr__', '__sizeof__', '__str__', '__subclasshook__', 'get', 'items', 'keys', 'values']

4* For execute operations it returns the status string, like COPY 0 200, INSERT 0 15 and it's quite handy in some cases, especially when you don't need RETURNING, but still want to know what action and how many items were processed.

With all of that above I'd like to ask - what if we expose asyncpg.Record as is?
and in that case is there some missing/desired/required functionality/compatibility that's not covered by the native record object?

[rlittlefield]

@gvbgduh I think it would be fine to just return asyncpg Record objects since the current databases Record is a different class for each backend. It also solves my initial reason for this github issue because dict(row) and myfunc(**row) would work.