Skip to content

fix: reject CLEAN submissions with no sequences - #162

Open
matthewberry wants to merge 2 commits into
mainfrom
mjberry/fix/clean-empty-input
Open

matthewberry wants to merge 2 commits into
mainfrom
mjberry/fix/clean-empty-input

Conversation

@matthewberry

Copy link
Copy Markdown
Collaborator

Problem

POST /clean/jobs accepts a CLEAN job with no sequences and runs it.

  • No schema check on the legacy route: it skips schema validation (app/routers/job.py). The v1 schema's minItems: 1 (clean.input.json) never applies to it.
  • The builder checks only the maximum: CleanService.build_clean_job_command rejects more than 20 sequences, but has no lower bound.
  • What follows: with {"input_fasta": {}} or [], the builder writes an empty FASTA and the Job starts. CLEAN then crashes, because an empty FASTA leaves get_ec_id_dict with no rows and torch.cat gets an empty list.

Job 8e8bca70… (2026-07-07) was this case. Its job_info is {"input_fasta":{}}, its command writes an empty FASTA, and it has no _maxsep.csv. It was marked completed only because of the pipefail bug that #161 fixes. With #161 alone, this case would end as error; this change rejects it before a Job is created.

Change

build_clean_job_command raises a 400 when there are no sequences, next to the existing over-20 check and in the same style. Both routes call it through job_builder.prepare_job. On the legacy route, prepare_job runs before the DB row is written, so a rejected submission leaves no row and creates no Job. On v1, the schema already rejects it first.

Testing

  • New tests/test_clean_empty_input.py posts to /clean/jobs:
    • an empty object and an empty list each get a 400 naming the problem, and no Kubernetes job is created;
    • one valid sequence still gets a 201 and one Job.
  • On main, both empty cases get a 201 and create a Job.
  • Full suite in the Dockerfile.test image: 626 passed.

The legacy route skips schema validation and the CLEAN command builder only checked
the upper bound, so `{"input_fasta": {}}` ran CLEAN on an empty FASTA.

@matthewberry matthewberry left a comment

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The fix is correct. Nothing blocking.

Checked

  • Nothing is written for a rejected submission: on the legacy route, prepare_job runs before the DB row is written (app/routers/job.py ~L107 vs ~L131), so a 400 from the builder leaves no row and creates no Job.
  • The v1 claim holds, by reading: _validate_against_schema runs unconditionally at app/routers/v1/jobs.py:243, before prepare_job at :300. The only early return between them is an idempotent replay of an existing job. clean.input.json makes input_fasta an array with minItems: 1, so {} and [] both get a 422 there and never reach the builder.
  • The tests catch the bug: I ran them in the Dockerfile.test image (3 passed). With the == 0 guard disabled, both empty cases fail and the one-sequence case still passes.
  • No conflict with #161: this branch and #161's merge cleanly with each other.

Notes, not for this PR

  • The v1 path is covered by the schema, but no test pins it. One POST /v1/tools/clean/jobs with input_fasta: [] expecting 422 would.
  • Other malformed input still returns a 500, as before this PR: a non-empty dict ({"input_fasta": {"a": 1}}) iterates its keys and fails on record["sequence"] with a TypeError, and a record without DNA_sequence raises KeyError. Only the legacy route is affected; v1's schema rejects both.

@matthewberry

Copy link
Copy Markdown
Collaborator Author

Thanks for the review. I took both notes.

v1 test: added in 1e00d2c. test_v1_rejects_no_sequences posts {} and [] to /v1/tools/clean/jobs and expects a 422 with no Kubernetes job. The file's 5 tests pass in the Dockerfile.test image.

500s on malformed input: I reproduced both on this branch:

  • {"input_fasta": {"a": 1}} raises TypeError at clean_service.py:65.
  • A record without DNA_sequence raises KeyError at clean_service.py:67.

v1 returns 422 for both. I agree they're out of scope here. Both come from the legacy route skipping schema validation, which also lets empty input through for 13 other job types, so I opened #170 to cover all of it.

# v1 validates against clean.input.json (an array with minItems 1) before the builder runs
resp = client.post('/v1/tools/clean/jobs', json={'input_fasta': input_fasta})

assert resp.status_code == 422

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit, optional: FastAPI also answers 422 for its own request-validation errors, so a status check alone could keep passing if this request later failed for a different reason. I ran these two payloads against v1 and the 422 is the schema's: title is "Input failed schema validation", with one error at pointer /input_fasta. One valid sequence gets a 201. Pinning that would keep the test honest:

    assert resp.status_code == 422
    assert resp.json()["title"] == "Input failed schema validation"
    assert [e["pointer"] for e in resp.json()["errors"]] == ["/input_fasta"]

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant