Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Use asyncpg dsn #704

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Conversation

mociepka
Copy link

@mociepka mociepka commented Jun 20, 2020

Currently, Gino is overriding an asyncpg connection host, port, user, password, database settings before passing them to the connection pool. In addition url params are not passed to the connection pool.

Because of this behavior developers are not able to pass host, port arguments in the query string. The asyncpg documentation explains that in more details.

In other words url like:

postgresql://postgres@postgres/mydb?host=/cloudsql/project:us-central1:bd/.s.PGSQL.5432

will use the localhost value instead of the socket path. This is how a DSN looks like when you work with the Google Cloud SQL server. Unfortunately, Google examples doesn't work with Gino.

@fantix
Copy link
Member

fantix commented Sep 7, 2020

Cool, thanks for the PR and sorry for the late reply! I'm also suggesting the following changes on your branch:

diff --git a/src/gino/dialects/asyncpg.py b/src/gino/dialects/asyncpg.py
index 795f8d9..f329e69 100644
--- a/src/gino/dialects/asyncpg.py
+++ b/src/gino/dialects/asyncpg.py
@@ -2,6 +2,7 @@ import inspect
 import itertools
 import time
 import warnings
+from copy import copy

 import asyncpg
 from sqlalchemy import util, exc, sql
@@ -244,8 +245,10 @@ class Pool(base.Pool):
                 super().__init__(*pargs, **kwargs)
                 self.baked_queries = {}

+        url = copy(self._url)
+        url.drivername = "postgresql"
         args.update(
-            connection_class=Connection, dsn=str(self._url), loop=self._loop,
+            connection_class=Connection, dsn=str(url), loop=self._loop,
         )

         if self._prebake and self._bakery:
diff --git a/src/gino/strategies.py b/src/gino/strategies.py
index 63bee32..21be879 100644
--- a/src/gino/strategies.py
+++ b/src/gino/strategies.py
@@ -6,7 +6,6 @@ from sqlalchemy.engine.url import make_url
 from sqlalchemy.engine.strategies import EngineStrategy

 from .engine import GinoEngine
-from .dialects.asyncpg import AsyncpgDialect


 class GinoStrategy(EngineStrategy):
@@ -15,7 +14,8 @@ class GinoStrategy(EngineStrategy):
     This strategy is initialized automatically as :mod:`gino` is imported.

     If :func:`sqlalchemy.create_engine` uses ``strategy="gino"``, it will return a
-    :class:`~collections.abc.Coroutine`.
+    :class:`~collections.abc.Coroutine`, and treat URL prefix ``postgresql://`` or
+    ``postgres://`` as ``postgresql+asyncpg://``.
     """

     name = "gino"
@@ -26,13 +26,11 @@ class GinoStrategy(EngineStrategy):
         url = make_url(name_or_url)
         if loop is None:
             loop = asyncio.get_event_loop()
+        if url.drivername in {"postgresql", "postgres"}:
+            url = copy(url)
+            url.drivername = "postgresql+asyncpg"

-        # The postgresql dialect is already taken by the PGDialect_psycopg2
-        # we need to force ourone.
-        if url.drivername in ("postgresql", "postgres"):
-            dialect_cls = AsyncpgDialect
-        else:
-            dialect_cls = url.get_dialect()
+        dialect_cls = url.get_dialect()

         pop_kwarg = kwargs.pop

diff --git a/tests/test_bind.py b/tests/test_bind.py
index 7685fad..2a1d9bf 100644
--- a/tests/test_bind.py
+++ b/tests/test_bind.py
@@ -79,4 +79,10 @@ async def test_bind_url(dsn, driver_name):
     assert url.drivername == driver_name
     await db.set_bind(dsn)
     assert url.drivername == driver_name
+    assert (
+        await db.scalar(
+            "SELECT client_addr FROM pg_stat_activity where pid = pg_backend_pid()"
+        )
+        is not None
+    )
     await db.pop_bind().close()

@fantix fantix added this to the GINO 1.1 milestone Sep 7, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants