Return error when you try to change a global property. Fixes #71. (#74)

* Change |set_global_property_value()| to return Result<IxaError>. Fixes #71. This keeps the semantics that global property values are immutable but returns an error when you try to change one. We might still want to change this semantic, but at least this prevents silent failure.
CDCgov · Dec 2, 2024 · d0478c0 · d0478c0
1 parent 1d9f8e3
commit d0478c0
Show file tree

Hide file tree

Showing 11 changed files with 83 additions and 30 deletions.
diff --git a/examples/births-deaths/infection_manager.rs b/examples/births-deaths/infection_manager.rs
@@ -127,7 +127,9 @@ mod test {
 
         let mut context = Context::new();
 
-        context.set_global_property_value(Parameters, p_values.clone());
+        context
+            .set_global_property_value(Parameters, p_values.clone())
+            .unwrap();
         context.init_random(p_values.seed);
         init(&mut context);
 

diff --git a/examples/births-deaths/parameters_loader.rs b/examples/births-deaths/parameters_loader.rs
@@ -32,7 +32,9 @@ define_global_property!(Foi, HashMap<AgeGroupRisk, f64>);
 
 pub fn init_parameters(context: &mut Context, file_path: &Path) -> Result<(), IxaError> {
     let parameters_json = context.load_parameters_from_json::<ParametersValues>(file_path)?;
-    context.set_global_property_value(Parameters, parameters_json.clone());
+    context
+        .set_global_property_value(Parameters, parameters_json.clone())
+        .unwrap();
 
     let foi_map = parameters_json
         .foi_groups
@@ -41,7 +43,9 @@ pub fn init_parameters(context: &mut Context, file_path: &Path) -> Result<(), Ix
         .map(|x| (x.group_name, x.foi))
         .collect::<HashMap<AgeGroupRisk, f64>>();
 
-    context.set_global_property_value(Foi, foi_map.clone());
+    context
+        .set_global_property_value(Foi, foi_map.clone())
+        .unwrap();
 
     Ok(())
 }
diff --git a/examples/births-deaths/population_manager.rs b/examples/births-deaths/population_manager.rs
@@ -278,7 +278,9 @@ mod test {
         };
 
         let mut context = Context::new();
-        context.set_global_property_value(Parameters, p_values.clone());
+        context
+            .set_global_property_value(Parameters, p_values.clone())
+            .unwrap();
         context.init_random(p_values.seed);
 
         schedule_birth(&mut context);
@@ -300,7 +302,9 @@ mod test {
         };
 
         let mut context = Context::new();
-        context.set_global_property_value(Parameters, p_values.clone());
+        context
+            .set_global_property_value(Parameters, p_values.clone())
+            .unwrap();
         context.init_random(p_values.seed);
         let _person = context.create_new_person(0);
         schedule_death(&mut context);

diff --git a/examples/parameter-loading/infection_manager.rs b/examples/parameter-loading/infection_manager.rs
@@ -66,7 +66,9 @@ mod test {
         };
         let mut context = Context::new();
 
-        context.set_global_property_value(Parameters, p_values.clone());
+        context
+            .set_global_property_value(Parameters, p_values.clone())
+            .unwrap();
         context.init_random(42);
         init(&mut context);
 

diff --git a/examples/parameter-loading/parameters_loader.rs b/examples/parameter-loading/parameters_loader.rs
@@ -21,6 +21,6 @@ define_global_property!(Parameters, ParametersValues);
 
 pub fn init_parameters(context: &mut Context, file_path: &Path) -> Result<(), IxaError> {
     let parameters_json = context.load_parameters_from_json::<ParametersValues>(file_path)?;
-    context.set_global_property_value(Parameters, parameters_json);
+    context.set_global_property_value(Parameters, parameters_json)?;
     Ok(())
 }
diff --git a/examples/parameter-loading/transmission_manager.rs b/examples/parameter-loading/transmission_manager.rs
@@ -70,7 +70,9 @@ mod test {
         };
 
         let mut context = Context::new();
-        context.set_global_property_value(Parameters, p_values);
+        context
+            .set_global_property_value(Parameters, p_values)
+            .unwrap();
         context.init_random(123);
         let pid = context.add_person(()).unwrap();
         attempt_infection(&mut context);

diff --git a/examples/time-varying-infection/exposure_manager.rs b/examples/time-varying-infection/exposure_manager.rs
@@ -94,7 +94,9 @@ mod test {
             output_file: ".".to_string(),
         };
         let mut context = Context::new();
-        context.set_global_property_value(Parameters, p_values);
+        context
+            .set_global_property_value(Parameters, p_values)
+            .unwrap();
         let parameters = context
             .get_global_property_value(Parameters)
             .unwrap()
@@ -132,7 +134,9 @@ mod test {
             output_file: ".".to_string(),
         };
         let mut context = Context::new();
-        context.set_global_property_value(Parameters, p_values);
+        context
+            .set_global_property_value(Parameters, p_values)
+            .unwrap();
         let parameters = context
             .get_global_property_value(Parameters)
             .unwrap()

diff --git a/examples/time-varying-infection/infection_manager.rs b/examples/time-varying-infection/infection_manager.rs
@@ -120,7 +120,9 @@ mod test {
         };
         let mut context = Context::new();
 
-        context.set_global_property_value(Parameters, p_values);
+        context
+            .set_global_property_value(Parameters, p_values)
+            .unwrap();
         let parameters = context
             .get_global_property_value(Parameters)
             .unwrap()
@@ -161,7 +163,9 @@ mod test {
             output_dir: ".".to_string(),
             output_file: ".".to_string(),
         };
-        context.set_global_property_value(Parameters, parameters.clone());
+        context
+            .set_global_property_value(Parameters, parameters.clone())
+            .unwrap();
         context.init_random(parameters.seed);
         for _ in 0..parameters.population {
             let person_id = context.add_person(()).unwrap();
@@ -211,7 +215,9 @@ mod test {
         let mut sum = 0.0;
         for seed in 0..n_iter {
             let mut context = Context::new();
-            context.set_global_property_value(Parameters, parameters.clone());
+            context
+                .set_global_property_value(Parameters, parameters.clone())
+                .unwrap();
             context.init_random(seed);
             init(&mut context);
             let person_id = context

diff --git a/examples/time-varying-infection/parameters_loader.rs b/examples/time-varying-infection/parameters_loader.rs
@@ -23,6 +23,6 @@ define_global_property!(Parameters, ParametersValues);
 
 pub fn init_parameters(context: &mut Context, file_path: &Path) -> Result<(), IxaError> {
     let parameters_json = context.load_parameters_from_json::<ParametersValues>(file_path)?;
-    context.set_global_property_value(Parameters, parameters_json);
+    context.set_global_property_value(Parameters, parameters_json)?;
     Ok(())
 }
diff --git a/src/error.rs b/src/error.rs
@@ -4,8 +4,8 @@ use std::io;
 
 #[derive(Debug)]
 #[allow(clippy::module_name_repetitions)]
-/// A wrapper around other error types, as well
-/// as our specific error type.
+/// Provides `IxaError` and maps to other errors to
+/// convert to an `IxaError`
 pub enum IxaError {
     IoError(io::Error),
     JsonError(serde_json::Error),

diff --git a/src/global_properties.rs b/src/global_properties.rs
@@ -20,7 +20,7 @@ use crate::error::IxaError;
 use serde::de::DeserializeOwned;
 use std::any::{Any, TypeId};
 use std::cell::RefCell;
-use std::collections::HashMap;
+use std::collections::{hash_map::Entry, HashMap};
 use std::fmt::Debug;
 use std::fs;
 use std::io::BufReader;
@@ -58,7 +58,7 @@ where
                 if context.get_global_property_value(T::new()).is_some() {
                     return Err(IxaError::IxaError(format!("Duplicate property {name}")));
                 }
-                context.set_global_property_value(T::new(), val);
+                let _ = context.set_global_property_value(T::new(), val);
                 Ok(())
             },
         ),
@@ -142,11 +142,14 @@ crate::context::define_data_plugin!(
 
 pub trait ContextGlobalPropertiesExt {
     /// Set the value of a global property of type T
+    ///
+    /// # Errors
+    /// Will return an error if an attempt is made to change a value.
     fn set_global_property_value<T: GlobalProperty + 'static>(
         &mut self,
         property: T,
         value: T::Value,
-    );
+    ) -> Result<(), IxaError>;
 
     /// Return value of global property T
     fn get_global_property_value<T: GlobalProperty + 'static>(
@@ -195,11 +198,14 @@ impl GlobalPropertiesDataContainer {
         &mut self,
         _property: &T,
         value: T::Value,
-    ) {
-        let _data_container = self
-            .global_property_container
-            .entry(TypeId::of::<T>())
-            .or_insert_with(|| Box::new(value));
+    ) -> Result<(), IxaError> {
+        match self.global_property_container.entry(TypeId::of::<T>()) {
+            Entry::Vacant(entry) => {
+                entry.insert(Box::new(value));
+                Ok(())
+            }
+            Entry::Occupied(_) => Err(IxaError::from("Entry already exists")),
+        }
     }
 
     #[must_use]
@@ -218,9 +224,9 @@ impl ContextGlobalPropertiesExt for Context {
         &mut self,
         property: T,
         value: T::Value,
-    ) {
+    ) -> Result<(), IxaError> {
         let data_container = self.get_data_container_mut(GlobalPropertiesPlugin);
-        data_container.set_global_property_value(&property, value);
+        data_container.set_global_property_value(&property, value)
     }
 
     #[allow(unused_variables)]
@@ -276,16 +282,37 @@ mod test {
     }
 
     define_global_property!(DiseaseParams, ParamType);
-    //Since global properties aren't mutable right now, only
-    // check that they are properly set
+
     #[test]
     fn set_get_global_property() {
         let params: ParamType = ParamType {
             days: 10,
             diseases: 2,
         };
+        let params2: ParamType = ParamType {
+            days: 11,
+            diseases: 3,
+        };
+
         let mut context = Context::new();
-        context.set_global_property_value(DiseaseParams, params.clone());
+
+        // Set and check the stored value.
+        context
+            .set_global_property_value(DiseaseParams, params.clone())
+            .unwrap();
+        let global_params = context
+            .get_global_property_value(DiseaseParams)
+            .unwrap()
+            .clone();
+        assert_eq!(global_params.days, params.days);
+        assert_eq!(global_params.diseases, params.diseases);
+
+        // Setting again should fail because global properties are immutable.
+        assert!(context
+            .set_global_property_value(DiseaseParams, params2.clone())
+            .is_err());
+
+        // Check that the value is unchanged.
         let global_params = context
             .get_global_property_value(DiseaseParams)
             .unwrap()
@@ -322,7 +349,9 @@ mod test {
             .load_parameters_from_json::<ParamType>(&file_path)
             .unwrap();
 
-        context.set_global_property_value(Parameters, params_json);
+        context
+            .set_global_property_value(Parameters, params_json)
+            .unwrap();
 
         let params_read = context
             .get_global_property_value(Parameters)