From 58e43a878ae6e901aed4916bea9d6bf5553f39b6 Mon Sep 17 00:00:00 2001
From: Emily Toop <etoop@mozilla.com>
Date: Fri, 23 Jun 2017 12:59:54 +0100
Subject: [PATCH] Address review comments r=rnewman

---
 query-algebrizer/src/clauses/mod.rs     |  9 +--------
 query-algebrizer/src/clauses/pattern.rs |  5 +++--
 tests/query.rs                          | 23 ++++++++++++++++++-----
 3 files changed, 22 insertions(+), 15 deletions(-)

diff --git a/query-algebrizer/src/clauses/mod.rs b/query-algebrizer/src/clauses/mod.rs
index d13a0cff..7ee93032 100644
--- a/query-algebrizer/src/clauses/mod.rs
+++ b/query-algebrizer/src/clauses/mod.rs
@@ -611,15 +611,8 @@ impl ConjoiningClauses {
     }
 
     /// Ensure that the given place has the correct types to be a tx-id.
-    /// Right now this is mostly unimplemented: we fail hard if anything but a placeholder is
-    /// present.
     fn constrain_to_tx(&mut self, tx: &PatternNonValuePlace) {
-        match *tx {
-            PatternNonValuePlace::Variable(ref v) => self.constrain_var_to_type(v.clone(), ValueType::Ref),
-            PatternNonValuePlace::Placeholder |
-            PatternNonValuePlace::Entid(_) |
-            PatternNonValuePlace::Ident(_) => (),
-        }
+        self.constrain_to_ref(tx);
     }
 
     /// Ensure that the given place can be an entity, and is congruent with existing types.
diff --git a/query-algebrizer/src/clauses/pattern.rs b/query-algebrizer/src/clauses/pattern.rs
index e6050eed..41d09024 100644
--- a/query-algebrizer/src/clauses/pattern.rs
+++ b/query-algebrizer/src/clauses/pattern.rs
@@ -241,14 +241,15 @@ impl ConjoiningClauses {
             },
         }
 
-        // TODO pattern.tx -> like handling entity place 
-        // carefully read Datomic query doc to check for correct handling
         match pattern.tx {
             PatternNonValuePlace::Placeholder => (),
             PatternNonValuePlace::Variable(ref v) => {
                 self.bind_column_to_var(schema, col.clone(), DatomsColumn::Tx, v.clone());
             },
             PatternNonValuePlace::Entid(entid) => {
+                // TODO: we want to check whether the tx-id is within range for the database's tx partition. 
+                // (That applies after ident lookup, too.)
+                // Possible solution: https://github.com/mozilla/mentat/tree/fluffyemily/tx-id-check
                 self.constrain_column_to_entity(col.clone(), DatomsColumn::Tx, entid);
             },
             PatternNonValuePlace::Ident(ref ident) => {
diff --git a/tests/query.rs b/tests/query.rs
index 8365908b..6bf2853a 100644
--- a/tests/query.rs
+++ b/tests/query.rs
@@ -263,10 +263,16 @@ fn test_tx() {
         [:db/add "s" :db/ident :foo/uuid]
         [:db/add "s" :db/valueType :db.type/uuid]
         [:db/add "s" :db/cardinality :db.cardinality/one]
-    ]"#).unwrap();
+    ]"#).expect("successful transaction");
+
     let t = conn.transact(&mut c, r#"[
         [:db/add "u" :foo/uuid #uuid "cf62d552-6569-4d1b-b667-04703041dfc4"]
-    ]"#).unwrap();
+    ]"#).expect("successful transaction");
+
+    conn.transact(&mut c, r#"[
+        [:db/add "u" :foo/uuid #uuid "550e8400-e29b-41d4-a716-446655440000"]
+    ]"#).expect("successful transaction");
+
     let r = conn.q_once(&mut c,
                         r#"[:find ?tx 
                             :where [?x :foo/uuid #uuid "cf62d552-6569-4d1b-b667-04703041dfc4" ?tx]]"#, None);
@@ -288,10 +294,17 @@ fn test_tx_as_input() {
         [:db/add "s" :db/ident :foo/uuid]
         [:db/add "s" :db/valueType :db.type/uuid]
         [:db/add "s" :db/cardinality :db.cardinality/one]
-    ]"#).unwrap();
+    ]"#).expect("successful transaction");
+    conn.transact(&mut c, r#"[
+        [:db/add "u" :foo/uuid #uuid "550e8400-e29b-41d4-a716-446655440000"]
+    ]"#).expect("successful transaction");
     let t = conn.transact(&mut c, r#"[
         [:db/add "u" :foo/uuid #uuid "cf62d552-6569-4d1b-b667-04703041dfc4"]
-    ]"#).unwrap();
+    ]"#).expect("successful transaction");
+    conn.transact(&mut c, r#"[
+        [:db/add "u" :foo/uuid #uuid "267bab92-ee39-4ca2-b7f0-1163a85af1fb"]
+    ]"#).expect("successful transaction");
+
     let tx = (Variable::from_valid_name("?tx"), TypedValue::Ref(t.tx_id));
     let inputs = QueryInputs::with_value_sequence(vec![tx]);
     let r = conn.q_once(&mut c,
@@ -301,7 +314,7 @@ fn test_tx_as_input() {
     match r {
         Result::Ok(QueryResults::Rel(ref v)) => {
             assert_eq!(*v, vec![
-                vec![TypedValue::Uuid(Uuid::from_str("cf62d552-6569-4d1b-b667-04703041dfc4").unwrap()),]
+                vec![TypedValue::Uuid(Uuid::from_str("cf62d552-6569-4d1b-b667-04703041dfc4").expect("Valid UUID")),]
             ]);
         },
         _ => panic!("Expected query to work."),