make sure we cannot add constant edge properties for layers that don't have updates for that edge

Author: ljeub-pometry

raphtory/src/core/mod.rs

@@ -162,8 +162,11 @@ impl PartialOrd for Prop {
 }
 
 impl Prop {
-    pub fn map(vals: impl IntoIterator<Item = (impl Into<ArcStr>, Prop)>) -> Self {
-        let h_map: FxHashMap<_, _> = vals.into_iter().map(|(k, v)| (k.into(), v)).collect();
+    pub fn map(vals: impl IntoIterator<Item = (impl Into<ArcStr>, impl Into<Prop>)>) -> Self {
+        let h_map: FxHashMap<_, _> = vals
+            .into_iter()
+            .map(|(k, v)| (k.into(), v.into()))
+            .collect();
         Prop::Map(h_map.into())
     }
 

raphtory/src/core/storage/raw_edges.rs

@@ -232,6 +232,19 @@ impl<'a> MutEdge<'a> {
         &mut self.guard.deletions[layer_id][self.i]
     }
 
+    fn has_layer(&self, layer_id: usize) -> bool {
+        if let Some(additions) = self.guard.additions.get(layer_id) {
+            if let Some(additions) = additions.get(self.i) {
+                return !additions.is_empty();
+            }
+        }
+        if let Some(deletions) = self.guard.deletions.get(layer_id) {
+            if let Some(deletions) = deletions.get(self.i) {
+                return !deletions.is_empty();
+            }
+        }
+        false
+    }
     pub fn additions_mut(&mut self, layer_id: usize) -> &mut TimeIndex<TimeIndexEntry> {
         if layer_id >= self.guard.additions.len() {
             self.guard
@@ -254,6 +267,11 @@ impl<'a> MutEdge<'a> {
 
         &mut self.guard.props[layer_id][self.i]
     }
+
+    /// Get a mutable reference to the layer only if it already exists but don't create a new one
+    pub fn get_layer_mut(&mut self, layer_id: usize) -> Option<&mut EdgeLayer> {
+        self.has_layer(layer_id).then(|| self.layer_mut(layer_id))
+    }
 }
 
 #[derive(Debug)]

raphtory/src/db/api/storage/graph/storage_ops/prop_add.rs

@@ -6,10 +6,34 @@ use raphtory_api::core::{
 use super::GraphStorage;
 use crate::{
     core::{entities::graph::tgraph::TemporalGraph, utils::errors::GraphError},
-    db::api::mutation::internal::InternalPropertyAdditionOps,
+    db::api::{
+        mutation::internal::InternalPropertyAdditionOps,
+        storage::graph::edges::edge_storage_ops::{EdgeStorageOps, MemEdge},
+    },
     prelude::Prop,
 };
 
+impl TemporalGraph {
+    fn missing_layer_error(&self, edge: MemEdge, layer_id: usize) -> GraphError {
+        let layer = self.get_layer_name(layer_id).to_string();
+        let src = self
+            .storage
+            .get_node(edge.src())
+            .get_entry()
+            .node()
+            .global_id
+            .to_string();
+        let dst = self
+            .storage
+            .get_node(edge.dst())
+            .get_entry()
+            .node()
+            .global_id
+            .to_string();
+        GraphError::InvalidEdgeLayer { layer, src, dst }
+    }
+}
+
 impl InternalPropertyAdditionOps for TemporalGraph {
     fn internal_add_properties(
         &self,
@@ -87,24 +111,27 @@ impl InternalPropertyAdditionOps for TemporalGraph {
         props: &[(usize, Prop)],
     ) -> Result<(), GraphError> {
         let mut edge = self.storage.get_edge_mut(eid);
-        let mut edge = edge.as_mut();
-        let edge_layer = edge.layer_mut(layer);
-        for (prop_id, prop) in props {
-            let prop = self.process_prop_value(prop);
-            edge_layer
-                .add_constant_prop(*prop_id, prop)
-                .map_err(|err| {
-                    let name = self.edge_meta.get_prop_name(*prop_id, true);
-                    GraphError::ConstantPropertyMutationError {
-                        name,
-                        new: err.new_value.expect("new value exists"),
-                        old: err
-                            .previous_value
-                            .expect("previous value exists if set failed"),
-                    }
-                })?;
+        let mut edge_mut = edge.as_mut();
+        if let Some(edge_layer) = edge_mut.get_layer_mut(layer) {
+            for (prop_id, prop) in props {
+                let prop = self.process_prop_value(prop);
+                edge_layer
+                    .add_constant_prop(*prop_id, prop)
+                    .map_err(|err| {
+                        let name = self.edge_meta.get_prop_name(*prop_id, true);
+                        GraphError::ConstantPropertyMutationError {
+                            name,
+                            new: err.new_value.expect("new value exists"),
+                            old: err
+                                .previous_value
+                                .expect("previous value exists if set failed"),
+                        }
+                    })?;
+            }
+            Ok(())
+        } else {
+            Err(self.missing_layer_error(edge.as_ref(), layer))
         }
-        Ok(())
     }
 
     fn internal_update_constant_edge_properties(
@@ -114,13 +141,16 @@ impl InternalPropertyAdditionOps for TemporalGraph {
         props: &[(usize, Prop)],
     ) -> Result<(), GraphError> {
         let mut edge = self.storage.get_edge_mut(eid);
-        let mut edge = edge.as_mut();
-        let edge_layer = edge.layer_mut(layer);
-        for (prop_id, prop) in props {
-            let prop = self.process_prop_value(prop);
-            edge_layer.update_constant_prop(*prop_id, prop)?;
+        let mut edge_mut = edge.as_mut();
+        if let Some(edge_layer) = edge_mut.get_layer_mut(layer) {
+            for (prop_id, prop) in props {
+                let prop = self.process_prop_value(prop);
+                edge_layer.update_constant_prop(*prop_id, prop)?;
+            }
+            Ok(())
+        } else {
+            Err(self.missing_layer_error(edge.as_ref(), layer))
         }
-        Ok(())
     }
 }
 
@@ -233,4 +263,41 @@ mod test {
             );
         });
     }
+
+    #[test]
+    fn test_constant_edge_prop_updates_multiple_layers() {
+        let graph = Graph::new();
+        graph.add_edge(0, 1, 2, NO_PROPS, Some("1")).unwrap();
+        let edge = graph.edge(1, 2).unwrap();
+        // check that it is not possible to add constant properties for layers that do not have updates
+        assert!(edge
+            .add_constant_properties([("test", "test")], None)
+            .is_err());
+        assert!(edge
+            .add_constant_properties([("test", "test")], Some("2"))
+            .is_err());
+        assert!(edge
+            .update_constant_properties([("test", "test")], None)
+            .is_err());
+        assert!(edge
+            .update_constant_properties([("test", "test")], Some("2"))
+            .is_err());
+
+        // make sure we didn't accidentally create a new layer in the failed updates
+        assert!(graph.layers("2").is_err());
+
+        // make sure constant property updates for existing layers work
+        edge.add_constant_properties([("test", "test")], Some("1"))
+            .unwrap();
+        assert_eq!(
+            edge.properties().constant().get("test"),
+            Some(Prop::map([("1", "test")]))
+        );
+        edge.update_constant_properties([("test", "test2")], Some("1"))
+            .unwrap();
+        assert_eq!(
+            edge.properties().constant().get("test"),
+            Some(Prop::map([("1", "test2")]))
+        );
+    }
 }