Repository navigation
feat(stgcmt): Implement DICOMweb storage commitment - #63
Conversation
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
|
@nickamzol Can you give this a review please? We need this feature in production soon |
| struct TestInstance { | ||
| study_instance_uid: String, | ||
| series_instance_uid: String, | ||
| sop_class_uid: String, | ||
| sop_instance_uid: String, | ||
| } |
There was a problem hiding this comment.
Note to self: We should update our clippy config because these kind of pedantic lints are not useful and rather annoying.
| } | ||
|
|
||
| #[tokio::test] | ||
| async fn can_commit_storage_and_check_result() -> anyhow::Result<()> { |
nickamzol
added
backend:dimse
nickamzol
left a comment
nickamzol
left a comment
There was a problem hiding this comment.
Great work!
Just a few robustness fixes, all small and local, nothing structural:
| struct TestInstance { | ||
| study_instance_uid: String, | ||
| series_instance_uid: String, | ||
| sop_class_uid: String, | ||
| sop_instance_uid: String, | ||
| } |
There was a problem hiding this comment.
Note to self: We should update our clippy config because these kind of pedantic lints are not useful and rather annoying.
| if let Err(err) = result { | ||
| // The N-ACTION-RQ itself failed synchronously, so no N-EVENT-REPORT-RQ will ever | ||
| // arrive for this Transaction UID - allow the client to retry. | ||
| self.store.remove(&request.transaction_uid); |
There was a problem hiding this comment.
This removes the entry on every error, but for CommitError::Read errors the N-ACTION-RQ already went out: write_message succeeded and only read_message failed (e.g. a timeout). The PACS may already have the request and will then send the N-EVENT-REPORT-RQ on a new association.
If that report arrives before our timeout, complete() stores the result and then this line deletes it. The client gets a 404 even though the commit went through.
| .context("Missing tag COMMAND_FIELD (0000,0100)")?; | ||
|
|
||
| if command_field == COMMAND_FIELD_N_EVENT_REPORT_REQUEST { | ||
| handle_event_report(&association, message, &inner.stgcmt_store).await?; |
There was a problem hiding this comment.
This branch is taken when the PACS delivers the report on a new association.
If I understand correctly, a conformant SCP negotiates the SCP role (PS3.4 J.3.3.1.2 Note 1)), but our Store SCP is built with dicom-ul's DefaultNegotiation which omits role selection.
If the report comes back on the same association that carried the N-ACTION, this isn't a problem: DICOM-RST is the association requestor and the PACS is the acceptor, so under the implicit default roles the PACS is already the SCP. But delivery on a new association reverses the requestor/acceptor roles: now the PACS opens the association and becomes the requestor.
This works with Orthanc, which sends the report regardless, but we shouldn't count on every PACS being that forgiving.
There was a problem hiding this comment.
Do you know any open PACS i can test this against (as you said Orthanc doesn't have this)
There was a problem hiding this comment.
That was more of a theoretical consideration based on the specification. I'm not aware of any PACS that does this either, but let's not block on this. It doesn't hurt to include the role negotiation.
I'll publish this as a pre-release, so we can gather some experience with various implementations.
Fixes #29
Standard: https://dicom.nema.org/medical/dicom/current/output/chtml/part18/chapter_13.html
Can you maybe take an early look @nickamzol ?