address review remarks

Author: Zoran Cvetkov

graph/src/components/store/entity_cache.rs

@@ -1,4 +1,4 @@
-use anyhow::anyhow;
+use anyhow::{anyhow, bail};
 use std::borrow::Borrow;
 use std::collections::HashMap;
 use std::fmt::{self, Debug};
@@ -17,8 +17,8 @@ use super::{BlockNumber, DerivedEntityQuery, LoadRelatedRequest, StoreError};
 
 pub type EntityLfuCache = LfuCache<EntityKey, Option<Arc<Entity>>>;
 
-// Number of VIDs that are reserved ourside of the generated ones here.
-// Currently only 1 for POIs is used, but lets reserve a few more.
+// Number of VIDs that are reserved outside of the generated ones here.
+// Currently none is used, but lets reserve a few more.
 const RESERVED_VIDS: u32 = 100;
 
 /// The scope in which the `EntityCache` should perform a `get` operation
@@ -374,7 +374,9 @@ impl EntityCache {
         let mut entity = entity;
         let old_vid = entity.set_vid(vid).expect("the vid should be set");
         // Make sure that there was no VID previously set for this entity.
-        assert!(old_vid.is_none());
+        if old_vid.is_some() {
+            bail!("VID was already present when set in EntityCache");
+        }
 
         self.entity_op(key.clone(), EntityOp::Update(entity));
 

graph/src/data/store/mod.rs

@@ -3,7 +3,7 @@ use crate::{
     derive::CacheWeight,
     prelude::{lazy_static, q, r, s, CacheWeight, QueryExecutionError},
     runtime::gas::{Gas, GasSizeOf},
-    schema::{EntityKey, EntityType},
+    schema::{input::VID_FIELD, EntityKey, EntityType},
     util::intern::{self, AtomPool},
     util::intern::{Error as InternError, NullValue, Object},
 };
@@ -913,21 +913,21 @@ 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.get("vid")
+        self.get(VID_FIELD)
             .expect("the vid is set")
             .as_int8()
             .expect("the vid is set to a valid value")
     }
 
     /// Sets the VID of the entity. The previous one is returned.
     pub fn set_vid(&mut self, value: i64) -> Result<Option<Value>, InternError> {
-        self.0.insert("vid", value.into())
+        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");
+        let vid = self.get(VID_FIELD);
         if vid.is_none() {
             let _ = self.set_vid(100).expect("the vid should be set");
         }

graph/src/data/subgraph/api_version.rs

@@ -54,9 +54,10 @@ pub const SPEC_VERSION_1_1_0: Version = Version::new(1, 1, 0);
 // Enables eth call declarations and indexed arguments(topics) filtering in manifest
 pub const SPEC_VERSION_1_2_0: Version = Version::new(1, 2, 0);
 
-// Enables subgraphs as datasource
-// Change the way the VID field is generated. It used to be autoincrement. Now its
-// based on block number and the sequence of the entities in a block.
+// Enables subgraphs as datasource.
+// Changes the way the VID field is generated. It used to be autoincrement. Now its
+// based on block number and the order of the entities in a block. The later is
+// representing the writting order of all entity types in a subgraph.
 pub const SPEC_VERSION_1_3_0: Version = Version::new(1, 3, 0);
 
 // The latest spec version available

graph/src/schema/input/mod.rs

@@ -35,7 +35,7 @@ pub(crate) const POI_OBJECT: &str = "Poi$";
 const POI_DIGEST: &str = "digest";
 /// The name of the PoI attribute for storing the block time
 const POI_BLOCK_TIME: &str = "blockTime";
-const VID_FIELD: &str = "vid";
+pub(crate) const VID_FIELD: &str = "vid";
 
 pub mod kw {
     pub const ENTITY: &str = "entity";

graph/src/schema/mod.rs

@@ -21,7 +21,7 @@ pub mod ast;
 mod entity_key;
 mod entity_type;
 mod fulltext;
-mod input;
+pub(crate) mod input;
 
 pub use api::{is_introspection_field, APISchemaError, INTROSPECTION_QUERY_TYPE};
 

store/postgres/src/relational/ddl.rs

@@ -116,10 +116,10 @@ impl Table {
             Ok(cols)
         }
 
-        let vid_type = if !self.object.is_object_type() {
-            "bigserial"
-        } else {
+        let vid_type = if self.object.is_object_type() {
             "bigint"
+        } else {
+            "bigserial"
         };
 
         if self.immutable {

store/postgres/src/relational_queries.rs

@@ -38,7 +38,7 @@ use std::string::ToString;
 use crate::block_range::{BoundSide, EntityBlockRange};
 use crate::relational::{
     Column, ColumnType, Layout, SqlName, Table, BYTE_ARRAY_PREFIX_SIZE, PRIMARY_KEY_COLUMN,
-    STRING_PREFIX_SIZE,
+    STRING_PREFIX_SIZE, VID_COLUMN,
 };
 use crate::{
     block_range::{
@@ -539,11 +539,11 @@ impl EntityData {
                     // table column; those will be things like the
                     // block_range that `select *` pulls in but that we
                     // don't care about here
-                    if key == "vid" {
+                    if key == VID_COLUMN {
                         // VID is not in the input schema but we need it, so deserialize it too
                         match T::Value::from_column_value(&ColumnType::Int8, json) {
                             Ok(value) if value.is_null() => None,
-                            Ok(value) => Some(Ok((Word::from("vid"), value))),
+                            Ok(value) => Some(Ok((Word::from(VID_COLUMN), value))),
                             Err(e) => Some(Err(e)),
                         }
                     } else if let Some(column) = table.column(&SqlName::verbatim(key)) {