Support for `connection.raw_connection` #58

gvbgduh · 2019-03-03T13:29:50Z

Adds ability to access the underlying driver's connection interface.

gvbgduh · 2019-03-03T13:42:33Z

I followed 2 different approaches:

Pass the target method as str to the generic method like raw_api_call, so the driver can be called as getattr(obj, str). While it saves some lines for implementation it can introduced unacceptable complexity for generalised solution. The source of problem that mysql and sqlite (and asyncpg in case of explicit cursor) require two actions to preform the query. It might also lead to a dead-end.
Just exposing the raw backend connection. It gives full flexibility and control, but requires more verbose implementation, which is probably ok for such cases.

If anything of these looks good I can get rid of redundant bits in the code and finish the rest of unittests. I'm also quite bad at naming.
Any feedback would be much appreciated.

gvbgduh · 2019-03-03T13:50:44Z

Yeah, p. 2 (expose_backend_connection) looks somehow neat without interfering with existing, but might require some attention for releasing connections back to pool.

gvbgduh · 2019-03-04T08:06:32Z

databases/backends/postgres.py

-    def __getitem__(self, key: str) -> typing.Any:
-        idx, datatype = self._column_map[key]
-        raw = self._row[idx]
+    def __getitem__(self, key: typing.Any) -> typing.Any:


Not happy with this implementation in general and especially passing typing.Any. I think it might be limited to str and int as a raw record's fields can be accessed by an int index or by the str key.

tomchristie

Crackin' stuff. A few comments here - most importantly, tweaking things so that .raw_connection is a property.

tomchristie · 2019-03-04T12:40:26Z

databases/backends/postgres.py

@@ -150,6 +150,10 @@ def __init__(self, database: PostgresBackend, dialect: Dialect):
        async for row in self._connection.cursor(query, *args):
            yield Record(row, result_columns, self._dialect)

+    async def raw_connection(self) -> asyncpg.connection.Connection:


This doesn't need to (shouldn't) be an async method. (It's not performing any I/O itself, just returning the handle)

I guess we should probably use a property too, rather than a method.

@property def raw_connection(self): ...

It's possible that the implementations ought to use a def get_raw_connection() method, and the developer facing interface should be a def raw_connection property, that calls get_raw_connection()

yeah, was thinking about the property as well, and naming would be more natural as well.

tomchristie · 2019-03-04T12:43:04Z

databases/core.py

@@ -111,6 +111,10 @@ def __init__(
            async for record in connection.iterate(query):
                yield record

+    async def raw_connection(self) -> typing.Any:
+        async with self.connection() as connection:
+            return await connection.raw_connection()


This shouldn't be exposed at the top-level. Ie. You have to acquire the connection first, in order to access .raw_connection

That makes sense and sets explicit flow.

That also gives clarity. As connection already acquired (and it seems reasonable to avoid spawning the raw_connection from the database level acquiring a new one) we can expose already allocated connection object, that makes it a perfect case for the property.

tomchristie · 2019-03-04T12:44:13Z

databases/core.py

@@ -168,6 +172,9 @@ def __init__(self, backend: DatabaseBackend) -> None:
    async def execute_many(self, query: ClauseElement, values: list) -> None:
        await self._connection.execute_many(query, values)

+    async def raw_connection(self) -> typing.Any:
+        return await self._connection.raw_connection()


Let's go with...

@property def raw_connection(self) -> typing.Any: return self._connection.get_raw_connection()

Also let's move the implementation to the very end of the class. That layout feels more natural to me.

Happy with layout, but considering the given context I think I'm looking forward to:

@property def raw_connection(self) -> typing.Any: return self._connection.raw_connection

if there're no objections

That's fine with me, yup. 👍

tomchristie · 2019-03-04T12:47:08Z

tests/test_databases.py

+                raw_connection_2 = await connection_2.raw_connection()
+
+                assert raw_connection_0 is raw_connection_1 is raw_connection_2
+                assert raw_connection_0 is connection_1._connection._connection


Using properties I think we'd really want to change this to something like this...

async with Database(database_url) as database: async with database.connection() as connection_1: async with database.connection() as connection_2: assert connection_1 is connection_2 assert connection_1.raw_connection is connection_2.raw_connection

yes, that would be neat

tomchristie · 2019-03-04T12:49:38Z

tests/test_databases.py

+    async with Database(database_url) as database:
+        async with database.transaction(force_rollback=True):
+            # Get the raw connection
+            con = await database.raw_connection()


Let's use something like connection, raw_connection, or driver rather than the shorthand variable name.

tomchristie · 2019-03-04T13:46:47Z

Looks great! I don't think there's anything else to do on this part right? You happy for it to go in now?

gvbgduh · 2019-03-04T14:11:05Z

Yeah, I'm quite happy now, can't think of anything else as well. Thanks for the feedback!

gvbgduh · 2019-03-04T14:13:26Z

I'll prepare the documentation examples tomorrow, but within the
#57

tomchristie · 2019-03-04T14:16:05Z

@gvbgduh Makes sense - thanks.
Released now, as 0.1.10.

gvbgduh added 2 commits March 3, 2019 23:56

Rough implementation of the raw driver call

0bf7cb7

For encode#9

6b77bae

Get rid of experimental solution and minor fixes.

c4cbf2a

gvbgduh marked this pull request as ready for review March 3, 2019 22:05

gvbgduh added 2 commits March 4, 2019 18:28

Handling raw response from sql drivers (as part of encode#9)

c708bb7

Trim the comment

b75004b

gvbgduh changed the title ~~Rough implementation of the raw driver call~~ An approach for accessing the raw sql driver Mar 4, 2019

gvbgduh commented Mar 4, 2019

View reviewed changes

Some refectoring and polishing for raw_connection encode#9

555e4bf

gvbgduh changed the title ~~An approach for accessing the raw sql driver~~ Support for connection.raw_connection to access the underlying driver's connection interface Mar 4, 2019

gvbgduh changed the title ~~Support for connection.raw_connection to access the underlying driver's connection interface~~ Support for connection.raw_connection Mar 4, 2019

tomchristie requested changes Mar 4, 2019

View reviewed changes

gvbgduh mentioned this pull request Mar 4, 2019

Support raw-string queries #9

Closed

Refactoring: using properties instead of methods

101ab4f

tomchristie approved these changes Mar 4, 2019

View reviewed changes

tomchristie merged commit cf0b368 into encode:master Mar 4, 2019

tomchristie mentioned this pull request Mar 6, 2019

Support returning the rowcount for a given query? #61

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support for `connection.raw_connection` #58

Support for `connection.raw_connection` #58

gvbgduh commented Mar 3, 2019 •

edited

Loading

gvbgduh commented Mar 3, 2019

gvbgduh commented Mar 3, 2019

gvbgduh Mar 4, 2019

tomchristie left a comment

tomchristie Mar 4, 2019

tomchristie Mar 4, 2019

gvbgduh Mar 4, 2019

tomchristie Mar 4, 2019

gvbgduh Mar 4, 2019

gvbgduh Mar 4, 2019

tomchristie Mar 4, 2019

tomchristie Mar 4, 2019

gvbgduh Mar 4, 2019

tomchristie Mar 4, 2019

tomchristie Mar 4, 2019

gvbgduh Mar 4, 2019

tomchristie Mar 4, 2019

tomchristie commented Mar 4, 2019

gvbgduh commented Mar 4, 2019

gvbgduh commented Mar 4, 2019

tomchristie commented Mar 4, 2019

Support for connection.raw_connection #58

Support for connection.raw_connection #58

Conversation

gvbgduh commented Mar 3, 2019 • edited Loading

gvbgduh commented Mar 3, 2019

gvbgduh commented Mar 3, 2019

Choose a reason for hiding this comment

tomchristie left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tomchristie commented Mar 4, 2019

gvbgduh commented Mar 4, 2019

gvbgduh commented Mar 4, 2019

tomchristie commented Mar 4, 2019

Support for `connection.raw_connection` #58

Support for `connection.raw_connection` #58

gvbgduh commented Mar 3, 2019 •

edited

Loading