From 83f810870919aacb63c005ca0f6279c64175f939 Mon Sep 17 00:00:00 2001 From: Brendan Foley Date: Wed, 19 Aug 2026 13:30:48 -0700 Subject: [PATCH 1/3] test(tests): resolved nfkc normalization to be idempotent and fixed unawaited method call --- .../mpcontribs_api/domains/_shared/types.py | 11 +- mpcontribs-api/tests/integration/conftest.py | 4 - .../tests/integration/test_redirects.py | 143 ------------------ .../unit/domains/test_contribution_service.py | 6 + .../unit/domains/test_search_str_tags.py | 18 +-- 5 files changed, 22 insertions(+), 160 deletions(-) delete mode 100644 mpcontribs-api/tests/integration/test_redirects.py diff --git a/mpcontribs-api/src/mpcontribs_api/domains/_shared/types.py b/mpcontribs-api/src/mpcontribs_api/domains/_shared/types.py index b7ace7d38..7e3dbfbb3 100644 --- a/mpcontribs-api/src/mpcontribs_api/domains/_shared/types.py +++ b/mpcontribs-api/src/mpcontribs_api/domains/_shared/types.py @@ -266,9 +266,16 @@ def _serialize_frame(data: pl.DataFrame) -> dict: def _nfkc_casefold(value: str) -> str: """NFKC + casefold: the case-insensitive, compatibility-folded form used for search/matching. - Surrounding whitespace is stripped by :func:`nfkc_normalize` before casefolding. + This is Unicode's ``NFKC_Casefold`` transform: ``casefold(NFKC(casefold(NFKC(x))))`` with + surrounding whitespace stripped by :func:`nfkc_normalize` first. The extra NFKC+casefold round is + what makes the result *idempotent* — a value re-normalizes to itself on read. A single + NFKC-then-casefold is not stable: casefold can expand a character (``ß`` -> ``ss``) sitting before + a combining mark, leaving an NFKC-unstable sequence that re-composes (``s`` + circumflex -> ``ŝ``) + on a second fold; and NFKC can compose a decomposed form (``t`` + diaeresis -> ``ẗ``) that then + casefolds back to the decomposed form. Folding twice reaches the fixed point either way, so the + output is both NFKC-stable and casefold-stable. """ - return nfkc_normalize(value).casefold() + return unicodedata.normalize("NFKC", nfkc_normalize(value).casefold()).casefold() def nfkc_normalize(value: str) -> str: diff --git a/mpcontribs-api/tests/integration/conftest.py b/mpcontribs-api/tests/integration/conftest.py index aedf43110..efc5b2927 100644 --- a/mpcontribs-api/tests/integration/conftest.py +++ b/mpcontribs-api/tests/integration/conftest.py @@ -79,12 +79,8 @@ async def _noop_lifespan(app: FastAPI): register_exception_handlers(app) from mpcontribs_api.api.v1.router import router as v1_router - from mpcontribs_api.domains._redirects.router import router as redirects_router app.include_router(v1_router, prefix="/api/v1") - # Mounted last (root path), exactly as in the real app, so legacy-endpoint - # redirect/deprecation behaviour is exercised by integration tests. - app.include_router(redirects_router) return app diff --git a/mpcontribs-api/tests/integration/test_redirects.py b/mpcontribs-api/tests/integration/test_redirects.py deleted file mode 100644 index 31f18f646..000000000 --- a/mpcontribs-api/tests/integration/test_redirects.py +++ /dev/null @@ -1,143 +0,0 @@ -"""Tests for the legacy-compatibility redirect/deprecation router. - -The legacy (Flask) API lived at the service root (e.g. ``/contributions/``); -the rewrite serves everything under ``/api/v1``. The redirects router mounted -at the root either 308-redirects to the new location (preserving method, body -and query string) or returns ``410 Gone`` for endpoints with no counterpart. -""" - -import pytest -from fastapi import FastAPI -from fastapi.testclient import TestClient - -from tests.integration.conftest import make_test_app - -# 308 keeps the method/body intact; assert it explicitly so a future change to -# 301/302 (which downgrade POST/PUT to GET) is caught. -PERMANENT_REDIRECT = 308 -GONE = 410 - - -@pytest.fixture(scope="module") -def app() -> FastAPI: - return make_test_app() - - -@pytest.fixture -def client(app: FastAPI): - """Client that does NOT auto-follow redirects, so we can assert on 3xx.""" - with TestClient(app, raise_server_exceptions=False, follow_redirects=False) as c: - yield c - - -# --------------------------------------------------------------------------- -# Redirects: legacy endpoints with a direct /api/v1 counterpart -# --------------------------------------------------------------------------- - -# (method, legacy_path, expected /api/v1 location) -REDIRECT_CASES = [ - # contributions - ("GET", "/contributions/", "/api/v1/contributions"), - ("POST", "/contributions/", "/api/v1/contributions"), - ("PUT", "/contributions/", "/api/v1/contributions"), - ("DELETE", "/contributions/", "/api/v1/contributions"), - ("GET", "/contributions/abc123/", "/api/v1/contributions/abc123"), - ("PUT", "/contributions/abc123/", "/api/v1/contributions/abc123"), - ("DELETE", "/contributions/abc123/", "/api/v1/contributions/abc123"), - ("GET", "/contributions/download/gz/", "/api/v1/contributions/download/gz"), - # projects (GET collection + item verbs) - ("GET", "/projects/", "/api/v1/projects"), - ("GET", "/projects/my-proj/", "/api/v1/projects/my-proj"), - ("PUT", "/projects/my-proj/", "/api/v1/projects/my-proj"), - ("DELETE", "/projects/my-proj/", "/api/v1/projects/my-proj"), - # read-only components - ("GET", "/structures/", "/api/v1/structures"), - ("GET", "/structures/sid/", "/api/v1/structures/sid"), - ("GET", "/structures/download/gz/", "/api/v1/structures/download/gz"), - ("GET", "/tables/", "/api/v1/tables"), - ("GET", "/tables/tid/", "/api/v1/tables/tid"), - ("GET", "/tables/download/gz/", "/api/v1/tables/download/gz"), - ("GET", "/attachments/", "/api/v1/attachments"), - ("GET", "/attachments/aid/", "/api/v1/attachments/aid"), - ("GET", "/attachments/download/gz/", "/api/v1/attachments/download/gz"), -] - - -class TestRedirects: - @pytest.mark.parametrize("method, legacy_path, new_path", REDIRECT_CASES) - def test_status_is_permanent_redirect(self, client, method, legacy_path, new_path): - r = client.request(method, legacy_path) - assert r.status_code == PERMANENT_REDIRECT - - @pytest.mark.parametrize("method, legacy_path, new_path", REDIRECT_CASES) - def test_location_points_to_v1(self, client, method, legacy_path, new_path): - r = client.request(method, legacy_path) - assert r.headers["location"] == new_path - - def test_query_string_is_preserved(self, client): - r = client.get("/contributions/?project=foo&_limit=5") - assert r.headers["location"] == "/api/v1/contributions?project=foo&_limit=5" - - def test_query_string_preserved_on_item(self, client): - r = client.get("/structures/sid/?_fields=id,label") - assert r.headers["location"] == "/api/v1/structures/sid?_fields=id,label" - - def test_download_query_preserved(self, client): - r = client.get("/tables/download/gz/?format=csv") - assert r.headers["location"] == "/api/v1/tables/download/gz?format=csv" - - def test_no_query_string_has_no_trailing_question_mark(self, client): - r = client.get("/projects/") - assert r.headers["location"] == "/api/v1/projects" - assert "?" not in r.headers["location"] - - -# --------------------------------------------------------------------------- -# Deprecations: legacy endpoints with no counterpart → 410 Gone -# --------------------------------------------------------------------------- - -# (method, legacy_path) -GONE_CASES = [ - # search helpers (Atlas $search) were not ported - ("GET", "/contributions/search"), - ("GET", "/projects/search"), - # project creation has no POST endpoint in the new API - ("POST", "/projects/"), - # email-driven application approval links - ("GET", "/projects/applications/sometoken"), - ("GET", "/projects/applications/sometoken/approve"), - ("GET", "/projects/applications/sometoken/deny"), - # the whole notebooks resource was dropped - ("GET", "/notebooks/"), - ("GET", "/notebooks/nbid/"), - ("GET", "/notebooks/build"), - ("GET", "/notebooks/result"), - ("GET", "/notebooks/result/job-1"), -] - - -class TestDeprecated: - @pytest.mark.parametrize("method, path", GONE_CASES) - def test_status_is_gone(self, client, method, path): - assert client.request(method, path).status_code == GONE - - @pytest.mark.parametrize("method, path", GONE_CASES) - def test_error_envelope(self, client, method, path): - body = client.request(method, path).json() - assert body["error"]["code"] == "endpoint_deprecated" - assert body["error"]["message"] - - @pytest.mark.parametrize("method, path", GONE_CASES) - def test_deprecation_header(self, client, method, path): - assert client.request(method, path).headers["deprecation"] == "true" - - def test_post_projects_points_at_put_replacement(self, client): - r = client.post("/projects/") - assert r.status_code == GONE - body = r.json() - assert body["error"]["detail"]["replacement"] == "/api/v1/projects/{id}" - - def test_deprecated_responses_are_not_redirects(self, client): - # A deprecated endpoint must never carry a Location header. - r = client.get("/notebooks/build") - assert "location" not in r.headers diff --git a/mpcontribs-api/tests/unit/domains/test_contribution_service.py b/mpcontribs-api/tests/unit/domains/test_contribution_service.py index b62d99aa0..85cb33c73 100644 --- a/mpcontribs-api/tests/unit/domains/test_contribution_service.py +++ b/mpcontribs-api/tests/unit/domains/test_contribution_service.py @@ -199,6 +199,12 @@ def _make_service( struct_repo = structures or AsyncMock() table_repo = tables or AsyncMock() attach_repo = attachments or AsyncMock() + # ``coerce_identifiers`` is a *sync* repo method (see MongoDbRepository), but a bare AsyncMock + # would turn it into a coroutine factory: the service passes its result straight into get_one/ + # patch_one without awaiting, leaking un-awaited coroutines. Make it a sync passthrough on every + # repo so it behaves like the real thing (returns the identifiers dict unchanged). + for repo in (contrib_repo, struct_repo, table_repo, attach_repo): + repo.coerce_identifiers = MagicMock(side_effect=lambda identifiers: identifiers) # Default identity resolution: every referenced project reports its ``unique_column`` (None by # default -> identity is the fixed-field triple), and no identity exists yet, so the common path # resolves with no conflict. Tests exercising duplicates override ``existing_identities``. diff --git a/mpcontribs-api/tests/unit/domains/test_search_str_tags.py b/mpcontribs-api/tests/unit/domains/test_search_str_tags.py index ebaabe31b..6c0c19cee 100644 --- a/mpcontribs-api/tests/unit/domains/test_search_str_tags.py +++ b/mpcontribs-api/tests/unit/domains/test_search_str_tags.py @@ -108,19 +108,15 @@ def test_searchstr_normalized_across_models(extract): assert extract(_MESSY_TAG) == ["file"] -@pytest.mark.xfail( - strict=True, - reason="_nfkc_casefold is not idempotent when casefold expands a char sitting before a combining mark", -) -def test_searchstr_casefold_expansion_breaks_idempotency(): - """Documents a real edge: a casefold-expanding char (ß -> ss) followed by a combining mark. +def test_searchstr_casefold_expansion_is_idempotent(): + """Covers the tricky edge: a casefold-expanding char (ß -> ss) followed by a combining mark. NFKC runs before casefold, so ``ß`` + combining circumflex stays decomposed through the first - fold (-> ``ss`` + circumflex). Re-folding then NFKC-composes ``s`` + circumflex into ``ŝ``, so - the value is not stable under a second pass. Because ``ProjectOut.tags`` is also - ``list[SearchStr]``, a stored tag re-normalizes on read and can round-trip to a different - string. xfail(strict) so this flips to a failure the moment the normalizer is made idempotent - (e.g. a trailing NFKC pass after casefold). + fold (-> ``ss`` + circumflex). A naive fold would stop there and leave an NFKC-unstable value: + re-folding NFKC-composes ``s`` + circumflex into ``ŝ``, so it would round-trip to a different + string. ``_nfkc_casefold`` runs a trailing NFKC pass after casefold to collapse this now, so the + value is stable under a second pass. This matters because ``ProjectOut.tags`` is also + ``list[SearchStr]`` and a stored tag re-normalizes on read. """ once = _search.validate_python("ß̂") # eszett + COMBINING CIRCUMFLEX ACCENT assert _search.validate_python(once) == once From 4d75d50ba0a0f29be5ef72a45d984d7ebae1b563 Mon Sep 17 00:00:00 2001 From: Brendan Foley Date: Wed, 19 Aug 2026 13:31:24 -0700 Subject: [PATCH 2/3] refactor(_redirects): removed redirect routes kong will handle redirects upstream --- mpcontribs-api/src/mpcontribs_api/app.py | 4 - .../domains/_redirects/router.py | 161 ------------------ 2 files changed, 165 deletions(-) delete mode 100644 mpcontribs-api/src/mpcontribs_api/domains/_redirects/router.py diff --git a/mpcontribs-api/src/mpcontribs_api/app.py b/mpcontribs-api/src/mpcontribs_api/app.py index 3878b703b..afdadee10 100644 --- a/mpcontribs-api/src/mpcontribs_api/app.py +++ b/mpcontribs-api/src/mpcontribs_api/app.py @@ -17,7 +17,6 @@ consumer_username_scheme, ) from mpcontribs_api.config import Settings, get_settings -from mpcontribs_api.domains._redirects.router import router as redirects_router from mpcontribs_api.domains.attachments.models import Attachment from mpcontribs_api.domains.consumers.models import Consumer from mpcontribs_api.domains.contributions.models import Contribution @@ -147,9 +146,6 @@ def create_app(settings: Settings | None = None) -> FastAPI: register_exception_handlers(app) app.include_router(healthcheck_router, prefix="/healthcheck") app.include_router(v1_router, prefix="/api/v1") - # Legacy (root-path) endpoints: 308-redirect to /api/v1 where a counterpart - # exists, else 410 Gone. Registered last so it never shadows live routes. - app.include_router(redirects_router) return app diff --git a/mpcontribs-api/src/mpcontribs_api/domains/_redirects/router.py b/mpcontribs-api/src/mpcontribs_api/domains/_redirects/router.py deleted file mode 100644 index d965e4eec..000000000 --- a/mpcontribs-api/src/mpcontribs_api/domains/_redirects/router.py +++ /dev/null @@ -1,161 +0,0 @@ -"""Compatibility shims for the legacy (Flask/flask-mongorest) MPContribs API. - -The old API was served from the root path (e.g. ``/contributions/``, -``/projects//``). The rewrite serves everything under ``/api/v1`` with -slightly different paths and verbs. This router is mounted at the root and: - -- 308-redirects every legacy endpoint that has a direct counterpart to its new - location (preserving method, body, and query string), and -- returns ``410 Gone`` with a machine-readable "deprecated" body for legacy - endpoints that have no equivalent in the new API (notebooks, the formula/term - search helpers, project-application approval links, and project creation). - -Redirects are permanent (308) so well-behaved clients update their bookmarks -while keeping the original HTTP method and request body intact. -""" - -from fastapi import APIRouter, Request -from fastapi.responses import JSONResponse, RedirectResponse -from starlette.status import HTTP_308_PERMANENT_REDIRECT, HTTP_410_GONE - -router = APIRouter(include_in_schema=False) - -# Base path of the new API. The legacy API lived at the service root. -API_V1 = "/api/v1" - - -def _redirect(request: Request, new_path: str) -> RedirectResponse: - """308-redirect to ``new_path`` under the v1 API, preserving the query string. - - 308 (rather than 301/302) keeps the original method and body, so a legacy - ``POST``/``PUT``/``DELETE`` is replayed against the new endpoint instead of - being silently downgraded to a ``GET``. - """ - target = f"{API_V1}{new_path}" - if request.url.query: - target = f"{target}?{request.url.query}" - return RedirectResponse(url=target, status_code=HTTP_308_PERMANENT_REDIRECT) - - -def _deprecated(message: str, *, replacement: str | None = None) -> JSONResponse: - """Return a ``410 Gone`` in the app's uniform error shape. - - Used for legacy endpoints that have no counterpart in the new API. - """ - detail: dict[str, str] = {} - if replacement is not None: - detail["replacement"] = replacement - body: dict = {"error": {"code": "endpoint_deprecated", "message": message}} - if detail: - body["error"]["detail"] = detail - return JSONResponse( - status_code=HTTP_410_GONE, - content=body, - headers={"Deprecation": "true"}, - ) - - -# --------------------------------------------------------------------------- -# contributions -# --------------------------------------------------------------------------- -@router.get("/contributions/search") -def redirect_contributions_search() -> JSONResponse: - # Formula autocomplete (Atlas $search) was not ported to the new API. - return _deprecated("The contributions formula search endpoint has been removed.") - - -@router.get("/contributions/download/{short_mime}/") -def redirect_download_contributions(request: Request, short_mime: str) -> RedirectResponse: - return _redirect(request, f"/contributions/download/{short_mime}") - - -@router.api_route("/contributions/", methods=["GET", "POST", "PUT", "DELETE"]) -def redirect_contributions_collection(request: Request) -> RedirectResponse: - # GET=list, POST=bulk insert, PUT=bulk upsert, DELETE=delete-by-filter. - return _redirect(request, "/contributions") - - -@router.api_route("/contributions/{pk}/", methods=["GET", "PUT", "DELETE"]) -def redirect_contribution_item(request: Request, pk: str) -> RedirectResponse: - # GET=fetch, PUT=update/upsert, DELETE=delete (all keyed by id). - return _redirect(request, f"/contributions/{pk}") - - -# --------------------------------------------------------------------------- -# projects -# --------------------------------------------------------------------------- -@router.get("/projects/search") -def redirect_projects_search() -> JSONResponse: - return _deprecated("The projects search endpoint has been removed.") - - -@router.get("/projects/applications/{token}") -@router.get("/projects/applications/{token}/{action}") -def redirect_projects_applications(token: str, action: str | None = None) -> JSONResponse: - # Email-driven project approval/denial links; not part of the new API. - return _deprecated("Project application approval links have been removed.") - - -@router.api_route("/projects/", methods=["GET", "POST"], response_model=None) -def redirect_projects_collection(request: Request) -> RedirectResponse | JSONResponse: - if request.method == "POST": - # No project-creation endpoint exists in the new API. - return _deprecated( - "Creating projects via POST is no longer supported. Create a project with PUT /api/v1/projects/{id}.", - replacement=f"{API_V1}/projects/{{id}}", - ) - return _redirect(request, "/projects") - - -@router.api_route("/projects/{pk}/", methods=["GET", "PUT", "DELETE"]) -def redirect_project_item(request: Request, pk: str) -> RedirectResponse: - # GET=fetch, PUT=update/upsert, DELETE=delete. - return _redirect(request, f"/projects/{pk}") - - -# --------------------------------------------------------------------------- -# Components -# --------------------------------------------------------------------------- -def _register_component_redirects(component: str) -> None: - @router.get(f"/{component}/download/{{short_mime}}/", name=f"redirect_download_{component}") - def redirect_download(request: Request, short_mime: str) -> RedirectResponse: - return _redirect(request, f"/{component}/download/{short_mime}") - - @router.get(f"/{component}/", name=f"redirect_{component}_collection") - def redirect_collection(request: Request) -> RedirectResponse: - return _redirect(request, f"/{component}") - - @router.get(f"/{component}/{{pk}}/", name=f"redirect_{component}_item") - def redirect_item(request: Request, pk: str) -> RedirectResponse: - return _redirect(request, f"/{component}/{pk}") - - -for _component in ("structures", "tables", "attachments"): - _register_component_redirects(_component) - - -# --------------------------------------------------------------------------- -# notebooks -# --------------------------------------------------------------------------- -_NOTEBOOKS_GONE = "The notebooks API has been removed." - - -@router.get("/notebooks/build") -def redirect_notebooks_build() -> JSONResponse: - return _deprecated(_NOTEBOOKS_GONE) - - -@router.get("/notebooks/result") -@router.get("/notebooks/result/{job_id}") -def redirect_notebooks_result(job_id: str | None = None) -> JSONResponse: - return _deprecated(_NOTEBOOKS_GONE) - - -@router.get("/notebooks/") -def redirect_notebooks_collection() -> JSONResponse: - return _deprecated(_NOTEBOOKS_GONE) - - -@router.get("/notebooks/{pk}/") -def redirect_notebooks_item(pk: str) -> JSONResponse: - return _deprecated(_NOTEBOOKS_GONE) From 4aaa484964ad551fd92ed4b9e3bb484f5695d046 Mon Sep 17 00:00:00 2001 From: Brendan Foley Date: Wed, 19 Aug 2026 13:56:08 -0700 Subject: [PATCH 3/3] docs(types.py): removed some docstring fluff --- .../src/mpcontribs_api/domains/_shared/types.py | 13 +++++-------- 1 file changed, 5 insertions(+), 8 deletions(-) diff --git a/mpcontribs-api/src/mpcontribs_api/domains/_shared/types.py b/mpcontribs-api/src/mpcontribs_api/domains/_shared/types.py index 7e3dbfbb3..cfbd75d96 100644 --- a/mpcontribs-api/src/mpcontribs_api/domains/_shared/types.py +++ b/mpcontribs-api/src/mpcontribs_api/domains/_shared/types.py @@ -266,14 +266,11 @@ def _serialize_frame(data: pl.DataFrame) -> dict: def _nfkc_casefold(value: str) -> str: """NFKC + casefold: the case-insensitive, compatibility-folded form used for search/matching. - This is Unicode's ``NFKC_Casefold`` transform: ``casefold(NFKC(casefold(NFKC(x))))`` with - surrounding whitespace stripped by :func:`nfkc_normalize` first. The extra NFKC+casefold round is - what makes the result *idempotent* — a value re-normalizes to itself on read. A single - NFKC-then-casefold is not stable: casefold can expand a character (``ß`` -> ``ss``) sitting before - a combining mark, leaving an NFKC-unstable sequence that re-composes (``s`` + circumflex -> ``ŝ``) - on a second fold; and NFKC can compose a decomposed form (``t`` + diaeresis -> ``ẗ``) that then - casefolds back to the decomposed form. Folding twice reaches the fixed point either way, so the - output is both NFKC-stable and casefold-stable. + An idempotent nfkc + casefold operation. A single NFKC-then-casefold is not stable: casefold can + expand a character (``ß`` -> ``ss``) sitting before a combining mark, leaving an NFKC-unstable + sequence that re-composes (``s`` + circumflex -> ``ŝ``) on a second fold; and NFKC can compose a + decomposed form (``t`` + diaeresis -> ``ẗ``) that then casefolds back to the decomposed form. + Folding twice reaches the fixed point either way, so the output is both NFKC-stable and casefold-stable. """ return unicodedata.normalize("NFKC", nfkc_normalize(value).casefold()).casefold()