Merge pull request #1460 from metacpan/oalders/metacpan-api/max-result-window

bump MAX_RESULT_WINDOW for favorites

Author: oalders

lib/MetaCPAN/Query/Favorite.pm

@@ -4,7 +4,8 @@ use MetaCPAN::Moose;
 
 use Log::Contextual    qw( :log );
 use MetaCPAN::ESConfig qw( es_doc_path );
-use MetaCPAN::Util     qw( hit_total paginate );
+use MetaCPAN::Util
+    qw( hit_total paginate MAX_FAVORITE_RESULT_WINDOW MAX_FAVORITE_PAGE_SIZE );
 
 with 'MetaCPAN::Query::Role::Common';
 
@@ -65,7 +66,9 @@ sub agg_by_distributions {
 sub by_user {
     my ( $self, $user, $page, $size ) = @_;
     my $from;
-    ( $page, $size, $from ) = paginate( $page, $size );
+    ( $page, $size, $from )
+        = paginate( $page, $size, MAX_FAVORITE_RESULT_WINDOW,
+        MAX_FAVORITE_PAGE_SIZE );
 
     return +{ favorites => [], took => 0, total => 0 }
         unless defined $page;

lib/MetaCPAN/Util.pm

@@ -42,26 +42,35 @@ use Sub::Exporter -setup => {
         is_bool
         to_bool
         MAX_RESULT_WINDOW
+        MAX_FAVORITE_RESULT_WINDOW
         MAX_PAGE_SIZE
+        MAX_FAVORITE_PAGE_SIZE
         paginate
     ) ]
 };
 
-# Limit the maximum result window to 1000, really should be enough!
+# Must not exceed the ES index.max_result_window setting (default 10,000).
 use constant MAX_RESULT_WINDOW => 1000;
-use constant MAX_PAGE_SIZE     => 250;
+
+# The UI's list_as_json needs a complete set of user favorites, so the default
+# of 250 will not be enough for heavy users of the ++ button.
+use constant MAX_FAVORITE_RESULT_WINDOW => 5000;
+use constant MAX_PAGE_SIZE              => 250;
+use constant MAX_FAVORITE_PAGE_SIZE     => 2000;
 
 # Returns ($page, $size, $from) or empty list if the request exceeds
-# the ES result window.  Use strict ">" so that page*size == MAX_RESULT_WINDOW
-# (e.g. page 4 × 250 = 1000) is still valid — ES allows from+size ≤ window.
+# the configured result window.  Use strict ">" so that page*size equal to
+# the window (e.g. page 4 × 250 = 1000) is still valid — ES allows from+size ≤ window.
 sub paginate {
-    my ( $page, $size ) = @_;
+    my ( $page, $size, $max_result_window, $max_page_size ) = @_;
+    $max_result_window //= MAX_RESULT_WINDOW;
+    $max_page_size     //= MAX_PAGE_SIZE;
     $page = Int->check($page) ? max( 1, $page ) : 1;
     $size
         = Int->check($size) && $size >= 1
-        ? min( MAX_PAGE_SIZE, $size )
-        : MAX_PAGE_SIZE;
-    return if $page * $size > MAX_RESULT_WINDOW;
+        ? min( $max_page_size, $size )
+        : $max_page_size;
+    return if $page * $size > $max_result_window;
     return ( $page, $size, ( $page - 1 ) * $size );
 }
 

t/server/controller/favorite.t

@@ -37,12 +37,12 @@ test_psgi app, sub {
         );
     };
 
-    subtest 'page beyond MAX_RESULT_WINDOW returns empty' => sub {
+    subtest 'page beyond MAX_FAVORITE_RESULT_WINDOW returns empty' => sub {
         ok(
             my $res = $cb->(
-                GET '/favorite/by_user/DOESNOTEXIST?page=100&page_size=250'
+                GET '/favorite/by_user/DOESNOTEXIST?page=21&page_size=250'
             ),
-            'GET with page*size > MAX_RESULT_WINDOW'
+            'GET with page*size > MAX_FAVORITE_RESULT_WINDOW'
         );
         is( $res->code, 200, 'code 200' );
         my $json = decode_json_ok($res);
@@ -53,12 +53,12 @@ test_psgi app, sub {
         );
     };
 
-    subtest 'page at MAX_RESULT_WINDOW boundary is allowed' => sub {
+    subtest 'page at MAX_FAVORITE_RESULT_WINDOW boundary is allowed' => sub {
         ok(
             my $res = $cb->(
-                GET '/favorite/by_user/DOESNOTEXIST?page=4&page_size=250'
+                GET '/favorite/by_user/DOESNOTEXIST?page=20&page_size=250'
             ),
-            'GET with page*size == MAX_RESULT_WINDOW'
+            'GET with page*size == MAX_FAVORITE_RESULT_WINDOW'
         );
         is( $res->code, 200, 'code 200' );
         my $json = decode_json_ok($res);

t/util.t

@@ -159,6 +159,27 @@ subtest 'paginate' => sub {
         'max page accepted'
     );
     is_deeply( [ paginate( 1001, 1 ) ], [], 'page beyond window' );
+
+    # Custom max_result_window parameter.
+    is_deeply(
+        [ paginate( 20, 250, 5000 ) ],
+        [ 20, 250, 4750 ],
+        'custom window: page*size == 5000 is allowed'
+    );
+    is_deeply( [ paginate( 21, 250, 5000 ) ],
+        [], 'custom window: page*size > 5000 returns empty' );
+
+    # Custom max_page_size parameter.
+    is_deeply(
+        [ paginate( 1, 2000, 5000, 2000 ) ],
+        [ 1, 2000, 0 ],
+        'custom page size: 2000 accepted'
+    );
+    is_deeply(
+        [ paginate( 1, 3000, 5000, 2000 ) ],
+        [ 1, 2000, 0 ],
+        'custom page size: 3000 capped to 2000'
+    );
     is_deeply( [ paginate( 1, 500 ) ],  [ 1, 250, 0 ], 'size > 250 capped' );
     is_deeply( [ paginate( 1, 1000 ) ], [ 1, 250, 0 ], 'size = 1000 capped' );