Better support for binary inputs

Turns out that nushell tries to auto-convert binary data to strings.
This can be avoided for scalar values by not using SimplePluginCommand
and switching to PluginCommand that has access to the input binary
stream.

Unfortunately nushell doesn't support lists of streams and it converts
them to list of strings.

Author: realcundo

README.md

@@ -2,18 +2,18 @@
 
 *Note that this plugin works with nu 0.105. If you want to use nu 0.60, use version 0.1.8 of this plugin.*
 
-A [nushell](https://www.nushell.sh/) plugin to parse [Dicom](https://en.wikipedia.org/wiki/DICOM) objects.
+A [nushell](https://www.nushell.sh/) plugin to parse [DICOM](https://en.wikipedia.org/wiki/DICOM) objects.
 
 This plugin is in the early stage of the development. It is usable but it might not be able to cope
-with all Dicom objects. One notable limitation is that all Dicom objects are expected to have a preamble.
+with all DICOM objects. One notable limitation is that all DICOM objects are expected to have a preamble.
 
 I'm still trying to figure out what is the most useful way of using this plugin. Please feel free to try it out,
 send feedback in [Discussions](https://github.com/realcundo/nu_plugin_dcm/discussions) or report problems in [Issues](https://github.com/realcundo/nu_plugin_dcm/issues).
 
 ## Usage
 `dcm` plugin reads its input from single values or from specific columns:
-- `dcm`: expects a string/filename or binary Dicom data
-- `dcm $column_name`: reads a string/filename or binary Dicom data from `$column`. This is
+- `dcm`: expects a string/filename or binary DICOM data
+- `dcm $column_name`: reads a string/filename or binary DICOM data from `$column`. This is
   equivalent to `get $column | dcm`.
 
 ## Error handling
@@ -24,28 +24,36 @@ send feedback in [Discussions](https://github.com/realcundo/nu_plugin_dcm/discus
 
 ## Known Limitations
 
-- Dicom objects without a preamble and DCIM header will fail to load.
-- PixelData is always skipped. For now I'm considering this to be a feature that speeds up Dicom parsing.
+- DICOM objects without a preamble and DCIM header will fail to load.
+- PixelData is always skipped. For now I'm considering this to be a feature that speeds up DICOM parsing.
+- `dcm` can process binary data. You can pass it directly to `dcm` as `open --raw file.dcm | dcm`. However, when passing a list of binary streams,
+  `nushell` will try to convert it to a list of strings. To work around this, use `into binary`, e.g.:
+  ```sh
+  [(open --raw file1.dcm | into binary), (open --raw file2.dcm | into binary)] | dcm
+  ```
+
+  Without `into binary`, `dcm` would see a list of strings, assuming it's a list of filenames.
+
 
 
 ## Examples
 
-### Output Dicom file as a table
+### Output DICOM file as a table
 ```sh
-echo file.dcm | dcm                # uses filename/string to specify which file to open
-open file.dcm | dcm                # pass binary data to `dcm`
-ls file.dcm | dcm name             # use `name` column as the filename
-echo file.dcm | wrap foo | dcm foo # use `foo` column as the filename
-open file.dcm | wrap foo | dcm foo # use `foo` column as binary data
+echo file.dcm | dcm                   # uses filename/string to specify which file to open
+open --raw file.dcm | dcm             # pass binary data to `dcm`
+ls file.dcm | dcm name                # use `name` column as the filename (equivalent of `ls file.dcm | select name | dcm`)
+echo file.dcm | wrap foo | dcm foo    # use `foo` column as the filename
+open -r file.dcm | wrap foo | dcm foo # use `foo` column as binary data
 ```
 
-### Dump Dicom file as a JSON/YAML document
+### Dump DICOM file as a JSON/YAML document
 ```sh
-open file.dcm | dcm | to json --indent 2
-open file.dcm | dcm | to yaml
+open -r file.dcm | dcm | to json --indent 2
+open -r file.dcm | dcm | to yaml
 ```
 
-### Dump all Dicom files in the current directory to a JSON/YAML document
+### Dump all DICOM files in the current directory to a JSON/YAML document
 ```sh
 ls *.dcm | dcm name | to json --indent 2
 ls *.dcm | dcm name | to yaml
@@ -78,7 +86,7 @@ would fail since selected columns are missing. Another option would be using `de
 for missing columns.)
 
 
-### For each file in all subdirectories, show filename, file size, SHA256 hash of the file, SOP Instance UID and a Dicom parsing error, if any
+### For each file in all subdirectories, show filename, file size, SHA256 hash of the file, SOP Instance UID and a DICOM parsing error, if any
 Use `par-each` to process files in parallel:
 ```sh
 ls **/* | where type == file |

src/plugin.rs

@@ -8,10 +8,11 @@ use crate::dcm;
 use dicom::object::DefaultDicomObject;
 use dicom::object::StandardDataDictionary;
 use indexmap::IndexMap;
-use nu_plugin::{EngineInterface, Plugin, SimplePluginCommand};
+use nu_plugin::{EngineInterface, Plugin, PluginCommand};
 use nu_protocol::ast::CellPath;
 use nu_protocol::{
-    Category, Example, LabeledError, Record, ShellError, Signature, Span, SyntaxShape, Type, Value,
+    Category, Example, LabeledError, PipelineData, Record, ShellError, Signature, Span,
+    SyntaxShape, Type, Value,
 };
 
 #[derive(Default)]
@@ -32,7 +33,7 @@ impl Plugin for DcmPlugin {
 #[derive(Default)]
 pub struct DcmPluginCommand;
 
-impl SimplePluginCommand for DcmPluginCommand {
+impl PluginCommand for DcmPluginCommand {
     type Plugin = DcmPlugin;
 
     fn name(&self) -> &str {
@@ -134,10 +135,25 @@ impl SimplePluginCommand for DcmPluginCommand {
         plugin: &DcmPlugin,
         engine: &EngineInterface,
         call: &nu_plugin::EvaluatedCall,
-        input: &Value,
-    ) -> Result<Value, LabeledError> {
+        input: PipelineData,
+    ) -> Result<PipelineData, LabeledError> {
         let current_dir = engine.get_current_dir().map(PathBuf::from);
 
+        let span = input.span().unwrap_or(call.head);
+        let input_metadata = input.metadata();
+
+        // Convert input PipelineData to Value. The default behaviour for BinaryStream is to detect the type of the stream and if unknown, try to read it as a UTF-8 string.
+        // This behaviour is wrong for us, since we want to read any binary input stream as binary data.
+        // Unfortunately, a list of `[(`open file.dcm`), ...]` gets converted to a list of strings by nushell before we can access it.
+        let input = if let PipelineData::ByteStream(byte_stream, ..) = input {
+            // TODO eventually support streaming?
+            let bytes = byte_stream.into_bytes()?;
+            Value::binary(bytes, span)
+        } else {
+            // convert PipelineData to Value the usual way
+            input.into_value(span)?
+        };
+
         // parse args, nu makes sure the num of args and type is correct
         let source_column = call.nth(0);
         let source_column = if let Some(Value::CellPath { val, .. }) = source_column {
@@ -154,13 +170,18 @@ impl SimplePluginCommand for DcmPluginCommand {
         };
 
         // run
-        self.run_filter(
+        let output = self.run_filter(
             plugin,
             current_dir.as_deref(),
-            input,
+            &input,
             source_column,
             error_column,
-        )
+        )?;
+
+        // Forward DataSource metadata from input to output, but clear any content type. This keeps the source.
+        let output_metadata = input_metadata.map(|m| m.with_content_type(None));
+
+        Ok(PipelineData::Value(output, output_metadata))
     }
 }
 
@@ -223,13 +244,21 @@ impl DcmPluginCommand {
                 val, internal_span, ..
             } => {
                 // make absolute if needed
-                let val = resolve_path(val, current_dir, value.span())?;
+                let file = resolve_path(val, current_dir, value.span())?;
+
+                // TODO add some heuristics to determine if the input string is filename or DICOM binary data connverted to utf-8 by nu?
+                // (see `ByteStream::into_value()` which does the conversion to string.)
+
+                let obj = read_dcm_file(&file).map_err(|e| {
+                    // Report a better error if the input string looks like DICOM binary data with preamble.
+                    // TODO this is messy. In fact the whole error reporting is messy.
+                    let text = if val.get(128..132) == Some("DICM") {
+                        "Input string looks like DICOM binary data. Either pass binary data, or a filename.".to_string()
+                    } else {
+                        format!("{} [file {}]", e, file.to_string_lossy())
+                    };
 
-                let obj = read_dcm_file(&val).map_err(|e| {
-                    LabeledError::new("`dcm` expects valid DICOM binary data").with_label(
-                        format!("{} [file {}]", e, val.to_string_lossy()),
-                        *internal_span,
-                    )
+                    LabeledError::new("`dcm` expects valid DICOM binary data").with_label( text, *internal_span)
                 })?;
 
                 self.process_dicom_object(plugin, internal_span, obj, error_column)

tests/assets/file.dcm

@@ -1,4 +1,4 @@
-use nu_plugin_dcm::plugin::DcmPlugin;
+use nu_plugin_dcm::plugin::{DcmPlugin, DcmPluginCommand};
 use nu_plugin_test_support::PluginTest;
 use nu_protocol::{record, IntoPipelineData, Span, Value};
 use std::path::PathBuf;
@@ -13,11 +13,14 @@ macro_rules! assert_dicom_field {
     };
 }
 
-fn get_asset_path(filename: &str) -> PathBuf {
+fn get_asset_base_path() -> PathBuf {
     PathBuf::from(env!("CARGO_MANIFEST_DIR"))
         .join("tests")
         .join("assets")
-        .join(filename)
+}
+
+fn get_asset_path(filename: &str) -> PathBuf {
+    get_asset_base_path().join(filename)
 }
 
 fn create_binary_value(filename: &str) -> Result<Value, Box<dyn std::error::Error>> {
@@ -38,6 +41,82 @@ fn create_file_record_value<S: AsRef<str>>(filename: S) -> Value {
     )
 }
 
+#[test]
+#[ignore]
+fn test_examples() -> Result<(), nu_protocol::ShellError> {
+    let mut plugin_test = PluginTest::new("dcm", DcmPlugin::default().into())?;
+
+    plugin_test.add_decl(Box::new(nu_command::Open))?;
+    plugin_test.add_decl(Box::new(nu_command::Ls))?;
+
+    plugin_test.engine_state_mut().add_env_var(
+        "PWD".to_string(),
+        Value::string(get_asset_base_path().to_string_lossy(), Span::test_data()),
+    );
+
+    plugin_test.test_command_examples(&DcmPluginCommand)
+}
+
+#[test]
+fn test_command_scalar_open() -> Result<(), nu_protocol::ShellError> {
+    let mut plugin_test = PluginTest::new("dcm", DcmPlugin::default().into())?;
+
+    plugin_test.add_decl(Box::new(nu_command::Open))?;
+
+    plugin_test.engine_state_mut().add_env_var(
+        "PWD".to_string(),
+        Value::string(get_asset_base_path().to_string_lossy(), Span::test_data()),
+    );
+
+    let result = plugin_test.eval("open --raw file.dcm | dcm")?;
+    let result = result.into_value(Span::test_data())?;
+
+    // TODO actually test the result
+    assert!(result.as_record().is_ok());
+    Ok(())
+}
+
+#[test]
+fn test_command_vector_open() -> Result<(), nu_protocol::ShellError> {
+    let mut plugin_test = PluginTest::new("dcm", DcmPlugin::default().into())?;
+
+    plugin_test.add_decl(Box::new(nu_command::Open))?;
+    plugin_test.add_decl(Box::new(nu_command::IntoBinary))?;
+
+    plugin_test.engine_state_mut().add_env_var(
+        "PWD".to_string(),
+        Value::string(get_asset_base_path().to_string_lossy(), Span::test_data()),
+    );
+
+    let result = plugin_test
+        .eval("[(open --raw file.dcm | into binary), (open --raw file.dcm | into binary)] | dcm")?;
+
+    let result = result.into_value(Span::test_data())?;
+
+    // TODO actually test the result
+    assert!(result.as_list().is_ok());
+    Ok(())
+}
+
+#[test]
+fn test_command_ls() -> Result<(), nu_protocol::ShellError> {
+    let mut plugin_test = PluginTest::new("dcm", DcmPlugin::default().into())?;
+
+    plugin_test.add_decl(Box::new(nu_command::Ls))?;
+
+    plugin_test.engine_state_mut().add_env_var(
+        "PWD".to_string(),
+        Value::string(get_asset_base_path().to_string_lossy(), Span::test_data()),
+    );
+
+    let result = plugin_test.eval("ls *.dcm | dcm name")?;
+    let result = result.into_value(Span::test_data())?;
+
+    // TODO actually test the result
+    assert!(result.as_list().is_ok());
+    Ok(())
+}
+
 /// Simulate `open file | dcm`
 #[test]
 fn test_scalar_binary_input() -> Result<(), Box<dyn std::error::Error>> {
@@ -138,7 +217,7 @@ fn test_vector_record_input() -> Result<(), Box<dyn std::error::Error>> {
     let mut plugin_test = PluginTest::new("dcm", DcmPlugin::default().into())?;
 
     // Create multiple records that simulate what 'ls *.dcm' would produce
-    let test_files = vec![
+    let test_files = [
         "ExplicitVRLittleEndian-Preamble.dcm",
         "ExplicitVRBigEndian-Preamble.dcm",
         "ImplicitVRLittleEndian-Preamble.dcm",
@@ -173,7 +252,7 @@ fn test_vector_record_input() -> Result<(), Box<dyn std::error::Error>> {
     }
 
     // Verify specific patient names match the filenames
-    let expected_names = vec![
+    let expected_names = [
         "ExplicitVRLittleEndian-Preamble",
         "ExplicitVRBigEndian-Preamble",
         "ImplicitVRLittleEndian-Preamble",