Bug 19196 - Add pagination helpers
Summary: Add pagination helpers
Status: Pushed to Master
Alias: None
Product: Koha
Classification: Unclassified
Component: REST api (show other bugs)
Version: master
Hardware: All All
: P5 - low enhancement (vote)
Assignee: Tomás Cohen Arazi
QA Contact:
URL:
Whiteboard:
Keywords:
Depends on: 18137
Blocks: 18731 19234 19250 19410
  Show dependency treegraph
 
Reported: 2017-08-29 13:31 UTC by Tomás Cohen Arazi
Modified: 2017-10-19 12:05 UTC (History)
7 users (show)

See Also:
Change sponsored?: Sponsored
Patch complexity: ---
Bot Control: ---
When did the bot last check this:
Who signed the patch off:
Text to go in the release notes:


Attachments
Bug 19196: Unit tests (4.17 KB, patch)
2017-08-29 20:05 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 19196: Add Koha::REST::Plugin::Pagination (4.12 KB, patch)
2017-08-29 20:05 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 19196: Make plugin available to endpoints (926 bytes, patch)
2017-08-29 20:05 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 19196: Unit tests (4.16 KB, patch)
2017-08-30 12:24 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 19196: Add Koha::REST::Plugin::Pagination (5.06 KB, patch)
2017-08-30 12:24 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 19196: Make plugin available to endpoints (926 bytes, patch)
2017-08-30 12:24 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 19196: Unit tests (4.02 KB, patch)
2017-08-30 23:04 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 19196: Add Koha::REST::Plugin::Pagination (4.67 KB, patch)
2017-08-30 23:04 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 19196: Make plugin available to endpoints (926 bytes, patch)
2017-08-30 23:04 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 19196: Unit tests (5.72 KB, patch)
2017-09-04 15:04 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 19196: Add Koha::REST::Plugin::Pagination (5.30 KB, patch)
2017-09-04 15:04 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 19196: Make plugin available to endpoints (926 bytes, patch)
2017-09-04 15:04 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 19196: Unit tests (5.77 KB, patch)
2017-09-08 14:00 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 19196: Add Koha::REST::Plugin::Pagination (5.35 KB, patch)
2017-09-08 14:00 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 19196: Make plugin available to endpoints (976 bytes, patch)
2017-09-08 14:00 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 19196: Unit tests (5.78 KB, patch)
2017-09-22 22:01 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 19196: Add Koha::REST::Plugin::Pagination (5.32 KB, patch)
2017-09-22 22:01 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 19196: Make plugin available to endpoints (982 bytes, patch)
2017-09-22 22:01 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 19196: Rename page and per_page (7.73 KB, patch)
2017-09-25 18:03 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 19196: Unit tests (5.82 KB, patch)
2017-09-26 15:15 UTC, Lari Taskula
Details | Diff | Splinter Review
Bug 19196: Add Koha::REST::Plugin::Pagination (5.36 KB, patch)
2017-09-26 15:15 UTC, Lari Taskula
Details | Diff | Splinter Review
Bug 19196: Make plugin available to endpoints (1.00 KB, patch)
2017-09-26 15:15 UTC, Lari Taskula
Details | Diff | Splinter Review
Bug 19196: Rename page and per_page (7.78 KB, patch)
2017-09-26 15:16 UTC, Lari Taskula
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Tomás Cohen Arazi 2017-08-29 13:31:28 UTC
In order to aid UI devs work on top of the REST api, we need to provide pagination information along with the responses.

It was discussed on the mailing list that following RFC5988 [1] proposal for Link headers was desirable.

And also adding a header telling the total objects count would be desirable, in the lines of this link [2].



[1] https://tools.ietf.org/html/rfc5988#section-6
[2] http://www.vinaysahni.com/best-practices-for-a-pragmatic-restful-api#pagination
Comment 1 Tomás Cohen Arazi 2017-08-29 20:05:38 UTC Comment hidden (obsolete)
Comment 2 Tomás Cohen Arazi 2017-08-29 20:05:44 UTC Comment hidden (obsolete)
Comment 3 Tomás Cohen Arazi 2017-08-29 20:05:49 UTC Comment hidden (obsolete)
Comment 4 Tomás Cohen Arazi 2017-08-30 12:24:32 UTC Comment hidden (obsolete)
Comment 5 Tomás Cohen Arazi 2017-08-30 12:24:40 UTC Comment hidden (obsolete)
Comment 6 Tomás Cohen Arazi 2017-08-30 12:24:49 UTC Comment hidden (obsolete)
Comment 7 Tomás Cohen Arazi 2017-08-30 23:04:08 UTC Comment hidden (obsolete)
Comment 8 Tomás Cohen Arazi 2017-08-30 23:04:14 UTC Comment hidden (obsolete)
Comment 9 Tomás Cohen Arazi 2017-08-30 23:04:20 UTC Comment hidden (obsolete)
Comment 10 Tomás Cohen Arazi 2017-09-04 15:04:02 UTC Comment hidden (obsolete)
Comment 11 Tomás Cohen Arazi 2017-09-04 15:04:09 UTC Comment hidden (obsolete)
Comment 12 Tomás Cohen Arazi 2017-09-04 15:04:15 UTC Comment hidden (obsolete)
Comment 13 Tomás Cohen Arazi 2017-09-04 15:06:01 UTC
Patches reworked so the resulting pagination links preserve the query params :-D
Comment 14 Benjamin Rokseth 2017-09-05 09:25:56 UTC
Great work Tomas.
please fix typo in your example ;)

>     $c->add_pagination_headers({
>            total  => $count,
>            params => $params )};

and add some sensible defaults (10/10?) to page and per_page so Pagination.pm doesnt die if none is given as params.
Comment 15 Kyle M Hall 2017-09-08 14:00:01 UTC Comment hidden (obsolete)
Comment 16 Kyle M Hall 2017-09-08 14:00:09 UTC Comment hidden (obsolete)
Comment 17 Kyle M Hall 2017-09-08 14:00:13 UTC Comment hidden (obsolete)
Comment 18 Martin Renvoize 2017-09-21 14:29:22 UTC
I really like this.. only minor query would be can we document the decision as to why we've chosen to do page based instead of cursor based result 'paging'. I'm personally happy to go with either.. but to prevent FUD in future developers I think it's worth documenting the decision process so we can point to that when someone asks down the line ;)
Comment 19 Tomás Cohen Arazi 2017-09-22 22:01:32 UTC
Created attachment 67338 [details] [review]
Bug 19196: Unit tests

This patch adds unit tests for the new pagination Mojo plugin.

Sponsored-by: ByWater Solutions
Sponsored-by: Camden County

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 20 Tomás Cohen Arazi 2017-09-22 22:01:42 UTC
Created attachment 67339 [details] [review]
Bug 19196: Add Koha::REST::Plugin::Pagination

This patch introduces a Mojolicious plugin to be used on the REST api.
It adds a helper method:

add_pagination_headers
======================

When used, it adds a _Link_ header to the reponse with the calculated
values for pagination, and X-Total-Count containing the total results
like this:

    my $params  = $c->validation->output;
    my $patrons = Koha::Patrons->search;
    my $count   = $patrons->count;

    $c->add_pagination_headers({
            total  => $count,
            params => $params )};

To test:
- Run:
  $ sudo koha-shell kohadev
 k$ cd kohaclone
 k$ prove t/Koha/REST/Plugin/Pagination.t
=> SUCCESS: Tests pass!
- Sign off :-D

Sponsored-by: ByWater solutions
Sponsored-by: Camden County

Edit: I fixed a mistake on the POD (tcohen)

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 21 Tomás Cohen Arazi 2017-09-22 22:01:49 UTC
Created attachment 67340 [details] [review]
Bug 19196: Make plugin available to endpoints

This patch just initializes the plugin on the main controller class so
it is available for all endpoints to use.

As it is not used, in order to test just restart plack and make sure the
endpoints work (run the t/db_dependent/api/v1 tests).

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 22 Tomás Cohen Arazi 2017-09-22 22:10:39 UTC
(In reply to Martin Renvoize from comment #18)
> I really like this.. only minor query would be can we document the decision
> as to why we've chosen to do page based instead of cursor based result
> 'paging'. I'm personally happy to go with either.. but to prevent FUD in
> future developers I think it's worth documenting the decision process so we
> can point to that when someone asks down the line ;)

I think we could implement both pagination methods on a as-needed basis and there should be no conflict. A new set of helper can be added based on this work, with proper tests c&p-ed from the ones on this bug.

Regarding the specifics of why I chose to start with page-based results paging, I'd say:
- It looked pretty *natural* when you look at SQL::Abstract syntax for it.
- It is what Github uses (https://developer.github.com/v3/#pagination) and I really like its simplicity.
- Cursor based pagination is really harder to implement, and the use cases I'm tackling wouldn't benefit much from it (i.e. it'ss just vendor orders, not stuffs that change very often and pagination would become really difficult, in which case other artifacts would get involved like websockets/server side events to notify changes).
Comment 23 Tomás Cohen Arazi 2017-09-25 18:03:48 UTC
Created attachment 67346 [details] [review]
Bug 19196: Rename page and per_page

This patch just renames 'page' and 'per_page' params, for
'_page' and '_per_page' respectively.

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 24 Lari Taskula 2017-09-26 15:15:23 UTC
Created attachment 67366 [details] [review]
Bug 19196: Unit tests

This patch adds unit tests for the new pagination Mojo plugin.

Sponsored-by: ByWater Solutions
Sponsored-by: Camden County

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>

Signed-off-by: Lari Taskula <lari.taskula@jns.fi>
Comment 25 Lari Taskula 2017-09-26 15:15:39 UTC
Created attachment 67367 [details] [review]
Bug 19196: Add Koha::REST::Plugin::Pagination

This patch introduces a Mojolicious plugin to be used on the REST api.
It adds a helper method:

add_pagination_headers
======================

When used, it adds a _Link_ header to the reponse with the calculated
values for pagination, and X-Total-Count containing the total results
like this:

    my $params  = $c->validation->output;
    my $patrons = Koha::Patrons->search;
    my $count   = $patrons->count;

    $c->add_pagination_headers({
            total  => $count,
            params => $params )};

To test:
- Run:
  $ sudo koha-shell kohadev
 k$ cd kohaclone
 k$ prove t/Koha/REST/Plugin/Pagination.t
=> SUCCESS: Tests pass!
- Sign off :-D

Sponsored-by: ByWater solutions
Sponsored-by: Camden County

Edit: I fixed a mistake on the POD (tcohen)

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>

Signed-off-by: Lari Taskula <lari.taskula@jns.fi>
Comment 26 Lari Taskula 2017-09-26 15:15:59 UTC
Created attachment 67368 [details] [review]
Bug 19196: Make plugin available to endpoints

This patch just initializes the plugin on the main controller class so
it is available for all endpoints to use.

As it is not used, in order to test just restart plack and make sure the
endpoints work (run the t/db_dependent/api/v1 tests).

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>

Signed-off-by: Lari Taskula <lari.taskula@jns.fi>
Comment 27 Lari Taskula 2017-09-26 15:16:11 UTC
Created attachment 67369 [details] [review]
Bug 19196: Rename page and per_page

This patch just renames 'page' and 'per_page' params, for
'_page' and '_per_page' respectively.

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>

Signed-off-by: Lari Taskula <lari.taskula@jns.fi>
Comment 28 Marcel de Rooy 2017-10-06 10:14:11 UTC
=head3 register

=cut

sub register {
    my ( $self, $app ) = @_;

=head2 Helper methods

This sub contains POD. Please adjust.
Comment 29 Tomás Cohen Arazi 2017-10-06 12:08:41 UTC
(In reply to Marcel de Rooy from comment #28)
> =head3 register
> 
> =cut
> 
> sub register {
>     my ( $self, $app ) = @_;
> 
> =head2 Helper methods
> 
> This sub contains POD. Please adjust.

What do you suggest? I thought doing it 'inline' was better. Keep in mind that the 'register' sub is the only sub, and it contains the 'helpers' definitiions. The 'helpers' are the only important thing here.
If you prefer, I can move them POD into a separate section like here:

http://cpansearch.perl.org/src/JBERGER/Mojo-IOLoop-ForkCall-0.16/lib/Mojolicious/Plugin/ForkCall.pm

but I still prefer this way.
Comment 30 Marcel de Rooy 2017-10-09 07:26:24 UTC
(In reply to Tomás Cohen Arazi from comment #29)
> What do you suggest? I thought doing it 'inline' was better. Keep in mind
> that the 'register' sub is the only sub, and it contains the 'helpers'
> definitiions. The 'helpers' are the only important thing here.
> If you prefer, I can move them POD into a separate section like here:
> 
> http://cpansearch.perl.org/src/JBERGER/Mojo-IOLoop-ForkCall-0.16/lib/
> Mojolicious/Plugin/ForkCall.pm
> 
> but I still prefer this way.

OK. You have a reason. Personally I would not prefer it, but it should not be a blocker. Passing it back to RM for final decision.
Comment 31 Jonathan Druart 2017-10-09 19:18:50 UTC
Pushed to master for 17.11, thanks to everybody involved!
Comment 32 Marcel de Rooy 2017-10-10 14:22:38 UTC
Error while loading /etc/koha/plack.psgi: Can't load application from file "api/v1/app.pl": Can't locate Mojolicious/Plugin/Koha/REST/Plugin/Pagination.pm:   ./Mojolicious/Plugin/Koha/REST/Plugin/Pagination.pm: Permission denied at (eval 1295) line 1.
Comment 33 Tomás Cohen Arazi 2017-10-10 15:35:58 UTC
(In reply to Marcel de Rooy from comment #32)
> Error while loading /etc/koha/plack.psgi: Can't load application from file
> "api/v1/app.pl": Can't locate
> Mojolicious/Plugin/Koha/REST/Plugin/Pagination.pm:  
> ./Mojolicious/Plugin/Koha/REST/Plugin/Pagination.pm: Permission denied at
> (eval 1295) line 1.

Mojolicious version?
Comment 34 Marcel de Rooy 2017-10-10 15:54:43 UTC
(In reply to Tomás Cohen Arazi from comment #33)
> (In reply to Marcel de Rooy from comment #32)
> > Error while loading /etc/koha/plack.psgi: Can't load application from file
> > "api/v1/app.pl": Can't locate
> > Mojolicious/Plugin/Koha/REST/Plugin/Pagination.pm:  
> > ./Mojolicious/Plugin/Koha/REST/Plugin/Pagination.pm: Permission denied at
> > (eval 1295) line 1.
> 
> Mojolicious version?

7.21
Comment 35 Marcel de Rooy 2017-10-11 11:48:16 UTC
Since commenting this line solves the problem:
# $self->plugin( 'Koha::REST::Plugin::Pagination' );
I still hope to see something moving here ;)
Anything with setting path for Mojo plugins or so? Mojo experts around?
Comment 36 Tomás Cohen Arazi 2017-10-11 12:14:33 UTC
(In reply to Marcel de Rooy from comment #35)
> Since commenting this line solves the problem:
> # $self->plugin( 'Koha::REST::Plugin::Pagination' );
> I still hope to see something moving here ;)
> Anything with setting path for Mojo plugins or so? Mojo experts around?

For the record, I cannot reproduce.
Comment 37 Stefan Berndtsson 2017-10-19 11:43:04 UTC
(In reply to Marcel de Rooy from comment #32)
> Error while loading /etc/koha/plack.psgi: Can't load application from file
> "api/v1/app.pl": Can't locate
> Mojolicious/Plugin/Koha/REST/Plugin/Pagination.pm:  
> ./Mojolicious/Plugin/Koha/REST/Plugin/Pagination.pm: Permission denied at
> (eval 1295) line 1.

Same issue here. Also 7.21 of Mojolicious. Commenting the line in Koha/REST/V1.pm works as a temporary fix here as well.
Comment 38 Marcel de Rooy 2017-10-19 11:45:40 UTC
(In reply to Stefan Berndtsson from comment #37)
> (In reply to Marcel de Rooy from comment #32)
> > Error while loading /etc/koha/plack.psgi: Can't load application from file
> > "api/v1/app.pl": Can't locate
> > Mojolicious/Plugin/Koha/REST/Plugin/Pagination.pm:  
> > ./Mojolicious/Plugin/Koha/REST/Plugin/Pagination.pm: Permission denied at
> > (eval 1295) line 1.
> 
> Same issue here. Also 7.21 of Mojolicious. Commenting the line in
> Koha/REST/V1.pm works as a temporary fix here as well.

Thanks for your confirmation of this issue. We might convince Tomas ;)
Comment 39 Tomás Cohen Arazi 2017-10-19 12:05:55 UTC
(In reply to Marcel de Rooy from comment #38)
> (In reply to Stefan Berndtsson from comment #37)
> > (In reply to Marcel de Rooy from comment #32)
> > > Error while loading /etc/koha/plack.psgi: Can't load application from file
> > > "api/v1/app.pl": Can't locate
> > > Mojolicious/Plugin/Koha/REST/Plugin/Pagination.pm:  
> > > ./Mojolicious/Plugin/Koha/REST/Plugin/Pagination.pm: Permission denied at
> > > (eval 1295) line 1.
> > 
> > Same issue here. Also 7.21 of Mojolicious. Commenting the line in
> > Koha/REST/V1.pm works as a temporary fix here as well.
> 
> Thanks for your confirmation of this issue. We might convince Tomas ;)

I belive you! I just can't reproduce it