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.

Sign up for GitHub

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

Jump to bottom

Use min_size and max_size in postgres backends #129

Closed
wants to merge 11 commits into from
Closed

Use min_size and max_size in postgres backends #129

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
36d54c8
Added failing test
euri10 Jul 23, 2019
aaa0a4b
Stripping query params
euri10 Jul 23, 2019
8ee141b
Using connect/disconnect without the context manager
euri10 Jul 23, 2019
0347c50
Please mypy
euri10 Jul 23, 2019
17c7d9a
Lint
euri10 Jul 23, 2019
54d8d33
Moved POSTGRES_URLS where it's used
euri10 Jul 23, 2019
c341a77
Lint
euri10 Jul 23, 2019
3fd7fba
Using ExitStack
euri10 Jul 23, 2019
c6ab59e
Using own contextmanager
euri10 Jul 23, 2019
32d9776
Used @gvbgduh for postgres create_pool
euri10 Jul 24, 2019
6e11017
Deleted old lines
euri10 Jul 24, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 4 additions & 1 deletion databases/backends/postgres.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
import logging
import typing
from collections.abc import Mapping
from urllib.parse import urlparse

import asyncpg
from sqlalchemy.dialects.postgresql import pypostgresql
Expand Down Expand Up @@ -62,7 +63,9 @@ def _get_connection_kwargs(self) -> dict:
async def connect(self) -> None:
assert self._pool is None, "DatabaseBackend is already running"
kwargs = self._get_connection_kwargs()
self._pool = await asyncpg.create_pool(str(self._database_url), **kwargs)
self._pool = await asyncpg.create_pool(
str(urlparse(self._database_url._url)._replace(query="").geturl()), **kwargs
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Probably something like this instead:

url = str(self._database_url.replace(query=''))
self._pool = await asyncpg.create_pool(url, **kwargs)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

Copy link
Member

@gvbgduh gvbgduh Jul 23, 2019 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@tomchristie @euri10 sorry for jumping into, but it'll break other things. The matter is that the query is the only way to pass other params to asyncpg like socket or host that breaks the dsn parsing or caching params, etc.
self._get_connection_kwargs() will return only pool size details and the flag for ssl, but the rest will erased.
I think we should catch all query params in self._get_connection_kwargs() or use an approach used in other backends -

    async def connect(self) -> None:
        assert self._pool is None, "DatabaseBackend is already running"
        kwargs = self._get_connection_kwargs()
        self._pool = await aiomysql.create_pool(
            host=self._database_url.hostname,
            port=self._database_url.port or 3306,
            user=self._database_url.username or getpass.getuser(),
            password=self._database_url.password,
            db=self._database_url.database,
            autocommit=True,
            **kwargs,
        )

The second one seems as a better option as its aligned with the unified approach.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

does that look better @gvbgduh ?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it definitely does @euri10!
But what I missed is that params from query are coming through either.

>>> from databases import DatabaseURL
>>> url = DatabaseURL('pg://user:pass@/db?host=host&port=5432')
>>> url.password
'pass'
>>> url.username
'user'
>>> url.database
'db'
>>> url.port
>>> url.hostname
>>> url.options
{'host': 'host', 'port': '5432'}

_get_connection_kwargs() accesses url.options, but looks for specific params, and DatabaseURL doesn't look in query for missing things.

It might be reasonable to tweak DatabaseURL to look into options/query if it can't find params in the main url, at least for {username, password, database, port, hostname}.

If @tomchristie has no objections

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it's true that _get_connection_kwargs currently removes anything that is not min_size, max_size or ssl
with an dsn like f"{POSTGRES_URLS[0]}?min_size=1&max_size=2&sslmode=require" for instance the sslmode is lost

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

actually, it should pass it as of line https://github.com/encode/databases/blob/master/databases/backends/postgres.py#L58, but those options come from the Database instantiation - https://github.com/encode/databases/blob/master/databases/core.py#L32.
So it'll broadcast provided kwargs as is to the DB driver.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can't see the kwarg sslmode for the connection or for the pool in asyncpg, so you should probably use:

ssl – Pass True or an ssl.SSLContext instance to require an SSL connection. If True, a default SSL context returned by ssl.create_default_context() will be used.

but in general you should be able to provide it as

database = Database('pg://user:pass@/db?host=host&port=5432', server_settings={...}, ...)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

got it, needs to be passed from Database
for reference sslmode is defined here in asyncpg and used in those tests

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeah, I see, thanks!

)

async def disconnect(self) -> None:
assert self._pool is not None, "DatabaseBackend is not running"
Expand Down
23 changes: 23 additions & 0 deletions tests/test_connection_options.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,9 +1,14 @@
"""
Unit tests for the backend connection arguments.
"""
from contextlib import suppress

import pytest

from databases import Database
from databases.backends.mysql import MySQLBackend
from databases.backends.postgres import PostgresBackend
from tests.test_databases import POSTGRES_URLS, async_adapter


def test_postgres_pool_size():
Expand All @@ -30,6 +35,24 @@ def test_postgres_explicit_ssl():
assert kwargs == {"ssl": True}


urls_with_options = [
(f"{POSTGRES_URLS[0]}?min_size=1&max_size=20", suppress()),
(f"{POSTGRES_URLS[0]}?min_size=0&max_size=0", pytest.raises(ValueError)),
(f"{POSTGRES_URLS[0]}?min_size=10&max_size=0", pytest.raises(ValueError)),
]


@pytest.mark.parametrize("database_url, expectation", urls_with_options)
@async_adapter
async def test_postgres_pool_size_connect(database_url, expectation):
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we could do with structuring this a bit more simply. Not sure exactly how, but I'd rather have something a bit more plain.

The empty supress is used in a non-obvious way, and POSTGRES_URLS is defined at a long distance from the place it's used.

Copy link
Member Author

@euri10 euri10 Jul 23, 2019 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I moved POSTGRES_URLS where it's used.
as for supress we can maybe use a more explicit does_not_raise() that is either its own contextmanager (c6ab59e)
either directly an alias for ExitStack (3fd7fba)
it depends what version of python need to be supported (http://doc.pytest.org/en/latest/example/parametrize.html#parametrizing-conditional-raising)

with expectation:
database = Database(database_url)
await database.connect()
assert database.is_connected
await database.disconnect()
assert not database.is_connected


def test_mysql_pool_size():
backend = MySQLBackend("mysql://localhost/database?min_size=1&max_size=20")
kwargs = backend._get_connection_kwargs()
Expand Down
2 changes: 2 additions & 0 deletions tests/test_databases.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@
import decimal
import functools
import os
from urllib.parse import urlparse

import pytest
import sqlalchemy
Expand All @@ -12,6 +13,7 @@
assert "TEST_DATABASE_URLS" in os.environ, "TEST_DATABASE_URLS is not set."

DATABASE_URLS = [url.strip() for url in os.environ["TEST_DATABASE_URLS"].split(",")]
POSTGRES_URLS = [url for url in DATABASE_URLS if urlparse(url).scheme == "postgresql"]


class MyEpochType(sqlalchemy.types.TypeDecorator):
Expand Down