From 000bced9f6a9d1517d2196cbfd9b4021c0c7f329 Mon Sep 17 00:00:00 2001 From: Loren Yu Date: Tue, 21 Feb 2023 14:46:21 -0800 Subject: [PATCH] Use Flask custom CLI commands for custom scripts (#138) ## Changes * Use Flask custom CLI commands for custom scripts * Remove main() entry point from create_user_csv service * Remove script_utils * Rename api.route package to api.api, and organize all things related to blueprints together: * X_blueprint.py * X_commands.py * X_routes.py * X_schemas.py * Rewrite test_create_user_csv to use test_cli_runner rather than only test the service * Add migration to cascade on delete * Rewrite test_create_user_csv to truncate the user table rather than requiring an isolated db schema * Remove now unused isolated_db fixture * Remove unused transaction rollback functionality in db_session test fixture * Rename /user REST resource to /users Other changes * Organize Makefile commands: * move setup-local to top * group openapi-spec with other CLI commands) * Install pytest-lazy-fixture dev dependency * Move empty_schema fixture to test_migrations, the only place it's used ## Context This PR consolidates the way we run the API server and the way we run background scripts to both use Flask's create_app factory. This removes the need to have two separate main() entrypoints, two separate ways to initialize the database, logger, etc. To do this, we leverage [Flask's built-in method of creating custom CLI commands](https://flask.palletsprojects.com/en/2.2.x/cli/#custom-commands) which itself is based on the [click library](https://click.palletsprojects.com/en/8.1.x/arguments/), a popular library for building Python command line applications that's more powerful than `argparse`. This allows us to remove the script_utils file. This PR also renames the `api.route` package to `api.api` since we want to add "commands" to the user blueprint, so "route" is no longer an appropriate name that applies to everything in the blueprint. We could also consider other names like `api.components` or `api.blueprints`, and/or we could consider renaming the top level source code folder to something like `src` rather than `api`. Finally, we made test_create_user_csv test things at using the test_cli_runner to be closer to the way we test routes using the test_client. This approach has more end-to-end test coverage. Other tangential changes include making REST resources use plural names to follow REST resource naming conventions. --- app/Makefile | 44 ++++--- app/api/{route => api}/__init__.py | 0 app/api/{route => api}/healthcheck.py | 4 +- app/api/{route => api}/response.py | 2 +- app/api/{route => api}/schemas/__init__.py | 0 .../{route => api}/schemas/request_schema.py | 0 .../{route => api}/schemas/response_schema.py | 2 +- app/api/api/users/__init__.py | 10 ++ app/api/api/users/user_blueprint.py | 3 + app/api/api/users/user_commands.py | 34 +++++ .../users/user_routes.py} | 44 ++----- .../schemas => api/users}/user_schemas.py | 11 +- app/api/app.py | 6 +- .../versions/2023_02_21_cascade_on_delete.py | 30 +++++ app/api/db/models/user_models.py | 6 +- app/api/route/route_utils.py | 21 --- app/api/scripts/__init__.py | 0 app/api/scripts/util/__init__.py | 0 app/api/scripts/util/script_util.py | 41 ------ app/api/services/users/__init__.py | 2 + app/api/services/users/create_user_csv.py | 21 --- app/api/util/file_util.py | 61 --------- app/api/util/string_utils.py | 16 +-- app/local.env | 5 - app/openapi.yml | 6 +- app/poetry.lock | 29 +++- app/pyproject.toml | 2 +- app/tests/api/db/models/factories.py | 4 +- app/tests/api/db/models/test_factories.py | 13 +- app/tests/api/db/test_migrations.py | 15 +++ app/tests/api/route/test_user_route.py | 26 ++-- app/tests/api/scripts/test_create_user_csv.py | 115 +++++++--------- .../scripts/test_create_user_csv_expected.csv | 4 + app/tests/api/util/test_file_util.py | 124 ------------------ app/tests/api/util/test_string_utils.py | 21 --- app/tests/conftest.py | 88 ++++--------- docs/app/database/database-testing.md | 2 +- docs/app/getting-started.md | 11 +- .../logging-configuration.md | 2 +- 39 files changed, 277 insertions(+), 548 deletions(-) rename app/api/{route => api}/__init__.py (100%) rename app/api/{route => api}/healthcheck.py (92%) rename app/api/{route => api}/response.py (97%) rename app/api/{route => api}/schemas/__init__.py (100%) rename app/api/{route => api}/schemas/request_schema.py (100%) rename app/api/{route => api}/schemas/response_schema.py (95%) create mode 100644 app/api/api/users/__init__.py create mode 100644 app/api/api/users/user_blueprint.py create mode 100644 app/api/api/users/user_commands.py rename app/api/{route/user_route.py => api/users/user_routes.py} (68%) rename app/api/{route/schemas => api/users}/user_schemas.py (91%) create mode 100644 app/api/db/migrations/versions/2023_02_21_cascade_on_delete.py delete mode 100644 app/api/route/route_utils.py delete mode 100644 app/api/scripts/__init__.py delete mode 100644 app/api/scripts/util/__init__.py delete mode 100644 app/api/scripts/util/script_util.py create mode 100644 app/tests/api/scripts/test_create_user_csv_expected.csv delete mode 100644 app/tests/api/util/test_file_util.py delete mode 100644 app/tests/api/util/test_string_utils.py diff --git a/app/Makefile b/app/Makefile index 90a08f46..0e8194d5 100644 --- a/app/Makefile +++ b/app/Makefile @@ -33,6 +33,19 @@ else PY_RUN_CMD := docker-compose run $(DOCKER_EXEC_ARGS) --rm $(APP_NAME) poetry run endif +FLASK_CMD := $(PY_RUN_CMD) flask --env-file local.env + +################################################## +# Local Development Environment Setup +################################################## + +setup-local: + # Configure poetry to use virtualenvs in the project directory + poetry config virtualenvs.in-project true + + # Install dependencies + poetry install --no-root + ################################################## # API Build & Run ################################################## @@ -46,7 +59,6 @@ start: run-logs: start docker-compose logs --follow --no-color $(APP_NAME) - init: build init-db clean-volumes: ## Remove project docker volumes (which includes the DB state) @@ -57,13 +69,6 @@ stop: check: format-check lint test -# Set init-db as pre-requisite since there seems to be a race condition -# where the DB can't yet receive connections if it's starting from a -# clean state (e.g. after make stop, make clean-volumes, make openapi-spec) -openapi-spec: init-db ## Generate OpenAPI spec - $(PY_RUN_CMD) flask spec --format yaml --output ./openapi.yml - - ################################################## # DB & migrations ################################################## @@ -168,11 +173,21 @@ lint-security: # https://bandit.readthedocs.io/en/latest/index.html ################################################## -# Scripts +# CLI Commands ################################################## -create-user-csv: - $(PY_RUN_CMD) create-user-csv +cmd: ## Run Flask app CLI command (Run `make cli args="--help"` to see list of CLI commands) + $(FLASK_CMD) $(args) + +cmd-user-create-csv: ## Create a CSV of the useres in the database (Run `make cli-user-create-csv args="--help"` to see the command's options) + $(FLASK_CMD) user create-csv $(args) + +# Set init-db as pre-requisite since there seems to be a race condition +# where the DB can't yet receive connections if it's starting from a +# clean state (e.g. after make stop, make clean-volumes, make openapi-spec) +openapi-spec: init-db ## Generate OpenAPI spec + $(FLASK_CMD) spec --format yaml --output ./openapi.yml + ################################################## # Miscellaneous Utilities @@ -184,10 +199,3 @@ login: start ## Start shell in running container # Pauses for 5 seconds sleep-5: sleep 5 - -setup-local: - # Configure poetry to use virtualenvs in the project directory - poetry config virtualenvs.in-project true - - # Install dependencies - poetry install --no-root diff --git a/app/api/route/__init__.py b/app/api/api/__init__.py similarity index 100% rename from app/api/route/__init__.py rename to app/api/api/__init__.py diff --git a/app/api/route/healthcheck.py b/app/api/api/healthcheck.py similarity index 92% rename from app/api/route/healthcheck.py rename to app/api/api/healthcheck.py index 80b76f6a..4df442b2 100644 --- a/app/api/route/healthcheck.py +++ b/app/api/api/healthcheck.py @@ -7,8 +7,8 @@ from werkzeug.exceptions import ServiceUnavailable import api.adapters.db.flask_db as flask_db -from api.route import response -from api.route.schemas import request_schema +from api.api import response +from api.api.schemas import request_schema logger = logging.getLogger(__name__) diff --git a/app/api/route/response.py b/app/api/api/response.py similarity index 97% rename from app/api/route/response.py rename to app/api/api/response.py index 4c843a62..65f6e0bb 100644 --- a/app/api/route/response.py +++ b/app/api/api/response.py @@ -1,8 +1,8 @@ import dataclasses from typing import Optional +from api.api.schemas import response_schema from api.db.models.base import Base -from api.route.schemas import response_schema @dataclasses.dataclass diff --git a/app/api/route/schemas/__init__.py b/app/api/api/schemas/__init__.py similarity index 100% rename from app/api/route/schemas/__init__.py rename to app/api/api/schemas/__init__.py diff --git a/app/api/route/schemas/request_schema.py b/app/api/api/schemas/request_schema.py similarity index 100% rename from app/api/route/schemas/request_schema.py rename to app/api/api/schemas/request_schema.py diff --git a/app/api/route/schemas/response_schema.py b/app/api/api/schemas/response_schema.py similarity index 95% rename from app/api/route/schemas/response_schema.py rename to app/api/api/schemas/response_schema.py index 6dd25c0a..9ae6f6ac 100644 --- a/app/api/route/schemas/response_schema.py +++ b/app/api/api/schemas/response_schema.py @@ -1,6 +1,6 @@ from apiflask import fields -from api.route.schemas import request_schema +from api.api.schemas import request_schema class ValidationErrorSchema(request_schema.OrderedSchema): diff --git a/app/api/api/users/__init__.py b/app/api/api/users/__init__.py new file mode 100644 index 00000000..1da0ac9f --- /dev/null +++ b/app/api/api/users/__init__.py @@ -0,0 +1,10 @@ +from api.api.users.user_blueprint import user_blueprint + +# import user_commands module to register the CLI commands on the user_blueprint +import api.api.users.user_commands # noqa: F401 E402 isort:skip + +# import user_commands module to register the API routes on the user_blueprint +import api.api.users.user_routes # noqa: F401 E402 isort:skip + + +__all__ = ["user_blueprint"] diff --git a/app/api/api/users/user_blueprint.py b/app/api/api/users/user_blueprint.py new file mode 100644 index 00000000..3155fe4e --- /dev/null +++ b/app/api/api/users/user_blueprint.py @@ -0,0 +1,3 @@ +from apiflask import APIBlueprint + +user_blueprint = APIBlueprint("user", __name__, tag="User", cli_group="user") diff --git a/app/api/api/users/user_commands.py b/app/api/api/users/user_commands.py new file mode 100644 index 00000000..515bd4b4 --- /dev/null +++ b/app/api/api/users/user_commands.py @@ -0,0 +1,34 @@ +import logging +import os.path as path +from typing import Optional + +import click + +import api.adapters.db as db +import api.adapters.db.flask_db as flask_db +import api.services.users as user_service +from api.api.users.user_blueprint import user_blueprint +from api.util.datetime_util import utcnow + +logger = logging.getLogger(__name__) + +user_blueprint.cli.help = "User commands" + + +@user_blueprint.cli.command("create-csv", help="Create a CSV of all users and their roles") +@flask_db.with_db_session +@click.option( + "--dir", + default=".", + help="Directory to save output file in. Can be an S3 path (e.g. 's3://bucketname/folder/') Defaults to current directory ('.').", +) +@click.option( + "--filename", + default=None, + help="Filename to save output file as. Defaults to '[timestamp]-user-roles.csv.", +) +def create_csv(db_session: db.Session, dir: str, filename: Optional[str]) -> None: + if filename is None: + filename = utcnow().strftime("%Y-%m-%d-%H-%M-%S") + "-user-roles.csv" + filepath = path.join(dir, filename) + user_service.create_user_csv(db_session, filepath) diff --git a/app/api/route/user_route.py b/app/api/api/users/user_routes.py similarity index 68% rename from app/api/route/user_route.py rename to app/api/api/users/user_routes.py index 30cbd8a8..5ef77eee 100644 --- a/app/api/route/user_route.py +++ b/app/api/api/users/user_routes.py @@ -1,44 +1,34 @@ import logging from typing import Any -from apiflask import APIBlueprint - import api.adapters.db as db import api.adapters.db.flask_db as flask_db +import api.api.response as response +import api.api.users.user_schemas as user_schemas import api.services.users as user_service +import api.services.users as users +from api.api.users.user_blueprint import user_blueprint from api.auth.api_key_auth import api_key_auth from api.db.models.user_models import User -from api.route import response -from api.route.schemas import user_schemas -from api.services import users logger = logging.getLogger(__name__) -user_blueprint = APIBlueprint("user", __name__, tag="User") - - -@user_blueprint.post("/v1/user") +@user_blueprint.post("/v1/users") @user_blueprint.input(user_schemas.UserSchema) @user_blueprint.output(user_schemas.UserSchema, status_code=201) @user_blueprint.auth_required(api_key_auth) @flask_db.with_db_session def user_post(db_session: db.Session, user_params: users.CreateUserParams) -> dict: """ - POST /v1/user + POST /v1/users """ user = user_service.create_user(db_session, user_params) - - logger.info( - "Successfully inserted user", - extra=get_user_log_params(user), - ) - print("Successfully inserted user", get_user_log_params(user)) - + logger.info("Successfully inserted user", extra=get_user_log_params(user)) return response.ApiResponse(message="Success", data=user).asdict() -@user_blueprint.patch("/v1/user/") +@user_blueprint.patch("/v1/users/") # Allow partial updates. partial=true means requests that are missing # required fields will not be rejected. # https://marshmallow.readthedocs.io/en/stable/quickstart.html#partial-loading @@ -50,29 +40,19 @@ def user_patch( db_session: db.Session, user_id: str, patch_user_params: users.PatchUserParams ) -> dict: user = user_service.patch_user(db_session, user_id, patch_user_params) - - logger.info( - "Successfully patched user", - extra=get_user_log_params(user), - ) - + logger.info("Successfully patched user", extra=get_user_log_params(user)) return response.ApiResponse(message="Success", data=user).asdict() -@user_blueprint.get("/v1/user/") +@user_blueprint.get("/v1/users/") @user_blueprint.output(user_schemas.UserSchema) @user_blueprint.auth_required(api_key_auth) @flask_db.with_db_session def user_get(db_session: db.Session, user_id: str) -> dict: user = user_service.get_user(db_session, user_id) - - logger.info( - "Successfully fetched user", - extra=get_user_log_params(user), - ) - + logger.info("Successfully fetched user", extra=get_user_log_params(user)) return response.ApiResponse(message="Success", data=user).asdict() def get_user_log_params(user: User) -> dict[str, Any]: - return {"user_id": user.id} + return {"user.id": user.id} diff --git a/app/api/route/schemas/user_schemas.py b/app/api/api/users/user_schemas.py similarity index 91% rename from app/api/route/schemas/user_schemas.py rename to app/api/api/users/user_schemas.py index c02267bd..cc44c6b1 100644 --- a/app/api/route/schemas/user_schemas.py +++ b/app/api/api/users/user_schemas.py @@ -1,12 +1,8 @@ from apiflask import fields from marshmallow import fields as marshmallow_fields +from api.api.schemas import request_schema from api.db.models import user_models -from api.route.schemas import request_schema - -############## -# Role Models -############## class RoleSchema(request_schema.OrderedSchema): @@ -20,11 +16,6 @@ class RoleSchema(request_schema.OrderedSchema): # will always be a nested fields of the API user -############## -# User Models -############## - - class UserSchema(request_schema.OrderedSchema): id = fields.UUID(dump_only=True) first_name = fields.String(metadata={"description": "The user's first name"}, required=True) diff --git a/app/api/app.py b/app/api/app.py index 81c6c146..4c118c90 100644 --- a/app/api/app.py +++ b/app/api/app.py @@ -10,10 +10,10 @@ import api.adapters.db.flask_db as flask_db import api.logging import api.logging.flask_logger as flask_logger +from api.api.healthcheck import healthcheck_blueprint +from api.api.schemas import response_schema +from api.api.users import user_blueprint from api.auth.api_key_auth import User, get_app_security_scheme -from api.route.healthcheck import healthcheck_blueprint -from api.route.schemas import response_schema -from api.route.user_route import user_blueprint logger = logging.getLogger(__name__) diff --git a/app/api/db/migrations/versions/2023_02_21_cascade_on_delete.py b/app/api/db/migrations/versions/2023_02_21_cascade_on_delete.py new file mode 100644 index 00000000..fa443cf4 --- /dev/null +++ b/app/api/db/migrations/versions/2023_02_21_cascade_on_delete.py @@ -0,0 +1,30 @@ +"""cascade on delete + +Revision ID: 9fe657340f70 +Revises: 4ff1160282d1 +Create Date: 2023-02-21 18:16:56.052679 + +""" +from alembic import op + +# revision identifiers, used by Alembic. +revision = "9fe657340f70" +down_revision = "4ff1160282d1" +branch_labels = None +depends_on = None + + +def upgrade(): + # ### commands auto generated by Alembic - please adjust! ### + op.drop_constraint("role_user_id_user_fkey", "role", type_="foreignkey") + op.create_foreign_key( + op.f("role_user_id_user_fkey"), "role", "user", ["user_id"], ["id"], ondelete="CASCADE" + ) + # ### end Alembic commands ### + + +def downgrade(): + # ### commands auto generated by Alembic - please adjust! ### + op.drop_constraint(op.f("role_user_id_user_fkey"), "role", type_="foreignkey") + op.create_foreign_key("role_user_id_user_fkey", "role", "user", ["user_id"], ["id"]) + # ### end Alembic commands ### diff --git a/app/api/db/models/user_models.py b/app/api/db/models/user_models.py index 47341160..6fca730e 100644 --- a/app/api/db/models/user_models.py +++ b/app/api/db/models/user_models.py @@ -28,13 +28,15 @@ class User(Base, IdMixin, TimestampMixin): date_of_birth: date = Column(Date, nullable=False) is_active: bool = Column(Boolean, nullable=False) - roles: list["Role"] = relationship("Role", back_populates="user", order_by="Role.type") + roles: list["Role"] = relationship( + "Role", back_populates="user", cascade="all, delete", order_by="Role.type" + ) class Role(Base, TimestampMixin): __tablename__ = "role" user_id: Mapped[UUID] = Column( - postgresql.UUID(as_uuid=True), ForeignKey("user.id"), primary_key=True + postgresql.UUID(as_uuid=True), ForeignKey("user.id", ondelete="CASCADE"), primary_key=True ) # Set native_enum=False to use store enum values as VARCHAR/TEXT diff --git a/app/api/route/route_utils.py b/app/api/route/route_utils.py deleted file mode 100644 index 19b1f1f0..00000000 --- a/app/api/route/route_utils.py +++ /dev/null @@ -1,21 +0,0 @@ -from typing import Type, TypeVar -from uuid import UUID - -from apiflask import HTTPError -from sqlalchemy.orm import scoped_session - -_T = TypeVar("_T") - - -def get_or_404(db_session: scoped_session, model: Type[_T], id: UUID | str | int) -> _T: - """ - Utility method for fetching a single record from the DB by - its primary key ID, and raising a NotFound exception if - no such record exists. - """ - result = db_session.query(model).get(id) - - if result is None: - raise HTTPError(404, message=f"Could not find {model.__name__} with ID {id}") - - return result diff --git a/app/api/scripts/__init__.py b/app/api/scripts/__init__.py deleted file mode 100644 index e69de29b..00000000 diff --git a/app/api/scripts/util/__init__.py b/app/api/scripts/util/__init__.py deleted file mode 100644 index e69de29b..00000000 diff --git a/app/api/scripts/util/script_util.py b/app/api/scripts/util/script_util.py deleted file mode 100644 index 3cc5a2bb..00000000 --- a/app/api/scripts/util/script_util.py +++ /dev/null @@ -1,41 +0,0 @@ -# TODO use built in @flask.cli commands so that we can reuse flask app and no longer need this file -import logging -from contextlib import contextmanager -from dataclasses import dataclass -from typing import Generator - -import api.adapters.db as db -import api.logging -from api.util.local import load_local_env_vars - -logger = logging.getLogger(__name__) - - -@dataclass -class ScriptContext: - db_session: db.Session - - -# TODO remove -@contextmanager -def script_context_manager() -> Generator[ScriptContext, None, None]: - """ - Context manager for running a script - that needs to access the DB. Initializes - a few useful components like the DB connection, - logging, and local environment variables (if local). - """ - load_local_env_vars() - api.logging.init(__package__) - - # TODO - Note this is really basic, but - # it could a good place to fetch CLI args, initialize - # metrics (eg. New Relic) and so on in a way that - # helps prevent so much boilerplate code. - - db_client = db.init() - db_client.check_db_connection() - with db_client.get_session() as db_session: - script_context = ScriptContext(db_session) - yield script_context - logger.info("Script complete") diff --git a/app/api/services/users/__init__.py b/app/api/services/users/__init__.py index c550e936..9d01b20f 100644 --- a/app/api/services/users/__init__.py +++ b/app/api/services/users/__init__.py @@ -1,4 +1,5 @@ from .create_user import CreateUserParams, RoleParams, create_user +from .create_user_csv import create_user_csv from .get_user import get_user from .patch_user import PatchUserParams, patch_user @@ -9,4 +10,5 @@ "create_user", "get_user", "patch_user", + "create_user_csv", ] diff --git a/app/api/services/users/create_user_csv.py b/app/api/services/users/create_user_csv.py index f365b2fd..761d288e 100644 --- a/app/api/services/users/create_user_csv.py +++ b/app/api/services/users/create_user_csv.py @@ -1,15 +1,11 @@ -# TODO move create_csv_script to a flask cli command so we don't need script_util.script_context_manager import csv import logging -import os from dataclasses import asdict, dataclass from smart_open import open as smart_open import api.adapters.db as db from api.db.models.user_models import User -from api.scripts.util.script_util import script_context_manager -from api.util.datetime_util import utcnow logger = logging.getLogger(__name__) @@ -83,20 +79,3 @@ def convert_user_records_for_csv(records: list[User]) -> list[UserCsvRecord]: ) return out_records - - -def main() -> None: - # Initialize DB session / logging / env vars - with script_context_manager() as script_context: - # Build the path for the output file - # This will create a file in the folder specified like: - # s3://your-bucket/path/to/2022-09-09-12-00-00-user-roles.csv - # The file path can be either S3 or local disk. - output_path = os.getenv("USER_ROLE_CSV_OUTPUT_PATH", None) - if not output_path: - raise Exception("Please specify an USER_ROLE_CSV_OUTPUT_PATH env var") - - file_name = utcnow().strftime("%Y-%m-%d-%H-%M-%S") + "-user-roles.csv" - output_file_path = os.path.join(output_path, file_name) - - create_user_csv(script_context.db_session, output_file_path) diff --git a/app/api/util/file_util.py b/app/api/util/file_util.py index ced414f2..5972ba3a 100644 --- a/app/api/util/file_util.py +++ b/app/api/util/file_util.py @@ -45,64 +45,3 @@ def get_s3_client(boto_session: Optional[boto3.Session] = None) -> botocore.clie return boto_session.client("s3") return boto3.client("s3") - - -################################## -# File operation utils -################################## - - -def list_files( - path: str, recursive: bool = False, boto_session: Optional[boto3.Session] = None -) -> list[str]: - """List the immediate files under path. - - There is minor inconsistency between local path handling and S3 paths. - Directory names will be included for local paths, whereas they will not for - S3 paths. - - Args: - path: Supports s3:// and local paths. - recursive: Only applicable for S3 paths. - If set to True will recursively list all relative key paths under the prefix. - boto_session: Boto session object to use for S3 access. Only necessary - if needing to access an S3 bucket with assumed credentials (e.g., - cross-account bucket access). - """ - if is_s3_path(path): - bucket_name, prefix = split_s3_url(path) - - # in order for s3.list_objects to only list the immediate "files" under - # the given path, the prefix should end in the path delimiter - if prefix and not prefix.endswith("/"): - prefix = prefix + "/" - - s3 = get_s3_client(boto_session) - - # When the delimiter is provided, s3 knows to stop listing keys that contain it (starting after the prefix). - # https://docs.aws.amazon.com/AmazonS3/latest/dev/ListingKeysHierarchy.html - delimiter = "" if recursive else "/" - - paginator = s3.get_paginator("list_objects_v2") - pages = paginator.paginate(Bucket=bucket_name, Prefix=prefix, Delimiter=delimiter) - - file_paths = [] - for page in pages: - object_contents = page.get("Contents") - - if object_contents: - for object in object_contents: - if recursive: - key = object["Key"] - start_index = key.index(prefix) + len(prefix) - file_paths.append(key[start_index:]) - else: - file_paths.append(get_file_name(object["Key"])) - - return file_paths - - # os.listdir throws an exception if the path doesn't exist - # Make it behave like S3 list and return an empty list - if os.path.exists(path): - return os.listdir(path) - return [] diff --git a/app/api/util/string_utils.py b/app/api/util/string_utils.py index c309f72b..9ad1b7e5 100644 --- a/app/api/util/string_utils.py +++ b/app/api/util/string_utils.py @@ -1,4 +1,4 @@ -from typing import Any, Optional +from typing import Optional def join_list(joining_list: Optional[list], join_txt: str = "\n") -> str: @@ -12,17 +12,3 @@ def join_list(joining_list: Optional[list], join_txt: str = "\n") -> str: return "" return join_txt.join(joining_list) - - -def blank_for_null(value: Optional[Any]) -> str: - """ - Utility to make a string blank if its null - - Functionally equivalent to - - ```"" if value is None else str(value)``` - """ - # If a value is blank - if value is None: - return "" - return str(value) diff --git a/app/local.env b/app/local.env index 71961e47..63f135e9 100644 --- a/app/local.env +++ b/app/local.env @@ -74,8 +74,3 @@ AWS_SECRET_ACCESS_KEY=DO_NOT_SET_HERE #AWS_SESSION_TOKEN=DO_NOT_SET_HERE AWS_DEFAULT_REGION=us-east-1 - -############################ -# Example CSV Generation -############################ -USER_ROLE_CSV_OUTPUT_PATH = ./ diff --git a/app/openapi.yml b/app/openapi.yml index c5832aab..b935bc5f 100644 --- a/app/openapi.yml +++ b/app/openapi.yml @@ -46,7 +46,7 @@ paths: tags: - Health summary: Health - /v1/user: + /v1/users: post: parameters: [] responses: @@ -87,7 +87,7 @@ paths: description: Authentication error tags: - User - summary: POST /v1/user + summary: POST /v1/users requestBody: content: application/json: @@ -95,7 +95,7 @@ paths: $ref: '#/components/schemas/User' security: - ApiKeyAuth: [] - /v1/user/{user_id}: + /v1/users/{user_id}: get: parameters: - in: path diff --git a/app/poetry.lock b/app/poetry.lock index 6023d809..051b6ebe 100644 --- a/app/poetry.lock +++ b/app/poetry.lock @@ -21,14 +21,14 @@ tz = ["python-dateutil"] [[package]] name = "apiflask" -version = "1.2.0" +version = "1.2.1" description = "A lightweight web API framework based on Flask and marshmallow-code projects." category = "main" optional = false python-versions = ">=3.7" files = [ - {file = "APIFlask-1.2.0-py3-none-any.whl", hash = "sha256:e93b3323d7cc62db672c59a9b425c768b52a1d626e5ef2e5f5f86b254012e19e"}, - {file = "APIFlask-1.2.0.tar.gz", hash = "sha256:0bb24d79cc8e381673d9ef7d3b5cfd299e6813f9c086d5d372d97116768e251e"}, + {file = "APIFlask-1.2.1-py3-none-any.whl", hash = "sha256:71452cb2dee41053f8cd1e7dd7d976ba9deb54f1a76845e375c1ca5af3c7c794"}, + {file = "APIFlask-1.2.1.tar.gz", hash = "sha256:505431a2c61ec3fc7e8fdb81f653d09c1ccec5b1e4e94f878f730af70a31be54"}, ] [package.dependencies] @@ -1308,6 +1308,21 @@ toml = "*" [package.extras] testing = ["argcomplete", "hypothesis (>=3.56)", "mock", "nose", "requests", "xmlschema"] +[[package]] +name = "pytest-lazy-fixture" +version = "0.6.3" +description = "It helps to use fixtures in pytest.mark.parametrize" +category = "dev" +optional = false +python-versions = "*" +files = [ + {file = "pytest-lazy-fixture-0.6.3.tar.gz", hash = "sha256:0e7d0c7f74ba33e6e80905e9bfd81f9d15ef9a790de97993e34213deb5ad10ac"}, + {file = "pytest_lazy_fixture-0.6.3-py3-none-any.whl", hash = "sha256:e0b379f38299ff27a653f03eaa69b08a6fd4484e46fd1c9907d984b9f9daeda6"}, +] + +[package.dependencies] +pytest = ">=3.2.5" + [[package]] name = "pytest-watch" version = "4.2.0" @@ -1579,7 +1594,7 @@ sqlalchemy2-stubs = {version = "*", optional = true, markers = "extra == \"mypy\ [package.extras] aiomysql = ["aiomysql", "greenlet (!=0.4.17)"] -aiosqlite = ["aiosqlite", "greenlet (!=0.4.17)", "typing-extensions (!=3.10.0.1)"] +aiosqlite = ["aiosqlite", "greenlet (!=0.4.17)", "typing_extensions (!=3.10.0.1)"] asyncio = ["greenlet (!=0.4.17)"] asyncmy = ["asyncmy (>=0.2.3,!=0.2.4)", "greenlet (!=0.4.17)"] mariadb-connector = ["mariadb (>=1.0.1,!=1.1.2)"] @@ -1589,14 +1604,14 @@ mssql-pyodbc = ["pyodbc"] mypy = ["mypy (>=0.910)", "sqlalchemy2-stubs"] mysql = ["mysqlclient (>=1.4.0)", "mysqlclient (>=1.4.0,<2)"] mysql-connector = ["mysql-connector-python"] -oracle = ["cx-oracle (>=7)", "cx-oracle (>=7,<8)"] +oracle = ["cx_oracle (>=7)", "cx_oracle (>=7,<8)"] postgresql = ["psycopg2 (>=2.7)"] postgresql-asyncpg = ["asyncpg", "greenlet (!=0.4.17)"] postgresql-pg8000 = ["pg8000 (>=1.16.6,!=1.29.0)"] postgresql-psycopg2binary = ["psycopg2-binary"] postgresql-psycopg2cffi = ["psycopg2cffi"] pymysql = ["pymysql", "pymysql (<1)"] -sqlcipher = ["sqlcipher3-binary"] +sqlcipher = ["sqlcipher3_binary"] [[package]] name = "sqlalchemy2-stubs" @@ -1822,4 +1837,4 @@ files = [ [metadata] lock-version = "2.0" python-versions = "^3.10" -content-hash = "1f654cfd2377d8065e73d9e96fe708fe13c76d5d789cd04702ace59d9e4b23a8" +content-hash = "01e8dc779c9db34a85e0c5ed33b87700a38feae540d9677eacf6ec5d6b8b4c7f" diff --git a/app/pyproject.toml b/app/pyproject.toml index b67bbe61..eb85e2cd 100644 --- a/app/pyproject.toml +++ b/app/pyproject.toml @@ -37,6 +37,7 @@ bandit = "^1.7.4" [tool.poetry.group.dev.dependencies] pytest-watch = "^4.2.0" +pytest-lazy-fixture = "^0.6.3" [build-system] requires = ["poetry-core>=1.0.0"] @@ -46,7 +47,6 @@ build-backend = "poetry.core.masonry.api" db-migrate-up = "api.db.migrations.run:up" db-migrate-down = "api.db.migrations.run:down" db-migrate-down-all = "api.db.migrations.run:downall" -create-user-csv = "api.scripts.create_user_csv:main" [tool.black] line-length = 100 diff --git a/app/tests/api/db/models/factories.py b/app/tests/api/db/models/factories.py index c822f680..811d634d 100644 --- a/app/tests/api/db/models/factories.py +++ b/app/tests/api/db/models/factories.py @@ -25,7 +25,7 @@ def get_db_session() -> db.Session: - # _db_session is only set in the pytest fixture `factories_db_session` + # _db_session is only set in the pytest fixture `enable_factory_create` # so that tests do not unintentionally write to the database. if _db_session is None: raise Exception( @@ -36,7 +36,7 @@ def get_db_session() -> db.Session: not persist the generated model. If running tests that actually need data in the DB, pull in the - `factories_db_session` fixture to initialize the db_session. + `enable_factory_create` fixture to initialize the db_session. """ ) diff --git a/app/tests/api/db/models/test_factories.py b/app/tests/api/db/models/test_factories.py index 79807224..f1500501 100644 --- a/app/tests/api/db/models/test_factories.py +++ b/app/tests/api/db/models/test_factories.py @@ -2,6 +2,7 @@ import pytest +import api.adapters.db as db from api.db.models.user_models import User from tests.api.db.models.factories import RoleFactory, UserFactory @@ -54,25 +55,25 @@ def test_user_factory_build(): def test_factory_create_uninitialized_db_session(): # DB factory access is disabled from tests unless you add the - # 'factories_db_session' fixture. + # 'enable_factory_create' fixture. with pytest.raises(Exception, match="Factory db_session is not initialized."): UserFactory.create() -def test_user_factory_create(factories_db_session): +def test_user_factory_create(enable_factory_create, db_session: db.Session): # Create actually writes a record to the DB when run # so we'll check the DB directly as well. user = UserFactory.create() validate_user_record(user) - db_record = factories_db_session.query(User).filter(User.id == user.id).one_or_none() + db_record = db_session.query(User).filter(User.id == user.id).one_or_none() # Make certain the DB record matches the factory one. validate_user_record(db_record, user.for_json()) user = UserFactory.create(**user_params) validate_user_record(user, user_params) - db_record = factories_db_session.query(User).filter(User.id == user.id).one_or_none() + db_record = db_session.query(User).filter(User.id == user.id).one_or_none() # Make certain the DB record matches the factory one. validate_user_record(db_record, db_record.for_json()) @@ -81,11 +82,11 @@ def test_user_factory_create(factories_db_session): user = UserFactory.create(**null_params) validate_user_record(user, null_params) - all_db_records = factories_db_session.query(User).all() + all_db_records = db_session.query(User).all() assert len(all_db_records) == 3 -def test_role_factory_create(factories_db_session): +def test_role_factory_create(enable_factory_create): # Verify if you build a Role directly, it gets # a user attached to it with that single role role = RoleFactory.create() diff --git a/app/tests/api/db/test_migrations.py b/app/tests/api/db/test_migrations.py index 0f286631..60a0babd 100644 --- a/app/tests/api/db/test_migrations.py +++ b/app/tests/api/db/test_migrations.py @@ -6,7 +6,22 @@ from alembic.script.revision import MultipleHeads from alembic.util.exc import CommandError +import api.adapters.db as db from api.db.migrations.run import alembic_cfg +from tests.lib import db_testing + + +@pytest.fixture +def empty_schema(monkeypatch) -> db.DBClient: + """ + Create a test schema, if it doesn't already exist, and drop it after the + test completes. + + This is similar to what the db_client fixture does but does not create any tables in the + schema. + """ + with db_testing.create_isolated_db(monkeypatch) as db_client: + yield db_client def test_only_single_head_revision_in_migrations(): diff --git a/app/tests/api/route/test_user_route.py b/app/tests/api/route/test_user_route.py index fcdb2a96..00a8b80a 100644 --- a/app/tests/api/route/test_user_route.py +++ b/app/tests/api/route/test_user_route.py @@ -27,7 +27,7 @@ def base_request(): @pytest.fixture def created_user(client, api_auth_token, base_request): - response = client.post("/v1/user", json=base_request, headers={"X-Auth": api_auth_token}) + response = client.post("/v1/users", json=base_request, headers={"X-Auth": api_auth_token}) return response.get_json()["data"] @@ -44,7 +44,7 @@ def test_create_and_get_user(client, base_request, api_auth_token, roles): **base_request, "roles": roles, } - post_response = client.post("/v1/user", json=request, headers={"X-Auth": api_auth_token}) + post_response = client.post("/v1/users", json=request, headers={"X-Auth": api_auth_token}) post_response_data = post_response.get_json()["data"] expected_response = { **request, @@ -60,7 +60,7 @@ def test_create_and_get_user(client, base_request, api_auth_token, roles): # Get the user user_id = post_response.get_json()["data"]["id"] - get_response = client.get(f"/v1/user/{user_id}", headers={"X-Auth": api_auth_token}) + get_response = client.get(f"/v1/users/{user_id}", headers={"X-Auth": api_auth_token}) assert get_response.status_code == 200 @@ -117,7 +117,7 @@ def test_create_and_get_user(client, base_request, api_auth_token, roles): @pytest.mark.parametrize("request_data,expected_response_data", test_create_user_bad_request_data) def test_create_user_bad_request(client, api_auth_token, request_data, expected_response_data): - response = client.post("/v1/user", json=request_data, headers={"X-Auth": api_auth_token}) + response = client.post("/v1/users", json=request_data, headers={"X-Auth": api_auth_token}) assert response.status_code == 422 response_data = response.get_json()["detail"]["json"] @@ -128,7 +128,7 @@ def test_patch_user(client, api_auth_token, created_user): user_id = created_user["id"] patch_request = {"first_name": fake.first_name()} patch_response = client.patch( - f"/v1/user/{user_id}", json=patch_request, headers={"X-Auth": api_auth_token} + f"/v1/users/{user_id}", json=patch_request, headers={"X-Auth": api_auth_token} ) patch_response_data = patch_response.get_json()["data"] expected_response_data = { @@ -140,7 +140,7 @@ def test_patch_user(client, api_auth_token, created_user): assert patch_response.status_code == 200 assert patch_response_data == expected_response_data - get_response = client.get(f"/v1/user/{user_id}", headers={"X-Auth": api_auth_token}) + get_response = client.get(f"/v1/users/{user_id}", headers={"X-Auth": api_auth_token}) get_response_data = get_response.get_json()["data"] assert get_response_data == expected_response_data @@ -154,13 +154,13 @@ def test_patch_user_roles(client, base_request, api_auth_token, initial_roles, u "roles": initial_roles, } created_user = client.post( - "/v1/user", json=post_request, headers={"X-Auth": api_auth_token} + "/v1/users", json=post_request, headers={"X-Auth": api_auth_token} ).get_json()["data"] user_id = created_user["id"] patch_request = {"roles": updated_roles} patch_response = client.patch( - f"/v1/user/{user_id}", json=patch_request, headers={"X-Auth": api_auth_token} + f"/v1/users/{user_id}", json=patch_request, headers={"X-Auth": api_auth_token} ) patch_response_data = patch_response.get_json()["data"] expected_response_data = { @@ -172,16 +172,16 @@ def test_patch_user_roles(client, base_request, api_auth_token, initial_roles, u assert patch_response.status_code == 200 assert patch_response_data == expected_response_data - get_response = client.get(f"/v1/user/{user_id}", headers={"X-Auth": api_auth_token}) + get_response = client.get(f"/v1/users/{user_id}", headers={"X-Auth": api_auth_token}) get_response_data = get_response.get_json()["data"] assert get_response_data == expected_response_data test_unauthorized_data = [ - pytest.param("post", "/v1/user", get_base_request(), id="post"), - pytest.param("get", f"/v1/user/{uuid.uuid4()}", None, id="get"), - pytest.param("patch", f"/v1/user/{uuid.uuid4()}", {}, id="patch"), + pytest.param("post", "/v1/users", get_base_request(), id="post"), + pytest.param("get", f"/v1/users/{uuid.uuid4()}", None, id="get"), + pytest.param("patch", f"/v1/users/{uuid.uuid4()}", {}, id="patch"), ] @@ -205,7 +205,7 @@ def test_unauthorized(client, method, url, body, api_auth_token): @pytest.mark.parametrize("method,body", test_not_found_data) def test_not_found(client, api_auth_token, method, body): user_id = uuid.uuid4() - url = f"/v1/user/{user_id}" + url = f"/v1/users/{user_id}" response = getattr(client, method)(url, json=body, headers={"X-Auth": api_auth_token}) assert response.status_code == 404 diff --git a/app/tests/api/scripts/test_create_user_csv.py b/app/tests/api/scripts/test_create_user_csv.py index 0b2a24d7..31f0c577 100644 --- a/app/tests/api/scripts/test_create_user_csv.py +++ b/app/tests/api/scripts/test_create_user_csv.py @@ -1,79 +1,60 @@ -import csv import os +import os.path as path +import re +import flask.testing import pytest +from pytest_lazyfixture import lazy_fixture from smart_open import open as smart_open -from api.services.users.create_user_csv import USER_CSV_RECORD_HEADERS, create_user_csv -from api.util.file_util import list_files -from api.util.string_utils import blank_for_null +import api.adapters.db as db +from api.db.models.user_models import User from tests.api.db.models.factories import UserFactory @pytest.fixture -def tmp_file_path(tmp_path): - return tmp_path / "example_file.csv" - - -def read_csv_records(file_path): - with smart_open(file_path) as csv_file: - csv_reader = csv.DictReader(csv_file) - csv_rows = list(csv_reader) - return csv_rows - - -def validate_csv_records(db_records, csv_records): - - assert len(csv_records) == len(db_records) - - # Sort the two lists by name and zip together for validation - csv_records.sort(key=lambda record: record["User Name"]) - db_records.sort(key=lambda record: record.first_name) - for csv_record, db_record in zip(csv_records, db_records): - assert ( - csv_record[USER_CSV_RECORD_HEADERS.user_name] - == f"{db_record.first_name} {db_record.last_name}" - ) - assert csv_record[USER_CSV_RECORD_HEADERS.roles] == " ".join( - [role.type for role in db_record.roles] - ) - assert csv_record[USER_CSV_RECORD_HEADERS.is_user_active] == blank_for_null( - db_record.is_active - ) - - -def test_create_user_csv_s3(isolated_db_factories_session, mock_s3_bucket): - s3_filepath = f"s3://{mock_s3_bucket}/path/to/test.csv" - - # To make validating these easier in the CSV, make the names consistent - db_records = [ - UserFactory.create(first_name="A"), - UserFactory.create(first_name="B"), - ] - - create_user_csv(isolated_db_factories_session, s3_filepath) - csv_rows = read_csv_records(s3_filepath) - validate_csv_records(db_records, csv_rows) - - # If we add another DB record it'll go in the file as well - db_records.append(UserFactory.create(first_name="C")) - create_user_csv(isolated_db_factories_session, s3_filepath) - csv_rows = read_csv_records(s3_filepath) - validate_csv_records(db_records, csv_rows) - - assert "test.csv" in list_files(f"s3://{mock_s3_bucket}/path/to/") - - -def test_create_user_csv_local(isolated_db_factories_session, tmp_path, tmp_file_path): - # Same as above test, but verifying the file logic - # works locally in addition to S3. - db_records = [ - UserFactory.create(first_name="A"), - UserFactory.create(first_name="B"), +def prepopulate_user_table(enable_factory_create, db_session: db.Session) -> list[User]: + # First make sure the table is empty, as other tests may have inserted data + # and this test expects a clean slate (unlike most tests that are designed to + # be isolated from other tests) + db_session.query(User).delete() + return [ + UserFactory.create(first_name="Jon", last_name="Doe", is_active=True), + UserFactory.create(first_name="Jane", last_name="Doe", is_active=False), + UserFactory.create( + first_name="Alby", + last_name="Testin", + is_active=True, + ), ] - create_user_csv(isolated_db_factories_session, tmp_file_path) - csv_rows = read_csv_records(tmp_file_path) - validate_csv_records(db_records, csv_rows) - assert os.path.exists(tmp_file_path) +@pytest.fixture +def tmp_s3_folder(mock_s3_bucket): + return f"s3://{mock_s3_bucket}/path/to/" + + +@pytest.mark.parametrize( + "dir", + [ + pytest.param(lazy_fixture("tmp_s3_folder"), id="write-to-s3"), + pytest.param(lazy_fixture("tmp_path"), id="write-to-local"), + ], +) +def test_create_user_csv( + prepopulate_user_table: list[User], + cli_runner: flask.testing.FlaskCliRunner, + dir: str, +): + cli_runner.invoke(args=["user", "create-csv", "--dir", dir, "--filename", "test.csv"]) + output = smart_open(path.join(dir, "test.csv")).read() + expected_output = open( + path.join(path.dirname(__file__), "test_create_user_csv_expected.csv") + ).read() + assert output == expected_output + + +def test_default_filename(cli_runner: flask.testing.FlaskCliRunner, tmp_path: str): + cli_runner.invoke(args=["user", "create-csv", "--dir", tmp_path]) + filenames = os.listdir(tmp_path) + assert re.match(r"\d{4}-\d{2}-\d{2}-\d{2}-\d{2}-\d{2}-user-roles.csv", filenames[0]) diff --git a/app/tests/api/scripts/test_create_user_csv_expected.csv b/app/tests/api/scripts/test_create_user_csv_expected.csv new file mode 100644 index 00000000..da0371ff --- /dev/null +++ b/app/tests/api/scripts/test_create_user_csv_expected.csv @@ -0,0 +1,4 @@ +"User Name","Roles","Is User Active?" +"Jon Doe","ADMIN USER","True" +"Jane Doe","ADMIN USER","False" +"Alby Testin","ADMIN USER","True" diff --git a/app/tests/api/util/test_file_util.py b/app/tests/api/util/test_file_util.py deleted file mode 100644 index d8b5794a..00000000 --- a/app/tests/api/util/test_file_util.py +++ /dev/null @@ -1,124 +0,0 @@ -import os - -import pytest -from smart_open import open as smart_open - -import api.util.file_util as file_util - - -def create_file(root_path, file_path): - full_path = os.path.join(root_path, file_path) - - if not file_util.is_s3_path(str(full_path)): - os.makedirs(os.path.dirname(full_path), exist_ok=True) - - with smart_open(full_path, mode="w") as outfile: - outfile.write("hello") - - return full_path - - -@pytest.mark.parametrize( - "path,is_s3", - [ - ("s3://bucket/folder/test.txt", True), - ("./relative/folder/test.txt", False), - ("http://example.com/test.txt", False), - ], -) -def test_is_s3_path(path, is_s3): - assert file_util.is_s3_path(path) is is_s3 - - -@pytest.mark.parametrize( - "path,bucket,prefix", - [ - ("s3://my_bucket/my_key", "my_bucket", "my_key"), - ("s3://my_bucket/path/to/directory/", "my_bucket", "path/to/directory/"), - ("s3://my_bucket/path/to/file.txt", "my_bucket", "path/to/file.txt"), - ], -) -def test_split_s3_url(path, bucket, prefix): - assert file_util.split_s3_url(path) == (bucket, prefix) - - -@pytest.mark.parametrize( - "path,bucket", - [ - ("s3://bucket/folder/test.txt", "bucket"), - ("s3://bucket_x/folder", "bucket_x"), - ("s3://bucket-y/folder/", "bucket-y"), - ("s3://bucketz", "bucketz"), - ], -) -def test_get_s3_bucket(path, bucket): - assert file_util.get_s3_bucket(path) == bucket - - -@pytest.mark.parametrize( - "path,file_key", - [ - ("s3://bucket/folder/test.txt", "folder/test.txt"), - ("s3://bucket_x/file.csv", "file.csv"), - ("s3://bucket-y/folder/path/to/abc.zip", "folder/path/to/abc.zip"), - ("./folder/path", "/folder/path"), - ("sftp://folder/filename", "filename"), - ], -) -def test_get_s3_file_key(path, file_key): - assert file_util.get_s3_file_key(path) == file_key - - -@pytest.mark.parametrize( - "path,file_name", - [ - ("s3://bucket/folder/test.txt", "test.txt"), - ("s3://bucket_x/file.csv", "file.csv"), - ("s3://bucket-y/folder/path/to/abc.zip", "abc.zip"), - ("./folder/path", "path"), - ("sftp://filename", "filename"), - ], -) -def test_get_s3_file_name(path, file_name): - assert file_util.get_file_name(path) == file_name - - -def test_list_files_in_folder_fs(tmp_path): - create_file(tmp_path, "file1.txt") - create_file(tmp_path, "folder/file2.txt") - create_file(tmp_path, "different_folder/file3.txt") - create_file(tmp_path, "folder/nested_folder/file4.txt") - - assert "file1.txt" in file_util.list_files(tmp_path) - assert "file2.txt" in file_util.list_files(tmp_path / "folder") - assert "file3.txt" in file_util.list_files(tmp_path / "different_folder") - assert "file4.txt" in file_util.list_files(tmp_path / "folder/nested_folder") - - # Note that recursive doesn't work as implemented for the - # local filesystem, so no further testing is needed. - - -def test_list_files_in_folder_s3(mock_s3_bucket): - prefix = f"s3://{mock_s3_bucket}/" - create_file(prefix, "file1.txt") - create_file(prefix, "folder/file2.txt") - create_file(prefix, "different_folder/file3.txt") - create_file(prefix, "folder/nested_folder/file4.txt") - - assert "file1.txt" in file_util.list_files(prefix) - assert "file2.txt" in file_util.list_files(prefix + "folder") - assert "file3.txt" in file_util.list_files(prefix + "different_folder") - assert "file4.txt" in file_util.list_files(prefix + "folder/nested_folder") - - root_files_recursive = file_util.list_files(prefix, recursive=True) - assert set(root_files_recursive) == set( - [ - "file1.txt", - "folder/file2.txt", - "different_folder/file3.txt", - "folder/nested_folder/file4.txt", - ] - ) - - folder_files_recursive = file_util.list_files(prefix + "folder", recursive=True) - assert set(folder_files_recursive) == set(["file2.txt", "nested_folder/file4.txt"]) diff --git a/app/tests/api/util/test_string_utils.py b/app/tests/api/util/test_string_utils.py deleted file mode 100644 index 2390d715..00000000 --- a/app/tests/api/util/test_string_utils.py +++ /dev/null @@ -1,21 +0,0 @@ -from api.util.string_utils import blank_for_null, join_list - - -def test_join_list(): - assert join_list(None) == "" - assert join_list(None, ",") == "" - assert join_list(None, "|") == "" - assert join_list([]) == "" - assert join_list([], ",") == "" - assert join_list([], "|") == "" - - assert join_list(["a", "b", "c"]) == "a\nb\nc" - assert join_list(["a", "b", "c"], ",") == "a,b,c" - assert join_list(["a", "b", "c"], "|") == "a|b|c" - - -def test_blank_for_null(): - assert blank_for_null(None) == "" - assert blank_for_null("hello") == "hello" - assert blank_for_null(4) == "4" - assert blank_for_null(["a", "b"]) == "['a', 'b']" diff --git a/app/tests/conftest.py b/app/tests/conftest.py index af80dc63..842251b0 100644 --- a/app/tests/conftest.py +++ b/app/tests/conftest.py @@ -2,9 +2,10 @@ import _pytest.monkeypatch import boto3 +import flask +import flask.testing import moto import pytest -import sqlalchemy import api.adapters.db as db import api.app as app_entry @@ -53,7 +54,7 @@ def db_client(monkeypatch_session) -> db.DBClient: Creates an isolated database for the test session. Creates a new empty PostgreSQL schema, creates all tables in the new schema - using SQLAlchemy, then returns a db.DB instance that can be used to + using SQLAlchemy, then returns a db.DBClient instance that can be used to get connections or sessions to this database schema. The schema is dropped after the test suite session completes. """ @@ -63,73 +64,27 @@ def db_client(monkeypatch_session) -> db.DBClient: yield db_client -@pytest.fixture(scope="function") -def isolated_db(monkeypatch) -> db.DBClient: +@pytest.fixture +def db_session(db_client: db.DBClient) -> db.Session: """ - Creates an isolated database for the test function. - - Creates a new empty PostgreSQL schema, creates all tables in the new schema - using SQLAlchemy, then returns a db.DB instance that can be used to - get connections or sessions to this database schema. The schema is dropped - after the test function completes. - - This is similar to the db fixture except the scope of the schema is the - individual test rather the test session. + Returns a database session connected to the schema used for the test session. """ - - with db_testing.create_isolated_db(monkeypatch) as db: - models.metadata.create_all(bind=db.get_connection()) - yield db + with db_client.get_session() as session: + yield session @pytest.fixture -def empty_schema(monkeypatch) -> db.DBClient: +def enable_factory_create(monkeypatch, db_session) -> db.Session: """ - Create a test schema, if it doesn't already exist, and drop it after the - test completes. - - This is similar to the db fixture but does not create any tables in the - schema. This is used by migration tests. + Allows the create method of factories to be called. By default, the create + throws an exception to prevent accidental creation of database objects for tests + that do not need persistence. This fixture only allows the create method to be + called for the current test. Each test that needs to call Factory.create should pull in + this fixture. """ - with db_testing.create_isolated_db(monkeypatch) as db: - yield db - - -@pytest.fixture -def test_db_session(db_client: db.DBClient) -> db.Session: - # Based on https://docs.sqlalchemy.org/en/13/orm/session_transaction.html#joining-a-session-into-an-external-transaction-such-as-for-test-suites - with db_client.get_connection() as connection: - trans = connection.begin() - - # Rather than call db.get_session() to create a new session with a new connection, - # create a session bound to the existing connection that has a transaction manually start. - # This allows the transaction to be rolled back after the test completes. - with db.Session(bind=connection, autocommit=False, expire_on_commit=False) as session: - session.begin_nested() - - @sqlalchemy.event.listens_for(session, "after_transaction_end") - def restart_savepoint(session, transaction): - if transaction.nested and not transaction._parent.nested: - session.begin_nested() - - yield session - - trans.rollback() - - -@pytest.fixture -def factories_db_session(monkeypatch, test_db_session) -> db.Session: - monkeypatch.setattr(factories, "_db_session", test_db_session) - logger.info("set factories db_session to %s", test_db_session) - return test_db_session - - -@pytest.fixture -def isolated_db_factories_session(monkeypatch, isolated_db: db.DBClient) -> db.Session: - with isolated_db.get_session() as session: - monkeypatch.setattr(factories, "_db_session", session) - logger.info("set factories db_session to %s", session) - yield session + monkeypatch.setattr(factories, "_db_session", db_session) + logger.info("set factories db_session to %s", db_session) + return db_session #################### @@ -140,15 +95,20 @@ def isolated_db_factories_session(monkeypatch, isolated_db: db.DBClient) -> db.S # Make app session scoped so the database connection pool is only created once # for the test session. This speeds up the tests. @pytest.fixture(scope="session") -def app(): +def app(db_client) -> flask.Flask: return app_entry.create_app() @pytest.fixture -def client(app): +def client(app: flask.Flask) -> flask.testing.FlaskClient: return app.test_client() +@pytest.fixture +def cli_runner(app: flask.Flask) -> flask.testing.CliRunner: + return app.test_cli_runner() + + @pytest.fixture def api_auth_token(monkeypatch): auth_token = "abcd1234" diff --git a/docs/app/database/database-testing.md b/docs/app/database/database-testing.md index 1ccf9ff4..7404bc15 100644 --- a/docs/app/database/database-testing.md +++ b/docs/app/database/database-testing.md @@ -10,4 +10,4 @@ Note that [PostgreSQL schemas](https://www.postgresql.org/docs/current/ddl-schem ## Test Factories -The application uses [Factory Boy](https://factoryboy.readthedocs.io/en/stable/) to generate test data for the application. This can be used to create models `Factory.build` that can be used in any test, or to prepopulate the database with persisted models using `Factory.create`. In order to use `Factory.create` to create persisted database models, include the `factories_db_session` fixture in the test. +The application uses [Factory Boy](https://factoryboy.readthedocs.io/en/stable/) to generate test data for the application. This can be used to create models `Factory.build` that can be used in any test, or to prepopulate the database with persisted models using `Factory.create`. In order to use `Factory.create` to create persisted database models, include the `enable_factory_create` fixture in the test. diff --git a/docs/app/getting-started.md b/docs/app/getting-started.md index 99f7857e..444efb10 100644 --- a/docs/app/getting-started.md +++ b/docs/app/getting-started.md @@ -31,12 +31,13 @@ make setup-local ## Run the application -1. In your terminal, `cd` to the app directory of this repo. +1. In your terminal, `cd` to the `app` directory of this repo. 2. Make sure you have [Docker Desktop](https://www.docker.com/products/docker-desktop/) installed & running. -3. Run `make init start` to build the image and start the container. -4. Navigate to `localhost:8080/v1/docs` to access the Swagger UI. -5. Run `make run-logs` to see the logs of the running API container -6. Run `make stop` when you are done to delete the container. +3. Run `make setup-local` to copy over `.env.example` to `.env` +4. Run `make init start` to build the image and start the container. +5. Navigate to `localhost:8080/v1/docs` to access the Swagger UI. +6. Run `make run-logs` to see the logs of the running API container +7. Run `make stop` when you are done to delete the container. ## Next steps diff --git a/docs/app/monitoring-and-observability/logging-configuration.md b/docs/app/monitoring-and-observability/logging-configuration.md index c1eaa778..1a62e37b 100644 --- a/docs/app/monitoring-and-observability/logging-configuration.md +++ b/docs/app/monitoring-and-observability/logging-configuration.md @@ -12,7 +12,7 @@ We have two separate ways of formatting the logs which are controlled by the `LO ```json { - "name": "api.route.healthcheck", + "name": "api.api.healthcheck", "levelname": "INFO", "funcName": "healthcheck_get", "created": "1663261542.0465896",