graph, store: address review comments

- Remove logs and the logger from the RowGroup
- Simplify schema tests
- Add ObjectMutability enum instead of two booleans

Author: isum

graph/examples/append_row.rs

@@ -104,12 +104,7 @@ pub fn main() -> anyhow::Result<()> {
             };
             mods.push(md);
         }
-        let mut group = RowGroup::new(
-            THING_TYPE.clone(),
-            false,
-            false,
-            slog::Logger::root(slog::Discard, slog::o!()),
-        );
+        let mut group = RowGroup::new(THING_TYPE.clone());
 
         let start = Instant::now();
         for md in mods {

graph/src/components/store/write.rs

@@ -4,7 +4,7 @@ use std::{
     sync::Arc,
 };
 
-use slog::{warn, Logger};
+use slog::Logger;
 
 use crate::{
     blockchain::{block_stream::FirehoseCursor, BlockPtr, BlockTime},
@@ -340,33 +340,22 @@ impl LastMod {
 #[derive(Debug, CacheWeight)]
 pub struct RowGroup {
     pub entity_type: EntityType,
+
     /// All changes for this entity type, ordered by block; i.e., if `i < j`
     /// then `rows[i].block() <= rows[j].block()`. Several methods on this
     /// struct rely on the fact that this ordering is observed.
     rows: Vec<EntityModification>,
 
-    immutable: bool,
-    skip_duplicates: bool,
-    logger: Logger,
-
     /// Map the `key.entity_id` of all entries in `rows` to the index with
     /// the most recent entry for that id to speed up lookups.
     last_mod: LastMod,
 }
 
 impl RowGroup {
-    pub fn new(
-        entity_type: EntityType,
-        immutable: bool,
-        skip_duplicates: bool,
-        logger: Logger,
-    ) -> Self {
+    pub fn new(entity_type: EntityType) -> Self {
         Self {
             entity_type,
             rows: Vec::new(),
-            immutable,
-            skip_duplicates,
-            logger,
             last_mod: LastMod::new(),
         }
     }
@@ -482,7 +471,7 @@ impl RowGroup {
     /// Append `row` to `self.rows` by combining it with a previously
     /// existing row, if that is possible
     fn append_row(&mut self, row: EntityModification) -> Result<(), StoreError> {
-        if self.immutable {
+        if self.entity_type.is_immutable() {
             match row {
                 EntityModification::Insert { .. } => {
                     // Check if this is an attempt to overwrite an immutable
@@ -499,11 +488,7 @@ impl RowGroup {
                         .and_then(|&idx| self.rows.get(idx))
                     {
                         Some(prev) if prev.block() != row.block() => {
-                            if self.skip_duplicates {
-                                warn!(self.logger, "Skipping duplicate insert for immutable entity";
-                                    "entity" => row.key().to_string(),
-                                    "block" => row.block(),
-                                    "previous_block" => prev.block());
+                            if self.entity_type.skip_duplicates() {
                                 return Ok(());
                             }
                             return Err(StoreError::Input(
@@ -516,10 +501,7 @@ impl RowGroup {
                     self.push_row(row);
                 }
                 EntityModification::Overwrite { .. } | EntityModification::Remove { .. } => {
-                    if self.skip_duplicates {
-                        warn!(self.logger, "Skipping unsupported operation for immutable entity";
-                            "entity_type" => self.entity_type.to_string(),
-                            "operation" => format!("{:?}", row));
+                    if self.entity_type.skip_duplicates() {
                         return Ok(());
                     }
                     return Err(internal_error!(
@@ -628,18 +610,8 @@ impl RowGroup {
 pub struct RowGroupForPerfTest(RowGroup);
 
 impl RowGroupForPerfTest {
-    pub fn new(
-        entity_type: EntityType,
-        immutable: bool,
-        skip_duplicates: bool,
-        logger: Logger,
-    ) -> Self {
-        Self(RowGroup::new(
-            entity_type,
-            immutable,
-            skip_duplicates,
-            logger,
-        ))
+    pub fn new(entity_type: EntityType) -> Self {
+        Self(RowGroup::new(entity_type))
     }
 
     pub fn push(&mut self, emod: EntityModification, block: BlockNumber) -> Result<(), StoreError> {
@@ -722,14 +694,7 @@ impl RowGroups {
         match pos {
             Some(pos) => &mut self.groups[pos],
             None => {
-                let immutable = entity_type.is_immutable();
-                let skip_duplicates = entity_type.skip_duplicates();
-                self.groups.push(RowGroup::new(
-                    entity_type.clone(),
-                    immutable,
-                    skip_duplicates,
-                    self.logger.clone(),
-                ));
+                self.groups.push(RowGroup::new(entity_type.clone()));
                 // unwrap: we just pushed an entry
                 self.groups.last_mut().unwrap()
             }
@@ -1092,8 +1057,6 @@ mod test {
     };
     use lazy_static::lazy_static;
 
-    use slog::Logger;
-
     use super::{LastMod, RowGroup};
 
     #[track_caller]
@@ -1125,9 +1088,6 @@ mod test {
         let group = RowGroup {
             entity_type: ENTRY_TYPE.clone(),
             rows,
-            immutable: false,
-            skip_duplicates: false,
-            logger: Logger::root(slog::Discard, slog::o!()),
             last_mod,
         };
         let act = group
@@ -1239,12 +1199,7 @@ mod test {
     impl Group {
         fn new() -> Self {
             Self {
-                group: RowGroup::new(
-                    THING_TYPE.clone(),
-                    false,
-                    false,
-                    Logger::root(slog::Discard, slog::o!()),
-                ),
+                group: RowGroup::new(THING_TYPE.clone()),
             }
         }
 
@@ -1379,13 +1334,9 @@ mod test {
         }
     }
 
-    fn discard_logger() -> Logger {
-        Logger::root(slog::Discard, slog::o!())
-    }
-
     #[test]
     fn append_row_immutable_default_rejects_cross_block_duplicate() {
-        let mut group = RowGroup::new(IMM_THING_TYPE.clone(), true, false, discard_logger());
+        let mut group = RowGroup::new(IMM_THING_TYPE.clone());
         let res = group.push(make_insert(&IMM_THING_TYPE, "one", 1), 1);
         assert!(res.is_ok());
         let res = group.push(make_insert(&IMM_THING_TYPE, "one", 2), 2);
@@ -1394,7 +1345,7 @@ mod test {
 
     #[test]
     fn append_row_skip_duplicates_allows_cross_block_duplicate() {
-        let mut group = RowGroup::new(SKIP_DUP_THING_TYPE.clone(), true, true, discard_logger());
+        let mut group = RowGroup::new(SKIP_DUP_THING_TYPE.clone());
         let res = group.push(make_insert(&SKIP_DUP_THING_TYPE, "one", 1), 1);
         assert!(res.is_ok());
         let res = group.push(make_insert(&SKIP_DUP_THING_TYPE, "one", 2), 2);
@@ -1404,21 +1355,21 @@ mod test {
 
     #[test]
     fn append_row_skip_duplicates_allows_overwrite() {
-        let mut group = RowGroup::new(SKIP_DUP_THING_TYPE.clone(), true, true, discard_logger());
+        let mut group = RowGroup::new(SKIP_DUP_THING_TYPE.clone());
         let res = group.append_row(make_overwrite(&SKIP_DUP_THING_TYPE, "one", 1));
         assert!(res.is_ok());
     }
 
     #[test]
     fn append_row_skip_duplicates_allows_remove() {
-        let mut group = RowGroup::new(SKIP_DUP_THING_TYPE.clone(), true, true, discard_logger());
+        let mut group = RowGroup::new(SKIP_DUP_THING_TYPE.clone());
         let res = group.append_row(make_remove(&SKIP_DUP_THING_TYPE, "one", 1));
         assert!(res.is_ok());
     }
 
     #[test]
     fn append_row_skip_duplicates_same_block_still_pushes() {
-        let mut group = RowGroup::new(SKIP_DUP_THING_TYPE.clone(), true, true, discard_logger());
+        let mut group = RowGroup::new(SKIP_DUP_THING_TYPE.clone());
         let res = group.push(make_insert(&SKIP_DUP_THING_TYPE, "one", 1), 1);
         assert!(res.is_ok());
         let res = group.push(make_insert(&SKIP_DUP_THING_TYPE, "one", 1), 1);

graph/src/schema/input/mod.rs

@@ -126,15 +126,22 @@ impl TypeInfo {
 
     fn is_immutable(&self) -> bool {
         match self {
-            TypeInfo::Object(obj_type) => obj_type.immutable,
+            TypeInfo::Object(obj_type) => {
+                matches!(obj_type.mutability, ObjectMutability::Immutable { .. })
+            }
             TypeInfo::Interface(_) => false,
             TypeInfo::Aggregation(_) => true,
         }
     }
 
     fn skip_duplicates(&self) -> bool {
         match self {
-            TypeInfo::Object(obj_type) => obj_type.immutable && obj_type.skip_duplicates,
+            TypeInfo::Object(obj_type) => matches!(
+                obj_type.mutability,
+                ObjectMutability::Immutable {
+                    skip_duplicates: true
+                }
+            ),
             TypeInfo::Interface(_) => false,
             TypeInfo::Aggregation(_) => false,
         }
@@ -418,12 +425,11 @@ pub struct ObjectType {
     pub name: Atom,
     pub id_type: IdType,
     pub fields: Box<[Field]>,
-    pub immutable: bool,
+    pub mutability: ObjectMutability,
     /// The name of the aggregation to which this object type belongs if it
     /// is part of an aggregation
     aggregation: Option<Atom>,
     pub timeseries: bool,
-    pub skip_duplicates: bool,
     interfaces: Box<[Word]>,
     shared_interfaces: Box<[Atom]>,
 }
@@ -458,23 +464,30 @@ impl ObjectType {
             None => false,
             _ => unreachable!("validations ensure we don't get here"),
         };
-        let immutable = match dir.argument("immutable") {
+        let immutable = match dir.argument(kw::IMMUTABLE) {
             Some(Value::Boolean(im)) => *im,
             None => timeseries,
             _ => unreachable!("validations ensure we don't get here"),
         };
-        let skip_duplicates = match dir.argument(kw::SKIP_DUPLICATES) {
-            Some(Value::Boolean(sd)) => *sd,
-            _ => false,
+
+        let mutability = if immutable {
+            let skip_duplicates = match dir.argument(kw::SKIP_DUPLICATES) {
+                Some(Value::Boolean(sd)) => *sd,
+                _ => false,
+            };
+
+            ObjectMutability::Immutable { skip_duplicates }
+        } else {
+            ObjectMutability::Mutable
         };
+
         Self {
             name,
             fields,
             id_type,
-            immutable,
+            mutability,
             aggregation: None,
             timeseries,
-            skip_duplicates,
             interfaces,
             shared_interfaces,
         }
@@ -503,10 +516,9 @@ impl ObjectType {
             name,
             interfaces: Box::new([]),
             id_type: IdType::String,
-            immutable: false,
+            mutability: ObjectMutability::Mutable,
             aggregation: None,
             timeseries: false,
-            skip_duplicates: false,
             fields,
             shared_interfaces: Box::new([]),
         }
@@ -522,6 +534,12 @@ impl ObjectType {
     }
 }
 
+#[derive(Debug, Clone, Copy, PartialEq)]
+pub enum ObjectMutability {
+    Mutable,
+    Immutable { skip_duplicates: bool },
+}
+
 #[derive(PartialEq, Debug)]
 pub struct InterfaceType {
     pub name: Atom,
@@ -902,10 +920,11 @@ impl Aggregation {
                         .cloned()
                         .chain(aggregates.iter().map(Aggregate::as_agg_field))
                         .collect(),
-                    immutable: true,
+                    mutability: ObjectMutability::Immutable {
+                        skip_duplicates: false,
+                    },
                     aggregation: Some(name),
                     timeseries: false,
-                    skip_duplicates: false,
                     interfaces: Box::new([]),
                     shared_interfaces: Box::new([]),
                 }
@@ -1368,7 +1387,7 @@ impl InputSchema {
                 TypeInfo::Object(obj_type) => Some(obj_type),
                 TypeInfo::Interface(_) | TypeInfo::Aggregation(_) => None,
             })
-            .filter(|obj_type| obj_type.immutable)
+            .filter(|obj_type| matches!(obj_type.mutability, ObjectMutability::Immutable { .. }))
             .map(|obj_type| EntityType::new(self.cheap_clone(), obj_type.name))
     }
 
@@ -3187,43 +3206,6 @@ type Gravatar @entity {
                 }
             }
         }
-
-        #[test]
-        fn validate_entity_directives_skip_duplicates_non_boolean() {
-            let schema =
-                parse("type Foo @entity(immutable: true, skipDuplicates: \"yes\") { id: ID! }");
-            let errors = validate(&schema).unwrap_err();
-            assert!(
-                errors.contains(&SchemaValidationError::EntityDirectiveNonBooleanArgValue(
-                    "skipDuplicates".to_string()
-                )),
-                "expected EntityDirectiveNonBooleanArgValue for non-boolean skipDuplicates, got: {errors:?}"
-            );
-        }
-
-        #[test]
-        fn validate_entity_directives_skip_duplicates_requires_immutable() {
-            let schema = parse("type Foo @entity(skipDuplicates: true) { id: ID! }");
-            let errors = validate(&schema).unwrap_err();
-            assert!(
-                errors.contains(&SchemaValidationError::SkipDuplicatesRequiresImmutable(
-                    "Foo".to_string()
-                )),
-                "expected SkipDuplicatesRequiresImmutable for mutable entity, got: {errors:?}"
-            );
-        }
-
-        #[test]
-        fn validate_entity_directives_timeseries_skip_duplicates_valid() {
-            let schema = parse(
-                "type Foo @entity(timeseries: true, skipDuplicates: true) { id: Int8! timestamp: Timestamp! }",
-            );
-            let result = validate(&schema);
-            assert!(
-                result.is_ok(),
-                "expected timeseries + skipDuplicates to pass validation, got: {result:?}"
-            );
-        }
     }
 }
 

graph/src/schema/test_schemas/skip_duplicates_not_a_boolean.graphql

@@ -0,0 +1,5 @@
+# fail: EntityDirectiveNonBooleanArgValue("skipDuplicates")
+
+type Data @entity(immutable: true, skipDuplicates: "yes") {
+  id: Bytes!
+}

graph/src/schema/test_schemas/skip_duplicates_requires_immutable.graphql

@@ -0,0 +1,5 @@
+# fail: SkipDuplicatesRequiresImmutable("Data")
+
+type Data @entity(skipDuplicates: true) {
+  id: Bytes!
+}

graph/src/schema/test_schemas/skip_duplicates_valid.graphql

@@ -0,0 +1,5 @@
+# valid: Minimal example
+
+type Data @entity(immutable: true, skipDuplicates: true) {
+  id: Bytes!
+}