diff --git a/pyproject.toml b/pyproject.toml index 871d2994..763d50f4 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -111,8 +111,8 @@ testpaths = "tests/" pythonpath = "." norecursedirs = "tests/helpers/" markers = """ - integration: tests that exercise multi-component flows and/or database interactions. - unit: fast unit tests with isolated dependencies. + integration: tests that exercise a multi-component flow end-to-end, with no internal mocking. + unit: fast, isolated tests. Real un-mocked collaborators are fine if they're local, fast, and deterministic. network: tests which interact with external services over a network connection. slow: tests which are slow running. """ diff --git a/src/mavedb/routers/refget.py b/src/mavedb/routers/refget.py index 979c63d1..70e18e2b 100644 --- a/src/mavedb/routers/refget.py +++ b/src/mavedb/routers/refget.py @@ -183,37 +183,47 @@ def get_sequence( ) seq_id = seq_ids[0] - seqinfo = sr.sequences.fetch_seqinfo(seq_id) + seq_len = sr.sequences.fetch_seqinfo(seq_id)["len"] - if start is not None and end is not None: - if start >= seqinfo["len"]: + # Resolve to concrete half-open bounds up front so validation, Content-Length, and the streamed + # body all agree, even when only one of start/end is supplied. + seq_start = start if start is not None else 0 + seq_end = end if end is not None else seq_len + + if start is not None or end is not None: + if seq_start >= seq_len: raise HTTPException( status_code=416, detail="Invalid coordinates: start > sequence length", - headers={"Content-Range": f"bytes */{seqinfo['len']}"}, + headers={"Content-Range": f"bytes */{seq_len}"}, ) - if end > seqinfo["len"]: + if seq_end > seq_len: raise HTTPException( status_code=416, detail="Invalid coordinates: end > sequence length", - headers={"Content-Range": f"bytes */{seqinfo['len']}"}, + headers={"Content-Range": f"bytes */{seq_len}"}, ) - if not (0 <= start <= end <= seqinfo["len"]): + if not (0 <= seq_start <= seq_end <= seq_len): raise HTTPException( status_code=416, detail="Invalid coordinates: must obey 0 <= start <= end <= sequence_length", - headers={"Content-Range": f"bytes */{seqinfo['len']}"}, + headers={"Content-Range": f"bytes */{seq_len}"}, ) - headers = {"Content-Length": str(seqinfo["len"])} - if start is not None and end is not None and range_header: + # Content-Length must match bytes actually streamed. Overstating it aborts the response mid-stream + # once the ASGI server has already committed the status line. + headers = {"Content-Length": str(seq_end - seq_start)} + if range_header: status = 206 - headers["Content-Range"] = f"bytes {start}-{end - 1}/{seqinfo['len']}" + headers["Content-Range"] = f"bytes {seq_start}-{seq_end - 1}/{seq_len}" headers["Accept-Ranges"] = "bytes" else: status = 200 headers["Accept-Ranges"] = "none" return StreamingResponse( - sequence_generator(sr, seq_ids[0], start, end), media_type="text/plain", status_code=status, headers=headers + sequence_generator(sr, seq_id, seq_start, seq_end), + media_type="text/plain", + status_code=status, + headers=headers, ) diff --git a/src/mavedb/routers/seqrepo.py b/src/mavedb/routers/seqrepo.py index 42ec1464..9cd27131 100644 --- a/src/mavedb/routers/seqrepo.py +++ b/src/mavedb/routers/seqrepo.py @@ -71,7 +71,23 @@ def get_sequence( status_code=400, detail=f"Multiple sequences exist for alias '{alias}'. Use an explicit namespace." ) - return StreamingResponse(sequence_generator(sr, seq_ids[0], start, end), media_type="text/plain") + seq_id = seq_ids[0] + seq_len = sr.sequences.fetch_seqinfo(seq_id)["len"] + + # Resolve to concrete half-open bounds so the check and the streamed body agree, even when only + # one of start/end is supplied. + seq_start = start if start is not None else 0 + seq_end = end if end is not None else seq_len + + if start is not None or end is not None: + if not 0 <= seq_start < seq_len: + logger.error(msg="Invalid coordinates: start lies outside the sequence.", extra=logging_context()) + raise HTTPException(status_code=422, detail=f"Invalid coordinates: must obey 0 <= start < {seq_len}") + if not seq_start <= seq_end <= seq_len: + logger.error(msg="Invalid coordinates: end lies outside the sequence.", extra=logging_context()) + raise HTTPException(status_code=422, detail=f"Invalid coordinates: must obey start <= end <= {seq_len}") + + return StreamingResponse(sequence_generator(sr, seq_id, seq_start, seq_end), media_type="text/plain") @router.get("/metadata/{alias}", response_model=SeqRepoMetadata, summary="Get sequence metadata by alias") diff --git a/tests/routers/test_refget.py b/tests/routers/test_refget.py index 760b9f02..762f60be 100644 --- a/tests/routers/test_refget.py +++ b/tests/routers/test_refget.py @@ -3,6 +3,8 @@ import pytest +pytestmark = pytest.mark.unit + arq = pytest.importorskip("arq") cdot = pytest.importorskip("cdot") fastapi = pytest.importorskip("fastapi") @@ -12,6 +14,24 @@ from tests.helpers.constants import TEST_SEQREPO_INITIAL_STATE, VALID_ENSEMBL_IDENTIFIER +@pytest.mark.parametrize( + "env_value,expected_data_version", + [(None, "unknown"), ("/some/path/seqrepo/20240101", "20240101")], +) +def test_service_info(client, monkeypatch, env_value, expected_data_version): + if env_value is None: + monkeypatch.delenv("HGVS_SEQREPO_DIR", raising=False) + else: + monkeypatch.setenv("HGVS_SEQREPO_DIR", env_value) + + resp = client.get("/api/v1/refget/sequence/service-info") + assert resp.status_code == 200 + data = resp.json() + assert data["name"] == "MaveDB API" + assert data["seqrepo_data_version"] == expected_data_version + assert data["refget"]["identifier_types"] == ["refseq", "ensembl"] + + @pytest.mark.parametrize("entry", TEST_SEQREPO_INITIAL_STATE) def test_get_metadata_success(client, entry): alias = list(entry.keys())[0] @@ -76,6 +96,88 @@ def test_get_sequence_with_range_query(client, entry): assert resp.text == metadata["seq"][start:end] +@pytest.mark.parametrize("entry", TEST_SEQREPO_INITIAL_STATE) +def test_get_sequence_only_start(client, entry): + alias = list(entry.keys())[0] + metadata = list(entry.values())[0] + start = 1 + resp = client.get(f"/api/v1/refget/sequence/{alias}", params={"start": start}) + assert resp.status_code == 200 + assert resp.text == metadata["seq"][start:] + + +@pytest.mark.parametrize("entry", TEST_SEQREPO_INITIAL_STATE) +def test_get_sequence_only_end(client, entry): + alias = list(entry.keys())[0] + metadata = list(entry.values())[0] + end = 3 + resp = client.get(f"/api/v1/refget/sequence/{alias}", params={"end": end}) + assert resp.status_code == 200 + assert resp.text == metadata["seq"][:end] + + +def test_get_sequence_range_header_and_query_params_conflict(client): + resp = client.get( + f"/api/v1/refget/sequence/{VALID_ENSEMBL_IDENTIFIER}", + params={"start": 1}, + headers={"Range": "bytes=1-2"}, + ) + assert resp.status_code == 400 + assert "Cannot use both start/end query parameters and Range header" in resp.text + + +def test_get_sequence_invalid_query_range_only_start_negative(client): + resp = client.get(f"/api/v1/refget/sequence/{VALID_ENSEMBL_IDENTIFIER}", params={"start": -1}) + assert resp.status_code == 416 + assert "Invalid coordinates" in resp.text + assert "Content-Range" in resp.headers + + +def test_get_sequence_invalid_query_range_only_start_too_large(client): + resp = client.get(f"/api/v1/refget/sequence/{VALID_ENSEMBL_IDENTIFIER}", params={"start": 7}) + assert resp.status_code == 416 + assert "Invalid coordinates" in resp.text + assert "Content-Range" in resp.headers + + +def test_get_sequence_invalid_query_range_only_end_too_large(client): + resp = client.get(f"/api/v1/refget/sequence/{VALID_ENSEMBL_IDENTIFIER}", params={"end": 10}) + assert resp.status_code == 416 + assert "Invalid coordinates" in resp.text + assert "Content-Range" in resp.headers + + +@pytest.mark.parametrize( + "params,headers", + [ + ({}, {}), + ({"start": 1, "end": 3}, {}), + ({"start": 1}, {}), + ({"end": 3}, {}), + ({}, {"Range": "bytes=1-3"}), + ], +) +def test_get_sequence_content_length_matches_body(client, params, headers): + resp = client.get( + f"/api/v1/refget/sequence/{VALID_ENSEMBL_IDENTIFIER}", + params=params, + # Accept-Encoding: identity is required — GZipMiddleware strips Content-Length from compressed responses. + headers={**headers, "Accept-Encoding": "identity"}, + ) + assert resp.status_code in (200, 206) + assert resp.headers["Content-Length"] == str(len(resp.content)) + + +def test_get_sequence_range_header_reports_requested_span(client): + resp = client.get( + f"/api/v1/refget/sequence/{VALID_ENSEMBL_IDENTIFIER}", + headers={"Range": "bytes=1-2", "Accept-Encoding": "identity"}, + ) + assert resp.status_code == 206 + assert resp.headers["Content-Range"] == "bytes 1-2/4" + assert resp.headers["Content-Length"] == "2" + + def test_get_sequence_not_found(client): resp = client.get("/api/v1/refget/sequence/notfound") assert resp.status_code == 404 diff --git a/tests/routers/test_seqrepo.py b/tests/routers/test_seqrepo.py index 231f06a5..c9163c07 100644 --- a/tests/routers/test_seqrepo.py +++ b/tests/routers/test_seqrepo.py @@ -3,6 +3,8 @@ import pytest +pytestmark = pytest.mark.unit + arq = pytest.importorskip("arq") cdot = pytest.importorskip("cdot") fastapi = pytest.importorskip("fastapi") @@ -46,12 +48,49 @@ def test_get_sequence_multiple_ids(client): assert "Multiple sequences exist" in resp.text +@pytest.mark.parametrize("entry", TEST_SEQREPO_INITIAL_STATE) +def test_get_sequence_only_start(client, entry): + alias = list(entry.keys())[0] + metadata = list(entry.values())[0] + start = 1 + resp = client.get(f"/api/v1/seqrepo/sequence/{alias}?start={start}") + assert resp.status_code == 200 + assert resp.text == metadata["seq"][start:] + + +@pytest.mark.parametrize("entry", TEST_SEQREPO_INITIAL_STATE) +def test_get_sequence_only_end(client, entry): + alias = list(entry.keys())[0] + metadata = list(entry.values())[0] + end = 3 + resp = client.get(f"/api/v1/seqrepo/sequence/{alias}?end={end}") + assert resp.status_code == 200 + assert resp.text == metadata["seq"][:end] + + def test_get_sequence_invalid_coords(client): resp = client.get(f"/api/v1/seqrepo/sequence/{VALID_ENSEMBL_IDENTIFIER}?start=10&end=5") assert resp.status_code == 422 assert "Invalid coordinates" in resp.text +# Coordinates outside the sequence used to stream a truncated body under a 200 instead of being rejected. +@pytest.mark.parametrize( + "query", + [ + "start=10&end=12", + "start=1&end=12", + "start=10", + "end=12", + "start=-1&end=2", + ], +) +def test_get_sequence_coords_outside_sequence(client, query): + resp = client.get(f"/api/v1/seqrepo/sequence/{VALID_ENSEMBL_IDENTIFIER}?{query}") + assert resp.status_code == 422 + assert "Invalid coordinates" in resp.text + + @pytest.mark.parametrize("entry", TEST_SEQREPO_INITIAL_STATE) def test_get_metadata_success(client, entry): alias = list(entry.keys())[0]