From 11f08911ae990fad226c3540d1b681222c5ef6be Mon Sep 17 00:00:00 2001 From: Roman Inflianskas Date: Wed, 30 Aug 2023 20:00:34 +0300 Subject: [PATCH] mypy: analyze types in all possible libraries Preparation for #646 * Remove `ignore_missing_imports = true` from mypy configuration. * Run mypy in the same environment instead of separate to check types in dependencies like fastapi. * Move mypy dependencies from pre-commit configuration to `setup.cfg`. * Update mypy dependencies there. * Move `rq` from `environment.yml` to `setup.cfg`: conda-forge version: 1.9.0, pypi version : 1.15.1 (two years difference; types were added). * Add libraries with missing types to ignore list in mypy configuration. * Add pydantic mypy plugin. * Allow running mypy without explicit paths. * Update GitHub Actions. * Temporarily add ignore `annotation-unchecked` to make mypy pass. * Fix new mypy issues: * Use https://github.com/hauntsaninja/no_implicit_optional to make `Optional` explicit. * If there is no default, first `pydantic.Field` argument should be omitted (`None` means that the default argument is `None`). * Refactor `_run_migrations`. There were two different paths: one for normal execution, another one for testing. Simplify arguments and the function code itself, introduce a new mock `run_migrations`. * Introduce `ChannelWithOptionalName` for `/channels` patch method to preserve compatibility. Note, that the solution is a bit dirty (I had to use `type: ignore[assignment]`) to minimize the number of models and the diff. * Trivial errors. --- .github/workflows/lint.yml | 3 +++ .pre-commit-config.yaml | 14 +------------- environment.yml | 1 - pyproject.toml | 21 ++++++++++++++++++++- quetz/cli.py | 24 +++++++++++------------- quetz/config.py | 4 ++-- quetz/dao.py | 4 ++-- quetz/hooks.py | 2 +- quetz/jobs/rest_models.py | 26 +++++++++++++------------- quetz/main.py | 18 +++++++++--------- quetz/metrics/view.py | 4 ++-- quetz/pkgstores.py | 2 +- quetz/rest_models.py | 24 +++++++++++++++--------- quetz/tasks/mirror.py | 10 +++++----- quetz/testing/fixtures.py | 16 +++++++++++++++- quetz/tests/test_cli.py | 33 +++++++++++++++++++++++---------- setup.cfg | 20 ++++++++++++++++++++ 17 files changed, 143 insertions(+), 83 deletions(-) diff --git a/.github/workflows/lint.yml b/.github/workflows/lint.yml index 3b12e16c..08334cb8 100644 --- a/.github/workflows/lint.yml +++ b/.github/workflows/lint.yml @@ -17,6 +17,9 @@ jobs: - name: Add micromamba to GITHUB_PATH run: echo "${HOME}/micromamba-bin" >> "$GITHUB_PATH" - run: ln -s "${CONDA_PREFIX}" .venv # Necessary for pyright. + - run: pip install -e .[mypy] + - name: Add mypy to GITHUB_PATH + run: echo "${GITHUB_WORKSPACE}/.venv/bin" >> "$GITHUB_PATH" - uses: pre-commit/action@v3.0.0 with: extra_args: --all-files --show-diff-on-failure diff --git a/.pre-commit-config.yaml b/.pre-commit-config.yaml index 1b2adb0e..7723ce43 100644 --- a/.pre-commit-config.yaml +++ b/.pre-commit-config.yaml @@ -19,19 +19,7 @@ repos: hooks: - id: mypy files: ^quetz/ - additional_dependencies: - - sqlalchemy-stubs - - types-click - - types-Jinja2 - - types-mock - - types-orjson - - types-pkg-resources - - types-redis - - types-requests - - types-six - - types-toml - - types-ujson - - types-aiofiles + language: system args: [--show-error-codes] - repo: https://github.com/Quantco/pre-commit-mirrors-prettier rev: 2.7.1 diff --git a/environment.yml b/environment.yml index 9c4dccfe..6fcb27fa 100644 --- a/environment.yml +++ b/environment.yml @@ -44,7 +44,6 @@ dependencies: - pre-commit - pytest - pytest-mock - - rq - libcflib - mamba - conda-content-trust diff --git a/pyproject.toml b/pyproject.toml index 95bd793c..5ed2f021 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -58,14 +58,33 @@ venv = ".venv" venvPath= "." [tool.mypy] -ignore_missing_imports = true +packages = [ + "quetz" +] plugins = [ + "pydantic.mypy", "sqlmypy" ] disable_error_code = [ + "annotation-unchecked", "misc" ] +[[tool.mypy.overrides]] +module = [ + "adlfs", + "authlib", + "authlib.*", + "fsspec", + "gcsfs", + "pamela", + "sqlalchemy_utils", + "sqlalchemy_utils.*", + "s3fs", + "xattr" +] +ignore_missing_imports = true + [tool.coverage.run] omit = [ "quetz/tests/*", diff --git a/quetz/cli.py b/quetz/cli.py index 5cc2a82c..a35a6d6f 100644 --- a/quetz/cli.py +++ b/quetz/cli.py @@ -87,20 +87,17 @@ def _alembic_config(db_url: str) -> AlembicConfig: def _run_migrations( - db_url: Optional[str] = None, - alembic_config: Optional[AlembicConfig] = None, + db_url: str, branch_name: str = "heads", ) -> None: - if db_url: - if db_url.startswith("postgre"): - db_engine = "PostgreSQL" - elif db_url.startswith("sqlite"): - db_engine = "SQLite" - else: - db_engine = db_url.split("/")[0] - logger.info('Running DB migrations on %s', db_engine) - if not alembic_config: - alembic_config = _alembic_config(db_url) + if db_url.startswith("postgre"): + db_engine = "PostgreSQL" + elif db_url.startswith("sqlite"): + db_engine = "SQLite" + else: + db_engine = db_url.split("/")[0] + logger.info('Running DB migrations on %s', db_engine) + alembic_config = _alembic_config(db_url) command.upgrade(alembic_config, branch_name) @@ -135,6 +132,7 @@ def _make_migrations( logger.info('Making DB migrations on %r for %r', db_url, plugin_name) if not alembic_config and db_url: alembic_config = _alembic_config(db_url) + assert alembic_config is not None # find path if plugin_name == "quetz": @@ -594,7 +592,7 @@ def start( uvicorn.run( "quetz.main:app", reload=reload, - reload_dirs=(quetz_src,), + reload_dirs=[quetz_src], port=port, proxy_headers=proxy_headers, host=host, diff --git a/quetz/config.py b/quetz/config.py index 82803877..3d718bc0 100644 --- a/quetz/config.py +++ b/quetz/config.py @@ -230,7 +230,7 @@ class Config: _instances: Dict[Optional[str], "Config"] = {} - def __new__(cls, deployment_config: str = None): + def __new__(cls, deployment_config: Optional[str] = None): if not deployment_config and None in cls._instances: return cls._instances[None] @@ -254,7 +254,7 @@ def __getattr__(self, name: str) -> Any: super().__getattr__(self, name) @classmethod - def find_file(cls, deployment_config: str = None): + def find_file(cls, deployment_config: Optional[str] = None): config_file_env = os.getenv(f"{_env_prefix}{_env_config_file}") deployment_config_files = [] for f in (deployment_config, config_file_env): diff --git a/quetz/dao.py b/quetz/dao.py index 3e61aa0b..01e67464 100644 --- a/quetz/dao.py +++ b/quetz/dao.py @@ -926,8 +926,8 @@ def create_version( def get_package_versions( self, package, - time_created_ge: datetime = None, - version_match_str: str = None, + time_created_ge: Optional[datetime] = None, + version_match_str: Optional[str] = None, skip: int = 0, limit: int = -1, ): diff --git a/quetz/hooks.py b/quetz/hooks.py index 40f9c7ea..9e615f22 100644 --- a/quetz/hooks.py +++ b/quetz/hooks.py @@ -11,7 +11,7 @@ @hookspec -def register_router() -> 'fastapi.APIRouter': +def register_router() -> 'fastapi.APIRouter': # type: ignore[empty-body] """add extra endpoints to the url tree. It should return an :py:class:`fastapi.APIRouter` with new endpoints definitions. diff --git a/quetz/jobs/rest_models.py b/quetz/jobs/rest_models.py index c518a587..ced6dc8e 100644 --- a/quetz/jobs/rest_models.py +++ b/quetz/jobs/rest_models.py @@ -83,7 +83,7 @@ def parse_job_name(v): class JobBase(BaseModel): """New job spec""" - manifest: str = Field(None, title='Name of the function') + manifest: str = Field(title='Name of the function') start_at: Optional[datetime] = Field( None, title="date and time the job should start, if None it starts immediately" @@ -110,35 +110,35 @@ def validate_job_name(cls, function_name): class JobCreate(JobBase): """Create job spec""" - items_spec: str = Field(..., title='Item selector spec') + items_spec: str = Field(title='Item selector spec') class JobUpdateModel(BaseModel): """Modify job spec items (status and items_spec)""" - items_spec: str = Field(None, title='Item selector spec') - status: JobStatus = Field(None, title='Change status') + items_spec: Optional[str] = Field(None, title='Item selector spec') + status: JobStatus = Field(title='Change status') force: bool = Field(False, title="force re-running job on all matching packages") class Job(JobBase): - id: int = Field(None, title='Unique id for job') - owner_id: uuid.UUID = Field(None, title='User id of the owner') + id: int = Field(title='Unique id for job') + owner_id: uuid.UUID = Field(title='User id of the owner') - created: datetime = Field(None, title='Created at') + created: datetime = Field(title='Created at') - status: JobStatus = Field(None, title='Status of the job (running, paused, ...)') + status: JobStatus = Field(title='Status of the job (running, paused, ...)') items_spec: Optional[str] = Field(None, title='Item selector spec') model_config = ConfigDict(from_attributes=True) class Task(BaseModel): - id: int = Field(None, title='Unique id for task') - job_id: int = Field(None, title='ID of the parent job') - package_version: dict = Field(None, title='Package version') - created: datetime = Field(None, title='Created at') - status: TaskStatus = Field(None, title='Status of the task (running, paused, ...)') + id: int = Field(title='Unique id for task') + job_id: int = Field(title='ID of the parent job') + package_version: dict = Field(title='Package version') + created: datetime = Field(title='Created at') + status: TaskStatus = Field(title='Status of the task (running, paused, ...)') @field_validator("package_version", mode="before") @classmethod diff --git a/quetz/main.py b/quetz/main.py index fe7d0c53..e348d114 100644 --- a/quetz/main.py +++ b/quetz/main.py @@ -326,7 +326,7 @@ def get_users_handler(dao, q, auth, skip, limit): @api_router.get("/users", response_model=List[rest_models.User], tags=["users"]) def get_users( dao: Dao = Depends(get_dao), - q: str = None, + q: Optional[str] = None, auth: authorization.Rules = Depends(get_rules), ): return get_users_handler(dao, q, auth, 0, -1) @@ -341,7 +341,7 @@ def get_paginated_users( dao: Dao = Depends(get_dao), skip: int = 0, limit: int = PAGINATION_LIMIT, - q: str = None, + q: Optional[str] = None, auth: authorization.Rules = Depends(get_rules), ): return get_users_handler(dao, q, auth, skip, limit) @@ -521,7 +521,7 @@ def set_user_role( def get_channels( public: bool = True, dao: Dao = Depends(get_dao), - q: str = None, + q: Optional[str] = None, auth: authorization.Rules = Depends(get_rules), ): """List all channels""" @@ -540,7 +540,7 @@ def get_paginated_channels( skip: int = 0, limit: int = PAGINATION_LIMIT, public: bool = True, - q: str = None, + q: Optional[str] = None, auth: authorization.Rules = Depends(get_rules), ): """List all channels, as a paginated response""" @@ -780,7 +780,7 @@ def post_channel( response_model=rest_models.ChannelBase, ) def patch_channel( - channel_data: rest_models.Channel, + channel_data: rest_models.ChannelWithOptionalName, dao: Dao = Depends(get_dao), auth: authorization.Rules = Depends(get_rules), channel: db_models.Channel = Depends(get_channel_or_fail), @@ -1054,8 +1054,8 @@ def post_package_member( def get_package_versions( package: db_models.Package = Depends(get_package_or_fail), dao: Dao = Depends(get_dao), - time_created__ge: datetime.datetime = None, - version_match_str: str = None, + time_created__ge: Optional[datetime.datetime] = None, + version_match_str: Optional[str] = None, ): version_profile_list = dao.get_package_versions( package, time_created__ge, version_match_str @@ -1079,8 +1079,8 @@ def get_paginated_package_versions( dao: Dao = Depends(get_dao), skip: int = 0, limit: int = PAGINATION_LIMIT, - time_created__ge: datetime.datetime = None, - version_match_str: str = None, + time_created__ge: Optional[datetime.datetime] = None, + version_match_str: Optional[str] = None, ): version_profile_list = dao.get_package_versions( package, time_created__ge, version_match_str, skip, limit diff --git a/quetz/metrics/view.py b/quetz/metrics/view.py index 9beebf20..cb9b3f43 100644 --- a/quetz/metrics/view.py +++ b/quetz/metrics/view.py @@ -1,5 +1,6 @@ import os +from fastapi import FastAPI from prometheus_client import ( CONTENT_TYPE_LATEST, REGISTRY, @@ -9,7 +10,6 @@ from prometheus_client.multiprocess import MultiProcessCollector from starlette.requests import Request from starlette.responses import Response -from starlette.types import ASGIApp from .middleware import PrometheusMiddleware @@ -24,6 +24,6 @@ def metrics(request: Request) -> Response: return Response(generate_latest(registry), media_type=CONTENT_TYPE_LATEST) -def init(app: ASGIApp): +def init(app: FastAPI): app.add_middleware(PrometheusMiddleware) app.add_route("/metricsp", metrics) diff --git a/quetz/pkgstores.py b/quetz/pkgstores.py index 5ce49497..141e4932 100644 --- a/quetz/pkgstores.py +++ b/quetz/pkgstores.py @@ -116,7 +116,7 @@ def file_exists(self, channel: str, destination: str): def get_filemetadata(self, channel: str, src: str) -> Tuple[int, int, str]: """get file metadata: returns (file size, last modified time, etag)""" - @abc.abstractclassmethod + @abc.abstractmethod def cleanup_temp_files(self, channel: str, dry_run: bool = False): """clean up temporary `*.json{HASH}.[bz2|gz]` files from pkgstore""" diff --git a/quetz/rest_models.py b/quetz/rest_models.py index ac6d8e0c..a393dcfb 100644 --- a/quetz/rest_models.py +++ b/quetz/rest_models.py @@ -37,7 +37,7 @@ class User(BaseUser): Profile.model_rebuild() -Role = Field(None, pattern='owner|maintainer|member') +Role = Field(pattern='owner|maintainer|member') class Member(BaseModel): @@ -58,7 +58,7 @@ class MirrorMode(str, Enum): class ChannelBase(BaseModel): - name: str = Field(None, title='The name of the channel', max_length=50) + name: str = Field(title='The name of the channel', max_length=50) description: Optional[str] = Field( None, title='The description of the channel', max_length=300 ) @@ -134,7 +134,7 @@ class ChannelMetadata(BaseModel): class Channel(ChannelBase): metadata: ChannelMetadata = Field( - default_factory=ChannelMetadata, title="channel metadata", examples={} + default_factory=ChannelMetadata, title="channel metadata", examples=[] ) actions: Optional[List[ChannelActionEnum]] = Field( @@ -160,8 +160,14 @@ def check_mirror_params(self) -> "Channel": return self +class ChannelWithOptionalName(Channel): + name: Optional[str] = Field( # type: ignore[assignment] + None, title='The name of the channel', max_length=50 + ) + + class ChannelMirrorBase(BaseModel): - url: str = Field(None, pattern="^(http|https)://.+") + url: str = Field(pattern="^(http|https)://.+") api_endpoint: Optional[str] = Field(None, pattern="^(http|https)://.+") metrics_endpoint: Optional[str] = Field(None, pattern="^(http|https)://.+") model_config = ConfigDict(from_attributes=True) @@ -173,7 +179,7 @@ class ChannelMirror(ChannelMirrorBase): class Package(BaseModel): name: str = Field( - None, title='The name of package', max_length=1500, pattern=r'^[a-z0-9-_\.]*$' + title='The name of package', max_length=1500, pattern=r'^[a-z0-9-_\.]*$' ) summary: Optional[str] = Field(None, title='The summary of the package') description: Optional[str] = Field(None, title='The description of the package') @@ -201,18 +207,18 @@ class PackageRole(BaseModel): class PackageSearch(Package): - channel_name: str = Field(None, title='The channel this package belongs to') + channel_name: str = Field(title='The channel this package belongs to') class ChannelSearch(BaseModel): - name: str = Field(None, title='The name of the channel', max_length=1500) + name: str = Field(title='The name of the channel', max_length=1500) description: Optional[str] = Field(None, title='The description of the channel') - private: bool = Field(None, title='The visibility of the channel') + private: bool = Field(title='The visibility of the channel') model_config = ConfigDict(from_attributes=True) class PaginatedResponse(BaseModel, Generic[T]): - pagination: Pagination = Field(None, title="Pagination object") + pagination: Pagination = Field(title="Pagination object") result: List[T] = Field([], title="Result objects") diff --git a/quetz/tasks/mirror.py b/quetz/tasks/mirror.py index cc20f749..bd3eb8bf 100644 --- a/quetz/tasks/mirror.py +++ b/quetz/tasks/mirror.py @@ -6,7 +6,7 @@ from concurrent.futures import ThreadPoolExecutor from http.client import IncompleteRead from tempfile import SpooledTemporaryFile -from typing import List +from typing import List, Optional import requests from fastapi import HTTPException, status @@ -285,8 +285,8 @@ def initial_sync_mirror( dao: Dao, pkgstore: PackageStore, auth: authorization.Rules, - includelist: List[str] = None, - excludelist: List[str] = None, + includelist: Optional[List[str]] = None, + excludelist: Optional[List[str]] = None, skip_errors: bool = True, use_repodata: bool = False, ): @@ -528,8 +528,8 @@ def synchronize_packages( pkgstore: PackageStore, auth: authorization.Rules, session: requests.Session, - includelist: List[str] = None, - excludelist: List[str] = None, + includelist: Optional[List[str]] = None, + excludelist: Optional[List[str]] = None, use_repodata: bool = False, ): logger.info(f"executing synchronize_packages task in a process {os.getpid()}") diff --git a/quetz/testing/fixtures.py b/quetz/testing/fixtures.py index 25a964c9..6d97a779 100644 --- a/quetz/testing/fixtures.py +++ b/quetz/testing/fixtures.py @@ -1,13 +1,17 @@ import os import shutil import tempfile -from typing import List +from functools import partial +from typing import Callable, List import pytest from alembic.command import upgrade as alembic_upgrade +from alembic.config import Config as AlembicConfig from fastapi.testclient import TestClient +from pytest_mock import MockerFixture import quetz +from quetz import cli from quetz.cli import _alembic_config from quetz.config import Config from quetz.dao import Dao @@ -81,6 +85,16 @@ def use_migrations() -> bool: ) +@pytest.fixture +def run_migrations( + alembic_config: AlembicConfig, + database_url: str, + mocker: MockerFixture, +) -> Callable[[], None]: + mocker.patch("quetz.cli._alembic_config", return_value=alembic_config) + return partial(cli._run_migrations, database_url) + + @pytest.fixture def sql_connection(engine): connection = engine.connect() diff --git a/quetz/tests/test_cli.py b/quetz/tests/test_cli.py index 8940cca2..6f5212dc 100644 --- a/quetz/tests/test_cli.py +++ b/quetz/tests/test_cli.py @@ -4,11 +4,13 @@ import tempfile from multiprocessing import Process from pathlib import Path +from typing import Callable from unittest import mock from unittest.mock import MagicMock import pytest import sqlalchemy as sa +from alembic.config import Config as AlembicConfig from alembic.script import ScriptDirectory from pytest_mock.plugin import MockerFixture from typer.testing import CliRunner @@ -148,13 +150,13 @@ def refresh_db(engine, database_url): def test_run_migrations( - config, sql_connection, engine, database_url, alembic_config, refresh_db + config, sql_connection, engine, refresh_db, run_migrations: Callable[[], None] ): db = sql_connection with pytest.raises(sa.exc.DatabaseError): db.execute(sa.text("SELECT * FROM users")) - cli._run_migrations(alembic_config=alembic_config) + run_migrations() db.execute(sa.text("SELECT * FROM users")) @@ -240,10 +242,15 @@ def test_make_migrations_plugin(mocker, config, config_dir, dummy_migration_plug def test_make_migrations_plugin_with_alembic( - config, config_dir, dummy_migration_plugin: Path, alembic_config, engine + config, + config_dir, + dummy_migration_plugin: Path, + engine, + run_migrations: Callable[[], None], + alembic_config: AlembicConfig, ): # make sure db is up-to-date - cli._run_migrations(alembic_config=alembic_config) + run_migrations() alembic_config.set_main_option( "version_locations", @@ -265,7 +272,7 @@ def test_make_migrations_plugin_with_alembic( assert migration_scripts # apply migrations - cli._run_migrations(alembic_config=alembic_config) + run_migrations() # add a new revision @@ -342,7 +349,13 @@ def downgrade(): pass def test_multi_head( - config, config_dir, dummy_migration_plugin: Path, alembic_config, engine, refresh_db + config, + config_dir, + dummy_migration_plugin: Path, + alembic_config, + engine, + refresh_db, + run_migrations: Callable[[], None], ): quetz_migrations_path = Path(config_dir) / "migrations" quetz_versions_path = quetz_migrations_path / "versions" @@ -365,7 +378,7 @@ def test_multi_head( " ".join(map(str, [plugin_versions_path, quetz_versions_path])), ) alembic_config.set_main_option("script_location", str(quetz_migrations_path)) - cli._run_migrations(alembic_config=alembic_config) + run_migrations() # initialize a plugin cli._make_migrations( @@ -375,7 +388,7 @@ def test_multi_head( initialize=True, alembic_config=alembic_config, ) - cli._run_migrations(alembic_config=alembic_config) + run_migrations() rev_file = next((plugin_versions_path).glob("*test_revision.py")) with open(rev_file) as fid: @@ -394,7 +407,7 @@ def test_multi_head( message="test revision", alembic_config=alembic_config, ) - cli._run_migrations(alembic_config=alembic_config) + run_migrations() rev_file = next((quetz_versions_path).glob("*test_revision.py")) with open(rev_file) as fid: @@ -414,7 +427,7 @@ def test_multi_head( content = fid.read() assert f"down_revision = '{plugin_rev_1}'" in content - cli._run_migrations(alembic_config=alembic_config) + run_migrations() # check heads diff --git a/setup.cfg b/setup.cfg index f5b2737d..670a4fd3 100644 --- a/setup.cfg +++ b/setup.cfg @@ -37,6 +37,7 @@ install_requires = pydantic>=2.0.0 pyyaml requests + rq sqlalchemy sqlalchemy-utils tenacity @@ -82,5 +83,24 @@ dev = pytest-cov pytest-timeout tbump + %(mypy)s +mypy = + mypy + sqlalchemy-stubs + types-Jinja2 + types-aiofiles + types-appdirs + types-click + types-mock + types-orjson + types-pkg-resources + types-pluggy + types-psycopg2 + types-redis + types-requests + types-six + types-toml + types-ujson + types-urllib3 test = %(dev)s