From bc85ced177f1f35c7253fe5525b02a615fbb0706 Mon Sep 17 00:00:00 2001 From: Juraj Mlaka Date: Sun, 11 Jun 2023 14:16:14 +0200 Subject: [PATCH 1/6] - InMemDicomObject wil have it's length set to - Length::UNDEFINED after modification - to it's inner entries value --- object/src/mem.rs | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/object/src/mem.rs b/object/src/mem.rs index 7fc5b277a..ecda9bcb1 100644 --- a/object/src/mem.rs +++ b/object/src/mem.rs @@ -612,18 +612,21 @@ where /// Insert a data element to the object, replacing (and returning) any /// previous element of the same attribute. pub fn put(&mut self, elt: InMemElement) -> Option> { + self.len = Length::UNDEFINED; self.put_element(elt) } /// Insert a data element to the object, replacing (and returning) any /// previous element of the same attribute. pub fn put_element(&mut self, elt: InMemElement) -> Option> { + self.len = Length::UNDEFINED; self.entries.insert(elt.tag(), elt) } /// Remove a DICOM element by its tag, /// reporting whether it was present. pub fn remove_element(&mut self, tag: Tag) -> bool { + self.len = Length::UNDEFINED; self.entries.remove(&tag).is_some() } @@ -631,11 +634,13 @@ where /// reporting whether it was present. pub fn remove_element_by_name(&mut self, name: &str) -> Result { let tag = self.lookup_name(name)?; + self.len = Length::UNDEFINED; Ok(self.entries.remove(&tag).is_some()) } /// Remove and return a particular DICOM element by its tag. pub fn take_element(&mut self, tag: Tag) -> Result> { + self.len = Length::UNDEFINED; self.entries .remove(&tag) .context(NoSuchDataElementTagSnafu { tag }) @@ -645,6 +650,7 @@ where /// if it is present, /// returns `None` otherwise. pub fn take(&mut self, tag: Tag) -> Option> { + self.len = Length::UNDEFINED; self.entries.remove(&tag) } @@ -654,6 +660,7 @@ where name: &str, ) -> Result, AccessByNameError> { let tag = self.lookup_name(name)?; + self.len = Length::UNDEFINED; self.entries .remove(&tag) .with_context(|| NoSuchDataElementAliasSnafu { @@ -669,6 +676,7 @@ where /// and those for which `f(&element)` returns `false` are removed. pub fn retain(&mut self, mut f: impl FnMut(&InMemElement) -> bool) { self.entries.retain(|_, elem| f(elem)); + self.len = Length::UNDEFINED; } /// Apply the given attribute operation on this object. @@ -721,6 +729,7 @@ where // replace element *e = DataElement::empty(tag, vr); } + self.len = Length::UNDEFINED; Ok(()) } AttributeAction::SetVr(new_vr) => { @@ -1368,6 +1377,7 @@ impl Extend> for InMemDicomObject { where I: IntoIterator>, { + self.len = Length::UNDEFINED; self.entries.extend(iter.into_iter().map(|e| (e.tag(), e))) } } From 02aed0ab947718ae44bfe4e7ca250ec87ea4e25a Mon Sep 17 00:00:00 2001 From: Juraj Mlaka Date: Mon, 12 Jun 2023 10:37:36 +0200 Subject: [PATCH 2/6] - correct conditional Length setting --- object/src/mem.rs | 33 +++++++++++++++++++++++++-------- 1 file changed, 25 insertions(+), 8 deletions(-) diff --git a/object/src/mem.rs b/object/src/mem.rs index ecda9bcb1..7fe1d20a0 100644 --- a/object/src/mem.rs +++ b/object/src/mem.rs @@ -626,23 +626,34 @@ where /// Remove a DICOM element by its tag, /// reporting whether it was present. pub fn remove_element(&mut self, tag: Tag) -> bool { - self.len = Length::UNDEFINED; - self.entries.remove(&tag).is_some() + if self.entries.remove(&tag).is_some() { + self.len = Length::UNDEFINED; + true + } else { + false + } } /// Remove a DICOM element by its keyword, /// reporting whether it was present. pub fn remove_element_by_name(&mut self, name: &str) -> Result { let tag = self.lookup_name(name)?; - self.len = Length::UNDEFINED; - Ok(self.entries.remove(&tag).is_some()) + Ok(self.entries.remove(&tag).is_some()).map(|removed| { + if removed { + self.len = Length::UNDEFINED; + } + removed + }) } /// Remove and return a particular DICOM element by its tag. pub fn take_element(&mut self, tag: Tag) -> Result> { - self.len = Length::UNDEFINED; self.entries .remove(&tag) + .map(|e| { + self.len = Length::UNDEFINED; + e + }) .context(NoSuchDataElementTagSnafu { tag }) } @@ -650,8 +661,10 @@ where /// if it is present, /// returns `None` otherwise. pub fn take(&mut self, tag: Tag) -> Option> { - self.len = Length::UNDEFINED; - self.entries.remove(&tag) + self.entries.remove(&tag).map(|e| { + self.len = Length::UNDEFINED; + e + }) } /// Remove and return a particular DICOM element by its name. @@ -663,6 +676,10 @@ where self.len = Length::UNDEFINED; self.entries .remove(&tag) + .map(|e| { + self.len = Length::UNDEFINED; + e + }) .with_context(|| NoSuchDataElementAliasSnafu { tag, alias: name.to_string(), @@ -728,8 +745,8 @@ where let vr = e.vr(); // replace element *e = DataElement::empty(tag, vr); + self.len = Length::UNDEFINED; } - self.len = Length::UNDEFINED; Ok(()) } AttributeAction::SetVr(new_vr) => { From 0ad1930e9141036890f3ca66a42c16d190edcd50 Mon Sep 17 00:00:00 2001 From: Juraj Mlaka Date: Tue, 13 Jun 2023 12:14:34 +0200 Subject: [PATCH 3/6] -duplicate code correction --- object/src/mem.rs | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/object/src/mem.rs b/object/src/mem.rs index 7fe1d20a0..39fcd4410 100644 --- a/object/src/mem.rs +++ b/object/src/mem.rs @@ -612,7 +612,6 @@ where /// Insert a data element to the object, replacing (and returning) any /// previous element of the same attribute. pub fn put(&mut self, elt: InMemElement) -> Option> { - self.len = Length::UNDEFINED; self.put_element(elt) } @@ -620,7 +619,10 @@ where /// previous element of the same attribute. pub fn put_element(&mut self, elt: InMemElement) -> Option> { self.len = Length::UNDEFINED; - self.entries.insert(elt.tag(), elt) + self.entries.insert(elt.tag(), elt).map(|e| { + self.len = Length::UNDEFINED; + e + }) } /// Remove a DICOM element by its tag, @@ -673,7 +675,6 @@ where name: &str, ) -> Result, AccessByNameError> { let tag = self.lookup_name(name)?; - self.len = Length::UNDEFINED; self.entries .remove(&tag) .map(|e| { @@ -809,6 +810,7 @@ where if let Some(e) = self.entries.get_mut(&tag) { let vr = e.vr(); *e = DataElement::new(tag, vr, new_value); + self.len = Length::UNDEFINED; } else { // infer VR from tag let vr = dicom_dictionary_std::StandardDataDictionary From 6dcc8521daeddbef0c25db440c78f5934326d1a6 Mon Sep 17 00:00:00 2001 From: Juraj Mlaka Date: Wed, 14 Jun 2023 16:46:08 +0200 Subject: [PATCH 4/6] - test added, works --- object/src/mem.rs | 178 ++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 178 insertions(+) diff --git a/object/src/mem.rs b/object/src/mem.rs index 39fcd4410..3ca58e3f1 100644 --- a/object/src/mem.rs +++ b/object/src/mem.rs @@ -2452,4 +2452,182 @@ mod tests { ); } } + + #[test] + fn inmem_obj_reset_defined_length() { + let mut entries: BTreeMap> = BTreeMap::new(); + + let patient_name = + DataElement::new(tags::PATIENT_NAME, VR::CS, PrimitiveValue::from("Doe^John")); + + let study_description = DataElement::new( + tags::STUDY_DESCRIPTION, + VR::LO, + PrimitiveValue::from("Test study"), + ); + + entries.insert(tags::PATIENT_NAME, patient_name.clone()); + + // create object and force an arbitrary defined Length value + let obj = InMemDicomObject:: { + entries, + dict: StandardDataDictionary, + len: Length(1), + }; + + assert!(obj.length().is_defined()); + + let mut o = obj.clone(); + o.put_element(study_description); + assert!(o.length().is_undefined()); + + let mut o = obj.clone(); + o.remove_element(tags::PATIENT_NAME); + assert!(o.length().is_undefined()); + + let mut o = obj.clone(); + o.remove_element_by_name("PatientName").unwrap(); + assert!(o.length().is_undefined()); + + let mut o = obj.clone(); + o.take_element(tags::PATIENT_NAME).unwrap(); + assert!(o.length().is_undefined()); + + let mut o = obj.clone(); + o.take_element_by_name("PatientName").unwrap(); + assert!(o.length().is_undefined()); + + // resets Length even when retain does not make any changes + let mut o = obj.clone(); + o.retain(|e| e.tag() == tags::PATIENT_NAME); + assert!(o.length().is_undefined()); + + let mut o = obj.clone(); + o.apply(AttributeOp { + tag: tags::PATIENT_NAME, + action: AttributeAction::Remove, + }) + .unwrap(); + assert!(o.length().is_undefined()); + + let mut o = obj.clone(); + o.apply(AttributeOp { + tag: tags::PATIENT_NAME, + action: AttributeAction::Empty, + }) + .unwrap(); + assert!(o.length().is_undefined()); + + let mut o = obj.clone(); + o.apply(AttributeOp { + tag: tags::PATIENT_NAME, + action: AttributeAction::SetVr(VR::IS), + }) + .unwrap(); + assert!(o.length().is_undefined()); + + let mut o = obj.clone(); + o.apply(AttributeOp { + tag: tags::PATIENT_NAME, + action: AttributeAction::Set(dicom_value!(Str, "Unknown")), + }) + .unwrap(); + assert!(o.length().is_undefined()); + + let mut o = obj.clone(); + o.apply(AttributeOp { + tag: tags::PATIENT_NAME, + action: AttributeAction::SetStr("Patient^Anonymous".into()), + }) + .unwrap(); + assert!(o.length().is_undefined()); + + let mut o = obj.clone(); + o.apply(AttributeOp { + tag: tags::PATIENT_AGE, + action: AttributeAction::SetIfMissing(dicom_value!(75)), + }) + .unwrap(); + assert!(o.length().is_undefined()); + + let mut o = obj.clone(); + o.apply(AttributeOp { + tag: tags::PATIENT_ADDRESS, + action: AttributeAction::SetStrIfMissing("Chicago".into()), + }) + .unwrap(); + assert!(o.length().is_undefined()); + + let mut o = obj.clone(); + o.apply(AttributeOp { + tag: tags::PATIENT_NAME, + action: AttributeAction::Replace(dicom_value!(Str, "Unknown")), + }) + .unwrap(); + assert!(o.length().is_undefined()); + + let mut o = obj.clone(); + o.apply(AttributeOp { + tag: tags::PATIENT_NAME, + action: AttributeAction::ReplaceStr("Unknown".into()), + }) + .unwrap(); + assert!(o.length().is_undefined()); + + let mut o = obj.clone(); + o.apply(AttributeOp { + tag: tags::PATIENT_NAME, + action: AttributeAction::PushStr("^Prof".into()), + }) + .unwrap(); + assert!(o.length().is_undefined()); + + let mut o = obj.clone(); + o.apply(AttributeOp { + tag: tags::PATIENT_NAME, + action: AttributeAction::PushI32(-16), + }) + .unwrap(); + assert!(o.length().is_undefined()); + + let mut o = obj.clone(); + o.apply(AttributeOp { + tag: tags::PATIENT_NAME, + action: AttributeAction::PushU32(16), + }) + .unwrap(); + assert!(o.length().is_undefined()); + + let mut o = obj.clone(); + o.apply(AttributeOp { + tag: tags::PATIENT_NAME, + action: AttributeAction::PushI16(-16), + }) + .unwrap(); + assert!(o.length().is_undefined()); + + let mut o = obj.clone(); + o.apply(AttributeOp { + tag: tags::PATIENT_NAME, + action: AttributeAction::PushU16(16), + }) + .unwrap(); + assert!(o.length().is_undefined()); + + let mut o = obj.clone(); + o.apply(AttributeOp { + tag: tags::PATIENT_NAME, + action: AttributeAction::PushF32(16.16), + }) + .unwrap(); + assert!(o.length().is_undefined()); + + let mut o = obj.clone(); + o.apply(AttributeOp { + tag: tags::PATIENT_NAME, + action: AttributeAction::PushF64(16.1616), + }) + .unwrap(); + assert!(o.length().is_undefined()); + } } From 3bffa2c8d0302acd943deaee6e30d6167e00f688 Mon Sep 17 00:00:00 2001 From: jmlaka <74211692+jmlaka@users.noreply.github.com> Date: Wed, 14 Jun 2023 19:20:55 +0200 Subject: [PATCH 5/6] Update object/src/mem.rs Co-authored-by: Eduardo Pinho --- object/src/mem.rs | 1 - 1 file changed, 1 deletion(-) diff --git a/object/src/mem.rs b/object/src/mem.rs index 3ca58e3f1..deb4fc9a7 100644 --- a/object/src/mem.rs +++ b/object/src/mem.rs @@ -618,7 +618,6 @@ where /// Insert a data element to the object, replacing (and returning) any /// previous element of the same attribute. pub fn put_element(&mut self, elt: InMemElement) -> Option> { - self.len = Length::UNDEFINED; self.entries.insert(elt.tag(), elt).map(|e| { self.len = Length::UNDEFINED; e From 3ba0e903afd0b4c88aac9cdd52c0f7ee4c2c3f72 Mon Sep 17 00:00:00 2001 From: Juraj Mlaka Date: Wed, 14 Jun 2023 19:55:23 +0200 Subject: [PATCH 6/6] - put_element always resets length --- object/src/mem.rs | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/object/src/mem.rs b/object/src/mem.rs index deb4fc9a7..e5f26197d 100644 --- a/object/src/mem.rs +++ b/object/src/mem.rs @@ -618,10 +618,8 @@ where /// Insert a data element to the object, replacing (and returning) any /// previous element of the same attribute. pub fn put_element(&mut self, elt: InMemElement) -> Option> { - self.entries.insert(elt.tag(), elt).map(|e| { - self.len = Length::UNDEFINED; - e - }) + self.len = Length::UNDEFINED; + self.entries.insert(elt.tag(), elt) } /// Remove a DICOM element by its tag,