Fix `query` #1402

maackle · 2022-06-01T22:42:49Z

Summary

Fixed hdk::query, which was showing some incorrect behavior:
- When using ChainQueryFilterRange::HeaderHashRange, 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.

TODO:

CHANGELOG(s) updated with appropriate info
Just before pressing the merge button, ensure new entries to CHANGELOG(s) are still under the UNRELEASED heading

maackle · 2022-06-02T16:44:58Z

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.


Suggested change

#[error("The supplied query parameters contains filters that are mutually incompatible.

#[error("The query contains filters that are mutually incompatible.

maackle · 2022-06-02T16:47:11Z

crates/holochain_zome_types/src/query.rs

+// TODO: get feedback on whether it's OK to remove non_exhaustive
+// #[non_exhaustive]


I removed non_exhaustive just because I was lazy and wanted it to be easier to write my test. Also I don't think our API is stable enough to warrant niceties like this yet. I will put it back in if anyone wants it.

I don't think our API is stable enough to warrant niceties like this yet

is this statement specific to the query API?

If we ever want this, it'll be easier to include it sooner, specifically because of things like tests getting written without it. But I don't have an opinion otherwise.

maackle · 2022-06-02T18:33:39Z

The current CI failure has nothing to do with this PR. I think it may have to do with #1389 which was recently merged.

steveej · 2022-06-02T22:00:28Z

The current CI failure has nothing to do with this PR. I think it may have to do with #1389 which was recently merged.

test failures for "ci/circleci: slow-test" can be ignored as it's being replaced by "ci/circleci: slow-test-nextest". failures of the former won't block the PR, it's merely a cosmetic issue.

freesig

I think this might be the wrong approach. I think we want to only query the database and scratch for full ranges then filter the types in memory. So the combinations of queries should still be valid.

freesig · 2022-06-27T02:11:35Z

crates/holochain_state/src/source_chain.rs

@@ -662,6 +662,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<Element>> {
+        if query.sequence_range != ChainQueryFilterRange::Unbounded
+            && (query.header_type.is_some()


Why can these filters only be combined with unbounded?

maackle · 2022-06-28T18:09:22Z

@freesig precisely, query is implemented with the wrong approach, but that wasn't introduced in this PR. This PR is disabling the combinations of features that are unable to work properly due to the current approach, until we fix the approach. Specifically, the fork disambiguation part that happens in memory can't work with discontinuous items, so that's why range queries are incompatible with any kind of filter.

I responded to the change request saying that I think it's asking for a change not introduced in this PR

steveej

mostly questions from my side as i don't understand this enough to immediately approve or request changes.

i'm primarily trying to fully comprehend the introduced test, which i appreciate a lot btw.

steveej · 2022-07-06T20:59:21Z

crates/holochain_zome_types/src/query.rs

+// TODO: get feedback on whether it's OK to remove non_exhaustive
+// #[non_exhaustive]


I don't think our API is stable enough to warrant niceties like this yet

is this statement specific to the query API?

crates/holochain_state/src/source_chain.rs

crates/hdk/CHANGELOG.md

neonphog · 2022-07-13T22:37:11Z

crates/holo_hash/src/hash.rs

+
+    /// 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
+    }


subtle-encoding is already in our dependency tree, should we make use of that rather than re-inventing the wheel again, or do we want to keep that out of holo_hash specifically?

I'll do that as a quick follow-up, since we already had it reinvented twice before I added a third

Actually, the instance of this in holo_hash should not be replaced with subtle-encoding, since that would require pulling in that dependency, and to_hex() could be something useful in wasm which shouldn't require a whole extra dependency of bloat. So the case for DRYing up the remaining two instances is less compelling.

neonphog

👍

freesig

I'm ok with merging this for now as long as we follow it up soon.

Co-authored-by: Stefan Junker <1181362+steveeJ@users.noreply.github.com>

made the change requested

maackle added 4 commits June 1, 2022 13:25

Add failing test for SourceChain::query

8c72024

Fix query for simple seq range. Still broken for other ranges.

9a08d8f

Make test pass by emitting error for unsupported filter combinations

d227463

Changelog

fed71d1

maackle requested review from steveej, thedavidmeister and freesig June 1, 2022 22:42

maackle commented Jun 2, 2022

View reviewed changes

Merge branch 'develop' into fix-query

7d5a80d

maackle requested a review from zo-el June 7, 2022 19:01

freesig previously requested changes Jun 27, 2022

View reviewed changes

maackle requested a review from freesig June 28, 2022 18:09

Merge remote-tracking branch 'origin/develop' into fix-query

3db6a4d

maackle force-pushed the fix-query branch from de57c1a to 3db6a4d Compare June 28, 2022 19:58

steveej reviewed Jul 7, 2022

View reviewed changes

steveej previously requested changes Jul 12, 2022

View reviewed changes

crates/hdk/CHANGELOG.md Outdated Show resolved Hide resolved

maackle requested a review from neonphog July 13, 2022 22:30

neonphog reviewed Jul 13, 2022

View reviewed changes

neonphog approved these changes Jul 13, 2022

View reviewed changes

freesig approved these changes Jul 13, 2022

View reviewed changes

maackle and others added 2 commits July 13, 2022 20:37

Merge remote-tracking branch 'origin/develop' into fix-query

8f15460

Update crates/hdk/CHANGELOG.md

532e4da

Co-authored-by: Stefan Junker <1181362+steveeJ@users.noreply.github.com>

maackle enabled auto-merge July 14, 2022 03:47

maackle merged commit b60132c into develop Jul 14, 2022

maackle deleted the fix-query branch July 14, 2022 03:56

abe-njama mentioned this pull request Aug 10, 2022

query!() may be nicer to accept multiple variants of HeaderType #387

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix `query` #1402

Fix `query` #1402

maackle commented Jun 1, 2022

maackle Jun 2, 2022

maackle Jun 2, 2022

steveej Jul 6, 2022

neonphog Jul 13, 2022

maackle commented Jun 2, 2022

steveej commented Jun 2, 2022

freesig left a comment

freesig Jun 27, 2022

maackle commented Jun 28, 2022

steveej left a comment

steveej Jul 6, 2022

neonphog Jul 13, 2022

maackle Jul 14, 2022

maackle Jul 14, 2022

neonphog left a comment

freesig left a comment

	#[error("The supplied query parameters contains filters that are mutually incompatible.
	#[error("The query contains filters that are mutually incompatible.

		// TODO: get feedback on whether it's OK to remove non_exhaustive
		// #[non_exhaustive]

Fix query #1402

Fix query #1402

Conversation

maackle commented Jun 1, 2022

Summary

TODO:

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

maackle commented Jun 2, 2022

steveej commented Jun 2, 2022

freesig left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

maackle commented Jun 28, 2022

steveej left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

neonphog left a comment

Choose a reason for hiding this comment

freesig left a comment

Choose a reason for hiding this comment

Fix `query` #1402

Fix `query` #1402