Part 0: Allow retractions of installed attributes

This is necessary for the timelines work ahead. When schema is being
moved off of a main timeline, we need to be able to retract it cleanly.

Retractions are only processed if the whole defining attribute set
is being retracted at once (:db/ident, :db/valueType, :db/cardinality).

db/src/db.rs

@@ -465,7 +465,7 @@ pub(crate) fn read_ident_map(conn: &rusqlite::Connection) -> Result<IdentMap> {
 pub(crate) 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, ::std::iter::empty())?;
+    metadata::update_attribute_map_from_entid_triples(&mut attribute_map, entid_triples, vec![])?;
     Ok(attribute_map)
 }
 
@@ -1015,14 +1015,30 @@ pub fn update_metadata(conn: &rusqlite::Connection, _old_schema: &Schema, new_sc
                      &[])?;
     }
 
+    // Populate the materialized view directly from datoms.
+    // It's possible that an "ident" was removed, along with its attributes.
+    // That's not counted as an "alteration" of attributes, so we explicitly check
+    // for non-emptiness of 'idents_altered'.
 
-    let mut stmt = conn.prepare(format!("INSERT INTO schema SELECT e, a, v, value_type_tag FROM datoms WHERE e = ? AND a IN {}", entids::SCHEMA_SQL_LIST.as_str()).as_str())?;
+    // TODO expand metadata report to allow for better signaling for the above.
-    for &entid in &metadata_report.attributes_installed {
+
-        stmt.execute(&[&entid as &ToSql])?;
+    if !metadata_report.attributes_installed.is_empty()
+        || !metadata_report.attributes_altered.is_empty()
+        || !metadata_report.idents_altered.is_empty() {
+
+        conn.execute(format!("DELETE FROM schema").as_str(),
+                     &[])?;
+        // NB: we're using :db/valueType as a placeholder for the entire schema-defining set.
+        let s = format!(r#"
+            WITH s(e) AS (SELECT e FROM datoms WHERE a = {})
+            INSERT INTO schema
+            SELECT s.e, a, v, value_type_tag
+            FROM datoms, s
+            WHERE s.e = datoms.e AND a IN {}
+        "#, entids::DB_VALUE_TYPE, entids::SCHEMA_SQL_LIST.as_str());
+        conn.execute(&s, &[])?;
     }
 
-    let mut delete_stmt = conn.prepare(format!("DELETE FROM schema WHERE e = ? AND a IN {}", entids::SCHEMA_SQL_LIST.as_str()).as_str())?;
-    let mut insert_stmt = conn.prepare(format!("INSERT INTO schema SELECT e, a, v, value_type_tag FROM datoms WHERE e = ? AND a IN {}", entids::SCHEMA_SQL_LIST.as_str()).as_str())?;
     let mut index_stmt = conn.prepare("UPDATE datoms SET index_avet = ? WHERE a = ?")?;
     let mut unique_value_stmt = conn.prepare("UPDATE datoms SET unique_value = ? WHERE a = ?")?;
     let mut cardinality_stmt = conn.prepare(r#"
@@ -1035,9 +1051,6 @@ SELECT EXISTS
         left.v <> right.v)"#)?;
 
     for (&entid, alterations) in &metadata_report.attributes_altered {
-        delete_stmt.execute(&[&entid as &ToSql])?;
-        insert_stmt.execute(&[&entid as &ToSql])?;
-
         let attribute = new_schema.require_attribute_for_entid(entid)?;
 
         for alteration in alterations {
@@ -1523,7 +1536,10 @@ mod tests {
     fn test_db_install() {
         let mut conn = TestConn::default();
 
-        // We can assert a new attribute.
+        // We're missing some tests here, since our implementation is incomplete.
+        // See https://github.com/mozilla/mentat/issues/797
+
+        // We can assert a new schema attribute.
         assert_transact!(conn, "[[:db/add 100 :db/ident :test/ident]
                                  [:db/add 100 :db/valueType :db.type/long]
                                  [:db/add 100 :db/cardinality :db.cardinality/many]]");
@@ -1560,11 +1576,33 @@ mod tests {
                           [101 :test/ident -9 ?tx true]
                           [?tx :db/txInstant ?ms ?tx true]]");
 
-        // Cannot retract a characteristic of an installed attribute.
+        // Cannot retract a single characteristic of an installed attribute.
         assert_transact!(conn,
                          "[[:db/retract 100 :db/cardinality :db.cardinality/many]]",
                          Err("bad schema assertion: Retracting attribute 8 for entity 100 not permitted."));
 
+        // Cannot retract a single characteristic of an installed attribute.
+        assert_transact!(conn,
+                         "[[:db/retract 100 :db/valueType :db.type/long]]",
+                         Err("bad schema assertion: Retracting attribute 7 for entity 100 not permitted."));
+
+        // Cannot retract a non-defining set of characteristics of an installed attribute.
+        assert_transact!(conn,
+                         "[[:db/retract 100 :db/valueType :db.type/long]
+                         [:db/retract 100 :db/cardinality :db.cardinality/many]]",
+                         Err("bad schema assertion: Retracting defining attributes of a schema without retracting its :db/ident is not permitted."));
+
+        // See https://github.com/mozilla/mentat/issues/796.
+        // assert_transact!(conn,
+        //                 "[[:db/retract 100 :db/ident :test/ident]]",
+        //                 Err("bad schema assertion: Retracting :db/ident of a schema without retracting its defining attributes is not permitted."));
+
+        // Can retract all of characterists of an installed attribute in one go.
+        assert_transact!(conn,
+                         "[[:db/retract 100 :db/cardinality :db.cardinality/many]
+                         [:db/retract 100 :db/valueType :db.type/long]
+                         [:db/retract 100 :db/ident :test/ident]]");
+
         // Trying to install an attribute without a :db/ident is allowed.
         assert_transact!(conn, "[[:db/add 101 :db/valueType :db.type/long]
                                  [:db/add 101 :db/cardinality :db.cardinality/many]]");

db/src/entids.rs

@@ -79,6 +79,21 @@ pub fn might_update_metadata(attribute: Entid) -> bool {
     }
 }
 
+/// Return 'false' if the given attribute might be used to describe a schema attribute.
+pub fn is_a_schema_attribute(attribute: Entid) -> bool {
+    match attribute {
+        DB_IDENT |
+        DB_CARDINALITY |
+        DB_FULLTEXT |
+        DB_INDEX |
+        DB_IS_COMPONENT |
+        DB_UNIQUE |
+        DB_VALUE_TYPE =>
+            true,
+        _ => false,
+    }
+}
+
 lazy_static! {
     /// Attributes that are "ident related".  These might change the "idents" materialized view.
     pub static ref IDENTS_SQL_LIST: String = {

db/src/metadata.rs

@@ -52,6 +52,10 @@ use schema::{
     AttributeValidation,
 };
 
+use types::{
+    EAV,
+};
+
 /// An alteration to an attribute.
 #[derive(Clone, Debug, Eq, Hash, Ord, PartialOrd, PartialEq)]
 pub enum AttributeAlteration {
@@ -97,16 +101,76 @@ impl MetadataReport {
     }
 }
 
+/// Update an 'AttributeMap' in place given two sets of ident and attribute retractions, which
+/// together contain enough information to reason about a "schema retraction".
+///
+/// Schema may only be retracted if all of its necessary attributes are being retracted:
+/// - :db/ident, :db/valueType, :db/cardinality.
+///
+/// Note that this is currently incomplete/flawed:
+/// - we're allowing optional attributes to not be retracted and dangle afterwards
+///
+/// Returns a set of attribute retractions which do not involve schema-defining attributes.
+fn update_attribute_map_from_schema_retractions(attribute_map: &mut AttributeMap, retractions: Vec<EAV>, ident_retractions: &BTreeMap<Entid, symbols::Keyword>) -> Result<Vec<EAV>> {
+    // Process retractions of schema attributes first. It's allowed to retract a schema attribute
+    // if all of the schema-defining schema attributes are being retracted.
+    // A defining set of attributes is :db/ident, :db/valueType, :db/cardinality.
+    let mut filtered_retractions = vec![];
+    let mut suspect_retractions = vec![];
+
+    // Filter out sets of schema altering retractions.
+    let mut eas = BTreeMap::new();
+    for (e, a, v) in retractions.into_iter() {
+        if entids::is_a_schema_attribute(a) {
+            eas.entry(e).or_insert(vec![]).push(a);
+            suspect_retractions.push((e, a, v));
+        } else {
+            filtered_retractions.push((e, a, v));
+        }
+    }
+
+    // TODO (see https://github.com/mozilla/mentat/issues/796).
+    // Retraction of idents is allowed, but if an ident names a schema attribute, then we should enforce
+    // retraction of all of the associated schema attributes.
+    // Unfortunately, our current in-memory schema representation (namely, how we define an Attribute) is not currently
+    // rich enough: it lacks distinction between presence and absence, and instead assumes default values.
+
+    // Currently, in order to do this enforcement correctly, we'd need to inspect 'datoms'.
+
+    // Here is an incorrect way to enforce this. It's incorrect because it prevents us from retracting non-"schema naming" idents.
+    // for retracted_e in ident_retractions.keys() {
+    //     if !eas.contains_key(retracted_e) {
+    //         bail!(DbErrorKind::BadSchemaAssertion(format!("Retracting :db/ident of a schema without retracting its defining attributes is not permitted.")));
+    //     }
+    // }
+
+    for (e, a, v) in suspect_retractions.into_iter() {
+        let attributes = eas.get(&e).unwrap();
+
+        // Found a set of retractions which negate a schema.
+        if attributes.contains(&entids::DB_CARDINALITY) && attributes.contains(&entids::DB_VALUE_TYPE) {
+            // Ensure that corresponding :db/ident is also being retracted at the same time.
+            if ident_retractions.contains_key(&e) {
+                // Remove attributes corresponding to retracted attribute.
+                attribute_map.remove(&e);
+            } else {
+                bail!(DbErrorKind::BadSchemaAssertion(format!("Retracting defining attributes of a schema without retracting its :db/ident is not permitted.")));
+            }
+        } else {
+            filtered_retractions.push((e, a, v));
+        }
+    }
+
+    Ok(filtered_retractions)
+}
+
 /// Update a `AttributeMap` in place from the given `[e a typed_value]` triples.
 ///
 /// This is suitable for producing a `AttributeMap` from the `schema` materialized view, which does not
 /// contain install and alter markers.
 ///
 /// Returns a report summarizing the mutations that were applied.
-pub fn update_attribute_map_from_entid_triples<A, R>(attribute_map: &mut AttributeMap, assertions: A, retractions: R) -> Result<MetadataReport>
+pub fn update_attribute_map_from_entid_triples(attribute_map: &mut AttributeMap, assertions: Vec<EAV>, retractions: Vec<EAV>) -> Result<MetadataReport> {
-    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)
@@ -118,7 +182,7 @@ pub fn update_attribute_map_from_entid_triples<A, R>(attribute_map: &mut Attribu
 
     // 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() {
+    for (entid, attr, ref value) in retractions {
         let builder = builders.entry(entid).or_insert_with(|| attribute_builder_to_modify(entid, attribute_map));
         match attr {
             // You can only retract :db/unique, :db/isComponent; all others must be altered instead
@@ -249,7 +313,7 @@ pub fn update_attribute_map_from_entid_triples<A, R>(attribute_map: &mut Attribu
                 // … and twice, now we have the Attribute.
                 let a = builder.build();
                 a.validate(|| entid.to_string())?;
-                entry.insert(builder.build());
+                entry.insert(a);
                 attributes_installed.insert(entid);
             },
 
@@ -306,9 +370,16 @@ pub fn update_schema_from_entid_quadruples<U>(schema: &mut Schema, assertions: U
     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));
 
+    // First we process retractions which remove schema.
+    // This operation consumes our current list of attribute retractions, producing a filtered one.
+    let non_schema_retractions = update_attribute_map_from_schema_retractions(&mut schema.attribute_map,
+                                                                              retracted_triples.collect(),
+                                                                              &ident_set.retracted)?;
+
+    // Now we process all other retractions.
     let report = update_attribute_map_from_entid_triples(&mut schema.attribute_map,
-                                                         asserted_triples.chain(altered_triples),
+                                                         asserted_triples.chain(altered_triples).collect(),
-                                                         retracted_triples)?;
+                                                         non_schema_retractions)?;
 
     let mut idents_altered: BTreeMap<Entid, IdentAlteration> = BTreeMap::new();
 
@@ -326,13 +397,20 @@ pub fn update_schema_from_entid_quadruples<U>(schema: &mut Schema, assertions: U
         idents_altered.insert(entid, IdentAlteration::Ident(new_ident.clone()));
     }
 
-    for (entid, ident) in ident_set.retracted {
+    for (entid, ident) in &ident_set.retracted {
-        schema.entid_map.remove(&entid);
+        schema.entid_map.remove(entid);
-        schema.ident_map.remove(&ident);
+        schema.ident_map.remove(ident);
-        idents_altered.insert(entid, IdentAlteration::Ident(ident.clone()));
+        idents_altered.insert(*entid, IdentAlteration::Ident(ident.clone()));
     }
 
-    if report.attributes_did_change() {
+    // Component attributes need to change if either:
+    // - a component attribute changed
+    // - a schema attribute that was a component was retracted
+    // These two checks are a rather heavy-handed way of keeping schema's
+    // component_attributes up-to-date: most of the time we'll rebuild it
+    // even though it's not necessary (e.g. a schema attribute that's _not_
+    // a component was removed, or a non-component related attribute changed).
+    if report.attributes_did_change() || ident_set.retracted.len() > 0 {
         schema.update_component_attributes();
     }
 

db/src/schema.rs

@@ -283,7 +283,7 @@ impl SchemaBuilding for Schema {
         let metadata_report = metadata::update_attribute_map_from_entid_triples(&mut schema.attribute_map,
                                                                                 entid_assertions?,
                                                                                 // No retractions.
-                                                                                ::std::iter::empty())?;
+                                                                                vec![])?;
 
         // Rebuild the component attributes list if necessary.
         if metadata_report.attributes_did_change() {

db/src/types.rs

@@ -144,6 +144,9 @@ impl DB {
 /// Used to represent lookup-refs and [TEMPID a v] upserts as they are resolved.
 pub type AVPair = (Entid, TypedValue);
 
+/// Used to represent assertions and retractions.
+pub(crate) type EAV = (Entid, Entid, TypedValue);
+
 /// Map [a v] pairs to existing entids.
 ///
 /// Used to resolve lookup-refs and upserts.