Repository navigation
Conversation
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
Instance retrieves now list objects by `{study}/{series}/{instance}.dcm`
instead of `{study}/{series}/{instance}`, matching the documented key layout.
The trailing CRLF is optional per RFC 2046, but multipart-rs (used by dicom-web) only recognizes the close delimiter at a line ending and drops the last part without it.
The tests use the S3-compatible storage at localhost:8333, which CI provides as a service container.
alexluft
left a comment
alexluft
left a comment
There was a problem hiding this comment.
Approving. Verified locally at c5d4f66: cargo fmt clean, clippy introduces no new warnings, cargo test --features s3 = 18 unit + 4 s3 integration + 4 stow green on stable, cargo deny check passes all four gates. Against a local SeaweedFS (nginx-forced 1,000-key LIST pages, +100 ms per S3 response, synthetic 512 KiB objects in 2/400/4,800-object studies), study TTFB p50/p90 drops from 809/826 ms to 256/264 ms for the 4,800-object study with the multipart part count equal to the object count in every run, and the gain holds (267 vs 799 ms p50) under 4-way parallel load; S2/S400 (single page) are unchanged. Also verified the .dcm instance lookup ends the 1.2.3 vs 1.2.34 collision, both backends' responses now end with a CRLF, and the old unwrap() panic on a bad object is now a clean error.
Non-blocking notes inline; the one to land before the release is the MSRV metadata (Cargo.toml says 1.91, the updated dependency tree requires 1.94.1) and, ideally, the pre-header 500 on a peeked error.
src/api/wado/routes.rs:81(outside this PR's diff): the peek distinguishes empty vs non-empty, notOk/Err: a first stream error (first GET/parse, or a page-2+ LIST failure that wins the race) becomes a 200 with a failing multipart body instead of a clean 500. Nothing has been sent yet at this point, so returningINTERNAL_SERVER_ERRORonSome(Err(_))is safe and preserves the empty→404 case.
Drafted with AI agents (Qoder, codex), checked against the code and tested as above.
| trace!("Streaming {key}"); | ||
| let object = client.get_object().bucket(&bucket).key(key).send().await?; | ||
| let bytes = object.body.collect().await?.reader(); | ||
| anyhow::Ok(Arc::new(FileDicomObject::from_reader(bytes)?)) |
There was a problem hiding this comment.
dropping the tokio::spawn means FileDicomObject::from_reader now runs serially inside the response task; buffer_unordered overlaps the I/O but not the parsing. Measured total time is ~equal in my bench, but for fast storage / big parse+decode workloads consider spawn_blocking within the existing concurrency slots.
| runs-on: ubuntu-latest | ||
| steps: | ||
| - uses: actions/checkout@v4 | ||
| - uses: EmbarkStudios/cargo-deny-action@v2 |
There was a problem hiding this comment.
EmbarkStudios/cargo-deny-action@v2 is a movable tag; pin a full commit SHA, as the workflow already does for the cargo-about installer.
There was a problem hiding this comment.
I don't think it's good practice to use commit SHA's in github action workflow files
The published images contain the compiled dependencies, whose licenses require their notices in binary distributions. The docker-publish workflow generates licenses.html with cargo-about for the features of each image; both Dockerfiles copy it with LICENSE to /usr/share/doc/dicom-rst/.
Set workflow-level permissions to contents: read so jobs without an explicit block (cargo-deny) cannot write, including on manual runs. Grant check-rust-build the security-events: write and actions: read permissions it needs to upload the clippy SARIF report.
Store 1.2.3.1.100 next to 1.2.3.1.10 and assert that retrieving 1.2.3.1.10 yields exactly one part, so a prefix match that also returns the longer UID is caught. The instance URL is requested via retrieve_series because retrieve_instance only reads the first part. Collect study and series responses into a Vec and check that the part count equals the stored object count before comparing UID sets, so duplicate parts are no longer hidden by the set.
The AWS SDK crates updated by cargo update (aws-sdk-s3 1.156.0, aws-config 1.13.0 and their smithy dependencies) require rustc 1.94.1, so the previous rust-version of 1.91.0 no longer builds the locked tree.
{instance}.dcmexactly;1.2.3no longer also returns1.2.34.dicom-webno longer drops the last instance.deny.toml, adds a CI job, and drops default features of the AWS SDK anddicom-test-files, which removes all vulnerable and unsound dependencies./usr/share/doc/dicom-rst/.