Skip to content
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

add pagination to GET /api/v1/find endpoint #1699

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

paskal
Copy link
Collaborator

@paskal paskal commented Nov 18, 2023

format=tree pagination provides top-level comments with all replies and returns the last top-level comment as last_comment to be used as offset for the next page. If comments and replies overflow the limit, the one stepping out of the limit will not be returned. If the first comment and its replies after the given offset overflow the limit, it will be returned with all the replies.

format=plain pagination works by providing all comments and returning the last comment as last_comment to be used as offset for the next page.

Requires #1685 to be merged first. Backend part for #782.

Copy link

github-actions bot commented Nov 18, 2023

Pull Request Test Coverage Report for Build 7512258238

  • 0 of 0 changed or added relevant lines in 0 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage decreased (-0.5%) to 84.281%

Totals Coverage Status
Change from base Build 7495000583: -0.5%
Covered Lines: 5946
Relevant Lines: 7055

💛 - Coveralls

@paskal paskal force-pushed the paskal/comments_pagination branch 3 times, most recently from f7f18c8 to d099924 Compare February 22, 2024 17:13
@paskal paskal force-pushed the paskal/comments_pagination branch from d099924 to 4bbb980 Compare March 4, 2024 22:12
`format=tree` pagination provides top-level comments with all replies
and returns the last top-level comment as `last_comment` to be used
as `offset` for the next page. If comments and replies overflow
the limit, the one stepping out of the limit will not be returned.
 If the first comment and its replies after the given offset overflow
 the limit, it will be returned with all the replies.

`format=plain` pagination works by providing all comments and returning
the last comment as `last_comment` to be used as `offset`
for the next page.
Copy link
Owner

@umputun umputun left a comment

Choose a reason for hiding this comment

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

few minor things. I have not checked the tree part of this, and asked for some tests to make sure everyting is working as designed.

}
}

log.Printf("[DEBUG] get comments for %+v, sort %s, format %s, since %v, limit %d, offset %s", locator, sort, format, since, limit, offsetID)
Copy link
Owner

Choose a reason for hiding this comment

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

I think %q for the offset will be less confusing in the log if it was input.

@@ -70,7 +79,24 @@ func (s *public) findCommentsCtrl(w http.ResponseWriter, r *http.Request) {
since = time.Time{} // since doesn't make sense for tree
}

log.Printf("[DEBUG] get comments for %+v, sort %s, format %s, since %v", locator, sort, format, since)
limitParam := r.URL.Query().Get("limit")
var limit int
Copy link
Owner

Choose a reason for hiding this comment

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

nit: personally, I'd prefer limit:=0 instead, as it communicates the default value more clearly.

if offsetID != "" {
for i, comment := range c {
if comment.ID == offsetID {
c = c[i+1:]
Copy link
Owner

Choose a reason for hiding this comment

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

This is confusing and even scary a bit. Generally, the idea of changing the slice we are ranging over inside the loop feels wrong to me. Probably it is safe to do now, as we are breaking right away, but someday, as we add code to this function, it may not be the case. I'd rather add a new lcs variable for the result slice.

//
// In case limit is less than the number of replies to first comment after given offset, that first comment is
// returned completely with all replies.
func (t *Tree) limit(limit int, offsetID string) {
Copy link
Owner

Choose a reason for hiding this comment

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

I would suggest calling it setLimit or updateWithLimit, as this function modifies nodes and does not return anything.

@@ -145,3 +159,81 @@ func (t *Tree) sortNodes(sortType string) {
}
})
}

// limit limits number of comments in tree and sets countLeft and lastLimitedComment,
Copy link
Owner

Choose a reason for hiding this comment

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

I think there is no direct test for this non-trivial function, but rather only via MakeTree? It's up to you, but I will feel better if we have some.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants