Fix all typing issues, re-adding Mypy to pre-commit checks (#1948)

.gitignore

@@ -2,6 +2,8 @@
 *.egg-info
 *.pyc
 .coverage
+coverage/
+coverage.xml
 .eslintcache
 .mypy_cache
 .pytest_cache
@@ -46,7 +48,6 @@ tests/cypress/screenshots
 tests/cypress/videos
 tests/screenshots
 tests/unit/utils/markdown/data/output.html
-coverage/
 
 # Instance files that should not be checked-in
 instance/development.py

.pre-commit-config.yaml

@@ -12,6 +12,7 @@ ci:
       'no-commit-to-branch',
       # 'hadolint-docker',
       'docker-compose-check',
+      'mypy', # Runs as a local hook now
     ]
 repos:
   - repo: https://github.com/pre-commit-ci/pre-commit-ci-config
@@ -51,7 +52,7 @@ repos:
       - id: pyupgrade
         args: ['--keep-runtime-typing', '--py311-plus']
   - repo: https://github.com/astral-sh/ruff-pre-commit
-    rev: v0.1.9
+    rev: v0.1.11
     hooks:
       - id: ruff
         args: ['--fix', '--exit-non-zero-on-fix']
@@ -100,28 +101,8 @@ repos:
     rev: 23.12.1
     hooks:
       - id: black
-  # Mypy is temporarily disabled until the SQLAlchemy 2.0 migration is complete
-  # - repo: https://github.com/pre-commit/mirrors-mypy
-  #   rev: v1.3.0
-  #   hooks:
-  #     - id: mypy
-  #       # warn-unused-ignores is unsafe with pre-commit, see
-  #       # https://github.com/python/mypy/issues/2960
-  #       args: ['--no-warn-unused-ignores', '--ignore-missing-imports']
-  #       additional_dependencies:
-  #         - flask
-  #         - lxml-stubs
-  #         - sqlalchemy
-  #         - toml
-  #         - tomli
-  #         - types-chevron
-  #         - types-geoip2
-  #         - types-python-dateutil
-  #         - types-pytz
-  #         - types-requests
-  #         - typing-extensions
   - repo: https://github.com/PyCQA/flake8
-    rev: 6.1.0
+    rev: 7.0.0
     hooks:
       - id: flake8
         additional_dependencies: *flake8deps
@@ -149,23 +130,12 @@ repos:
     rev: v3.0.0
     hooks:
       - id: creosote
+  - repo: https://github.com/shellcheck-py/shellcheck-py
+    rev: v0.9.0.6
+    hooks:
+      - id: shellcheck
         args:
-          - --venv=.venv
-          - --path=funnel
-          - --path=tests
-          - --path=migrations/versions
-          - --deps-file=requirements/base.in
-          - --exclude-dep=argon2-cffi # Optional dep for passlib
-          - --exclude-dep=bcrypt # Optional dep for passlib
-          - --exclude-dep=greenlet # Optional dep for SQLAlchemy's asyncio support
-          - --exclude-dep=gunicorn # Not imported, used as server
-          - --exclude-dep=linkify-it-py # Optional dep for markdown-it-py
-          - --exclude-dep=psycopg # Optional dep for SQLAlchemy
-          - --exclude-dep=rq-dashboard # Creosote fails to recognise the import
-          - --exclude-dep=tzdata # Data-only dep, therefore no import statement
-          - --exclude-dep=urllib3 # Required to silence a pip-audit warning
-          - --exclude-dep=wtforms-sqlalchemy # Temp dep on an unreleased git branch
-
+          - --external-sources
   - repo: https://github.com/pre-commit/pre-commit-hooks
     rev: v4.5.0
     hooks:
@@ -231,3 +201,16 @@ repos:
     rev: v3.0.1
     hooks:
       - id: docker-compose-check
+  - repo: local
+    hooks:
+      - id: mypy
+        name: mypy
+        entry: .venv/bin/mypy
+        args:
+          - --no-warn-unused-ignores
+          - --no-warn-redundant-casts
+          - . # Required to honour settings in pyproject.toml
+        language: system
+        types_or: [python, pyi]
+        require_serial: true
+        pass_filenames: false # Mypy is unreliable if module import order varies

.testenv

@@ -21,7 +21,7 @@ FLASK_LOG_TELEGRAM_APIKEY=null
 FLASK_LOG_SLACK_WEBHOOKS='[]'
 # Run RQ jobs inline in tests
 FLASK_RQ_ASYNC=false
-# Recaptcha keys from https://developers.google.com/recaptcha/docfaq#id-like-to-run-automated-tests-with-recaptcha.-what-should-i-do
+# Recaptcha keys from https://developers.google.com/recaptcha/docs/faq#id-like-to-run-automated-tests-with-recaptcha.-what-should-i-do
 FLASK_RECAPTCHA_USE_SSL=true
 FLASK_RECAPTCHA_PUBLIC_KEY=6LeIxAcTAAAAAJcZVRqyHh71UMIEGNQ_MXjiZKhI
 FLASK_RECAPTCHA_PRIVATE_KEY=6LeIxAcTAAAAAGG-vFI1TnRWxMZNFuojJ4WifJWe

docker/entrypoints/ci-test.sh

@@ -1,5 +1,5 @@
 #!/bin/sh
 
 make install-test
-pytest --allow-hosts=127.0.0.1,::1,$(hostname -i),$(getent ahosts db-test | awk '/STREAM/ { print $1}'),$(getent ahosts redis-test | awk '/STREAM/ { print $1}') --gherkin-terminal-reporter -vv --showlocals --cov=funnel
+pytest "--allow-hosts=127.0.0.1,::1,$(hostname -i),$(getent ahosts db-test | awk '/STREAM/ { print $1}'),$(getent ahosts redis-test | awk '/STREAM/ { print $1}')" --gherkin-terminal-reporter -vv --showlocals --cov=funnel
 coverage lcov -o coverage/funnel.lcov

docker/entrypoints/dev.sh

@@ -1,6 +1,6 @@
 #!/bin/bash
 
-if [ "$(psql -XtA -U postgres -h $DB_HOST funnel -c "select count(*) from information_schema.tables where table_schema = 'public';")" = "0" ]; then
+if [ "$(psql -XtA -U postgres -h "$DB_HOST" funnel -c "select count(*) from information_schema.tables where table_schema = 'public';")" = "0" ]; then
     flask dbcreate
     flask db stamp
 fi

funnel/__init__.py

@@ -89,7 +89,7 @@
 # present, it is loaded in debug and testing modes only
 for each_app in all_apps:
     coaster.app.init_app(
-        each_app, ['py', 'env'], env_prefix=['FLASK', f'APP_{each_app.name.upper()}']
+        each_app, ['env'], env_prefix=['FLASK', f'APP_{each_app.name.upper()}']
     )
 
 # Force specific config settings, overriding deployment config
@@ -147,7 +147,7 @@
 baseframe.init_app(
     app,
     requires=['funnel'],
-    theme='funnel',  # type: ignore[arg-type]
+    theme='funnel',  # type: ignore[arg-type]  # FIXME
     error_handlers=False,
 )
 
@@ -162,7 +162,7 @@
 transports.init()
 
 # Register JS and CSS assets on both apps
-app.assets.register(  # type: ignore[attr-defined]
+app.assets.register(  # type: ignore[attr-defined]  # FIXME
     'js_fullcalendar',
     Bundle(
         assets.require(
@@ -178,15 +178,15 @@
         filters='rjsmin',
     ),
 )
-app.assets.register(  # type: ignore[attr-defined]
+app.assets.register(  # type: ignore[attr-defined]  # FIXME
     'css_fullcalendar',
     Bundle(
         assets.require('jquery.fullcalendar.css', 'spectrum.css'),
         output='css/fullcalendar.packed.css',
         filters='cssmin',
     ),
 )
-app.assets.register(  # type: ignore[attr-defined]
+app.assets.register(  # type: ignore[attr-defined]  # FIXME
     'js_schedules',
     Bundle(
         assets.require('schedules.js'),
@@ -199,12 +199,9 @@
 
 # --- Serve static files with WhiteNoise -----------------------------------------------
 
-app.wsgi_app = WhiteNoise(  # type: ignore[method-assign]
-    app.wsgi_app, root=app.static_folder, prefix=app.static_url_path
-)
-app.wsgi_app.add_files(  # type: ignore[attr-defined]
-    baseframe.static_folder, prefix=baseframe.static_url_path
-)
+_wn = WhiteNoise(app.wsgi_app, root=app.static_folder, prefix=app.static_url_path)
+_wn.add_files(baseframe.static_folder, prefix=baseframe.static_url_path)
+app.wsgi_app = _wn  # type: ignore[method-assign]
 
 # --- Init SQLAlchemy mappers ----------------------------------------------------------
 

funnel/auth.py

@@ -0,0 +1,41 @@
+"""Auth proxy."""
+
+from coaster.auth import (
+    CurrentAuth as CurrentAuthBase,
+    GetCurrentAuth,
+    add_auth_anchor,
+    add_auth_attribute,
+    request_has_auth,
+)
+
+from . import all_apps
+from .models import User
+
+__all__ = [
+    'CurrentAuth',
+    'add_auth_attribute',
+    'add_auth_anchor',
+    'current_auth',
+    'request_has_auth',
+]
+
+
+class CurrentAuth(CurrentAuthBase):
+    """CurrentAuth for Funnel."""
+
+    # These attrs are typed as not-optional because they're typically accessed in a view
+    # that is already gated with the `@requires_login` or related decorator, so they're
+    # guaranteed to be present within the view. However, this will require a type-ignore
+    # for any code that tests `if current_auth.actor`, so those will need a rewrite to
+    # `if current_auth`. When auth clients become supported actors, this may need some
+    # form of PEP 647 typeguard to identify the actor's exact type.
+
+    user: User
+    actor: User
+
+
+current_auth = GetCurrentAuth.proxy(CurrentAuth)
+
+# Install this proxy in all apps, overriding the proxy provided by coaster.app.init_app
+for _app in all_apps:
+    _app.jinja_env.globals['current_auth'] = current_auth

funnel/cli/geodata.py

@@ -167,7 +167,7 @@ def load_country_info(filename: str) -> None:
         GeoCountryInfo.query.all()  # Load everything into session cache
         for item in countryinfo:
             if item.geonameid:
-                ci = GeoCountryInfo.query.get(int(item.geonameid))
+                ci = db.session.get(GeoCountryInfo, int(item.geonameid))
                 if ci is None:
                     ci = GeoCountryInfo(geonameid=int(item.geonameid))
                     db.session.add(ci)
@@ -276,7 +276,7 @@ def load_geonames(filename: str) -> None:
 
     for item in rich.progress.track(geonames):
         if item.geonameid:
-            gn = GeoName.query.get(int(item.geonameid))
+            gn = db.session.get(GeoName, int(item.geonameid))
             if gn is None:
                 gn = GeoName(geonameid=int(item.geonameid))
                 db.session.add(gn)
@@ -335,7 +335,7 @@ def load_alt_names(filename: str) -> None:
 
     for item in rich.progress.track(altnames):
         if item.geonameid:
-            rec = GeoAltName.query.get(int(item.id))
+            rec = db.session.get(GeoAltName, int(item.id))
             if rec is None:
                 rec = GeoAltName(id=int(item.id))
                 db.session.add(rec)
@@ -368,7 +368,7 @@ def load_admin1_codes(filename: str) -> None:
     GeoAdmin1Code.query.all()  # Load all data into session cache for faster lookup
     for item in rich.progress.track(admincodes):
         if item.geonameid:
-            rec = GeoAdmin1Code.query.get(item.geonameid)
+            rec = db.session.get(GeoAdmin1Code, item.geonameid)
             if rec is None:
                 rec = GeoAdmin1Code(geonameid=int(item.geonameid))
                 db.session.add(rec)
@@ -397,7 +397,7 @@ def load_admin2_codes(filename: str) -> None:
     GeoAdmin2Code.query.all()  # Load all data into session cache for faster lookup
     for item in rich.progress.track(admincodes):
         if item.geonameid:
-            rec = GeoAdmin2Code.query.get(item.geonameid)
+            rec = db.session.get(GeoAdmin2Code, item.geonameid)
             if rec is None:
                 rec = GeoAdmin2Code(geonameid=int(item.geonameid))
                 db.session.add(rec)

funnel/cli/refresh/markdown.py

@@ -9,16 +9,19 @@
 import rich.progress
 
 from ... import models
-from ...models import MarkdownModelUnion, db, sa_orm
+from ...models import db, sa_orm
 from . import refresh
 
-_M = TypeVar('_M', bound=MarkdownModelUnion)
+_M = TypeVar('_M', bound=models.ModelIdProtocol)
 
 
 class MarkdownModel(Generic[_M]):
     """Holding class for a model that has markdown fields with custom configuration."""
 
+    #: Dict of ``{MarkdownModel().name: MarkdownModel()}``
     registry: ClassVar[dict[str, MarkdownModel]] = {}
+    #: Dict of ``{config_name: MarkdownModel()}``, where the fields on the model using
+    #: that config are enumerated in :attr:`config_fields`
     config_registry: ClassVar[dict[str, set[MarkdownModel]]] = {}
 
     def __init__(self, model: type[_M], fields: set[str]) -> None:
@@ -54,12 +57,12 @@ def reparse(self, config: str | None = None, obj: _M | None = None) -> None:
             iter_total = 1
         else:
             load_columns = (
-                [self.model.id]
+                [self.model.id_]
                 + [getattr(self.model, f'{field}_text'.lstrip('_')) for field in fields]
                 + [getattr(self.model, f'{field}_html'.lstrip('_')) for field in fields]
             )
             iter_list = (
-                self.model.query.order_by(self.model.id)
+                self.model.query.order_by(self.model.id_)
                 .options(sa_orm.load_only(*load_columns))
                 .yield_per(10)
             )

funnel/devtest.py

@@ -18,7 +18,7 @@
 
 from flask import Flask
 
-from . import app as main_app, shortlinkapp, transports, unsubscribeapp
+from . import all_apps, app as main_app, transports
 from .models import db
 from .typing import ReturnView
 
@@ -100,7 +100,7 @@ def __call__(self, environ: Any, start_response: Any) -> Iterable[bytes]:
         return use_app(environ, start_response)
 
 
-devtest_app = AppByHostWsgi(main_app, shortlinkapp, unsubscribeapp)
+devtest_app = AppByHostWsgi(*all_apps)
 
 # --- Background worker ----------------------------------------------------------------
 
@@ -143,7 +143,7 @@ def _signature_without_annotations(func) -> inspect.Signature:
     )
 
 
-def install_mock(func: Callable, mock: Callable) -> None:
+def install_mock(func: Any, mock: Any) -> None:
     """
     Patch all existing references to :attr:`func` with :attr:`mock`.
 
@@ -161,9 +161,9 @@ def install_mock(func: Callable, mock: Callable) -> None:
     # Use weakref to dereference func from local namespace
     func = weakref.ref(func)
     gc.collect()
-    refs = gc.get_referrers(func())  # type: ignore[misc]  # Typeshed says not callable
+    refs = gc.get_referrers(func())
     # Recover func from the weakref so we can do an `is` match in referrers
-    func = func()  # type: ignore[misc]
+    func = func()
     for ref in refs:
         if isinstance(ref, dict):
             # We have a namespace dict. Iterate through contents to find the reference
@@ -312,7 +312,7 @@ def start(self) -> None:
                 raise RuntimeError(f"Server exited with code {self._process.exitcode}")
 
     def _is_ready(self) -> bool:
-        """Probe for readyness with a socket connection."""
+        """Probe for readiness with a socket connection."""
         if not self.probe_at:
             return False
         sock = socket.socket(socket.AF_INET, socket.SOCK_STREAM)