From 7a179107872e6684fe89fdec0a90a13ecad1e4c9 Mon Sep 17 00:00:00 2001 From: Oscar Moya Date: Thu, 6 Jun 2024 09:12:14 +0200 Subject: [PATCH] Add lint tools to the project Added two Makefile recipes in order to format the code and pass the linter and the type checker using autopep8 for formatting and mypy and flake for linting. This will ensure that all contributions to this project will follow the same rules and the code will be more traceble and easier to read and follow. As part of this PR there are some other minor fixes: * Removed `*` imports * Fixed some naming conventions * Fixed some Imports between SqlModel/SqlAlchemy objects * Added type returns to all the functions * Removed unused imports * Fix minor style issues Signed-off-by: Oscar Moya --- .flake8 | 18 ++++++++++ .github/workflows/ci.yaml | 50 ++++++++++++++++++++++++++- .mypy.ini | 4 +++ Makefile | 8 +++++ requirements-dev.txt | 3 ++ requirements.txt | 1 + src/constants.py | 26 +++++++------- src/data/database.py | 32 ++++++++++++----- src/models/turnilo_dashboard.py | 5 +-- src/routes/routes.py | 51 +++++++++++++++------------ src/services/turnilo_dashboards.py | 44 +++++++++++++++--------- test/client.py | 27 ++++++++++----- test/unit_test.py | 55 +++++++++++++++++------------- 13 files changed, 230 insertions(+), 94 deletions(-) create mode 100644 .flake8 create mode 100644 .mypy.ini create mode 100644 requirements-dev.txt diff --git a/.flake8 b/.flake8 new file mode 100644 index 0000000..1724aa7 --- /dev/null +++ b/.flake8 @@ -0,0 +1,18 @@ +[flake8] +max-line-length = 120 +exclude = .git, + __pycache__, + docs/source/conf.py, + old,build, + dist, + venv, + .venv, + .tox, + .eggs, + *.egg, + *.egg-info, + *.egg-info/* + __init__.py +ignore = E203, E266, W503 +max-complexity = 18 +select = B,C,E,F,W,T4,B9 \ No newline at end of file diff --git a/.github/workflows/ci.yaml b/.github/workflows/ci.yaml index 5373ef9..74c72cd 100644 --- a/.github/workflows/ci.yaml +++ b/.github/workflows/ci.yaml @@ -12,6 +12,54 @@ env: PLATFORMS: linux/amd64,linux/arm64 jobs: + lint: + runs-on: ubuntu-22.04 + steps: + - name: "Checkout backend(REST)" + uses: actions/checkout@v4 + with: + path: backend + fetch-depth: 0 + fetch-tags: 1 + + - name: "lint" + run: | + cd backend + + echo "Fix mess with tags in actions/checkout..." + git fetch -f && git fetch -f --tags + + sudo apt-get install -y make + pip3 install -r requirements.txt + pip3 install -r requirements-dev.txt + + #Run linter + make lint + + format: + runs-on: ubuntu-22.04 + steps: + - name: "Checkout backend(REST)" + uses: actions/checkout@v4 + with: + path: backend + fetch-depth: 0 + fetch-tags: 1 + + - name: "format" + run: | + cd backend + + echo "Fix mess with tags in actions/checkout..." + git fetch -f && git fetch -f --tags + + sudo apt-get install -y make + pip3 install -r requirements.txt + pip3 install -r requirements-dev.txt + + #Run formatter + AUTOPEP8_EXIT_CODE=--exit-code make format-code + unit-test: runs-on: ubuntu-22.04 steps: @@ -37,7 +85,7 @@ jobs: docker_build_and_publish: runs-on: ubuntu-22.04 - needs: [unit-test] + needs: [unit-test, format, lint] steps: - name: "Checkout backend(REST)" uses: actions/checkout@v4 diff --git a/.mypy.ini b/.mypy.ini new file mode 100644 index 0000000..24aca63 --- /dev/null +++ b/.mypy.ini @@ -0,0 +1,4 @@ +[mypy] +check_untyped_defs = True +disallow_untyped_calls = True +ignore_missing_imports = True diff --git a/Makefile b/Makefile index 8fbba85..f02b7e1 100644 --- a/Makefile +++ b/Makefile @@ -1,3 +1,6 @@ +AUTOPEP8_OPTS ?= --in-place --recursive --aggressive --aggressive +AUTOPEP8_EXIT_CODE ?= + all: start start: _build _run @@ -10,3 +13,8 @@ stop: @docker kill rest_backend || true @docker rm rest_backend || true reload: stop start +format-code: + autopep8 $(AUTOPEP8_OPTS) $(AUTOPEP8_EXIT_CODE) ./ +lint: + flake8 ./ + mypy ./ diff --git a/requirements-dev.txt b/requirements-dev.txt new file mode 100644 index 0000000..073a7c8 --- /dev/null +++ b/requirements-dev.txt @@ -0,0 +1,3 @@ +mypy +flake8 +autopep8 diff --git a/requirements.txt b/requirements.txt index 28ff299..bd19035 100644 --- a/requirements.txt +++ b/requirements.txt @@ -6,3 +6,4 @@ psycopg2==2.9.9 jinja2==3.1.3 pyyaml==6.0.1 pytest +types-requests diff --git a/src/constants.py b/src/constants.py index 0b54fef..38f3a22 100644 --- a/src/constants.py +++ b/src/constants.py @@ -1,17 +1,17 @@ -#DB +# DB -##Driver -DRIVER="sqlite" +# Driver +DRIVER = "sqlite" -##SQLite -SQLITE_FILE="./data/backend.db" +# SQLite +SQLITE_FILE = "./data/backend.db" -##Postgres -POSTGRES_HOST="127.0.0.1" -POSTGRES_PORT=5432 -POSTGRES_DB="stack" -POSTGRES_USERNAME="postgres" -POSTGRES_PASSWORD="test" +# Postgres +POSTGRES_HOST = "127.0.0.1" +POSTGRES_PORT = 5432 +POSTGRES_DB = "stack" +POSTGRES_USERNAME = "postgres" +POSTGRES_PASSWORD = "test" -#API -URL_PATH="/rest" +# API +URL_PATH = "/rest" diff --git a/src/data/database.py b/src/data/database.py index 04e26de..914780c 100644 --- a/src/data/database.py +++ b/src/data/database.py @@ -1,22 +1,36 @@ +from typing import Any, Generator from sqlmodel import SQLModel, create_engine, Session from sqlalchemy_utils import database_exists, create_database -from constants import * +import constants -def createTables(db_engine): + +def create_tables(db_engine) -> None: from models.turnilo_dashboard import TurniloDashboard - SQLModel.metadata.create_all(db_engine) + SQLModel.metadata.create_all(db_engine, tables=[TurniloDashboard.__table__]) + -if DRIVER == "sqlite": - engine = create_engine("sqlite:///"+SQLITE_FILE, connect_args={"check_same_thread": False}) +if constants.DRIVER == "sqlite": + engine = create_engine("sqlite:///" + constants.SQLITE_FILE, connect_args={"check_same_thread": False}) else: - engine = create_engine("postgresql://"+POSTGRES_USERNAME+":"+POSTGRES_PASSWORD+"@"+POSTGRES_HOST+":"+str(POSTGRES_PORT)+"/"+POSTGRES_DB) + engine = create_engine( + "postgresql://" + + constants.POSTGRES_USERNAME + + ":" + + constants.POSTGRES_PASSWORD + + "@" + + constants.POSTGRES_HOST + + ":" + + str(constants.POSTGRES_PORT) + + "/" + + constants.POSTGRES_DB) if not database_exists(engine.url): print("Creating DB...") - create_database(engine.url) - createTables(engine) + create_database(engine.url) # type: ignore + create_tables(engine) + -def get_session(): +def get_session() -> Generator[Session, Any, None]: db_session = Session(engine) try: yield db_session diff --git a/src/models/turnilo_dashboard.py b/src/models/turnilo_dashboard.py index 351cb50..3371301 100644 --- a/src/models/turnilo_dashboard.py +++ b/src/models/turnilo_dashboard.py @@ -1,8 +1,9 @@ from typing import Optional -from sqlmodel import Field, Session, SQLModel, UniqueConstraint +from sqlmodel import Field, SQLModel, UniqueConstraint + class TurniloDashboard(SQLModel, table=True): - __tablename__ = "turniloDashboards" + __tablename__ = "turniloDashboards" # type: ignore __table_args__ = (UniqueConstraint('dataCube', 'shortName', name='_dataCube_shortName'),) id: Optional[int] = Field(default=None, primary_key=True, index=True) diff --git a/src/routes/routes.py b/src/routes/routes.py index 6185b3f..31c5b30 100644 --- a/src/routes/routes.py +++ b/src/routes/routes.py @@ -1,70 +1,77 @@ -from fastapi import APIRouter, Depends, HTTPException, status, Path +from fastapi import APIRouter, Depends, HTTPException from typing import List from sqlalchemy.orm import Session from models.turnilo_dashboard import TurniloDashboard -from services.turnilo_dashboards import * +from services import turnilo_dashboards as td -from constants import * +import constants import data.database as db api_router = APIRouter() ### Dashboards ### -#GET +# GET + + @api_router.get( - URL_PATH+"/turnilo/dashboards/", + constants.URL_PATH + "/turnilo/dashboards/", response_model=List[TurniloDashboard], summary="Gets all Turnilo dashboards" ) def turnilo_get_dashboards(db_session: Session = Depends(db.get_session)): - return dashboards_get_all(db_session) + return td.dashboards_get_all(db_session) + @api_router.get( - URL_PATH+"/turnilo/dashboards/{id}", + constants.URL_PATH + "/turnilo/dashboards/{id}", response_model=TurniloDashboard, summary="Get a Turnilo dashboard by id (integer)" ) -def turnilo_get_dashboard_id(id:str, db_session: Session = Depends(db.get_session)): +def turnilo_get_dashboard_id(id: str, db_session: Session = Depends(db.get_session)): try: int_id = int(id) - except: + except BaseException: raise HTTPException(status_code=400, detail="Id is not an integer") - return dashboards_get_id(db_session, int_id) + return td.dashboards_get_id(db_session, int_id) + +# POST + -#POST @api_router.post( - URL_PATH+"/turnilo/dashboards/", + constants.URL_PATH + "/turnilo/dashboards/", response_model=TurniloDashboard, summary="Create a Turnilo dashboard. A unique id will be assigned." ) def turnilo_create_dashboard(dashboard: TurniloDashboard, db_session: Session = Depends(db.get_session)): - return dashboards_create(db_session, dashboard) + return td.dashboards_create(db_session, dashboard) -#PUT +# PUT @api_router.put( - URL_PATH+"/turnilo/dashboards/{id}", + constants.URL_PATH + "/turnilo/dashboards/{id}", response_model=TurniloDashboard, summary="Update/replace a Turnilo dashboard. The dashboard (id) must exist" ) -def turnilo_update_dashboard(id:str, dashboard: TurniloDashboard, db_session: Session = Depends(db.get_session)): +def turnilo_update_dashboard(id: str, dashboard: TurniloDashboard, db_session: Session = Depends(db.get_session)): try: int_id = int(id) dashboard.id = int_id - except: + except BaseException: raise HTTPException(status_code=400, detail="Id is not an integer") - return dashboards_update(db_session, dashboard) + return td.dashboards_update(db_session, dashboard) + +# DELETE + -#DELETE @api_router.delete( - URL_PATH+"/turnilo/dashboards/{id}", + constants.URL_PATH + "/turnilo/dashboards/{id}", response_model=TurniloDashboard, summary="Delete a Turnilo dashboard" ) def turnilo_delete_dashboard(id: str, db_session: Session = Depends(db.get_session)): try: int_id = int(id) - except: + except BaseException: raise HTTPException(status_code=400, detail="Id is not an integer") - return dashboards_delete(db_session, id) + return td.dashboards_delete(db_session, int_id) diff --git a/src/services/turnilo_dashboards.py b/src/services/turnilo_dashboards.py index 5607b6f..b754742 100644 --- a/src/services/turnilo_dashboards.py +++ b/src/services/turnilo_dashboards.py @@ -1,21 +1,30 @@ -from sqlmodel import Session +from typing import List +from sqlalchemy.orm import Session from sqlalchemy import exc from models.turnilo_dashboard import TurniloDashboard from fastapi import HTTPException -#Turnilo Dashboards -def dashboards_get_all(session: Session): +# Turnilo Dashboards + + +def dashboards_get_all(session: Session) -> List[TurniloDashboard]: return session.query(TurniloDashboard).all() -def _dashboards_return_single_obj(results): - if results == None or len(results) == 0: + + +def _dashboards_return_single_obj(results: List[TurniloDashboard]): + if results is None or len(results) == 0: raise HTTPException(status_code=404, detail="Item not found") elif len(results) > 1: raise HTTPException(status_code=500, detail="Corrupted state. Get query returned > 1 result") return results[0] -def dashboards_get_id(session: Session, _id: int): - results = session.query(TurniloDashboard).filter(TurniloDashboard.id == _id).all() + + +def dashboards_get_id(session: Session, _id: int) -> TurniloDashboard: + results: List[TurniloDashboard] = session.query(TurniloDashboard).filter_by(id=_id).all() return _dashboards_return_single_obj(results) -def dashboards_create(session: Session, dashboard: TurniloDashboard): + + +def dashboards_create(session: Session, dashboard: TurniloDashboard) -> TurniloDashboard: if dashboard.id: raise HTTPException(status_code=400, detail="'id' must NOT be set") if not dashboard.shortName or dashboard.shortName == "": @@ -26,16 +35,18 @@ def dashboards_create(session: Session, dashboard: TurniloDashboard): except exc.IntegrityError as e: print(str(e)) raise HTTPException(status_code=400, detail="Integrity error: duplicated datacube+shortName") - print("Created dashboard: "+str(dashboard.id)) + print("Created dashboard: " + str(dashboard.id)) print(dashboard) return dashboard -def dashboards_update(session: Session, dashboard: TurniloDashboard): + + +def dashboards_update(session: Session, dashboard: TurniloDashboard) -> TurniloDashboard: if not dashboard.id: raise HTTPException(status_code=400, detail="'id' MUST be set") if not dashboard.shortName or dashboard.shortName == "": raise HTTPException(status_code=400, detail="shortName not present or empty") - #First check if it exists + # First check if it exists dashboards_get_id(session, dashboard.id) try: session.merge(dashboard) @@ -43,17 +54,18 @@ def dashboards_update(session: Session, dashboard: TurniloDashboard): except exc.IntegrityError as e: print(str(e)) raise HTTPException(status_code=400, detail="Integrity error: duplicated datacube+shortName") - print("Updated dashboard:"+str(dashboard.id)) + print("Updated dashboard:" + str(dashboard.id)) print(dashboard) return dashboard -def dashboards_delete(session: Session, _id: int): - dashboard=None + +def dashboards_delete(session: Session, _id: int) -> TurniloDashboard: + dashboard = None try: dashboard = session.query(TurniloDashboard).filter_by(id=_id).one() - except: + except BaseException: pass - if dashboard == None: + if dashboard is None: raise HTTPException(status_code=404, detail="Item not found") session.delete(dashboard) session.commit() diff --git a/test/client.py b/test/client.py index 22e2539..af0b584 100755 --- a/test/client.py +++ b/test/client.py @@ -8,27 +8,31 @@ DEFAULT_PORT = 8080 DEFAULT_PATH = "rest/turnilo/dashboards" -def create_dashboard(host, port, json_string): + +def create_dashboard(host, port, json_string) -> requests.Response: url = f"http://{host}:{port}/{DEFAULT_PATH}" data = json.loads(json_string) response = requests.post(url, json=data) print_response(response) return response -def update_dashboard(host, port, json_string, dashboard_id): + +def update_dashboard(host, port, json_string, dashboard_id) -> requests.Response: url = f"http://{host}:{port}/{DEFAULT_PATH}/{dashboard_id}" data = json.loads(json_string) response = requests.put(url, json=data) print_response(response) return response -def delete_dashboard(host, port, dashboard_id): + +def delete_dashboard(host, port, dashboard_id) -> requests.Response: url = f"http://{host}:{port}/{DEFAULT_PATH}/{dashboard_id}" response = requests.delete(url) print_response(response) return response -def get_dashboard(host, port, dashboard_id=None): + +def get_dashboard(host, port, dashboard_id=None) -> requests.Response: if dashboard_id: url = f"http://{host}:{port}/{DEFAULT_PATH}/{dashboard_id}" else: @@ -37,17 +41,23 @@ def get_dashboard(host, port, dashboard_id=None): print_response(response) return response -def print_response(response): + +def print_response(response) -> None: print(f"Status Code: {response.status_code}") try: print(f"Response: {response.json()}") except ValueError: print(f"Response: {response.text}") -def main(): - parser = argparse.ArgumentParser(description="Interact with the REST API to create, update, delete, or get dashboards.") + +def main() -> None: + parser = argparse.ArgumentParser( + description="Interact with the REST API to create, update, delete, or get dashboards.") parser.add_argument('action', choices=['create', 'update', 'delete', 'get'], help="Action to perform") - parser.add_argument('json_file_path_or_id', nargs='?', help="Path to JSON file for create/update, ID for delete/get") + parser.add_argument( + 'json_file_path_or_id', + nargs='?', + help="Path to JSON file for create/update, ID for delete/get") parser.add_argument('dashboard_id', nargs='?', help="ID of the dashboard to update/delete/get") parser.add_argument('-H', '--host', default=DEFAULT_HOST, help="Host of the API (default: 127.0.0.1)") parser.add_argument('-p', '--port', type=int, default=DEFAULT_PORT, help="Port of the API (default: 8080)") @@ -77,5 +87,6 @@ def main(): else: sys.exit(1) + if __name__ == "__main__": main() diff --git a/test/unit_test.py b/test/unit_test.py index 01ac9f8..6a49ca8 100644 --- a/test/unit_test.py +++ b/test/unit_test.py @@ -1,21 +1,25 @@ -import threading, time, json, signal +import threading +import time +import json +import signal import pytest import uvicorn from fastapi import FastAPI -#REST API routes +# REST API routes from routes import routes -#Python Client (test it too) +# Python Client (test it too) from client import create_dashboard, get_dashboard, update_dashboard, delete_dashboard from typing import Generator, Any -HOST="127.0.0.1" -PORT=8080 +HOST = "127.0.0.1" +PORT = 8080 + @pytest.fixture(scope="module") -def sample_dashboard(): +def sample_dashboard() -> dict[str, str]: return { "dataCube": "networkFlows", "shortName": "simple_dashboard", @@ -23,6 +27,7 @@ def sample_dashboard(): "hash": "" } + @pytest.fixture(scope="session", autouse=True) def rest_server() -> Generator[None, Any, None]: app = FastAPI() @@ -36,24 +41,25 @@ def rest_server() -> Generator[None, Any, None]: server.handle_exit(signal.SIGINT, None) t.join() + @pytest.mark.parametrize("with_optional_fields", [True, False]) -def test_create_dashboard(sample_dashboard, with_optional_fields): +def test_create_dashboard(sample_dashboard: dict[str, Any], with_optional_fields: bool) -> None: dashboard = sample_dashboard.copy() if with_optional_fields: dashboard["preset"] = True dashboard["description"] = "This is a description" dashboard["id"] = 999 - #Setting the ID should fail + # Setting the ID should fail res = create_dashboard(HOST, PORT, json.dumps(dashboard)) assert res.status_code == 400 - #Create the dashboard + # Create the dashboard dashboard.pop("id", None) res = create_dashboard(HOST, PORT, json.dumps(dashboard)) got = res.json() - #Create should populate the id, and set {preset=False, description=""} + # Create should populate the id, and set {preset=False, description=""} dashboard["id"] = 1 if not with_optional_fields: dashboard["preset"] = False @@ -62,15 +68,16 @@ def test_create_dashboard(sample_dashboard, with_optional_fields): print(f"GOT:\n{got}") assert got == dashboard - #Cleanup + # Cleanup res = delete_dashboard(HOST, PORT, 1) assert res.status_code == 200 -def test_get_all_dashboards(sample_dashboard): + +def test_get_all_dashboards(sample_dashboard: dict[str, Any]) -> None: res = get_dashboard(HOST, PORT) assert res.json() == [] - #Create two dashboards + # Create two dashboards dashboard = sample_dashboard.copy() res = create_dashboard(HOST, PORT, json.dumps(dashboard)) assert res.json()["id"] == 1 @@ -81,7 +88,7 @@ def test_get_all_dashboards(sample_dashboard): res = create_dashboard(HOST, PORT, json.dumps(dashboard)) assert res.json()["id"] == 2 - #Get both + # Get both res = get_dashboard(HOST, PORT) dashs = res.json() assert len(dashs) == 2 @@ -92,16 +99,17 @@ def test_get_all_dashboards(sample_dashboard): elif dash["id"] == 2: assert dash["dataCube"] == "myDatacube" - #Cleanup + # Cleanup res = delete_dashboard(HOST, PORT, 1) assert res.status_code == 200 res = delete_dashboard(HOST, PORT, 2) assert res.status_code == 200 -def test_update_dashboard(sample_dashboard): + +def test_update_dashboard(sample_dashboard: dict[str, Any]) -> None: dashboard = sample_dashboard.copy() - #Create dashboard + # Create dashboard res = create_dashboard(HOST, PORT, json.dumps(dashboard)) got = res.json() dashboard["description"] = "" @@ -122,14 +130,15 @@ def test_update_dashboard(sample_dashboard): print(f"GOT:\n{got}") assert got == dashboard - #Cleanup + # Cleanup res = delete_dashboard(HOST, PORT, 1) assert res.status_code == 200 -def test_delete_dashboard(sample_dashboard): + +def test_delete_dashboard(sample_dashboard: dict[str, Any]) -> None: dashboard = sample_dashboard.copy() - #Create dashboard + # Create dashboard res = create_dashboard(HOST, PORT, json.dumps(dashboard)) got = res.json() dashboard["description"] = "" @@ -140,14 +149,14 @@ def test_delete_dashboard(sample_dashboard): print(f"GOT:\n{got}") assert got == dashboard - #Delete an inexistent Dashbaord + # Delete an inexistent Dashbaord res = delete_dashboard(HOST, PORT, 99) assert res.status_code == 404 - #Cleanup + # Cleanup res = delete_dashboard(HOST, PORT, 1) assert res.status_code == 200 - #100% dead + # 100% dead res = delete_dashboard(HOST, PORT, 1) assert res.status_code == 404