Skip to content
This repository has been archived by the owner on Feb 3, 2023. It is now read-only.

Fix pagination by time to filter entries from_time into the past (desc) #2144

Open
wants to merge 6 commits into
base: develop
Choose a base branch
from
Open

Fix pagination by time to filter entries from_time into the past (desc) #2144

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
b9a7d03
Fix pagination by time to filter entries from_time into the past (desc)
Mar 6, 2020
b5abaa4
Updated changelog and docs for get_links time pagination fix
Mar 6, 2020
ec852e8
Merge branch 'develop' into fix-time-pagination
StaticallyTypedAnxiety Mar 8, 2020
afc5b7e
Merge branch 'develop' into fix-time-pagination
Mar 13, 2020
d284fec
Merge branch 'develop' into fix-time-pagination
zippy Mar 14, 2020
98f1885
Merge branch 'develop' into fix-time-pagination
zippy Mar 18, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions CHANGELOG-UNRELEASED.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,5 +16,6 @@ This project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.htm
- Older rust-tracing traces.

### Fixed
- Pagination option for get_links now retrieves entries before `from_time`, not after [PR#2144](https://github.com/holochain/holochain-rust/pull/2144)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well I certainly agree with the intention of the PR and I though we were supposed to be defaulting to descending dates in the sort order which should get you before... but I guess if that was happening, you wouldn't be trying to fix it. :)


### Security
8 changes: 4 additions & 4 deletions app_spec/test/files/links.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -148,8 +148,8 @@ module.exports = scenario => {
limit:3
})

t.equal(0, bob_posts_live_time.Ok.links.length)
t.equal(0, alice_posts_live_time.Ok.links.length)
t.equal(3, bob_posts_live_time.Ok.links.length)
t.equal(3, alice_posts_live_time.Ok.links.length)

const bob_posts_time_2 = await bob.call('app', 'simple', 'get_my_links_with_time_pagination',
{
Expand All @@ -165,8 +165,8 @@ module.exports = scenario => {
limit:3
})

t.equal(3, bob_posts_time_2.Ok.links.length)
t.equal(3, alice_posts_time_2.Ok.links.length)
t.equal(0, bob_posts_time_2.Ok.links.length)
t.equal(0, alice_posts_time_2.Ok.links.length)
})

scenario('get_links_crud', async (s, t) => {
Expand Down
2 changes: 1 addition & 1 deletion crates/core/src/dht/dht_store.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -188,7 +188,7 @@ impl DhtStore {
.skip_while(move |eavi| {
let from_time: DateTime<FixedOffset> =
paginated_time.from_time.into();
from_time.timestamp_nanos() >= eavi.index()
from_time.timestamp_nanos() <= eavi.index()
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is the line that I see a couple problems with:

  1. If the specified timestamp is LESS than the target times we're trying to retrieve, then we're going to get entries NEWER than specified, and I thought you were trying to make this go backwards to get OLDER ones? Am I misunderstanding your intent?
  2. But the bigger issue I see is that it looks like we're comparing the specified date to the eavi.index instead of the timestamp in the header of the LinkAdd entry. Normally the eavi.index is an indicator of when this node received it via gossip which may have no correlation to the order that links were published, and I'm assuming you want them in time order by when they were created, not by when a random node happened to receive the gossip about them.

Copy link
Author

@lorenjohnson lorenjohnson Mar 15, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  • If the specified timestamp is LESS than the target times we're trying to retrieve, then we're going to get entries NEWER than specified, and I thought you were trying to make this go backwards to get OLDER ones? Am I misunderstanding your intent?
  1. Reversing this comparison did get me the results I was needing. I've been locally testing against a core build off this branch which did make everything work nicely all the way through to my front-end code. I believe the thing to pay attention to is that this is a skip_while loop so the expected logic can seem a bit inverted on first blush. I would still be second guessing myself if it weren't for the fact that it's simply working with this change :) I think this explanation will help, let me know if we're on the same page here yet:
Given these Entries
    8
    7
    6
    5
    4
    3

Entries from: 10, limit 3
    8 Include: 10 <= 8 is FALSE
    7 Include: 10 <= 7 is FALSE
    6 Include: 10 <= 6 is FALSE, LIMIT 3 <
    5
    4
    3

Entries from: 6, limit 3
    8 Skip: 6 <= 8 is TRUE
    7 Skip: 6 <= 7 is TRUE
    6 Skip: 6 <= 6 IS TRUE
    5 Include: 6 <= 5 IS FALSE
    4 Include: 6 <= 4 IS FALSE
    3 Include: 6 <= 3 IS FALSE, LIMIT 3 <
  1. @AshantiMutinta will know more about why she chose to implement against eavi.index, it sure does sound like what you're pointing to is an important correction though.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is the line that I see a couple problems with:

1. If the specified timestamp is LESS than the target times we're trying to retrieve, then we're going to get entries NEWER than specified, and I thought you were trying to make this go backwards to get OLDER ones? Am I misunderstanding your intent?

2. But the bigger issue I see is that it looks like we're comparing the specified date to the eavi.index instead of the timestamp in the header of the LinkAdd entry. Normally the eavi.index is an indicator of when this node received it via gossip which may have no correlation to the order that links were published, and I'm assuming you want them in time order by when they were created, not by when a random node happened to receive the gossip about them.

I can make the change, eavi has a constructor that allows you to create it with the index so any node that recieves a link and stores it on their dht will do that according to the timestamp in the link topchainheader. Which means from the link perspective, there is no explicit timestamp generation on the eavi side. The reasoning was because if a node has the base it could use the eavi system to query based on timestamps.

Pro of this method is that you wouldn't have to do an extra get entry for the lazy pagination.

E.G. eavi.index is essentially link timestamp but if a distinction has to be made that I could change that around no problem

})
.take(time_pagination.limit),
)
Expand Down
2 changes: 1 addition & 1 deletion doc/holochain_101/src/links/get_links.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@ Get Links allows the zome developer to query links from the DHT. The call accept
`Timeout` : The timeout variable on the options specifies how long the query process should wait befor a response before it timesout
`LinksStatusRequest` : This is a variable in which you can specify 3 modes, `All`,`Live`,`Delete`. This allows you to query the links based on crud_status in which `All` will return everything will `Live` will only return live links and `Delete` as such.
`Headers`: boolean value which if set to true indicates that the link headers should also be returned.```
`Pagination`: The pagination type has two variants which are `Size` and `Time` These variants allow the user to paginate based on choosing pages and specifying page size, or by specifying a point time and specifying a limit of the number of entries (based on the timestamp in the entry header) to return from that point.
`Pagination`: The pagination type has two variants which are `Size` and `Time` These variants allow the user to paginate based on choosing pages and specifying page size, or by specifying a point in time and specifying a limit of the number of entries (based on the timestamp in the entry header) to return before that point.
`Sort Order` : Allows get_links to define which order `Ascending` or `Descending` the links should be returned in

# Link Results
Expand Down