Merge pull request #1313 from metacpan/haarg/normalize-limit-pagination

haarg · web-flow · commit 2633bba9964d · 2024-11-05T11:14:21.000+01:00
normalize pagination parameters and add size limits
diff --git a/lib/MetaCPAN/API/Controller/Search.pm b/lib/MetaCPAN/API/Controller/Search.pm
@@ -17,9 +17,13 @@ sub web {
     return unless $c->openapi->valid_input;
     my $args = $c->validation->output;
 
-    my @search = ( @{$args}{qw/q from size/} );
-    push @search, $args->{collapsed} if exists $args->{collapsed};
-    my $results = $c->model->search->search_web(@search);
+    my $query = $args->{q};
+    my $size  = $args->{page_size} // $args->{size} // 20;
+    my $page = $args->{page} // ( 1 + int( ( $args->{from} // 0 ) / $size ) );
+    my $collapsed = $args->{collapsed};
+
+    my $results
+        = $c->model->search->search_web( $query, $page, $size, $collapsed );
 
     return $c->render( json => $results );
 }
diff --git a/lib/MetaCPAN/Query/Favorite.pm b/lib/MetaCPAN/Query/Favorite.pm
@@ -3,7 +3,7 @@ package MetaCPAN::Query::Favorite;
 use MetaCPAN::Moose;
 
 use MetaCPAN::ESConfig qw( es_doc_path );
-use MetaCPAN::Util     qw(hit_total);
+use MetaCPAN::Util     qw( MAX_RESULT_WINDOW hit_total );
 
 with 'MetaCPAN::Query::Role::Common';
 
@@ -148,6 +148,14 @@ sub recent {
     $page //= 1;
     $size //= 100;
 
+    if ( $page * $size >= MAX_RESULT_WINDOW ) {
+        return +{
+            favorites => [],
+            took      => 0,
+            total     => 0,
+        };
+    }
+
     my $favs = $self->es->search(
         es_doc_path('favorite'),
         body => {
diff --git a/lib/MetaCPAN/Query/Release.pm b/lib/MetaCPAN/Query/Release.pm
@@ -4,7 +4,7 @@ use MetaCPAN::Moose;
 
 use MetaCPAN::ESConfig qw( es_doc_path );
 use MetaCPAN::Util
-    qw( hit_total single_valued_arrayref_to_scalar true false );
+    qw( MAX_RESULT_WINDOW hit_total single_valued_arrayref_to_scalar true false );
 
 with 'MetaCPAN::Query::Role::Common';
 
@@ -403,10 +403,18 @@ sub by_author_and_names {
 }
 
 sub by_author {
-    my ( $self, $pauseid, $size, $page ) = @_;
+    my ( $self, $pauseid, $page, $size ) = @_;
     $size //= 1000;
     $page //= 1;
 
+    if ( $page * $size >= MAX_RESULT_WINDOW ) {
+        return {
+            releases => [],
+            took     => 0,
+            total    => 0,
+        };
+    }
+
     my $body = {
         query => {
             bool => {
@@ -495,10 +503,18 @@ sub latest_by_author {
 }
 
 sub all_by_author {
-    my ( $self, $author, $size, $page ) = @_;
+    my ( $self, $author, $page, $size ) = @_;
     $size //= 100;
     $page //= 1;
 
+    if ( $page * $size >= MAX_RESULT_WINDOW ) {
+        return {
+            releases => [],
+            took     => 0,
+            total    => 0,
+        };
+    }
+
     my $body = {
         query   => { term => { author => uc($author) } },
         sort    => [ { date => 'desc' } ],
@@ -639,11 +655,11 @@ sub top_uploaders {
 sub requires {
     my ( $self, $module, $page, $page_size, $sort ) = @_;
     return $self->_get_depended_releases( [$module], $page, $page_size,
-        $page_size, $sort );
+        $sort );
 }
 
 sub reverse_dependencies {
-    my ( $self, $distribution, $page, $page_size, $size, $sort ) = @_;
+    my ( $self, $distribution, $page, $page_size, $sort ) = @_;
 
     # get the latest release of given distribution
     my $release = $self->_get_latest_release($distribution) || return;
@@ -653,7 +669,7 @@ sub reverse_dependencies {
 
     # return releases depended on those modules
     return $self->_get_depended_releases( $modules, $page, $page_size,
-        $size, $sort );
+        $sort );
 }
 
 sub _get_latest_release {
@@ -725,10 +741,18 @@ sub _fix_sort_value {
 }
 
 sub _get_depended_releases {
-    my ( $self, $modules, $page, $page_size, $size, $sort ) = @_;
+    my ( $self, $modules, $page, $page_size, $sort ) = @_;
     $page      //= 1;
     $page_size //= 50;
 
+    if ( $page * $page_size >= MAX_RESULT_WINDOW ) {
+        return +{
+            data  => [],
+            took  => 0,
+            total => 0,
+        };
+    }
+
     $sort = _fix_sort_value($sort);
 
     my $dependency_filter = {
@@ -770,8 +794,8 @@ sub _get_depended_releases {
                     ],
                 },
             },
-            size => $size || $page_size,
-            from => $page * $page_size - $page_size,
+            size => $page_size,
+            from => ( $page - 1 ) * $page_size,
             sort => $sort,
         }
     );
@@ -784,22 +808,20 @@ sub _get_depended_releases {
 }
 
 sub recent {
-    my ( $self, $page, $page_size, $type ) = @_;
+    my ( $self, $type, $page, $page_size ) = @_;
     $page      //= 1;
     $page_size //= 10000;
     $type      //= '';
 
-    my $query;
-    my $from = ( $page - 1 ) * $page_size;
-
-    if ( $from + $page_size > 10000 ) {
-        return {
+    if ( $page * $page_size >= MAX_RESULT_WINDOW ) {
+        return +{
             releases => [],
-            total    => 0,
             took     => 0,
+            total    => 0,
         };
     }
 
+    my $query;
     if ( $type eq 'n' ) {
         $query = {
             constant_score => {
@@ -829,7 +851,7 @@ sub recent {
 
     my $body = {
         size    => $page_size,
-        from    => $from,
+        from    => ( $page - 1 ) * $page_size,
         query   => $query,
         _source =>
             [qw(name author status abstract date distribution maturity)],
diff --git a/lib/MetaCPAN/Query/Search.pm b/lib/MetaCPAN/Query/Search.pm
@@ -8,7 +8,7 @@ use List::Util                qw( min uniq );
 use Log::Contextual           qw( :log :dlog );
 use MetaCPAN::ESConfig        qw( es_doc_path );
 use MetaCPAN::Types::TypeTiny qw( Object Str );
-use MetaCPAN::Util            qw( hit_total true false );
+use MetaCPAN::Util            qw( MAX_RESULT_WINDOW hit_total true false );
 use MooseX::StrictConstructor;
 
 with 'MetaCPAN::Query::Role::Common';
@@ -54,11 +54,20 @@ sub search_for_first_result {
 =cut
 
 sub search_web {
-    my ( $self, $search_term, $from, $page_size, $collapsed,
+    my ( $self, $search_term, $page, $page_size, $collapsed,
         $max_collapsed_hits )
         = @_;
     $page_size //= 20;
-    $from      //= 0;
+    $page      //= 1;
+
+    if ( $page * $page_size >= MAX_RESULT_WINDOW ) {
+        return {
+            results  => [],
+            total    => 0,
+            tool     => 0,
+            colapsed => $collapsed ? true : false,
+        };
+    }
 
     $search_term =~ s{([+=><!&|\(\)\{\}[\]\^"~*?\\/])}{\\$1}g;
 
@@ -78,15 +87,15 @@ sub search_web {
 
     my $results
         = $collapsed // $search_term !~ /(distribution|module\.name\S*):/
-        ? $self->_search_collapsed( $search_term, $from, $page_size,
+        ? $self->_search_collapsed( $search_term, $page, $page_size,
         $max_collapsed_hits )
-        : $self->_search_expanded( $search_term, $from, $page_size );
+        : $self->_search_expanded( $search_term, $page, $page_size );
 
     return $results;
 }
 
 sub _search_expanded {
-    my ( $self, $search_term, $from, $page_size ) = @_;
+    my ( $self, $search_term, $page, $page_size ) = @_;
 
     # Used for distribution and module searches, the limit is included in
     # the query and ES does the right thing (because we are not collapsing
@@ -95,7 +104,7 @@ sub _search_expanded {
         $search_term,
         {
             size => $page_size,
-            from => $from
+            from => ( $page - 1 ) * $page_size,
         }
     );
 
@@ -122,11 +131,12 @@ sub _search_expanded {
 }
 
 sub _search_collapsed {
-    my ( $self, $search_term, $from, $page_size, $max_collapsed_hits ) = @_;
+    my ( $self, $search_term, $page, $page_size, $max_collapsed_hits ) = @_;
 
     $max_collapsed_hits ||= 5;
 
-    my $total_size = $from + $page_size;
+    my $from       = ( $page - 1 ) * $page_size;
+    my $total_size = $page * $page_size;
 
     my $es_query_opts = {
         size    => 0,
diff --git a/lib/MetaCPAN/Server/Controller/Favorite.pm b/lib/MetaCPAN/Server/Controller/Favorite.pm
@@ -43,7 +43,7 @@ sub recent : Path('recent') : Args(0) {
     $c->stash_or_detach(
         $self->model($c)->recent(
             $c->req->param('page') || 1,
-            $c->req->param('size') || 100
+            $c->req->param('page_size') || $c->req->param('size') || 100,
         )
     );
 }
diff --git a/lib/MetaCPAN/Server/Controller/Release.pm b/lib/MetaCPAN/Server/Controller/Release.pm
@@ -45,14 +45,20 @@ sub modules : Path('modules') : Args(2) {
 
 sub recent : Path('recent') : Args(0) {
     my ( $self, $c ) = @_;
-    my @params = @{ $c->req->params }{qw( page page_size type )};
-    $c->stash_or_detach( $self->model($c)->recent(@params) );
+    my $params = $c->req->params;
+    my $type   = $params->{type};
+    my $page   = $params->{page};
+    my $size   = $params->{page_size};
+    $c->stash_or_detach( $self->model($c)->recent( $type, $page, $size ) );
 }
 
 sub by_author : Path('by_author') : Args(1) {
     my ( $self, $c, $pauseid ) = @_;
-    $c->stash_or_detach( $self->model($c)
-            ->by_author( $pauseid, @{ $c->req->params }{qw( size page )} ) );
+    my $params = $c->req->params;
+    my $page   = $params->{page};
+    my $size   = $params->{page_size} // $params->{size};
+    $c->stash_or_detach(
+        $self->model($c)->by_author( $pauseid, $page, $size ) );
 }
 
 sub latest_by_distribution : Path('latest_by_distribution') : Args(1) {
@@ -69,9 +75,11 @@ sub latest_by_author : Path('latest_by_author') : Args(1) {
 
 sub all_by_author : Path('all_by_author') : Args(1) {
     my ( $self, $c, $pauseid ) = @_;
-    my @params = @{ $c->req->params }{qw( page_size page )};
+    my $params = $c->req->params;
+    my $page   = $params->{page};
+    my $size   = $params->{page_size};
     $c->stash_or_detach(
-        $self->model($c)->all_by_author( $pauseid, @params ) );
+        $self->model($c)->all_by_author( $pauseid, $page, $size ) );
 }
 
 sub versions : Path('versions') : Args(1) {
diff --git a/lib/MetaCPAN/Server/Controller/ReverseDependencies.pm b/lib/MetaCPAN/Server/Controller/ReverseDependencies.pm
@@ -15,7 +15,7 @@ sub dist : Path('dist') : Args(1) {
     my ( $self, $c, $dist ) = @_;
     $c->stash_or_detach(
         $c->model('ESModel')->doc('release')->reverse_dependencies(
-            $dist, @{ $c->req->params }{qw< page page_size size sort >}
+            $dist, @{ $c->req->params }{qw< page page_size sort >}
         )
     );
 }
diff --git a/lib/MetaCPAN/Server/Controller/Search/Web.pm b/lib/MetaCPAN/Server/Controller/Search/Web.pm
@@ -30,8 +30,13 @@ sub web : Chained('/search/index') : PathPart('web') : Args(0) {
     my ( $self, $c ) = @_;
     my $args = $c->req->params;
 
+    my $query = $args->{q};
+    my $size  = $args->{page_size} // $args->{size} // 20;
+    my $page = $args->{page} // ( 1 + int( ( $args->{from} // 0 ) / $size ) );
+    my $collapsed = $args->{collapsed};
+
     my $model   = $c->model('Search');
-    my $results = $model->search_web( @{$args}{qw( q from size collapsed )} );
+    my $results = $model->search_web( $query, $page, $size, $collapsed );
 
     $c->stash($results);
 }
diff --git a/lib/MetaCPAN/Util.pm b/lib/MetaCPAN/Util.pm
@@ -37,9 +37,12 @@ use Sub::Exporter -setup => {
         true
         false
         is_bool
+        MAX_RESULT_WINDOW
     ) ]
 };
 
+use constant MAX_RESULT_WINDOW => 10000;
+
 *true    = \&Cpanel::JSON::XS::true;
 *false   = \&Cpanel::JSON::XS::false;
 *is_bool = \&Cpanel::JSON::XS::is_bool;
diff --git a/root/static/requests/search.yml b/root/static/requests/search.yml
@@ -18,14 +18,26 @@ search_web:
           See also `collapsed`
         type: string
         required: true
+      - name: page
+        in: query
+        description: The page of the results to return
+        type: integer
+      - name: page_size
+        in: query
+        description: Number of results per page
+        type: integer
       - name: from
         in: query
-        description: The offset to use in the result set
+        description: |
+          The offset to use in the result set. Deprecated. Only used if `page`
+          is not set.
         type: integer
         default: 0
       - name: size
         in: query
-        description: Number of results per page
+        description: |
+          Number of results per page. Deprecated. Only used if `page_size` is
+          not set.
         type: integer
         default: 20
       - name: collapsed
diff --git a/t/model/search.t b/t/model/search.t
@@ -35,12 +35,12 @@ ok( $search, 'search' );
 
     ok( $collapsed_search->{collapsed}, 'results are flagged as collapsed' );
 
-    my $from      = 0;
+    my $page      = 1;
     my $page_size = 20;
     my $collapsed = 0;
 
     my $expanded
-        = $search->search_web( 'Foo', $from, $page_size, $collapsed );
+        = $search->search_web( 'Foo', $page, $page_size, $collapsed );
 
     ok( !$expanded->{collapsed}, 'results are flagged as expanded' );
 

Original file line number	Diff line number	Diff line change
`@@ -43,7 +43,7 @@ sub recent : Path('recent') : Args(0) {`
`43`	`43`	`$c->stash_or_detach(`
`44`	`44`	`$self->model($c)->recent(`
`45`	`45`	`$c->req->param('page') \|\| 1,`
`46`		`- $c->req->param('size') \|\| 100`
	`46`	`+ $c->req->param('page_size') \|\| $c->req->param('size') \|\| 100,`
`47`	`47`	`)`
`48`	`48`	`);`
`49`	`49`	`}`
Original file line number	Diff line number	Diff line change
`@@ -15,7 +15,7 @@ sub dist : Path('dist') : Args(1) {`
`15`	`15`	`my ( $self, $c, $dist ) = @_;`
`16`	`16`	`$c->stash_or_detach(`
`17`	`17`	`$c->model('ESModel')->doc('release')->reverse_dependencies(`
`18`		`- $dist, @{ $c->req->params }{qw< page page_size size sort >}`
	`18`	`+ $dist, @{ $c->req->params }{qw< page page_size sort >}`
`19`	`19`	`)`
`20`	`20`	`);`
`21`	`21`	`}`