diff --git a/flowbio/cli/_samples.py b/flowbio/cli/_samples.py index c51b093..d82a759 100644 --- a/flowbio/cli/_samples.py +++ b/flowbio/cli/_samples.py @@ -271,10 +271,11 @@ def _configure_import(import_parser: argparse.ArgumentParser) -> None: def _job_id(value: str) -> SampleImportJobId: - try: - return SampleImportJobId(int(value)) - except ValueError: - raise argparse.ArgumentTypeError(f"job id must be an integer, got {value!r}") from None + # Ids are strings on the wire but always digits; a non-numeric one would + # fail deep inside the server's query building rather than at the boundary. + if not (value.isascii() and value.isdigit()): + raise argparse.ArgumentTypeError(f"job id must be an integer, got {value!r}") + return SampleImportJobId(value) def _configure_import_status(import_status: argparse.ArgumentParser) -> None: @@ -666,7 +667,7 @@ def _import_status_command( def _job_summary(job: SampleImportJob) -> str: if job.status == "COMPLETED": - ids = ", ".join(str(sample_id) for sample_id in job.sample_ids) or "none" + ids = ", ".join(job.sample_ids) or "none" suffix = _timestamp_suffix("finished", job.finished) return f"Job {job.id}: COMPLETED{suffix}. Sample ids: {ids}." if job.status == "FAILED": diff --git a/flowbio/v2/samples.py b/flowbio/v2/samples.py index 709e01e..86ba32a 100644 --- a/flowbio/v2/samples.py +++ b/flowbio/v2/samples.py @@ -32,7 +32,7 @@ from pathlib import Path from typing import TYPE_CHECKING, Literal, NewType -from pydantic import BaseModel, Field, field_validator +from pydantic import BaseModel, ConfigDict, Field, field_validator from flowbio.v2._pagination import PageIterator from flowbio.v2.exceptions import ( @@ -147,9 +147,10 @@ class MultiplexedUpload(BaseModel, frozen=True): ) -SampleImportJobId = NewType("SampleImportJobId", int) +SampleImportJobId = NewType("SampleImportJobId", str) """The identifier of a sample-import job, as returned by -:meth:`SampleResource.import_samples`.""" +:meth:`SampleResource.import_samples`. A bare integer written as a string — +the API serialises record ids as strings so browsers do not round them.""" SampleImportStatus = Literal["RUNNING", "COMPLETED", "FAILED"] @@ -198,7 +199,7 @@ class SampleImportSpec: metadata: dict[str, str] | None = None -class SampleImportJob(BaseModel, frozen=True): +class SampleImportJob(BaseModel): """A batch job that imports one or more accessions into samples. All accessions submitted in one :meth:`SampleResource.import_samples` call @@ -207,6 +208,12 @@ class SampleImportJob(BaseModel, frozen=True): ``"COMPLETED"``. """ + # coerce_numbers_to_str: a server that hasn't shipped string ids yet + # (an on-prem install caught between upgrades) sends these as integers, + # and pydantic does not coerce int -> str in lax mode by default. Python + # ints are unbounded, so accepting one here loses no precision. + model_config = ConfigDict(frozen=True, coerce_numbers_to_str=True) + id: SampleImportJobId = Field(description="Unique identifier for this import job.") status: SampleImportStatus = Field(description="The job's current lifecycle state.") created: datetime | None = Field(default=None, description="When the job was created.") @@ -217,11 +224,11 @@ class SampleImportJob(BaseModel, frozen=True): accessions: list[str] = Field( default_factory=list, description="The accessions submitted with this job, in submission order.", ) - sample_ids: list[int] = Field( + sample_ids: list[str] = Field( default_factory=list, description="The created samples' ids, corresponding to ``accessions`` once the job has completed.", ) - execution_id: int | None = Field( + execution_id: str | None = Field( default=None, description="The pipeline execution backing this job, if one was created.", ) error: str | None = Field( diff --git a/setup.py b/setup.py index bab87fd..6d007c2 100644 --- a/setup.py +++ b/setup.py @@ -5,7 +5,7 @@ setup( name="flowbio", - version="0.11.1", + version="0.12.0", description="A client for the Flow API.", long_description=long_description, long_description_content_type="text/markdown", diff --git a/source/cli.rst b/source/cli.rst index be722d8..9119e4c 100644 --- a/source/cli.rst +++ b/source/cli.rst @@ -460,7 +460,7 @@ otherwise the standard mapping above. Started import job 42 for 2 accession(s) (status: RUNNING). Check progress with 'flowbio samples import-status --job-id 42'. $ flowbio samples import --sheet ./accessions.csv --json - {"id": 42, "status": "RUNNING", "created": "2024-04-05T19:34:38Z", "started": null, "finished": null, "accessions": ["ERR1160845", "ERR10677146"], "sample_ids": [], "execution_id": null, "error": null} + {"id": "42", "status": "RUNNING", "created": "2024-04-05T19:34:38Z", "started": null, "finished": null, "accessions": ["ERR1160845", "ERR10677146"], "sample_ids": [], "execution_id": null, "error": null} ``samples import-status`` ~~~~~~~~~~~~~~~~~~~~~~~~~ @@ -517,7 +517,7 @@ mapping above. Job 42: COMPLETED (finished 2024-04-05 19:38:20 UTC). Sample ids: 101, 102. $ flowbio samples import-status --job-id 42 --json - {"id": 42, "status": "COMPLETED", "created": "2024-04-05T19:34:38Z", "started": "2024-04-05T19:34:40Z", "finished": "2024-04-05T19:38:20Z", "accessions": ["ERR1160845", "ERR10677146"], "sample_ids": [101, 102], "execution_id": 7, "error": null} + {"id": "42", "status": "COMPLETED", "created": "2024-04-05T19:34:38Z", "started": "2024-04-05T19:34:40Z", "finished": "2024-04-05T19:38:20Z", "accessions": ["ERR1160845", "ERR10677146"], "sample_ids": ["101", "102"], "execution_id": "7", "error": null} ``api get`` ~~~~~~~~~~~ diff --git a/tests/unit/cli/test_samples.py b/tests/unit/cli/test_samples.py index 23ef963..6ca5e55 100644 --- a/tests/unit/cli/test_samples.py +++ b/tests/unit/cli/test_samples.py @@ -953,14 +953,14 @@ def _job_json( execution_id: int | None = 7, ) -> dict: return { - "id": job_id, + "id": str(job_id), "status": status, "created": 1700000000, "started": 1700000001 if status != "RUNNING" else None, "finished": 1700000002 if status in ("COMPLETED", "FAILED") else None, "accessions": accessions, - "sample_ids": sample_ids or [], - "execution_id": execution_id, + "sample_ids": [str(sample_id) for sample_id in sample_ids or []], + "execution_id": None if execution_id is None else str(execution_id), "error": error, } @@ -1010,14 +1010,14 @@ def test_json_document_reports_job_fields( document = json.loads(result.stdout) assert result.stdout.count("\n") == 1 assert document == { - "id": 42, + "id": "42", "status": "RUNNING", "created": "2023-11-14T22:13:20Z", "started": None, "finished": None, "accessions": ["ERR1"], "sample_ids": [], - "execution_id": 7, + "execution_id": "7", "error": None, } @@ -1391,7 +1391,7 @@ def test_reports_running_job(self, run_cli) -> None: def test_running_job_with_started_reports_when_it_started(self, run_cli) -> None: respx.get(f"{SAMPLE_IMPORTS_URL}/42").mock( return_value=httpx.Response(HTTPStatus.OK, json={ - "id": 42, "status": "RUNNING", "created": 1700000000, + "id": "42", "status": "RUNNING", "created": 1700000000, "started": 1700000001, "finished": None, "accessions": ["ERR1"], "sample_ids": [], "execution_id": None, "error": None, }), @@ -1408,7 +1408,7 @@ def test_running_job_with_started_reports_when_it_started(self, run_cli) -> None def test_started_with_non_utc_offset_is_reported_in_utc(self, run_cli) -> None: respx.get(f"{SAMPLE_IMPORTS_URL}/42").mock( return_value=httpx.Response(HTTPStatus.OK, json={ - "id": 42, "status": "RUNNING", "created": 1700000000, + "id": "42", "status": "RUNNING", "created": 1700000000, "started": "2024-04-05T19:34:38+02:00", "finished": None, "accessions": ["ERR1"], "sample_ids": [], "execution_id": None, "error": None, }), @@ -1425,7 +1425,7 @@ def test_started_with_non_utc_offset_is_reported_in_utc(self, run_cli) -> None: def test_started_with_no_offset_is_treated_as_utc(self, run_cli) -> None: respx.get(f"{SAMPLE_IMPORTS_URL}/42").mock( return_value=httpx.Response(HTTPStatus.OK, json={ - "id": 42, "status": "RUNNING", "created": 1700000000, + "id": "42", "status": "RUNNING", "created": 1700000000, "started": "2024-04-05T19:34:38", "finished": None, "accessions": ["ERR1"], "sample_ids": [], "execution_id": None, "error": None, }), @@ -1442,7 +1442,7 @@ def test_started_with_no_offset_is_treated_as_utc(self, run_cli) -> None: def test_naive_started_is_reported_as_utc_in_json_too(self, run_cli) -> None: respx.get(f"{SAMPLE_IMPORTS_URL}/42").mock( return_value=httpx.Response(HTTPStatus.OK, json={ - "id": 42, "status": "RUNNING", "created": 1700000000, + "id": "42", "status": "RUNNING", "created": 1700000000, "started": "2024-04-05T19:34:38", "finished": None, "accessions": ["ERR1"], "sample_ids": [], "execution_id": None, "error": None, }), @@ -1460,7 +1460,7 @@ def test_naive_started_is_reported_as_utc_in_json_too(self, run_cli) -> None: def test_non_utc_offset_started_is_reported_as_utc_in_json_too(self, run_cli) -> None: respx.get(f"{SAMPLE_IMPORTS_URL}/42").mock( return_value=httpx.Response(HTTPStatus.OK, json={ - "id": 42, "status": "RUNNING", "created": 1700000000, + "id": "42", "status": "RUNNING", "created": 1700000000, "started": "2024-04-05T19:34:38+02:00", "finished": None, "accessions": ["ERR1"], "sample_ids": [], "execution_id": None, "error": None, }), @@ -1478,7 +1478,7 @@ def test_non_utc_offset_started_is_reported_as_utc_in_json_too(self, run_cli) -> def test_running_job_falls_back_to_created_when_not_started(self, run_cli) -> None: respx.get(f"{SAMPLE_IMPORTS_URL}/42").mock( return_value=httpx.Response(HTTPStatus.OK, json={ - "id": 42, "status": "RUNNING", "created": 1700000000, + "id": "42", "status": "RUNNING", "created": 1700000000, "started": None, "finished": None, "accessions": ["ERR1"], "sample_ids": [], "execution_id": None, "error": None, }), @@ -1565,6 +1565,15 @@ def test_non_numeric_job_id_reports_clear_message(self, run_cli) -> None: assert "_job_id" not in result.stderr assert "job id" in result.stderr.lower() + def test_non_ascii_digit_job_id_reports_clear_message(self, run_cli) -> None: + result = run_cli( + "--token", TOKEN, "samples", "import-status", "--job-id", "٤٢", + ) + + assert result.exit_code == 2 + assert "_job_id" not in result.stderr + assert "job id" in result.stderr.lower() + @respx.mock def test_completed_job_with_no_sample_ids_reports_none(self, run_cli) -> None: respx.get(f"{SAMPLE_IMPORTS_URL}/42").mock( @@ -1596,14 +1605,14 @@ def test_json_document_matches_job_shape(self, run_cli) -> None: document = json.loads(result.stdout) assert result.stdout.count("\n") == 1 assert document == { - "id": 42, + "id": "42", "status": "COMPLETED", "created": "2023-11-14T22:13:20Z", "started": "2023-11-14T22:13:21Z", "finished": "2023-11-14T22:13:22Z", "accessions": ["ERR1"], - "sample_ids": [101], - "execution_id": 7, + "sample_ids": ["101"], + "execution_id": "7", "error": None, } diff --git a/tests/unit/v2/test_samples.py b/tests/unit/v2/test_samples.py index ae57639..ae1857c 100644 --- a/tests/unit/v2/test_samples.py +++ b/tests/unit/v2/test_samples.py @@ -1063,7 +1063,7 @@ class TestImportSamples: def test_posts_imports_and_parses_job(self) -> None: route = respx.post(f"{DEFAULT_BASE_URL}/v2/sample-imports").mock( return_value=httpx.Response(HTTPStatus.CREATED, json={ - "id": 42, + "id": "42", "status": "RUNNING", "created": 1700000000, "started": None, @@ -1081,7 +1081,7 @@ def test_posts_imports_and_parses_job(self) -> None: ]) assert result == SampleImportJob( - id=SampleImportJobId(42), + id=SampleImportJobId("42"), status="RUNNING", created=1700000000, started=None, @@ -1097,7 +1097,7 @@ def test_posts_imports_and_parses_job(self) -> None: def test_sends_accession_and_sample_type(self) -> None: route = respx.post(f"{DEFAULT_BASE_URL}/v2/sample-imports").mock( return_value=httpx.Response(HTTPStatus.CREATED, json={ - "id": 1, "status": "RUNNING", "created": 1700000000, "accessions": ["ERR1"], + "id": "1", "status": "RUNNING", "created": 1700000000, "accessions": ["ERR1"], "sample_ids": [], "execution_id": None, "error": None, }), ) @@ -1116,7 +1116,7 @@ def test_sends_accession_and_sample_type(self) -> None: def test_sends_optional_fields_when_present(self) -> None: route = respx.post(f"{DEFAULT_BASE_URL}/v2/sample-imports").mock( return_value=httpx.Response(HTTPStatus.CREATED, json={ - "id": 1, "status": "RUNNING", "created": 1700000000, "accessions": ["ERR1"], + "id": "1", "status": "RUNNING", "created": 1700000000, "accessions": ["ERR1"], "sample_ids": [], "execution_id": None, "error": None, }), ) @@ -1151,7 +1151,7 @@ def test_sends_optional_fields_when_present(self) -> None: def test_empty_string_optional_fields_are_omitted_not_the_required_ones(self) -> None: route = respx.post(f"{DEFAULT_BASE_URL}/v2/sample-imports").mock( return_value=httpx.Response(HTTPStatus.CREATED, json={ - "id": 1, "status": "RUNNING", "created": 1700000000, "accessions": ["ERR1"], + "id": "1", "status": "RUNNING", "created": 1700000000, "accessions": ["ERR1"], "sample_ids": [], "execution_id": None, "error": None, }), ) @@ -1172,7 +1172,7 @@ def test_empty_string_optional_fields_are_omitted_not_the_required_ones(self) -> def test_sends_multiple_imports_in_one_request(self) -> None: route = respx.post(f"{DEFAULT_BASE_URL}/v2/sample-imports").mock( return_value=httpx.Response(HTTPStatus.CREATED, json={ - "id": 1, "status": "RUNNING", "created": 1700000000, "accessions": ["ERR1", "ERR2"], + "id": "1", "status": "RUNNING", "created": 1700000000, "accessions": ["ERR1", "ERR2"], "sample_ids": [], "execution_id": None, "error": None, }), ) @@ -1210,30 +1210,30 @@ class TestGetImport: def test_parses_completed_job(self) -> None: respx.get(f"{DEFAULT_BASE_URL}/v2/sample-imports/42").mock( return_value=httpx.Response(HTTPStatus.OK, json={ - "id": 42, + "id": "42", "status": "COMPLETED", "created": 1700000000, "started": 1700000001, "finished": 1700000002, "accessions": ["ERR1160845", "ERR10677146"], - "sample_ids": [101, 102], - "execution_id": 7, + "sample_ids": ["101", "102"], + "execution_id": "7", "error": None, }), ) client = Client() - result = client.samples.get_import(SampleImportJobId(42)) + result = client.samples.get_import(SampleImportJobId("42")) assert result == SampleImportJob( - id=SampleImportJobId(42), + id=SampleImportJobId("42"), status="COMPLETED", created=1700000000, started=1700000001, finished=1700000002, accessions=["ERR1160845", "ERR10677146"], - sample_ids=[101, 102], - execution_id=7, + sample_ids=["101", "102"], + execution_id="7", error=None, ) @@ -1241,20 +1241,20 @@ def test_parses_completed_job(self) -> None: def test_parses_failed_job_with_error(self) -> None: respx.get(f"{DEFAULT_BASE_URL}/v2/sample-imports/42").mock( return_value=httpx.Response(HTTPStatus.OK, json={ - "id": 42, + "id": "42", "status": "FAILED", "created": 1700000000, "started": 1700000001, "finished": 1700000002, "accessions": ["ERR1160845"], "sample_ids": [], - "execution_id": 7, + "execution_id": "7", "error": "download failed: connection reset", }), ) client = Client() - result = client.samples.get_import(SampleImportJobId(42)) + result = client.samples.get_import(SampleImportJobId("42")) assert result.status == "FAILED" assert result.error == "download failed: connection reset" @@ -1270,19 +1270,46 @@ def test_raises_not_found_for_unknown_job(self) -> None: client = Client() with pytest.raises(NotFoundError): - client.samples.get_import(SampleImportJobId(999)) + client.samples.get_import(SampleImportJobId("999")) @respx.mock - def test_parses_job_with_only_id_and_status(self) -> None: + def test_parses_a_job_from_a_server_that_still_sends_integer_ids(self) -> None: + # An on-prem install between client and server upgrades is a normal + # state; pydantic does not coerce int -> str in lax mode by default, + # so a server that hasn't shipped string ids yet would otherwise + # raise ValidationError on every import/import-status call. respx.get(f"{DEFAULT_BASE_URL}/v2/sample-imports/42").mock( return_value=httpx.Response(HTTPStatus.OK, json={ "id": 42, + "status": "COMPLETED", + "created": 1700000000, + "started": 1700000001, + "finished": 1700000002, + "accessions": ["ERR1160845"], + "sample_ids": [845739323725217744], + "execution_id": 7, + "error": None, + }), + ) + + client = Client() + result = client.samples.get_import(SampleImportJobId("42")) + + assert result.id == "42" + assert result.sample_ids == ["845739323725217744"] + assert result.execution_id == "7" + + @respx.mock + def test_parses_job_with_only_id_and_status(self) -> None: + respx.get(f"{DEFAULT_BASE_URL}/v2/sample-imports/42").mock( + return_value=httpx.Response(HTTPStatus.OK, json={ + "id": "42", "status": "RUNNING", }), ) client = Client() - result = client.samples.get_import(SampleImportJobId(42)) + result = client.samples.get_import(SampleImportJobId("42")) assert result.created is None assert result.started is None @@ -1296,13 +1323,13 @@ def test_parses_job_with_only_id_and_status(self) -> None: def test_naive_timestamp_is_treated_as_utc(self) -> None: respx.get(f"{DEFAULT_BASE_URL}/v2/sample-imports/42").mock( return_value=httpx.Response(HTTPStatus.OK, json={ - "id": 42, + "id": "42", "status": "RUNNING", "started": "2024-04-05T19:34:38", }), ) client = Client() - result = client.samples.get_import(SampleImportJobId(42)) + result = client.samples.get_import(SampleImportJobId("42")) assert result.started == datetime(2024, 4, 5, 19, 34, 38, tzinfo=timezone.utc)