diff --git a/CLAUDE.md b/CLAUDE.md index 3487a982..13a2308b 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -99,6 +99,8 @@ Solr documents contain: `curie`, `preferred_name`, `names` (synonym list), and b - **The empty-query rejection is not the same rule as `minimum_query_length`.** An empty string reaches Solr as `"" OR ()`, which is a parse error and therefore an HTTP 500 for what is really an empty search box, so `lookup()` floors its length check at 1 (`max(1, config.minimum_query_length)`) rather than deriving it from the setting alone. Folding the two together breaks the moment someone sets `NAMERES_MINIMUM_QUERY_LENGTH=0` to turn the minimum off. Exact mode is exempt from the setting but not from the floor. - **Do not declare a custom `responses={422: ...}` on an endpoint.** FastAPI adds the `HTTPValidationError` body only when the operation has not already declared a 422 of its own, so a hand-written one silently strips the schema and leaves client generators with an untyped error. For the same reason, `lookup()` reports a too-short query with `RequestValidationError`, not `HTTPException(422)`: the latter returns `{"detail": ""}` where FastAPI's own validation returns `{"detail": [...]}`. `tests/test_service.py` pins both. - **Query-side string normalization must not be applied to exact matching.** The `*_exactish` fields are a KeywordTokenizer plus a LowerCaseFilter and fold nothing else, so the smart-quote rewrite (and anything like it) would search for a string the caller never typed. The default path is unaffected because StandardTokenizer discards the punctuation anyway. +- **The custom OpenAPI document must be installed by overriding `app.openapi`, not by assigning `app.openapi_schema`.** Since FastAPI 0.137.0, `openapi()` rebuilds the schema whenever the app's recorded routes version doesn't match the router's current one, and a schema assigned directly to the attribute never carries that stamp -- so FastAPI quietly overwrites it on the first request to `/openapi.json` and serves its default document, losing `info.x-translator` (which is what SmartAPI registration keys off), `contact`, `termsOfService`, `tags` and `servers`. It fails open, so nothing but the served spec shows it: that is how v1.7.0 shipped it (issue #294). `tests/test_openapi.py` pins this, and has to go through `TestClient` -- asserting on `construct_open_api_schema()` directly passes throughout the bug. `fastapi` is pinned in `requirements.txt` for the same reason. +- **Declaring metadata in `openapi.yml` is not enough to serve it.** `construct_open_api_schema()` copies an explicit allowlist of `info` keys into the document (and `get_app_info()` a narrower one for the `FastAPI()` constructor); anything not named there is dropped without a word. That is how `info.contact` and `info.license` sat declared-but-unserved for years. Adding a key means adding it to the copy list *and* asserting it in `tests/test_openapi.py`. ## Documentation - `documentation/API.md` - Endpoint reference diff --git a/api/apidocs.py b/api/apidocs.py index 9434b6b3..d1ee3815 100644 --- a/api/apidocs.py +++ b/api/apidocs.py @@ -3,6 +3,8 @@ """ import os +from copy import deepcopy +from functools import lru_cache from pathlib import Path from typing import Dict @@ -10,12 +12,32 @@ from fastapi.openapi.utils import get_openapi +@lru_cache(maxsize=1) +def _parse_api_docs() -> Dict: + """ + Parse openapi.yml. Cached, since the file can't change under a running process and + get_app_info() is called on every /status request. + """ + with open(Path(__file__).parent / 'resources' / 'openapi.yml', 'r') as apd_file: + return load(apd_file, Loader=SafeLoader) + + +def load_api_docs() -> Dict: + """ + A fresh copy of the parsed openapi.yml. + + Callers mutate what they take out of this -- construct_open_api_schema() rewrites the + servers block from the environment -- so they must not share the cached parse, or one + call would edit a document already handed to someone else. + """ + return deepcopy(_parse_api_docs()) + + def get_app_info() -> Dict[str, str]: """ Get title, version, description from openapi.yml """ - with open(Path(__file__).parent / 'resources' / 'openapi.yml', 'r') as apd_file: - api_docs = load(apd_file, Loader=SafeLoader) + api_docs = load_api_docs() return { k : v for k,v in api_docs['info'].items() if k in [ @@ -32,11 +54,7 @@ def construct_open_api_schema(app) -> Dict[str, str]: https://fastapi.tiangolo.com/advanced/extending-openapi/ """ - with open(Path(__file__).parent / 'resources' / 'openapi.yml', 'r') as apd_file: - api_docs = load(apd_file, Loader=SafeLoader) - - if app.openapi_schema: - return app.openapi_schema() + api_docs = load_api_docs() open_api_schema = get_openapi( title=api_docs['info']['title'], @@ -53,6 +71,9 @@ def construct_open_api_schema(app) -> Dict[str, str]: if 'contact' in api_docs['info']: open_api_schema['info']['contact'] = api_docs['info']['contact'] + if 'license' in api_docs['info']: + open_api_schema['info']['license'] = api_docs['info']['license'] + if 'termsOfService' in api_docs['info']: open_api_schema['info']['termsOfService'] = api_docs['info']['termsOfService'] diff --git a/api/resources/openapi.yml b/api/resources/openapi.yml index f981ae4d..6b342a3d 100644 --- a/api/resources/openapi.yml +++ b/api/resources/openapi.yml @@ -2,10 +2,11 @@ openapi: 3.0.2 info: title: Name Resolver version: 1.7.0 - email: bizon@renci.org - name: Chris Bizon - x-id: https://github.com/cbizon - x-role: responsible developer + contact: + email: bizon@renci.org + name: Chris Bizon + x-id: https://github.com/cbizon + x-role: responsible developer description: 'Name Resolver (Name Lookup) service

This service takes lexical strings and attempts to map them to identifiers (CURIEs) from a vocabulary or ontology. An optional autocomplete mode (which assumes the query is incomplete) is available, as is an exact mode (which requires the whole string to match a name or synonym), along with many other options. diff --git a/api/server.py b/api/server.py index 5c4c4258..96abb2a3 100755 --- a/api/server.py +++ b/api/server.py @@ -917,8 +917,21 @@ async def do_lookup(string: str): return result -# Override open api schema with custom schema -app.openapi_schema = construct_open_api_schema(app) +# Override the OpenAPI schema with the one we build from api/resources/openapi.yml. +# +# This has to replace the openapi() method rather than assign to app.openapi_schema: +# since FastAPI 0.137.0, openapi() rebuilds the schema whenever the app's recorded +# routes version doesn't match the router's current one, and a schema we assigned +# ourselves never carries that stamp -- so the first request to /openapi.json silently +# overwrote it with FastAPI's default document (issue #294). +def custom_openapi(): + """Build the custom OpenAPI schema once, then serve it from the cache.""" + if not app.openapi_schema: + app.openapi_schema = construct_open_api_schema(app) + return app.openapi_schema + + +app.openapi = custom_openapi # Set up opentelemetry if enabled. if os.environ.get('OTEL_ENABLED', 'false') == 'true': diff --git a/requirements.txt b/requirements.txt index 2fcfc546..e814927f 100644 --- a/requirements.txt +++ b/requirements.txt @@ -1,5 +1,5 @@ requests -fastapi +fastapi~=0.141.1 httpx uvicorn pyyaml diff --git a/tests/test_openapi.py b/tests/test_openapi.py new file mode 100644 index 00000000..c6aff6d5 --- /dev/null +++ b/tests/test_openapi.py @@ -0,0 +1,84 @@ +""" +Tests for the OpenAPI document served at /openapi.json. + +These go through TestClient on purpose. The regression in issue #294 was invisible to +any test that called construct_open_api_schema() directly: the builder kept returning +the right document throughout, while FastAPI served its own default one instead. +""" + +from fastapi.testclient import TestClient + +from api.apidocs import construct_open_api_schema +from api.server import app + + +def test_openapi_json_carries_translator_metadata(): + """The served spec must carry what openapi.yml declares, not FastAPI's default document.""" + openapi = TestClient(app).get("/openapi.json").json() + info = openapi["info"] + + # x-translator.infores is what SmartAPI registration keys off: without it, the + # deployment looks like an unregistered service. + assert info["x-translator"]["infores"] == "infores:sri-name-resolver" + assert info["x-translator"]["component"] == "Utility" + assert info["x-translator"]["team"] + assert info["termsOfService"] + assert info["license"]["name"] + assert info["contact"]["email"] + assert info["contact"]["name"] + assert info["description"] + assert openapi["servers"] + assert openapi["tags"] + + # Every server needs the maturity and location values ITRB sets. + for server in openapi["servers"]: + assert server["x-maturity"] + assert server["x-location"] + + +def test_openapi_json_is_stable_across_requests(): + """Every request must get the same custom schema, not just the first one.""" + client = TestClient(app) + + first = client.get("/openapi.json") + second = client.get("/openapi.json") + + assert first.status_code == 200 + assert second.status_code == 200 + assert first.json() == second.json() + + +def test_server_metadata_respects_environment(monkeypatch): + """SERVER_ROOT, MATURITY_VALUE and LOCATION_VALUE override the servers block. + + This calls the builder rather than the route because the served schema is cached + after the first request, so it can't see an environment changed later. + """ + monkeypatch.setenv("SERVER_ROOT", "/nameres") + monkeypatch.setenv("MATURITY_VALUE", "testing") + monkeypatch.setenv("LOCATION_VALUE", "RENCI") + + servers = construct_open_api_schema(app)["servers"] + + assert servers + for server in servers: + assert server["url"] == "/nameres/" + assert server["x-maturity"] == "testing" + assert server["x-location"] == "RENCI" + + +def test_building_the_schema_twice_leaves_the_first_alone(monkeypatch): + """openapi.yml is parsed once and cached, so each build must get its own copy. + + construct_open_api_schema() rewrites the servers block in place from the + environment; if it worked on the cached parse, a later build would reach back and + edit a document already served to somebody. + """ + monkeypatch.setenv("MATURITY_VALUE", "development") + first = construct_open_api_schema(app) + + monkeypatch.setenv("MATURITY_VALUE", "production") + second = construct_open_api_schema(app) + + assert first["servers"][0]["x-maturity"] == "development" + assert second["servers"][0]["x-maturity"] == "production"