From d9dcaabb2012c4d096fc71ae7a8f2a0dda277983 Mon Sep 17 00:00:00 2001 From: Stephan Vedder Date: Tue, 10 Nov 2026 14:19:18 +0100 Subject: [PATCH 1/2] refactor(stow): handle storage outcome correctly --- deny.toml | 1 + src/api/stgcmt/service.rs | 10 +-- src/api/stow/routes.rs | 3 +- src/api/stow/service.rs | 108 ++++++++++++++++++++++++++- src/backend/dimse/cstore/mod.rs | 42 ++++++++++- src/backend/dimse/cstore/storescp.rs | 3 +- src/backend/dimse/cstore/storescu.rs | 55 ++++++++++++-- src/backend/dimse/stow.rs | 17 +++-- tests/stow.rs | 76 ++++++++++++++++--- 9 files changed, 278 insertions(+), 37 deletions(-) diff --git a/deny.toml b/deny.toml index 81390d2..51b1868 100644 --- a/deny.toml +++ b/deny.toml @@ -7,6 +7,7 @@ unsound = "all" ignore = [ { id = "RUSTSEC-2021-0153", reason = "encoding is used by dicom-encoding (dicom-rs); no maintained replacement upstream yet" }, { id = "RUSTSEC-2024-0436", reason = "paste is used by rav1e (image, via dicom-pixeldata); proc-macro only" }, + { id = "RUSTSEC-2026-0440", reason = "astral-tokio-tar is used by testcontainers (dev-dependency only); no testcontainers release without it yet" }, ] [licenses] diff --git a/src/api/stgcmt/service.rs b/src/api/stgcmt/service.rs index 8bd56d8..6644927 100644 --- a/src/api/stgcmt/service.rs +++ b/src/api/stgcmt/service.rs @@ -1,5 +1,6 @@ +pub use crate::api::stow::FailedInstance; use crate::api::stow::InstanceReference; -use crate::types::{UI, US}; +use crate::types::UI; use async_trait::async_trait; use axum::body::Body; use axum::http::{Response, StatusCode}; @@ -106,13 +107,6 @@ pub struct InstanceLocation { pub series_instance_uid: UI, } -/// An instance for which storage has not been committed. -#[derive(Debug, Clone)] -pub struct FailedInstance { - pub reference: InstanceReference, - pub failure_reason: US, -} - /// The result of a storage commitment request, once known. /// Storage Commitment Response Module. /// diff --git a/src/api/stow/routes.rs b/src/api/stow/routes.rs index 57cdd68..7b02baf 100644 --- a/src/api/stow/routes.rs +++ b/src/api/stow/routes.rs @@ -63,10 +63,11 @@ async fn studies( instances: receiver.boxed(), }; let response = stow.store(request).await?; + let status = response.status_code(); let json = DicomJson::from(InMemDicomObject::from(response)); Ok(Response::builder() - .status(StatusCode::OK) + .status(status) .header(CONTENT_TYPE, mime::APPLICATION_JSON.as_ref()) .body(Body::from(serde_json::to_string(&json).unwrap())) .unwrap()) diff --git a/src/api/stow/service.rs b/src/api/stow/service.rs index 7f2044c..c3a79fe 100644 --- a/src/api/stow/service.rs +++ b/src/api/stow/service.rs @@ -1,4 +1,4 @@ -use crate::types::UI; +use crate::types::{UI, US}; use async_trait::async_trait; use axum::body::Body; use axum::http::{Response, StatusCode}; @@ -25,12 +25,47 @@ pub struct InstanceReference { pub sop_instance_uid: UI, } +/// An instance that could not be stored, along with the reason. +#[derive(Debug, Clone)] +pub struct FailedInstance { + pub reference: InstanceReference, + /// Failure Reason (0008,1197) + pub failure_reason: US, +} + +/// Failure Reason values of the Store Instances Response Module. +/// +pub mod failure_reason { + use crate::types::US; + + /// The origin server failed to store the instance for an unspecified reason. + pub const PROCESSING_FAILURE: US = 0x0110; +} + #[derive(Debug, Default)] pub struct StoreResponse { - pub failed_sequence: Vec, + pub failed_sequence: Vec, pub referenced_sequence: Vec, } +impl StoreResponse { + /// The HTTP status code of the response. + /// + pub const fn status_code(&self) -> StatusCode { + match ( + self.referenced_sequence.is_empty(), + self.failed_sequence.is_empty(), + ) { + // Some instances failed, others were stored. + (false, false) => StatusCode::ACCEPTED, + // No instance was stored. + (true, false) => StatusCode::CONFLICT, + // Every instance was stored (or there was nothing to store). + (_, true) => StatusCode::OK, + } + } +} + impl From for InMemDicomObject { fn from(response: StoreResponse) -> Self { let mut object = Self::new_empty(); @@ -69,12 +104,17 @@ impl From for InMemDicomObject { DataElement::new( tags::REFERENCED_SOP_INSTANCE_UID, VR::UI, - dicom_value!(Str, failed.sop_instance_uid), + dicom_value!(Str, failed.reference.sop_instance_uid), ), DataElement::new( tags::REFERENCED_SOP_CLASS_UID, VR::UI, - dicom_value!(Str, failed.sop_class_uid), + dicom_value!(Str, failed.reference.sop_class_uid), + ), + DataElement::new( + tags::FAILURE_REASON, + VR::US, + dicom_value!(U16, [failed.failure_reason]), ), ]); failed_items.push(item); @@ -152,3 +192,63 @@ impl From for StoreError { Self::Stream(error) } } + +#[cfg(test)] +mod tests { + use super::*; + + fn reference() -> InstanceReference { + InstanceReference { + sop_class_uid: UI::from("1.2.840.10008.5.1.4.1.1.2"), + sop_instance_uid: UI::from("1.2.3"), + } + } + + fn failed() -> FailedInstance { + FailedInstance { + reference: reference(), + failure_reason: failure_reason::PROCESSING_FAILURE, + } + } + + #[test] + fn status_code_reflects_stored_and_failed_instances() { + let response = |referenced_sequence, failed_sequence| StoreResponse { + failed_sequence, + referenced_sequence, + }; + + assert_eq!( + response(vec![reference()], vec![]).status_code(), + StatusCode::OK + ); + assert_eq!( + response(vec![reference()], vec![failed()]).status_code(), + StatusCode::ACCEPTED + ); + assert_eq!( + response(vec![], vec![failed()]).status_code(), + StatusCode::CONFLICT + ); + assert_eq!(response(vec![], vec![]).status_code(), StatusCode::OK); + } + + #[test] + fn failed_sequence_contains_failure_reason() { + let object = InMemDicomObject::from(StoreResponse { + failed_sequence: vec![failed()], + referenced_sequence: vec![], + }); + + let items = object + .element(tags::FAILED_SOP_SEQUENCE) + .unwrap() + .items() + .unwrap(); + let failure_reason = items[0] + .element(tags::FAILURE_REASON) + .unwrap() + .to_int::(); + assert_eq!(failure_reason.unwrap(), failure_reason::PROCESSING_FAILURE); + } +} diff --git a/src/backend/dimse/cstore/mod.rs b/src/backend/dimse/cstore/mod.rs index 9a6e2d8..fc31822 100644 --- a/src/backend/dimse/cstore/mod.rs +++ b/src/backend/dimse/cstore/mod.rs @@ -1,15 +1,17 @@ pub mod storescp; pub mod storescu; -use crate::backend::dimse::{DicomMessage, DATA_SET_EXISTS, DATA_SET_MISSING}; +use crate::backend::dimse::{DicomMessage, ReadError, DATA_SET_EXISTS, DATA_SET_MISSING}; use crate::types::{AE, UI, US}; use dicom::core::{DataElement, VR}; use dicom::dicom_value; use dicom::dictionary_std::tags; +use dicom::object::mem::InMemElement; use dicom::object::InMemDicomObject; // Magic numbers defined by the DICOM specification. pub const COMMAND_FIELD_COMPOSITE_STORE_REQUEST: US = 0x0001; +pub const STATUS_SUCCESS: US = 0x0000; /// C-STORE-RQ pub struct CompositeStoreRequest { @@ -55,6 +57,7 @@ pub struct CompositeStoreResponse { pub message_id: US, pub sop_class_uid: UI, pub sop_instance_uid: UI, + pub status: US, } impl From for DicomMessage { @@ -65,7 +68,7 @@ impl From for DicomMessage { DataElement::new(tags::COMMAND_FIELD, VR::US, dicom_value!(U16, [0x8001])), DataElement::new(tags::MESSAGE_ID_BEING_RESPONDED_TO, VR::US, dicom_value!(U16, [response.message_id])), DataElement::new(tags::COMMAND_DATA_SET_TYPE, VR::US, dicom_value!(U16, [DATA_SET_MISSING])), - DataElement::new(tags::STATUS, VR::US, dicom_value!(U16, [0x00000])), + DataElement::new(tags::STATUS, VR::US, dicom_value!(U16, [response.status])), DataElement::new(tags::AFFECTED_SOP_INSTANCE_UID, VR::UI, dicom_value!(Str, response.sop_instance_uid)) ]); @@ -76,3 +79,38 @@ impl From for DicomMessage { } } } + +/// The outcome of a C-STORE operation, as reported in the C-STORE-RSP of the peer. +#[derive(Debug)] +pub struct CompositeStoreOutcome { + /// Status (0000,0900) + pub status: US, + /// Error Comment (0000,0902), which the peer may send along a non-success status. + pub error_comment: Option, +} + +impl TryFrom<&DicomMessage> for CompositeStoreOutcome { + type Error = ReadError; + + fn try_from(message: &DicomMessage) -> Result { + let status = message + .command + .get(tags::STATUS) + .map(InMemElement::to_int::) + .and_then(Result::ok) + .ok_or(ReadError::MissingAttribute(tags::STATUS))?; + + let error_comment = message + .command + .get(tags::ERROR_COMMENT) + .map(InMemElement::to_str) + .and_then(Result::ok) + .map(|comment| comment.trim_end_matches([' ', '\0']).to_owned()) + .filter(|comment| !comment.is_empty()); + + Ok(Self { + status, + error_comment, + }) + } +} diff --git a/src/backend/dimse/cstore/storescp.rs b/src/backend/dimse/cstore/storescp.rs index ff9e642..d7656aa 100644 --- a/src/backend/dimse/cstore/storescp.rs +++ b/src/backend/dimse/cstore/storescp.rs @@ -3,7 +3,7 @@ use crate::backend::dimse::cmove::{ MediatorError, MoveMediator, MoveSubOperation, SubscriptionTopic, }; use crate::backend::dimse::cstore::{ - CompositeStoreResponse, COMMAND_FIELD_COMPOSITE_STORE_REQUEST, + CompositeStoreResponse, COMMAND_FIELD_COMPOSITE_STORE_REQUEST, STATUS_SUCCESS, }; use crate::backend::dimse::stgcmt::store::StorageCommitmentStore; use crate::backend::dimse::stgcmt::{handle_event_report, COMMAND_FIELD_N_EVENT_REPORT_REQUEST}; @@ -181,6 +181,7 @@ impl StoreServiceClassProvider { sop_instance_uid: UI::from(sop_instance_uid.clone()), sop_class_uid: UI::from(sop_class_uid.clone()), message_id, + status: STATUS_SUCCESS, }; let move_originator_id = message diff --git a/src/backend/dimse/cstore/storescu.rs b/src/backend/dimse/cstore/storescu.rs index 3224f63..8e22752 100644 --- a/src/backend/dimse/cstore/storescu.rs +++ b/src/backend/dimse/cstore/storescu.rs @@ -1,7 +1,8 @@ +use crate::api::stow::failure_reason; use crate::backend::dimse::association; -use crate::backend::dimse::cstore::CompositeStoreRequest; +use crate::backend::dimse::cstore::{CompositeStoreOutcome, CompositeStoreRequest}; use crate::backend::dimse::{ - next_message_id, DicomMessageReader, DicomMessageWriter, ReadError, WriteError, + next_message_id, DicomMessageReader, DicomMessageWriter, ReadError, StatusType, WriteError, }; use crate::types::{Priority, UI, US}; use association::pool::{AssociationPool, PoolError, PresentationParameter}; @@ -9,7 +10,7 @@ use association::AssociationError; use dicom::object::{FileDicomObject, InMemDicomObject}; use std::time::Duration; use thiserror::Error; -use tracing::trace; +use tracing::{trace, warn}; pub struct StoreServiceClassUser { pool: AssociationPool, @@ -31,6 +32,7 @@ impl StoreServiceClassUser { }) .await?; + let sop_instance_uid = file.meta().media_storage_sop_instance_uid().to_owned(); let request = CompositeStoreRequest { affected_sop_class_uid: file.meta().media_storage_sop_class_uid.clone(), affected_sop_instance_uid: file.meta().media_storage_sop_instance_uid.clone(), @@ -46,10 +48,30 @@ impl StoreServiceClassUser { trace!("Sent C-STORE-RQ"); let read = association.read_message(self.timeout).await; - association.discard_on_err(read)?; + let response = association.discard_on_err(read)?; trace!("Received C-STORE-RSP"); - Ok(()) + // A non-success status is a regular DIMSE response, so the association stays usable. + let outcome = CompositeStoreOutcome::try_from(&response)?; + match StatusType::try_from(outcome.status) { + Ok(StatusType::Success) => Ok(()), + // PS3.4 B.2.3: the instance was stored, but e.g. some elements were coerced. + Ok(StatusType::Warning) => { + warn!( + sop_instance_uid, + status = format!("{:#06X}", outcome.status), + error_comment = outcome.error_comment, + "C-STORE completed with a warning" + ); + Ok(()) + } + // Failure, but also statuses that are not valid for C-STORE (Pending, Cancel) + // or unknown to us (e.g. the 0xCxxx "Cannot understand" range). + _ => Err(StoreError::Status { + status: outcome.status, + error_comment: outcome.error_comment, + }), + } } } @@ -61,4 +83,27 @@ pub enum StoreError { Write(#[from] WriteError), #[error(transparent)] Association(#[from] PoolError), + #[error( + "C-STORE failed with status {status:#06X}: {}", + .error_comment.as_deref().unwrap_or("no error comment") + )] + Status { + status: US, + error_comment: Option, + }, +} + +impl StoreError { + /// The Failure Reason (0008,1197) to report to the STOW-RS client. + pub const fn failure_reason(&self) -> US { + match self { + // The C-STORE status codes are valid failure reasons (PS3.18 Annex I). + Self::Status { status, .. } => *status, + // Includes an association that was rejected, e.g. because the peer does not support + // the SOP class or transfer syntax. The reason for the rejection is not available. + Self::Read(_) | Self::Write(_) | Self::Association(_) => { + failure_reason::PROCESSING_FAILURE + } + } + } } diff --git a/src/backend/dimse/stow.rs b/src/backend/dimse/stow.rs index 38de105..cde23ba 100644 --- a/src/backend/dimse/stow.rs +++ b/src/backend/dimse/stow.rs @@ -1,4 +1,6 @@ -use crate::api::stow::{InstanceReference, StoreError, StoreRequest, StoreResponse, StowService}; +use crate::api::stow::{ + FailedInstance, InstanceReference, StoreError, StoreRequest, StoreResponse, StowService, +}; use crate::backend::dimse::association; use crate::backend::dimse::cstore::storescu::StoreServiceClassUser; use crate::types::UI; @@ -6,7 +8,7 @@ use association::pool::AssociationPool; use async_trait::async_trait; use futures::TryStreamExt; use std::time::Duration; -use tracing::info; +use tracing::{info, warn}; pub struct DimseStowService { storescu: StoreServiceClassUser, @@ -40,10 +42,13 @@ impl StowService for DimseStowService { }); } Err(err) => { - info!(sop_instance_uid, "Failed to store instance: {err}",); - failed_sequence.push(InstanceReference { - sop_class_uid, - sop_instance_uid, + warn!(sop_instance_uid, "Failed to store instance: {err}"); + failed_sequence.push(FailedInstance { + reference: InstanceReference { + sop_class_uid, + sop_instance_uid, + }, + failure_reason: err.failure_reason(), }); } } diff --git a/tests/stow.rs b/tests/stow.rs index 6c4fcfb..0ac20ad 100644 --- a/tests/stow.rs +++ b/tests/stow.rs @@ -105,12 +105,11 @@ async fn does_not_leak_semaphore_permits_if_association_is_rejected() -> anyhow: with_test_environment(config, async |client| { let start = Instant::now(); - let response = client + let result = client .store_instances() .with_instances(futures::stream::iter(instances)) .run() - .await - .context("STOW-RS request failed")?; + .await; let elapsed = start.elapsed(); assert!( @@ -118,14 +117,15 @@ async fn does_not_leak_semaphore_permits_if_association_is_rejected() -> anyhow: "STOW-RS took {elapsed:?}; expected fast failure" ); - let failed_sequence = response - .element(tags::FAILED_SOP_SEQUENCE) - .context("STOW-RS response is missing FailedSOPSequence")?; + // No instance was stored, so the request fails as a whole. assert!( - failed_sequence - .items() - .is_some_and(|items| items.len() == 3), - "All three rejected instances should appear in FailedSOPSequence" + matches!( + result, + Err(DicomWebError::HttpStatusFailure { + status_code: StatusCode::CONFLICT + }) + ), + "Expected 409 Conflict, got {result:?}" ); Ok(()) @@ -133,6 +133,62 @@ async fn does_not_leak_semaphore_permits_if_association_is_rejected() -> anyhow: .await } +#[tokio::test] +async fn returns_failure_reason_if_some_instances_fail() -> anyhow::Result<()> { + let config = " + server: + http: + port: 0 + dimse: + - aet: DICOM-RST + interface: 0.0.0.0 + port: 0 + aets: + - aet: ORTHANC + host: 127.0.0.1 + port: ${ORTHANC_PORT} + backend: DIMSE + "; + + let valid = open_file(dicom_test_files::path("pydicom/CT_small.dcm").unwrap()).unwrap(); + let mut invalid = valid.clone(); + // Set a fake SOPClassUID; Orthanc will reject the association for this instance + invalid.meta_mut().media_storage_sop_class_uid = String::from("1.2.3.4.5.6.7.8.9.10"); + + with_test_environment(config, async |client| { + let response = client + .store_instances() + .with_instances(futures::stream::iter([valid, invalid])) + .run() + .await + .context("STOW-RS request failed")?; + + let referenced_items = response + .element(tags::REFERENCED_SOP_SEQUENCE) + .context("STOW-RS response is missing ReferencedSOPSequence")? + .items() + .context("ReferencedSOPSequence is not a sequence")?; + assert_eq!(referenced_items.len(), 1); + + let failed_items = response + .element(tags::FAILED_SOP_SEQUENCE) + .context("STOW-RS response is missing FailedSOPSequence")? + .items() + .context("FailedSOPSequence is not a sequence")?; + assert_eq!(failed_items.len(), 1); + + let failure_reason = failed_items[0] + .element(tags::FAILURE_REASON) + .context("FailedSOPSequence item is missing FailureReason")? + .to_int::()?; + // Processing failure, as the reason for the rejected association is not known + assert_eq!(failure_reason, 0x0110); + + Ok(()) + }) + .await +} + // https://github.com/UMEssen/DICOM-RST/issues/55 #[tokio::test] async fn returns_413_if_max_upload_size_is_exceeded() -> anyhow::Result<()> { From dedd81525d65f13d91e9b2f34887aaf3a892ca8e Mon Sep 17 00:00:00 2001 From: Stephan Vedder Date: Tue, 10 Nov 2026 16:51:16 +0100 Subject: [PATCH 2/2] feat(stow): add transfer syntax fallbacks --- CHANGELOG.md | 9 ++ Cargo.lock | 94 +++++++++-- Cargo.toml | 2 +- Dockerfile | 4 + Dockerfile-s3 | 3 +- docs/topics/backend-dimse.md | 9 ++ docs/topics/configuration.md | 10 ++ src/api/stow/service.rs | 4 +- src/backend/dimse/association/client.rs | 13 +- src/backend/dimse/association/pool.rs | 47 ++++++ src/backend/dimse/cfind/findscu.rs | 3 + src/backend/dimse/cmove/movescu.rs | 1 + src/backend/dimse/cstore/storescu.rs | 133 ++++++++++++++-- src/backend/dimse/stgcmt/scu.rs | 1 + src/backend/dimse/stow.rs | 8 +- src/backend/mod.rs | 1 + src/config/mod.rs | 15 +- tests/common/mod.rs | 34 +++- tests/stow.rs | 197 +++++++++++++++++++++++- 19 files changed, 554 insertions(+), 34 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 14147bd..de74424 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -9,10 +9,19 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ### Added +- STOW-RS (DIMSE backend) additionally offers the uncompressed transfer syntaxes and decompresses instances if the PACS does not accept their transfer syntax (#83). Can be disabled with `stow-rs.uncompressed-fallback: false`. +- JPEG 2000 and JPEG-LS pixel data can now be decoded, e.g. when transcoding WADO-RS responses. Building from source requires a C++ compiler and CMake. +- STOW-RS responses include a `FailureReason` for each failed instance. + ### Changed +- STOW-RS responds with `202 Accepted` if some instances failed and with `409 Conflict` if all instances failed, instead of `200 OK`. +- Failures to store an instance are logged as warnings. + ### Fixed +- STOW-RS (DIMSE backend) no longer reports an instance as stored if the PACS returned a failure status in the C-STORE-RSP. + ## [0.3.4] - 2026-10-27 ### Added diff --git a/Cargo.lock b/Cargo.lock index 3ccfedf..e7f4e99 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -344,7 +344,7 @@ dependencies = [ "num-traits", "pastey", "rayon", - "thiserror", + "thiserror 2.0.21", "v_frame", "y4m", ] @@ -1026,7 +1026,7 @@ dependencies = [ "serde_derive", "serde_json", "serde_urlencoded", - "thiserror", + "thiserror 2.0.21", "time", "tokio", "tokio-stream", @@ -1184,6 +1184,25 @@ dependencies = [ "rand_core 0.10.1", ] +[[package]] +name = "charls" +version = "0.4.2" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "461ee0e8f15d5a4a14c6d79e7cf593a85d28e635e4ce7211c93ac649f03cfb5f" +dependencies = [ + "charls-sys", +] + +[[package]] +name = "charls-sys" +version = "2.4.5" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "28bd4cd0583158019ce942b42ec9b4200462199e65c76cd3be5df5322d3a159b" +dependencies = [ + "cmake", + "link-cplusplus", +] + [[package]] name = "chrono" version = "0.4.45" @@ -1481,7 +1500,7 @@ version = "1.1.0" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "71958f69fb2de856c5dd51ac9377e986ca1aa5919f931bf5395a807277f889a0" dependencies = [ - "thiserror", + "thiserror 2.0.21", ] [[package]] @@ -1699,7 +1718,7 @@ dependencies = [ "serde", "serde_json", "testcontainers", - "thiserror", + "thiserror 2.0.21", "tokio", "tower", "tower-http", @@ -1727,11 +1746,13 @@ source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "93fccc295971fd942d8b1b3573229ca2d48746afd51df6650acf353313f1626f" dependencies = [ "byteordered", + "charls", "dicom-core", "dicom-encoding", "flate2", "jpeg-decoder", "jpeg-encoder", + "jpeg2k", "jxl-oxide", "lazy_static", "tracing", @@ -2897,7 +2918,7 @@ dependencies = [ "jni-sys", "log", "simd_cesu8", - "thiserror", + "thiserror 2.0.21", "walkdir", "windows-link", ] @@ -2959,6 +2980,18 @@ version = "0.7.1" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "a0370574b86f7eca156b9f298392b5e69a23f8c86f3f865add60bbc2e79467a6" +[[package]] +name = "jpeg2k" +version = "0.10.1" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "be2982db6b9f254c9df44d89bcfdc758924d538f576d9649f5b10008150f2a60" +dependencies = [ + "anyhow", + "log", + "openjpeg-sys", + "thiserror 1.0.69", +] + [[package]] name = "js-sys" version = "0.3.106" @@ -3207,6 +3240,15 @@ dependencies = [ "cc", ] +[[package]] +name = "link-cplusplus" +version = "1.0.12" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "7f78c730aaa7d0b9336a299029ea49f9ee53b0ed06e9202e8cb7db9bae7b8c82" +dependencies = [ + "cc", +] + [[package]] name = "linux-raw-sys" version = "0.12.1" @@ -3743,6 +3785,16 @@ version = "1.21.4" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "9f7c3e4beb33f85d45ae3e3a1792185706c8e16d043238c593331cc7cd313b50" +[[package]] +name = "openjpeg-sys" +version = "1.0.12" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "92682cb92e7b01e2c020cf8ec12f3374558924bb24621df1112d8a7c93d419ef" +dependencies = [ + "cc", + "libc", +] + [[package]] name = "openssl" version = "0.10.81" @@ -4111,7 +4163,7 @@ dependencies = [ "rustc-hash", "rustls", "socket2", - "thiserror", + "thiserror 2.0.21", "tokio", "tracing", "web-time", @@ -4134,7 +4186,7 @@ dependencies = [ "rustls", "rustls-pki-types", "slab", - "thiserror", + "thiserror 2.0.21", "tinyvec", "tracing", "web-time", @@ -4260,7 +4312,7 @@ dependencies = [ "rand 0.9.5", "rand_chacha", "simd_helpers", - "thiserror", + "thiserror 2.0.21", "v_frame", "wasm-bindgen", ] @@ -4823,7 +4875,7 @@ dependencies = [ "rand 0.9.5", "serde", "serde_json", - "thiserror", + "thiserror 2.0.21", "time", "url", "uuid", @@ -5268,20 +5320,40 @@ dependencies = [ "serde", "serde_json", "serde_with", - "thiserror", + "thiserror 2.0.21", "tokio", "tokio-stream", "tokio-util", "url", ] +[[package]] +name = "thiserror" +version = "1.0.69" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "b6aaf5339b578ea85b50e080feb250a3e8ae8cfcdff9a461c9ec2904bc923f52" +dependencies = [ + "thiserror-impl 1.0.69", +] + [[package]] name = "thiserror" version = "2.0.21" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "09e52cb86a36cede5cb101bf8908837b3e4c6e5e59fe7fd85c23fb56200d189e" dependencies = [ - "thiserror-impl", + "thiserror-impl 2.0.21", +] + +[[package]] +name = "thiserror-impl" +version = "1.0.69" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "4fee6c4efc90059e10f81e6d42c60a18f76588c3d74cb83a0b242a2b6c7504c1" +dependencies = [ + "proc-macro2", + "quote", + "syn 2.0.119", ] [[package]] diff --git a/Cargo.toml b/Cargo.toml index b63977b..6087e48 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -29,7 +29,7 @@ mimalloc = ["dep:mimalloc"] # DICOM processing dicom = "0.10.0" dicom-json = "0.10.0" -dicom-pixeldata = { version = "0.10.0", features = ["image"] } +dicom-pixeldata = { version = "0.10.0", features = ["image", "openjpeg-sys", "charls"] } sentry = { version = "0.45.0", features = ["tracing"] } # Serialization diff --git a/Dockerfile b/Dockerfile index d9d9fb2..0043bc2 100644 --- a/Dockerfile +++ b/Dockerfile @@ -1,4 +1,8 @@ FROM lukemathwalker/cargo-chef:latest-rust-1.94.1 AS chef +# cmake builds CharLS (JPEG-LS codec) +RUN apt-get update \ + && apt-get install -y --no-install-recommends cmake \ + && rm -rf /var/lib/apt/lists/* WORKDIR /app FROM chef AS planner diff --git a/Dockerfile-s3 b/Dockerfile-s3 index 6640c16..48d5aaa 100644 --- a/Dockerfile-s3 +++ b/Dockerfile-s3 @@ -1,5 +1,6 @@ FROM rust:1.94.1-alpine3.23 AS builder -RUN apk add musl-dev openssl-dev openssl-libs-static +# g++, cmake and make build CharLS (JPEG-LS codec) +RUN apk add musl-dev openssl-dev openssl-libs-static g++ cmake make WORKDIR /app diff --git a/docs/topics/backend-dimse.md b/docs/topics/backend-dimse.md index 959b68e..53d0f9c 100644 --- a/docs/topics/backend-dimse.md +++ b/docs/topics/backend-dimse.md @@ -127,6 +127,15 @@ sequenceDiagram end ``` +Each instance is offered in its own transfer syntax and, unless +[`stow-rs.uncompressed-fallback`](configuration.md#dicomweb.stow-rs.uncompressed-fallback) is disabled, +additionally in the uncompressed transfer syntaxes. +If the PACS does not accept the original transfer syntax, the instance is decompressed before sending it. + +Instances that could not be stored are listed in the `FailedSOPSequence` of the response. +The `FailureReason` contains the status of the C-STORE-RSP, `0xC122` if the instance could not be +converted to an accepted transfer syntax, or `0x0110` for other errors (e.g. a rejected association). + ### Resources | Resource | URI Template | Support Status | diff --git a/docs/topics/configuration.md b/docs/topics/configuration.md index caa8d5a..0e53df1 100644 --- a/docs/topics/configuration.md +++ b/docs/topics/configuration.md @@ -205,6 +205,7 @@ aets: timeout: 3000 stow-rs: timeout: 3000 + uncompressed-fallback: true ``` Each AET (regardless of the backend) has additional settings specific to the DICOMweb endpoints. @@ -244,6 +245,15 @@ Each AET (regardless of the backend) has additional settings specific to the DIC This is the timeout for a single operation (e.g. receiving a DIMSE-C response primitive). If you want to set a timeout for the total execution time, use the server.http.request-timeout option instead. + + DIMSE-backend only: + Whether to also offer the uncompressed transfer syntaxes (Explicit and Implicit VR Little Endian) when storing an instance. + Instances are sent in their original transfer syntax whenever the PACS accepts it. + If the PACS only accepts the uncompressed transfer syntaxes, the instance is decompressed before sending it. + Supported compressions are JPEG, JPEG Lossless, JPEG-LS, JPEG 2000 and RLE. + If disabled, instances in a transfer syntax that the PACS does not accept fail to store. + Defaults to true. + ## S3 Backend Config diff --git a/src/api/stow/service.rs b/src/api/stow/service.rs index c3a79fe..3b7b508 100644 --- a/src/api/stow/service.rs +++ b/src/api/stow/service.rs @@ -34,12 +34,14 @@ pub struct FailedInstance { } /// Failure Reason values of the Store Instances Response Module. -/// +/// pub mod failure_reason { use crate::types::US; /// The origin server failed to store the instance for an unspecified reason. pub const PROCESSING_FAILURE: US = 0x0110; + /// The transfer syntax of the instance is not supported. + pub const TRANSFER_SYNTAX_NOT_SUPPORTED: US = 0xC122; } #[derive(Debug, Default)] diff --git a/src/backend/dimse/association/client.rs b/src/backend/dimse/association/client.rs index 8960674..e98d97f 100644 --- a/src/backend/dimse/association/client.rs +++ b/src/backend/dimse/association/client.rs @@ -24,6 +24,8 @@ pub struct ClientAssociationOptions { pub called_aet: String, pub abstract_syntax: String, pub transfer_syntaxes: Vec, + /// Proposed in a separate presentation context for the same abstract syntax, if not empty. + pub fallback_transfer_syntaxes: Vec, pub address: SocketAddr, } @@ -68,10 +70,17 @@ impl ClientAssociation { let called_aet = options.called_aet; let address = options.address; - let options = dicom::ul::ClientAssociationOptions::new() + let mut ul_options = dicom::ul::ClientAssociationOptions::new() .calling_ae_title(calling_aet.clone()) .called_ae_title(called_aet.clone()) - .with_presentation_context(options.abstract_syntax, options.transfer_syntaxes); + .with_presentation_context(options.abstract_syntax.clone(), options.transfer_syntaxes); + if !options.fallback_transfer_syntaxes.is_empty() { + ul_options = ul_options.with_presentation_context( + options.abstract_syntax, + options.fallback_transfer_syntaxes, + ); + } + let options = ul_options; let _handle = thread::Builder::new() .name(String::from("calling_aet")) diff --git a/src/backend/dimse/association/pool.rs b/src/backend/dimse/association/pool.rs index fa28424..428dd02 100644 --- a/src/backend/dimse/association/pool.rs +++ b/src/backend/dimse/association/pool.rs @@ -209,18 +209,27 @@ pub struct AssociationManager { pub called_aet: String, } +#[derive(Debug)] pub struct PresentationParameter { pub abstract_syntax_uid: UI, pub transfer_syntax_uids: Vec, + /// Transfer syntaxes proposed in a separate presentation context for the same abstract syntax. + /// The peer may accept either context, so the caller has to pick the one to use. + /// Empty if no fallback should be proposed. + pub fallback_transfer_syntax_uids: Vec, } impl PartialEq for PresentationParameter { fn eq(&self, other: &Self) -> bool { + // The fallback must match exactly: if the peer only accepted the fallback context of + // an association, it must not be reused for another (preferred) transfer syntax that + // the peer might support. self.abstract_syntax_uid == other.abstract_syntax_uid && self .transfer_syntax_uids .iter() .any(|ts| other.transfer_syntax_uids.contains(ts)) + && self.fallback_transfer_syntax_uids == other.fallback_transfer_syntax_uids } } @@ -238,6 +247,7 @@ impl Manager for AssociationManager { called_aet: self.called_aet.clone(), abstract_syntax: param.abstract_syntax_uid.clone(), transfer_syntaxes: param.transfer_syntax_uids.clone(), + fallback_transfer_syntaxes: param.fallback_transfer_syntax_uids.clone(), address: self.address, }; @@ -337,3 +347,40 @@ impl AssociationPools { self.0.keys() } } + +#[cfg(test)] +mod tests { + use super::*; + use dicom::dictionary_std::uids; + + fn parameter(transfer_syntax: &str, fallback: &[&str]) -> PresentationParameter { + PresentationParameter { + abstract_syntax_uid: UI::from(uids::CT_IMAGE_STORAGE), + transfer_syntax_uids: vec![UI::from(transfer_syntax)], + fallback_transfer_syntax_uids: fallback.iter().copied().map(UI::from).collect(), + } + } + + #[test] + fn presentation_parameters_with_different_fallbacks_are_not_equal() { + let fallback = [ + uids::EXPLICIT_VR_LITTLE_ENDIAN, + uids::IMPLICIT_VR_LITTLE_ENDIAN, + ]; + + assert_eq!( + parameter(uids::JPEG_LOSSLESS_SV1, &fallback), + parameter(uids::JPEG_LOSSLESS_SV1, &fallback) + ); + // An association that only got the fallback accepted must not be reused for + // another transfer syntax that the peer might accept. + assert_ne!( + parameter(uids::JPEG_LOSSLESS_SV1, &fallback), + parameter(uids::JPEG2000_LOSSLESS, &fallback) + ); + assert_ne!( + parameter(uids::JPEG_LOSSLESS_SV1, &fallback), + parameter(uids::JPEG_LOSSLESS_SV1, &[]) + ); + } +} diff --git a/src/backend/dimse/cfind/findscu.rs b/src/backend/dimse/cfind/findscu.rs index 31e2474..932b04d 100644 --- a/src/backend/dimse/cfind/findscu.rs +++ b/src/backend/dimse/cfind/findscu.rs @@ -56,16 +56,19 @@ impl FindServiceClassUser { uids::STUDY_ROOT_QUERY_RETRIEVE_INFORMATION_MODEL_FIND, ), transfer_syntax_uids, + fallback_transfer_syntax_uids: vec![], }, QueryInformationModel::Patient => PresentationParameter { abstract_syntax_uid: String::from( uids::PATIENT_ROOT_QUERY_RETRIEVE_INFORMATION_MODEL_FIND, ), transfer_syntax_uids, + fallback_transfer_syntax_uids: vec![], }, QueryInformationModel::Worklist => PresentationParameter { abstract_syntax_uid: String::from(uids::MODALITY_WORKLIST_INFORMATION_MODEL_FIND), transfer_syntax_uids, + fallback_transfer_syntax_uids: vec![], }, }; diff --git a/src/backend/dimse/cmove/movescu.rs b/src/backend/dimse/cmove/movescu.rs index dbac541..87ab722 100644 --- a/src/backend/dimse/cmove/movescu.rs +++ b/src/backend/dimse/cmove/movescu.rs @@ -32,6 +32,7 @@ impl MoveServiceClassUser { uids::STUDY_ROOT_QUERY_RETRIEVE_INFORMATION_MODEL_MOVE, ), transfer_syntax_uids: vec![UI::from(uids::IMPLICIT_VR_LITTLE_ENDIAN)], + fallback_transfer_syntax_uids: vec![], }) .await?; diff --git a/src/backend/dimse/cstore/storescu.rs b/src/backend/dimse/cstore/storescu.rs index 8e22752..4feb94f 100644 --- a/src/backend/dimse/cstore/storescu.rs +++ b/src/backend/dimse/cstore/storescu.rs @@ -2,36 +2,86 @@ use crate::api::stow::failure_reason; use crate::backend::dimse::association; use crate::backend::dimse::cstore::{CompositeStoreOutcome, CompositeStoreRequest}; use crate::backend::dimse::{ - next_message_id, DicomMessageReader, DicomMessageWriter, ReadError, StatusType, WriteError, + next_message_id, DicomMessageReader, DicomMessageWriter, NegotiationError, ReadError, + StatusType, WriteError, }; use crate::types::{Priority, UI, US}; use association::pool::{AssociationPool, PoolError, PresentationParameter}; -use association::AssociationError; +use association::{Association, AssociationError}; +use dicom::dictionary_std::uids; +use dicom::encoding::TransferSyntaxIndex; use dicom::object::{FileDicomObject, InMemDicomObject}; +use dicom::transfer_syntax::TransferSyntaxRegistry; +use dicom::ul::pdu::PresentationContextNegotiated; +use dicom_pixeldata::Transcode; use std::time::Duration; use thiserror::Error; -use tracing::{trace, warn}; +use tracing::{info, trace, warn}; + +/// Transfer syntaxes offered as a fallback, in order of preference. +const UNCOMPRESSED_TRANSFER_SYNTAXES: [&str; 2] = [ + uids::EXPLICIT_VR_LITTLE_ENDIAN, + uids::IMPLICIT_VR_LITTLE_ENDIAN, +]; pub struct StoreServiceClassUser { pool: AssociationPool, timeout: Duration, + uncompressed_fallback: bool, } impl StoreServiceClassUser { - pub const fn new(pool: AssociationPool, timeout: Duration) -> Self { - Self { pool, timeout } + pub const fn new( + pool: AssociationPool, + timeout: Duration, + uncompressed_fallback: bool, + ) -> Self { + Self { + pool, + timeout, + uncompressed_fallback, + } } #[allow(clippy::significant_drop_tightening)] - pub async fn store(&self, file: FileDicomObject) -> Result<(), StoreError> { + pub async fn store( + &self, + mut file: FileDicomObject, + ) -> Result<(), StoreError> { + let sop_class_uid = file.meta().media_storage_sop_class_uid().to_owned(); + let transfer_syntax_uid = file.meta().transfer_syntax().to_owned(); + let mut association = self .pool .get(PresentationParameter { - abstract_syntax_uid: UI::from(file.meta().media_storage_sop_class_uid().to_owned()), - transfer_syntax_uids: vec![UI::from(file.meta().transfer_syntax())], + abstract_syntax_uid: UI::from(sop_class_uid.clone()), + transfer_syntax_uids: vec![UI::from(transfer_syntax_uid.clone())], + fallback_transfer_syntax_uids: fallback_transfer_syntaxes( + &transfer_syntax_uid, + self.uncompressed_fallback, + ), }) .await?; + let (presentation_context_id, negotiated_transfer_syntax_uid) = + select_presentation_context( + association.presentation_contexts(), + &sop_class_uid, + &transfer_syntax_uid, + ) + .map(|pctx| (pctx.id, trim_uid(&pctx.transfer_syntax).to_owned())) + .ok_or_else(|| WriteError::from(NegotiationError::NoPresentationContext))?; + + if negotiated_transfer_syntax_uid != transfer_syntax_uid { + info!( + sop_instance_uid = file.meta().media_storage_sop_instance_uid(), + transfer_syntax = transfer_syntax_uid, + negotiated_transfer_syntax = negotiated_transfer_syntax_uid, + "Peer did not accept the transfer syntax of the instance, transcoding" + ); + file = transcode(file, negotiated_transfer_syntax_uid).await?; + } + let sop_instance_uid = file.meta().media_storage_sop_instance_uid().to_owned(); let request = CompositeStoreRequest { affected_sop_class_uid: file.meta().media_storage_sop_class_uid.clone(), @@ -43,7 +93,9 @@ impl StoreServiceClassUser { data_set: file.into_inner(), }; - let written = association.write_message(request, None, self.timeout).await; + let written = association + .write_message(request, Some(presentation_context_id), self.timeout) + .await; association.discard_on_err(written)?; trace!("Sent C-STORE-RQ"); @@ -91,19 +143,78 @@ pub enum StoreError { status: US, error_comment: Option, }, + #[error("Failed to transcode instance: {0}")] + Transcode(#[from] dicom_pixeldata::TranscodeError), + #[error("Transcoding task failed: {0}")] + TranscodeTask(#[from] tokio::task::JoinError), } impl StoreError { /// The Failure Reason (0008,1197) to report to the STOW-RS client. pub const fn failure_reason(&self) -> US { match self { - // The C-STORE status codes are valid failure reasons (PS3.18 Annex I). + // The C-STORE status codes are valid failure reasons (PS3.18 Table I.2-2). Self::Status { status, .. } => *status, + // The peer did not accept the transfer syntax of the instance and it could not be + // converted to one of the fallback transfer syntaxes. + Self::Transcode(_) => failure_reason::TRANSFER_SYNTAX_NOT_SUPPORTED, // Includes an association that was rejected, e.g. because the peer does not support // the SOP class or transfer syntax. The reason for the rejection is not available. - Self::Read(_) | Self::Write(_) | Self::Association(_) => { + Self::Read(_) | Self::Write(_) | Self::Association(_) | Self::TranscodeTask(_) => { failure_reason::PROCESSING_FAILURE } } } } + +/// The transfer syntaxes to propose in addition to the one of the instance. +fn fallback_transfer_syntaxes(transfer_syntax_uid: &str, enabled: bool) -> Vec { + if !enabled { + return vec![]; + } + UNCOMPRESSED_TRANSFER_SYNTAXES + .into_iter() + .filter(|ts| *ts != transfer_syntax_uid) + .map(UI::from) + .collect() +} + +/// Selects the accepted presentation context to send the instance with. +/// The transfer syntax of the instance is preferred, so it is only transcoded if necessary. +fn select_presentation_context<'a>( + presentation_contexts: &'a [PresentationContextNegotiated], + sop_class_uid: &str, + transfer_syntax_uid: &str, +) -> Option<&'a PresentationContextNegotiated> { + let mut candidates = presentation_contexts + .iter() + .filter(|pctx| trim_uid(&pctx.abstract_syntax) == sop_class_uid); + let preferred = candidates + .clone() + .find(|pctx| trim_uid(&pctx.transfer_syntax) == transfer_syntax_uid); + preferred.or_else(|| candidates.next()) +} + +fn trim_uid(uid: &str) -> &str { + uid.trim_end_matches(['\0', ' ']) +} + +/// Transcodes the instance on the blocking thread pool, as decoding pixel data is CPU-bound. +async fn transcode( + mut file: FileDicomObject, + transfer_syntax_uid: String, +) -> Result, StoreError> { + let transfer_syntax = TransferSyntaxRegistry + .get(&transfer_syntax_uid) + .ok_or_else(|| { + WriteError::from(NegotiationError::UnknownTransferSyntax(UI::from( + transfer_syntax_uid.as_str(), + ))) + })?; + + tokio::task::spawn_blocking(move || { + file.transcode(transfer_syntax)?; + Ok(file) + }) + .await? +} diff --git a/src/backend/dimse/stgcmt/scu.rs b/src/backend/dimse/stgcmt/scu.rs index 48749c1..77f6b70 100644 --- a/src/backend/dimse/stgcmt/scu.rs +++ b/src/backend/dimse/stgcmt/scu.rs @@ -59,6 +59,7 @@ impl StorageCommitmentServiceClassUser { .get(PresentationParameter { abstract_syntax_uid: UI::from(uids::STORAGE_COMMITMENT_PUSH_MODEL), transfer_syntax_uids: vec![UI::from(uids::IMPLICIT_VR_LITTLE_ENDIAN)], + fallback_transfer_syntax_uids: vec![], }) .await?; diff --git a/src/backend/dimse/stow.rs b/src/backend/dimse/stow.rs index cde23ba..a955539 100644 --- a/src/backend/dimse/stow.rs +++ b/src/backend/dimse/stow.rs @@ -15,8 +15,12 @@ pub struct DimseStowService { } impl DimseStowService { - pub const fn new(pool: AssociationPool, timeout: Duration) -> Self { - let storescu = StoreServiceClassUser::new(pool, timeout); + pub const fn new( + pool: AssociationPool, + timeout: Duration, + uncompressed_fallback: bool, + ) -> Self { + let storescu = StoreServiceClassUser::new(pool, timeout, uncompressed_fallback); Self { storescu } } } diff --git a/src/backend/mod.rs b/src/backend/mod.rs index ba7fbb0..0a82f2f 100644 --- a/src/backend/mod.rs +++ b/src/backend/mod.rs @@ -75,6 +75,7 @@ where stow: Some(Box::new(DimseStowService::new( pool.to_owned(), Duration::from_millis(ae_config.stow.timeout), + ae_config.stow.uncompressed_fallback, ))), mwl: Some(Box::new(DimseMwlService::new( pool.to_owned(), diff --git a/src/config/mod.rs b/src/config/mod.rs index 7454c1c..3bd6902 100644 --- a/src/config/mod.rs +++ b/src/config/mod.rs @@ -177,11 +177,24 @@ pub enum RetrieveMode { #[serde(rename_all = "kebab-case")] pub struct StowConfig { pub timeout: u64, + /// Additionally offer the uncompressed transfer syntaxes when storing an instance. + /// If the peer does not accept the transfer syntax of the instance, it is transcoded. + #[serde(default = "StowConfig::default_uncompressed_fallback")] + pub uncompressed_fallback: bool, +} + +impl StowConfig { + pub const fn default_uncompressed_fallback() -> bool { + true + } } impl Default for StowConfig { fn default() -> Self { - Self { timeout: 30_000 } + Self { + timeout: 30_000, + uncompressed_fallback: Self::default_uncompressed_fallback(), + } } } diff --git a/tests/common/mod.rs b/tests/common/mod.rs index 5def437..5231c03 100644 --- a/tests/common/mod.rs +++ b/tests/common/mod.rs @@ -9,11 +9,15 @@ use testcontainers::{ContainerAsync, GenericImage, ImageExt}; use tokio::io::{AsyncBufReadExt, BufReader, Lines}; use tokio::process::{Child, ChildStdout, Command}; -pub async fn spawn_orthanc() -> anyhow::Result> { +fn orthanc_image() -> GenericImage { GenericImage::new("jodogne/orthanc", "latest") .with_exposed_port(4242.tcp()) .with_exposed_port(8042.tcp()) .with_wait_for(WaitFor::message_on_stderr("Orthanc has started")) +} + +pub async fn spawn_orthanc() -> anyhow::Result> { + orthanc_image() // Allows the Orthanc container to dial back into a DICOM-RST process running on the // test host, e.g. to deliver a Storage Commitment N-EVENT-REPORT-RQ. .with_host("host.docker.internal", Host::HostGateway) @@ -22,6 +26,34 @@ pub async fn spawn_orthanc() -> anyhow::Result> { .context("failed to start Orthanc container") } +/// Starts Orthanc with the given configuration, which replaces the default `orthanc.json` +/// of the image. The REST API is reachable with the credentials orthanc:orthanc. +pub async fn spawn_orthanc_with_config( + config: &serde_json::Value, +) -> anyhow::Result> { + let mut config = config.clone(); + let defaults = serde_json::json!({ + "Name": "Orthanc inside Docker", + "StorageDirectory": "/var/lib/orthanc/db", + "RemoteAccessAllowed": true, + "AuthenticationEnabled": true, + "RegisteredUsers": { "orthanc": "orthanc" }, + }); + for (key, value) in defaults.as_object().expect("defaults are an object") { + config + .as_object_mut() + .context("Orthanc configuration must be an object")? + .entry(key) + .or_insert_with(|| value.clone()); + } + + orthanc_image() + .with_copy_to("/etc/orthanc/orthanc.json", serde_json::to_vec(&config)?) + .start() + .await + .context("failed to start Orthanc container") +} + pub async fn spawn_dicomrst(config: &str) -> anyhow::Result { let mut server = ServerProcess::spawn(config)?; let (http_port, dimse_port) = server.wait_until_started().await?; diff --git a/tests/stow.rs b/tests/stow.rs index 0ac20ad..fc344ff 100644 --- a/tests/stow.rs +++ b/tests/stow.rs @@ -3,10 +3,12 @@ mod common; use anyhow::Context; use axum::http::StatusCode; use common::*; -use dicom::dictionary_std::tags; -use dicom::object::open_file; -use dicom_web::DicomWebError; +use dicom::core::{DataElement, PrimitiveValue, VR}; +use dicom::dictionary_std::{tags, uids}; +use dicom::object::{open_file, FileDicomObject, InMemDicomObject}; +use dicom_web::{DicomWebClient, DicomWebError}; use std::time::{Duration, Instant}; +use testcontainers::core::IntoContainerPort; #[allow(clippy::redundant_closure_for_method_calls)] #[tokio::test] @@ -324,3 +326,192 @@ async fn reuses_associations_for_multiple_instances() -> anyhow::Result<()> { }) .await } + +/// Compressed instances whose pixel data DICOM-RST can decode, each with a unique SOP Instance UID. +fn compressed_instances() -> Vec> { + [ + "pydicom/JPEG-LL.dcm", + "pydicom/MR_small_RLE.dcm", + "pydicom/MR_small_jp2klossless.dcm", + "pydicom/MR_small_jpeg_ls_lossless.dcm", + ] + .map(|path| { + let mut instance = open_file(dicom_test_files::path(path).unwrap()).unwrap(); + let sop_instance_uid = format!("2.25.{}", uuid::Uuid::new_v4().as_u128()); + instance.put(DataElement::new( + tags::SOP_INSTANCE_UID, + VR::UI, + PrimitiveValue::from(sop_instance_uid.as_str()), + )); + instance.meta_mut().media_storage_sop_instance_uid = sop_instance_uid; + instance + }) + .into() +} + +/// Runs a STOW-RS request against an Orthanc with the given configuration. +/// Returns the response and the transfer syntax in which Orthanc stored each instance. +async fn store_in_configured_orthanc( + orthanc_config: &serde_json::Value, + stow_config: &str, + instances: Vec>, +) -> anyhow::Result<( + Result, + Vec<(String, Option)>, +)> { + let orthanc = spawn_orthanc_with_config(orthanc_config).await?; + let orthanc_dimse_port = orthanc.get_host_port_ipv4(4242.tcp()).await?; + let orthanc_http_port = orthanc.get_host_port_ipv4(8042.tcp()).await?; + + let config = format!( + " + server: + http: + port: 0 + dimse: + - aet: DICOM-RST + interface: 0.0.0.0 + port: 0 + aets: + - aet: ORTHANC + host: 127.0.0.1 + port: {orthanc_dimse_port} + backend: DIMSE + stow-rs: + timeout: 10000 + {stow_config} + " + ); + let server = spawn_dicomrst(&config).await?; + let client = DicomWebClient::with_single_url(&format!( + "http://localhost:{}/aets/ORTHANC", + server.http_port() + )); + + let sop_instance_uids: Vec = instances + .iter() + .map(|instance| instance.meta().media_storage_sop_instance_uid().to_owned()) + .collect(); + let response = client + .store_instances() + .with_instances(futures::stream::iter(instances)) + .run() + .await; + + let http = reqwest::Client::new(); + let orthanc_url = format!("http://localhost:{orthanc_http_port}"); + let mut stored_transfer_syntaxes = vec![]; + for sop_instance_uid in sop_instance_uids { + let matches: serde_json::Value = http + .post(format!("{orthanc_url}/tools/lookup")) + .basic_auth("orthanc", Some("orthanc")) + .body(sop_instance_uid.clone()) + .send() + .await? + .error_for_status()? + .json() + .await?; + let transfer_syntax = match matches[0]["ID"].as_str() { + Some(id) => Some( + http.get(format!( + "{orthanc_url}/instances/{id}/metadata/TransferSyntax" + )) + .basic_auth("orthanc", Some("orthanc")) + .send() + .await? + .error_for_status()? + .text() + .await?, + ), + None => None, + }; + stored_transfer_syntaxes.push((sop_instance_uid, transfer_syntax)); + } + + Ok((response, stored_transfer_syntaxes)) +} + +fn uncompressed_only_orthanc() -> serde_json::Value { + serde_json::json!({ + "AcceptedTransferSyntaxes": [uids::IMPLICIT_VR_LITTLE_ENDIAN, uids::EXPLICIT_VR_LITTLE_ENDIAN], + }) +} + +// https://github.com/UMEssen/DICOM-RST/issues/83 +#[tokio::test] +async fn transcodes_instances_if_transfer_syntax_is_not_accepted() -> anyhow::Result<()> { + let (response, stored) = + store_in_configured_orthanc(&uncompressed_only_orthanc(), "", compressed_instances()) + .await?; + + let response = response.context("STOW-RS request failed")?; + let failed_items = response + .element(tags::FAILED_SOP_SEQUENCE)? + .items() + .context("FailedSOPSequence is not a sequence")?; + assert!( + failed_items.is_empty(), + "Unexpected failures: {failed_items:?}" + ); + + for (sop_instance_uid, transfer_syntax) in stored { + assert_eq!( + transfer_syntax.as_deref(), + Some(uids::EXPLICIT_VR_LITTLE_ENDIAN), + "Instance {sop_instance_uid} was not stored uncompressed" + ); + } + + Ok(()) +} + +#[tokio::test] +async fn fails_if_transfer_syntax_is_not_accepted_and_fallback_is_disabled() -> anyhow::Result<()> { + let (response, stored) = store_in_configured_orthanc( + &uncompressed_only_orthanc(), + "uncompressed-fallback: false", + compressed_instances(), + ) + .await?; + + assert!( + matches!( + response, + Err(DicomWebError::HttpStatusFailure { + status_code: StatusCode::CONFLICT + }) + ), + "Expected 409 Conflict, got {response:?}" + ); + assert!(stored + .iter() + .all(|(_, transfer_syntax)| transfer_syntax.is_none())); + + Ok(()) +} + +#[tokio::test] +async fn keeps_transfer_syntax_if_accepted() -> anyhow::Result<()> { + let instances = compressed_instances(); + let original_transfer_syntaxes: Vec = instances + .iter() + .map(|instance| instance.meta().transfer_syntax().to_owned()) + .collect(); + + // Orthanc accepts all transfer syntaxes by default + let (response, stored) = + store_in_configured_orthanc(&serde_json::json!({}), "", instances).await?; + response.context("STOW-RS request failed")?; + + for ((sop_instance_uid, transfer_syntax), original) in + stored.into_iter().zip(original_transfer_syntaxes) + { + assert_eq!( + transfer_syntax.as_deref(), + Some(original.as_str()), + "Instance {sop_instance_uid} was transcoded unnecessarily" + ); + } + + Ok(()) +}