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 system to http endpoint executions/list #1689

Merged
merged 3 commits into from
Feb 28, 2020

Conversation

NicolasMahe
Copy link
Member

@NicolasMahe NicolasMahe commented Feb 27, 2020

Added a simple and not optimized pagination system on executions/list HTTP endpoint so the explorer can fetch executions without breaking.

The parameters page, limit and sort=reverse can be passed to the endpoint to return different executions:

curl 'localhost:1317/execution/list?page=12&limit=1&sort=reverse'

The sort is based on the execution's blockHeight

Related to #1679


Improvements that can be done now or later:

  1. Move the pagination and sort system in the module's querier but need to pass as parameters page, limit and sort.
  2. Move the pagination and sort system in the module's keeper and use KVStorePrefixIteratorPaginated but need to pass as parameters page, limit and find a way to sort the data without iterate on all of them (index?)

@NicolasMahe NicolasMahe marked this pull request as ready for review February 27, 2020 11:55
@NicolasMahe NicolasMahe added this to the next milestone Feb 27, 2020
@NicolasMahe NicolasMahe self-assigned this Feb 27, 2020
@NicolasMahe NicolasMahe added the release:add Pull requests that add something label Feb 27, 2020
Copy link
Contributor

@krhubert krhubert left a comment

Choose a reason for hiding this comment

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

I hope this is only solution until next sprint.

Comment on lines 85 to 86
if start < 0 || end < 0 {
execs = make([]*execution.Execution, 0)
Copy link
Contributor

Choose a reason for hiding this comment

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

We should return error if the pagination boundaries are invalid (so in that case check if page/limit <= 0), otherwise returning empty list.

s/execs = make([]*execution.Execution, 0)/execs = nil/

Copy link
Member Author

Choose a reason for hiding this comment

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

the page and limit <= 0 error is managed by rest.ParseHTTPArgs(r):

} else if page <= 0 {
	return tags, page, limit, errors.New("page must greater than 0")
}
} else if limit <= 0 {
	return tags, page, limit, errors.New("limit must greater than 0")
}

the problem with returning execs = nil is it's not a list anymore in the json response:

{"height":"757","result":null}

so we will have issue with clients.
i would keep it as it is:

{"height":"773","result":[]}

i replaced the make([]*execution.Execution, 0) by simply []*execution.Execution{}

Copy link
Member

@antho1404 antho1404 left a comment

Choose a reason for hiding this comment

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

Except for one suggestion, lgtm

x/execution/client/rest/query.go Outdated Show resolved Hide resolved
@NicolasMahe
Copy link
Member Author

@krhubert @antho1404 modif done. please check again

@antho1404 antho1404 merged commit 6f1ca86 into dev Feb 28, 2020
@antho1404 antho1404 deleted the feature/execution-pagination branch February 28, 2020 08:47
@NicolasMahe NicolasMahe changed the title Add pagination system on http handler of executions/list Add pagination system to http endpoint executions/list Mar 2, 2020
@NicolasMahe NicolasMahe mentioned this pull request Mar 2, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
release:add Pull requests that add something
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants