fix: refget endpoint reported an incorrect Content-Length, leading to RuntimeErrors - #847
Open
bencap wants to merge 2 commits into
Open
Conversation
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
…erage 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
Coverage Report for CI Build 32058748293Warning No base build found for commit Coverage: 89.066%Details
Uncovered ChangesNo uncovered changes found. Coverage RegressionsRequires a base build to compare against. How to fix this → Coverage Stats
💛 - Coveralls |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
The refget sequence endpoint declared
Content-Lengthas the full length of the sequence on every response, including subsequence requests.NC_000016.10?start=2077608&end=2085800streamed 8,192 bytes under a declared 90,338,345, and the ASGI server aborted the response withRuntimeError: Response content shorter than Content-Lengthafter the status line was already committed. Clients got a truncated body.Both sequence endpoints now resolve
start/endto concrete half-open bounds once the sequence length is known, and derive validation, headers, and the streamed body from those same values.Changes
Content-Lengthreports the bytes actually streamed. Bounds validation runs whenever either bound is supplied, not only when both are. A one-sided out-of-range request previously skipped validation entirely and returned an empty 200 carrying the full sequence length.Content-Lengthand so could not crash, but returned a truncated body under a 200 for coordinates past the end of a sequence.Content-Length == len(body); added coverage for service-info and the range/query-param conflict; sharpened theunit/integrationmarker definitions.Behavior changes
Requests that were broken and now work:
RuntimeErrorserver-sideContent-Length?start=1,?end=3)Content-LengthRequests that were silently wrong and are now rejected:
Content-RangestartNote
GZipMiddlewarestripsContent-Lengthfrom compressed responses, andTestClientrequests gzip by default. That is why this reached production with the endpoint under test, and why the new assertions pinAccept-Encoding: identity. Without that header they pass regardless of the fix.