Replace --verbose with --log-ndjson and fix clippy

FNNDSC · Aug 1, 2023 · 737dac4 · 737dac4
1 parent bc57932
commit 737dac4
Show file tree

Hide file tree

Showing 8 changed files with 84 additions and 28 deletions.
diff --git a/Cargo.lock b/Cargo.lock
diff --git a/Cargo.toml b/Cargo.toml
@@ -1,7 +1,7 @@
 [package]
 name = "rx-repack"
 description = "Rust re-write of px-repack"
-version = "0.1.0"
+version = "0.2.0"
 edition = "2021"
 
 # See more keys and their definitions at https://doc.rust-lang.org/cargo/reference/manifest.html
@@ -14,7 +14,7 @@ regex = "1.9.1"
 serde = { version = "1.0.171", features = ["derive"] }
 serde_json = "1.0.103"
 fs-err = "2.9.0"
-camino = "1.1.6"
+camino = { version = "1.1.6", features = ["serde1"] }
 hashbrown = { version = "0.14.0", features = ["serde"] }
 thiserror = "1.0.43"
 

diff --git a/Dockerfile b/Dockerfile
@@ -22,4 +22,4 @@ COPY --from=builder /app/target/release/rx-repack /usr/local/bin/rx-repack
 
 EXPOSE 11113
 ENTRYPOINT ["/docker-entrypoint.sh"]
-CMD ["storescp", "--fork", "-od", "/tmp/storescp", "-pm", "-sp", "-xcr", "rx-repack --xcrdir '#p' --xcrfile '#f' --verbosity 0 --logdir /home/dicom/log --datadir /home/dicom/data --cleanup --verbose", "11113"]
+CMD ["storescp", "--fork", "-od", "/tmp/storescp", "-pm", "-sp", "-xcr", "rx-repack --xcrdir '#p' --xcrfile '#f' --verbosity 0 --logdir /home/dicom/log --datadir /home/dicom/data --cleanup --log-ndjson", "11113"]
diff --git a/src/log_models.rs b/src/log_models.rs
@@ -30,9 +30,9 @@ impl<'a> PatientData<'a> {
         e: &'a PypxPathElements,
     ) -> Result<Self, DicomTagReadError> {
         {
-            let name = tt(&d, tags::PATIENT_NAME)?;
-            let age = tt(&d, tags::PATIENT_AGE)?;
-            let sex = tt(&d, tags::PATIENT_SEX)?;
+            let name = tt(d, tags::PATIENT_NAME)?;
+            let age = tt(d, tags::PATIENT_AGE)?;
+            let sex = tt(d, tags::PATIENT_SEX)?;
             let patient = Self {
                 PatientID: Cow::Borrowed(e.PatientID),
                 PatientName: Cow::Borrowed(name),
@@ -65,7 +65,7 @@ impl<'a> StudyDataMeta<'a> {
             StudyDescription: e.StudyDescription,
             StudyDate: e.StudyDate,
             StudyInstanceUID: &e.StudyInstanceUID,
-            PerformedStationAETitle: tt(&d, tags::PERFORMED_STATION_AE_TITLE).unwrap_or(""),
+            PerformedStationAETitle: tt(d, tags::PERFORMED_STATION_AE_TITLE).unwrap_or(""),
         };
         Ok(data)
     }
@@ -198,7 +198,7 @@ impl<'a> SeriesDataMeta<'a> {
             StudyInstanceUID: &e.StudyInstanceUID,
             SeriesInstanceUID: &e.SeriesInstanceUID,
             SeriesDescription: e.SeriesDescription,
-            SeriesNumber: e.SeriesNumber.clone(),
+            SeriesNumber: e.SeriesNumber,
             SeriesDate: e.StudyDate,
             Modality: tt(d, tags::MODALITY)?,
         };

diff --git a/src/log_write.rs b/src/log_write.rs
@@ -31,7 +31,7 @@ pub(crate) fn write_logs(
         load_json_carelessly(&patient_data_fname).unwrap_or_else(|| HashMap::with_capacity(1));
     patient_data
         .entry_ref(elements.PatientID)
-        .or_insert_with(|| PatientData::new(&dcm, &elements).unwrap()) // FIXME
+        .or_insert_with(|| PatientData::new(dcm, elements).unwrap())
         .StudyList
         .insert(elements.StudyInstanceUID.to_string());
     write_json(patient_data, patient_data_fname)?;
@@ -46,7 +46,7 @@ pub(crate) fn write_logs(
         let study_series_meta = StudyDataSeriesMeta::new(
             elements.SeriesInstanceUID.to_string(),
             unpack.dir.to_string(),
-            &dcm,
+            dcm,
         )?;
         let data: HashMap<_, _> = [(&elements.StudyInstanceUID, study_series_meta)].into();
         write_json(data, study_series_meta_fname)?;

diff --git a/src/main.rs b/src/main.rs
@@ -1,3 +1,6 @@
+mod ndjson_log;
+
+use crate::ndjson_log::json_message;
 use camino::Utf8PathBuf;
 use clap::Parser;
 use rx_repack::repack;
@@ -42,8 +45,9 @@ struct Cli {
     #[clap(long)]
     verbosity: Option<u8>,
 
+    /// Write to stdout the outcome JSON
     #[clap(short, long, default_value_t = false)]
-    verbose: bool,
+    log_ndjson: bool,
 }
 
 fn main() -> anyhow::Result<()> {
@@ -54,10 +58,14 @@ fn main() -> anyhow::Result<()> {
         &args.datadir,
         args.logdir.as_ref().map(|p| p.as_path()),
         args.cleanup,
-    )?;
+    );
 
-    if args.verbose {
-        eprintln!("{} -> {}", dicom_file, &dst);
+    if args.log_ndjson {
+        // 12-factor app recommends writing to stdout (not stderr)
+        // https://12factor.net/logs
+        // NDJson is a best practice for logging:
+        // http://ndjson.org/
+        println!("{}", json_message(&dicom_file, dst)?);
     }
 
     anyhow::Ok(())

diff --git a/src/ndjson_log.rs b/src/ndjson_log.rs
@@ -0,0 +1,45 @@
+use camino::{Utf8Path, Utf8PathBuf};
+use serde::Serialize;
+use std::os::unix::fs::MetadataExt;
+
+/// Produce a JSON string which describes the outcome of `rx-repack`.
+pub(crate) fn json_message(
+    src: &Utf8Path,
+    result: anyhow::Result<Utf8PathBuf>,
+) -> anyhow::Result<String> {
+    let msg = Message::new(src, result);
+    serde_json::to_string(&msg).map_err(anyhow::Error::from)
+}
+
+#[derive(Serialize, Debug)]
+struct Message<'a> {
+    src: &'a Utf8Path,
+    dst: Option<Utf8PathBuf>,
+    #[serde(skip_serializing_if = "Option::is_none")]
+    size: Option<u64>,
+    #[serde(skip_serializing_if = "Option::is_none")]
+    err: Option<String>,
+}
+
+impl<'a> Message<'a> {
+    fn new(src: &'a Utf8Path, result: anyhow::Result<Utf8PathBuf>) -> Self {
+        result
+            .and_then(|dst| {
+                fs_err::metadata(&dst)
+                    .map(|m| (dst, m.size()))
+                    .map_err(anyhow::Error::from)
+            })
+            .map(|(dst, size)| Self {
+                src,
+                dst: Some(dst),
+                size: Some(size),
+                err: None,
+            })
+            .unwrap_or_else(|e| Self {
+                src,
+                dst: None,
+                size: None,
+                err: Some(e.to_string()),
+            })
+    }
+}
diff --git a/src/pack_path.rs b/src/pack_path.rs
@@ -34,7 +34,7 @@ impl PypxPath {
             "{:0>4}-{}.dcm",
             dcm.InstanceNumber, dcm.SOPInstanceUID
         ));
-        let pack_dir = data_dir.join(&pack_dir_rel);
+        let pack_dir = data_dir.join(pack_dir_rel);
         let path = pack_dir.join(&fname);
         Self {
             fname,
@@ -71,18 +71,18 @@ impl<'a> TryFrom<&'a DefaultDicomObject> for PypxPathElements<'a> {
         // - dcm.element(...)?.string() produces a reference to the data w/o cloning nor parsing
         // - dcm.element is more efficient than dcm.element_by_name, sinnce the latter does a map lookup
         let data = Self {
-            InstanceNumber: tt(&dcm, tags::INSTANCE_NUMBER)?,
-            SOPInstanceUID: tt(&dcm, tags::SOP_INSTANCE_UID)?,
-            PatientID: tt(&dcm, tags::PATIENT_ID)?,
-            PatientName: tt(&dcm, tags::PATIENT_NAME)?,
-            PatientBirthDate: tt(&dcm, tags::PATIENT_BIRTH_DATE)?,
-            StudyDescription: tt(&dcm, tags::STUDY_DESCRIPTION)?,
-            AccessionNumnber: tt(&dcm, tags::ACCESSION_NUMBER)?,
-            StudyDate: tt(&dcm, tags::STUDY_DATE)?,
-            SeriesNumber: tt(&dcm, tags::SERIES_NUMBER)?.parse()?,
-            SeriesDescription: tt(&dcm, tags::SERIES_DESCRIPTION)?,
-            StudyInstanceUID: tts(&dcm, tags::STUDY_INSTANCE_UID)?,
-            SeriesInstanceUID: tts(&dcm, tags::SERIES_INSTANCE_UID)?,
+            InstanceNumber: tt(dcm, tags::INSTANCE_NUMBER)?,
+            SOPInstanceUID: tt(dcm, tags::SOP_INSTANCE_UID)?,
+            PatientID: tt(dcm, tags::PATIENT_ID)?,
+            PatientName: tt(dcm, tags::PATIENT_NAME)?,
+            PatientBirthDate: tt(dcm, tags::PATIENT_BIRTH_DATE)?,
+            StudyDescription: tt(dcm, tags::STUDY_DESCRIPTION)?,
+            AccessionNumnber: tt(dcm, tags::ACCESSION_NUMBER)?,
+            StudyDate: tt(dcm, tags::STUDY_DATE)?,
+            SeriesNumber: tt(dcm, tags::SERIES_NUMBER)?.parse()?,
+            SeriesDescription: tt(dcm, tags::SERIES_DESCRIPTION)?,
+            StudyInstanceUID: tts(dcm, tags::STUDY_INSTANCE_UID)?,
+            SeriesInstanceUID: tts(dcm, tags::SERIES_INSTANCE_UID)?,
         };
         Ok(data)
     }