Validate attributes installed after open. (#538) r=emily

Make AttributeBuilder optionally helpful, fix tests.

db/src/db.rs

@@ -1421,6 +1421,24 @@ mod tests {
                           [200 :db.schema/attribute 101]]");
     }
 
+    // Unique is required!
+    #[test]
+    fn test_upsert_issue_538() {
+        let mut conn = TestConn::default();
+        assert_transact!(conn, "
+            [{:db/ident :person/name
+              :db/valueType :db.type/string
+              :db/cardinality :db.cardinality/many}
+             {:db/ident :person/age
+              :db/valueType :db.type/long
+              :db/cardinality :db.cardinality/one}
+             {:db/ident :person/email
+              :db/valueType :db.type/string
+              :db/unique :db.unique/identity
+              :db/cardinality :db.cardinality/many}]",
+              Err("bad schema assertion: :db/unique :db/unique_identity without :db/index true for entid: 65538"));
+    }
+
     // TODO: don't use :db/ident to test upserts!
     #[test]
     fn test_upsert_vector() {

db/src/lib.rs

@@ -53,7 +53,10 @@ pub use bootstrap::{
     USER0,
 };
 
-pub use schema::AttributeBuilder;
+pub use schema::{
+    AttributeBuilder,
+    AttributeValidation,
+};
 
 pub use bootstrap::{
     CORE_SCHEMA_VERSION,

db/src/metadata.rs

@@ -47,8 +47,10 @@ use mentat_core::{
     TypedValue,
     ValueType,
 };
+
 use schema::{
     AttributeBuilder,
+    AttributeValidation,
 };
 
 /// An alteration to an attribute.
@@ -181,14 +183,20 @@ pub fn update_attribute_map_from_entid_triples<U>(attribute_map: &mut AttributeM
     for (entid, builder) in builders.into_iter() {
         match attribute_map.entry(entid) {
             Entry::Vacant(entry) => {
+                // Validate once…
                 builder.validate_install_attribute()
-                    .chain_err(|| ErrorKind::BadSchemaAssertion(format!("Schema alteration for new attribute with entid {} is not valid", entid)))?;
+                       .chain_err(|| ErrorKind::BadSchemaAssertion(format!("Schema alteration for new attribute with entid {} is not valid", entid)))?;
+
+                // … and twice, now we have the Attribute.
+                let a = builder.build();
+                a.validate(|| entid.to_string())?;
                 entry.insert(builder.build());
                 attributes_installed.insert(entid);
             },
+
             Entry::Occupied(mut entry) => {
                 builder.validate_alter_attribute()
-                    .chain_err(|| ErrorKind::BadSchemaAssertion(format!("Schema alteration for existing attribute with entid {} is not valid", entid)))?;
+                       .chain_err(|| ErrorKind::BadSchemaAssertion(format!("Schema alteration for existing attribute with entid {} is not valid", entid)))?;
                 let mutations = builder.mutate(entry.get_mut());
                 attributes_altered.insert(entid, mutations);
             },

db/src/schema.rs

@@ -32,35 +32,48 @@ use metadata::{
     AttributeAlteration,
 };
 
-/// Return `Ok(())` if `attribute_map` defines a valid Mentat schema.
+pub trait AttributeValidation {
-fn validate_attribute_map(entid_map: &EntidMap, attribute_map: &AttributeMap) -> Result<()> {
+    fn validate<F>(&self, ident: F) -> Result<()> where F: Fn() -> String;
-    for (entid, attribute) in attribute_map {
+}
-        let ident = || entid_map.get(entid).map(|ident| ident.to_string()).unwrap_or(entid.to_string());
+
-        if attribute.unique == Some(attribute::Unique::Value) && !attribute.index {
+impl AttributeValidation for Attribute {
+    fn validate<F>(&self, ident: F) -> Result<()> where F: Fn() -> String {
+        if self.unique == Some(attribute::Unique::Value) && !self.index {
             bail!(ErrorKind::BadSchemaAssertion(format!(":db/unique :db/unique_value without :db/index true for entid: {}", ident())))
         }
-        if attribute.unique == Some(attribute::Unique::Identity) && !attribute.index {
+        if self.unique == Some(attribute::Unique::Identity) && !self.index {
+    println!("Unique identity without index. Bailing.");
             bail!(ErrorKind::BadSchemaAssertion(format!(":db/unique :db/unique_identity without :db/index true for entid: {}", ident())))
         }
-        if attribute.fulltext && attribute.value_type != ValueType::String {
+        if self.fulltext && self.value_type != ValueType::String {
             bail!(ErrorKind::BadSchemaAssertion(format!(":db/fulltext true without :db/valueType :db.type/string for entid: {}", ident())))
         }
-        if attribute.fulltext && !attribute.index {
+        if self.fulltext && !self.index {
             bail!(ErrorKind::BadSchemaAssertion(format!(":db/fulltext true without :db/index true for entid: {}", ident())))
         }
-        if attribute.component && attribute.value_type != ValueType::Ref {
+        if self.component && self.value_type != ValueType::Ref {
             bail!(ErrorKind::BadSchemaAssertion(format!(":db/isComponent true without :db/valueType :db.type/ref for entid: {}", ident())))
         }
         // TODO: consider warning if we have :db/index true for :db/valueType :db.type/string,
         // since this may be inefficient.  More generally, we should try to drive complex
         // :db/valueType (string, uri, json in the future) users to opt-in to some hash-indexing
         // scheme, as discussed in https://github.com/mozilla/mentat/issues/69.
+        Ok(())
+    }
+}
+
+/// Return `Ok(())` if `attribute_map` defines a valid Mentat schema.
+fn validate_attribute_map(entid_map: &EntidMap, attribute_map: &AttributeMap) -> Result<()> {
+    for (entid, attribute) in attribute_map {
+        let ident = || entid_map.get(entid).map(|ident| ident.to_string()).unwrap_or(entid.to_string());
+        attribute.validate(ident)?;
     }
     Ok(())
 }
 
 #[derive(Clone,Debug,Default,Eq,Hash,Ord,PartialOrd,PartialEq)]
 pub struct AttributeBuilder {
+    helpful: bool,
     value_type: Option<ValueType>,
     multival: Option<bool>,
     unique: Option<Option<attribute::Unique>>,
@@ -70,6 +83,15 @@ pub struct AttributeBuilder {
 }
 
 impl AttributeBuilder {
+    /// Make a new AttributeBuilder for human consumption: it will help you
+    /// by flipping relevant flags.
+    pub fn new() -> Self {
+        AttributeBuilder {
+            helpful: true,
+            ..Default::default()
+        }
+    }
+
     pub fn value_type<'a>(&'a mut self, value_type: ValueType) -> &'a mut Self {
         self.value_type = Some(value_type);
         self
@@ -81,6 +103,9 @@ impl AttributeBuilder {
     }
 
     pub fn unique<'a>(&'a mut self, unique: attribute::Unique) -> &'a mut Self {
+        if self.helpful && unique == attribute::Unique::Identity {
+            self.index = Some(true);
+        }
         self.unique = Some(Some(unique));
         self
     }
@@ -92,6 +117,9 @@ impl AttributeBuilder {
 
     pub fn fulltext<'a>(&'a mut self, fulltext: bool) -> &'a mut Self {
         self.fulltext = Some(fulltext);
+        if self.helpful && fulltext {
+            self.index = Some(true);
+        }
         self
     }
 

query-algebrizer/src/clauses/pattern.rs

@@ -754,6 +754,7 @@ mod testing {
         associate_ident(&mut schema, NamespacedKeyword::new("foo", "bar"), 99);
         add_attribute(&mut schema, 99, Attribute {
             value_type: ValueType::String,
+            index: true,
             fulltext: true,
             ..Default::default()
         });

query-algebrizer/tests/fulltext.rs

@@ -45,6 +45,7 @@ fn prepopulated_schema() -> Schema {
     });
     add_attribute(&mut schema, 66, Attribute {
         value_type: ValueType::String,
+        index: true,
         fulltext: true,
         multival: true,
         ..Default::default()

src/vocabulary.rs

@@ -66,7 +66,7 @@
 //!             version: 1,
 //!             attributes: vec![
 //!                 (kw!(:link/title),
-//!                  vocabulary::AttributeBuilder::default()
+//!                  vocabulary::AttributeBuilder::new()
 //!                    .value_type(ValueType::String)
 //!                    .multival(false)
 //!                    .fulltext(true)

tests/query.rs

@@ -339,6 +339,7 @@ fn test_fulltext() {
         [:db/add "s" :db/ident :foo/fts]
         [:db/add "s" :db/valueType :db.type/string]
         [:db/add "s" :db/fulltext true]
+        [:db/add "s" :db/index true]
         [:db/add "s" :db/cardinality :db.cardinality/many]
     ]"#).unwrap();
 

tests/vocabulary.rs

@@ -31,6 +31,9 @@ use mentat_core::{
     HasSchema,
 };
 
+// To check our working.
+use mentat_db::AttributeValidation;
+
 use mentat::{
     Conn,
     NamespacedKeyword,
@@ -61,13 +64,13 @@ lazy_static! {
             version: 1,
             attributes: vec![
                 (FOO_NAME.clone(),
-                vocabulary::AttributeBuilder::default()
+                vocabulary::AttributeBuilder::new()
                     .value_type(ValueType::String)
                     .multival(false)
                     .unique(vocabulary::attribute::Unique::Identity)
                     .build()),
                 (FOO_MOMENT.clone(),
-                vocabulary::AttributeBuilder::default()
+                vocabulary::AttributeBuilder::new()
                     .value_type(ValueType::Instant)
                     .multival(false)
                     .index(true)
@@ -117,14 +120,34 @@ fn test_real_world() {
                vec![vec![alice, now.clone()], vec![barbara, now.clone()]]);
 }
 
+#[test]
+fn test_default_attributebuilder_complains() {
+    // ::new is helpful. ::default is not.
+    assert!(vocabulary::AttributeBuilder::default()
+                  .value_type(ValueType::String)
+                  .multival(true)
+                  .fulltext(true)
+                  .build()
+                  .validate(|| "Foo".to_string())
+                  .is_err());
+
+    assert!(vocabulary::AttributeBuilder::new()
+                  .value_type(ValueType::String)
+                  .multival(true)
+                  .fulltext(true)
+                  .build()
+                  .validate(|| "Foo".to_string())
+                  .is_ok());
+}
+
 #[test]
 fn test_add_vocab() {
-    let bar = vocabulary::AttributeBuilder::default()
+    let bar = vocabulary::AttributeBuilder::new()
                   .value_type(ValueType::Instant)
                   .multival(false)
                   .index(true)
                   .build();
-    let baz = vocabulary::AttributeBuilder::default()
+    let baz = vocabulary::AttributeBuilder::new()
                   .value_type(ValueType::String)
                   .multival(true)
                   .fulltext(true)