From 27a45fff5b20c86be5b47c4df8eca660cafd7397 Mon Sep 17 00:00:00 2001 From: Nathan Richman Date: Tue, 12 Mar 2024 10:43:44 -0500 Subject: [PATCH] MAIN: Review comments * Remove dangerous methods * Add documentation for new methods that are staying --- core/src/header.rs | 4 - object/src/mem.rs | 189 +++++++++++++++------------------------------ 2 files changed, 64 insertions(+), 129 deletions(-) diff --git a/core/src/header.rs b/core/src/header.rs index b55d2dfc5..bcc254b68 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 99907e46a..11a69f63e 100644 --- a/object/src/mem.rs +++ b/object/src/mem.rs @@ -684,7 +684,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) } @@ -805,7 +809,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 @@ -846,6 +850,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, @@ -860,80 +913,9 @@ 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. - /// - /// # 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!() - } - + /// 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, @@ -982,7 +964,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> { @@ -1030,55 +1015,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.