From 6e8f407012c482a6c4c59e0d375fe984231b182a Mon Sep 17 00:00:00 2001 From: Benjamin Capodanno Date: Mon, 17 Aug 2026 11:57:48 -0700 Subject: [PATCH 1/3] fix(refget): declare Content-Length for the bytes actually streamed The refget sequence endpoint set Content-Length to the length of the whole sequence on every response, including subsequence requests. A request for NC_000016.10?start=2077608&end=2085800 streamed 8192 bytes under a declared 90338345, and the ASGI server aborted the response with "Response content shorter than Content-Length" once the status line was already committed. GZipMiddleware strips Content-Length from compressed responses, so only clients requesting identity encoding were affected, and TestClient's default gzip request meant no existing test could observe the header. - Resolve start/end to concrete half-open bounds as soon as the sequence length is known, and derive validation, Content-Length, Content-Range, and the generator bounds from those same values - Run bounds validation whenever either bound is supplied, not only when both are; a one-sided out-of-range request previously skipped validation and returned an empty 200 carrying the full sequence length - Apply the same one-sided bounds check to the seqrepo sequence endpoint, which never set Content-Length and so could not crash, but returned a truncated body under a 200 for coordinates past the end of the sequence. It now rejects them with 422, the code that endpoint already used for start > end - Cover every request shape with tests asserting Content-Length equals the returned body length, pinning Accept-Encoding: identity so gzip cannot mask a regression Closes #846 --- src/mavedb/routers/refget.py | 34 +++++++++++------ src/mavedb/routers/seqrepo.py | 18 ++++++++- tests/routers/test_refget.py | 72 +++++++++++++++++++++++++++++++++++ tests/routers/test_seqrepo.py | 37 ++++++++++++++++++ 4 files changed, 148 insertions(+), 13 deletions(-) diff --git a/src/mavedb/routers/refget.py b/src/mavedb/routers/refget.py index 979c63d13..70e18e2bf 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 42ec14645..9cd271312 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 760b9f022..75ddde71a 100644 --- a/tests/routers/test_refget.py +++ b/tests/routers/test_refget.py @@ -76,6 +76,78 @@ 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_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 231f06a5e..130e1a847 100644 --- a/tests/routers/test_seqrepo.py +++ b/tests/routers/test_seqrepo.py @@ -46,12 +46,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] From 26664e3fe61268613fe92368fb3282a072ac90f1 Mon Sep 17 00:00:00 2001 From: Benjamin Capodanno Date: Mon, 17 Aug 2026 11:58:02 -0700 Subject: [PATCH 2/3] chore(tests): sharpen unit and integration markers, extend refget coverage Tighten the marker definitions so the distinction is about mocking and scope rather than a vague notion of size, and mark the refget and seqrepo router suites accordingly. - Redefine `integration` as an end-to-end multi-component flow with no internal mocking, and `unit` as fast and isolated, explicitly allowing real collaborators when they are local, fast, and deterministic - Mark tests/routers/test_refget.py and tests/routers/test_seqrepo.py as unit suites - Cover the refget service-info endpoint, including the HGVS_SEQREPO_DIR-derived data version and its "unknown" fallback - Cover the 400 returned when start/end query parameters are combined with a Range header --- pyproject.toml | 4 ++-- tests/routers/test_refget.py | 30 ++++++++++++++++++++++++++++++ tests/routers/test_seqrepo.py | 2 ++ 3 files changed, 34 insertions(+), 2 deletions(-) diff --git a/pyproject.toml b/pyproject.toml index 871d29941..763d50f4a 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/tests/routers/test_refget.py b/tests/routers/test_refget.py index 75ddde71a..762f60be4 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] @@ -96,6 +116,16 @@ def test_get_sequence_only_end(client, entry): 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 diff --git a/tests/routers/test_seqrepo.py b/tests/routers/test_seqrepo.py index 130e1a847..c9163c072 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") From ab892504a9d95d104a079a2d3275f9039a22b33e Mon Sep 17 00:00:00 2001 From: Benjamin Capodanno Date: Mon, 21 Sep 2026 09:17:24 -0700 Subject: [PATCH 3/3] fix(refget): close the review's inverted-range and bounds-checking gaps Address the code-review feedback on this PR: - Reject a Range header whose last-byte-pos is exactly one less than its first-byte-pos (e.g. "bytes=5-4"). Converting it to a half-open bound collapsed it to start == end, letting it slip past every downstream check and return a 206 with an inverted, spec-invalid Content-Range. - Per the refget spec, only a start beyond the sequence length must be rejected; there's no rule requiring a lone over-long end to be rejected too, so it's now clamped to the sequence length instead (previously both were rejected with 416, a spec deviation this PR introduced when it started validating the one-sided case at all). - Align seqrepo.py's newly-added bounds validation with refget.py's: same boundary math (a start equal to the sequence length -- an empty tail slice -- is legal in both), same one-sided/two-sided split. The 422 status family is unchanged since seqrepo.py predates and isn't bound by the refget spec. Closes the malformed-range hole and the seqrepo.py/refget.py boundary disagreement flagged in review; the over-length-range clamping decision was confirmed against the refget spec's explicit start/end rules. --- src/mavedb/routers/refget.py | 21 +++++++++++++++++++-- src/mavedb/routers/seqrepo.py | 20 ++++++++++++++------ tests/routers/test_refget.py | 27 +++++++++++++++++++-------- tests/routers/test_seqrepo.py | 8 +++++++- 4 files changed, 59 insertions(+), 17 deletions(-) diff --git a/src/mavedb/routers/refget.py b/src/mavedb/routers/refget.py index 70e18e2bf..9e406c97a 100644 --- a/src/mavedb/routers/refget.py +++ b/src/mavedb/routers/refget.py @@ -158,7 +158,12 @@ def get_sequence( logger.error(msg="Invalid range header format", extra=logging_context()) raise HTTPException(status_code=400, detail="Invalid range header format") + # Rejects "bytes=5-4"-style headers (RFC 9110 S14.1.1): after conversion to a half-open + # bound, start == end here would otherwise silently pass as a valid empty range. start, end = int(m.group(1)), int(m.group(2)) + 1 + if start >= end: + logger.error(msg="Invalid range header format", extra=logging_context()) + raise HTTPException(status_code=400, detail="Invalid range header format") save_to_logging_context({"requested_refget_start": start, "requested_refget_end": end}) if start is not None and end is not None: @@ -190,8 +195,9 @@ def get_sequence( 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: + # Refget spec: only a start "larger than" the sequence length is invalid (strict inequality). + if start is not None and end is not None: + if seq_start > seq_len: raise HTTPException( status_code=416, detail="Invalid coordinates: start > sequence length", @@ -209,6 +215,17 @@ def get_sequence( detail="Invalid coordinates: must obey 0 <= start <= end <= sequence_length", headers={"Content-Range": f"bytes */{seq_len}"}, ) + # The refget spec only requires rejecting a start beyond the sequence length; there's no + # equivalent rule for a lone end (see below). + elif start is not None: + if not 0 <= seq_start <= seq_len: + raise HTTPException( + status_code=400, + detail=f"Invalid coordinates: start must satisfy 0 <= start <= {seq_len}", + ) + # Clamp the end coordinate to the valid range [0, seq_len]. + elif end is not None: + seq_end = max(0, min(seq_end, seq_len)) # Content-Length must match bytes actually streamed. Overstating it aborts the response mid-stream # once the ASGI server has already committed the status line. diff --git a/src/mavedb/routers/seqrepo.py b/src/mavedb/routers/seqrepo.py index 9cd271312..91d81c9a2 100644 --- a/src/mavedb/routers/seqrepo.py +++ b/src/mavedb/routers/seqrepo.py @@ -79,13 +79,21 @@ def get_sequence( 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: + # Mirrors refget.py's two-sided rule: start == seq_len (an empty tail slice) is legal. + if start is not None and end is not None: + if not 0 <= seq_start <= seq_end <= seq_len: + logger.error(msg="Invalid coordinates: range lies outside the sequence.", extra=logging_context()) + raise HTTPException( + status_code=422, detail=f"Invalid coordinates: must obey 0 <= start <= end <= {seq_len}" + ) + # Mirrors refget.py: a start beyond the sequence length is rejected. + elif start 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}") + raise HTTPException(status_code=422, detail=f"Invalid coordinates: must obey 0 <= start <= {seq_len}") + # Mirrors refget.py: an over-long end is clamped rather than rejected. + elif end is not None: + seq_end = max(0, min(seq_end, seq_len)) return StreamingResponse(sequence_generator(sr, seq_id, seq_start, seq_end), media_type="text/plain") diff --git a/tests/routers/test_refget.py b/tests/routers/test_refget.py index 762f60be4..c6afd8623 100644 --- a/tests/routers/test_refget.py +++ b/tests/routers/test_refget.py @@ -128,23 +128,21 @@ def test_get_sequence_range_header_and_query_params_conflict(client): 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 resp.status_code == 400 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 resp.status_code == 400 assert "Invalid coordinates" in resp.text - assert "Content-Range" in resp.headers -def test_get_sequence_invalid_query_range_only_end_too_large(client): +def test_get_sequence_only_end_too_large_clamps(client): + # Unlike start, an over-long lone end is clamped rather than rejected. 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 + assert resp.status_code == 200 + assert resp.text == "GGGG" @pytest.mark.parametrize( @@ -238,3 +236,16 @@ def test_get_sequence_range_header_invalid(client): resp = client.get(f"/api/v1/refget/sequence/{VALID_ENSEMBL_IDENTIFIER}", headers=headers) assert resp.status_code == 400 assert "Invalid range header format" in resp.text + + +def test_get_sequence_range_header_invalid_inverted_by_one(client): + resp = client.get(f"/api/v1/refget/sequence/{VALID_ENSEMBL_IDENTIFIER}", headers={"Range": "bytes=5-4"}) + assert resp.status_code == 400 + assert "Invalid range header format" in resp.text + + +def test_get_sequence_range_header_single_byte(client): + # Boundary case adjacent to the inverted range above: first-byte-pos == last-byte-pos is valid. + resp = client.get(f"/api/v1/refget/sequence/{VALID_ENSEMBL_IDENTIFIER}", headers={"Range": "bytes=1-1"}) + assert resp.status_code == 206 + assert resp.text == "G" diff --git a/tests/routers/test_seqrepo.py b/tests/routers/test_seqrepo.py index c9163c072..cfa8da063 100644 --- a/tests/routers/test_seqrepo.py +++ b/tests/routers/test_seqrepo.py @@ -81,7 +81,6 @@ def test_get_sequence_invalid_coords(client): "start=10&end=12", "start=1&end=12", "start=10", - "end=12", "start=-1&end=2", ], ) @@ -91,6 +90,13 @@ def test_get_sequence_coords_outside_sequence(client, query): assert "Invalid coordinates" in resp.text +def test_get_sequence_only_end_too_large_clamps(client): + # Mirrors refget.py: an over-long lone end is clamped rather than rejected. + resp = client.get(f"/api/v1/seqrepo/sequence/{VALID_ENSEMBL_IDENTIFIER}?end=12") + assert resp.status_code == 200 + assert resp.text == "GGGG" + + @pytest.mark.parametrize("entry", TEST_SEQREPO_INITIAL_STATE) def test_get_metadata_success(client, entry): alias = list(entry.keys())[0]