From c1659726848e26c3f0c4eabc9b873ad442f5104a Mon Sep 17 00:00:00 2001 From: Nick Alexander Date: Wed, 7 Jun 2017 14:17:56 -0700 Subject: [PATCH] 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.