From d88823e7c4638a7f2e5f1ab96ea1ff04ac06fda6 Mon Sep 17 00:00:00 2001 From: Nick Alexander Date: Tue, 6 Jun 2017 15:21:51 -0700 Subject: [PATCH] Handle :attribute/_reverse in transactor. Fixes #187 There are two broad approaches: 1) Handle reverse attribute notation dynamically, in the style that Datomic does. This is the most flexible, but it's not a good fit given that we produce strongly typed output from the parser. Strongly typed input to the transactor has had many benefits, so I don't want to roll it back for a relatively unimportant feature like reverse notation -- especially not since Mentat does not require :db.install/_attribute to modify schema attributes. 2) Handle reverse attribute in the parser itself, so that we can produce strongly typed parser output while restricting the input. I implemented this first and discovered that it's very difficult to give sensible error messages in common cases. In any case, the bulk of the code is the same between the two approaches, and I wrote the tests for the dynamic version (with error output), so that's what I'm rolling with. This patch preserves the existing indentation, to highlight the differences. The next patch will indent. --- db/src/db.rs | 123 +++++++++++++++++++++++++++++++++++++++++++++ db/src/tx.rs | 90 +++++++++++++++++++++++++++++---- tx/src/entities.rs | 16 ++++++ 3 files changed, 218 insertions(+), 11 deletions(-) diff --git a/db/src/db.rs b/db/src/db.rs index 9aa773b9..cced8dfd 100644 --- a/db/src/db.rs +++ b/db/src/db.rs @@ -2087,4 +2087,127 @@ mod tests { // dangling, if we give a :db/id explicitly. assert_transact!(conn, "[{:test/dangling {:db/id \"t\" :test/many 11}}]"); } + + #[test] + fn test_explode_reversed_notation() { + let mut conn = TestConn::default(); + + // Start by installing a few attributes. + assert_transact!(conn, "[[:db/add 111 :db/ident :test/many] + [:db/add 111 :db/valueType :db.type/long] + [:db/add 111 :db/cardinality :db.cardinality/many] + [:db/add 222 :db/ident :test/component] + [:db/add 222 :db/isComponent true] + [:db/add 222 :db/valueType :db.type/ref] + [:db/add 333 :db/ident :test/unique] + [:db/add 333 :db/unique :db.unique/identity] + [:db/add 333 :db/index true] + [:db/add 333 :db/valueType :db.type/long] + [:db/add 444 :db/ident :test/dangling] + [:db/add 444 :db/valueType :db.type/ref]]"); + + // Check that we can explode direct reversed notation, entids. + let report = assert_transact!(conn, "[[:db/add 100 :test/_dangling 200]]"); + assert_matches!(conn.last_transaction(), + "[[200 :test/dangling 100 ?tx true] + [?tx :db/txInstant ?ms ?tx true]]"); + assert_matches!(tempids(&report), + "{}"); + + // Check that we can explode direct reversed notation, idents. + let report = assert_transact!(conn, "[[:db/add :test/many :test/_dangling :test/unique]]"); + assert_matches!(conn.last_transaction(), + "[[333 :test/dangling :test/many ?tx true] + [?tx :db/txInstant ?ms ?tx true]]"); + assert_matches!(tempids(&report), + "{}"); + + // Check that we can explode direct reversed notation, tempids. + let report = assert_transact!(conn, "[[:db/add \"s\" :test/_dangling \"t\"]]"); + assert_matches!(conn.last_transaction(), + "[[65537 :test/dangling 65536 ?tx true] + [?tx :db/txInstant ?ms ?tx true]]"); + // This is implementation specific, but it should be deterministic. + assert_matches!(tempids(&report), + "{\"s\" 65536 + \"t\" 65537}"); + + // Check that we can explode reversed notation in map notation without :db/id. + let report = assert_transact!(conn, "[{:test/_dangling 501} + {:test/_dangling :test/many} + {:test/_dangling \"t\"}]"); + assert_matches!(conn.last_transaction(), + "[[111 :test/dangling ?e1 ?tx true] + [501 :test/dangling ?e2 ?tx true] + [65538 :test/dangling ?e3 ?tx true] + [?tx :db/txInstant ?ms ?tx true]]"); + assert_matches!(tempids(&report), + "{\"t\" 65538}"); + + // Check that we can explode reversed notation in map notation with :db/id, entid. + let report = assert_transact!(conn, "[{:db/id 600 :test/_dangling 601}]"); + assert_matches!(conn.last_transaction(), + "[[601 :test/dangling 600 ?tx true] + [?tx :db/txInstant ?ms ?tx true]]"); + assert_matches!(tempids(&report), + "{}"); + + // Check that we can explode reversed notation in map notation with :db/id, ident. + let report = assert_transact!(conn, "[{:db/id :test/component :test/_dangling :test/component}]"); + assert_matches!(conn.last_transaction(), + "[[222 :test/dangling :test/component ?tx true] + [?tx :db/txInstant ?ms ?tx true]]"); + assert_matches!(tempids(&report), + "{}"); + + // Check that we can explode reversed notation in map notation with :db/id, tempid. + let report = assert_transact!(conn, "[{:db/id \"s\" :test/_dangling \"t\"}]"); + assert_matches!(conn.last_transaction(), + "[[65543 :test/dangling 65542 ?tx true] + [?tx :db/txInstant ?ms ?tx true]]"); + // This is implementation specific, but it should be deterministic. + assert_matches!(tempids(&report), + "{\"s\" 65542 + \"t\" 65543}"); + + // Verify that we can't explode direct reverse notation with nested value maps. + assert_transact!(conn, + "[[:db/add \"t\" :test/_dangling {:test/many 11}]]", + Err("not yet implemented: Cannot explode map notation value in :attr/_reversed notation for attribute 444")); + + // Verify that we can't explode reverse notation in map notation with nested value maps. + assert_transact!(conn, + "[{:test/_dangling {:test/many 11}}]", + Err("not yet implemented: Cannot explode map notation value in :attr/_reversed notation for attribute 444")); + + // Verify that we can't explode direct reverse notation with nested value vectors. + assert_transact!(conn, + "[[:db/add \"t\" :test/_dangling [:test/many]]]", + Err("not yet implemented: Cannot explode vector value in :attr/_reversed notation for attribute 444")); + + // Verify that we can't explode reverse notation in map notation with nested value vectors. + assert_transact!(conn, + "[{:test/_dangling [:test/many]}]", + Err("not yet implemented: Cannot explode vector value in :attr/_reversed notation for attribute 444")); + + // Verify that we can't use reverse notation with non-:db.type/ref attributes. + assert_transact!(conn, + "[{:test/_unique 500}]", + Err("not yet implemented: Cannot use :attr/_reversed notation for attribute 333 that is not :db/valueType :db.type/ref")); + + // Verify that we can't use reverse notation with unrecognized attributes. + assert_transact!(conn, + "[{:test/_unknown 500}]", + Err("no entid found for ident: :test/unknown")); // TODO: make this error reference the original :test/_unknown. + + // Verify that we can't use reverse notation with bad value types: here, an unknown keyword + // that can't be coerced to a ref. + assert_transact!(conn, + "[{:test/_dangling :test/unknown}]", + Err("no entid found for ident: :test/unknown")); + // And here, a float. + assert_transact!(conn, + "[{:test/_dangling 1.23}]", + Err("EDN value \'1.23\' is not the expected Mentat value type Ref")); + } } diff --git a/db/src/tx.rs b/db/src/tx.rs index 6cdf30a8..e755d292 100644 --- a/db/src/tx.rs +++ b/db/src/tx.rs @@ -269,6 +269,53 @@ impl<'conn, 'a> Tx<'conn, 'a> { fn entity_e_into_term_v(&mut self, x: entmod::EntidOrLookupRefOrTempId) -> Result> { self.entity_e_into_term_e(x).map(|r| r.map_left(TypedValue::Ref)) } + + fn entity_v_into_term_e(&mut self, x: entmod::AtomOrLookupRefOrVectorOrMapNotation, backward_a: &entmod::Entid) -> Result> { + let reversed_a = match backward_a { + &entmod::Entid::Entid(a) => { + // Programmer error. + unreachable!("Expected ident backward attribute but got entid {}", a) + }, + &entmod::Entid::Ident(ref a) => { + entmod::Entid::Ident(a.to_reversed()) + }, + }; + let (reversed_a, reversed_attribute) = self.entity_a_into_term_a(reversed_a)?; + + if reversed_attribute.value_type != ValueType::Ref { + bail!(ErrorKind::NotYetImplemented(format!("Cannot use :attr/_reversed notation for attribute {} that is not :db/valueType :db.type/ref", reversed_a))) + } + + match x { + entmod::AtomOrLookupRefOrVectorOrMapNotation::Atom(ref v) => { + // Here is where we do schema-aware typechecking: we either assert + // that the given value is in the attribute's value set, or (in + // limited cases) coerce the value into the attribute's value set. + if let Some(text) = v.inner.as_text() { + Ok(Either::Right(LookupRefOrTempId::TempId(self.intern_temp_id(TempId::External(text.clone()))))) + } else { + // Here is where we do schema-aware typechecking: we either assert that + // the given value is in the attribute's value set, or (in limited + // cases) coerce the value into the attribute's value set. + if let TypedValue::Ref(entid) = self.schema.to_typed_value(&v.clone().without_spans(), &reversed_attribute)? { + Ok(Either::Left(entid)) + } else { + // reversed_attribute is :db.type/ref, so this shouldn't happen. + unreachable!() + } + } + }, + + entmod::AtomOrLookupRefOrVectorOrMapNotation::LookupRef(ref lookup_ref) => + Ok(Either::Right(LookupRefOrTempId::LookupRef(self.intern_lookup_ref(lookup_ref)?))), + + entmod::AtomOrLookupRefOrVectorOrMapNotation::Vector(_) => + bail!(ErrorKind::NotYetImplemented(format!("Cannot explode vector value in :attr/_reversed notation for attribute {}", reversed_a))), + + entmod::AtomOrLookupRefOrVectorOrMapNotation::MapNotation(_) => + bail!(ErrorKind::NotYetImplemented(format!("Cannot explode map notation value in :attr/_reversed notation for attribute {}", reversed_a))), + } + } } let mut in_process = InProcess::with_schema(&self.schema); @@ -301,7 +348,13 @@ impl<'conn, 'a> Tx<'conn, 'a> { }, Entity::AddOrRetract { op, e, a, v } => { - let (a, attribute) = in_process.entity_a_into_term_a(a)?; + if a.is_backward() { + let reversed_e = in_process.entity_v_into_term_e(v, &a)?; + let (reversed_a, _reversed_attribute) = in_process.entity_a_into_term_a(a.to_reversed().unwrap())?; + let reversed_v = in_process.entity_e_into_term_v(e)?; + terms.push(Term::AddOrRetract(OpType::Add, reversed_e, reversed_a, reversed_v)); + } else { + let (a, attribute) = in_process.entity_a_into_term_a(a)?; let v = match v { entmod::AtomOrLookupRefOrVectorOrMapNotation::Atom(v) => { @@ -373,18 +426,32 @@ impl<'conn, 'a> Tx<'conn, 'a> { } for (inner_a, inner_v) in map_notation { - let (inner_a, inner_attribute) = in_process.entity_a_into_term_a(inner_a)?; - - if inner_attribute.unique == Some(attribute::Unique::Identity) { + if inner_a.is_backward() { + // We definitely have a reference. The reference might be + // dangling (a bare entid, for example), but we don't yet + // support nested maps and reverse notation simultaneously + // (i.e., we don't accept {:reverse/_attribute {:nested map}}) + // so we don't need to check that the nested map reference isn't + // danglign. dangling = false; - } - deque.push_front(Entity::AddOrRetract { - op: OpType::Add, - e: db_id.clone(), - a: entmod::Entid::Entid(inner_a), - v: inner_v, - }); + let reversed_e = in_process.entity_v_into_term_e(inner_v, &inner_a)?; + let (reversed_a, _reversed_attribute) = in_process.entity_a_into_term_a(inner_a.to_reversed().unwrap())?; + let reversed_v = in_process.entity_e_into_term_v(db_id.clone())?; + terms.push(Term::AddOrRetract(OpType::Add, reversed_e, reversed_a, reversed_v)); + } else { + let (inner_a, inner_attribute) = in_process.entity_a_into_term_a(inner_a)?; + if inner_attribute.unique == Some(attribute::Unique::Identity) { + dangling = false; + } + + deque.push_front(Entity::AddOrRetract { + op: OpType::Add, + e: db_id.clone(), + a: entmod::Entid::Entid(inner_a), + v: inner_v, + }); + } } if dangling { @@ -397,6 +464,7 @@ impl<'conn, 'a> Tx<'conn, 'a> { let e = in_process.entity_e_into_term_e(e)?; terms.push(Term::AddOrRetract(op, e, a, v)); + } }, } }; diff --git a/tx/src/entities.rs b/tx/src/entities.rs index a943e49c..96ee2716 100644 --- a/tx/src/entities.rs +++ b/tx/src/entities.rs @@ -56,6 +56,22 @@ pub enum Entid { Ident(NamespacedKeyword), } +impl Entid { + pub fn is_backward(&self) -> bool { + match self { + &Entid::Entid(_) => false, + &Entid::Ident(ref a) => a.is_backward(), + } + } + + pub fn to_reversed(&self) -> Option { + match self { + &Entid::Entid(_) => None, + &Entid::Ident(ref a) => Some(Entid::Ident(a.to_reversed())), + } + } +} + #[derive(Clone, Debug, Eq, Hash, Ord, PartialOrd, PartialEq)] pub struct LookupRef { pub a: Entid,