Revert "Make VID element internal to the Entity (#5857)"

This reverts commit 6bbd0d2.

Author: Zoran Cvetkov

graph/src/components/store/entity_cache.rs

@@ -213,7 +213,18 @@ impl EntityCache {
         // always creates it in a new style.
         debug_assert!(match scope {
             GetScope::Store => {
-                entity == self.store.get(key).unwrap().map(Arc::new)
+                // Release build will never call this function and hence it's OK
+                // when that implementation is not correct.
+                fn remove_vid(entity: Option<Arc<Entity>>) -> Option<Entity> {
+                    entity.map(|e| {
+                        #[allow(unused_mut)]
+                        let mut entity = (*e).clone();
+                        #[cfg(debug_assertions)]
+                        entity.remove("vid");
+                        entity
+                    })
+                }
+                remove_vid(entity.clone()) == remove_vid(self.store.get(key).unwrap().map(Arc::new))
             }
             GetScope::InBlock => true,
         });

graph/src/data/store/mod.rs

@@ -740,17 +740,16 @@ lazy_static! {
 }
 
 /// An entity is represented as a map of attribute names to values.
-#[derive(Clone, CacheWeight, Eq, Serialize)]
+#[derive(Clone, CacheWeight, PartialEq, Eq, Serialize)]
 pub struct Entity(Object<Value>);
 
 impl<'a> IntoIterator for &'a Entity {
-    type Item = (&'a str, &'a Value);
+    type Item = (Word, Value);
 
-    type IntoIter =
-        std::iter::Filter<intern::ObjectIter<'a, Value>, fn(&(&'a str, &'a Value)) -> bool>;
+    type IntoIter = intern::ObjectOwningIter<Value>;
 
     fn into_iter(self) -> Self::IntoIter {
-        (&self.0).into_iter().filter(|(k, _)| *k != VID_FIELD)
+        self.0.clone().into_iter()
     }
 }
 
@@ -875,18 +874,10 @@ impl Entity {
     }
 
     pub fn get(&self, key: &str) -> Option<&Value> {
-        // VID field is private and not visible outside
-        if key == VID_FIELD {
-            return None;
-        }
         self.0.get(key)
     }
 
     pub fn contains_key(&self, key: &str) -> bool {
-        // VID field is private and not visible outside
-        if key == VID_FIELD {
-            return false;
-        }
         self.0.contains_key(key)
     }
 
@@ -924,8 +915,7 @@ impl Entity {
     /// Return the VID of this entity and if its missing or of a type different than
     /// i64 it panics.
     pub fn vid(&self) -> i64 {
-        self.0
-            .get(VID_FIELD)
+        self.get(VID_FIELD)
             .expect("the vid must be set")
             .as_int8()
             .expect("the vid must be set to a valid value")
@@ -936,6 +926,15 @@ impl Entity {
         self.0.insert(VID_FIELD, value.into())
     }
 
+    /// Sets the VID if it's not already set. Should be used only for tests.
+    #[cfg(debug_assertions)]
+    pub fn set_vid_if_empty(&mut self) {
+        let vid = self.get(VID_FIELD);
+        if vid.is_none() {
+            let _ = self.set_vid(100).expect("the vid should be set");
+        }
+    }
+
     /// Merges an entity update `update` into this entity.
     ///
     /// If a key exists in both entities, the value from `update` is chosen.
@@ -1059,13 +1058,6 @@ impl Entity {
     }
 }
 
-/// Checks equality of two entities while ignoring the VID fields
-impl PartialEq for Entity {
-    fn eq(&self, other: &Self) -> bool {
-        self.0.eq_ignore_key(&other.0, VID_FIELD)
-    }
-}
-
 /// Convenience methods to modify individual attributes for tests.
 /// Production code should not use/need this.
 #[cfg(debug_assertions)]
@@ -1085,14 +1077,6 @@ impl Entity {
     ) -> Result<Option<Value>, InternError> {
         self.0.insert(name, value.into())
     }
-
-    /// Sets the VID if it's not already set. Should be used only for tests.
-    pub fn set_vid_if_empty(&mut self) {
-        let vid = self.0.get(VID_FIELD);
-        if vid.is_none() {
-            let _ = self.set_vid(100).expect("the vid should be set");
-        }
-    }
 }
 
 impl<'a> From<&'a Entity> for Cow<'a, Entity> {
@@ -1284,47 +1268,3 @@ fn fmt_debug() {
     let bi = Value::BigInt(scalar::BigInt::from(-17i32));
     assert_eq!("BigInt(-17)", format!("{:?}", bi));
 }
-
-#[test]
-fn entity_hidden_vid() {
-    use crate::schema::InputSchema;
-    let subgraph_id = "oneInterfaceOneEntity";
-    let document = "type Thing @entity {id: ID!, name: String!}";
-    let schema = InputSchema::raw(document, subgraph_id);
-
-    let entity = entity! { schema => id: "1", name: "test", vid: 3i64 };
-    let debug_str = format!("{:?}", entity);
-    let entity_str = "Entity { id: String(\"1\"), name: String(\"test\"), vid: Int8(3) }";
-    assert_eq!(debug_str, entity_str);
-
-    // get returns nothing...
-    assert_eq!(entity.get(VID_FIELD), None);
-    assert_eq!(entity.contains_key(VID_FIELD), false);
-    // ...while vid is present
-    assert_eq!(entity.vid(), 3i64);
-
-    // into_iter() misses it too
-    let mut it = entity.into_iter();
-    assert_eq!(Some(("id", &Value::String("1".to_string()))), it.next());
-    assert_eq!(
-        Some(("name", &Value::String("test".to_string()))),
-        it.next()
-    );
-    assert_eq!(None, it.next());
-
-    let mut entity2 = entity! { schema => id: "1", name: "test", vid: 5i64 };
-    assert_eq!(entity2.vid(), 5i64);
-    // equal with different vid
-    assert_eq!(entity, entity2);
-
-    entity2.remove(VID_FIELD);
-    // equal if one has no vid
-    assert_eq!(entity, entity2);
-    let debug_str2 = format!("{:?}", entity2);
-    let entity_str2 = "Entity { id: String(\"1\"), name: String(\"test\") }";
-    assert_eq!(debug_str2, entity_str2);
-
-    // set again
-    _ = entity2.set_vid(7i64);
-    assert_eq!(entity2.vid(), 7i64);
-}

graph/src/util/intern.rs

@@ -308,45 +308,6 @@ impl<V> Object<V> {
     }
 }
 
-impl<V: PartialEq> Object<V> {
-    fn len_ignore_atom(&self, atom: &Atom) -> usize {
-        // Because of tombstones and the ignored atom, we can't just return `self.entries.len()`.
-        self.entries
-            .iter()
-            .filter(|entry| entry.key != TOMBSTONE_KEY && entry.key != *atom)
-            .count()
-    }
-
-    /// Check for equality while ignoring one particular element
-    pub fn eq_ignore_key(&self, other: &Self, ignore_key: &str) -> bool {
-        let ignore = self.pool.lookup(ignore_key);
-        let len1 = if let Some(to_ignore) = ignore {
-            self.len_ignore_atom(&to_ignore)
-        } else {
-            self.len()
-        };
-        let len2 = if let Some(to_ignore) = other.pool.lookup(ignore_key) {
-            other.len_ignore_atom(&to_ignore)
-        } else {
-            other.len()
-        };
-        if len1 != len2 {
-            return false;
-        }
-
-        if self.same_pool(other) {
-            self.entries
-                .iter()
-                .filter(|e| e.key != TOMBSTONE_KEY && ignore.map_or(true, |ig| e.key != ig))
-                .all(|Entry { key, value }| other.get_by_atom(key).map_or(false, |o| o == value))
-        } else {
-            self.iter()
-                .filter(|(key, _)| *key != ignore_key)
-                .all(|(key, value)| other.get(key).map_or(false, |o| o == value))
-        }
-    }
-}
-
 impl<V: NullValue> Object<V> {
     /// Remove `key` from the object and return the value that was
     /// associated with the `key`. The entry is actually not removed for