From a4fc04ea865ff4f18304a56a3f9ffa9f04e34927 Mon Sep 17 00:00:00 2001 From: Nick Alexander Date: Tue, 6 Jun 2017 13:18:31 -0700 Subject: [PATCH 1/9] Pre: Crib map_{left,right} for Either. --- db/src/internal_types.rs | 21 +++++++++++++++++++++ 1 file changed, 21 insertions(+) diff --git a/db/src/internal_types.rs b/db/src/internal_types.rs index 7b90978c..724a1cf4 100644 --- a/db/src/internal_types.rs +++ b/db/src/internal_types.rs @@ -39,6 +39,27 @@ pub enum Either { Right(R), } +// Cribbed from https://github.com/bluss/either/blob/f793721f3fdeb694f009e731b23a2858286bc0d6/src/lib.rs#L219-L259. +impl Either { + pub fn map_left(self, f: F) -> Either + where F: FnOnce(L) -> M + { + match self { + Left(l) => Left(f(l)), + Right(r) => Right(r), + } + } + + pub fn map_right(self, f: F) -> Either + where F: FnOnce(R) -> S + { + match self { + Left(l) => Left(l), + Right(r) => Right(f(r)), + } + } +} + use self::Either::*; pub type EntidOr = Either; From 2650fe163d76e5f13ffddea131f21ec60ee29b91 Mon Sep 17 00:00:00 2001 From: Nick Alexander Date: Mon, 5 Jun 2017 20:31:39 -0700 Subject: [PATCH 2/9] Pre: Intern lookup_ref by reference. --- db/src/tx.rs | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/db/src/tx.rs b/db/src/tx.rs index ee6db13e..0e280227 100644 --- a/db/src/tx.rs +++ b/db/src/tx.rs @@ -194,7 +194,7 @@ impl<'conn, 'a> Tx<'conn, 'a> { let mut temp_ids: intern_set::InternSet = intern_set::InternSet::new(); let mut lookup_refs: intern_set::InternSet = intern_set::InternSet::new(); - let intern_lookup_ref = |lookup_refs: &mut intern_set::InternSet, lookup_ref: entmod::LookupRef| -> Result { + let intern_lookup_ref = |lookup_refs: &mut intern_set::InternSet, lookup_ref: &entmod::LookupRef| -> Result { let lr_a: i64 = match lookup_ref.a { entmod::Entid::Entid(ref a) => *a, entmod::Entid::Ident(ref a) => self.schema.require_entid(&a)?, @@ -265,7 +265,7 @@ impl<'conn, 'a> Tx<'conn, 'a> { } }, - entmod::AtomOrLookupRefOrVectorOrMapNotation::LookupRef(lookup_ref) => { + entmod::AtomOrLookupRefOrVectorOrMapNotation::LookupRef(ref lookup_ref) => { if attribute.value_type != ValueType::Ref { bail!(ErrorKind::NotYetImplemented(format!("Cannot resolve value lookup ref for attribute {} that is not :db/valueType :db.type/ref", a))) } @@ -360,7 +360,7 @@ impl<'conn, 'a> Tx<'conn, 'a> { Either::Right(LookupRefOrTempId::TempId(temp_ids.intern(e))) }, - entmod::EntidOrLookupRefOrTempId::LookupRef(lookup_ref) => { + entmod::EntidOrLookupRefOrTempId::LookupRef(ref lookup_ref) => { Either::Right(LookupRefOrTempId::LookupRef(intern_lookup_ref(&mut lookup_refs, lookup_ref)?)) }, } @@ -380,7 +380,7 @@ impl<'conn, 'a> Tx<'conn, 'a> { Either::Right(LookupRefOrTempId::TempId(temp_ids.intern(e))) }, - entmod::EntidOrLookupRefOrTempId::LookupRef(lookup_ref) => { + entmod::EntidOrLookupRefOrTempId::LookupRef(ref lookup_ref) => { Either::Right(LookupRefOrTempId::LookupRef(intern_lookup_ref(&mut lookup_refs, lookup_ref)?)) }, }; @@ -395,7 +395,7 @@ impl<'conn, 'a> Tx<'conn, 'a> { /// Pipeline stage 2: rewrite `Term` instances with lookup refs into `Term` instances without /// lookup refs. /// - /// The `Term` instances produce share interned TempId handles and have no LookupRef references. + /// The `Term` instances produced share interned TempId handles and have no LookupRef references. fn resolve_lookup_refs(&self, lookup_ref_map: &AVMap, terms: I) -> Result> where I: IntoIterator { terms.into_iter().map(|term: TermWithTempIdsAndLookupRefs| -> Result { match term { From 05129cefbb0bc650d44391d29cd3cf414d2b7fdb Mon Sep 17 00:00:00 2001 From: Nick Alexander Date: Wed, 7 Jun 2017 13:09:11 -0700 Subject: [PATCH 3/9] Pre: Use ValueType rather than Attribute to convert edn::Value to TypedValue. This is expedient now, but might require work in the future to achieve better error messages. --- db/src/schema.rs | 34 ++++++++++++++++++---------------- db/src/tx.rs | 4 ++-- 2 files changed, 20 insertions(+), 18 deletions(-) diff --git a/db/src/schema.rs b/db/src/schema.rs index ccd79093..bf1d22a5 100644 --- a/db/src/schema.rs +++ b/db/src/schema.rs @@ -223,30 +223,32 @@ impl SchemaBuilding for Schema { pub trait SchemaTypeChecking { /// Do schema-aware typechecking and coercion. /// - /// Either assert that the given value is in the attribute's value set, or (in limited cases) - /// coerce the given value into the attribute's value set. - fn to_typed_value(&self, value: &edn::Value, attribute: &Attribute) -> Result; + /// Either assert that the given value is in the value type's value set, or (in limited cases) + /// coerce the given value into the value type's value set. + fn to_typed_value(&self, value: &edn::Value, value_type: ValueType) -> Result; } impl SchemaTypeChecking for Schema { - fn to_typed_value(&self, value: &edn::Value, attribute: &Attribute) -> Result { - // TODO: encapsulate entid-ident-attribute for better error messages. + fn to_typed_value(&self, value: &edn::Value, value_type: ValueType) -> Result { + // TODO: encapsulate entid-ident-attribute for better error messages, perhaps by including + // the attribute (rather than just the attribute's value type) into this function or a + // wrapper function. match TypedValue::from_edn_value(value) { // We don't recognize this EDN at all. Get out! - None => bail!(ErrorKind::BadEDNValuePair(value.clone(), attribute.value_type.clone())), - Some(typed_value) => match (&attribute.value_type, typed_value) { + None => bail!(ErrorKind::BadEDNValuePair(value.clone(), value_type)), + Some(typed_value) => match (value_type, typed_value) { // Most types don't coerce at all. - (&ValueType::Boolean, tv @ TypedValue::Boolean(_)) => Ok(tv), - (&ValueType::Long, tv @ TypedValue::Long(_)) => Ok(tv), - (&ValueType::Double, tv @ TypedValue::Double(_)) => Ok(tv), - (&ValueType::String, tv @ TypedValue::String(_)) => Ok(tv), - (&ValueType::Uuid, tv @ TypedValue::Uuid(_)) => Ok(tv), - (&ValueType::Keyword, tv @ TypedValue::Keyword(_)) => Ok(tv), + (ValueType::Boolean, tv @ TypedValue::Boolean(_)) => Ok(tv), + (ValueType::Long, tv @ TypedValue::Long(_)) => Ok(tv), + (ValueType::Double, tv @ TypedValue::Double(_)) => Ok(tv), + (ValueType::String, tv @ TypedValue::String(_)) => Ok(tv), + (ValueType::Uuid, tv @ TypedValue::Uuid(_)) => Ok(tv), + (ValueType::Keyword, tv @ TypedValue::Keyword(_)) => Ok(tv), // Ref coerces a little: we interpret some things depending on the schema as a Ref. - (&ValueType::Ref, TypedValue::Long(x)) => Ok(TypedValue::Ref(x)), - (&ValueType::Ref, TypedValue::Keyword(ref x)) => self.require_entid(&x).map(|entid| TypedValue::Ref(entid)), + (ValueType::Ref, TypedValue::Long(x)) => Ok(TypedValue::Ref(x)), + (ValueType::Ref, TypedValue::Keyword(ref x)) => self.require_entid(&x).map(|entid| TypedValue::Ref(entid)), // Otherwise, we have a type mismatch. - (value_type, _) => bail!(ErrorKind::BadEDNValuePair(value.clone(), value_type.clone())), + (value_type, _) => bail!(ErrorKind::BadEDNValuePair(value.clone(), value_type)), } } } diff --git a/db/src/tx.rs b/db/src/tx.rs index 0e280227..48a0ef8a 100644 --- a/db/src/tx.rs +++ b/db/src/tx.rs @@ -205,7 +205,7 @@ impl<'conn, 'a> Tx<'conn, 'a> { bail!(ErrorKind::NotYetImplemented(format!("Cannot resolve (lookup-ref {} {}) with attribute that is not :db/unique", lr_a, lookup_ref.v))) } - let lr_typed_value: TypedValue = self.schema.to_typed_value(&lookup_ref.v, &lr_attribute)?; + let lr_typed_value: TypedValue = self.schema.to_typed_value(&lookup_ref.v, lr_attribute.value_type)?; Ok(lookup_refs.intern((lr_a, lr_typed_value))) }; @@ -260,7 +260,7 @@ impl<'conn, 'a> Tx<'conn, 'a> { // 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. - let typed_value: TypedValue = self.schema.to_typed_value(&v.without_spans(), &attribute)?; + let typed_value: TypedValue = self.schema.to_typed_value(&v.without_spans(), attribute.value_type)?; Either::Left(typed_value) } }, From 4b0881a9573040747a04924c84c16b5dd47a0e2e Mon Sep 17 00:00:00 2001 From: Nick Alexander Date: Tue, 6 Jun 2017 12:59:54 -0700 Subject: [PATCH 4/9] Pre: Push bookkeeping into an InProcess struct. --- db/src/tx.rs | 83 ++++++++++++++++++++++++++++++++-------------------- 1 file changed, 52 insertions(+), 31 deletions(-) diff --git a/db/src/tx.rs b/db/src/tx.rs index 48a0ef8a..6095eded 100644 --- a/db/src/tx.rs +++ b/db/src/tx.rs @@ -51,6 +51,7 @@ use std::collections::{ BTreeSet, VecDeque, }; +use std::rc::Rc; use db; use db::{ @@ -191,23 +192,51 @@ impl<'conn, 'a> Tx<'conn, 'a> { /// The `Term` instances produce share interned TempId and LookupRef handles, and we return the /// interned handle sets so that consumers can ensure all handles are used appropriately. fn entities_into_terms_with_temp_ids_and_lookup_refs(&self, entities: I) -> Result<(Vec, intern_set::InternSet, intern_set::InternSet)> where I: IntoIterator { - let mut temp_ids: intern_set::InternSet = intern_set::InternSet::new(); - let mut lookup_refs: intern_set::InternSet = intern_set::InternSet::new(); + struct InProcess<'a> { + schema: &'a Schema, + mentat_id_count: i64, + temp_ids: intern_set::InternSet, + lookup_refs: intern_set::InternSet, + } - let intern_lookup_ref = |lookup_refs: &mut intern_set::InternSet, lookup_ref: &entmod::LookupRef| -> Result { - let lr_a: i64 = match lookup_ref.a { - entmod::Entid::Entid(ref a) => *a, - entmod::Entid::Ident(ref a) => self.schema.require_entid(&a)?, - }; - let lr_attribute: &Attribute = self.schema.require_attribute_for_entid(lr_a)?; - - if lr_attribute.unique.is_none() { - bail!(ErrorKind::NotYetImplemented(format!("Cannot resolve (lookup-ref {} {}) with attribute that is not :db/unique", lr_a, lookup_ref.v))) + impl<'a> InProcess<'a> { + fn with_schema(schema: &'a Schema) -> InProcess<'a> { + InProcess { + schema: schema, + mentat_id_count: 0, + temp_ids: intern_set::InternSet::new(), + lookup_refs: intern_set::InternSet::new(), + } } - let lr_typed_value: TypedValue = self.schema.to_typed_value(&lookup_ref.v, lr_attribute.value_type)?; - Ok(lookup_refs.intern((lr_a, lr_typed_value))) - }; + fn intern_lookup_ref(&mut self, lookup_ref: &entmod::LookupRef) -> Result { + let lr_a: i64 = match lookup_ref.a { + entmod::Entid::Entid(ref a) => *a, + entmod::Entid::Ident(ref a) => self.schema.require_entid(&a)?, + }; + let lr_attribute: &Attribute = self.schema.require_attribute_for_entid(lr_a)?; + + if lr_attribute.unique.is_none() { + bail!(ErrorKind::NotYetImplemented(format!("Cannot resolve (lookup-ref {} {}) with attribute that is not :db/unique", lr_a, lookup_ref.v))) + } + + let lr_typed_value: TypedValue = self.schema.to_typed_value(&lookup_ref.v, lr_attribute.value_type)?; + Ok(self.lookup_refs.intern((lr_a, lr_typed_value))) + } + + fn intern_temp_id(&mut self, temp_id: TempId) -> Rc { + self.temp_ids.intern(temp_id) + } + + /// Allocate private internal tempids reserved for Mentat. Internal tempids just need to be + /// unique within one transaction; they should never escape a transaction. + fn allocate_mentat_id(&mut self) -> entmod::EntidOrLookupRefOrTempId { + self.mentat_id_count += 1; + entmod::EntidOrLookupRefOrTempId::TempId(TempId::Internal(self.mentat_id_count)) + } + } + + let mut in_process = InProcess::with_schema(&self.schema); // We want to handle entities in the order they're given to us, while also "exploding" some // entities into many. We therefore push the initial entities onto the back of the deque, @@ -215,14 +244,6 @@ impl<'conn, 'a> Tx<'conn, 'a> { let mut deque: VecDeque = VecDeque::default(); deque.extend(entities); - // Allocate private internal tempids reserved for Mentat. Internal tempids just need to be - // unique within one transaction; they should never escape a transaction. - let mut mentat_id_count = 0; - let mut allocate_mentat_id = move || { - mentat_id_count += 1; - entmod::EntidOrLookupRefOrTempId::TempId(TempId::Internal(mentat_id_count)) - }; - let mut terms: Vec = Vec::with_capacity(deque.len()); while let Some(entity) = deque.pop_front() { @@ -230,7 +251,7 @@ impl<'conn, 'a> Tx<'conn, 'a> { Entity::MapNotation(mut map_notation) => { // :db/id is optional; if it's not given, we generate a special internal tempid // to use for upserting. This tempid will not be reported in the TxReport. - let db_id: entmod::EntidOrLookupRefOrTempId = mentat_tx_parser::remove_db_id(&mut map_notation)?.unwrap_or_else(&mut allocate_mentat_id); + let db_id: entmod::EntidOrLookupRefOrTempId = mentat_tx_parser::remove_db_id(&mut map_notation)?.unwrap_or_else(|| in_process.allocate_mentat_id()); // We're not nested, so :db/isComponent is not relevant. We just explode the // map notation. @@ -255,7 +276,7 @@ impl<'conn, 'a> Tx<'conn, 'a> { let v = match v { entmod::AtomOrLookupRefOrVectorOrMapNotation::Atom(v) => { if attribute.value_type == ValueType::Ref && v.inner.is_text() { - Either::Right(LookupRefOrTempId::TempId(temp_ids.intern(v.inner.as_text().cloned().map(TempId::External).unwrap()))) + Either::Right(LookupRefOrTempId::TempId(in_process.intern_temp_id(v.inner.as_text().cloned().map(TempId::External).unwrap()))) } 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 @@ -270,7 +291,7 @@ impl<'conn, 'a> Tx<'conn, 'a> { bail!(ErrorKind::NotYetImplemented(format!("Cannot resolve value lookup ref for attribute {} that is not :db/valueType :db.type/ref", a))) } - Either::Right(LookupRefOrTempId::LookupRef(intern_lookup_ref(&mut lookup_refs, lookup_ref)?)) + Either::Right(LookupRefOrTempId::LookupRef(in_process.intern_lookup_ref(lookup_ref)?)) }, entmod::AtomOrLookupRefOrVectorOrMapNotation::Vector(vs) => { @@ -306,7 +327,7 @@ impl<'conn, 'a> Tx<'conn, 'a> { // to use for upserting. This tempid will not be reported in the TxReport. let db_id: Option = mentat_tx_parser::remove_db_id(&mut map_notation)?; let mut dangling = db_id.is_none(); - let db_id: entmod::EntidOrLookupRefOrTempId = db_id.unwrap_or_else(&mut allocate_mentat_id); + let db_id: entmod::EntidOrLookupRefOrTempId = db_id.unwrap_or_else(|| in_process.allocate_mentat_id()); // We're nested, so we want to ensure we're not creating "dangling" // entities that can't be reached. If we're :db/isComponent, then this @@ -357,11 +378,11 @@ impl<'conn, 'a> Tx<'conn, 'a> { }, entmod::EntidOrLookupRefOrTempId::TempId(e) => { - Either::Right(LookupRefOrTempId::TempId(temp_ids.intern(e))) + Either::Right(LookupRefOrTempId::TempId(in_process.intern_temp_id(e))) }, entmod::EntidOrLookupRefOrTempId::LookupRef(ref lookup_ref) => { - Either::Right(LookupRefOrTempId::LookupRef(intern_lookup_ref(&mut lookup_refs, lookup_ref)?)) + Either::Right(LookupRefOrTempId::LookupRef(in_process.intern_lookup_ref(lookup_ref)?)) }, } }, @@ -377,11 +398,11 @@ impl<'conn, 'a> Tx<'conn, 'a> { }, entmod::EntidOrLookupRefOrTempId::TempId(e) => { - Either::Right(LookupRefOrTempId::TempId(temp_ids.intern(e))) + Either::Right(LookupRefOrTempId::TempId(in_process.intern_temp_id(e))) }, entmod::EntidOrLookupRefOrTempId::LookupRef(ref lookup_ref) => { - Either::Right(LookupRefOrTempId::LookupRef(intern_lookup_ref(&mut lookup_refs, lookup_ref)?)) + Either::Right(LookupRefOrTempId::LookupRef(in_process.intern_lookup_ref(lookup_ref)?)) }, }; @@ -389,7 +410,7 @@ impl<'conn, 'a> Tx<'conn, 'a> { }, } }; - Ok((terms, temp_ids, lookup_refs)) + Ok((terms, in_process.temp_ids, in_process.lookup_refs)) } /// Pipeline stage 2: rewrite `Term` instances with lookup refs into `Term` instances without From 0be78cf9567237e3f555dbc1791e55e42547a8fc Mon Sep 17 00:00:00 2001 From: Nick Alexander Date: Tue, 6 Jun 2017 13:18:47 -0700 Subject: [PATCH 5/9] Pre: Extract entity_*_into_term_* helpers. --- db/src/tx.rs | 92 +++++++++++++++++++++++----------------------------- 1 file changed, 41 insertions(+), 51 deletions(-) diff --git a/db/src/tx.rs b/db/src/tx.rs index 6095eded..6cdf30a8 100644 --- a/db/src/tx.rs +++ b/db/src/tx.rs @@ -62,14 +62,16 @@ use entids; use errors::{ErrorKind, Result}; use internal_types::{ Either, + EntidOr, LookupRef, LookupRefOrTempId, TempIdHandle, TempIdMap, Term, - TermWithTempIdsAndLookupRefs, TermWithTempIds, + TermWithTempIdsAndLookupRefs, TermWithoutTempIds, + TypedValueOr, replace_lookup_ref}; use mentat_core::{ @@ -234,6 +236,39 @@ impl<'conn, 'a> Tx<'conn, 'a> { self.mentat_id_count += 1; entmod::EntidOrLookupRefOrTempId::TempId(TempId::Internal(self.mentat_id_count)) } + + fn entity_e_into_term_e(&mut self, x: entmod::EntidOrLookupRefOrTempId) -> Result> { + match x { + entmod::EntidOrLookupRefOrTempId::Entid(e) => { + let e: i64 = match e { + entmod::Entid::Entid(ref e) => *e, + entmod::Entid::Ident(ref e) => self.schema.require_entid(&e)?, + }; + Ok(Either::Left(e)) + }, + + entmod::EntidOrLookupRefOrTempId::TempId(e) => { + Ok(Either::Right(LookupRefOrTempId::TempId(self.intern_temp_id(e)))) + }, + + entmod::EntidOrLookupRefOrTempId::LookupRef(ref lookup_ref) => { + Ok(Either::Right(LookupRefOrTempId::LookupRef(self.intern_lookup_ref(lookup_ref)?))) + }, + } + } + + fn entity_a_into_term_a(&mut self, x: entmod::Entid) -> Result<(Entid, &'a Attribute)> { + 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)) + } + + 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)) + } } let mut in_process = InProcess::with_schema(&self.schema); @@ -266,12 +301,7 @@ impl<'conn, 'a> Tx<'conn, 'a> { }, Entity::AddOrRetract { op, e, a, v } => { - let a: i64 = match a { - 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)?; + let (a, attribute) = in_process.entity_a_into_term_a(a)?; let v = match v { entmod::AtomOrLookupRefOrVectorOrMapNotation::Atom(v) => { @@ -343,12 +373,8 @@ impl<'conn, 'a> Tx<'conn, 'a> { } for (inner_a, inner_v) in map_notation { - let inner_entid: i64 = match inner_a { - entmod::Entid::Entid(ref a) => *a, - entmod::Entid::Ident(ref a) => self.schema.require_entid(&a)?, - }; + let (inner_a, inner_attribute) = in_process.entity_a_into_term_a(inner_a)?; - let inner_attribute: &Attribute = self.schema.require_attribute_for_entid(inner_entid)?; if inner_attribute.unique == Some(attribute::Unique::Identity) { dangling = false; } @@ -356,7 +382,7 @@ impl<'conn, 'a> Tx<'conn, 'a> { deque.push_front(Entity::AddOrRetract { op: OpType::Add, e: db_id.clone(), - a: entmod::Entid::Entid(inner_entid), + a: entmod::Entid::Entid(inner_a), v: inner_v, }); } @@ -365,47 +391,11 @@ impl<'conn, 'a> Tx<'conn, 'a> { bail!(ErrorKind::NotYetImplemented(format!("Cannot explode nested map value that would lead to dangling entity for attribute {}", a))); } - // Similar, but not identical, to the expansion of the entity position e - // below. This returns Either::Left(TypedValue) instances; that returns - // Either::Left(i64) instances. - match db_id { - entmod::EntidOrLookupRefOrTempId::Entid(e) => { - let e: i64 = match e { - entmod::Entid::Entid(ref e) => *e, - entmod::Entid::Ident(ref e) => self.schema.require_entid(&e)?, - }; - Either::Left(TypedValue::Ref(e)) - }, - - entmod::EntidOrLookupRefOrTempId::TempId(e) => { - Either::Right(LookupRefOrTempId::TempId(in_process.intern_temp_id(e))) - }, - - entmod::EntidOrLookupRefOrTempId::LookupRef(ref lookup_ref) => { - Either::Right(LookupRefOrTempId::LookupRef(in_process.intern_lookup_ref(lookup_ref)?)) - }, - } - }, - }; - - let e = match e { - entmod::EntidOrLookupRefOrTempId::Entid(e) => { - let e: i64 = match e { - entmod::Entid::Entid(ref e) => *e, - entmod::Entid::Ident(ref e) => self.schema.require_entid(&e)?, - }; - Either::Left(e) - }, - - entmod::EntidOrLookupRefOrTempId::TempId(e) => { - Either::Right(LookupRefOrTempId::TempId(in_process.intern_temp_id(e))) - }, - - entmod::EntidOrLookupRefOrTempId::LookupRef(ref lookup_ref) => { - Either::Right(LookupRefOrTempId::LookupRef(in_process.intern_lookup_ref(lookup_ref)?)) + in_process.entity_e_into_term_v(db_id)? }, }; + let e = in_process.entity_e_into_term_e(e)?; terms.push(Term::AddOrRetract(op, e, a, v)); }, } From d88823e7c4638a7f2e5f1ab96ea1ff04ac06fda6 Mon Sep 17 00:00:00 2001 From: Nick Alexander Date: Tue, 6 Jun 2017 15:21:51 -0700 Subject: [PATCH 6/9] 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, From eb220528bfc1cf459f82b2cabebb7883eaebc38b Mon Sep 17 00:00:00 2001 From: Nick Alexander Date: Tue, 6 Jun 2017 15:38:38 -0700 Subject: [PATCH 7/9] Post: Indent. --- db/src/tx.rs | 204 +++++++++++++++++++++++++-------------------------- 1 file changed, 102 insertions(+), 102 deletions(-) diff --git a/db/src/tx.rs b/db/src/tx.rs index e755d292..c3a2d4fb 100644 --- a/db/src/tx.rs +++ b/db/src/tx.rs @@ -297,7 +297,7 @@ impl<'conn, 'a> Tx<'conn, 'a> { // 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)? { + 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. @@ -356,115 +356,115 @@ impl<'conn, 'a> Tx<'conn, 'a> { } else { let (a, attribute) = in_process.entity_a_into_term_a(a)?; - let v = match v { - entmod::AtomOrLookupRefOrVectorOrMapNotation::Atom(v) => { - if attribute.value_type == ValueType::Ref && v.inner.is_text() { - Either::Right(LookupRefOrTempId::TempId(in_process.intern_temp_id(v.inner.as_text().cloned().map(TempId::External).unwrap()))) - } 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. - let typed_value: TypedValue = self.schema.to_typed_value(&v.without_spans(), attribute.value_type)?; - Either::Left(typed_value) - } - }, - - entmod::AtomOrLookupRefOrVectorOrMapNotation::LookupRef(ref lookup_ref) => { - if attribute.value_type != ValueType::Ref { - bail!(ErrorKind::NotYetImplemented(format!("Cannot resolve value lookup ref for attribute {} that is not :db/valueType :db.type/ref", a))) - } - - Either::Right(LookupRefOrTempId::LookupRef(in_process.intern_lookup_ref(lookup_ref)?)) - }, - - entmod::AtomOrLookupRefOrVectorOrMapNotation::Vector(vs) => { - if !attribute.multival { - bail!(ErrorKind::NotYetImplemented(format!("Cannot explode vector value for attribute {} that is not :db.cardinality :db.cardinality/many", a))); - } - - for vv in vs { - deque.push_front(Entity::AddOrRetract { - op: op.clone(), - e: e.clone(), - a: entmod::Entid::Entid(a), - v: vv, - }); - } - continue - }, - - entmod::AtomOrLookupRefOrVectorOrMapNotation::MapNotation(mut map_notation) => { - // TODO: consider handling this at the tx-parser level. That would be - // more strict and expressive, but it would lead to splitting - // AddOrRetract, which proliferates types and code, or only handling - // nested maps rather than map values, like Datomic does. - if op != OpType::Add { - bail!(ErrorKind::NotYetImplemented(format!("Cannot explode nested map value in :db/retract for attribute {}", a))); - } - - if attribute.value_type != ValueType::Ref { - bail!(ErrorKind::NotYetImplemented(format!("Cannot explode nested map value for attribute {} that is not :db/valueType :db.type/ref", a))) - } - - // :db/id is optional; if it's not given, we generate a special internal tempid - // to use for upserting. This tempid will not be reported in the TxReport. - let db_id: Option = mentat_tx_parser::remove_db_id(&mut map_notation)?; - let mut dangling = db_id.is_none(); - let db_id: entmod::EntidOrLookupRefOrTempId = db_id.unwrap_or_else(|| in_process.allocate_mentat_id()); - - // We're nested, so we want to ensure we're not creating "dangling" - // entities that can't be reached. If we're :db/isComponent, then this - // is not dangling. Otherwise, the resulting map needs to have a - // :db/unique :db.unique/identity [a v] pair, so that it's reachable. - // Per http://docs.datomic.com/transactions.html: "Either the reference - // to the nested map must be a component attribute, or the nested map - // must include a unique attribute. This constraint prevents the - // accidental creation of easily-orphaned entities that have no identity - // or relation to other entities." - if attribute.component { - dangling = false; - } - - for (inner_a, inner_v) in map_notation { - 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; - - 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)); + let v = match v { + entmod::AtomOrLookupRefOrVectorOrMapNotation::Atom(v) => { + if attribute.value_type == ValueType::Ref && v.inner.is_text() { + Either::Right(LookupRefOrTempId::TempId(in_process.intern_temp_id(v.inner.as_text().cloned().map(TempId::External).unwrap()))) } 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; - } + // 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. + let typed_value: TypedValue = self.schema.to_typed_value(&v.without_spans(), attribute.value_type)?; + Either::Left(typed_value) + } + }, + entmod::AtomOrLookupRefOrVectorOrMapNotation::LookupRef(ref lookup_ref) => { + if attribute.value_type != ValueType::Ref { + bail!(ErrorKind::NotYetImplemented(format!("Cannot resolve value lookup ref for attribute {} that is not :db/valueType :db.type/ref", a))) + } + + Either::Right(LookupRefOrTempId::LookupRef(in_process.intern_lookup_ref(lookup_ref)?)) + }, + + entmod::AtomOrLookupRefOrVectorOrMapNotation::Vector(vs) => { + if !attribute.multival { + bail!(ErrorKind::NotYetImplemented(format!("Cannot explode vector value for attribute {} that is not :db.cardinality :db.cardinality/many", a))); + } + + for vv in vs { deque.push_front(Entity::AddOrRetract { - op: OpType::Add, - e: db_id.clone(), - a: entmod::Entid::Entid(inner_a), - v: inner_v, + op: op.clone(), + e: e.clone(), + a: entmod::Entid::Entid(a), + v: vv, }); } - } + continue + }, - if dangling { - bail!(ErrorKind::NotYetImplemented(format!("Cannot explode nested map value that would lead to dangling entity for attribute {}", a))); - } + entmod::AtomOrLookupRefOrVectorOrMapNotation::MapNotation(mut map_notation) => { + // TODO: consider handling this at the tx-parser level. That would be + // more strict and expressive, but it would lead to splitting + // AddOrRetract, which proliferates types and code, or only handling + // nested maps rather than map values, like Datomic does. + if op != OpType::Add { + bail!(ErrorKind::NotYetImplemented(format!("Cannot explode nested map value in :db/retract for attribute {}", a))); + } - in_process.entity_e_into_term_v(db_id)? - }, - }; + if attribute.value_type != ValueType::Ref { + bail!(ErrorKind::NotYetImplemented(format!("Cannot explode nested map value for attribute {} that is not :db/valueType :db.type/ref", a))) + } - let e = in_process.entity_e_into_term_e(e)?; - terms.push(Term::AddOrRetract(op, e, a, v)); - } + // :db/id is optional; if it's not given, we generate a special internal tempid + // to use for upserting. This tempid will not be reported in the TxReport. + let db_id: Option = mentat_tx_parser::remove_db_id(&mut map_notation)?; + let mut dangling = db_id.is_none(); + let db_id: entmod::EntidOrLookupRefOrTempId = db_id.unwrap_or_else(|| in_process.allocate_mentat_id()); + + // We're nested, so we want to ensure we're not creating "dangling" + // entities that can't be reached. If we're :db/isComponent, then this + // is not dangling. Otherwise, the resulting map needs to have a + // :db/unique :db.unique/identity [a v] pair, so that it's reachable. + // Per http://docs.datomic.com/transactions.html: "Either the reference + // to the nested map must be a component attribute, or the nested map + // must include a unique attribute. This constraint prevents the + // accidental creation of easily-orphaned entities that have no identity + // or relation to other entities." + if attribute.component { + dangling = false; + } + + for (inner_a, inner_v) in map_notation { + 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; + + 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 { + bail!(ErrorKind::NotYetImplemented(format!("Cannot explode nested map value that would lead to dangling entity for attribute {}", a))); + } + + in_process.entity_e_into_term_v(db_id)? + }, + }; + + let e = in_process.entity_e_into_term_e(e)?; + terms.push(Term::AddOrRetract(op, e, a, v)); + } }, } }; From 59a710f80fd8f7e69a83771afc480ed9eceedd28 Mon Sep 17 00:00:00 2001 From: Nick Alexander Date: Wed, 7 Jun 2017 13:37:40 -0700 Subject: [PATCH 8/9] 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)] From c1659726848e26c3f0c4eabc9b873ad442f5104a Mon Sep 17 00:00:00 2001 From: Nick Alexander Date: Wed, 7 Jun 2017 14:17:56 -0700 Subject: [PATCH 9/9] Post: Reject at parse-time reversed attributes in direct notation with bad values. This is an optimization that trades rejecting inputs earlier at the cost of expressive error messages. It should be possible to recover the error messages, however. This will reject input like `[:db/{add,retract} v :attribute/_reversed NOT-AN-ENTITY]`. --- db/src/db.rs | 36 +++++++++++++++++-------- parser-utils/src/value_and_span.rs | 20 ++++++++++++++ tx-parser/src/lib.rs | 42 ++++++++++++++++++++++-------- tx-parser/tests/parser.rs | 18 +++++++++++++ 4 files changed, 94 insertions(+), 22 deletions(-) diff --git a/db/src/db.rs b/db/src/db.rs index e26395f0..7df269ab 100644 --- a/db/src/db.rs +++ b/db/src/db.rs @@ -2085,7 +2085,7 @@ mod tests { // Verify that we can explode map notation with nested maps, even if the inner map would be // dangling, if we give a :db/id explicitly. - assert_transact!(conn, "[{:test/dangling {:db/id \"t\" :test/many 11}}]"); + assert_transact!(conn, "[{:test/dangling {:db/id \"t\" :test/many 12}}]"); } #[test] @@ -2191,21 +2191,35 @@ mod tests { 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}]]", - Err("not yet implemented: Cannot explode map notation value in :attr/_reversed notation for attribute 444")); + } + + #[test] + fn test_explode_reversed_notation_errors() { + 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]]"); + + // `tx-parser` should fail to parse direct reverse notation with nested value maps and + // nested value vectors, so we only test things that "get through" to the map notation + // dynamic processor here. // Verify that we can't explode reverse notation in map notation with nested value maps. assert_transact!(conn, - "[{:test/_dangling {:test/many 11}}]", + "[{:test/_dangling {:test/many 14}}]", 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]}]", diff --git a/parser-utils/src/value_and_span.rs b/parser-utils/src/value_and_span.rs index 20b9de3c..ba073ad7 100644 --- a/parser-utils/src/value_and_span.rs +++ b/parser-utils/src/value_and_span.rs @@ -452,6 +452,26 @@ pub fn namespaced_keyword<'a>() -> Expected, fn(Stream<'a>) parser(namespaced_keyword_ as fn(Stream<'a>) -> ParseResult<&'a edn::NamespacedKeyword, Stream<'a>>).expected("namespaced_keyword") } +pub fn forward_keyword_<'a>(input: Stream<'a>) -> ParseResult<&'a edn::NamespacedKeyword, Stream<'a>> { + satisfy_map(|v: &'a edn::ValueAndSpan| v.inner.as_namespaced_keyword().and_then(|k| if k.is_forward() { Some(k) } else { None })) + .parse_lazy(input) + .into() +} + +pub fn forward_keyword<'a>() -> Expected, fn(Stream<'a>) -> ParseResult<&'a edn::NamespacedKeyword, Stream<'a>>>> { + parser(forward_keyword_ as fn(Stream<'a>) -> ParseResult<&'a edn::NamespacedKeyword, Stream<'a>>).expected("forward_keyword") +} + +pub fn backward_keyword_<'a>(input: Stream<'a>) -> ParseResult<&'a edn::NamespacedKeyword, Stream<'a>> { + satisfy_map(|v: &'a edn::ValueAndSpan| v.inner.as_namespaced_keyword().and_then(|k| if k.is_backward() { Some(k) } else { None })) + .parse_lazy(input) + .into() +} + +pub fn backward_keyword<'a>() -> Expected, fn(Stream<'a>) -> ParseResult<&'a edn::NamespacedKeyword, Stream<'a>>>> { + parser(backward_keyword_ as fn(Stream<'a>) -> ParseResult<&'a edn::NamespacedKeyword, Stream<'a>>).expected("backward_keyword") +} + /// Generate a `satisfy` expression that matches a `PlainSymbol` value with the given name. /// /// We do this rather than using `combine::token` so that we don't need to allocate a new `String` diff --git a/tx-parser/src/lib.rs b/tx-parser/src/lib.rs index e92ccf35..875303e3 100644 --- a/tx-parser/src/lib.rs +++ b/tx-parser/src/lib.rs @@ -45,6 +45,8 @@ use mentat_parser_utils::{ResultParser}; use mentat_parser_utils::value_and_span::{ Item, OfExactlyParsing, + backward_keyword, + forward_keyword, integer, list, map, @@ -57,12 +59,25 @@ pub use errors::*; pub struct Tx<'a>(std::marker::PhantomData<&'a ()>); +// Accepts entid, :attribute/forward, and :attribute/_backward. def_parser!(Tx, entid, Entid, { integer() .map(|x| Entid::Entid(x)) .or(namespaced_keyword().map(|x| Entid::Ident(x.clone()))) }); +// Accepts entid and :attribute/forward. +def_parser!(Tx, forward_entid, Entid, { + integer() + .map(|x| Entid::Entid(x)) + .or(forward_keyword().map(|x| Entid::Ident(x.clone()))) +}); + +// Accepts only :attribute/_backward. +def_parser!(Tx, backward_entid, Entid, { + backward_keyword().map(|x| Entid::Ident(x.to_reversed())) +}); + def_matches_plain_symbol!(Tx, literal_lookup_ref, "lookup-ref"); def_parser!(Tx, lookup_ref, LookupRef, { @@ -106,18 +121,23 @@ def_matches_namespaced_keyword!(Tx, literal_db_retract, "db", "retract"); def_parser!(Tx, add_or_retract, Entity, { vector().of_exactly( + // TODO: This commits as soon as it sees :db/{add,retract}, but could use an improved error message. (Tx::literal_db_add().map(|_| OpType::Add).or(Tx::literal_db_retract().map(|_| OpType::Retract)), - Tx::entid_or_lookup_ref_or_temp_id(), - Tx::entid(), - Tx::atom_or_lookup_ref_or_vector()) - .map(|(op, e, a, v)| { - Entity::AddOrRetract { - op: op, - e: e, - a: a, - v: v, - } - })) + try((Tx::entid_or_lookup_ref_or_temp_id(), + Tx::forward_entid(), + Tx::atom_or_lookup_ref_or_vector())) + .or(try((Tx::atom_or_lookup_ref_or_vector(), + Tx::backward_entid(), + Tx::entid_or_lookup_ref_or_temp_id())) + .map(|(v, a, e)| (e, a, v)))) + .map(|(op, (e, a, v))| { + Entity::AddOrRetract { + op: op, + e: e, + a: a, + v: v, + } + })) }); def_parser!(Tx, map_notation, MapNotation, { diff --git a/tx-parser/tests/parser.rs b/tx-parser/tests/parser.rs index 67d60631..8602487d 100644 --- a/tx-parser/tests/parser.rs +++ b/tx-parser/tests/parser.rs @@ -85,4 +85,22 @@ fn test_entities() { ]); } +#[test] +fn test_reverse_notation_illegal_nested_values() { + // Verify that we refuse to parse direct reverse notation with nested value maps or vectors. + + let input = "[[:db/add 100 :test/_dangling {:test/many 13}]]"; + let edn = parse::value(input).expect("to parse test input"); + let result = Tx::parse(&edn); + // TODO: it would be much better to assert details about the error (here and below), but right + // now the error message isn't clear that the given value isn't valid for the backward attribute + // :test/_dangling. + assert!(result.is_err()); + + let input = "[[:db/add 100 :test/_dangling [:test/many 13]]]"; + let edn = parse::value(input).expect("to parse test input"); + let result = Tx::parse(&edn); + assert!(result.is_err()); +} + // TODO: test error handling in select cases.