diff --git a/db/src/db.rs b/db/src/db.rs index 93094eba..3374ce11 100644 --- a/db/src/db.rs +++ b/db/src/db.rs @@ -465,7 +465,7 @@ pub(crate) fn read_ident_map(conn: &rusqlite::Connection) -> Result { pub(crate) fn read_attribute_map(conn: &rusqlite::Connection) -> Result { 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())?; - for &entid in &metadata_report.attributes_installed { - stmt.execute(&[&entid as &ToSql])?; + // TODO expand metadata report to allow for better signaling for the above. + + 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]]"); diff --git a/db/src/entids.rs b/db/src/entids.rs index 2c2e0bb4..44b73ccd 100644 --- a/db/src/entids.rs +++ b/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 = { diff --git a/db/src/metadata.rs b/db/src/metadata.rs index 6e5768d0..ab257f7d 100644 --- a/db/src/metadata.rs +++ b/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, ident_retractions: &BTreeMap) -> Result> { + // 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(attribute_map: &mut AttributeMap, assertions: A, retractions: R) -> Result - where A: IntoIterator, - R: IntoIterator { - +pub fn update_attribute_map_from_entid_triples(attribute_map: &mut AttributeMap, assertions: Vec, retractions: Vec) -> Result { 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(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(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(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), - retracted_triples)?; + asserted_triples.chain(altered_triples).collect(), + non_schema_retractions)?; let mut idents_altered: BTreeMap = BTreeMap::new(); @@ -326,13 +397,20 @@ pub fn update_schema_from_entid_quadruples(schema: &mut Schema, assertions: U idents_altered.insert(entid, IdentAlteration::Ident(new_ident.clone())); } - for (entid, ident) in ident_set.retracted { - schema.entid_map.remove(&entid); - schema.ident_map.remove(&ident); - idents_altered.insert(entid, IdentAlteration::Ident(ident.clone())); + for (entid, ident) in &ident_set.retracted { + schema.entid_map.remove(entid); + schema.ident_map.remove(ident); + 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(); } diff --git a/db/src/schema.rs b/db/src/schema.rs index a112ca08..d7db8384 100644 --- a/db/src/schema.rs +++ b/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() { diff --git a/db/src/types.rs b/db/src/types.rs index 9c326e48..fb89fb0f 100644 --- a/db/src/types.rs +++ b/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.