Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions .env.example
Original file line number Diff line number Diff line change
Expand Up @@ -7,3 +7,7 @@ CLEAN_DB_PASSWORD=mmli
CLEAN_DB_HOST=host.docker.internal
CLEAN_DB_PORT=5432
CLEAN_DB_NAME=mmlidb

# Matomo usage tracking (optional; disabled unless MATOMO_SITE_ID is set)
# MATOMO_SITE_ID=
# MATOMO_TOKEN=
16 changes: 14 additions & 2 deletions app/core/config.py
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
from typing import List
from typing import List, Optional

from pydantic import ConfigDict
from pydantic import ConfigDict, field_validator
from pydantic_settings import BaseSettings


Expand All @@ -16,6 +16,18 @@ class Settings(BaseSettings):
# CORS configuration
CORS_ORIGINS: List[str] = ["*"]

# Matomo usage tracking; disabled unless MATOMO_SITE_ID is set
MATOMO_URL: str = "https://matomo.mmli2.ncsa.illinois.edu"
MATOMO_SITE_ID: Optional[int] = None

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

An empty value stops the app from starting. Reproduced with pydantic-settings 2.9.1: MATOMO_SITE_ID='' raises ValidationError: Input should be a valid integer ... input_value=''. .env.example ships the line as # MATOMO_SITE_ID=, so uncommenting it as written (or setting an empty value in a deployment) prevents the pod from booting. That's the opposite of off by default. env_ignore_empty=True in model_config would treat it as unset.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in 216400a, but not with env_ignore_empty=True. I tried it with pydantic-settings 2.9.1, and it turns every empty setting into a missing one. CLEAN_DB_PASSWORD is required, and the chart sets it to '' in local-dev mode, so that setup would then fail with "Field required". Instead, a mode="before" validator turns "" into None for MATOMO_SITE_ID and MATOMO_TOKEN only. test_empty_settings_mean_unset covers it, and it fails if the validator is removed.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You're right, and my suggestion was wrong. I checked it with pydantic-settings 2.9.1: env_ignore_empty=True turns an empty CLEAN_DB_PASSWORD into "Field required", and the chart's plaintext mode does render it empty. Scoping the validator to the two Matomo settings is the better fix. I also confirmed that removing it makes the suite error out before any test runs.

# Token of a user with write access to the site; lets Matomo record the caller's IP
MATOMO_TOKEN: Optional[str] = None

@field_validator("MATOMO_SITE_ID", "MATOMO_TOKEN", mode="before")
@classmethod
def empty_is_unset(cls, value):
"""Treat MATOMO_SITE_ID= (as in .env.example) as unset instead of failing to start."""
return None if value == "" else value

# Database configuration
CLEAN_DB_USER: str
CLEAN_DB_PASSWORD: str
Expand Down
94 changes: 94 additions & 0 deletions app/core/matomo.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,94 @@
import asyncio
import hashlib
import hmac

import httpx
from loguru import logger
from starlette.requests import Request

from app.core.config import settings

# Shared client, created in start() only when tracking is configured; None means tracking is off.
_client: httpx.AsyncClient | None = None
# Hits still being sent; holding a reference keeps the tasks from being garbage collected.
_pending: set[asyncio.Task] = set()


def start() -> None:
global _client
if settings.MATOMO_SITE_ID is not None:
# The connection limit caps concurrent hits; hits waiting for a connection give up
# after the pool timeout, so a slow Matomo can't pile up open connections.
_client = httpx.AsyncClient(timeout=5, limits=httpx.Limits(max_connections=10))


async def stop() -> None:
"""Give in-flight hits a few seconds to finish, then close the client."""
global _client
if _client is None:
return
if _pending:
await asyncio.wait(_pending, timeout=5)
await _client.aclose()

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Minor, not run: when asyncio.wait times out, the leftover tasks are still running when aclose() closes the client underneath them. Each one then logs a tracking-failure warning, and any task still pending when the loop closes can produce "Task was destroyed but it is pending". Cancelling what's left in _pending before aclose() would make shutdown quiet.

_client = None


def build_hit(request: Request) -> dict:
"""Build the Matomo tracking parameters for one API request."""
ua = request.headers.get("user-agent", "")
params = {
"idsite": settings.MATOMO_SITE_ID,
"rec": 1,
"apiv": 1,
"send_image": 0,
# Path only: query strings would make every search and typeahead keystroke its own page.
"url": str(request.url.replace(query="")),
"action_name": request.url.path,
"ua": ua,
"urlref": request.headers.get("referer", ""),
"lang": request.headers.get("accept-language", ""),
}
ip = request.client.host if request.client else None
# Matomo ignores cip (the caller's IP) unless the request carries a token with write access.
if settings.MATOMO_TOKEN and ip:
params.update(
token_auth=settings.MATOMO_TOKEN,
cip=ip,
# API clients send no Matomo cookie, so derive a stable visitor id from IP + user
# agent. Keyed with the token so the id can't be reversed into the caller's IP;
# rotating the token resets visitor ids.
cid=hmac.new(settings.MATOMO_TOKEN.encode(), f"{ip}|{ua}".encode(),
hashlib.sha256).hexdigest()[:16],
)
return params


async def send_hit(params: dict) -> None:
try:
response = await _client.post(f"{settings.MATOMO_URL}/matomo.php", data=params)
response.raise_for_status()
except Exception as e:
# Tracking must never affect the API, whatever goes wrong.
logger.warning(f"Matomo tracking failed: {e!r}")

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Minor, not run: when Matomo is down, every API request logs one warning, and with typeahead that's one per keystroke. A long outage would flood the pod logs and bury real errors. Logging the first failure and then suppressing (or rate-limiting) until a hit succeeds again would keep the signal without the noise.



class MatomoTrackingMiddleware:
"""Record each API request in Matomo, after the response, without delaying it.

Disabled unless MATOMO_SITE_ID is set (see start()). CORS preflight (OPTIONS) requests are
skipped so a browser call is counted once.
"""

def __init__(self, app):
self.app = app

async def __call__(self, scope, receive, send):
if scope["type"] != "http" or _client is None or scope["method"] == "OPTIONS":
await self.app(scope, receive, send)
return
try:
await self.app(scope, receive, send)
finally:
task = asyncio.create_task(send_hit(build_hit(Request(scope))))

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nothing limits how many of these run at once, and each one opens its own HTTPS connection (AsyncClient per hit, line 39). Typeahead sends a request per keystroke, so if Matomo is slow or unreachable, tasks accumulate for up to 5 s each, each holding a TCP+TLS connection, and the timeout is the only limit. A single AsyncClient created in lifespan (closed next to _db.disconnect()) with connection limits would reuse connections and cap concurrency. It would also give shutdown a place to wait for _pending, so in-flight hits aren't lost.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in 216400a. matomo.start() in lifespan creates one shared AsyncClient with httpx.Limits(max_connections=10) and a 5 s timeout. Hits waiting for a connection give up after the pool timeout, and those failures are logged. matomo.stop() waits up to 5 s for _pending, then closes the client. test_stop_waits_for_in_flight_hits covers the wait and fails if it's removed. I haven't measured how the limit behaves under real typeahead traffic.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good. With the validator and the wait in stop() removed, the suite catches both. The task count itself is still uncapped (each request starts a task that can wait up to 5 s for a connection), but with sockets capped at 10 that seems acceptable. I agree it's worth watching once real typeahead traffic hits it.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

build_hit(Request(scope)) runs right here in the finally, outside send_hit's try/except, so an error while building the hit escapes the middleware. Reproduced with the Starlette installed in .venv: a request with Host: [abc makes request.url.replace(query="") raise ValueError: Invalid IPv6 URL. That gets logged as "Exception in ASGI application". If the endpoint had already raised, this error replaces the original one in ServerErrorMiddleware and in the logs.

Reachability is limited: the mmli2 ingress routes by host, so a request like this can't arrive through Traefik. It can only arrive by calling the pod or service directly. Still, building the hit inside the task (for example asyncio.create_task(send_hit(...)), with send_hit calling build_hit inside its try) would make "tracking can't break the API" true for the whole path, not just the POST. Note that Request(scope) would then have to be built before the task starts, because the scope belongs to this call.

_pending.add(task)
task.add_done_callback(_pending.discard)
6 changes: 6 additions & 0 deletions app/main.py
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@
from fastapi.middleware.cors import CORSMiddleware
from fastapi.middleware.gzip import GZipMiddleware

from app.core import matomo
from app.core.config import settings
from app.db.database import _db
from app.routers import search
Expand All @@ -14,9 +15,11 @@ async def lifespan(app: FastAPI):
"""Lifespan context manager for database connection handling."""
# Connect to database on startup
await _db.connect()
matomo.start()
yield
# Disconnect from database on shutdown
await _db.disconnect()
await matomo.stop()

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Minor, not run: if _db.disconnect() raises, matomo.stop() never runs, so in-flight hits are dropped and the client isn't closed. Hits don't depend on the DB, so stopping Matomo first, or putting the shutdown in a try/finally, would avoid tying the two together.



# Initialize FastAPI application
Expand Down Expand Up @@ -100,5 +103,8 @@ async def lifespan(app: FastAPI):
# Add GZip compression middleware to compress large responses
app.add_middleware(GZipMiddleware, minimum_size=1000)

# Record API usage in Matomo
app.add_middleware(matomo.MatomoTrackingMiddleware)

# Include API routers
app.include_router(search.router, prefix="/api/v1")
2 changes: 2 additions & 0 deletions tests/conftest.py
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,8 @@
# Settings require DB credentials at import time; tests mock the DB calls.
for key in ("CLEAN_DB_HOST", "CLEAN_DB_USER", "CLEAN_DB_PASSWORD", "CLEAN_DB_NAME"):
os.environ.setdefault(key, "test")
# Keep a developer's .env from sending test requests to the real Matomo site.
os.environ["MATOMO_SITE_ID"] = ""


@pytest.fixture
Expand Down
131 changes: 131 additions & 0 deletions tests/test_matomo.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,131 @@
import asyncio
import hashlib
from unittest.mock import AsyncMock, patch

import httpx
import pytest
from starlette.requests import Request

import app.core.matomo as matomo
from app.core.config import Settings, settings
from app.main import app


@pytest.fixture
async def tracking(monkeypatch):
monkeypatch.setattr(settings, "MATOMO_SITE_ID", 7)

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Only tests using this fixture control MATOMO_SITE_ID. Settings read .env (env_file=".env"), and conftest.py doesn't clear it. So a developer with MATOMO_SITE_ID set locally who runs the full suite would have the TestClient tests in other modules record hits against the live Matomo site. I didn't run this, and I'm not certain the background tasks finish before the client closes. Forcing MATOMO_SITE_ID to None for every test in conftest.py would remove the risk.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in 216400a, in two ways. conftest.py now sets MATOMO_SITE_ID="" before settings load; environment variables take precedence over .env, and the new validator reads "" as unset. Separately, tracking only turns on in lifespan, and the existing TestClient(app) fixture doesn't use a with block, so it never runs lifespan. I didn't test whether the background tasks would have finished before the client closed. Both changes make that question moot.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed. Either change alone would have been enough, and with both the question is moot.

monkeypatch.setattr(settings, "MATOMO_TOKEN", None)
matomo.start()
with patch.object(matomo, "send_hit", AsyncMock()) as send_hit:
yield send_hit
await matomo.stop()


async def request(method, path, **kwargs):
transport = httpx.ASGITransport(app=app, client=("203.0.113.5", 4321))
async with httpx.AsyncClient(transport=transport, base_url="https://cleandb-api.test") as client:
response = await client.request(method, path, **kwargs)
await asyncio.gather(*matomo._pending)
return response


async def test_records_request_path_without_query(tracking):
response = await request("GET", "/api/v1/openapi.json?accession=P51451",
headers={"user-agent": "curl/8", "referer": "https://example.org/"})
assert response.status_code == 200
params = tracking.await_args.args[0]
assert params["idsite"] == 7
assert params["rec"] == 1
assert params["url"] == "https://cleandb-api.test/api/v1/openapi.json"
assert params["action_name"] == "/api/v1/openapi.json"
assert params["ua"] == "curl/8"
assert params["urlref"] == "https://example.org/"
assert not {"cip", "cid", "token_auth"} & params.keys()


async def test_token_sends_caller_ip_and_keyed_visitor_id(tracking, monkeypatch):
monkeypatch.setattr(settings, "MATOMO_TOKEN", "secret")
await request("GET", "/api/v1/openapi.json", headers={"user-agent": "a"})
await request("GET", "/api/v1/openapi.json", headers={"user-agent": "a"})
await request("GET", "/api/v1/openapi.json", headers={"user-agent": "b"})
first, again, other_ua = (c.args[0] for c in tracking.await_args_list)
assert first["token_auth"] == "secret"
assert first["cip"] == "203.0.113.5"
assert len(first["cid"]) == 16
assert first["cid"] == again["cid"] != other_ua["cid"]
# Not recoverable by hashing candidate IPs and user agents without the token.
assert first["cid"] != hashlib.sha256(b"203.0.113.5|a").hexdigest()[:16]

monkeypatch.setattr(settings, "MATOMO_TOKEN", "other")
await request("GET", "/api/v1/openapi.json", headers={"user-agent": "a"})
assert tracking.await_args.args[0]["cid"] != first["cid"]


def test_no_caller_address_sends_no_ip_or_visitor_id(monkeypatch):
monkeypatch.setattr(settings, "MATOMO_TOKEN", "secret")
scope = {"type": "http", "method": "GET", "scheme": "https", "server": ("api.test", 443),
"path": "/api/v1/search", "query_string": b"", "headers": [], "client": None}
params = matomo.build_hit(Request(scope))
assert not {"cip", "cid", "token_auth"} & params.keys()


async def test_records_failed_requests(tracking):
response = await request("GET", "/api/v1/no-such-endpoint")
assert response.status_code == 404
tracking.assert_awaited_once()


async def test_skips_cors_preflight(tracking):
await request("OPTIONS", "/api/v1/search",
headers={"origin": "https://example.org", "access-control-request-method": "GET"})
tracking.assert_not_awaited()


async def test_disabled_without_site_id(monkeypatch):
monkeypatch.setattr(settings, "MATOMO_SITE_ID", None)
matomo.start()
with patch.object(matomo, "send_hit", AsyncMock()) as send_hit:
response = await request("GET", "/api/v1/openapi.json")
assert response.status_code == 200
send_hit.assert_not_awaited()


def test_empty_settings_mean_unset(monkeypatch):
monkeypatch.setenv("MATOMO_SITE_ID", "")
monkeypatch.setenv("MATOMO_TOKEN", "")
loaded = Settings(_env_file=None)
assert loaded.MATOMO_SITE_ID is None
assert loaded.MATOMO_TOKEN is None


@pytest.mark.parametrize("url", [
"http://127.0.0.1:9", # nothing listens on port 9, so the POST fails to connect
"http://matomo:not-a-port", # httpx.InvalidURL, which is not an httpx.HTTPError
])
async def test_send_failure_is_logged_not_raised(monkeypatch, url):
monkeypatch.setattr(settings, "MATOMO_SITE_ID", 7)
monkeypatch.setattr(settings, "MATOMO_URL", url)
matomo.start()
with patch.object(matomo.logger, "warning") as warning:
await matomo.send_hit({"idsite": 7, "rec": 1})
await matomo.stop()
warning.assert_called_once()


async def test_stop_waits_for_in_flight_hits(monkeypatch):
monkeypatch.setattr(settings, "MATOMO_SITE_ID", 7)
matomo.start()

async def slow_post(*args, **kwargs):
await asyncio.sleep(0.1)
return httpx.Response(204, request=httpx.Request("POST", "https://matomo.test"))

with patch.object(matomo._client, "post", side_effect=slow_post) as post:
transport = httpx.ASGITransport(app=app)
async with httpx.AsyncClient(transport=transport, base_url="https://cleandb-api.test") as client:
await client.get("/api/v1/openapi.json")
assert matomo._pending
await matomo.stop()
post.assert_awaited_once()
assert not matomo._pending
assert matomo._client is None
Loading