fix: remove skip from subsequent pages

Author: ricellis

modules/cloudant/src/main/java/com/ibm/cloud/cloudant/features/pagination/BasePageIterator.java

@@ -63,8 +63,11 @@ List<I> nextRequest() {
     if (items.size() < this.pageSize) {
       this.hasNext = false;
     } else {
+      O options = this.nextPageOptionsRef.get();
       B optionsBuilder = optsHandler.builderFromOptions(this.nextPageOptionsRef.get());
       setNextPageOptions(optionsBuilder, result);
+      // Remove any options valid on the user request, but invalid during paging
+      optionsBuilder = this.optsHandler.removeOptsForSubsequentPage(options, optionsBuilder);
       buildAndSetOptions(optionsBuilder);
     }
     return items;

modules/cloudant/src/main/java/com/ibm/cloud/cloudant/features/pagination/OptionsHandler.java

@@ -88,6 +88,11 @@ B applyLimit(B builder, Long newLimit) {
     return this.limitSetter.apply(builder, newLimit);
   }
 
+  B removeOptsForSubsequentPage(O options, B builder) {
+    // Default is a no-op
+    return builder;
+  }
+
   Long getPageSizeFromOptionsLimit(O opts) {
     return Optional.ofNullable(this.limitGetter.apply(opts)).orElse(MAX_LIMIT);
   }
@@ -172,15 +177,17 @@ private abstract static class KeyOptionsHandler<B, O, K> extends OptionsHandler<
 
     protected final Function<O, K> keyGetter;
     private final Function<O, List<K>> keysGetter;
+    protected final Function<O, Long> skipGetter;
     private final String keyErrorMsg =
         keyErrorMessage(new StringBuilder("when using pagination. "));
 
     protected KeyOptionsHandler(Function<B, O> builderToOptions, Function<O, B> optionsToBuilder,
         Function<O, Long> limitGetter, BiFunction<B, Long, B> limitSetter, Function<O, K> keyGetter,
-        Function<O, List<K>> keysGetter) {
+        Function<O, List<K>> keysGetter, Function<O, Long> skipGetter) {
       super(builderToOptions, optionsToBuilder, limitGetter, limitSetter);
       this.keyGetter = keyGetter;
       this.keysGetter = keysGetter;
+      this.skipGetter = skipGetter;
     }
 
     protected String keyErrorMessage(StringBuilder baseMessage) {
@@ -201,14 +208,27 @@ protected void validate(O options) {
           this.keyErrorMsg);
       super.validate(options);
     }
+
+    @Override
+    B removeOptsForSubsequentPage(O options, B builder) {
+      // Unset the skip option if necessary
+      if (optionIsPresent(options, this.skipGetter)) {
+        builder = this.builderFromOptions(replaceOpts(builder));
+      }
+      return super.removeOptsForSubsequentPage(options, builder);
+    }
+
+    protected abstract O replaceOpts(B builder);
   }
 
   private abstract static class ViewsOptionsHandler<B, O> extends KeyOptionsHandler<B, O, Object> {
 
     protected ViewsOptionsHandler(Function<B, O> builderToOptions, Function<O, B> optionsToBuilder,
         Function<O, Long> limitGetter, BiFunction<B, Long, B> limitSetter,
-        Function<O, Object> keyGetter, Function<O, List<Object>> keysGetter) {
-      super(builderToOptions, optionsToBuilder, limitGetter, limitSetter, keyGetter, keysGetter);
+        Function<O, Object> keyGetter, Function<O, List<Object>> keysGetter,
+        Function<O, Long> skipGetter) {
+      super(builderToOptions, optionsToBuilder, limitGetter, limitSetter, keyGetter, keysGetter,
+          skipGetter);
     }
 
     @Override
@@ -233,7 +253,17 @@ private static final class AllDocsOptionsHandler
     private AllDocsOptionsHandler() {
       super(PostAllDocsOptions.Builder::build, PostAllDocsOptions::newBuilder,
           PostAllDocsOptions::limit, PostAllDocsOptions.Builder::limit, PostAllDocsOptions::key,
-          PostAllDocsOptions::keys);
+          PostAllDocsOptions::keys, PostAllDocsOptions::skip);
+    }
+
+    @Override
+    protected PostAllDocsOptions replaceOpts(PostAllDocsOptions.Builder builder) {
+      return new PostAllDocsOptions(builder) {
+        PostAllDocsOptions unsetOpts() {
+          this.skip = null;
+          return this;
+        }
+      }.unsetOpts();
     }
 
   }
@@ -244,19 +274,45 @@ private static final class DesignDocsOptionsHandler
     private DesignDocsOptionsHandler() {
       super(PostDesignDocsOptions.Builder::build, PostDesignDocsOptions::newBuilder,
           PostDesignDocsOptions::limit, PostDesignDocsOptions.Builder::limit,
-          PostDesignDocsOptions::key, PostDesignDocsOptions::keys);
+          PostDesignDocsOptions::key, PostDesignDocsOptions::keys, PostDesignDocsOptions::skip);
+    }
+
+    @Override
+    protected PostDesignDocsOptions replaceOpts(PostDesignDocsOptions.Builder builder) {
+      return new PostDesignDocsOptions(builder) {
+        PostDesignDocsOptions unsetOpts() {
+          this.skip = null;
+          return this;
+        }
+      }.unsetOpts();
     }
 
   }
 
   private static final class FindOptionsHandler
       extends BookmarkOptionsHandler<PostFindOptions.Builder, PostFindOptions> {
 
+    private final Function<PostFindOptions, Long> skipGetter = PostFindOptions::skip;
+
     private FindOptionsHandler() {
       super(PostFindOptions.Builder::build, PostFindOptions::newBuilder, PostFindOptions::limit,
           PostFindOptions.Builder::limit);
     }
 
+    @Override
+    PostFindOptions.Builder removeOptsForSubsequentPage(PostFindOptions options,
+        PostFindOptions.Builder builder) {
+      if (optionIsPresent(options, this.skipGetter)) {
+        return new PostFindOptions(builder) {
+          PostFindOptions unsetOpts() {
+            this.skip = null;
+            return this;
+          }
+        }.unsetOpts().newBuilder();
+      }
+      return builder;
+    }
+
   }
 
   private static final class PartitionAllDocsOptionsHandler extends
@@ -265,19 +321,47 @@ private static final class PartitionAllDocsOptionsHandler extends
     private PartitionAllDocsOptionsHandler() {
       super(PostPartitionAllDocsOptions.Builder::build, PostPartitionAllDocsOptions::newBuilder,
           PostPartitionAllDocsOptions::limit, PostPartitionAllDocsOptions.Builder::limit,
-          PostPartitionAllDocsOptions::key, PostPartitionAllDocsOptions::keys);
+          PostPartitionAllDocsOptions::key, PostPartitionAllDocsOptions::keys,
+          PostPartitionAllDocsOptions::skip);
+    }
+
+    @Override
+    protected PostPartitionAllDocsOptions replaceOpts(PostPartitionAllDocsOptions.Builder builder) {
+      return new PostPartitionAllDocsOptions(builder) {
+        PostPartitionAllDocsOptions unsetOpts() {
+          this.skip = null;
+          return this;
+        }
+      }.unsetOpts();
     }
 
   }
 
   private static final class PartitionFindOptionsHandler
       extends BookmarkOptionsHandler<PostPartitionFindOptions.Builder, PostPartitionFindOptions> {
 
+    private final Function<PostPartitionFindOptions, Long> skipGetter =
+        PostPartitionFindOptions::skip;
+
     private PartitionFindOptionsHandler() {
       super(PostPartitionFindOptions.Builder::build, PostPartitionFindOptions::newBuilder,
           PostPartitionFindOptions::limit, PostPartitionFindOptions.Builder::limit);
     }
 
+    @Override
+    PostPartitionFindOptions.Builder removeOptsForSubsequentPage(PostPartitionFindOptions options,
+        PostPartitionFindOptions.Builder builder) {
+      if (optionIsPresent(options, this.skipGetter)) {
+        return new PostPartitionFindOptions(builder) {
+          PostPartitionFindOptions unsetOpts() {
+            this.skip = null;
+            return this;
+          }
+        }.unsetOpts().newBuilder();
+      }
+      return builder;
+    }
+
   }
 
   private static final class PartitionSearchOptionsHandler extends
@@ -296,7 +380,18 @@ private static final class PartitionViewOptionsHandler
     private PartitionViewOptionsHandler() {
       super(PostPartitionViewOptions.Builder::build, PostPartitionViewOptions::newBuilder,
           PostPartitionViewOptions::limit, PostPartitionViewOptions.Builder::limit,
-          PostPartitionViewOptions::key, PostPartitionViewOptions::keys);
+          PostPartitionViewOptions::key, PostPartitionViewOptions::keys,
+          PostPartitionViewOptions::skip);
+    }
+
+    @Override
+    protected PostPartitionViewOptions replaceOpts(PostPartitionViewOptions.Builder builder) {
+      return new PostPartitionViewOptions(builder) {
+        PostPartitionViewOptions unsetOpts() {
+          this.skip = null;
+          return this;
+        }
+      }.unsetOpts();
     }
 
   }
@@ -328,7 +423,18 @@ private static final class ViewOptionsHandler
 
     private ViewOptionsHandler() {
       super(PostViewOptions.Builder::build, PostViewOptions::newBuilder, PostViewOptions::limit,
-          PostViewOptions.Builder::limit, PostViewOptions::key, PostViewOptions::keys);
+          PostViewOptions.Builder::limit, PostViewOptions::key, PostViewOptions::keys,
+          PostViewOptions::skip);
+    }
+
+    @Override
+    protected PostViewOptions replaceOpts(PostViewOptions.Builder builder) {
+      return new PostViewOptions(builder) {
+        PostViewOptions unsetOpts() {
+          this.skip = null;
+          return this;
+        }
+      }.unsetOpts();
     }
 
   }

modules/cloudant/src/test/java/com/ibm/cloud/cloudant/features/pagination/BookmarkPageIteratorTest.java

@@ -38,7 +38,8 @@ public class BookmarkPageIteratorTest {
    * This test sub-class of BookmarkPager implicitly tests that various abstract methods are
    * correctly called.
    */
-  class TestBookmarkPager extends BookmarkPageIterator<Builder, PostFindOptions, TestResult, Integer> {
+  class TestBookmarkPager
+      extends BookmarkPageIterator<Builder, PostFindOptions, TestResult, Integer> {
 
     protected TestBookmarkPager(Cloudant client, PostFindOptions options) {
       super(client, options, OptionsHandler.POST_FIND);
@@ -187,4 +188,24 @@ void testGetAll() {
     Assert.assertEquals(actualItems, pageSupplier.allItems,
         "The results should match all the pages.");
   }
+
+  @Test
+  void testSkipRemovedForSubsequentPages() {
+    int pageSize = 3;
+    long expectedSkip = 17;
+    PageSupplier<TestResult, Integer> pageSupplier = newBasePageSupplier(pageSize * 3, pageSize);
+    MockPagerClient c = new MockPagerClient(pageSupplier);
+    PostFindOptions opts =
+        getRequiredTestFindOptionsBuilder().limit(pageSize).skip(expectedSkip).build();
+    TestBookmarkPager pager = new TestBookmarkPager(c, opts);
+    // Assert skip set for first request
+    Assert.assertEquals(pager.nextPageOptionsRef.get().skip(), expectedSkip,
+        "The skip should equal the user provided skip.");
+    pager.next();
+    // Assert skip not set for next page
+    Assert.assertNull(pager.nextPageOptionsRef.get().skip(),
+        "Skip should not be set for the next page.");
+    pager.next();
+  }
+
 }

modules/cloudant/src/test/java/com/ibm/cloud/cloudant/features/pagination/KeyPageIteratorTest.java

@@ -41,7 +41,8 @@ public class KeyPageIteratorTest {
    * This test sub-class of KeyPager implicitly tests that various abstract methods are correctly
    * called.
    */
-  class TestKeyPager extends KeyPageIterator<Integer, Builder, PostViewOptions, TestResult, Integer> {
+  class TestKeyPager
+      extends KeyPageIterator<Integer, Builder, PostViewOptions, TestResult, Integer> {
 
     protected TestKeyPager(Cloudant client, PostViewOptions options) {
       super(client, options, OptionsHandler.POST_VIEW);
@@ -289,4 +290,23 @@ Optional<String> checkBoundary(Integer penultimateItem, Integer lastItem) {
     Assert.assertFalse(pager.hasNext(), "hasNext() should return false.");
   }
 
+  @Test
+  void testSkipRemovedForSubsequentPages() {
+    int pageSize = 3;
+    long expectedSkip = 17;
+    PageSupplier<TestResult, Integer> pageSupplier = newKeyPageSupplier(pageSize * 3, pageSize);
+    MockPagerClient c = new MockPagerClient(pageSupplier);
+    PostViewOptions opts =
+        getRequiredTestOptionsBuilder().limit(pageSize).skip(expectedSkip).build();
+    TestKeyPager pager = new TestKeyPager(c, opts);
+    // Assert skip set for first request
+    Assert.assertEquals(pager.nextPageOptionsRef.get().skip(), expectedSkip,
+        "The skip should equal the user provided skip.");
+    pager.next();
+    // Assert skip not set for next page
+    Assert.assertNull(pager.nextPageOptionsRef.get().skip(),
+        "Skip should not be set for the next page.");
+    pager.next();
+  }
+
 }