-
Notifications
You must be signed in to change notification settings - Fork 3
feat(tx-cache): Pagination types and impl #131
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
695f90e to
38133ea
Compare
38133ea to
9c99eeb
Compare
|
is this a breaking change for existing consumers of the api? i.e. if they are using an older version of the builder will their builder break if I deploy this version of the tx cache? |
|
Not technically a breaking change. The builder will still be able to fetch items from the cache as serde can partially deserialize JSON (it's a self-described format). They will be missing the pagination info, which is fine for now. I've added partial deser tests on the companion pagination PR to confirm this. |
|
these changes are currently breaking, as serde will expect pagination info to be present unless given a specific |
|
I would prefer to see something like: |
|
took a stab at the suggestion @prestwich, wdyt? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
add serde roundtrip tests with what you think these json objects should look like.
If pagination params is a query, I don't think it's appropriate to use serde. Instead, there should be a method to apply it to a url?
| /// The cursor to start from. | ||
| cursor: Option<String>, | ||
| /// The number of items to return. | ||
| limit: Option<u32>, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
missing serde tags for if missing or none
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
missing serde renames
crates/tx-cache/src/types.rs
Outdated
| /// Represents the pagination information from a transaction cache response. | ||
| /// This applies to all GET endpoints that return a list of items. | ||
| #[derive(Debug, Clone, Serialize, Deserialize)] | ||
| pub struct PaginationInfo { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Missing serde for missing/none on next_cursor
crates/tx-cache/src/types.rs
Outdated
| /// Represents the pagination information from a transaction cache response. | ||
| /// This applies to all GET endpoints that return a list of items. | ||
| #[derive(Debug, Clone, Serialize, Deserialize)] | ||
| pub struct PaginationInfo { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
missing serde renames
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
isn't has_next_page redundant? shouldn't we use presence/absence of a next_cursor to indicate this?
crates/tx-cache/src/types.rs
Outdated
|
|
||
| impl From<PaginationInfo> for PaginationParams { | ||
| fn from(info: PaginationInfo) -> Self { | ||
| Self { cursor: info.into_next_cursor(), limit: None } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this feels like an inappropriate from impl. Maybe a next_page(&self, page_size: Option<usize>) function instead?
crates/tx-cache/src/types.rs
Outdated
|
|
||
| /// A response from the transaction cache, containing an item. | ||
| #[derive(Debug, Clone, Serialize, Deserialize)] | ||
| pub enum CacheResponse<T> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
missing serde tags
| @@ -1,18 +1,21 @@ | |||
| //! The endpoints for the transaction cache. | |||
| use std::collections::HashMap; | |||
|
|
|||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit remove newline
| } | ||
|
|
||
| async fn get_inner_with_query<T>( | ||
| async fn get_inner_with_query<C: CursorKey, T>( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if you make a trait for the output you can remove the C here and avoid specifying it in turbofish elsewhere
trait CacheObject {
type Key: CursorKey;
}

Adds cursor-based pagination types to the tx-cache crate. This allows us to implement pagination on the cache, and for the tx-cache client to be able to use pagination queries.
Getting all items in one of the collections in the cache is as simple as:
PaginationInfoPaginationInfointo the next request. This can easily be done by.into()ing aPaginationInfointo aPaginationParams.PaginationInfohas an utility method to detect this.These changes are not breaking for the current tx cache consumers.