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 min_size and max_size in postgres backends #129

Closed
wants to merge 11 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
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
10 changes: 9 additions & 1 deletion databases/backends/postgres.py
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
import getpass
import logging
import typing
from collections.abc import Mapping
Expand Down Expand Up @@ -62,7 +63,14 @@ 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(
host=self._database_url.hostname,
port=self._database_url.port or 5432,
user=self._database_url.username or getpass.getuser(),
password=self._database_url.password,
database=self._database_url.database,
**kwargs,
)

async def disconnect(self) -> None:
assert self._pool is not None, "DatabaseBackend is not running"
Expand Down
31 changes: 31 additions & 0 deletions tests/test_connection_options.py
Original file line number Diff line number Diff line change
@@ -1,9 +1,17 @@
"""
Unit tests for the backend connection arguments.
"""
from contextlib import contextmanager
from urllib.parse import urlparse

import pytest

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

POSTGRES_URLS = [url for url in DATABASE_URLS if urlparse(url).scheme == "postgresql"]


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


@contextmanager
def does_not_raise():
yield


urls_with_options = [
(f"{POSTGRES_URLS[0]}?min_size=1&max_size=20", does_not_raise()),
(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

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