Provide hdk::query_result API to allow return of Headers if desired #868
Conversation
…ature-query-headers
o Package up individual options into ...Options packages o Change u32 options to usize o Retain simple Vec<Address> for original hdk::query o chain_store.query now returns an enum, so handle appropriately
|
@pjkundert does this mean you can only query for headers or the entries, but not both? I'm wondering how this fits into the work I am about to start, which is to flesh out |
|
We need both to be improved; I couldn't quite figure out how to get the headers in the get_entry flow, but I noticed we were just throwing them away in the query, so I took a crack at obtaining them there. The APIs should be improved in both cases to make them symmetrical; the get_entry's GetEntryOptions had an extra unnecessary option for query (the StatusRequestKind), so I made a QueryArgsOptions, but perhaps we could just re-use the same struct and document that the ...Kind is ignored? Also, the return types differ, and query_result would probably need to be made consistent with get_entry_result; I tried to retain the basic hdk::query API (returns a simple |
o Simplify by removing some Option-ality o Add some unit tests
o Without tagging, an empty [] result is ambiguous, so can be assigned to the wrong enum when deserialized. o Add the install: target to Makefile, to rebuild nodejs_container.
…ature-query-headers
…ature-query-headers
…ature-query-headers
…ature-query-headers
o Since Iso8601 may fail to convert to DateTime, only PartialEq possible
pjkundert
changed the title
|
I think it's fine that Query would return something different then Get so it has it's own options. |
There was a problem hiding this comment.
So, I would approve this for the sake of expedience and getting us what we need for closed alpha, but I do think that the underlying API in core should allow you to get both headers and entries in one query. If that seems like to much to get done for this PR, lets just make sure it gets into the tree as a followup.
| } | ||
|
|
||
| #[derive(Debug)] | ||
| pub enum ChainStoreQueryResult { |
There was a problem hiding this comment.
I think I agree with @maackle that this shouldn't be an enum, but instead a struct with Option<Vec
> and Option<Vec> that have Some() or None() based on the query so that you don't have to do two queries if you want both. Then in the request we would have an enum for "Entries, Headers, Both".It would be fine to have helper functions in the hdk that assume you only want one or the other and do the ChainStoreQueryOptions setting correctly and unrwap to just one or the other.. But the core function should allow both to be returned.
There was a problem hiding this comment.
@maackle and I just paired; We'll be able to Addresses, Headers, and Entries without too much trouble. Should have it implemented for review by end of day, hopefully.
o Work in progress; needs unit testing
|
OK, the hdk::query_results API has been enhanced to optionally return Entry data. I'd like feedback on the API and return types. Not yet ready for merging; hasn't been heavily tested (ie. at all). |
| .collect(); | ||
| match maybe_headers_with_entries { | ||
| Ok(headers_with_entries) => QueryResult::HeadersWithEntries(headers_with_entries), | ||
| Err(_e) => return ribosome_error_code!(UnknownEntryType), // TODO: return actual error? |
There was a problem hiding this comment.
If it works, this is probably fine now, but would rather see the entry-getting stuff DRYed up and refactored into a function
| Err(_code) => return ribosome_error_code!(UnknownEntryType), | ||
| }; | ||
|
|
||
| runtime.store_result(result) | ||
| } | ||
|
|
||
| /// Get an Entry via the provided context, returning Entry or HolochainError on failure | ||
| fn get_entry_from_context( |
There was a problem hiding this comment.
Isn't query supposed to operate on the chain only? Why from_context which actually is do a get from_dht? This only works because we currently reuse the same CAS for both, chain and dht, but we do have to separate CASes in the context to keep them separate. This function should be get_entry_from_chain which only operates on the source chain.
There was a problem hiding this comment.
Done. I've augmented core/src/nucleus/actions/get_entry.rs to provide crate-local APIs to access Entry values from either the either the context's dht or the agent.
o Reduce some unnecessary copying of Addresses o Change hdk::query to get local-chain entries only from agent CAS
…ection' into feature-query-headers
…n-rust into feature-query-headers
…n-rust into feature-query-headers
The existing
hdk::query("post".into(), 0, 0)returningVec<Address>is maintained.A new
hdk::query_result("post.into(), QueryArgsOptions::default())is provided, returning an enum containing Vec or Vec. IfQueryArgsOptions { headers: Some(true),... }is provided it returns aVec<ChainHeader>; otherwise, aVec<Addresses>.I'd like to collect commentary on the API. It's somewhat like
hdk::get_entry_result, but the return type is an enum, instead of a struct w/ aresultmember.I've also cleaned up the .skip and .take a bit to avoid cloning more than I have to, since
ChainHeadersare somewhat bigger than just an Address to copy.Also, instead of
hdk::querytakingu32values for start and limit, I've switched tousizefor consistency with what they're used for internally. Is that a problem for JSON serialization or anything?