From 59a710f80fd8f7e69a83771afc480ed9eceedd28 Mon Sep 17 00:00:00 2001 From: Nick Alexander Date: Wed, 7 Jun 2017 13:37:40 -0700 Subject: [PATCH] Review comments: another test, add `unreversed()`. --- db/src/db.rs | 21 +++++++++++ db/src/tx.rs | 92 ++++++++++++++++++++++------------------------ edn/src/symbols.rs | 24 ++++++++++++ edn/src/types.rs | 1 - tx/src/entities.rs | 7 ++++ 5 files changed, 96 insertions(+), 49 deletions(-) diff --git a/db/src/db.rs b/db/src/db.rs index cced8dfd..e26395f0 100644 --- a/db/src/db.rs +++ b/db/src/db.rs @@ -2170,6 +2170,27 @@ mod tests { "{\"s\" 65542 \"t\" 65543}"); + // Check that we can use the same attribute in both forward and backward form in the same + // transaction. + let report = assert_transact!(conn, "[[:db/add 888 :test/dangling 889] + [:db/add 888 :test/_dangling 889]]"); + assert_matches!(conn.last_transaction(), + "[[888 :test/dangling 889 ?tx true] + [889 :test/dangling 888 ?tx true] + [?tx :db/txInstant ?ms ?tx true]]"); + assert_matches!(tempids(&report), + "{}"); + + // Check that we can use the same attribute in both forward and backward form in the same + // transaction in map notation. + let report = assert_transact!(conn, "[{:db/id 998 :test/dangling 999 :test/_dangling 999}]"); + assert_matches!(conn.last_transaction(), + "[[998 :test/dangling 999 ?tx true] + [999 :test/dangling 998 ?tx true] + [?tx :db/txInstant ?ms ?tx true]]"); + assert_matches!(tempids(&report), + "{}"); + // Verify that we can't explode direct reverse notation with nested value maps. assert_transact!(conn, "[[:db/add \"t\" :test/_dangling {:test/many 11}]]", diff --git a/db/src/tx.rs b/db/src/tx.rs index c3a2d4fb..1f6fc37c 100644 --- a/db/src/tx.rs +++ b/db/src/tx.rs @@ -257,13 +257,12 @@ impl<'conn, 'a> Tx<'conn, 'a> { } } - fn entity_a_into_term_a(&mut self, x: entmod::Entid) -> Result<(Entid, &'a Attribute)> { + fn entity_a_into_term_a(&mut self, x: entmod::Entid) -> Result { let a = match x { entmod::Entid::Entid(ref a) => *a, entmod::Entid::Ident(ref a) => self.schema.require_entid(&a)?, }; - let attribute: &Attribute = self.schema.require_attribute_for_entid(a)?; - Ok((a, attribute)) + Ok(a) } fn entity_e_into_term_v(&mut self, x: entmod::EntidOrLookupRefOrTempId) -> Result> { @@ -271,49 +270,44 @@ impl<'conn, 'a> Tx<'conn, 'a> { } 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) + match backward_a.unreversed() { + None => { + bail!(ErrorKind::NotYetImplemented(format!("Cannot explode map notation value in :attr/_reversed notation for forward attribute"))); }, - &entmod::Entid::Ident(ref a) => { - entmod::Entid::Ident(a.to_reversed()) - }, - }; - let (reversed_a, reversed_attribute) = self.entity_a_into_term_a(reversed_a)?; + Some(forward_a) => { + let forward_a = self.entity_a_into_term_a(forward_a)?; + let forward_attribute = self.schema.require_attribute_for_entid(forward_a)?; + if forward_attribute.value_type != ValueType::Ref { + bail!(ErrorKind::NotYetImplemented(format!("Cannot use :attr/_reversed notation for attribute {} that is not :db/valueType :db.type/ref", forward_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 { + if let TypedValue::Ref(entid) = self.schema.to_typed_value(&v.clone().without_spans(), ValueType::Ref)? { + Ok(Either::Left(entid)) + } else { + // The given value is expected to be :db.type/ref, so this shouldn't happen. + bail!(ErrorKind::NotYetImplemented(format!("Cannot use :attr/_reversed notation for attribute {} with value that is not :db.valueType :db.type/ref", forward_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.value_type)? { - 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 {}", forward_a))), + + entmod::AtomOrLookupRefOrVectorOrMapNotation::MapNotation(_) => + bail!(ErrorKind::NotYetImplemented(format!("Cannot explode map notation value in :attr/_reversed notation for attribute {}", forward_a))), } }, - - 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))), } } } @@ -348,13 +342,14 @@ impl<'conn, 'a> Tx<'conn, 'a> { }, Entity::AddOrRetract { op, e, a, v } => { - if a.is_backward() { + if let Some(reversed_a) = a.unreversed() { 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_a = in_process.entity_a_into_term_a(reversed_a)?; 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 a = in_process.entity_a_into_term_a(a)?; + let attribute = self.schema.require_attribute_for_entid(a)?; let v = match v { entmod::AtomOrLookupRefOrVectorOrMapNotation::Atom(v) => { @@ -426,21 +421,22 @@ impl<'conn, 'a> Tx<'conn, 'a> { } for (inner_a, inner_v) in map_notation { - if inner_a.is_backward() { + if let Some(reversed_a) = inner_a.unreversed() { // 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. dangling = false; 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_a = in_process.entity_a_into_term_a(reversed_a)?; 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)?; + let inner_a = in_process.entity_a_into_term_a(inner_a)?; + let inner_attribute = self.schema.require_attribute_for_entid(inner_a)?; if inner_attribute.unique == Some(attribute::Unique::Identity) { dangling = false; } diff --git a/edn/src/symbols.rs b/edn/src/symbols.rs index 866328c0..818a821b 100644 --- a/edn/src/symbols.rs +++ b/edn/src/symbols.rs @@ -214,6 +214,30 @@ impl NamespacedKeyword { namespace: self.namespace.clone(), } } + + /// If this `NamespacedKeyword` is 'backward' (see `symbols::NamespacedKeyword::is_backward`), + /// return `Some('forward name')`; otherwise, return `None`. + /// + /// # Examples + /// + /// ```rust + /// # use edn::symbols::NamespacedKeyword; + /// let nsk = NamespacedKeyword::new("foo", "bar"); + /// assert_eq!(None, nsk.unreversed()); + /// + /// let reversed = nsk.to_reversed(); + /// assert_eq!(Some(nsk), reversed.unreversed()); + /// ``` + pub fn unreversed(&self) -> Option { + if self.is_backward() { + Some(NamespacedKeyword { + name: self.name[1..].to_string(), + namespace: self.namespace.clone(), + }) + } else { + None + } + } } // diff --git a/edn/src/types.rs b/edn/src/types.rs index 332713cc..e002be6d 100644 --- a/edn/src/types.rs +++ b/edn/src/types.rs @@ -594,7 +594,6 @@ mod test { use chrono::{ DateTime, - TimeZone, UTC, }; use num::BigInt; diff --git a/tx/src/entities.rs b/tx/src/entities.rs index 96ee2716..6f741ed0 100644 --- a/tx/src/entities.rs +++ b/tx/src/entities.rs @@ -70,6 +70,13 @@ impl Entid { &Entid::Ident(ref a) => Some(Entid::Ident(a.to_reversed())), } } + + pub fn unreversed(&self) -> Option { + match self { + &Entid::Entid(_) => None, + &Entid::Ident(ref a) => a.unreversed().map(Entid::Ident), + } + } } #[derive(Clone, Debug, Eq, Hash, Ord, PartialOrd, PartialEq)]