Allow retraction of some schema attributes. (#379) r=nalexander

db/src/db.rs

@@ -499,7 +499,7 @@ fn read_ident_map(conn: &rusqlite::Connection) -> Result<IdentMap> {
 fn read_attribute_map(conn: &rusqlite::Connection) -> Result<AttributeMap> {
     let entid_triples = read_materialized_view(conn, "schema")?;
     let mut attribute_map = AttributeMap::default();
-    metadata::update_attribute_map_from_entid_triples(&mut attribute_map, entid_triples)?;
+    metadata::update_attribute_map_from_entid_triples(&mut attribute_map, entid_triples, ::std::iter::empty())?;
     Ok(attribute_map)
 }
 
@@ -1087,8 +1087,8 @@ SELECT EXISTS
                     // error message in this case.
                     if unique_value_stmt.execute(&[to_bool_ref(attribute.unique.is_some()), &entid as &ToSql]).is_err() {
                         match attribute.unique {
-                            Some(attribute::Unique::Value) => bail!(ErrorKind::NotYetImplemented(format!("Cannot alter schema attribute {} to be :db.unique/value", entid))),
+                            Some(attribute::Unique::Value) => bail!(ErrorKind::SchemaAlterationFailed(format!("Cannot alter schema attribute {} to be :db.unique/value", entid))),
-                            Some(attribute::Unique::Identity) => bail!(ErrorKind::NotYetImplemented(format!("Cannot alter schema attribute {} to be :db.unique/identity", entid))),
+                            Some(attribute::Unique::Identity) => bail!(ErrorKind::SchemaAlterationFailed(format!("Cannot alter schema attribute {} to be :db.unique/identity", entid))),
                             None => unreachable!(), // This shouldn't happen, even after we support removing :db/unique.
                         }
                     }
@@ -1102,7 +1102,7 @@ SELECT EXISTS
                     if !attribute.multival {
                         let mut rows = cardinality_stmt.query(&[&entid as &ToSql])?;
                         if rows.next().is_some() {
-                            bail!(ErrorKind::NotYetImplemented(format!("Cannot alter schema attribute {} to be :db.cardinality/one", entid)));
+                            bail!(ErrorKind::SchemaAlterationFailed(format!("Cannot alter schema attribute {} to be :db.cardinality/one", entid)));
                         }
                     }
                 },
@@ -1154,6 +1154,7 @@ mod tests {
     use edn;
     use mentat_core::{
         HasSchema,
+        NamespacedKeyword,
         Schema,
         attribute,
     };
@@ -1637,7 +1638,7 @@ mod tests {
         // Cannot retract a characteristic of an installed attribute.
         assert_transact!(conn,
                          "[[:db/retract 100 :db/cardinality :db.cardinality/many]]",
-                         Err("not yet implemented: Retracting metadata attribute assertions not yet implemented: retracted [e a] pairs [[100 8]]"));
+                         Err("bad schema assertion: Retracting attribute 8 for entity 100 not permitted."));
 
         // Trying to install an attribute without a :db/ident is allowed.
         assert_transact!(conn, "[[:db/add 101 :db/valueType :db.type/long]
@@ -1772,7 +1773,7 @@ mod tests {
         // We can't always go from :db.cardinality/many to :db.cardinality/one.
         assert_transact!(conn, "[[:db/add 100 :db/cardinality :db.cardinality/one]]",
                          // TODO: give more helpful error details.
-                         Err("not yet implemented: Cannot alter schema attribute 100 to be :db.cardinality/one"));
+                         Err("schema alteration failed: Cannot alter schema attribute 100 to be :db.cardinality/one"));
     }
 
     #[test]
@@ -1790,12 +1791,12 @@ mod tests {
         // We can't always migrate to be :db.unique/value.
         assert_transact!(conn, "[[:db/add :test/ident :db/unique :db.unique/value]]",
                          // TODO: give more helpful error details.
-                         Err("not yet implemented: Cannot alter schema attribute 100 to be :db.unique/value"));
+                         Err("schema alteration failed: Cannot alter schema attribute 100 to be :db.unique/value"));
 
         // Not even indirectly!
         assert_transact!(conn, "[[:db/add :test/ident :db/unique :db.unique/identity]]",
                          // TODO: give more helpful error details.
-                         Err("not yet implemented: Cannot alter schema attribute 100 to be :db.unique/identity"));
+                         Err("schema alteration failed: Cannot alter schema attribute 100 to be :db.unique/identity"));
 
         // But we can if we make sure there's no repeated [a v] pair.
         assert_transact!(conn, "[[:db/add 201 :test/ident 2]]");
@@ -1803,6 +1804,19 @@ mod tests {
         assert_transact!(conn, "[[:db/add :test/ident :db/index true]
                                  [:db/add :test/ident :db/unique :db.unique/value]
                                  [:db/add :db.part/db :db.alter/attribute 100]]");
+
+        // We can also retract the uniqueness constraint altogether.
+        assert_transact!(conn, "[[:db/retract :test/ident :db/unique :db.unique/value]]");
+
+        // Once we've done so, the schema shows it's not unique…
+        {
+            let attr = conn.schema.attribute_for_ident(&NamespacedKeyword::new("test", "ident")).unwrap().0;
+            assert_eq!(None, attr.unique);
+        }
+
+        // … and we can add more assertions with duplicate values.
+        assert_transact!(conn, "[[:db/add 121 :test/ident 1]
+                                 [:db/add 221 :test/ident 2]]");
     }
 
     /// Verify that we can't alter :db/fulltext schema characteristics at all.
@@ -1823,7 +1837,7 @@ mod tests {
 
         assert_transact!(conn,
                          "[[:db/retract 111 :db/fulltext true]]",
-                         Err("not yet implemented: Retracting metadata attribute assertions not yet implemented: retracted [e a] pairs [[111 12]]"));
+                         Err("bad schema assertion: Retracting attribute 12 for entity 111 not permitted."));
 
         assert_transact!(conn,
                          "[[:db/add 222 :db/fulltext true]]",

db/src/errors.rs

@@ -97,5 +97,10 @@ error_chain! {
             description("cannot reverse-cache non-unique attribute")
             display("cannot reverse-cache non-unique attribute: {}", attr)
         }
+
+        SchemaAlterationFailed(t: String) {
+            description("schema alteration failed")
+            display("schema alteration failed: {}", t)
+        }
     }
 }

db/src/metadata.rs

@@ -27,8 +27,6 @@
 use std::collections::{BTreeMap, BTreeSet};
 use std::collections::btree_map::Entry;
 
-use itertools::Itertools; // For join().
-
 use add_retract_alter_set::{
     AddRetractAlterSet,
 };
@@ -104,14 +102,66 @@ impl MetadataReport {
 /// contain install and alter markers.
 ///
 /// Returns a report summarizing the mutations that were applied.
-pub fn update_attribute_map_from_entid_triples<U>(attribute_map: &mut AttributeMap, assertions: U) -> Result<MetadataReport>
+pub fn update_attribute_map_from_entid_triples<A, R>(attribute_map: &mut AttributeMap, assertions: A, retractions: R) -> Result<MetadataReport>
-    where U: IntoIterator<Item=(Entid, Entid, TypedValue)> {
+    where A: IntoIterator<Item=(Entid, Entid, TypedValue)>,
+          R: IntoIterator<Item=(Entid, Entid, TypedValue)> {
+
+    fn attribute_builder_to_modify(attribute_id: Entid, existing: &AttributeMap) -> AttributeBuilder {
+        existing.get(&attribute_id)
+                .map(AttributeBuilder::to_modify_attribute)
+                .unwrap_or_else(AttributeBuilder::default)
+    }
 
     // Group mutations by impacted entid.
     let mut builders: BTreeMap<Entid, AttributeBuilder> = BTreeMap::new();
 
+    // For retractions, we start with an attribute builder that's pre-populated with the existing
+    // attribute values. That allows us to check existing values and unset them.
+    for (entid, attr, ref value) in retractions.into_iter() {
+        let builder = builders.entry(entid).or_insert_with(|| attribute_builder_to_modify(entid, attribute_map));
+        match attr {
+            // You can only retract :db/unique, :db/doc, :db/isComponent; all others
+            // must be altered instead of retracted, or are not allowed to change.
+            entids::DB_DOC => {
+                // Nothing to do here; we don't keep docstrings inside `Attribute`s.
+            },
+            entids::DB_IS_COMPONENT => {
+                match value {
+                    &TypedValue::Boolean(v) if builder.component == Some(v) => {
+                        builder.component(false);
+                    },
+                    v => {
+                        bail!(ErrorKind::BadSchemaAssertion(format!("Attempted to retract :db/isComponent with the wrong value {:?}.", v)));
+                    },
+                }
+            },
+            entids::DB_UNIQUE => {
+                match *value {
+                    TypedValue::Ref(u) => {
+                        match u {
+                            entids::DB_UNIQUE_VALUE if builder.unique == Some(Some(attribute::Unique::Value)) => {
+                                builder.non_unique();
+                            },
+                            entids::DB_UNIQUE_IDENTITY if builder.unique == Some(Some(attribute::Unique::Identity)) => {
+                                builder.non_unique();
+                            },
+                            v => {
+                                bail!(ErrorKind::BadSchemaAssertion(format!("Attempted to retract :db/unique with the wrong value {}.", v)));
+                            },
+                        }
+                    },
+                    _ => bail!(ErrorKind::BadSchemaAssertion(format!("Expected [:db/retract _ :db/unique :db.unique/_] but got [:db/retract {} :db/unique {:?}]", entid, value)))
+                }
+            },
+            _ => {
+                bail!(ErrorKind::BadSchemaAssertion(format!("Retracting attribute {} for entity {} not permitted.", attr, entid)));
+            },
+        }
+    }
+
     for (entid, attr, ref value) in assertions.into_iter() {
-        let builder = builders.entry(entid).or_insert(AttributeBuilder::default());
+        // For assertions, we can start with an empty attribute builder.
+        let builder = builders.entry(entid).or_insert_with(Default::default);
 
         // TODO: improve error messages throughout.
         match attr {
@@ -146,11 +196,6 @@ pub fn update_attribute_map_from_entid_triples<U>(attribute_map: &mut AttributeM
 
             entids::DB_UNIQUE => {
                 match *value {
-                    // TODO: accept nil in some form.
-                    // TypedValue::Nil => {
-                    //     builder.unique_value(false);
-                    //     builder.unique_identity(false);
-                    // },
                     TypedValue::Ref(entids::DB_UNIQUE_VALUE) => { builder.unique(attribute::Unique::Value); },
                     TypedValue::Ref(entids::DB_UNIQUE_IDENTITY) => { builder.unique(attribute::Unique::Identity); },
                     _ => bail!(ErrorKind::BadSchemaAssertion(format!("Expected [... :db/unique :db.unique/value|:db.unique/identity] but got [... :db/unique {:?}]", value)))
@@ -257,17 +302,14 @@ pub fn update_schema_from_entid_quadruples<U>(schema: &mut Schema, assertions: U
         attribute_set.witness((e, a), typed_value, added);
     }
 
-    // Datomic does not allow to retract attributes or idents.  For now, Mentat follows suit.
-    if !attribute_set.retracted.is_empty() {
-        bail!(ErrorKind::NotYetImplemented(format!("Retracting metadata attribute assertions not yet implemented: retracted [e a] pairs [{}]",
-                                                   attribute_set.retracted.keys().map(|&(e, a)| format!("[{} {}]", e, a)).join(", "))));
-    }
-
     // Collect triples.
+    let retracted_triples = attribute_set.retracted.into_iter().map(|((e, a), typed_value)| (e, a, typed_value));
     let asserted_triples = attribute_set.asserted.into_iter().map(|((e, a), typed_value)| (e, a, typed_value));
     let altered_triples = attribute_set.altered.into_iter().map(|((e, a), (_old_value, new_value))| (e, a, new_value));
 
-    let report = update_attribute_map_from_entid_triples(&mut schema.attribute_map, asserted_triples.chain(altered_triples))?;
+    let report = update_attribute_map_from_entid_triples(&mut schema.attribute_map,
+                                                         asserted_triples.chain(altered_triples),
+                                                         retracted_triples)?;
 
     let mut idents_altered: BTreeMap<Entid, IdentAlteration> = BTreeMap::new();
 

db/src/schema.rs

@@ -73,13 +73,13 @@ fn validate_attribute_map(entid_map: &EntidMap, attribute_map: &AttributeMap) ->
 #[derive(Clone,Debug,Default,Eq,Hash,Ord,PartialOrd,PartialEq)]
 pub struct AttributeBuilder {
     helpful: bool,
-    value_type: Option<ValueType>,
+    pub value_type: Option<ValueType>,
-    multival: Option<bool>,
+    pub multival: Option<bool>,
-    unique: Option<Option<attribute::Unique>>,
+    pub unique: Option<Option<attribute::Unique>>,
-    index: Option<bool>,
+    pub index: Option<bool>,
-    fulltext: Option<bool>,
+    pub fulltext: Option<bool>,
-    component: Option<bool>,
+    pub component: Option<bool>,
-    no_history: Option<bool>,
+    pub no_history: Option<bool>,
 }
 
 impl AttributeBuilder {
@@ -92,6 +92,16 @@ impl AttributeBuilder {
         }
     }
 
+    /// Make a new AttributeBuilder from an existing Attribute. This is important to allow
+    /// retraction. Only attributes that we allow to change are duplicated here.
+    pub fn to_modify_attribute(attribute: &Attribute) -> Self {
+        let mut ab = AttributeBuilder::default();
+        ab.multival   = Some(attribute.multival);
+        ab.unique     = Some(attribute.unique);
+        ab.component  = Some(attribute.component);
+        ab
+    }
+
     pub fn value_type<'a>(&'a mut self, value_type: ValueType) -> &'a mut Self {
         self.value_type = Some(value_type);
         self
@@ -102,6 +112,11 @@ impl AttributeBuilder {
         self
     }
 
+    pub fn non_unique<'a>(&'a mut self) -> &'a mut Self {
+        self.unique = Some(None);
+        self
+    }
+
     pub fn unique<'a>(&'a mut self, unique: attribute::Unique) -> &'a mut Self {
         if self.helpful && unique == attribute::Unique::Identity {
             self.index = Some(true);
@@ -185,12 +200,19 @@ impl AttributeBuilder {
                 mutations.push(AttributeAlteration::Cardinality);
             }
         }
+
         if let Some(ref unique) = self.unique {
             if *unique != attribute.unique {
                 attribute.unique = unique.clone();
                 mutations.push(AttributeAlteration::Unique);
             }
+        } else {
+            if attribute.unique != None {
+                attribute.unique = None;
+                mutations.push(AttributeAlteration::Unique);
+            }
         }
+
         if let Some(index) = self.index {
             if index != attribute.index {
                 attribute.index = index;
@@ -255,7 +277,10 @@ impl SchemaBuilding for Schema {
         }).collect();
 
         let mut schema = Schema::from_ident_map_and_attribute_map(ident_map, AttributeMap::default())?;
-        let metadata_report = metadata::update_attribute_map_from_entid_triples(&mut schema.attribute_map, entid_assertions?)?;
+        let metadata_report = metadata::update_attribute_map_from_entid_triples(&mut schema.attribute_map,
+                                                                                entid_assertions?,
+                                                                                // No retractions.
+                                                                                ::std::iter::empty())?;
 
         // Rebuild the component attributes list if necessary.
         if metadata_report.attributes_did_change() {