From 5868a3dab1aa863c5d919751ce41fe0121cc6b38 Mon Sep 17 00:00:00 2001 From: Nathan Richman Date: Mon, 11 Mar 2024 10:08:52 -0500 Subject: [PATCH 1/6] ENH: Add functionality for mutating element values * Add `value_at_mut`, `entry_at`, `entry_at_mut` and `update_value_at` methods on `InMemDicomObject` struct. * Add supporting methods to dependent objects --- core/src/header.rs | 4 ++ object/src/mem.rs | 164 ++++++++++++++++++++++++++++++++++++++++++++- 2 files changed, 167 insertions(+), 1 deletion(-) diff --git a/core/src/header.rs b/core/src/header.rs index 1d43c09bb..c31e60665 100644 --- a/core/src/header.rs +++ b/core/src/header.rs @@ -230,6 +230,10 @@ impl DataElement { &self.value } + pub fn value_mut(&mut self) -> &mut Value { + &mut self.value + } + /// Move the data value out of the element, discarding the rest. If the /// value is a sequence, its lifetime may still be bound to its original /// source. diff --git a/object/src/mem.rs b/object/src/mem.rs index 1b9135b8a..2b1f52620 100644 --- a/object/src/mem.rs +++ b/object/src/mem.rs @@ -677,6 +677,10 @@ where self.entries.get(&tag) } + pub fn get_mut(&mut self, tag: Tag) -> Option<&mut InMemElement> { + self.entries.get_mut(&tag) + } + /// Retrieve a particular DICOM element that might not exist by its name. /// /// If the element does not exist, @@ -841,6 +845,20 @@ where } } + pub fn update_value_at( + &mut self, + selector: impl Into, + f: impl FnMut(&mut Value, InMemFragment>) + ) -> Result<(), AtAccessError> { + self.entry_at_mut(selector).map(|e| { + e.update_value(f) + }) + .and_then(|_| { + self.len = Length::UNDEFINED; + Ok(()) + }) + } + /// Obtain the DICOM value by finding the element /// that matches the given selector. /// @@ -888,7 +906,7 @@ where AttributeSelectorStep::Nested { tag, item } => { let e = obj .entries - .get(tag) + .get(&tag) .with_context(|| crate::MissingSequenceSnafu { selector: selector.clone(), step_index: i as u32, @@ -924,6 +942,150 @@ where )); } + pub fn entry_at( + &self, + selector: impl Into, + ) -> Result<&InMemElement, AtAccessError> { + let selector = selector.into(); + + let mut obj = self; + for (i, step) in selector.iter().enumerate() { + match step { + // reached the leaf + AttributeSelectorStep::Tag(tag) => { + return obj.get(*tag).with_context(|| { + MissingLeafElementSnafu { + selector: selector.clone(), + } + }) + } + // navigate further down + AttributeSelectorStep::Nested { tag, item } => { + let e = obj + .entries + .get(&tag) + .with_context(|| crate::MissingSequenceSnafu { + selector: selector.clone(), + step_index: i as u32, + })?; + + // get items + let items = e.items().with_context(|| NotASequenceSnafu { + selector: selector.clone(), + step_index: i as u32, + })?; + + // if item.length == i and action is a constructive action, append new item + obj = + items + .get(*item as usize) + .with_context(|| crate::MissingSequenceSnafu { + selector: selector.clone(), + step_index: i as u32, + })?; + } + } + } + + unreachable!() + } + + pub fn entry_at_mut( + &mut self, + selector: impl Into, + ) -> Result<&mut InMemElement, AtAccessError> { + let selector = selector.into(); + + let mut obj = self; + for (i, step) in selector.iter().enumerate() { + match step { + // reached the leaf + AttributeSelectorStep::Tag(tag) => { + return obj.get_mut(*tag).with_context(|| { + MissingLeafElementSnafu { + selector: selector.clone(), + } + }) + } + // navigate further down + AttributeSelectorStep::Nested { tag, item } => { + let e = obj + .entries + .get_mut(&tag) + .with_context(|| crate::MissingSequenceSnafu { + selector: selector.clone(), + step_index: i as u32, + })?; + + // get items + let items = e.items_mut().with_context(|| NotASequenceSnafu { + selector: selector.clone(), + step_index: i as u32, + })?; + + // if item.length == i and action is a constructive action, append new item + obj = + items + .get_mut(*item as usize) + .with_context(|| crate::MissingSequenceSnafu { + selector: selector.clone(), + step_index: i as u32, + })?; + } + } + } + + unreachable!() + } + + pub fn value_at_mut( + &mut self, + selector: impl Into, + ) -> Result<&mut Value, InMemFragment>, AtAccessError> { + let selector = selector.into(); + + let mut obj = self; + for (i, step) in selector.iter().enumerate() { + match step { + // reached the leaf + AttributeSelectorStep::Tag(tag) => { + return obj.get_mut(*tag).map(|e| e.value_mut()).with_context(|| { + MissingLeafElementSnafu { + selector: selector.clone(), + } + }) + } + // navigate further down + AttributeSelectorStep::Nested { tag, item } => { + let e = obj + .entries + .get_mut(&tag) + .with_context(|| crate::MissingSequenceSnafu { + selector: selector.clone(), + step_index: i as u32, + })?; + + // get items + let items = e.items_mut().with_context(|| NotASequenceSnafu { + selector: selector.clone(), + step_index: i as u32, + })?; + + // if item.length == i and action is a constructive action, append new item + obj = + items + .get_mut(*item as usize) + .with_context(|| crate::MissingSequenceSnafu { + selector: selector.clone(), + step_index: i as u32, + })?; + } + } + } + + unreachable!() + } + /// Apply the given attribute operation on this object. /// /// See the [`dicom_core::ops`] module From 092c8a418b976f00c8b6e75e4e4276c5d185c9e9 Mon Sep 17 00:00:00 2001 From: Nathan Richman Date: Tue, 12 Mar 2024 10:43:44 -0500 Subject: [PATCH 2/6] MAIN: Review comments * Remove dangerous methods * Add documentation for new methods that are staying --- core/src/header.rs | 4 -- object/src/mem.rs | 115 +++++++++++++++++++++++++-------------------- 2 files changed, 64 insertions(+), 55 deletions(-) diff --git a/core/src/header.rs b/core/src/header.rs index c31e60665..1d43c09bb 100644 --- a/core/src/header.rs +++ b/core/src/header.rs @@ -230,10 +230,6 @@ impl DataElement { &self.value } - pub fn value_mut(&mut self) -> &mut Value { - &mut self.value - } - /// Move the data value out of the element, discarding the rest. If the /// value is a sequence, its lifetime may still be bound to its original /// source. diff --git a/object/src/mem.rs b/object/src/mem.rs index 2b1f52620..b60ad665e 100644 --- a/object/src/mem.rs +++ b/object/src/mem.rs @@ -677,7 +677,11 @@ where self.entries.get(&tag) } - pub fn get_mut(&mut self, tag: Tag) -> Option<&mut InMemElement> { + // Get a mutable reference to a particular DICOM attribute from this object by tag. + // + // Should be private as it would allow a user to change the tag of an + // element and diverge from the dictionary + fn get_mut(&mut self, tag: Tag) -> Option<&mut InMemElement> { self.entries.get_mut(&tag) } @@ -803,7 +807,7 @@ where self.len = Length::UNDEFINED; } - /// Obtain a temporary mutable reference to a DICOM value, + /// Obtain a temporary mutable reference to a DICOM value by tag, /// so that mutations can be applied within. /// /// If found, this method resets all related lengths recorded @@ -845,6 +849,55 @@ where } } + /// Obtain a temporary mutable reference to a DICOM value by AttributeSelector, + /// so that mutations can be applied within. + /// + /// If found, this method resets all related lengths recorded + /// and returns `true`. + /// Returns `false` otherwise. + /// + /// See the documentation of [`AttributeSelector`] for more information + /// on how to write attribute selectors. + /// + /// NOTE: Also consider using the [`apply`]. + /// + /// # Example + /// + /// ``` + /// # use dicom_core::{DataElement, VR, dicom_value, value::DataSetSequence}; + /// # use dicom_dictionary_std::tags; + /// # use dicom_object::InMemDicomObject; + /// # use dicom_core::ops::{AttributeAction, AttributeOp, ApplyOp}; + /// let mut dcm = InMemDicomObject::from_element_iter([ + /// DataElement::new( + /// tags::OTHER_PATIENT_I_DS_SEQUENCE, + /// VR::SQ, + /// DataSetSequence::from(vec![InMemDicomObject::from_element_iter([ + /// DataElement::new( + /// tags::PATIENT_ID, + /// VR::LO, + /// dicom_value!(Str, "1234") + /// )]) + /// ]) + /// ), + /// ]); + /// let selector = ( + /// tags::OTHER_PATIENT_I_DS_SEQUENCE, + /// 0, + /// tags::PATIENT_ID + /// ); + /// + /// // update referenced SOP instance UID for deidentification potentially + /// dcm.update_value_at(*&selector, |e| { + /// let mut v = e.primitive_mut().unwrap(); + /// *v = dicom_value!(Str, "abcd"); + /// }); + /// + /// assert_eq!( + /// dcm.entry_at(*&selector).unwrap().value().to_str().unwrap(), + /// "abcd" + /// ); + /// ``` pub fn update_value_at( &mut self, selector: impl Into, @@ -942,6 +995,9 @@ where )); } + /// Get a DataElement by AttributeSelector + /// + /// If the element or other intermediate elements do not exist, the method will return an error. pub fn entry_at( &self, selector: impl Into, @@ -990,7 +1046,10 @@ where unreachable!() } - pub fn entry_at_mut( + // Get a mutable reference to a particular entry by AttributeSelector + // + // Should be private for the same reason as `self.get_mut` + fn entry_at_mut( &mut self, selector: impl Into, ) -> Result<&mut InMemElement, AtAccessError> { @@ -1038,55 +1097,9 @@ where unreachable!() } - pub fn value_at_mut( - &mut self, - selector: impl Into, - ) -> Result<&mut Value, InMemFragment>, AtAccessError> { - let selector = selector.into(); - - let mut obj = self; - for (i, step) in selector.iter().enumerate() { - match step { - // reached the leaf - AttributeSelectorStep::Tag(tag) => { - return obj.get_mut(*tag).map(|e| e.value_mut()).with_context(|| { - MissingLeafElementSnafu { - selector: selector.clone(), - } - }) - } - // navigate further down - AttributeSelectorStep::Nested { tag, item } => { - let e = obj - .entries - .get_mut(&tag) - .with_context(|| crate::MissingSequenceSnafu { - selector: selector.clone(), - step_index: i as u32, - })?; - - // get items - let items = e.items_mut().with_context(|| NotASequenceSnafu { - selector: selector.clone(), - step_index: i as u32, - })?; - - // if item.length == i and action is a constructive action, append new item - obj = - items - .get_mut(*item as usize) - .with_context(|| crate::MissingSequenceSnafu { - selector: selector.clone(), - step_index: i as u32, - })?; - } - } - } - - unreachable!() - } - /// Apply the given attribute operation on this object. + /// + /// For a more free-form update, see [`update_value_at`]. /// /// See the [`dicom_core::ops`] module /// for more information. From 6828b2ff71b39bdecf9faa1bbfefef6c807fe53f Mon Sep 17 00:00:00 2001 From: Nathan Richman Date: Tue, 12 Mar 2024 11:30:45 -0500 Subject: [PATCH 3/6] MAIN: Fix formatting, add `value_at` back --- object/src/mem.rs | 105 ++++++++++++++++++++++++++++++++++++++++------ 1 file changed, 93 insertions(+), 12 deletions(-) diff --git a/object/src/mem.rs b/object/src/mem.rs index b60ad665e..afd73d0ef 100644 --- a/object/src/mem.rs +++ b/object/src/mem.rs @@ -678,7 +678,7 @@ where } // Get a mutable reference to a particular DICOM attribute from this object by tag. - // + // // Should be private as it would allow a user to change the tag of an // element and diverge from the dictionary fn get_mut(&mut self, tag: Tag) -> Option<&mut InMemElement> { @@ -849,16 +849,92 @@ where } } + /// Obtain the DICOM value by finding the element + /// that matches the given selector. + /// + /// Returns an error if the respective element or any of its parents + /// cannot be found. + /// + /// See the documentation of [`AttributeSelector`] for more information + /// on how to write attribute selectors. + /// + /// See also [`entry_at`] to get the entry rather than the value. + /// + /// # Example + /// + /// ```no_run + /// # use dicom_core::prelude::*; + /// # use dicom_core::ops::AttributeSelector; + /// # use dicom_dictionary_std::tags; + /// # use dicom_object::InMemDicomObject; + /// # let obj: InMemDicomObject = unimplemented!(); + /// let referenced_sop_instance_iod = obj.value_at( + /// ( + /// tags::SHARED_FUNCTIONAL_GROUPS_SEQUENCE, + /// tags::REFERENCED_IMAGE_SEQUENCE, + /// tags::REFERENCED_SOP_INSTANCE_UID, + /// ))? + /// .to_str()?; + /// # Ok::<_, Box>(()) + /// ``` + pub fn value_at( + &self, + selector: impl Into, + ) -> Result<&Value, InMemFragment>, AtAccessError> { + let selector = selector.into(); + + let mut obj = self; + for (i, step) in selector.iter().enumerate() { + match step { + // reached the leaf + AttributeSelectorStep::Tag(tag) => { + return obj.get(*tag).map(|e| e.value()).with_context(|| { + MissingLeafElementSnafu { + selector: selector.clone(), + } + }) + } + // navigate further down + AttributeSelectorStep::Nested { tag, item } => { + let e = obj + .entries + .get(tag) + .with_context(|| crate::MissingSequenceSnafu { + selector: selector.clone(), + step_index: i as u32, + })?; + + // get items + let items = e.items().with_context(|| NotASequenceSnafu { + selector: selector.clone(), + step_index: i as u32, + })?; + + // if item.length == i and action is a constructive action, append new item + obj = + items + .get(*item as usize) + .with_context(|| crate::MissingSequenceSnafu { + selector: selector.clone(), + step_index: i as u32, + })?; + } + } + } + + unreachable!() + } + /// Obtain a temporary mutable reference to a DICOM value by AttributeSelector, /// so that mutations can be applied within. /// /// If found, this method resets all related lengths recorded /// and returns `true`. /// Returns `false` otherwise. - /// + /// /// See the documentation of [`AttributeSelector`] for more information /// on how to write attribute selectors. - /// + /// /// NOTE: Also consider using the [`apply`]. /// /// # Example @@ -875,15 +951,15 @@ where /// DataSetSequence::from(vec![InMemDicomObject::from_element_iter([ /// DataElement::new( /// tags::PATIENT_ID, - /// VR::LO, + /// VR::LO, /// dicom_value!(Str, "1234") /// )]) /// ]) /// ), /// ]); /// let selector = ( - /// tags::OTHER_PATIENT_I_DS_SEQUENCE, - /// 0, + /// tags::OTHER_PATIENT_I_DS_SEQUENCE, + /// 0, /// tags::PATIENT_ID /// ); /// @@ -908,7 +984,7 @@ where }) .and_then(|_| { self.len = Length::UNDEFINED; - Ok(()) + Ok(()) }) } @@ -996,8 +1072,13 @@ where } /// Get a DataElement by AttributeSelector - /// + /// /// If the element or other intermediate elements do not exist, the method will return an error. + /// + /// See the documentation of [`AttributeSelector`] for more information + /// on how to write attribute selectors. + /// + /// If you only need the value, use [`value_at`]. pub fn entry_at( &self, selector: impl Into, @@ -1019,7 +1100,7 @@ where AttributeSelectorStep::Nested { tag, item } => { let e = obj .entries - .get(&tag) + .get(tag) .with_context(|| crate::MissingSequenceSnafu { selector: selector.clone(), step_index: i as u32, @@ -1070,7 +1151,7 @@ where AttributeSelectorStep::Nested { tag, item } => { let e = obj .entries - .get_mut(&tag) + .get_mut(tag) .with_context(|| crate::MissingSequenceSnafu { selector: selector.clone(), step_index: i as u32, @@ -1098,8 +1179,8 @@ where } /// Apply the given attribute operation on this object. - /// - /// For a more free-form update, see [`update_value_at`]. + /// + /// For more complex updates, see [`update_value_at`]. /// /// See the [`dicom_core::ops`] module /// for more information. From 35f9ca9b0b039569724a14655047fde6a205873e Mon Sep 17 00:00:00 2001 From: Nate Richman Date: Sat, 16 Mar 2024 09:17:41 -0500 Subject: [PATCH 4/6] MAIN: and_then -> map, run rustfmt --- object/src/mem.rs | 53 ++++++++++++++++++++--------------------------- 1 file changed, 23 insertions(+), 30 deletions(-) diff --git a/object/src/mem.rs b/object/src/mem.rs index afd73d0ef..a31ccc0ae 100644 --- a/object/src/mem.rs +++ b/object/src/mem.rs @@ -977,15 +977,13 @@ where pub fn update_value_at( &mut self, selector: impl Into, - f: impl FnMut(&mut Value, InMemFragment>) + f: impl FnMut(&mut Value, InMemFragment>), ) -> Result<(), AtAccessError> { - self.entry_at_mut(selector).map(|e| { - e.update_value(f) - }) - .and_then(|_| { - self.len = Length::UNDEFINED; - Ok(()) - }) + self.entry_at_mut(selector) + .map(|e| e.update_value(f)) + .map(|_| { + self.len = Length::UNDEFINED; + }) } /// Obtain the DICOM value by finding the element @@ -1090,10 +1088,8 @@ where match step { // reached the leaf AttributeSelectorStep::Tag(tag) => { - return obj.get(*tag).with_context(|| { - MissingLeafElementSnafu { - selector: selector.clone(), - } + return obj.get(*tag).with_context(|| MissingLeafElementSnafu { + selector: selector.clone(), }) } // navigate further down @@ -1141,21 +1137,19 @@ where match step { // reached the leaf AttributeSelectorStep::Tag(tag) => { - return obj.get_mut(*tag).with_context(|| { - MissingLeafElementSnafu { - selector: selector.clone(), - } + return obj.get_mut(*tag).with_context(|| MissingLeafElementSnafu { + selector: selector.clone(), }) } // navigate further down AttributeSelectorStep::Nested { tag, item } => { - let e = obj - .entries - .get_mut(tag) - .with_context(|| crate::MissingSequenceSnafu { - selector: selector.clone(), - step_index: i as u32, - })?; + let e = + obj.entries + .get_mut(tag) + .with_context(|| crate::MissingSequenceSnafu { + selector: selector.clone(), + step_index: i as u32, + })?; // get items let items = e.items_mut().with_context(|| NotASequenceSnafu { @@ -1164,13 +1158,12 @@ where })?; // if item.length == i and action is a constructive action, append new item - obj = - items - .get_mut(*item as usize) - .with_context(|| crate::MissingSequenceSnafu { - selector: selector.clone(), - step_index: i as u32, - })?; + obj = items.get_mut(*item as usize).with_context(|| { + crate::MissingSequenceSnafu { + selector: selector.clone(), + step_index: i as u32, + } + })?; } } } From 6978b2cb6bc41dd82320d2032100977e9e6486f4 Mon Sep 17 00:00:00 2001 From: Eduardo Pinho Date: Thu, 11 Apr 2024 08:44:10 +0100 Subject: [PATCH 5/6] [object] fix rebase - remove duplicate value_at method --- object/src/mem.rs | 76 ----------------------------------------------- 1 file changed, 76 deletions(-) diff --git a/object/src/mem.rs b/object/src/mem.rs index a31ccc0ae..0aa6cd94d 100644 --- a/object/src/mem.rs +++ b/object/src/mem.rs @@ -849,82 +849,6 @@ where } } - /// Obtain the DICOM value by finding the element - /// that matches the given selector. - /// - /// Returns an error if the respective element or any of its parents - /// cannot be found. - /// - /// See the documentation of [`AttributeSelector`] for more information - /// on how to write attribute selectors. - /// - /// See also [`entry_at`] to get the entry rather than the value. - /// - /// # Example - /// - /// ```no_run - /// # use dicom_core::prelude::*; - /// # use dicom_core::ops::AttributeSelector; - /// # use dicom_dictionary_std::tags; - /// # use dicom_object::InMemDicomObject; - /// # let obj: InMemDicomObject = unimplemented!(); - /// let referenced_sop_instance_iod = obj.value_at( - /// ( - /// tags::SHARED_FUNCTIONAL_GROUPS_SEQUENCE, - /// tags::REFERENCED_IMAGE_SEQUENCE, - /// tags::REFERENCED_SOP_INSTANCE_UID, - /// ))? - /// .to_str()?; - /// # Ok::<_, Box>(()) - /// ``` - pub fn value_at( - &self, - selector: impl Into, - ) -> Result<&Value, InMemFragment>, AtAccessError> { - let selector = selector.into(); - - let mut obj = self; - for (i, step) in selector.iter().enumerate() { - match step { - // reached the leaf - AttributeSelectorStep::Tag(tag) => { - return obj.get(*tag).map(|e| e.value()).with_context(|| { - MissingLeafElementSnafu { - selector: selector.clone(), - } - }) - } - // navigate further down - AttributeSelectorStep::Nested { tag, item } => { - let e = obj - .entries - .get(tag) - .with_context(|| crate::MissingSequenceSnafu { - selector: selector.clone(), - step_index: i as u32, - })?; - - // get items - let items = e.items().with_context(|| NotASequenceSnafu { - selector: selector.clone(), - step_index: i as u32, - })?; - - // if item.length == i and action is a constructive action, append new item - obj = - items - .get(*item as usize) - .with_context(|| crate::MissingSequenceSnafu { - selector: selector.clone(), - step_index: i as u32, - })?; - } - } - } - - unreachable!() - } - /// Obtain a temporary mutable reference to a DICOM value by AttributeSelector, /// so that mutations can be applied within. /// From 7542ff59d6e9013ee3abaa3bb226693107f1ad8e Mon Sep 17 00:00:00 2001 From: Eduardo Pinho Date: Sat, 13 Apr 2024 11:08:31 +0100 Subject: [PATCH 6/6] [object] clippy lint - needless borrow in `.get(&tag)` - work around rust-analyzer bug in line 1023 --- object/src/mem.rs | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/object/src/mem.rs b/object/src/mem.rs index 0aa6cd94d..a00b36050 100644 --- a/object/src/mem.rs +++ b/object/src/mem.rs @@ -957,7 +957,7 @@ where AttributeSelectorStep::Nested { tag, item } => { let e = obj .entries - .get(&tag) + .get(tag) .with_context(|| crate::MissingSequenceSnafu { selector: selector.clone(), step_index: i as u32, @@ -1005,7 +1005,7 @@ where &self, selector: impl Into, ) -> Result<&InMemElement, AtAccessError> { - let selector = selector.into(); + let selector: AttributeSelector = selector.into(); let mut obj = self; for (i, step) in selector.iter().enumerate() { @@ -1054,7 +1054,7 @@ where &mut self, selector: impl Into, ) -> Result<&mut InMemElement, AtAccessError> { - let selector = selector.into(); + let selector: AttributeSelector = selector.into(); let mut obj = self; for (i, step) in selector.iter().enumerate() {