diff --git a/db/src/db.rs b/db/src/db.rs index 7df3138e..68f96966 100644 --- a/db/src/db.rs +++ b/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() { diff --git a/db/src/lib.rs b/db/src/lib.rs index 2c9ca700..7af9ed34 100644 --- a/db/src/lib.rs +++ b/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, diff --git a/db/src/metadata.rs b/db/src/metadata.rs index 75ffe982..00e0c4ac 100644 --- a/db/src/metadata.rs +++ b/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(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); }, diff --git a/db/src/schema.rs b/db/src/schema.rs index cca9d1ac..d8168139 100644 --- a/db/src/schema.rs +++ b/db/src/schema.rs @@ -32,35 +32,48 @@ use metadata::{ AttributeAlteration, }; -/// 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()); - if attribute.unique == Some(attribute::Unique::Value) && !attribute.index { +pub trait AttributeValidation { + fn validate(&self, ident: F) -> Result<()> where F: Fn() -> String; +} + +impl AttributeValidation for Attribute { + fn validate(&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, multival: Option, unique: Option>, @@ -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 } diff --git a/query-algebrizer/src/clauses/pattern.rs b/query-algebrizer/src/clauses/pattern.rs index 5bf746a6..abc61e69 100644 --- a/query-algebrizer/src/clauses/pattern.rs +++ b/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() }); diff --git a/query-algebrizer/tests/fulltext.rs b/query-algebrizer/tests/fulltext.rs index 8b2cb198..9241d97c 100644 --- a/query-algebrizer/tests/fulltext.rs +++ b/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() diff --git a/src/vocabulary.rs b/src/vocabulary.rs index 1c671c85..39c7837f 100644 --- a/src/vocabulary.rs +++ b/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) diff --git a/tests/query.rs b/tests/query.rs index 9b2e5330..cc075c8c 100644 --- a/tests/query.rs +++ b/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(); diff --git a/tests/vocabulary.rs b/tests/vocabulary.rs index 2a90ea44..7e3b46d9 100644 --- a/tests/vocabulary.rs +++ b/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)