Fix query

crates/hdk/CHANGELOG.md

@@ -22,6 +22,9 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/).
 - **BREAKING CHANGE:** `create_entry` takes `ScopedEntryDefIndex: TryFrom<T>` instead of `EntryDefIndex: TryFrom<T>`.
 - **BREAKING CHANGE:** `get_links` and `get_link_details` take `impl LinkTypeFilterExt` instead of `TryInto<LinkTypeRanges>`.
 - hdk: **BREAKING CHANGE** `x_salsa20_poly1305_*` functions have been properly implemented. Any previous `KeyRef`s will no longer work. These new functions DO NOT work with legacy lair `v0.0.z`, you must use NEW lair `v0.y.z` (v0.2.0 as of this PR). [\#1446](https://github.com/holochain/holochain/pull/1446)
+- Fixed `hdk::query`, which was showing some incorrect behavior [\#1402](https://github.com/holochain/holochain/pull/1402):
+  - When using `ChainQueryFilterRange::ActionHashRange`, extraneous elements from other authors could be returned.
+  - Certain combinations of filters, like hash-bounded ranges and header type filters, are currently implemented incorrectly and lead to undefined behavior. Filter combinations which are unsupported now result in `SourceChainError::UnsupportedQuery`.
 
 ## 0.0.138
 
@@ -173,14 +176,14 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/).
 - hdk: Now supports deserializing countersigned entries in app entry `try_from`
 
 - hdk: implements multi-call for:
-  
+
   - `remote_call`
   - `call`
   - `get`
   - `get_details`
   - `get_links`
   - `get_link_details`
-  
+
   We strictly only needed `remote_call` for countersigning, but feedback from the community was that having to sequentially loop over these common HDK functions is a pain point, so we enabled all of them to be async over a vector of inputs.
 
 ## 0.0.102

crates/holo_hash/src/hash.rs

@@ -157,6 +157,16 @@ impl<T: HashType> HoloHash<T> {
         assert_length!(HOLO_HASH_FULL_LEN, &self.hash);
         self.hash
     }
+
+    /// Get the hex representation of the hash bytes
+    pub fn to_hex(&self) -> String {
+        use std::fmt::Write;
+        let mut s = String::with_capacity(self.hash.len());
+        for b in &self.hash {
+            write!(&mut s, "{:02x}", b).ok();
+        }
+        s
+    }
 }
 
 #[cfg(feature = "hashing")]

crates/holochain_state/src/source_chain.rs

@@ -694,6 +694,14 @@ where
     /// used by the `query` host function, which crosses the wasm boundary
     // FIXME: This query needs to be tested.
     pub async fn query(&self, query: QueryFilter) -> SourceChainResult<Vec<Record>> {
+        if query.sequence_range != ChainQueryFilterRange::Unbounded
+            && (query.action_type.is_some()
+                || query.entry_type.is_some()
+                || query.entry_hashes.is_some()
+                || query.include_entries)
+        {
+            return Err(SourceChainError::UnsupportedQuery(query));
+        }
         let author = self.author.clone();
         let public_only = self.public_only;
         let mut records = self
@@ -731,7 +739,8 @@ where
                 WHERE
                 Action.author = :author
                 AND
-                (:range_start IS NULL AND :range_end IS NULL AND :range_start_hash IS NULL AND :range_end_hash IS NULL AND :range_prior_count IS NULL)
+                (
+                    (:range_start IS NULL AND :range_end IS NULL AND :range_start_hash IS NULL AND :range_end_hash IS NULL AND :range_prior_count IS NULL)
                 ",
                     );
                     sql.push_str(match query.sequence_range {
@@ -741,20 +750,21 @@ where
                         ChainQueryFilterRange::ActionHashRange(_, _) => "
                         OR (
                             Action.seq BETWEEN
-                            (SELECT Action.seq WHERE Action.hash = :range_start_hash)
+                            (SELECT Action.seq from Action WHERE Action.hash = :range_start_hash)
                             AND
-                            (SELECT Action.seq WHERE Action.hash = :range_end_hash)
+                            (SELECT Action.seq from Action WHERE Action.hash = :range_end_hash)
                         )",
                         ChainQueryFilterRange::ActionHashTerminated(_, _) => "
                         OR (
                             Action.seq BETWEEN
-                            (SELECT Action.seq WHERE Action.hash = :range_end_hash) - :range_prior_count
+                            (SELECT Action.seq from Action WHERE Action.hash = :range_end_hash) - :range_prior_count
                             AND
-                            (SELECT Action.seq WHERE Action.hash = :range_end_hash)
+                            (SELECT Action.seq from Action WHERE Action.hash = :range_end_hash)
                         )",
                     });
                     sql.push_str(
                         "
+                )
                 AND
                 (:entry_type IS NULL OR Action.entry_type = :entry_type)
                 AND
@@ -1891,4 +1901,127 @@ pub mod tests {
 
         Ok(())
     }
+
+    #[tokio::test(flavor = "multi_thread")]
+    async fn source_chain_query() {
+        let test_db = test_authored_db();
+        let dht_db = test_dht_db();
+        let dht_db_cache = DhtDbQueryCache::new(dht_db.to_db().into());
+        let keystore = test_keystore();
+        let vault = test_db.to_db();
+        let alice = keystore.new_sign_keypair_random().await.unwrap();
+        let bob = keystore.new_sign_keypair_random().await.unwrap();
+        let dna_hash = fixt!(DnaHash);
+
+        genesis(
+            vault.clone().into(),
+            dht_db.to_db(),
+            &dht_db_cache,
+            keystore.clone(),
+            dna_hash.clone(),
+            alice.clone(),
+            None,
+        )
+        .await
+        .unwrap();
+
+        genesis(
+            vault.clone().into(),
+            dht_db.to_db(),
+            &dht_db_cache,
+            keystore.clone(),
+            dna_hash.clone(),
+            bob.clone(),
+            None,
+        )
+        .await
+        .unwrap();
+
+        test_db.dump_tmp();
+
+        let chain = SourceChain::new(vault, dht_db.to_db(), dht_db_cache, keystore, alice.clone())
+            .await
+            .unwrap();
+
+        let elements = chain.query(ChainQueryFilter::default()).await.unwrap();
+
+        // All of the range queries which should return a full set of elements
+        let full_ranges = [
+            ChainQueryFilterRange::Unbounded,
+            ChainQueryFilterRange::ActionSeqRange(0, 2),
+            ChainQueryFilterRange::ActionHashRange(
+                elements[0].action_address().clone(),
+                elements[2].action_address().clone(),
+            ),
+            ChainQueryFilterRange::ActionHashTerminated(elements[2].action_address().clone(), 2),
+        ];
+
+        // A variety of combinations of query parameters
+        let cases = [
+            ((None, None, vec![], false), 3),
+            ((None, None, vec![], true), 3),
+            ((Some(ActionType::Dna), None, vec![], false), 1),
+            ((None, Some(EntryType::AgentPubKey), vec![], false), 1),
+            ((None, Some(EntryType::AgentPubKey), vec![], true), 1),
+            ((Some(ActionType::Create), None, vec![], false), 1),
+            ((Some(ActionType::Create), None, vec![], true), 1),
+            (
+                (
+                    Some(ActionType::Create),
+                    Some(EntryType::AgentPubKey),
+                    vec![],
+                    false,
+                ),
+                1,
+            ),
+            (
+                (
+                    Some(ActionType::Create),
+                    Some(EntryType::AgentPubKey),
+                    vec![elements[2].action().entry_hash().unwrap().clone()],
+                    true,
+                ),
+                1,
+            ),
+        ];
+
+        // Test all permutations of cases defined with all full range queries,
+        // and both boolean values of `include_entries`.
+        for ((action_type, entry_type, entry_hashes, include_entries), num_expected) in cases {
+            let entry_hashes = if entry_hashes.is_empty() {
+                None
+            } else {
+                Some(entry_hashes.into_iter().collect())
+            };
+            for sequence_range in full_ranges.clone() {
+                let query = ChainQueryFilter {
+                    sequence_range: sequence_range.clone(),
+                    action_type: action_type.clone(),
+                    entry_type: entry_type.clone(),
+                    entry_hashes: entry_hashes.clone(),
+                    include_entries,
+                };
+                if sequence_range != ChainQueryFilterRange::Unbounded
+                    && (action_type.is_some()
+                        || entry_type.is_some()
+                        || entry_hashes.is_some()
+                        || include_entries)
+                {
+                    assert!(matches!(
+                        chain.query(query.clone()).await,
+                        Err(SourceChainError::UnsupportedQuery(_))
+                    ));
+                } else {
+                    let queried = chain.query(query.clone()).await.unwrap();
+                    let actual = queried.len();
+                    assert!(queried.iter().all(|e| e.action().author() == &alice));
+                    assert_eq!(
+                        num_expected, actual,
+                        "Expected {} items but got {} with filter {:?}",
+                        num_expected, actual, query
+                    );
+                }
+            }
+        }
+    }
 }

crates/holochain_state/src/source_chain/error.rs

@@ -110,6 +110,11 @@ pub enum SourceChainError {
     #[error("The source chain was missing for a host call that requires it.")]
     SourceChainMissing,
 
+    #[error("The supplied query parameters contains filters that are mutually incompatible.
+             In particular, `sequence_range` cannot currently be used with any other filter.
+             In the future, all filters will be compatible with each other and this will not be an error.")]
+    UnsupportedQuery(ChainQueryFilter),
+
     /// Other
     #[error("Other: {0}")]
     Other(Box<dyn std::error::Error + Send + Sync>),

crates/holochain_zome_types/src/query.rs

@@ -61,7 +61,8 @@ impl Default for ChainQueryFilterRange {
 #[derive(
     serde::Serialize, serde::Deserialize, SerializedBytes, Default, PartialEq, Clone, Debug,
 )]
-#[non_exhaustive]
+// TODO: get feedback on whether it's OK to remove non_exhaustive
+// #[non_exhaustive]
 pub struct ChainQueryFilter {
     /// Limit the results to a range of records according to their actions.
     pub sequence_range: ChainQueryFilterRange,