From 826b85097d1a09e9d73b82b8ec1bac275a42bd0e Mon Sep 17 00:00:00 2001 From: DanPaseltiner Date: Wed, 17 May 2023 16:00:43 -0400 Subject: [PATCH 01/14] release v1.0.4 --- pyproject.toml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pyproject.toml b/pyproject.toml index 75b22b0a7e..6a25b30562 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -1,6 +1,6 @@ [tool.poetry] name = "phdi" -version = "v1.0.3" +version = "v1.0.4" description = "Public health data infrastructure Building Blocks is a library to help public health departments work with their data" authors = ["Kenneth Chow ", "Brandon Mader ", "Spencer Kathol ", "Nick Clyde ", "Brady Fausett ", "Marcelle Goggins ", "Bryan Britten ", "Emma Stephenson " , "Gordon Farrell ", "Robert Mitchell "] homepage = "https://github.com/CDCgov/phdi" From 239e54f7d387f3691c0adbeffd72a5cdf323c709 Mon Sep 17 00:00:00 2001 From: DanPaseltiner Date: Thu, 7 Sep 2023 11:05:04 -0400 Subject: [PATCH 02/14] Add function to run pyway including unit test --- containers/record-linkage/app/utils.py | 32 +++++++++++++++++ containers/record-linkage/tests/test_utils.py | 35 +++++++++++++++++++ 2 files changed, 67 insertions(+) create mode 100644 containers/record-linkage/tests/test_utils.py diff --git a/containers/record-linkage/app/utils.py b/containers/record-linkage/app/utils.py index 792424b773..a093408104 100644 --- a/containers/record-linkage/app/utils.py +++ b/containers/record-linkage/app/utils.py @@ -2,6 +2,8 @@ import pathlib from app.config import get_settings from phdi.linkage import DIBBsConnectorClient +import subprocess +from typing import Literal def connect_to_mpi_with_env_vars(): @@ -35,3 +37,33 @@ def load_mpi_env_vars_os(): def read_json_from_assets(filename: str): return json.load(open((pathlib.Path(__file__).parent.parent / "assets" / filename))) + + +def run_pyway( + pyway_command: Literal["info", "validate", "migrate", "import"] +) -> subprocess.CompletedProcess: + """ + Helper function to run the pyway CLI from Python. + + :param pyway_command: The specific pyway command to run. + :return: A subprocess.CompletedProcess object containing the results of the pyway + command. + """ + + migrations_dir = str(pathlib.Path(__file__).parent.parent / "migrations") + settings = get_settings() + pyway_args = [ + f"--database-migration-dir {migrations_dir}", + f"--database-type {settings['mpi_db_type']}", + f"--database-host {settings['mpi_host']}", + f"--database-port {settings['mpi_port']}", + f"--database-name {settings['mpi_dbname']}", + f"--database-username {settings['mpi_user']}", + f"--database-password {settings['mpi_password']}", + ] + full_command = ["pyway", pyway_command] + pyway_args + pyway_response = subprocess.run( + full_command, shell=True, check=True, capture_output=True + ) + + return pyway_response diff --git a/containers/record-linkage/tests/test_utils.py b/containers/record-linkage/tests/test_utils.py new file mode 100644 index 0000000000..8cdb7918b4 --- /dev/null +++ b/containers/record-linkage/tests/test_utils.py @@ -0,0 +1,35 @@ +from app.utils import run_pyway +from unittest import mock +import pathlib + + +@mock.patch("app.utils.get_settings") +@mock.patch("app.utils.subprocess.run") +def test_run_pyway(patched_subprocess, patched_get_settings): + mock_settings = { + "mpi_db_type": "postgres", + "mpi_host": "localhost", + "mpi_port": "5432", + "mpi_dbname": "testdb", + "mpi_user": "postgres", + "mpi_password": "pw", + } + patched_get_settings.return_value = mock_settings + run_pyway("info") + migrations_dir = str(pathlib.Path(__file__).parent.parent / "migrations") + patched_subprocess.assert_called_once_with( + [ + "pyway", + "info", + f"--database-migration-dir {migrations_dir}", + "--database-type postgres", + "--database-host localhost", + "--database-port 5432", + "--database-name testdb", + "--database-username postgres", + "--database-password pw", + ], + shell=True, + check=True, + capture_output=True, + ) From 769a5cc7bb973080c98a1149ae0e563f6fa99bf9 Mon Sep 17 00:00:00 2001 From: DanPaseltiner Date: Thu, 7 Sep 2023 11:06:00 -0400 Subject: [PATCH 03/14] Add pyway and restructure migrations file names to match required naming convention --- containers/record-linkage/app/main.py | 2 +- .../migrations/{tables.ddl => V01_01__initial_schema.sql} | 0 containers/record-linkage/requirements.txt | 1 + 3 files changed, 2 insertions(+), 1 deletion(-) rename containers/record-linkage/migrations/{tables.ddl => V01_01__initial_schema.sql} (100%) diff --git a/containers/record-linkage/app/main.py b/containers/record-linkage/app/main.py index 4b58b1e57c..5cbf5acdfa 100644 --- a/containers/record-linkage/app/main.py +++ b/containers/record-linkage/app/main.py @@ -70,7 +70,7 @@ def run_migrations(): # Run MPI migrations on spin up -run_migrations() +# run_migrations() # Instantiate FastAPI via PHDI's BaseService class app = BaseService( diff --git a/containers/record-linkage/migrations/tables.ddl b/containers/record-linkage/migrations/V01_01__initial_schema.sql similarity index 100% rename from containers/record-linkage/migrations/tables.ddl rename to containers/record-linkage/migrations/V01_01__initial_schema.sql diff --git a/containers/record-linkage/requirements.txt b/containers/record-linkage/requirements.txt index 5d80345b34..420ab0085a 100644 --- a/containers/record-linkage/requirements.txt +++ b/containers/record-linkage/requirements.txt @@ -5,3 +5,4 @@ fastapi==0.96.0 phdi @ git+https://github.com/CDCgov/phdi@main httpx pathlib +pyway \ No newline at end of file From ce01e0634bbb5497a826e4efa5141ba28ab71fee Mon Sep 17 00:00:00 2001 From: DanPaseltiner Date: Fri, 8 Sep 2023 18:45:16 -0400 Subject: [PATCH 04/14] Validate and migrate MPI on start up with logging. --- containers/record-linkage/Dockerfile | 2 +- containers/record-linkage/app/log_config.yml | 34 +++++++++++ containers/record-linkage/app/main.py | 55 +----------------- containers/record-linkage/app/utils.py | 59 +++++++++++++++++++- containers/record-linkage/requirements.txt | 4 +- 5 files changed, 96 insertions(+), 58 deletions(-) create mode 100644 containers/record-linkage/app/log_config.yml diff --git a/containers/record-linkage/Dockerfile b/containers/record-linkage/Dockerfile index f0e5d21709..e01a0f0431 100644 --- a/containers/record-linkage/Dockerfile +++ b/containers/record-linkage/Dockerfile @@ -15,4 +15,4 @@ COPY ./migrations /code/migrations COPY ./assets /code/assets EXPOSE 8080 -CMD uvicorn app.main:app --host 0.0.0.0 --port 8080 \ No newline at end of file +CMD uvicorn app.main:app --host 0.0.0.0 --port 8080 --log-config app/log_config.yml \ No newline at end of file diff --git a/containers/record-linkage/app/log_config.yml b/containers/record-linkage/app/log_config.yml new file mode 100644 index 0000000000..cd209d6028 --- /dev/null +++ b/containers/record-linkage/app/log_config.yml @@ -0,0 +1,34 @@ +version: 1 +disable_existing_loggers: False +formatters: + default: + # "()": uvicorn.logging.DefaultFormatter + format: '%(asctime)s - %(name)s - %(levelname)s - %(message)s' + access: + # "()": uvicorn.logging.AccessFormatter + format: '%(asctime)s - %(name)s - %(levelname)s - %(message)s' +handlers: + default: + formatter: default + class: logging.StreamHandler + stream: ext://sys.stderr + access: + formatter: access + class: logging.StreamHandler + stream: ext://sys.stdout +loggers: + uvicorn.error: + level: INFO + handlers: + - default + propagate: no + uvicorn.access: + level: INFO + handlers: + - access + propagate: no +root: + level: DEBUG + handlers: + - default + propagate: no \ No newline at end of file diff --git a/containers/record-linkage/app/main.py b/containers/record-linkage/app/main.py index 5cbf5acdfa..49c037ba59 100644 --- a/containers/record-linkage/app/main.py +++ b/containers/record-linkage/app/main.py @@ -10,67 +10,16 @@ DIBBS_ENHANCED, ) from pydantic import BaseModel, Field -from psycopg2 import OperationalError, errors from typing import Optional from app.utils import ( connect_to_mpi_with_env_vars, load_mpi_env_vars_os, read_json_from_assets, + run_migrations ) -import psycopg2 -import sys -# https://kb.objectrocket.com/postgresql/python-error-handling-with-the-psycopg2-postgresql-adapter-645 -def print_psycopg2_exception(err): - # get details about the exception - err_type, _, traceback = sys.exc_info() - - # get the line number when exception occured - line_num = traceback.tb_lineno - - # print the connect() error - print("\npsycopg2 ERROR:", err, "on line number:", line_num) - print("psycopg2 traceback:", traceback, "-- type:", err_type) - - # psycopg2 extensions.Diagnostics object attribute - print("\nextensions.Diagnostics:", err.diag) - - # print the pgcode and pgerror exceptions - print("pgerror:", err.pgerror) - print("pgcode:", err.pgcode, "\n") - - -def run_migrations(): - dbname, user, password, host = load_mpi_env_vars_os() - try: - connection = psycopg2.connect( - dbname=dbname, - user=user, - password=password, - host=host, - ) - except OperationalError as err: - # pass exception to function - print_psycopg2_exception(err) - - # set the connection to 'None' in case of error - connection = None - if connection is not None: - try: - with connection.cursor() as cursor: - cursor.execute( - open( - Path(__file__).parent.parent / "migrations" / "tables.ddl", "r" - ).read() - ) - except errors.InFailedSqlTransaction as err: - # pass exception to function - print_psycopg2_exception(err) - - -# Run MPI migrations on spin up -# run_migrations() +run_migrations() # Instantiate FastAPI via PHDI's BaseService class app = BaseService( diff --git a/containers/record-linkage/app/utils.py b/containers/record-linkage/app/utils.py index a093408104..451bbad803 100644 --- a/containers/record-linkage/app/utils.py +++ b/containers/record-linkage/app/utils.py @@ -4,6 +4,7 @@ from phdi.linkage import DIBBsConnectorClient import subprocess from typing import Literal +import logging def connect_to_mpi_with_env_vars(): @@ -49,7 +50,10 @@ def run_pyway( :return: A subprocess.CompletedProcess object containing the results of the pyway command. """ + + logger = logging.getLogger(__name__) + # Prepare the pyway command. migrations_dir = str(pathlib.Path(__file__).parent.parent / "migrations") settings = get_settings() pyway_args = [ @@ -61,9 +65,58 @@ def run_pyway( f"--database-username {settings['mpi_user']}", f"--database-password {settings['mpi_password']}", ] + full_command = ["pyway", pyway_command] + pyway_args - pyway_response = subprocess.run( - full_command, shell=True, check=True, capture_output=True - ) + full_command = " ".join(full_command) + + # Attempt to run the pyway command. + try: + pyway_response = subprocess.run( + full_command, shell=True, check=True, capture_output=True + ) + except subprocess.CalledProcessError as error: + error_message = error.output.decode("utf-8") + if ( + "ERROR: no migrations applied yet, no validation necessary." + in error_message + ): + logger.warning(error_message) + return subprocess.CompletedProcess( + args= full_command, + returncode=0, + stdout=None, + stderr=error_message, + ) + else: + logger.error(error_message) + raise error + + logger.info(pyway_response.stdout.decode("utf-8")) return pyway_response + + +def run_migrations(): + """ + Use the pyway CLI to ensure that the MPI database is up to date with the latest + migrations. + """ + logger = logging.getLogger(__name__) + logger.info("Validating MPI database schema...") + validation_response = run_pyway("validate") + + if validation_response.returncode == 0: + logger.info("MPI database schema validations successful.") + + logger.info("Migrating MPI database...") + migrations_response = run_pyway("migrate") + + if migrations_response.returncode == 0: + logger.info("MPI database migrations successful.") + else: + logger.error("MPI database migrations failed.") + raise Exception(migrations_response.stderr.decode("utf-8")) + + else: + logger.error("MPI database schema validations failed.") + raise Exception(validation_response.stderr.decode("utf-8")) diff --git a/containers/record-linkage/requirements.txt b/containers/record-linkage/requirements.txt index 420ab0085a..0a08ef9d14 100644 --- a/containers/record-linkage/requirements.txt +++ b/containers/record-linkage/requirements.txt @@ -5,4 +5,6 @@ fastapi==0.96.0 phdi @ git+https://github.com/CDCgov/phdi@main httpx pathlib -pyway \ No newline at end of file +pyway +psycopg2-binary==2.9.7 +tabulate \ No newline at end of file From b5ae9ee5a2531edf0d9cce91fceafcfee01cb252 Mon Sep 17 00:00:00 2001 From: DanPaseltiner Date: Fri, 8 Sep 2023 18:49:27 -0400 Subject: [PATCH 05/14] Additional comments on pyway behavior --- containers/record-linkage/app/main.py | 2 +- containers/record-linkage/app/utils.py | 16 ++++++++++------ 2 files changed, 11 insertions(+), 7 deletions(-) diff --git a/containers/record-linkage/app/main.py b/containers/record-linkage/app/main.py index 49c037ba59..2c2ece3b96 100644 --- a/containers/record-linkage/app/main.py +++ b/containers/record-linkage/app/main.py @@ -15,7 +15,7 @@ connect_to_mpi_with_env_vars, load_mpi_env_vars_os, read_json_from_assets, - run_migrations + run_migrations, ) diff --git a/containers/record-linkage/app/utils.py b/containers/record-linkage/app/utils.py index 451bbad803..f75ae05687 100644 --- a/containers/record-linkage/app/utils.py +++ b/containers/record-linkage/app/utils.py @@ -50,7 +50,7 @@ def run_pyway( :return: A subprocess.CompletedProcess object containing the results of the pyway command. """ - + logger = logging.getLogger(__name__) # Prepare the pyway command. @@ -76,13 +76,17 @@ def run_pyway( ) except subprocess.CalledProcessError as error: error_message = error.output.decode("utf-8") + # Pyway validate returns an error if no migrations have been applied yet. + # This is expected behavior, so we can ignore this error and continue onto + # the migrations with pyway migrate. We'll encounter this error when we + # first deploy the service with a fresh database. if ( "ERROR: no migrations applied yet, no validation necessary." in error_message ): logger.warning(error_message) return subprocess.CompletedProcess( - args= full_command, + args=full_command, returncode=0, stdout=None, stderr=error_message, @@ -104,19 +108,19 @@ def run_migrations(): logger = logging.getLogger(__name__) logger.info("Validating MPI database schema...") validation_response = run_pyway("validate") - + if validation_response.returncode == 0: logger.info("MPI database schema validations successful.") - + logger.info("Migrating MPI database...") migrations_response = run_pyway("migrate") - + if migrations_response.returncode == 0: logger.info("MPI database migrations successful.") else: logger.error("MPI database migrations failed.") raise Exception(migrations_response.stderr.decode("utf-8")) - + else: logger.error("MPI database schema validations failed.") raise Exception(validation_response.stderr.decode("utf-8")) From 3d4f4e7087c5c486f87db1b71c15e3af33acc981 Mon Sep 17 00:00:00 2001 From: DanPaseltiner Date: Fri, 8 Sep 2023 18:54:40 -0400 Subject: [PATCH 06/14] Update description.md --- containers/record-linkage/description.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/containers/record-linkage/description.md b/containers/record-linkage/description.md index 05dc52eac1..c826a6571b 100644 --- a/containers/record-linkage/description.md +++ b/containers/record-linkage/description.md @@ -43,7 +43,7 @@ We recommend running the record linkage service from a container, but if that is 4. Make a fresh virtual environment with `python -m venv .venv`. 5. Activate the virtual environment with `source .venv/bin/activate` (MacOS and Linux), `venv\Scripts\activate` (Windows Command Prompt), or `.venv\Scripts\Activate.ps1` (Windows PowerShell). 5. Install all Python dependencies for the record linkage service with `pip install -r requirements.txt` into your virtual environment. -6. Run the FHIR Converter on `localhost:8080` with `python -m uvicorn app.main:app --host 0.0.0.0 --port 8080`. +6. Run the record linkage service on `localhost:8080` with `python -m uvicorn app.main:app --host 0.0.0.0 --port 8080 --log-config app/log_config.yml`. ### Building the Docker Image From 589d5cc7a36fff900a2878ae67fa7b2d11b05216 Mon Sep 17 00:00:00 2001 From: DanPaseltiner Date: Fri, 8 Sep 2023 19:00:30 -0400 Subject: [PATCH 07/14] Update test_run_pyway() --- containers/record-linkage/tests/test_utils.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/containers/record-linkage/tests/test_utils.py b/containers/record-linkage/tests/test_utils.py index 8cdb7918b4..e11e79f1ce 100644 --- a/containers/record-linkage/tests/test_utils.py +++ b/containers/record-linkage/tests/test_utils.py @@ -18,7 +18,7 @@ def test_run_pyway(patched_subprocess, patched_get_settings): run_pyway("info") migrations_dir = str(pathlib.Path(__file__).parent.parent / "migrations") patched_subprocess.assert_called_once_with( - [ + " ".join([ "pyway", "info", f"--database-migration-dir {migrations_dir}", @@ -28,7 +28,7 @@ def test_run_pyway(patched_subprocess, patched_get_settings): "--database-name testdb", "--database-username postgres", "--database-password pw", - ], + ]), shell=True, check=True, capture_output=True, From 0c04e35bc2b41a8f2535d7aeb52bd695c740cc7f Mon Sep 17 00:00:00 2001 From: DanPaseltiner Date: Fri, 8 Sep 2023 19:46:53 -0400 Subject: [PATCH 08/14] Test regular failure mode run_pyway() --- containers/record-linkage/tests/test_utils.py | 91 ++++++++++++++----- 1 file changed, 69 insertions(+), 22 deletions(-) diff --git a/containers/record-linkage/tests/test_utils.py b/containers/record-linkage/tests/test_utils.py index e11e79f1ce..4ffec3ac3d 100644 --- a/containers/record-linkage/tests/test_utils.py +++ b/containers/record-linkage/tests/test_utils.py @@ -1,34 +1,81 @@ from app.utils import run_pyway from unittest import mock import pathlib +from typing import Literal +import pytest +import subprocess + + +MOCK_SETTINGS = { + "mpi_db_type": "postgres", + "mpi_host": "localhost", + "mpi_port": "5432", + "mpi_dbname": "testdb", + "mpi_user": "postgres", + "mpi_password": "pw", +} + +def make_pyway_command(pyway_command: Literal["info", "validate", "migrate", "import"]) -> str: + """ + Helper function for tests that require a pyway command. + :param pyway_command: The specific pyway command to run. + :return: A string containing the pyway command. + """ + + migrations_dir = str(pathlib.Path(__file__).parent.parent / "migrations") + + pyway_command = " ".join([ + "pyway", + pyway_command, + f"--database-migration-dir {migrations_dir}", + f"--database-type {MOCK_SETTINGS['mpi_db_type']}", + f"--database-host {MOCK_SETTINGS['mpi_host']}", + f"--database-port {MOCK_SETTINGS['mpi_port']}", + f"--database-name {MOCK_SETTINGS['mpi_dbname']}", + f"--database-username {MOCK_SETTINGS['mpi_user']}", + f"--database-password {MOCK_SETTINGS['mpi_password']}", + ]) + return pyway_command @mock.patch("app.utils.get_settings") @mock.patch("app.utils.subprocess.run") -def test_run_pyway(patched_subprocess, patched_get_settings): - mock_settings = { - "mpi_db_type": "postgres", - "mpi_host": "localhost", - "mpi_port": "5432", - "mpi_dbname": "testdb", - "mpi_user": "postgres", - "mpi_password": "pw", - } - patched_get_settings.return_value = mock_settings +def test_run_pyway_success(patched_subprocess, patched_get_settings): + """ + Test the happy path in run_pyway() + """ + global MOCK_SETTINGS + patched_get_settings.return_value = MOCK_SETTINGS run_pyway("info") - migrations_dir = str(pathlib.Path(__file__).parent.parent / "migrations") + pyway_command = make_pyway_command("info") patched_subprocess.assert_called_once_with( - " ".join([ - "pyway", - "info", - f"--database-migration-dir {migrations_dir}", - "--database-type postgres", - "--database-host localhost", - "--database-port 5432", - "--database-name testdb", - "--database-username postgres", - "--database-password pw", - ]), + pyway_command, + shell=True, + check=True, + capture_output=True, + ) + + +@mock.patch("app.utils.get_settings") +@mock.patch("app.utils.subprocess.run") +def test_run_pyway_failure(patched_subprocess, patched_get_settings): + """ + The general failure mode of run_pyway() when a subprocess.CalledProcessError is + raised. + """ + + global MOCK_SETTINGS + patched_get_settings.return_value = MOCK_SETTINGS + output = mock.Mock() + output.decode.return_value = "test" + patched_subprocess.side_effect = subprocess.CalledProcessError( + returncode=1, cmd="test", stderr="test", output=output + ) + pyway_command = make_pyway_command("info") + with pytest.raises(subprocess.CalledProcessError): + run_pyway("info") + patched_subprocess.assert_called_once_with( + pyway_command, shell=True, check=True, capture_output=True, From 8aa461e68e4892293275a9357bf94707ed49427b Mon Sep 17 00:00:00 2001 From: DanPaseltiner Date: Fri, 8 Sep 2023 19:52:03 -0400 Subject: [PATCH 09/14] Test special case of pyway validate failure when no migrations have been applied --- containers/record-linkage/app/utils.py | 1 + containers/record-linkage/tests/test_utils.py | 42 ++++++++++++++++--- 2 files changed, 38 insertions(+), 5 deletions(-) diff --git a/containers/record-linkage/app/utils.py b/containers/record-linkage/app/utils.py index f75ae05687..967deaf178 100644 --- a/containers/record-linkage/app/utils.py +++ b/containers/record-linkage/app/utils.py @@ -76,6 +76,7 @@ def run_pyway( ) except subprocess.CalledProcessError as error: error_message = error.output.decode("utf-8") + # Pyway validate returns an error if no migrations have been applied yet. # This is expected behavior, so we can ignore this error and continue onto # the migrations with pyway migrate. We'll encounter this error when we diff --git a/containers/record-linkage/tests/test_utils.py b/containers/record-linkage/tests/test_utils.py index 4ffec3ac3d..1d467e6051 100644 --- a/containers/record-linkage/tests/test_utils.py +++ b/containers/record-linkage/tests/test_utils.py @@ -15,16 +15,20 @@ "mpi_password": "pw", } -def make_pyway_command(pyway_command: Literal["info", "validate", "migrate", "import"]) -> str: + +def make_pyway_command( + pyway_command: Literal["info", "validate", "migrate", "import"] +) -> str: """ Helper function for tests that require a pyway command. :param pyway_command: The specific pyway command to run. - :return: A string containing the pyway command. + :return: A string containing the pyway command. """ migrations_dir = str(pathlib.Path(__file__).parent.parent / "migrations") - pyway_command = " ".join([ + pyway_command = " ".join( + [ "pyway", pyway_command, f"--database-migration-dir {migrations_dir}", @@ -34,7 +38,8 @@ def make_pyway_command(pyway_command: Literal["info", "validate", "migrate", "im f"--database-name {MOCK_SETTINGS['mpi_dbname']}", f"--database-username {MOCK_SETTINGS['mpi_user']}", f"--database-password {MOCK_SETTINGS['mpi_password']}", - ]) + ] + ) return pyway_command @@ -60,7 +65,7 @@ def test_run_pyway_success(patched_subprocess, patched_get_settings): @mock.patch("app.utils.subprocess.run") def test_run_pyway_failure(patched_subprocess, patched_get_settings): """ - The general failure mode of run_pyway() when a subprocess.CalledProcessError is + The general failure mode of run_pyway() when a subprocess.CalledProcessError is raised. """ @@ -80,3 +85,30 @@ def test_run_pyway_failure(patched_subprocess, patched_get_settings): check=True, capture_output=True, ) + + +@mock.patch("app.utils.get_settings") +@mock.patch("app.utils.subprocess.run") +def test_run_pyway_no_migrations(patched_subprocess, patched_get_settings): + """ + Test the special case where 'pyway validate' returns an error if no migrations have + been applied yet. + """ + + global MOCK_SETTINGS + patched_get_settings.return_value = MOCK_SETTINGS + output = mock.Mock() + output.decode.return_value = ( + "ERROR: no migrations applied yet, no validation necessary." + ) + patched_subprocess.side_effect = subprocess.CalledProcessError( + returncode=1, cmd="test", stderr="test", output=output + ) + pyway_command = make_pyway_command("validate") + run_pyway("validate") + patched_subprocess.assert_called_once_with( + pyway_command, + shell=True, + check=True, + capture_output=True, + ) From d1c01940216b7e69b0cc6ae5f0e038b82125d5c6 Mon Sep 17 00:00:00 2001 From: DanPaseltiner Date: Sat, 9 Sep 2023 01:00:44 -0400 Subject: [PATCH 10/14] Test run_migrations() happy path. --- containers/record-linkage/app/main.py | 4 ++-- containers/record-linkage/app/utils.py | 4 ++-- containers/record-linkage/tests/test_utils.py | 15 ++++++++++++++- 3 files changed, 18 insertions(+), 5 deletions(-) diff --git a/containers/record-linkage/app/main.py b/containers/record-linkage/app/main.py index 2c2ece3b96..36c15bf623 100644 --- a/containers/record-linkage/app/main.py +++ b/containers/record-linkage/app/main.py @@ -18,7 +18,7 @@ run_migrations, ) - +# Ensure MPI is configured as expected. run_migrations() # Instantiate FastAPI via PHDI's BaseService class @@ -29,7 +29,7 @@ ).start() -# Request and and response models +# Request and response models class LinkRecordInput(BaseModel): """ Schema for requests to the /link-record endpoint. diff --git a/containers/record-linkage/app/utils.py b/containers/record-linkage/app/utils.py index 967deaf178..363f6e8580 100644 --- a/containers/record-linkage/app/utils.py +++ b/containers/record-linkage/app/utils.py @@ -77,8 +77,8 @@ def run_pyway( except subprocess.CalledProcessError as error: error_message = error.output.decode("utf-8") - # Pyway validate returns an error if no migrations have been applied yet. - # This is expected behavior, so we can ignore this error and continue onto + # Pyway validate returns an error if no migrations have been applied yet. + # This is expected behavior, so we can ignore this error and continue onto # the migrations with pyway migrate. We'll encounter this error when we # first deploy the service with a fresh database. if ( diff --git a/containers/record-linkage/tests/test_utils.py b/containers/record-linkage/tests/test_utils.py index 1d467e6051..35624507a2 100644 --- a/containers/record-linkage/tests/test_utils.py +++ b/containers/record-linkage/tests/test_utils.py @@ -1,4 +1,4 @@ -from app.utils import run_pyway +from app.utils import run_pyway, run_migrations from unittest import mock import pathlib from typing import Literal @@ -112,3 +112,16 @@ def test_run_pyway_no_migrations(patched_subprocess, patched_get_settings): check=True, capture_output=True, ) + +@mock.patch("app.utils.run_pyway") +def test_run_migrations_success(patched_run_pyway): + """ + Test the happy path in run_migrations() + """ + validation_response = mock.Mock() + validation_response.returncode = 0 + migration_response = mock.Mock() + migration_response.returncode = 0 + patched_run_pyway.side_effect = [validation_response, migration_response] + run_migrations() + patched_run_pyway.assert_has_calls([mock.call("validate"), mock.call("migrate")]) \ No newline at end of file From 47b16b63fbd26ca0277a127c4b4d617d4ae18e0c Mon Sep 17 00:00:00 2001 From: DanPaseltiner Date: Sat, 9 Sep 2023 01:07:27 -0400 Subject: [PATCH 11/14] Test run_migrations() failure modes. --- containers/record-linkage/tests/test_utils.py | 30 +++++++++++++++++++ 1 file changed, 30 insertions(+) diff --git a/containers/record-linkage/tests/test_utils.py b/containers/record-linkage/tests/test_utils.py index 35624507a2..0de32f3c8f 100644 --- a/containers/record-linkage/tests/test_utils.py +++ b/containers/record-linkage/tests/test_utils.py @@ -124,4 +124,34 @@ def test_run_migrations_success(patched_run_pyway): migration_response.returncode = 0 patched_run_pyway.side_effect = [validation_response, migration_response] run_migrations() + patched_run_pyway.assert_has_calls([mock.call("validate"), mock.call("migrate")]) + + +@mock.patch("app.utils.run_pyway") +def test_run_migrations_validation_failure(patched_run_pyway): + """ + Test the case where the validation step fails in run_migrations(). + """ + validation_response = mock.Mock() + validation_response.returncode = 1 + migration_response = mock.Mock() + migration_response.returncode = 0 + patched_run_pyway.side_effect = [validation_response, migration_response] + with pytest.raises(Exception): + run_migrations() + patched_run_pyway.assert_called_once_with("validate") + + +@mock.patch("app.utils.run_pyway") +def test_run_migrations_migration_failure(patched_run_pyway): + """ + Test the case where the migration step fails in run_migrations(). + """ + validation_response = mock.Mock() + validation_response.returncode = 0 + migration_response = mock.Mock() + migration_response.returncode = 1 + patched_run_pyway.side_effect = [validation_response, migration_response] + with pytest.raises(Exception): + run_migrations() patched_run_pyway.assert_has_calls([mock.call("validate"), mock.call("migrate")]) \ No newline at end of file From 785fba510a68661892554f8e7d8dc4082caf8d4b Mon Sep 17 00:00:00 2001 From: DanPaseltiner Date: Sat, 9 Sep 2023 01:08:38 -0400 Subject: [PATCH 12/14] black --- containers/record-linkage/tests/test_utils.py | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/containers/record-linkage/tests/test_utils.py b/containers/record-linkage/tests/test_utils.py index 0de32f3c8f..ad71829407 100644 --- a/containers/record-linkage/tests/test_utils.py +++ b/containers/record-linkage/tests/test_utils.py @@ -113,6 +113,7 @@ def test_run_pyway_no_migrations(patched_subprocess, patched_get_settings): capture_output=True, ) + @mock.patch("app.utils.run_pyway") def test_run_migrations_success(patched_run_pyway): """ @@ -154,4 +155,4 @@ def test_run_migrations_migration_failure(patched_run_pyway): patched_run_pyway.side_effect = [validation_response, migration_response] with pytest.raises(Exception): run_migrations() - patched_run_pyway.assert_has_calls([mock.call("validate"), mock.call("migrate")]) \ No newline at end of file + patched_run_pyway.assert_has_calls([mock.call("validate"), mock.call("migrate")]) From 2088d33bc1d5f68d26d1dd2f21c881edf7b6c058 Mon Sep 17 00:00:00 2001 From: DanPaseltiner Date: Sat, 9 Sep 2023 01:17:11 -0400 Subject: [PATCH 13/14] remove unused util function --- containers/record-linkage/app/main.py | 1 - 1 file changed, 1 deletion(-) diff --git a/containers/record-linkage/app/main.py b/containers/record-linkage/app/main.py index 36c15bf623..a8a4f972e5 100644 --- a/containers/record-linkage/app/main.py +++ b/containers/record-linkage/app/main.py @@ -13,7 +13,6 @@ from typing import Optional from app.utils import ( connect_to_mpi_with_env_vars, - load_mpi_env_vars_os, read_json_from_assets, run_migrations, ) From 5c5352a5d39f6482adb0bd561d8211f5bd4a466e Mon Sep 17 00:00:00 2001 From: DanPaseltiner Date: Mon, 11 Sep 2023 11:31:32 -0400 Subject: [PATCH 14/14] Updates to get unit tests working with additional Pyway table. --- containers/record-linkage/tests/test_record_linkage.py | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/containers/record-linkage/tests/test_record_linkage.py b/containers/record-linkage/tests/test_record_linkage.py index e6b31eea13..d0446f2f6a 100644 --- a/containers/record-linkage/tests/test_record_linkage.py +++ b/containers/record-linkage/tests/test_record_linkage.py @@ -56,6 +56,8 @@ def clean_up_db(): dbconn.commit() cursor.execute("DROP TABLE IF EXISTS person") dbconn.commit() + cursor.execute("DROP TABLE IF EXISTS pyway") + dbconn.commit() cursor.close() dbconn.close() @@ -113,9 +115,9 @@ def test_linkage_invalid_db_type(): def test_linkage_success(): # Clear MPI ahead of testing - clean_up_db() - run_migrations() - test_bundle = load_test_bundle() + # clean_up_db() + # run_migrations() + # test_bundle = load_test_bundle() set_mpi_env_vars() clean_up_db()