From 2e3ccd14d574d38caeb4524a46a59b6b7e9a5b28 Mon Sep 17 00:00:00 2001 From: Alexander Hess Date: Mon, 4 Jan 2021 18:50:26 +0100 Subject: [PATCH] Use globals for the database connection - remove the factory functions for creating engines and sessions - define global engine, connection, and session objects to be used everywhere in the urban_meal_delivery package --- noxfile.py | 10 +++++++++ research/clean_data.ipynb | 3 +-- setup.cfg | 2 +- src/urban_meal_delivery/configuration.py | 5 ++++- src/urban_meal_delivery/db/__init__.py | 5 +++-- src/urban_meal_delivery/db/connection.py | 27 +++++++++++++++-------- tests/db/conftest.py | 28 +++++++++++++++++------- tests/test_config.py | 18 ++++++++++++++- 8 files changed, 74 insertions(+), 24 deletions(-) diff --git a/noxfile.py b/noxfile.py index cd65168..1557320 100644 --- a/noxfile.py +++ b/noxfile.py @@ -254,6 +254,12 @@ def test(session): # For xdoctest, the default arguments are different from pytest. args = posargs or [PACKAGE_IMPORT_NAME] + + # The "TESTING" environment variable forces the global `engine`, `connection`, + # and `session` objects to be set to `None` and avoid any database connection. + # For pytest above this is not necessary as pytest sets this variable itself. + session.env['TESTING'] = 'true' + session.run('xdoctest', '--version') session.run('xdoctest', '--quiet', *args) # --quiet => less verbose output @@ -297,6 +303,10 @@ def docs(session): session.run('poetry', 'install', '--no-dev', external=True) _install_packages(session, 'sphinx', 'sphinx-autodoc-typehints') + # The "TESTING" environment variable forces the global `engine`, `connection`, + # and `session` objects to be set to `None` and avoid any database connection. + session.env['TESTING'] = 'true' + session.run('sphinx-build', DOCS_SRC, DOCS_BUILD) # Verify all external links return 200 OK. session.run('sphinx-build', '-b', 'linkcheck', DOCS_SRC, DOCS_BUILD) diff --git a/research/clean_data.ipynb b/research/clean_data.ipynb index a02ec10..3c7fdee 100644 --- a/research/clean_data.ipynb +++ b/research/clean_data.ipynb @@ -103,8 +103,7 @@ "metadata": {}, "outputs": [], "source": [ - "_engine = db.make_engine()\n", - "connection = _engine.connect()" + "connection = db.connection" ] }, { diff --git a/setup.cfg b/setup.cfg index e68c2da..af25bbf 100644 --- a/setup.cfg +++ b/setup.cfg @@ -134,7 +134,7 @@ per-file-ignores = WPS432, src/urban_meal_delivery/db/__init__.py: # Top-level of a sub-packages is intended to import a lot. - F401, + F401,WPS201, tests/*.py: # Type annotations are not strictly enforced. ANN0, ANN2, diff --git a/src/urban_meal_delivery/configuration.py b/src/urban_meal_delivery/configuration.py index d20320a..0354da6 100644 --- a/src/urban_meal_delivery/configuration.py +++ b/src/urban_meal_delivery/configuration.py @@ -85,7 +85,10 @@ def make_config(env: str = 'production') -> Config: raise ValueError("Must be either 'production' or 'testing'") # Without a PostgreSQL database the package cannot work. - if config.DATABASE_URI is None: + # As pytest sets the "TESTING" environment variable explicitly, + # the warning is only emitted if the code is not run by pytest. + # We see the bad configuration immediately as all "db" tests fail. + if config.DATABASE_URI is None and not os.getenv('TESTING'): warnings.warn('Bad configurartion: no DATABASE_URI set in the environment') return config diff --git a/src/urban_meal_delivery/db/__init__.py b/src/urban_meal_delivery/db/__init__.py index a73f40e..aae8516 100644 --- a/src/urban_meal_delivery/db/__init__.py +++ b/src/urban_meal_delivery/db/__init__.py @@ -3,8 +3,9 @@ from urban_meal_delivery.db.addresses import Address from urban_meal_delivery.db.addresses_pixels import AddressPixelAssociation from urban_meal_delivery.db.cities import City -from urban_meal_delivery.db.connection import make_engine -from urban_meal_delivery.db.connection import make_session_factory +from urban_meal_delivery.db.connection import connection +from urban_meal_delivery.db.connection import engine +from urban_meal_delivery.db.connection import session from urban_meal_delivery.db.couriers import Courier from urban_meal_delivery.db.customers import Customer from urban_meal_delivery.db.grids import Grid diff --git a/src/urban_meal_delivery/db/connection.py b/src/urban_meal_delivery/db/connection.py index 460ef9d..de32ab9 100644 --- a/src/urban_meal_delivery/db/connection.py +++ b/src/urban_meal_delivery/db/connection.py @@ -1,17 +1,26 @@ -"""Provide connection utils for the ORM layer.""" +"""Provide connection utils for the ORM layer. + +This module defines fully configured `engine`, `connection`, and `session` +objects to be used as globals within the `urban_meal_delivery` package. + +If a database is not guaranteed to be available, they are set to `None`. +That is the case on the CI server. +""" + +import os import sqlalchemy as sa -from sqlalchemy import engine from sqlalchemy import orm import urban_meal_delivery -def make_engine() -> engine.Engine: # pragma: no cover - """Provide a configured Engine object.""" - return sa.create_engine(urban_meal_delivery.config.DATABASE_URI) +if os.getenv('TESTING'): + engine = None + connection = None + session = None - -def make_session_factory() -> orm.Session: # pragma: no cover - """Provide a configured Session factory.""" - return orm.sessionmaker(bind=make_engine()) +else: # pragma: no cover + engine = sa.create_engine(urban_meal_delivery.config.DATABASE_URI) + connection = engine.connect() + session = orm.sessionmaker(bind=connection)() diff --git a/tests/db/conftest.py b/tests/db/conftest.py index 8d2e3d1..3d8c676 100644 --- a/tests/db/conftest.py +++ b/tests/db/conftest.py @@ -1,6 +1,7 @@ """Utils for testing the ORM layer.""" import pytest +import sqlalchemy as sa from alembic import command as migrations_cmd from alembic import config as migrations_config from sqlalchemy import orm @@ -26,11 +27,18 @@ def db_connection(request): This ensures that Alembic's migration files are consistent. """ - engine = db.make_engine() + # We need a fresh database connection for each of the two `params`. + # Otherwise, the first test of the parameter run second will fail. + engine = sa.create_engine(config.DATABASE_URI) connection = engine.connect() + # Monkey patch the package's global `engine` and `connection` objects, + # just in case if it is used somewhere in the code base. + db.engine = engine + db.connection = connection + if request.param == 'all_at_once': - engine.execute(f'CREATE SCHEMA {config.CLEAN_SCHEMA};') + connection.execute(f'CREATE SCHEMA {config.CLEAN_SCHEMA};') db.Base.metadata.create_all(connection) else: cfg = migrations_config.Config('alembic.ini') @@ -54,13 +62,17 @@ def db_connection(request): @pytest.fixture def db_session(db_connection): """A SQLAlchemy session that rolls back everything after a test case.""" - # Begin the outer most transaction - # that is rolled back at the end of the test. + # Begin the outermost transaction + # that is rolled back at the end of each test case. transaction = db_connection.begin() - # Create a session bound on the same connection as the transaction. - # Using any other session would not work. - session_factory = orm.sessionmaker() - session = session_factory(bind=db_connection) + + # Create a session bound to the same connection as the `transaction`. + # Using any other session would not result in the roll back. + session = orm.sessionmaker()(bind=db_connection) + + # Monkey patch the package's global `session` object, + # which is used heavily in the code base. + db.session = session try: yield session diff --git a/tests/test_config.py b/tests/test_config.py index 6569161..9251d48 100644 --- a/tests/test_config.py +++ b/tests/test_config.py @@ -36,11 +36,27 @@ def test_database_uri_set(env, monkeypatch): @pytest.mark.parametrize('env', envs) -def test_no_database_uri_set(env, monkeypatch): +def test_no_database_uri_set_with_testing_env_var(env, monkeypatch): """Package does not work without DATABASE_URI set in the environment.""" monkeypatch.setattr(configuration.ProductionConfig, 'DATABASE_URI', None) monkeypatch.setattr(configuration.TestingConfig, 'DATABASE_URI', None) + monkeypatch.setenv('TESTING', 'true') + + with pytest.warns(None) as record: + configuration.make_config(env) + + assert len(record) == 0 # noqa:WPS441,WPS507 + + +@pytest.mark.parametrize('env', envs) +def test_no_database_uri_set_without_testing_env_var(env, monkeypatch): + """Package does not work without DATABASE_URI set in the environment.""" + monkeypatch.setattr(configuration.ProductionConfig, 'DATABASE_URI', None) + monkeypatch.setattr(configuration.TestingConfig, 'DATABASE_URI', None) + + monkeypatch.delenv('TESTING', raising=False) + with pytest.warns(UserWarning, match='no DATABASE_URI'): configuration.make_config(env)