diff --git a/CLAUDE.md b/CLAUDE.md index 45657a39..3487a982 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -70,6 +70,7 @@ pip install -r requirements.txt ### Environment Variables - `SOLR_HOST` / `SOLR_PORT` - Solr connection (default: `localhost:8983`) - `SOLR_MAX_CONCURRENT_LOOKUPS` / `SOLR_TIMEOUT_SECONDS` - Bulk-lookup fan-out bound and Solr query timeout (see `documentation/Deployment.md`) +- `NAMERES_MINIMUM_QUERY_LENGTH` - Shortest query `/lookup` and `/bulk-lookup` will search for (default: 2) - `LOGLEVEL` - Logging level - `SERVER_ROOT` - API root path prefix - `MATURITY_VALUE` / `LOCATION_VALUE` - TRAPI metadata fields @@ -95,6 +96,8 @@ Solr documents contain: `curie`, `preferred_name`, `names` (synonym list), and b - **The default search is *tokenized*, not *fuzzy*.** It matches the query's tokens in any order (order and adjacency are rewarded by the phrase-field boost, not required), and `autocomplete=true` makes the final token a prefix. There is no edit-distance matching, and `lookup()` escapes Solr's `~` out of the query so callers cannot request it. Do not describe it as "fuzzy" in docs or parameter descriptions -- that promises typo tolerance the service has never had. `tests/test_service.py` pins both halves of this. - **Solr's `filterCache` is bounded by entry count (512), not by memory.** It earns its keep on shared, reusable filters (`types:`, `taxa:`, `curie:`). A filter whose value varies per query -- as exact mode's does -- must be marked `{!cache=false}`, or one bulk request evicts the whole cache and slows the ordinary search path down as collateral damage. See the comment in `lookup()`. +- **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. ## Documentation diff --git a/api/server.py b/api/server.py index 9bf24c9b..5c4c4258 100755 --- a/api/server.py +++ b/api/server.py @@ -11,10 +11,12 @@ import time import os import re +from dataclasses import dataclass from enum import Enum from typing import Dict, List, Union, Annotated, Optional from fastapi import Body, FastAPI, HTTPException, Query +from fastapi.exceptions import RequestValidationError from fastapi.responses import RedirectResponse import httpx from pydantic import BaseModel, Field @@ -22,35 +24,55 @@ from api.apidocs import get_app_info, construct_open_api_schema -SOLR_HOST = os.getenv("SOLR_HOST", "localhost") -SOLR_PORT = os.getenv("SOLR_PORT", "8983") -# The Solr core to query. In standalone mode this is the core name; the cloud-mode -# backups we used to ship called it name_lookup_shard1_replica_n1 instead (see status()). -SOLR_CORE = os.getenv("SOLR_CORE", "name_lookup") - -# The maximum number of Solr queries a single /bulk-lookup request may have in flight at once. -# bulk_lookup() runs its per-string lookups concurrently, and `strings` is unbounded, so without -# this a large bulk request would open one socket per string -- enough to exhaust this process's -# file descriptors and to stampede Solr. Raising this trades Solr load for bulk-lookup latency. -# -# Note that this bounds a *single* request, not the process: the Solr queries in flight across the -# whole service is this multiplied by the number of concurrent /bulk-lookup requests being served. -# 100 is set deliberately high on the assumption that Solr can take the strain, and should be -# revisited once we know the real request rate -- see TranslatorSRI/babel-validation#107. -# -# Clamped to at least 1: a value of 0 would produce a semaphore nobody can acquire, wedging every -# /bulk-lookup request forever with no error and no log line, which is a miserable thing to debug -# for what is usually a typo in a deployment's environment. -SOLR_MAX_CONCURRENT_LOOKUPS = max(1, int(os.getenv("SOLR_MAX_CONCURRENT_LOOKUPS", "100"))) - -# How long to wait for Solr before giving up on a single query, in seconds. -# -# This matters more than it used to. When bulk_lookup() ran its lookups sequentially, a stalled -# Solr connection held up one query; now that they run concurrently, one stalled connection can -# pin an otherwise-complete bulk request indefinitely while holding a semaphore slot. Set it to -# 0 to restore the previous behaviour of waiting forever. -SOLR_TIMEOUT_SECONDS = float(os.getenv("SOLR_TIMEOUT_SECONDS", "60")) -SOLR_TIMEOUT = SOLR_TIMEOUT_SECONDS if SOLR_TIMEOUT_SECONDS > 0 else None +@dataclass(frozen=True) +class Config: + """Runtime configuration, populated from environment variables at import.""" + # Solr connection (private — not exposed via /status). + solr_host: str = os.getenv("SOLR_HOST", "localhost") + solr_port: str = os.getenv("SOLR_PORT", "8983") + + # The Solr core to query. In standalone mode this is the core name; the cloud-mode + # backups we used to ship called it name_lookup_shard1_replica_n1 instead (see status()). + solr_core: str = os.getenv("SOLR_CORE", "name_lookup") + + # The maximum number of Solr queries a single /bulk-lookup request may have in flight at once. + # bulk_lookup() runs its per-string lookups concurrently, and `strings` is unbounded, so without + # this a large bulk request would open one socket per string -- enough to exhaust this process's + # file descriptors and to stampede Solr. Raising this trades Solr load for bulk-lookup latency. + # + # Note that this bounds a *single* request, not the process: the Solr queries in flight across + # the whole service is this multiplied by the number of concurrent /bulk-lookup requests being + # served. 100 is set deliberately high on the assumption that Solr can take the strain, and + # should be revisited once we know the real request rate -- see TranslatorSRI/babel-validation#107. + # + # Clamped to at least 1: a value of 0 would produce a semaphore nobody can acquire, wedging + # every /bulk-lookup request forever with no error and no log line, which is a miserable thing + # to debug for what is usually a typo in a deployment's environment. + solr_max_concurrent_lookups: int = max(1, int(os.getenv("SOLR_MAX_CONCURRENT_LOOKUPS", "100"))) + + # How long to wait for Solr before giving up on a single query, in seconds. + # + # This matters more than it used to. When bulk_lookup() ran its lookups sequentially, a stalled + # Solr connection held up one query; now that they run concurrently, one stalled connection can + # pin an otherwise-complete bulk request indefinitely while holding a semaphore slot. Set it to + # 0 to restore the previous behaviour of waiting forever. + solr_timeout_seconds: float = float(os.getenv("SOLR_TIMEOUT_SECONDS", "60")) + + # Queries shorter than this (after strip) are rejected: single-char queries + # are slow in Solr and never useful. Translator expects results at length 2. + minimum_query_length: int = int(os.getenv("NAMERES_MINIMUM_QUERY_LENGTH", "2")) + + @property + def solr_timeout(self) -> Optional[float]: + """The per-query httpx timeout; None (from a non-positive setting) waits forever.""" + return self.solr_timeout_seconds if self.solr_timeout_seconds > 0 else None + + def public(self) -> dict: + """Config values safe to surface via /status. Infra config stays private.""" + return {"minimum_query_length": self.minimum_query_length} + + +config = Config() app = FastAPI(**get_app_info()) logger = logging.getLogger(__name__) @@ -86,8 +108,8 @@ async def status_get() -> Dict: async def status() -> Dict: """ Return a dictionary containing status and count information for the underlying Solr instance. """ - query_url = f"http://{SOLR_HOST}:{SOLR_PORT}/solr/admin/cores" - async with httpx.AsyncClient(timeout=SOLR_TIMEOUT) as client: + query_url = f"http://{config.solr_host}:{config.solr_port}/solr/admin/cores" + async with httpx.AsyncClient(timeout=config.solr_timeout) as client: response = await client.get(query_url, params={ 'action': 'STATUS' }) @@ -112,12 +134,12 @@ async def status() -> Dict: if 'version' in app_info and app_info['version']: nameres_version = 'v' + app_info['version'] - # We should have a status for our core. Standalone Solr calls it ${SOLR_CORE} + # We should have a status for our core. Standalone Solr calls it $SOLR_CORE # (name_lookup); the older cloud-mode backups called it # name_lookup_shard1_replica_n1. A NameRes Solr only ever has one core, so if the # expected name isn't there but there is exactly one core, report on that one. cores = result.get('status', {}) - core = cores.get(SOLR_CORE) + core = cores.get(config.solr_core) if core is None and len(cores) == 1: core = next(iter(cores.values())) @@ -137,6 +159,7 @@ async def status() -> Dict: 'download_url': biolink_model_download_url, }, 'nameres_version': nameres_version, + 'config': config.public(), # .get() rather than [], like every field below it: Solr's core STATUS # returns a sparse entry for a core that is still initializing, and # /status is what the Kubernetes probes call. A KeyError here would turn @@ -162,6 +185,7 @@ async def status() -> Dict: 'download_url': biolink_model_download_url, }, 'nameres_version': nameres_version, + 'config': config.public(), } @@ -258,7 +282,7 @@ async def synonyms_post( async def name_lookup(curies) -> Dict[str, Dict]: """Returns a list of synonyms for a particular CURIE.""" time_start = time.time_ns() - query = f"http://{SOLR_HOST}:{SOLR_PORT}/solr/{SOLR_CORE}/select" + query = f"http://{config.solr_host}:{config.solr_port}/solr/{config.solr_core}/select" curie_filter = " OR ".join( f"curie:\"{curie}\"" for curie in curies @@ -267,7 +291,7 @@ async def name_lookup(curies) -> Dict[str, Dict]: "query": curie_filter, "limit": 1000000, } - async with httpx.AsyncClient(timeout=SOLR_TIMEOUT) as client: + async with httpx.AsyncClient(timeout=config.solr_timeout) as client: response = await client.post(query, json=params) response.raise_for_status() response_json = response.json() @@ -313,7 +337,11 @@ class LookupResult(BaseModel): ) async def lookup_curies_get( string: Annotated[str, Query( - description="The string to search for." + description="The string to search for. Must be at least the configured minimum length " + "(see `minimum_query_length` in `/status`, default 2) after leading/trailing " + "whitespace is stripped; shorter queries are rejected with HTTP 422. The " + "minimum does not apply when `exact` is set, where any non-empty string is " + "searched for." )], autocomplete: Annotated[bool, Query( description="Is the input string incomplete (autocomplete=true) or a complete phrase (autocomplete=false)?" @@ -370,7 +398,7 @@ async def lookup_curies_get( """ Returns cliques with a name or synonym that contains a specified string. """ - return await lookup(string, autocomplete, highlighting, offset, limit, biolink_type, only_prefixes, exclude_prefixes, only_taxa, debug, exact) + return await lookup(string, autocomplete, highlighting, offset, limit, biolink_type, only_prefixes, exclude_prefixes, only_taxa, debug, exact, raise_on_too_short=True) @app.post("/lookup", @@ -383,7 +411,11 @@ async def lookup_curies_get( ) async def lookup_curies_post( string: Annotated[str, Query( - description="The string to search for." + description="The string to search for. Must be at least the configured minimum length " + "(see `minimum_query_length` in `/status`, default 2) after leading/trailing " + "whitespace is stripped; shorter queries are rejected with HTTP 422. The " + "minimum does not apply when `exact` is set, where any non-empty string is " + "searched for." )], autocomplete: Annotated[bool, Query( description="Is the input string incomplete (autocomplete=true) or a complete phrase (autocomplete=false)?" @@ -440,7 +472,7 @@ async def lookup_curies_post( """ Returns cliques with a name or synonym that contains a specified string. """ - return await lookup(string, autocomplete, highlighting, offset, limit, biolink_type, only_prefixes, exclude_prefixes, only_taxa, debug, exact) + return await lookup(string, autocomplete, highlighting, offset, limit, biolink_type, only_prefixes, exclude_prefixes, only_taxa, debug, exact, raise_on_too_short=True) async def lookup(string: str, @@ -454,6 +486,7 @@ async def lookup(string: str, only_taxa: str = "", debug: DebugOptions = 'none', exact: Optional[ExactMatchMode] = None, + raise_on_too_short: bool = False, ) -> List[LookupResult]: """ Returns cliques with a name or synonym that contains a specified string. @@ -499,8 +532,33 @@ async def lookup(string: str, if not exact: string_lc = re.sub(r"[“”]", '"', re.sub(r"[‘’]", "'", string_lc)) - # Do we have a search string at all? - if string_lc == "": + # Is the query long enough to be worth searching? + # + # config.minimum_query_length exists because short queries are slow in the default tokenized + # search -- a one-character query matches a prefix of half the index -- and never return + # anything useful. Exact mode has neither problem: it is a single filter query against an + # untokenized field, and single-character labels are real (the gene T, the element symbols), + # so the minimum would put them permanently out of reach for no gain. It is therefore held to + # nothing but non-emptiness. + # + # The floor of 1 is not redundant with the setting. An empty query is rejected whatever + # minimum_query_length is set to, because it would otherwise reach Solr as `"" OR ()` and come + # back as a parse error -- an HTTP 500 for what is really an empty search box. + minimum_length = 1 if exact else max(1, config.minimum_query_length) + if len(string_lc) < minimum_length: + if raise_on_too_short: + # A RequestValidationError rather than an HTTPException, so that a query rejected for + # its length is reported in the same shape -- and documented by the same schema -- as + # one rejected by FastAPI's own parameter validation. A caller iterating `detail` as a + # list of errors should not have to special-case this one rejection. + raise RequestValidationError([{ + "type": "string_too_short", + "loc": ("query", "string"), + "msg": f"String should have at least {minimum_length} character(s) after " + f"leading and trailing whitespace is stripped", + "input": string, + "ctx": {"min_length": minimum_length}, + }]) return [] # For reasons I don't understand, we need to use backslash to escape characters (e.g. "\(") to remove the special @@ -649,8 +707,8 @@ async def lookup(string: str, logger.debug(f"Query: {json.dumps(params, indent=2)}") time_solr_start = time.time_ns() - query_url = f"http://{SOLR_HOST}:{SOLR_PORT}/solr/{SOLR_CORE}/select" - async with httpx.AsyncClient(timeout=SOLR_TIMEOUT) as client: + query_url = f"http://{config.solr_host}:{config.solr_port}/solr/{config.solr_core}/select" + async with httpx.AsyncClient(timeout=config.solr_timeout) as client: response = await client.post(query_url, json=params) if response.status_code >= 300: logger.error("Solr REST error: %s", response.text) @@ -831,8 +889,8 @@ async def bulk_lookup(query: NameResQuery) -> Dict[str, List[LookupResult]]: time_start = time.time_ns() # Bounded so that a single large request can't open a socket per string; see - # SOLR_MAX_CONCURRENT_LOOKUPS. - semaphore = asyncio.Semaphore(SOLR_MAX_CONCURRENT_LOOKUPS) + # Config.solr_max_concurrent_lookups. + semaphore = asyncio.Semaphore(config.solr_max_concurrent_lookups) async def do_lookup(string: str): async with semaphore: diff --git a/documentation/API.md b/documentation/API.md index 9dea8be7..9de44d9a 100644 --- a/documentation/API.md +++ b/documentation/API.md @@ -120,7 +120,7 @@ Search for cliques by a fragment of a name or synonym. **Parameters:** -- `string` (required, string): The string to search for. +- `string` (required, string): The string to search for. Must be at least the configured minimum length (see `minimum_query_length` in [`/status`](#status), default 2) after leading/trailing whitespace is stripped; shorter queries are rejected with HTTP 422, in the same body shape as any other parameter validation failure. The minimum does not apply when [`exact`](#exact-matching) is set — exact matching is a single filter query rather than a tokenized search, and short labels such as the gene `T` are legitimate targets for it — but an empty string is rejected in every mode. - `autocomplete` (optional, boolean, default: false): If `true`, treats the input string as incomplete and looks for terms that start with the final word. If `false`, treats the entire phrase as complete (entity linker mode). - `highlighting` (optional, boolean, default: false): If `true`, returns information on which labels and synonyms matched the search query. - `offset` (optional, integer, default: 0, minimum: 0): The number of results to skip. Used for pagination. @@ -195,7 +195,7 @@ Search for cliques for multiple strings in a single request. - `strings` (required, list of strings): A list of strings to search for. The returned results will be in a dictionary with these values as keys. - All other parameters are optional and apply to all searches. -**Returns:** A dictionary where each key is a string from the input `strings` array, and each value is a list of `LookupResult` objects (same structure as `/lookup` results). +**Returns:** A dictionary where each key is a string from the input `strings` array, and each value is a list of `LookupResult` objects (same structure as `/lookup` results). Unlike `/lookup`, a string shorter than the configured minimum length (see `minimum_query_length` in [`/status`](#status), default 2) is not rejected: it simply maps to an empty list, so one too-short string does not fail the whole batch. An empty string, and a string shorter than the minimum in `exact` mode, are treated the same way. **Example request:** @@ -348,7 +348,11 @@ Returns the status of the service. Most importantly, this returns the [Babel](ht version and changelog URL, which can be used to determine which version of Babel is currently loaded in this service. It also includes the NameRes version (also visible in the OpenAPI documentation) and the Biolink Model version used to build the Solr database, as well as bunch of information from the underlying -Solr database. +Solr database. The `config` object reports publicly-relevant configuration for this instance; currently just +`minimum_query_length`, the shortest query (in characters, after whitespace is stripped) that `/lookup` and +`/bulk-lookup` will search for in the default tokenized search. It defaults to 2 and can be set per deployment with the +`NAMERES_MINIMUM_QUERY_LENGTH` environment variable. It does not constrain [exact matching](#exact-matching), which +accepts any non-empty string. ```json { @@ -362,6 +366,9 @@ Solr database. "download_url": "https://raw.githubusercontent.com/biolink/biolink-model/v4.2.6-rc5/biolink-model.yaml" }, "nameres_version": "v1.5.1", + "config": { + "minimum_query_length": 2 + }, "startTime": "2025-12-19T11:53:09.638Z", "numDocs": 425583391, "maxDoc": 425586610, diff --git a/documentation/Deployment.md b/documentation/Deployment.md index 7734e778..b0b728d2 100644 --- a/documentation/Deployment.md +++ b/documentation/Deployment.md @@ -107,6 +107,7 @@ NameRes can be configured by setting environmental variables: * `SOLR_CORE`: The Solr core to query (defaults to `name_lookup`). * `SOLR_MAX_CONCURRENT_LOOKUPS`: The greatest number of Solr queries a single `/bulk-lookup` request may have in flight at once (defaults to `100`, and is clamped to at least `1`). The bound is per request, so the queries this service has in flight against Solr is this multiplied by the number of bulk lookups being served at once. * `SOLR_TIMEOUT_SECONDS`: How long to wait for any one Solr query before giving up (defaults to `60`). Set it to `0` to wait indefinitely. +* `NAMERES_MINIMUM_QUERY_LENGTH`: The shortest query, in characters after leading/trailing whitespace is stripped, that `/lookup` and `/bulk-lookup` will search for (defaults to `2`). Shorter queries are rejected with an HTTP 422 from `/lookup` and return no results from `/bulk-lookup`, since they are slow in Solr and never useful. This applies only to the default tokenized search: exact matching accepts any non-empty string, and an empty string is rejected whatever this is set to. The value in force is reported as `minimum_query_length` by `/status`. * `SERVER_NAME`: The name of this server (defaults to `infores:sri-name-resolver`) * `SERVER_ROOT`: The server root (defaults to `/`) * `MATURITY_VALUE`: How mature is this NameRes (defaults to `maturity`, e.g. `development`) diff --git a/tests/test_exact_mode.py b/tests/test_exact_mode.py index a6a9e7db..59086f14 100644 --- a/tests/test_exact_mode.py +++ b/tests/test_exact_mode.py @@ -1,8 +1,10 @@ # This file tests the exact-match mode (the `exact` parameter) on the lookup and bulk_lookup # endpoints. Tests for the default, tokenized search live in test_service.py. +import dataclasses import json import logging +import api.server from api.server import app from fastapi.testclient import TestClient @@ -221,3 +223,35 @@ def test_exact_rejects_autocomplete_in_bulk_lookup(): }) assert response.status_code == 400 assert 'autocomplete' in response.json()['detail'] + + +def test_exact_is_not_subject_to_the_minimum_query_length(monkeypatch): + """ + config.minimum_query_length guards the default tokenized search, where a very short query is + slow and useless. Exact mode is a single filter query against an untokenized field, and short + labels are real -- the gene T, the element symbols, and "AD" for Alzheimer disease here -- so + the minimum must not apply to it. + + The minimum is raised well above the query length rather than relying on the default of 2, + so that the test says something whatever the deployment default becomes. + """ + client = TestClient(app) + monkeypatch.setattr(api.server, "config", + dataclasses.replace(api.server.config, minimum_query_length=5)) + + # "AD" is a synonym of MONDO:0004975 (Alzheimer disease), and shorter than the minimum. + response = client.get("/lookup", params={'string': 'AD', 'exact': 'synonyms', 'limit': 100}) + assert response.status_code == 200 + assert 'MONDO:0004975' in [r['curie'] for r in response.json()] + + # The same query without exact mode is still rejected, which is what makes the case above a + # property of exact mode rather than of the minimum being unset. + response = client.get("/lookup", params={'string': 'AD', 'limit': 100}) + assert response.status_code == 422 + + # Exemption from the minimum is not exemption from the empty check: an empty exact query + # would still reach Solr as an unparseable query and come back as a 500. + for string in ('', ' '): + response = client.get("/lookup", params={'string': string, 'exact': 'any'}) + assert response.status_code == 422, \ + f"Expected exact lookup of {string!r} to be rejected, got HTTP {response.status_code}" diff --git a/tests/test_service.py b/tests/test_service.py index 7c67e44b..4e5e41a3 100644 --- a/tests/test_service.py +++ b/tests/test_service.py @@ -1,3 +1,4 @@ +import dataclasses import logging import api.server @@ -16,11 +17,112 @@ def test_simple_check(): assert len(syns) == 10 def test_empty(): - """ Checks that calling NameRes without an input string return an empty list. """ + """ An empty (too-short) query on /lookup is rejected with HTTP 422. """ client = TestClient(app) response = client.get("/lookup", params={'string':''}) - syns = response.json() - assert len(syns) == 0 + assert response.status_code == 422 + + +def test_minimum_query_length(): + """ + Queries shorter than config.minimum_query_length (default 2) are rejected on + /lookup with HTTP 422, but degrade to an empty list per-key on /bulk-lookup so + one short string doesn't fail the whole batch. The minimum is reported by /status. + """ + client = TestClient(app) + + # A single character on /lookup is too short -> 422. + response = client.get("/lookup", params={'string': 'a'}) + assert response.status_code == 422 + + # A valid two-character query is not rejected (goes to Solr, returns a 200 list). + response = client.get("/lookup", params={'string': 'ab'}) + assert response.status_code == 200 + assert isinstance(response.json(), list) + + # On /bulk-lookup, a too-short string yields [] for its key; others resolve; request is 200. + response = client.post("/bulk-lookup", json={'strings': ['a', 'Parkinson'], 'limit': 100}) + assert response.status_code == 200 + results = response.json() + assert results['a'] == [] + assert len(results['Parkinson']) == 34 + + # /status advertises the configured minimum under a nested config block. Compared against + # the running config rather than a literal 2, so that this still tests the wiring in a + # deployment that sets NAMERES_MINIMUM_QUERY_LENGTH, and survives another key being added. + response = client.get("/status") + assert response.json()['config']['minimum_query_length'] == api.server.config.minimum_query_length + + +def test_too_short_is_reported_as_a_validation_error(): + """ + A query rejected for its length must come back in the same shape as one rejected by FastAPI's + own parameter validation -- `detail` a list of error objects, not a bare string -- because a + client iterating `detail` should not have to special-case this one rejection. That is also + what the endpoint's documented 422 schema (HTTPValidationError) promises; see + test_lookup_documents_the_validation_error_schema. + """ + client = TestClient(app) + response = client.get("/lookup", params={'string': 'a'}) + assert response.status_code == 422 + + detail = response.json()['detail'] + assert isinstance(detail, list), f"Expected a list of errors, got {detail!r}" + assert detail[0]['loc'] == ['query', 'string'] + assert detail[0]['type'] == 'string_too_short' + assert 'msg' in detail[0] + + # An ordinary validation failure on the same endpoint (limit above its maximum) is shaped the + # same way, which is the point of the comparison. + response = client.get("/lookup", params={'string': 'Parkinson', 'limit': 100000}) + assert response.status_code == 422 + assert isinstance(response.json()['detail'], list) + + +def test_lookup_documents_the_validation_error_schema(): + """ + FastAPI only generates the HTTPValidationError body for a 422 that the operation has not + already declared itself, so a hand-written `responses={422: ...}` on these endpoints would + silently strip the schema from the OpenAPI document and leave client generators with an + untyped error. Pin that the schema is there for both /lookup operations. + """ + client = TestClient(app) + schema = client.get("/openapi.json").json() + + for method in ('get', 'post'): + response_422 = schema['paths']['/lookup'][method]['responses']['422'] + ref = response_422['content']['application/json']['schema']['$ref'] + assert ref.endswith('/HTTPValidationError'), \ + f"/lookup {method.upper()} documents 422 as {ref}, not HTTPValidationError" + + +def test_empty_query_is_rejected_however_low_the_minimum_is(monkeypatch): + """ + An empty query must never reach Solr: it builds the query `"" OR ()`, which Solr rejects as a + parse error, turning an empty search box into an HTTP 500. The length check is what stops it, + so it has to hold even when minimum_query_length is set to 0 -- the obvious way to turn the + minimum off, and the same thing SOLR_TIMEOUT_SECONDS=0 means one setting above it. + """ + client = TestClient(app) + monkeypatch.setattr(api.server, "config", + dataclasses.replace(api.server.config, minimum_query_length=0)) + + # A single character is now allowed through to Solr... + response = client.get("/lookup", params={'string': 'a'}) + assert response.status_code == 200 + + # ...but an empty string, and one that is empty once stripped, are still rejected. + for string in ('', ' '): + response = client.get("/lookup", params={'string': string}) + assert response.status_code == 422, \ + f"Expected {string!r} to be rejected, got HTTP {response.status_code}" + + # And on /bulk-lookup it degrades to [] for that key rather than failing the whole batch. + response = client.post("/bulk-lookup", json={'strings': ['', 'Parkinson'], 'limit': 100}) + assert response.status_code == 200 + results = response.json() + assert results[''] == [] + assert len(results['Parkinson']) > 0 def test_limit(): client = TestClient(app) @@ -263,12 +365,14 @@ def test_bulk_lookup_beyond_concurrency_limit(monkeypatch): keyed to its own results, however the lookups interleave. This is a property of bulk lookup rather than of exact matching; exact=label is used only to make each string's result definite. - The limit is patched down rather than sending SOLR_MAX_CONCURRENT_LOOKUPS-worth of real strings, - because the default (100) is larger than the number of distinct labels in the test data. Patching - the module attribute works because bulk_lookup() builds its semaphore per request, at call time. + The limit is patched down rather than sending a whole config.solr_max_concurrent_lookups worth + of real strings, because the default (100) is larger than the number of distinct labels in the + test data. Swapping in a replacement Config works because bulk_lookup() reads the limit and + builds its semaphore per request, at call time. """ client = TestClient(app) - monkeypatch.setattr(api.server, "SOLR_MAX_CONCURRENT_LOOKUPS", 3) + monkeypatch.setattr(api.server, "config", + dataclasses.replace(api.server.config, solr_max_concurrent_lookups=3)) expected = { 'parkinsonian disorder': 'HP:0001300', @@ -285,7 +389,7 @@ def test_bulk_lookup_beyond_concurrency_limit(monkeypatch): 'antiparkinson agent': 'CHEBI:48407', 'BACE1 inhibitor': 'CHEBI:74925', } - assert len(expected) > api.server.SOLR_MAX_CONCURRENT_LOOKUPS, \ + assert len(expected) > api.server.config.solr_max_concurrent_lookups, \ "This test is only meaningful with more strings than can be looked up concurrently." response = client.post("/bulk-lookup", json={ @@ -326,27 +430,27 @@ def test_default_search_does_not_tolerate_misspellings(): def test_solr_settings_are_sane(): # A concurrency limit of 0 would make every bulk lookup wait on a semaphore nobody can acquire. - assert api.server.SOLR_MAX_CONCURRENT_LOOKUPS >= 1 + assert api.server.config.solr_max_concurrent_lookups >= 1 # A stalled Solr connection must not be able to pin a bulk request forever by default. - assert api.server.SOLR_TIMEOUT is None or api.server.SOLR_TIMEOUT > 0 + assert api.server.config.solr_timeout is None or api.server.config.solr_timeout > 0 def test_concurrency_limit_is_clamped_to_at_least_one(monkeypatch): """ SOLR_MAX_CONCURRENT_LOOKUPS=0 would build a semaphore nobody can ever acquire, hanging every - bulk lookup with no error and no log line. The clamp runs at import, so this has to reload the - module to exercise it. + bulk lookup with no error and no log line. The clamp runs when Config is instantiated at import, + so this has to reload the module to exercise it. """ import importlib try: monkeypatch.setenv("SOLR_MAX_CONCURRENT_LOOKUPS", "0") importlib.reload(api.server) - assert api.server.SOLR_MAX_CONCURRENT_LOOKUPS == 1 + assert api.server.config.solr_max_concurrent_lookups == 1 monkeypatch.setenv("SOLR_MAX_CONCURRENT_LOOKUPS", "25") importlib.reload(api.server) - assert api.server.SOLR_MAX_CONCURRENT_LOOKUPS == 25 + assert api.server.config.solr_max_concurrent_lookups == 25 finally: # Restore the module for whatever runs next, since reload mutates it in place. monkeypatch.delenv("SOLR_MAX_CONCURRENT_LOOKUPS", raising=False)