diff --git a/.github/dependabot.yml b/.github/dependabot.yml new file mode 100644 index 0000000..1a9554d --- /dev/null +++ b/.github/dependabot.yml @@ -0,0 +1,17 @@ +version: 2 +updates: +- package-ecosystem: "pip" + directory: "/" + schedule: + interval: "daily" + time: "00:00" + timezone: "Europe/London" + open-pull-requests-limit: 5 + +- package-ecosystem: "github-actions" + directory: "/" + schedule: + interval: "daily" + time: "00:00" + timezone: "Europe/London" + open-pull-requests-limit: 5 diff --git a/.github/workflows/python-app.yml b/.github/workflows/python-app.yml index 041de38..f82987e 100644 --- a/.github/workflows/python-app.yml +++ b/.github/workflows/python-app.yml @@ -15,9 +15,9 @@ jobs: runs-on: ubuntu-latest steps: - - uses: actions/checkout@v3 + - uses: actions/checkout@v4 - name: Set up Python 3.10 - uses: actions/setup-python@v2 + uses: actions/setup-python@v5 with: python-version: "3.10" - name: Install dependencies diff --git a/CHANGELOG.md b/CHANGELOG.md new file mode 100644 index 0000000..6991894 --- /dev/null +++ b/CHANGELOG.md @@ -0,0 +1,38 @@ +# Change Log for npg_porch Project + +The format is based on [Keep a Changelog](http://keepachangelog.com/). +This project adheres to [Semantic Versioning](http://semver.org/). + +## [Unreleased] + +## [2.0.0] - 2024-07-31 + +### Added + +* A note about installing on macOS +* An endpoint for creating pipeline tokens +* A development Docker file +* A Token model + +### Changed + +* Moved the project to the standard Python project layout. +* Reorganised and renamed the modules: + 1. `npg.porch` namespace is collapsed into `npg_porch`, + 2. `npg.porchdb` is reorganised into `npg_porch.db`, + 3. the entry point of the application is relocated and + renamed `npg/porch_server.py` -> `npg_porch/server.py` +* Moved the test configuration file to the `tests` directory. +* Changed the HTTP code for the `pipeline` endpoint from 409 to 422 in case of + bad input. +* A Token model is returned rather than a bare string by the endpoint which +creates tokens for a pipeline. +* A request to create an already existing task is no longer invalid, does not +raise an error. +* Updated the Task model so that it is impossible to create or update the task + without explicitly specifying the new task status. +* pysqlite3 is moved from the production to test dependencies. + +## [1.0.0] - 2023-10-16 + +Initial release diff --git a/README.md b/README.md index 37de3cd..26b7f64 100644 --- a/README.md +++ b/README.md @@ -36,15 +36,25 @@ To run the server, please execute the following from the root directory: ```bash bash pip3 install -e . -cd server +cd src mkdir -p logs export DB_URL=postgresql+asyncpg://npg_rw:$PASS@npg_porch_db:$PORT/$DATABASE export DB_SCHEMA='non_default' -uvicorn npg.main:app --host 0.0.0.0 --port 8080 --reload --log-config logging.json +uvicorn npg_porch.server:app --host 0.0.0.0 --port 8080 --reload --log-config logging.json ``` and open your browser at `http://localhost:8080` to see links to the docs. +On macOS you will need to ensure that a version of the `sqlite3` library that supports SQLite extensions +is used when installing the `pysqlite3` package. The system library on macOS does not, so an alternative +such as the one provided by MacPorts or Homebrew should be used. For example, when using MacPorts this +can be done by setting the `CPPFLAGS` environment variable before running the `pip install` command: + +``` +export CPPFLAGS="-I/opt/local/include" +``` + + The server will not start without `DB_URL` in the environment ## Running in production @@ -52,7 +62,7 @@ The server will not start without `DB_URL` in the environment When you want HTTPS, logging and all that jazz: ```bash -uvicorn main:app --workers 2 --host 0.0.0.0 --port 8080 --log-config ~/logging.json --ssl-keyfile ~/.ssh/key.pem --ssl-certfile ~/.ssh/cert.pem --ssl-ca-certs /usr/local/share/ca-certificates/institute_ca.crt +uvicorn server:app --workers 2 --host 0.0.0.0 --port 8080 --log-config ~/logging.json --ssl-keyfile ~/.ssh/key.pem --ssl-certfile ~/.ssh/cert.pem --ssl-ca-certs /usr/local/share/ca-certificates/institute_ca.crt ``` Consider running with nohup or similar. @@ -62,7 +72,7 @@ Some notes on arguments: --host: 0.0.0.0 = bind to all network interfaces. Reliable but greedy in some situations ---log-config: Refers to a JSON file for python logging library. An example file is found in /server/logging.json. Uvicorn provides its own logging configuration via `uvicorn.access` and `uvicorn.error`. These may behave undesirably, and can be overridden in the JSON file with an alternate config. Likewise, fastapi logs to `fastapi` if that needs filtering. For logging to files, set `use_colors = False` in the relevant handlers or shell colour settings will appear as garbage in the logs. +--log-config: Refers to a JSON file for python logging library. An example file is found in /src/logging.json. Uvicorn provides its own logging configuration via `uvicorn.access` and `uvicorn.error`. These may behave undesirably, and can be overridden in the JSON file with an alternate config. Likewise, fastapi logs to `fastapi` if that needs filtering. For logging to files, set `use_colors = False` in the relevant handlers or shell colour settings will appear as garbage in the logs. --ssl-keyfile: A PEM format key for the server certificate --ssl-certfile: A PEM format certificate for signing HTTPS communications @@ -77,11 +87,11 @@ pip install -e .[test] pytest ``` -Individual tests are run in the form `pytest server/tests/init_test.py` +Individual tests are run in the form `pytest tests/init_test.py` ### Fixtures -Fixtures reside under `server/tests/fixtures` and are registered in `server/tests/conftest.py` +Fixtures reside under `tests/fixtures` and are registered in `tests/conftest.py` They can also be listed by invoking `pytest --fixtures` Any fixtures that are not imported in `conftest.py` will not be detected. @@ -106,7 +116,7 @@ The SET command ensures that the new schema is visible _for one session only_ in DB=npg_porch export DB_URL=postgresql+psycopg2://npg_admin:$PASS@npg_porch_db:$PORT/$DB # note that the script requires a regular PG driver, not the async version showed above -server/deploy_schema.py +src/deploy_schema.py psql --host=npg_porch_db --port=$PORT --username=npg_admin --password -d $DB ``` diff --git a/docker/Dockerfile.dev b/docker/Dockerfile.dev new file mode 100644 index 0000000..cf4f678 --- /dev/null +++ b/docker/Dockerfile.dev @@ -0,0 +1,99 @@ + +ARG BASE_IMAGE=python:3.10-slim +FROM $BASE_IMAGE as builder + +ARG DEBIAN_FRONTEND="noninteractive" + +RUN apt-get update && \ + apt-get install -q -y --no-install-recommends \ + build-essential \ + gcc \ + libsqlite3-dev \ + unattended-upgrades && \ + unattended-upgrade -v + +WORKDIR /app + +COPY .. . + +RUN python -m venv /app && \ + . ./bin/activate && \ + pip install --no-cache-dir --upgrade pip && \ + pip install --no-cache-dir . + + +FROM $BASE_IMAGE + +ARG DEBIAN_FRONTEND + +RUN apt-get update && \ + apt-get upgrade -y && \ + apt-get install -q -y --no-install-recommends \ + libsqlite3-0 \ + postgresql \ + sudo \ + tini \ + locales && \ + locale-gen en_GB en_GB.UTF-8 && \ + localedef -i en_GB -c -f UTF-8 -A /usr/share/locale/locale.alias en_GB.UTF-8 + +RUN apt-get install -q -y --no-install-recommends \ + unattended-upgrades && \ + unattended-upgrade -v && \ + apt-get remove -q -y unattended-upgrades && \ + apt-get autoremove -q -y && \ + apt-get clean -q -y && \ + rm -rf /var/lib/apt/lists/* + +ENV LANG=en_GB.UTF-8 \ + LANGUAGE=en_GB \ + LC_ALL=en_GB.UTF-8 \ + TZ="Etc/UTC" + +ARG APP_USER=appuser +ARG APP_UID=1000 +ARG APP_GID=$APP_UID + +WORKDIR /app + +RUN groupadd --gid $APP_GID $APP_USER && \ + useradd --uid $APP_UID --gid $APP_GID --shell /bin/bash --create-home $APP_USER + +COPY --from=builder --chown=$APP_USER:$APP_GID /app /app + +ARG DB_HOST=localhost +ARG DB_PORT=5432 +ARG DB_SCHEMA=porch_dev +ARG DB_NAME=porch_dev_db +ARG DB_USER=porch_admin +ARG DB_PASS=porch +ARG URL_SLUG="$DB_USER:$DB_PASS@$DB_HOST:$DB_PORT/$DB_NAME" + +ENV DB_HOST=$DB_HOST \ + DB_PORT=$DB_PORT \ + DB_SCHEMA=$DB_SCHEMA \ + DB_NAME=$DB_NAME \ + DB_USER=$DB_USER \ + DB_PASS=$DB_PASS \ + DB_URL="postgresql+psycopg2://$URL_SLUG" + +RUN service postgresql start && \ + /app/docker/scripts/create_database.sh && \ + /app/docker/scripts/configure_database_service.sh && \ + . /app/bin/activate && \ + /app/scripts/deploy_schema.py && \ + /app/docker/scripts/insert_admin_token.sh && \ + service postgresql stop + +USER $APP_USER + +ARG PORT=8081 + +ENV DB_URL="postgresql+asyncpg://$URL_SLUG" \ + PORT=${PORT} + +EXPOSE ${PORT} + +ENTRYPOINT ["/usr/bin/tini", "--"] + +CMD ["/app/docker/scripts/entrypoint.sh"] diff --git a/docker/README.md b/docker/README.md new file mode 100644 index 0000000..73b0fe2 --- /dev/null +++ b/docker/README.md @@ -0,0 +1,19 @@ +# Development Dockerfile + +The Dockerfile and scripts in this directory may be use to create a development image +that hosts both the PostgreSQL database and the npg_porch server. + +The application is populated with a hard-coded administrator user, password and +administration token and is configured log to STDERR and STDOUT. + +To create an image using the Dockerfile, run the following command from the root of the +repository: + +```bash +docker build --rm -f docker/Dockerfile.dev -t npg_porch_dev . +``` + +The Dockerfile supports a number of arguments that can be passed to the `docker build` +command to configure most aspects of the application, including user names, passwords, +database names and ports. However, the default values should be suitable for most +needs. diff --git a/docker/logging.json b/docker/logging.json new file mode 100644 index 0000000..53723a0 --- /dev/null +++ b/docker/logging.json @@ -0,0 +1,51 @@ +{ + "version": 1, + "formatters": { + "default": { + "()": "uvicorn.logging.DefaultFormatter", + "fmt": "%(levelprefix)s %(message)s", + "use_colors": null + }, + "access": { + "()": "uvicorn.logging.AccessFormatter", + "fmt": "%(levelprefix)s %(client_addr)s - \"%(request_line)s\" %(status_code)s" + } + }, + "handlers": { + "stderr": { + "formatter": "default", + "class": "logging.StreamHandler", + "stream": "ext://sys.stderr" + }, + "stdout": { + "formatter": "default", + "class": "logging.StreamHandler", + "stream": "ext://sys.stdout" + }, + "access": { + "formatter": "access", + "class": "logging.StreamHandler", + "stream": "ext://sys.stdout" + } + }, + "loggers": { + "uvicorn": { + "handlers": ["access"], + "level": "INFO", + "propagate": false + }, + "uvicorn.error": { + "handlers": ["stderr"], + "level": "DEBUG", + "propagate": false + }, + "fastapi": { + "handlers": ["stderr"], + "level": "INFO" + } + }, + "root": { + "handlers": ["stdout"], + "level": "DEBUG" + } +} diff --git a/docker/scripts/configure_database_service.sh b/docker/scripts/configure_database_service.sh new file mode 100755 index 0000000..7df868c --- /dev/null +++ b/docker/scripts/configure_database_service.sh @@ -0,0 +1,12 @@ +#!/bin/bash + +set -eo pipefail +set -x + +APP_USER=${APP_USER:? The APP_USER environment variable must be set} + +cat > "/etc/sudoers.d/$APP_USER" << EOF +$APP_USER ALL= NOPASSWD: /usr/sbin/service postgresql start +$APP_USER ALL= NOPASSWD: /usr/sbin/service postgresql restart +$APP_USER ALL= NOPASSWD: /usr/sbin/service postgresql stop +EOF diff --git a/docker/scripts/create_database.sh b/docker/scripts/create_database.sh new file mode 100755 index 0000000..52d094b --- /dev/null +++ b/docker/scripts/create_database.sh @@ -0,0 +1,30 @@ +#!/bin/bash + +set -eo pipefail +set -x + +pg_isready --quiet || { + echo "PostgreSQL is not ready" >&2 + exit 1 +} + +DB_SCHEMA=${DB_SCHEMA:? The DB_SCHEMA environment variable must be set} +DB_NAME=${DB_NAME:? The DB_NAME environment variable must be set} +DB_USER=${DB_USER:? The DB_USER environment variable must be set} +DB_PASS=${DB_PASS:? The DB_PASS environment variable must be set} + +sudo -u postgres createuser -D -R -S ${DB_USER} +sudo -u postgres createdb -O ${DB_USER} ${DB_NAME} + +sudo -u postgres psql -d ${DB_NAME} << EOF +ALTER USER ${DB_USER} WITH PASSWORD '${DB_PASS}'; + +CREATE SCHEMA ${DB_SCHEMA}; + +SET search_path TO ${DB_SCHEMA}, public; + +GRANT ALL PRIVILEGES ON SCHEMA ${DB_SCHEMA} TO ${DB_USER}; +GRANT ALL PRIVILEGES ON ALL TABLES IN SCHEMA ${DB_SCHEMA} TO ${DB_USER}; +GRANT USAGE ON ALL SEQUENCES IN SCHEMA ${DB_SCHEMA} TO ${DB_USER}; +GRANT SELECT, INSERT, UPDATE, DELETE ON ALL TABLES IN SCHEMA ${DB_SCHEMA} TO ${DB_USER}; +EOF diff --git a/docker/scripts/entrypoint.sh b/docker/scripts/entrypoint.sh new file mode 100755 index 0000000..777f43a --- /dev/null +++ b/docker/scripts/entrypoint.sh @@ -0,0 +1,16 @@ +#!/bin/bash + +set -eo pipefail + +sudo service postgresql start + +pg_isready --quiet --timeout=30 || { + echo "PostgreSQL is not ready" >&2 + exit 1 +} + +PORT=${PORT:? The PORT environment variable must be set} + +source /app/bin/activate + +uvicorn npg_porch.server:app --host 0.0.0.0 --port ${PORT} --reload --log-config /app/docker/logging.json diff --git a/docker/scripts/insert_admin_token.sh b/docker/scripts/insert_admin_token.sh new file mode 100755 index 0000000..34ef944 --- /dev/null +++ b/docker/scripts/insert_admin_token.sh @@ -0,0 +1,18 @@ +#!/bin/bash + +set -eo pipefail +set -x + +pg_isready --quiet || { + echo "PostgreSQL is not ready" >&2 + exit 1 +} + +DB_SCHEMA=${DB_SCHEMA:? The DB_SCHEMA environment variable must be set} +DB_NAME=${DB_NAME:? The DB_NAME environment variable must be set} + +ADMIN_TOKEN=${ADMIN_TOKEN:="00000000000000000000000000000000"} + +sudo -u postgres psql -d ${DB_NAME} << EOF +INSERT INTO ${DB_SCHEMA}."token" (token, description, date_issued) VALUES ('${ADMIN_TOKEN}', 'Admin token', NOW()); +EOF diff --git a/docs/user_guide.md b/docs/user_guide.md index 73e50ba..e4f762c 100644 --- a/docs/user_guide.md +++ b/docs/user_guide.md @@ -22,19 +22,33 @@ Security is necessary in order to prevent accidental misuse of npg_porch. An aut A note on HTTPS: Client libraries like `requests`, certain GUIs and Firefox will try to verify the server certificate authority. System-administered software are already configured correctly, but other packages installed by conda or pip may need to be told how the client may verify with a client certificate e.g. contained in `/usr/share/ca-certificates/`. It may also be useful to unset `https_proxy` and `HTTPS_PROXY` in your environment. -### Step 0 - get issued security tokens +### Step 0 - get issued authorisation tokens Access to the service is loosely controlled with authorisation tokens. You will be issued with an admin token that enables you to register pipelines, and further tokens for pipeline-specific communication. Please do not share the tokens around and use them for purposes besides the specific pipeline. This will help us to monitor pipeline reliability and quality of service. Authorisation is achieved by HTTP Bearer Token: `curl -L -H "Authorization: Bearer $TOKEN" https://$SERVER:$PORT` +Authorisation tokens are specific to a pipeline and more than one token can be issued for a pipeline. New tokens for a pipeline can be obtained using the admin token, from the pipeline's token endpoint: + +`curl -L -X POST -H "Authorization: Bearer $ADMIN_TOKEN" https://$SERVER:$PORT/pipelines/$PIPELINE_NAME/token/$TOKEN_DESCRIPTION` + +The server will respond with a JSON document containing the new bearer token which you may use for subsequent pipeline-specific communication: + +```javascript +{ + "name": "$PIPELINE_NAME", + "description": "$TOKEN_DESCRIPTION", + "token": "$TOKEN" +} +``` + ### Step 1 - register your pipeline with npg_porch -*Schema: npg.porch.model.pipeline* +*Schema: npg_porch.model.pipeline* Nothing in npg_porch can happen until there's a pipeline defined. For our purposes "pipeline" means "a thing you can run", and it may refer to specific code, or a wrapper that can run the pipeline in this particular way with some standard arguments. -You can name your pipeline however you like, but the name must be unique, and be as informative to you as possible. Version and a URI can be useful for undestanding what code is being run. +You can name your pipeline however you like, but the name must be unique, and be as informative to you as possible. Version and a URI can be useful for understanding what code is being run. **pipeline-def.json** @@ -50,7 +64,7 @@ You can name your pipeline however you like, but the name must be unique, and be Keep this pipeline definition with your data, as you will need it to tell npg_porch which pipeline you are acting on. -When communicating with npg_porch (as with any HTTP server) you must inspect the response code and message after each communication. See `-w " %{http_code}" above. The API documentation lists the response codes you can expect to have to handle. In this case, the server may respond with 400 - BAD REQUEST if you leave out a name, or 409 - CONFLICT if you chose a name that is already created. +As with any HTTP server, when communicating with npg_porch you must inspect the response code and message after each communication. See `-w " %{http_code}" above. The API documentation lists the response codes you can expect to have to handle. In this case, the server may respond with 400 - BAD REQUEST if you leave out a name, or 409 - CONFLICT if you chose a name that is already created. ### Step 2 - decide on the unique criteria for running the pipeline @@ -58,7 +72,7 @@ e.g. Once per 24 hours, poll iRODS metadata for data relating to a study. We might create a cronjob that runs a script. It invokes `imeta` and retrieves a list of results. Now we turn each of those results into a JSON document to our own specification: -*Schema: npg.porch.model.task* +*Schema: npg_porch.model.task* **study-100-id-run-45925.json** @@ -113,7 +127,7 @@ Note that it is possible to run the same `task_input` with a different `pipeline ### Step 3 - register the documents with npg_porch -*Schema: npg.porch.model.task* +*Schema: npg_porch.model.task* Now you want the pipeline to run once per specification, and so register the documents with npg_porch. @@ -144,8 +158,6 @@ $request->content($DOC); my $response = $ua->request($request); if ($response->is_success) { print "Submitted successfully\n"; -} elsif ($response->code == 409 ){ - print "Already exists, that's fine.\n"; } else { die q(It's all gone wrong!) } @@ -169,18 +181,10 @@ if ($response->is_success) { } ``` -If you get a 409 response, it is highly likely that this particular task is already registered. In this way it is possible to tell whether something has already been submitted. Note that if there are many many tasks to register some of which were submitted previously, further work is required to make the process efficient - such as to ask the npg_porch server for a list of previously registered tasks for this pipeline. - -**Example 409 failure response** - -```javascript -{ - "detail": "Unable to create task, as another like it already exists" -} -``` - Once a task has been submitted, and a 201 CREATED response has been received, the npg_porch server assigns a timestamp to the task, gives it a status of `PENDING` and assigns a unique ID to it. The response from the server contains this extra information. +A 200 OK response means that this particular task for this pipeline has already been registered. The current representation of the task is returned, the status of the task might be differ from `PENDING`. Note that if there are many tasks to register, some of which were submitted previously, further work is required to make the process efficient - such as to ask the npg_porch server for a list of previously registered tasks for this pipeline. + ### Step 4 - write a script or program that can launch the pipeline Supposing there are new tasks created every 24 hours, we then also need a client that checks every 24 hours for new work it can run on a compute farm. diff --git a/pyproject.toml b/pyproject.toml index 933ec91..e14255e 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -14,7 +14,6 @@ dependencies = [ "asyncpg", "fastapi", "pydantic > 2.0.0", - "pysqlite3", "psycopg2-binary", "sqlalchemy >2", "ujson", @@ -25,6 +24,7 @@ dynamic = ["version"] [project.optional-dependencies] test = [ + "pysqlite3", "pytest", "pytest-asyncio", "requests", diff --git a/scripts/deploy_schema.py b/scripts/deploy_schema.py old mode 100644 new mode 100755 index 9f56d71..2d264ed --- a/scripts/deploy_schema.py +++ b/scripts/deploy_schema.py @@ -1,9 +1,11 @@ +#!/usr/bin/env python + # Replace with Alembic in due course import os import sqlalchemy -import npg.porchdb.models +import npg_porch.db.models db_url = os.environ.get('DB_URL') schema_name = os.environ.get('DB_SCHEMA') @@ -17,5 +19,5 @@ connect_args={'options': f'-csearch_path={schema_name}'} ) -npg.porchdb.models.Base.metadata.schema = schema_name -npg.porchdb.models.Base.metadata.create_all(engine) +npg_porch.db.models.Base.metadata.schema = schema_name +npg_porch.db.models.Base.metadata.create_all(engine) diff --git a/scripts/issue_token.py b/scripts/issue_token.py index b31e4db..6300f1a 100755 --- a/scripts/issue_token.py +++ b/scripts/issue_token.py @@ -5,7 +5,7 @@ from sqlalchemy.orm import sessionmaker from sqlalchemy.orm.exc import NoResultFound -from npg.porchdb.models import Token, Pipeline +from npg_porch.db.models import Token, Pipeline parser = argparse.ArgumentParser( description='Creates a token in the backend DB and returns it' diff --git a/server/npg/porch/endpoints/__init__.py b/server/npg/porch/endpoints/__init__.py deleted file mode 100644 index e69de29..0000000 diff --git a/server/npg/porchdb/__init__.py b/server/npg/porchdb/__init__.py deleted file mode 100644 index e69de29..0000000 diff --git a/setup.cfg b/setup.cfg deleted file mode 100644 index 9eb1408..0000000 --- a/setup.cfg +++ /dev/null @@ -1,11 +0,0 @@ -[metadata] -name = npg_porch -version = 1.0.0 - -[options] -package_dir = - =server -packages = find: - -[options.packages.find] -where=server \ No newline at end of file diff --git a/server/logging.json b/src/logging.json similarity index 100% rename from server/logging.json rename to src/logging.json diff --git a/server/__init__.py b/src/npg_porch/__init__.py similarity index 100% rename from server/__init__.py rename to src/npg_porch/__init__.py diff --git a/server/npg/__init__.py b/src/npg_porch/auth/__init__.py similarity index 100% rename from server/npg/__init__.py rename to src/npg_porch/auth/__init__.py diff --git a/server/npg/porch/auth/token.py b/src/npg_porch/auth/token.py similarity index 91% rename from server/npg/porch/auth/token.py rename to src/npg_porch/auth/token.py index 49cf89c..e495d19 100644 --- a/server/npg/porch/auth/token.py +++ b/src/npg_porch/auth/token.py @@ -23,8 +23,8 @@ from fastapi.security import HTTPBearer from fastapi import HTTPException -from npg.porchdb.connection import get_CredentialsValidator -from npg.porchdb.auth import CredentialsValidationException +from npg_porch.db.connection import get_CredentialsValidator +from npg_porch.db.auth import CredentialsValidationException auth_scheme = HTTPBearer() @@ -34,7 +34,7 @@ async def validate( ): token = creds.credentials - p = None + try: p = await validator.token2permission(token) except CredentialsValidationException as e: diff --git a/server/npg/porch/__init__.py b/src/npg_porch/db/__init__.py similarity index 100% rename from server/npg/porch/__init__.py rename to src/npg_porch/db/__init__.py diff --git a/server/npg/porchdb/auth.py b/src/npg_porch/db/auth.py similarity index 95% rename from server/npg/porchdb/auth.py rename to src/npg_porch/db/auth.py index 3d30ef5..9bf9029 100644 --- a/server/npg/porchdb/auth.py +++ b/src/npg_porch/db/auth.py @@ -23,8 +23,8 @@ from sqlalchemy.orm import contains_eager from sqlalchemy.orm.exc import NoResultFound -from npg.porchdb.models import Token -from npg.porch.models.permission import Permission, RolesEnum +from npg_porch.db.models import Token +from npg_porch.models.permission import Permission, RolesEnum __AUTH_TOKEN_LENGTH__ = 32 __AUTH_TOKEN_REGEXP__ = re.compile( @@ -55,7 +55,6 @@ async def token2permission(self, token: str): 'Token failed character validation' ) - valid_token_row = None try: # Using 'outerjoin' to get the left join for token, pipeline. # We need to retrieve all token rows, regardless of whether @@ -74,7 +73,6 @@ async def token2permission(self, token: str): if (valid_token_row is not None) and (valid_token_row.date_revoked is not None): raise CredentialsValidationException('A revoked token is used') - permission = None pipeline = valid_token_row.pipeline token_id = valid_token_row.token_id if pipeline is None: diff --git a/server/npg/porchdb/connection.py b/src/npg_porch/db/connection.py similarity index 90% rename from server/npg/porchdb/connection.py rename to src/npg_porch/db/connection.py index 1374db3..fea8c3e 100644 --- a/server/npg/porchdb/connection.py +++ b/src/npg_porch/db/connection.py @@ -22,9 +22,9 @@ from sqlalchemy.orm import sessionmaker from sqlalchemy.ext.asyncio import AsyncSession, create_async_engine -from npg.porchdb.models import Base -from npg.porchdb.data_access import AsyncDbAccessor -from npg.porchdb.auth import Validator +from npg_porch.db.models import Base +from npg_porch.db.data_access import AsyncDbAccessor +from npg_porch.db.auth import Validator config = { 'DB_URL': os.environ.get('DB_URL'), @@ -34,7 +34,6 @@ if config['TEST']: config['DB_URL'] = 'sqlite+aiosqlite:///:memory:' - # config['DB_URL'] = 'sqlite+aiosqlite:///test.db' if config['DB_URL'] is None or config['DB_URL'] == '': raise Exception( @@ -92,6 +91,3 @@ async def deploy_schema(): async def close_engine(): 'Currently only needed when testing to force fixtures to refresh' await engine.dispose() - # Delete the data here for stateless testingĀ if not in-memory - # if config['TEST']: - # os.remove('test.db') diff --git a/server/npg/porchdb/data_access.py b/src/npg_porch/db/data_access.py similarity index 75% rename from server/npg/porchdb/data_access.py rename to src/npg_porch/db/data_access.py index effd0e4..2e71804 100644 --- a/server/npg/porchdb/data_access.py +++ b/src/npg_porch/db/data_access.py @@ -1,4 +1,4 @@ -# Copyright (C) 2021, 2022 Genome Research Ltd. +# Copyright (C) 2021, 2022, 2024 Genome Research Ltd. # # Author: Kieron Taylor kt19@sanger.ac.uk # Author: Marina Gourtovaia mg8@sanger.ac.uk @@ -19,13 +19,18 @@ # this program. If not, see . import logging + from sqlalchemy import select from sqlalchemy.exc import IntegrityError from sqlalchemy.orm import contains_eager, joinedload from sqlalchemy.orm.exc import NoResultFound -from npg.porchdb.models import Pipeline as DbPipeline, Task as DbTask, Event -from npg.porch.models import Task, Pipeline, TaskStateEnum +from npg_porch.db.models import Event +from npg_porch.db.models import Pipeline as DbPipeline +from npg_porch.db.models import Task as DbTask +from npg_porch.db.models import Token as DbToken +from npg_porch.models import Pipeline, Task, TaskStateEnum +from npg_porch.models.token import Token class AsyncDbAccessor: @@ -74,7 +79,6 @@ async def get_all_pipelines( uri: str | None = None, version: str | None = None ) -> list[Pipeline]: - pipelines = [] pipelines = await self._get_pipeline_db_objects(uri=uri, version=version) return [pipe.convert_to_model() for pipe in pipelines] @@ -92,28 +96,54 @@ async def create_pipeline(self, pipeline: Pipeline) -> Pipeline: await session.commit() return pipe.convert_to_model() - async def create_task(self, token_id: int, task: Task) -> Task: + async def create_pipeline_token(self, name: str, desc: str) -> Token: + session = self.session + db_pipeline = await self._get_pipeline_db_object(name) + + db_token = DbToken(pipeline=db_pipeline, description=desc) + session.add(db_token) + await session.commit() + + return Token(name=db_pipeline.name, token=db_token.token, description=desc) + + async def create_task(self, token_id: int, task: Task) -> tuple[Task, bool]: + '''Given a task definition creates a task. + + If the task does not exist, a tuple consisting of Task object for a + newly created database record and a boolean True object is returned. + + If the task already exists, a tuple consisting of Task object for an + existing database record and a boolean True object is returned. + ''' self.logger.debug('CREATE TASK: ' + str(task)) session = self.session db_pipeline = await self._get_pipeline_db_object( task.pipeline.name ) - # Check they exist and so on - task.status = TaskStateEnum.PENDING + task.status = TaskStateEnum.PENDING t = self.convert_task_to_db(task, db_pipeline) - session.add(t) - - event = Event( - task=t, - token_id = token_id, - change='Created' - ) - t.events.append(event) + created = True + try: + nested = await session.begin_nested() + session.add(t) + event = Event( + task=t, + token_id = token_id, + change='Created' + ) + t.events.append(event) + await session.commit() + except IntegrityError: + await nested.rollback() + # Task already exists, query the database to get the up-to-date + # representation of the task. + t = await self.get_db_task( + pipeline_name=task.pipeline.name, job_descriptor=t.job_descriptor + ) + created = False - await session.commit() - # Error handling to follow - return t.convert_to_model() + return (t.convert_to_model(), created) async def claim_tasks( self, token_id: int, pipeline: Pipeline, claim_limit: int | None = 1 @@ -217,6 +247,21 @@ async def get_tasks( tasks = task_result.scalars().all() return [t.convert_to_model() for t in tasks] + async def get_db_task( + self, + pipeline_name: str, + job_descriptor: str, + ) -> DbTask: + '''Get the task.''' + query = select(DbTask)\ + .join(DbTask.pipeline)\ + .options(joinedload(DbTask.pipeline))\ + .where(DbPipeline.name == pipeline_name)\ + .where(DbTask.job_descriptor == job_descriptor) + task_result = await self.session.execute(query) + + return task_result.scalars().one() + @staticmethod def convert_task_to_db(task: Task, pipeline: DbPipeline) -> DbTask: assert task.status in TaskStateEnum diff --git a/server/npg/porchdb/models/__init__.py b/src/npg_porch/db/models/__init__.py similarity index 100% rename from server/npg/porchdb/models/__init__.py rename to src/npg_porch/db/models/__init__.py diff --git a/server/npg/porchdb/models/base.py b/src/npg_porch/db/models/base.py similarity index 100% rename from server/npg/porchdb/models/base.py rename to src/npg_porch/db/models/base.py diff --git a/server/npg/porchdb/models/event.py b/src/npg_porch/db/models/event.py similarity index 100% rename from server/npg/porchdb/models/event.py rename to src/npg_porch/db/models/event.py diff --git a/server/npg/porchdb/models/pipeline.py b/src/npg_porch/db/models/pipeline.py similarity index 96% rename from server/npg/porchdb/models/pipeline.py rename to src/npg_porch/db/models/pipeline.py index 1f03d5d..b4803a6 100644 --- a/server/npg/porchdb/models/pipeline.py +++ b/src/npg_porch/db/models/pipeline.py @@ -25,7 +25,7 @@ from .base import Base -from npg.porch.models import Pipeline as ModeledPipeline +from npg_porch.models import Pipeline as ModeledPipeline class Pipeline(Base): ''' diff --git a/server/npg/porchdb/models/task.py b/src/npg_porch/db/models/task.py similarity index 98% rename from server/npg/porchdb/models/task.py rename to src/npg_porch/db/models/task.py index dd146eb..91a7550 100644 --- a/server/npg/porchdb/models/task.py +++ b/src/npg_porch/db/models/task.py @@ -26,7 +26,7 @@ from sqlalchemy.sql.sqltypes import DateTime from .base import Base -from npg.porch.models import Task as ModelledTask +from npg_porch.models import Task as ModelledTask class Task(Base): diff --git a/server/npg/porchdb/models/token.py b/src/npg_porch/db/models/token.py similarity index 95% rename from server/npg/porchdb/models/token.py rename to src/npg_porch/db/models/token.py index db9bdd2..aa3d725 100644 --- a/server/npg/porchdb/models/token.py +++ b/src/npg_porch/db/models/token.py @@ -29,11 +29,11 @@ class Token(Base): ''' - A string token ussued to client applications for the purpose of + A string token issued to client applications for the purpose of authorizing them to perform certain actions. ''' - def random_token(): + def random_token(self): ''' Returns a 32 characters long random string. The chance of a collision is small. diff --git a/server/npg/porch/auth/__init__.py b/src/npg_porch/endpoints/__init__.py similarity index 100% rename from server/npg/porch/auth/__init__.py rename to src/npg_porch/endpoints/__init__.py diff --git a/server/npg/porch/endpoints/pipelines.py b/src/npg_porch/endpoints/pipelines.py similarity index 75% rename from server/npg/porch/endpoints/pipelines.py rename to src/npg_porch/endpoints/pipelines.py index d1afcd7..8ab140e 100644 --- a/server/npg/porch/endpoints/pipelines.py +++ b/src/npg_porch/endpoints/pipelines.py @@ -1,4 +1,4 @@ -# Copyright (C) 2021, 2022 Genome Research Ltd. +# Copyright (C) 2021, 2022, 2024 Genome Research Ltd. # # Author: Kieron Taylor kt19@sanger.ac.uk # Author: Marina Gourtovaia mg8@sanger.ac.uk @@ -18,18 +18,19 @@ # You should have received a copy of the GNU General Public License along with # this program. If not, see . -from fastapi import APIRouter, HTTPException, Depends import logging import re + +from fastapi import APIRouter, Depends, HTTPException from sqlalchemy.exc import IntegrityError from sqlalchemy.orm.exc import NoResultFound from starlette import status -from npg.porch.models.pipeline import Pipeline -from npg.porch.models.permission import RolesEnum -from npg.porchdb.connection import get_DbAccessor -from npg.porch.auth.token import validate - +from npg_porch.auth.token import validate +from npg_porch.db.connection import get_DbAccessor +from npg_porch.models.permission import RolesEnum +from npg_porch.models.pipeline import Pipeline +from npg_porch.models.token import Token router = APIRouter( prefix="/pipelines", @@ -84,6 +85,33 @@ async def get_pipeline( return pipeline +@router.post( + "/{pipeline_name}/token/{token_desc}", + response_model=Token, + status_code=status.HTTP_201_CREATED, + responses={ + status.HTTP_201_CREATED: {"description": "Token was created"}, + status.HTTP_404_NOT_FOUND: {"description": "Pipeline not found"}, + }, + summary="Create a new token for a pipeline and return it.", + description=""" + Returns a token for the pipeline with the given name. + A valid token issued for any pipeline is required for authorisation.""" +) +async def create_pipeline_token( + pipeline_name: str, + token_desc: str, + db_accessor=Depends(get_DbAccessor), + permissions=Depends(validate) +) -> Token: + try: + token = await db_accessor.create_pipeline_token(name=pipeline_name, desc=token_desc) + except NoResultFound: + raise HTTPException(status_code=404, + detail=f"Pipeline '{pipeline_name}' not found") + return token + + @router.post( "/", response_model=Pipeline, @@ -108,7 +136,6 @@ async def create_pipeline( logging.error(f"Role {RolesEnum.POWER_USER} is required") raise HTTPException(status_code=403) - new_pipeline = None try: new_pipeline = await db_accessor.create_pipeline(pipeline) except IntegrityError as e: diff --git a/server/npg/porch/endpoints/tasks.py b/src/npg_porch/endpoints/tasks.py similarity index 84% rename from server/npg/porch/endpoints/tasks.py rename to src/npg_porch/endpoints/tasks.py index 3a57d2a..e601517 100644 --- a/server/npg/porch/endpoints/tasks.py +++ b/src/npg_porch/endpoints/tasks.py @@ -22,15 +22,16 @@ from typing import Annotated from fastapi import APIRouter, Depends, HTTPException, Query -from npg.porch.auth.token import validate -from npg.porch.models.permission import PermissionValidationException -from npg.porch.models.pipeline import Pipeline -from npg.porch.models.task import Task, TaskStateEnum -from npg.porchdb.connection import get_DbAccessor -from sqlalchemy.exc import IntegrityError +from fastapi.responses import JSONResponse from sqlalchemy.orm.exc import NoResultFound from starlette import status +from npg_porch.auth.token import validate +from npg_porch.db.connection import get_DbAccessor +from npg_porch.models.permission import PermissionValidationException +from npg_porch.models.pipeline import Pipeline +from npg_porch.models.task import Task, TaskStateEnum + def _validate_request(permission, pipeline): @@ -82,14 +83,20 @@ async def get_tasks( status_code=status.HTTP_201_CREATED, responses={ status.HTTP_201_CREATED: {"description": "Task creation was successful"}, - status.HTTP_409_CONFLICT: {"description": "A task with the same signature already exists"} + status.HTTP_200_OK: {"description": "A task with the same signature already exists"}, + status.HTTP_404_NOT_FOUND: {"description": "Pipeline does not exist."}, }, summary="Creates one task record.", description=''' Given a Task object, creates a database record for it and returns - the same object, the response HTTP status is 201 'Created'. The + a new Task object. The response HTTP status is 201 'Created'. The new task is assigned pending status, ie becomes available for claiming. + A request to create a task for which the database record already exists + is accepted. The return status code 200 is set in this case. The returned + Task object has its status set to the value currently available in the + database. + The pipeline specified by the `pipeline` attribute of the Task object should exist. If it does not exist, return status 404 'Not found'.''' ) @@ -100,21 +107,18 @@ async def create_task( ) -> Task: _validate_request(permission, task.pipeline) - created_task = None + try: - created_task = await db_accessor.create_task( + (task, created) = await db_accessor.create_task( token_id=permission.requestor_id, task=task ) - except IntegrityError: - raise HTTPException( - status_code=409, - detail='Unable to create task, as another like it already exists' - ) except NoResultFound: raise HTTPException(status_code=404, detail='Failed to find pipeline for this task') - return created_task + if created is True: + return task + return JSONResponse(status_code=status.HTTP_200_OK, content=task.model_dump()) @router.put( @@ -137,7 +141,7 @@ async def update_task( ) -> Task: _validate_request(permission, task.pipeline) - changed_task = None + try: changed_task = await db_accessor.update_task( token_id=permission.requestor_id, diff --git a/server/npg/porch/models/__init__.py b/src/npg_porch/models/__init__.py similarity index 100% rename from server/npg/porch/models/__init__.py rename to src/npg_porch/models/__init__.py diff --git a/server/npg/porch/models/permission.py b/src/npg_porch/models/permission.py similarity index 98% rename from server/npg/porch/models/permission.py rename to src/npg_porch/models/permission.py index df5b62c..783235f 100644 --- a/server/npg/porch/models/permission.py +++ b/src/npg_porch/models/permission.py @@ -22,7 +22,7 @@ from pydantic import BaseModel, Field, field_validator, FieldValidationInfo from typing import Optional -from npg.porch.models.pipeline import Pipeline +from npg_porch.models.pipeline import Pipeline class PermissionValidationException(Exception): diff --git a/server/npg/porch/models/pipeline.py b/src/npg_porch/models/pipeline.py similarity index 88% rename from server/npg/porch/models/pipeline.py rename to src/npg_porch/models/pipeline.py index ce212b8..ffdea9b 100644 --- a/server/npg/porch/models/pipeline.py +++ b/src/npg_porch/models/pipeline.py @@ -1,4 +1,4 @@ -# Copyright (C) 2021, 2022 Genome Research Ltd. +# Copyright (C) 2021, 2022, 2024 Genome Research Ltd. # # Author: Kieron Taylor kt19@sanger.ac.uk # Author: Marina Gourtovaia mg8@sanger.ac.uk @@ -18,9 +18,11 @@ # You should have received a copy of the GNU General Public License along with # this program. If not, see . -from pydantic import BaseModel, Field +from pydantic import BaseModel, ConfigDict, Field class Pipeline(BaseModel): + model_config = ConfigDict(extra='forbid') + name: str = Field( default = None, title='Pipeline Name', diff --git a/server/npg/porch/models/task.py b/src/npg_porch/models/task.py similarity index 90% rename from server/npg/porch/models/task.py rename to src/npg_porch/models/task.py index 788518d..64c4147 100644 --- a/server/npg/porch/models/task.py +++ b/src/npg_porch/models/task.py @@ -21,9 +21,9 @@ from enum import Enum import hashlib import ujson -from pydantic import BaseModel, Field +from pydantic import BaseModel, Field, ValidationError -from npg.porch.models.pipeline import Pipeline +from npg_porch.models.pipeline import Pipeline class TaskStateEnum(str, Enum): PENDING = 'PENDING' @@ -45,7 +45,7 @@ class Task(BaseModel): title='Task Input', description='A structured parameter set that uniquely identifies a piece of work, and enables an iteration of a pipeline' # noqa: E501 ) - status: TaskStateEnum | None = None + status: TaskStateEnum def generate_task_id(self): return hashlib.sha256(ujson.dumps(self.task_input, sort_keys=True).encode()).hexdigest() @@ -56,18 +56,21 @@ def __eq__(self, other): The pipeline and task_input_ids can partially differ and it still be a valid comparison. Clients do not get to create task_input_ids and may - not fully specify a pipeline. Status is also optional + not fully specify a pipeline. Automatically attempts to cast a dict into a Task, and therefore ignores any properties not valid for a Task ''' - if not isinstance(other, Task): - if isinstance(other, dict): + if isinstance(other, dict): + try: other = Task.model_validate(other) - else: + except ValidationError: return False + if not isinstance(other, Task): + return False + truths = [] for k, v in self.model_dump().items(): other_d = other.model_dump() @@ -81,5 +84,5 @@ def __eq__(self, other): truths.append(v == other_d[k]) if all(truths): return True - else: - return False + + return False diff --git a/src/npg_porch/models/token.py b/src/npg_porch/models/token.py new file mode 100644 index 0000000..c84e0dd --- /dev/null +++ b/src/npg_porch/models/token.py @@ -0,0 +1,38 @@ +# Copyright (C) 2024 Genome Research Ltd. +# +# This file is part of npg_porch +# +# npg_porch is free software: you can redistribute it and/or modify it +# under the terms of the GNU General Public License as published by the Free +# Software Foundation; either version 3 of the License, or (at your option) any +# later version. +# +# This program is distributed in the hope that it will be useful, but WITHOUT +# ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or FITNESS +# FOR A PARTICULAR PURPOSE. See the GNU General Public License for more +# details. +# +# You should have received a copy of the GNU General Public License along with +# this program. If not, see . + +from pydantic import BaseModel, ConfigDict, Field + + +class Token(BaseModel): + model_config = ConfigDict(extra='forbid') + + name: str = Field( + default=None, + title='Pipeline Name', + description='A user-controlled name of an existing pipeline' + ) + description: str | None = Field( + default=None, + title='Description', + description='A user-controlled description of the token' + ) + token: str | None = Field( + default=None, + title='Token', + description='An authorisation token' + ) diff --git a/server/npg/main.py b/src/npg_porch/server.py similarity index 97% rename from server/npg/main.py rename to src/npg_porch/server.py index 836928b..c7a9f8a 100644 --- a/server/npg/main.py +++ b/src/npg_porch/server.py @@ -21,7 +21,7 @@ from fastapi import FastAPI from fastapi.responses import HTMLResponse -from npg.porch.endpoints import pipelines, tasks +from npg_porch.endpoints import pipelines, tasks #https://fastapi.tiangolo.com/tutorial/bigger-applications/ #https://fastapi.tiangolo.com/tutorial/metadata diff --git a/tests/__init__.py b/tests/__init__.py deleted file mode 100644 index e69de29..0000000 diff --git a/tests/conftest.py b/tests/conftest.py index 0a4ef75..337f633 100644 --- a/tests/conftest.py +++ b/tests/conftest.py @@ -1,5 +1,5 @@ -from .fixtures.orm_session import sync_session, async_session -from .fixtures.deploy_db import ( +from fixtures.orm_session import sync_session, async_session +from fixtures.deploy_db import ( sync_minimum, async_minimum, minimum_data, diff --git a/tests/data_access_test.py b/tests/data_access_test.py index c463b0c..b2c2e5f 100644 --- a/tests/data_access_test.py +++ b/tests/data_access_test.py @@ -1,12 +1,13 @@ +import re + import pytest +from npg_porch.db.data_access import AsyncDbAccessor +from npg_porch.models import Pipeline as ModelledPipeline +from npg_porch.models import Task, TaskStateEnum from pydantic import ValidationError -import re from sqlalchemy.exc import IntegrityError from sqlalchemy.orm.exc import NoResultFound -from npg.porchdb.data_access import AsyncDbAccessor -from npg.porch.models import Pipeline as ModelledPipeline, Task, TaskStateEnum - def give_me_a_pipeline(number: int = 1): return ModelledPipeline( @@ -32,7 +33,7 @@ async def test_get_pipeline(db_accessor): db_accessor.get_pipeline_by_name() with pytest.raises(NoResultFound): - pipeline = await db_accessor.get_pipeline_by_name('not here') + await db_accessor.get_pipeline_by_name('not here') pipeline = await db_accessor.get_pipeline_by_name('ptest one') assert pipeline @@ -76,7 +77,7 @@ async def test_create_pipeline(db_accessor): assert saved_pipeline.uri == pipeline.uri with pytest.raises(AssertionError): - saved_pipeline = await db_accessor.create_pipeline({}) + await db_accessor.create_pipeline({}) with pytest.raises(IntegrityError) as exception: # Making duplicate provides a useful error await db_accessor.create_pipeline(pipeline) @@ -98,14 +99,15 @@ async def test_create_task(db_accessor): task = Task( pipeline=saved_pipeline, - task_input={'test': True} + task_input={'test': True}, + status=TaskStateEnum.PENDING ) - saved_task = await db_accessor.create_task( + (saved_task, created) = await db_accessor.create_task( token_id=1, task=task ) - + assert created is True assert saved_task.status == TaskStateEnum.PENDING, 'State automatically set to PENDING' assert saved_task.pipeline.name == 'ptest 1' assert saved_task.task_input_id, 'Input ID is created automatically' @@ -114,10 +116,12 @@ async def test_create_task(db_accessor): assert len(events) == 1, 'An event was created with a successful task creation' assert events[0].change == 'Created', 'Message set' - with pytest.raises(IntegrityError) as exception: - await db_accessor.create_task(1, task) - - assert re.match('UNIQUE constraint failed', exception.value) + (existing_task, created) = await db_accessor.create_task(1, task) + assert created is False + assert existing_task.status == TaskStateEnum.PENDING, 'State automatically set to PENDING' + assert existing_task.pipeline.name == 'ptest 1' + events = await db_accessor.get_events_for_task(existing_task) + assert len(events) == 1, 'No additional events' @pytest.mark.asyncio async def test_claim_tasks(db_accessor): @@ -125,7 +129,7 @@ async def test_claim_tasks(db_accessor): pipeline = give_me_a_pipeline() with pytest.raises(NoResultFound) as exception: - tasks = await db_accessor.claim_tasks(1, pipeline) + await db_accessor.claim_tasks(1, pipeline) assert exception.value == 'Pipeline not found' @@ -141,7 +145,8 @@ async def test_claim_tasks(db_accessor): token_id=1, task=Task( task_input={'number': i + 1}, - pipeline=pipeline + pipeline=pipeline, + status=TaskStateEnum.PENDING ) ) @@ -176,14 +181,16 @@ async def test_multi_claim_tasks(db_accessor): token_id=1, task=Task( task_input={'number': i + 1}, - pipeline=pipeline + pipeline=pipeline, + status=TaskStateEnum.PENDING ) ) await db_accessor.create_task( token_id=2, task=Task( task_input={'number': i + 1}, - pipeline=other_pipeline + pipeline=other_pipeline, + status=TaskStateEnum.PENDING ) ) @@ -201,11 +208,12 @@ async def test_multi_claim_tasks(db_accessor): @pytest.mark.asyncio async def test_update_tasks(db_accessor): saved_pipeline = await store_me_a_pipeline(db_accessor) - saved_task = await db_accessor.create_task( + (saved_task, created) = await db_accessor.create_task( token_id=1, task=Task( task_input={'number': 1}, - pipeline=saved_pipeline + pipeline=saved_pipeline, + status=TaskStateEnum.PENDING ) ) @@ -216,11 +224,13 @@ async def test_update_tasks(db_accessor): events = await db_accessor.get_events_for_task(modified_task) assert len(events) == 2, 'Task was created, and then updated' - events[1].change == 'Task changed, new status DONE' + assert events[1].change == 'Task changed, new status DONE' # Try to change a task that doesn't exist with pytest.raises(NoResultFound): - await db_accessor.update_task(1, Task(task_input={'number': None}, pipeline=saved_pipeline)) + await db_accessor.update_task(1, Task(task_input={'number': None}, + pipeline=saved_pipeline, + status=TaskStateEnum.PENDING)) # Try modifying something we're not allowed to saved_task.task_input_id = None @@ -251,7 +261,8 @@ async def test_get_tasks(db_accessor): token_id=1, task=Task( task_input={'number': i + 1}, - pipeline=pipeline + pipeline=pipeline, + status=TaskStateEnum.PENDING ) ) diff --git a/tests/db_auth_test.py b/tests/db_auth_test.py index e155452..625d493 100644 --- a/tests/db_auth_test.py +++ b/tests/db_auth_test.py @@ -2,16 +2,16 @@ import datetime from sqlalchemy import select -from npg.porchdb.models import Token, Pipeline -from npg.porchdb.auth import Validator, CredentialsValidationException -import npg.porch.models.permission -import npg.porch.models.pipeline +from npg_porch.db.models import Token, Pipeline +from npg_porch.db.auth import Validator, CredentialsValidationException +import npg_porch.models.permission +import npg_porch.models.pipeline @pytest.mark.asyncio async def test_token_string_is_valid(async_minimum): v = Validator(session = async_minimum) - assert isinstance(v, (npg.porchdb.auth.Validator)) + assert isinstance(v, (npg_porch.db.auth.Validator)) with pytest.raises(CredentialsValidationException, match=r'The token should be 32 chars long'): @@ -76,15 +76,15 @@ async def test_permission_object_is_returned(async_minimum): for t in token_rows: if t.description == 'Seqfarm host, job runner': p = await v.token2permission(t.token) - assert isinstance(p, (npg.porch.models.permission.Permission)) + assert isinstance(p, (npg_porch.models.permission.Permission)) assert p.pipeline is not None - assert isinstance(p.pipeline, (npg.porch.models.pipeline.Pipeline)) + assert isinstance(p.pipeline, (npg_porch.models.pipeline.Pipeline)) assert p.pipeline.name == 'ptest one' assert p.requestor_id == t.token_id assert p.role == 'regular_user' elif t.description == 'Seqfarm host, admin': p = await v.token2permission(t.token) - assert isinstance(p, (npg.porch.models.permission.Permission)) + assert isinstance(p, (npg_porch.models.permission.Permission)) assert p.pipeline is None assert p.requestor_id == t.token_id assert p.role == 'power_user' diff --git a/tests/db_task_test.py b/tests/db_task_test.py index 6672ea8..e1c9ec5 100644 --- a/tests/db_task_test.py +++ b/tests/db_task_test.py @@ -1,7 +1,7 @@ import pytest from sqlalchemy import select -from npg.porchdb.models import Task +from npg_porch.db.models import Task @pytest.mark.asyncio async def test_task_creation(async_minimum): diff --git a/tests/db_token_test.py b/tests/db_token_test.py index a7edbc0..0197b04 100644 --- a/tests/db_token_test.py +++ b/tests/db_token_test.py @@ -1,7 +1,7 @@ import pytest from sqlalchemy import select -from npg.porchdb.models import Token +from npg_porch.db.models import Token @pytest.mark.asyncio async def test_token_creation(async_minimum): diff --git a/tests/fixtures/deploy_db.py b/tests/fixtures/deploy_db.py index 1a0d966..654c467 100644 --- a/tests/fixtures/deploy_db.py +++ b/tests/fixtures/deploy_db.py @@ -3,12 +3,12 @@ import pytest_asyncio from starlette.testclient import TestClient -from npg.porchdb.models import ( +from npg_porch.db.models import ( Pipeline, Task, Event, Token ) -from npg.porchdb.data_access import AsyncDbAccessor -from npg.porch.models import Task as ModelledTask, TaskStateEnum -from npg.main import app +from npg_porch.db.data_access import AsyncDbAccessor +from npg_porch.models import Task as ModelledTask, TaskStateEnum +from npg_porch.server import app @pytest.fixture def minimum_data(): @@ -46,7 +46,8 @@ def minimum_data(): definition={ 'to_do': 'stuff', 'why': 'reasons' - } + }, + state=TaskStateEnum.PENDING ), Task( pipeline=pipeline, @@ -56,7 +57,8 @@ def minimum_data(): definition={ 'to_do': 'more stuff', 'why': 'reasons' - } + }, + state=TaskStateEnum.PENDING ) ] diff --git a/tests/fixtures/orm_session.py b/tests/fixtures/orm_session.py index 7cdbb09..3d5215f 100644 --- a/tests/fixtures/orm_session.py +++ b/tests/fixtures/orm_session.py @@ -4,8 +4,8 @@ import sqlalchemy import sqlalchemy.orm -from npg.porchdb.models import Base -from npg.porchdb.connection import session_factory, deploy_schema, close_engine +from npg_porch.db.models import Base +from npg_porch.db.connection import session_factory, deploy_schema, close_engine @pytest.fixture diff --git a/tests/init_test.py b/tests/init_test.py index 0f67135..9853441 100644 --- a/tests/init_test.py +++ b/tests/init_test.py @@ -1,7 +1,7 @@ import pytest from sqlalchemy import select -from npg.porchdb.models import Pipeline +from npg_porch.db.models import Pipeline def test_fixture(sync_minimum): diff --git a/tests/model_permission_test.py b/tests/model_permission_test.py index 23d3716..80c42b4 100644 --- a/tests/model_permission_test.py +++ b/tests/model_permission_test.py @@ -1,7 +1,7 @@ import pytest -from npg.porch.models.pipeline import Pipeline -from npg.porch.models.permission import Permission, PermissionValidationException +from npg_porch.models.pipeline import Pipeline +from npg_porch.models.permission import Permission, PermissionValidationException from pydantic import ValidationError diff --git a/tests/pipeline_route_test.py b/tests/pipeline_route_test.py index bf0f5d8..19feab4 100644 --- a/tests/pipeline_route_test.py +++ b/tests/pipeline_route_test.py @@ -1,6 +1,8 @@ +import json + from starlette import status -from npg.porch.models import Pipeline +from npg_porch.models import Pipeline headers = { @@ -100,6 +102,16 @@ def test_get_known_pipeline(async_minimum, fastapi_testclient): def test_create_pipeline(async_minimum, fastapi_testclient): + invalid_pipeline = { + "name": "ptest one", + "url": "http://test.com", # URL, not URI + "version": "1" + } + response = fastapi_testclient.post( + "/pipelines", json=json.dumps(invalid_pipeline), follow_redirects=True, + headers=headers4power_user + ) + assert response.status_code == status.HTTP_422_UNPROCESSABLE_ENTITY # Create a pipeline desired_pipeline = Pipeline( @@ -152,3 +164,39 @@ def test_create_pipeline(async_minimum, fastapi_testclient): headers=headers4power_user ) assert response.status_code == status.HTTP_400_BAD_REQUEST + + +def test_create_pipeline_token(async_minimum, fastapi_testclient): + pipeline_name = "ptest four" + token_desc = "this is a token" + + # Create a pipeline + desired_pipeline = Pipeline( + name=pipeline_name, + uri="http://test.com", + version="1" + ) + + http_create_pipeline(fastapi_testclient, desired_pipeline) + + # Create new tokens for the pipeline. There is no limit on the number of tokens + # that can be created and no restriction on token description. + for _ in range(3): + response = fastapi_testclient.post( + f"/pipelines/{pipeline_name}/token/{token_desc}", + follow_redirects=True, + headers=headers4power_user + ) + + assert response.status_code == status.HTTP_201_CREATED + assert response.json()["name"] == pipeline_name + assert response.json()["description"] == token_desc + assert len(response.json()["token"]) == 32 + + # Create a new token for a non-existent pipeline + response = fastapi_testclient.post( + "/pipelines/not here/token/{token_desc}", + follow_redirects=True, + headers=headers4power_user + ) + assert response.status_code == status.HTTP_404_NOT_FOUND diff --git a/pytest.ini b/tests/pytest.ini similarity index 100% rename from pytest.ini rename to tests/pytest.ini diff --git a/tests/task_route_test.py b/tests/task_route_test.py index c24bd6a..8c5af1b 100644 --- a/tests/task_route_test.py +++ b/tests/task_route_test.py @@ -1,7 +1,6 @@ +from npg_porch.models import Pipeline, Task, TaskStateEnum from starlette import status -from npg.porch.models import Task, TaskStateEnum, Pipeline - # Not testing get-all-tasks as this method will ultimately go headers4ptest_one = { @@ -17,12 +16,13 @@ def test_task_creation(async_minimum, fastapi_testclient): # Create a task with a sparse pipeline definition task_one = Task( - pipeline = { + pipeline={ 'name': 'ptest one' }, - task_input = { + task_input={ 'number': 1 - } + }, + status=TaskStateEnum.PENDING, ) response = fastapi_testclient.post( @@ -32,24 +32,28 @@ def test_task_creation(async_minimum, fastapi_testclient): headers=headers4ptest_one ) assert response.status_code == status.HTTP_201_CREATED - assert task_one == response.json() + response_obj = response.json() + assert task_one == response_obj - # Try again and expect to fail + # Try again and expect to succeed with a different status code and the + # same task returned. response = fastapi_testclient.post( 'tasks', json=task_one.model_dump(), follow_redirects=True, headers=headers4ptest_one ) - assert response.status_code == status.HTTP_409_CONFLICT + assert response.status_code == status.HTTP_200_OK + assert response.json() == response_obj task_two = Task( - pipeline = { + pipeline={ 'name': 'ptest none' }, - task_input = { + task_input={ 'number': 1 - } + }, + status=TaskStateEnum.PENDING, ) # The token is valid, but for a different pipeline. It is impossible # to have a valid token for a pipeline that does not exist. @@ -65,9 +69,9 @@ def test_task_creation(async_minimum, fastapi_testclient): def test_task_update(async_minimum, fastapi_testclient): task = fastapi_testclient.get('/tasks', headers=headers4ptest_one).json()[0] - assert task['status'] is None + assert task['status'] == TaskStateEnum.PENDING.value - task['status'] = TaskStateEnum.PENDING + task['status'] = TaskStateEnum.RUNNING response = fastapi_testclient.put( '/tasks', json=task, @@ -204,7 +208,16 @@ def test_get_tasks(async_minimum, async_tasks, fastapi_testclient): ) assert response.status_code == status.HTTP_200_OK, 'Other optional argument works' tasks = response.json() - assert len(tasks) == 10, 'Ten pending tasks selected' + # async_minimum provides 2 tasks, async_tasks provides 10 + assert len(tasks) == 12, 'Twelve pending tasks selected' + + response = fastapi_testclient.get( + '/tasks?status=RUNNING', + headers=headers4ptest_one + ) + assert response.status_code == status.HTTP_200_OK, 'Other optional argument works' + tasks = response.json() + assert len(tasks) == 0, 'No running tasks selected' response = fastapi_testclient.get( '/tasks?pipeline_name="ptest one"&status=PENDING',