From 14bb790d5c05e727e4b24640cbf80d892b2c76c6 Mon Sep 17 00:00:00 2001 From: Rich Ellis Date: Thu, 3 Jul 2025 12:10:49 +0100 Subject: [PATCH 1/6] test: don't use plus one paging in base --- .../pagination/BasePageIteratorTest.java | 108 +++++++++--------- 1 file changed, 54 insertions(+), 54 deletions(-) diff --git a/modules/cloudant/src/test/java/com/ibm/cloud/cloudant/features/pagination/BasePageIteratorTest.java b/modules/cloudant/src/test/java/com/ibm/cloud/cloudant/features/pagination/BasePageIteratorTest.java index d8bb10fff..3d8e6e5b4 100644 --- a/modules/cloudant/src/test/java/com/ibm/cloud/cloudant/features/pagination/BasePageIteratorTest.java +++ b/modules/cloudant/src/test/java/com/ibm/cloud/cloudant/features/pagination/BasePageIteratorTest.java @@ -29,12 +29,12 @@ import com.ibm.cloud.cloudant.features.pagination.PaginationTestHelpers.PageSupplier; import com.ibm.cloud.cloudant.features.pagination.PaginationTestHelpers.TestResult; import com.ibm.cloud.cloudant.v1.Cloudant; -import com.ibm.cloud.cloudant.v1.model.PostViewOptions; -import com.ibm.cloud.cloudant.v1.model.PostViewOptions.Builder; +import com.ibm.cloud.cloudant.v1.model.PostFindOptions; +import com.ibm.cloud.cloudant.v1.model.PostFindOptions.Builder; import com.ibm.cloud.sdk.core.http.ServiceCall; -import static com.ibm.cloud.cloudant.features.pagination.PaginationTestHelpers.getDefaultTestOptions; -import static com.ibm.cloud.cloudant.features.pagination.PaginationTestHelpers.getRequiredTestOptionsBuilder; +import static com.ibm.cloud.cloudant.features.pagination.PaginationTestHelpers.getDefaultTestFindOptions; +import static com.ibm.cloud.cloudant.features.pagination.PaginationTestHelpers.getRequiredTestFindOptionsBuilder; import static com.ibm.cloud.cloudant.features.pagination.PaginationTestHelpers.newPageSupplierFromList; public class BasePageIteratorTest { @@ -45,10 +45,10 @@ public class BasePageIteratorTest { * This test sub-class of BasePager implicitly tests that various abstract methods are correctly * called by non-abstract methods in the BasePager. */ - private static class TestPager extends BasePageIterator { + private static class TestPager extends BasePageIterator { - protected TestPager(Cloudant client, PostViewOptions options) { - super(client, options, OptionsHandler.POST_VIEW); + protected TestPager(Cloudant client, PostFindOptions options) { + super(client, options, OptionsHandler.POST_FIND); } Cloudant getClient() { @@ -75,7 +75,7 @@ protected Function> itemsGetter() { } /** - * These tests don't actually use the options, but we set a startKey so we can validate calls to + * These tests don't actually use the options, but we set a bookmark so we can validate calls to * setNextPageOptions. */ @Override @@ -85,7 +85,7 @@ protected void setNextPageOptions(Builder builder, TestResult result) { throw new IllegalStateException("Test failure: tried to setNextPageOptions on empty page."); } else { Integer i = rows.get(rows.size() - 1); - builder.startKey(i); + builder.bookmark(String.valueOf(i)); } } @@ -94,7 +94,7 @@ protected void setNextPageOptions(Builder builder, TestResult result) { * work. */ @Override - protected BiFunction> nextRequestFunction() { + protected BiFunction> nextRequestFunction() { return (c, o) -> { return ((MockPagerClient) c).testCall(); }; @@ -111,7 +111,7 @@ private static class ThrowingTestPager extends TestPager { private int callCounter = 0; - protected ThrowingTestPager(Cloudant client, PostViewOptions options) { + protected ThrowingTestPager(Cloudant client, PostFindOptions options) { super(client, options); } @@ -128,20 +128,20 @@ List nextRequest() { } - private Pagination newPagination(Cloudant client, - PostViewOptions options) { + private Pagination newPagination(Cloudant client, + PostFindOptions options) { return new Pagination<>(client, options, TestPager::new); } - private Pagination newThrowingPagination(Cloudant client, - PostViewOptions options) { + private Pagination newThrowingPagination(Cloudant client, + PostFindOptions options) { return new Pagination<>(client, options, ThrowingTestPager::new); } // test constructor @Test void testConstructor() { - TestPager pager = new TestPager(mockClient, getDefaultTestOptions(42)); + TestPager pager = new TestPager(mockClient, getDefaultTestFindOptions(42)); // Assert the client Assert.assertEquals(((TestPager) pager).getClient(), mockClient, "The client should be the supplied one."); @@ -150,7 +150,7 @@ void testConstructor() { // test page size default @Test void testDefaultPageSize() { - TestPager pager = new TestPager(mockClient, getRequiredTestOptionsBuilder().build()); + TestPager pager = new TestPager(mockClient, getRequiredTestFindOptionsBuilder().build()); // Assert the default page size Assert.assertEquals(pager.pageSize, 200, "The page size should be the default."); } @@ -158,7 +158,7 @@ void testDefaultPageSize() { // test page size limit @Test void testLimitPageSize() { - TestPager pager = new TestPager(mockClient, getDefaultTestOptions(42)); + TestPager pager = new TestPager(mockClient, getDefaultTestFindOptions(42)); // Assert the limit provided as page size Assert.assertEquals(pager.pageSize, 42, "The page size should match the limit."); } @@ -166,7 +166,7 @@ void testLimitPageSize() { // test hasNext @Test void testHasNextIsTrueInitially() { - TestPager pager = new TestPager(mockClient, getDefaultTestOptions(42)); + TestPager pager = new TestPager(mockClient, getDefaultTestFindOptions(42)); Assert.assertEquals(pager.hasNext(), true, "hasNext() should initially return true."); } @@ -176,7 +176,7 @@ void testHasNextIsTrueForResultEqualToLimit() { MockPagerClient c = new MockPagerClient(() -> { return new MockInstruction(new TestResult(Collections.singletonList(1))); }); - TestPager pager = new TestPager(c, getDefaultTestOptions(1)); + TestPager pager = new TestPager(c, getDefaultTestFindOptions(1)); // Get the first page (size 1) pager.next(); // hasNext should return true because results size is the same as limit @@ -189,7 +189,7 @@ void testHasNextIsFalseForResultLessThanLimit() { MockPagerClient c = new MockPagerClient(() -> { return new MockInstruction(new TestResult(Collections.emptyList())); }); - TestPager pager = new TestPager(c, getDefaultTestOptions(1)); + TestPager pager = new TestPager(c, getDefaultTestFindOptions(1)); // Get the first page (size 0) pager.next(); // hasNext should return false because results size smaller than limit @@ -203,7 +203,7 @@ void testNextFirstPage() { PageSupplier pageSupplier = PaginationTestHelpers.newBasePageSupplier(pageSize, pageSize); MockPagerClient c = new MockPagerClient(pageSupplier); - TestPager pager = new TestPager(c, getDefaultTestOptions(pageSize)); + TestPager pager = new TestPager(c, getDefaultTestFindOptions(pageSize)); List actualPage = pager.next(); // Assert first page Assert.assertEquals(actualPage, pageSupplier.pages.get(0), @@ -216,7 +216,7 @@ void testNextTwoPages() { PageSupplier pageSupplier = PaginationTestHelpers.newBasePageSupplier(2 * pageSize, pageSize); MockPagerClient c = new MockPagerClient(pageSupplier); - TestPager pager = new TestPager(c, getDefaultTestOptions(pageSize)); + TestPager pager = new TestPager(c, getDefaultTestFindOptions(pageSize)); Assert.assertEquals(pager.hasNext(), true, "hasNext() should return true."); List actualPage1 = pager.next(); // Assert pages @@ -235,7 +235,7 @@ void testNextUntilEmpty() { PageSupplier pageSupplier = PaginationTestHelpers.newBasePageSupplier(3 * pageSize, pageSize); MockPagerClient c = new MockPagerClient(pageSupplier); - TestPager pager = new TestPager(c, getDefaultTestOptions(pageSize)); + TestPager pager = new TestPager(c, getDefaultTestFindOptions(pageSize)); List actualItems = new ArrayList<>(); int pageCount = 0; while (pager.hasNext()) { @@ -259,7 +259,7 @@ void testNextUntilSmaller() { PageSupplier pageSupplier = PaginationTestHelpers.newBasePageSupplier(10, pageSize); MockPagerClient c = new MockPagerClient(pageSupplier); - TestPager pager = new TestPager(c, getDefaultTestOptions(pageSize)); + TestPager pager = new TestPager(c, getDefaultTestFindOptions(pageSize)); List actualItems = new ArrayList<>(); int pageCount = 0; while (pager.hasNext()) { @@ -283,7 +283,7 @@ void testNextException() { PageSupplier pageSupplier = PaginationTestHelpers.newBasePageSupplier(pageSize - 1, pageSize); MockPagerClient c = new MockPagerClient(pageSupplier); - TestPager pager = new TestPager(c, getDefaultTestOptions(pageSize)); + TestPager pager = new TestPager(c, getDefaultTestFindOptions(pageSize)); List actualPage = pager.next(); // Assert first page Assert.assertEquals(actualPage, pageSupplier.pages.get(0), @@ -302,7 +302,7 @@ void testPagesAreImmutable() { PageSupplier pageSupplier = PaginationTestHelpers.newBasePageSupplier(pageSize, pageSize); MockPagerClient c = new MockPagerClient(pageSupplier); - TestPager pager = new TestPager(c, getDefaultTestOptions(pageSize)); + TestPager pager = new TestPager(c, getDefaultTestFindOptions(pageSize)); List actualPage = pager.next(); // Assert immutable Assert.assertThrows(UnsupportedOperationException.class, () -> { @@ -317,20 +317,20 @@ void testSetNextPageOptions() { PageSupplier pageSupplier = PaginationTestHelpers.newBasePageSupplier(5 * pageSize, pageSize); MockPagerClient c = new MockPagerClient(pageSupplier); - TestPager pager = new TestPager(c, getDefaultTestOptions(pageSize)); - Assert.assertNull(pager.nextPageOptionsRef.get().startKey(), - "startKey should initially be null."); - // Since we use a page size of 1, each next page options key, is the same as the element from + TestPager pager = new TestPager(c, getDefaultTestFindOptions(pageSize)); + Assert.assertNull(pager.nextPageOptionsRef.get().bookmark(), + "bookamrk should initially be null."); + // Since we use a page size of 1, each next page options bookmark, is the same as the element from // the page int page = 0; while (pager.hasNext()) { pager.next(); if (pager.hasNext()) { - Assert.assertEquals(pager.nextPageOptionsRef.get().startKey(), page, - "the startKey should increment per page."); + Assert.assertEquals(pager.nextPageOptionsRef.get().bookmark(), String.valueOf(page), + "the bookmark should increment per page."); } else { // Options don't change for last page - Assert.assertEquals(pager.nextPageOptionsRef.get().startKey(), page - 1, + Assert.assertEquals(pager.nextPageOptionsRef.get().bookmark(), String.valueOf(page - 1), "The options should not be set for the final page."); } page++; @@ -343,20 +343,20 @@ void testNextResumesAfterError() { PageSupplier pageSupplier = PaginationTestHelpers.newBasePageSupplier(2 * pageSize, pageSize); MockPagerClient c = new MockPagerClient(pageSupplier); - TestPager pager = new ThrowingTestPager(c, getDefaultTestOptions(pageSize)); + TestPager pager = new ThrowingTestPager(c, getDefaultTestFindOptions(pageSize)); List actualPage1 = pager.next(); // Assert pages Assert.assertEquals(actualPage1, pageSupplier.pages.get(0), "The actual page should match the expected page."); - // The startKey should point to row 2 (the last row we saw, note this is not doing n+1 paging) - Assert.assertEquals(pager.nextPageOptionsRef.get().startKey(), 2, - "The startKey should be 2 for the second page."); + // The bookmark should point to row 2 (the last row we saw, note this is not doing n+1 paging) + Assert.assertEquals(pager.nextPageOptionsRef.get().bookmark(), "2", + "The bookmark should be 2 for the second page."); Assert.assertThrows(RuntimeException.class, () -> pager.next()); // Assert hasNext Assert.assertEquals(pager.hasNext(), true, "hasNext() should return true."); - // The startKey should still point to the second page - Assert.assertEquals(pager.nextPageOptionsRef.get().startKey(), 2, - "The startKey should be 2 for the second page."); + // The bookmark should still point to the second page + Assert.assertEquals(pager.nextPageOptionsRef.get().bookmark(), "2", + "The bookmark should be 2 for the second page."); List actualPage2 = pager.next(); Assert.assertEquals(actualPage2, pageSupplier.pages.get(1), "The actual page should match the expected page."); @@ -369,7 +369,7 @@ void testAsPagerGetNextFirstPage() { PageSupplier pageSupplier = PaginationTestHelpers.newBasePageSupplier(2 * pageSize, pageSize); MockPagerClient c = new MockPagerClient(pageSupplier); - Pager pager = newPagination(c, getDefaultTestOptions(pageSize)).pager(); + Pager pager = newPagination(c, getDefaultTestFindOptions(pageSize)).pager(); List actualPage = pager.getNext(); // Assert first page Assert.assertEquals(actualPage, pageSupplier.pages.get(0), @@ -383,7 +383,7 @@ void testAsPagerGetNextUntilConsumed() { PageSupplier pageSupplier = PaginationTestHelpers.newBasePageSupplier(2 * pageSize, pageSize); MockPagerClient c = new MockPagerClient(pageSupplier); - Pager pager = newPagination(c, getDefaultTestOptions(pageSize)).pager(); + Pager pager = newPagination(c, getDefaultTestFindOptions(pageSize)).pager(); List actualItems = new ArrayList<>(); Iterator> expectedPages = pageSupplier.pages.iterator(); int pageCount = 0; @@ -412,7 +412,7 @@ void testAsPagerGetAll() { PageSupplier pageSupplier = PaginationTestHelpers.newBasePageSupplier(71, pageSize); MockPagerClient c = new MockPagerClient(pageSupplier); - Pager pager = newPagination(c, getDefaultTestOptions(pageSize)).pager(); + Pager pager = newPagination(c, getDefaultTestFindOptions(pageSize)).pager(); List actualItems = pager.getAll(); Assert.assertEquals(actualItems, pageSupplier.allItems, "The results should match all the pages."); @@ -432,7 +432,7 @@ void testAsPagerGetNextResumesAfterError() { PageSupplier pageSupplier = PaginationTestHelpers.newBasePageSupplier(2 * pageSize, pageSize); MockPagerClient c = new MockPagerClient(pageSupplier); - Pager pager = newThrowingPagination(c, getDefaultTestOptions(pageSize)).pager(); + Pager pager = newThrowingPagination(c, getDefaultTestFindOptions(pageSize)).pager(); List actualPage1 = pager.getNext(); // Assert pages Assert.assertEquals(actualPage1, pageSupplier.pages.get(0), @@ -457,8 +457,8 @@ void testAsPagerGetAllRestartsAfterError() { Collections.emptyList())); // final empty page MockPagerClient c = new MockPagerClient(pageSupplier); final AtomicInteger constructedOnce = new AtomicInteger(); - Pagination errorOnFirst = new Pagination(c, - getDefaultTestOptions(pageSize), (client, opts) -> { + Pagination errorOnFirst = new Pagination(c, + getDefaultTestFindOptions(pageSize), (client, opts) -> { // Note that Pager automatically makes an iterator for hasNext/getNext so that is call 0 // Call 1 is the first getAll (that will throw) // Call 2 should not throw @@ -484,7 +484,7 @@ void testAsPagerGetNextGetAllThrows() { PageSupplier pageSupplier = PaginationTestHelpers.newBasePageSupplier(2 * pageSize, pageSize); MockPagerClient c = new MockPagerClient(pageSupplier); - Pager pager = newPagination(c, getDefaultTestOptions(pageSize)).pager(); + Pager pager = newPagination(c, getDefaultTestFindOptions(pageSize)).pager(); // Assert first page Assert.assertEquals(pager.getNext(), pageSupplier.pages.get(0), "The actual page should match the expected page."); @@ -505,8 +505,8 @@ void testAsPagerGetAllGetNextThrows() { PageSupplier pageSupplier = PaginationTestHelpers.newBasePageSupplier(2 * pageSize, pageSize); MockPagerClient c = new MockPagerClient(pageSupplier); - Pager pager = new Pagination(c, - getDefaultTestOptions(pageSize), ThrowingTestPager::new).pager(); + Pager pager = new Pagination(c, + getDefaultTestFindOptions(pageSize), ThrowingTestPager::new).pager(); // Make sure we stop the getAll in a non-consumed state Assert.assertThrows(RuntimeException.class, () -> pager.getAll() @@ -525,7 +525,7 @@ void testPagesIterable() { PaginationTestHelpers.newBasePageSupplier(3 * pageSize - 1, pageSize); MockPagerClient c = new MockPagerClient(pageSupplier); Iterator> expectedPages = pageSupplier.pages.iterator(); - for (List page : newPagination(c, getDefaultTestOptions(pageSize)).pages()) { + for (List page : newPagination(c, getDefaultTestFindOptions(pageSize)).pages()) { Assert.assertEquals(page, expectedPages.next()); } } @@ -538,7 +538,7 @@ void testRowsIterable() { PaginationTestHelpers.newBasePageSupplier(3 * pageSize - 1, pageSize); MockPagerClient c = new MockPagerClient(pageSupplier); Iterator expectedRows = pageSupplier.allItems.iterator(); - for (Integer row : newPagination(c, getDefaultTestOptions(pageSize)).rows()) { + for (Integer row : newPagination(c, getDefaultTestFindOptions(pageSize)).rows()) { Assert.assertEquals(row, expectedRows.next()); } } @@ -551,7 +551,7 @@ void testPagesStream() { PaginationTestHelpers.newBasePageSupplier(4 * pageSize, pageSize); MockPagerClient c = new MockPagerClient(pageSupplier); Iterator> expectedPages = pageSupplier.pages.iterator(); - newPagination(c, getDefaultTestOptions(pageSize)).pages() + newPagination(c, getDefaultTestFindOptions(pageSize)).pages() .forEach(i -> Assert.assertEquals(i, expectedPages.next())); } @@ -563,7 +563,7 @@ void testRowsStream() { PaginationTestHelpers.newBasePageSupplier(4 * pageSize, pageSize); MockPagerClient c = new MockPagerClient(pageSupplier); Iterator expectedRows = pageSupplier.allItems.iterator(); - newPagination(c, getDefaultTestOptions(pageSize)).rows() + newPagination(c, getDefaultTestFindOptions(pageSize)).rows() .forEach(i -> Assert.assertEquals(i, expectedRows.next())); } From 66c70c50bc73d26a5d80f71e81a73b895d6db04f Mon Sep 17 00:00:00 2001 From: Rich Ellis Date: Wed, 2 Jul 2025 16:55:01 +0100 Subject: [PATCH 2/6] fix: plus one paging limits --- .../pagination/AllDocsPageIterator.java | 16 -- .../AllDocsPartitionPageIterator.java | 16 -- .../features/pagination/BasePageIterator.java | 21 +-- .../pagination/DesignDocsPageIterator.java | 16 -- .../features/pagination/FindPager.java | 16 -- .../pagination/FindPartitionPager.java | 16 -- .../features/pagination/KeyPageIterator.java | 5 - .../features/pagination/OptionsHandler.java | 146 ++++++++++++++++-- .../pagination/SearchPageIterator.java | 16 -- .../SearchPartitionPageIterator.java | 16 -- .../features/pagination/ViewPageIterator.java | 16 -- .../pagination/ViewPartitionPageIterator.java | 16 -- .../pagination/BasePageIteratorTest.java | 14 -- .../pagination/BookmarkPageIteratorTest.java | 15 -- .../pagination/KeyPageIteratorTest.java | 26 ++-- 15 files changed, 151 insertions(+), 220 deletions(-) diff --git a/modules/cloudant/src/main/java/com/ibm/cloud/cloudant/features/pagination/AllDocsPageIterator.java b/modules/cloudant/src/main/java/com/ibm/cloud/cloudant/features/pagination/AllDocsPageIterator.java index 02871a546..376ba456d 100644 --- a/modules/cloudant/src/main/java/com/ibm/cloud/cloudant/features/pagination/AllDocsPageIterator.java +++ b/modules/cloudant/src/main/java/com/ibm/cloud/cloudant/features/pagination/AllDocsPageIterator.java @@ -14,7 +14,6 @@ package com.ibm.cloud.cloudant.features.pagination; import java.util.function.BiFunction; -import java.util.function.Function; import com.ibm.cloud.cloudant.v1.Cloudant; import com.ibm.cloud.cloudant.v1.model.AllDocsResult; import com.ibm.cloud.cloudant.v1.model.PostAllDocsOptions; @@ -27,16 +26,6 @@ final class AllDocsPageIterator extends AllDocsBasePageIterator optionsToBuilderFunction() { - return PostAllDocsOptions::newBuilder; - } - - @Override - Function builderToOptionsFunction() { - return Builder::build; - } - @Override BiFunction> nextRequestFunction() { return Cloudant::postAllDocs; @@ -47,9 +36,4 @@ BiFunction nextKeySetter() { return Builder::startKey; } - @Override - Function limitGetter() { - return PostAllDocsOptions::limit; - } - } diff --git a/modules/cloudant/src/main/java/com/ibm/cloud/cloudant/features/pagination/AllDocsPartitionPageIterator.java b/modules/cloudant/src/main/java/com/ibm/cloud/cloudant/features/pagination/AllDocsPartitionPageIterator.java index f48fa8534..cd38455df 100644 --- a/modules/cloudant/src/main/java/com/ibm/cloud/cloudant/features/pagination/AllDocsPartitionPageIterator.java +++ b/modules/cloudant/src/main/java/com/ibm/cloud/cloudant/features/pagination/AllDocsPartitionPageIterator.java @@ -14,7 +14,6 @@ package com.ibm.cloud.cloudant.features.pagination; import java.util.function.BiFunction; -import java.util.function.Function; import com.ibm.cloud.cloudant.v1.Cloudant; import com.ibm.cloud.cloudant.v1.model.AllDocsResult; import com.ibm.cloud.cloudant.v1.model.PostPartitionAllDocsOptions; @@ -27,16 +26,6 @@ final class AllDocsPartitionPageIterator extends AllDocsBasePageIterator optionsToBuilderFunction() { - return PostPartitionAllDocsOptions::newBuilder; - } - - @Override - Function builderToOptionsFunction() { - return Builder::build; - } - @Override BiFunction> nextRequestFunction() { return Cloudant::postPartitionAllDocs; @@ -48,9 +37,4 @@ BiFunction nextKeySetter() { return Builder::startKey; } - @Override - Function limitGetter() { - return PostPartitionAllDocsOptions::limit; - } - } diff --git a/modules/cloudant/src/main/java/com/ibm/cloud/cloudant/features/pagination/BasePageIterator.java b/modules/cloudant/src/main/java/com/ibm/cloud/cloudant/features/pagination/BasePageIterator.java index fe3930d53..091170c31 100644 --- a/modules/cloudant/src/main/java/com/ibm/cloud/cloudant/features/pagination/BasePageIterator.java +++ b/modules/cloudant/src/main/java/com/ibm/cloud/cloudant/features/pagination/BasePageIterator.java @@ -17,7 +17,6 @@ import java.util.Iterator; import java.util.List; import java.util.NoSuchElementException; -import java.util.Optional; import java.util.concurrent.atomic.AtomicReference; import java.util.function.BiFunction; import java.util.function.Function; @@ -36,9 +35,10 @@ abstract class BasePageIterator implements Iterator> { this.client = client; this.optsHandler = optsHandler; // Set the page size from the supplied options limit - this.pageSize = getPageSizeFromOptionsLimit(options); + this.pageSize = this.optsHandler.getPageSizeFromOptionsLimit(options); // Set the first page options - buildAndSetOptions(optsHandler.builderFromOptions(options)); + buildAndSetOptions( + this.optsHandler.applyLimit(this.optsHandler.builderFromOptions(options), this.pageSize)); } @Override @@ -56,7 +56,8 @@ public List next() { } List nextRequest() { - ServiceCall request = nextRequestFunction().apply(this.client, this.nextPageOptionsRef.get()); + ServiceCall request = + nextRequestFunction().apply(this.client, this.nextPageOptionsRef.get()); R result = request.execute().getResult(); List items = itemsGetter().apply(result); if (items.size() < this.pageSize) { @@ -69,24 +70,14 @@ List nextRequest() { return items; } - private void buildAndSetOptions(B optionsBuilder) { + protected void buildAndSetOptions(B optionsBuilder) { this.nextPageOptionsRef.set(optsHandler.optionsFromBuilder(optionsBuilder)); } - abstract Function optionsToBuilderFunction(); - - abstract Function builderToOptionsFunction(); - abstract Function> itemsGetter(); abstract void setNextPageOptions(B builder, R result); abstract BiFunction> nextRequestFunction(); - abstract Function limitGetter(); - - Long getPageSizeFromOptionsLimit(O opts) { - return Optional.ofNullable(limitGetter().apply(opts)).orElse(200L); - } - } diff --git a/modules/cloudant/src/main/java/com/ibm/cloud/cloudant/features/pagination/DesignDocsPageIterator.java b/modules/cloudant/src/main/java/com/ibm/cloud/cloudant/features/pagination/DesignDocsPageIterator.java index c593d5a41..5ce7bc496 100644 --- a/modules/cloudant/src/main/java/com/ibm/cloud/cloudant/features/pagination/DesignDocsPageIterator.java +++ b/modules/cloudant/src/main/java/com/ibm/cloud/cloudant/features/pagination/DesignDocsPageIterator.java @@ -14,7 +14,6 @@ package com.ibm.cloud.cloudant.features.pagination; import java.util.function.BiFunction; -import java.util.function.Function; import com.ibm.cloud.cloudant.v1.Cloudant; import com.ibm.cloud.cloudant.v1.model.AllDocsResult; import com.ibm.cloud.cloudant.v1.model.PostDesignDocsOptions; @@ -32,24 +31,9 @@ BiFunction nextKeySetter() { return Builder::key; } - @Override - Function optionsToBuilderFunction() { - return PostDesignDocsOptions::newBuilder; - } - - @Override - Function builderToOptionsFunction() { - return Builder::build; - } - @Override BiFunction> nextRequestFunction() { return Cloudant::postDesignDocs; } - @Override - Function limitGetter() { - return PostDesignDocsOptions::limit; - } - } diff --git a/modules/cloudant/src/main/java/com/ibm/cloud/cloudant/features/pagination/FindPager.java b/modules/cloudant/src/main/java/com/ibm/cloud/cloudant/features/pagination/FindPager.java index d05a462e4..d8ea14b85 100644 --- a/modules/cloudant/src/main/java/com/ibm/cloud/cloudant/features/pagination/FindPager.java +++ b/modules/cloudant/src/main/java/com/ibm/cloud/cloudant/features/pagination/FindPager.java @@ -14,7 +14,6 @@ package com.ibm.cloud.cloudant.features.pagination; import java.util.function.BiFunction; -import java.util.function.Function; import com.ibm.cloud.cloudant.v1.Cloudant; import com.ibm.cloud.cloudant.v1.model.FindResult; import com.ibm.cloud.cloudant.v1.model.PostFindOptions; @@ -27,16 +26,6 @@ final class FindPager extends FindBasePageIterator optionsToBuilderFunction() { - return PostFindOptions::newBuilder; - } - - @Override - Function builderToOptionsFunction() { - return Builder::build; - } - @Override BiFunction bookmarkSetter() { return Builder::bookmark; @@ -47,9 +36,4 @@ BiFunction> nextRequestFuncti return Cloudant::postFind; } - @Override - Function limitGetter() { - return PostFindOptions::limit; - } - } diff --git a/modules/cloudant/src/main/java/com/ibm/cloud/cloudant/features/pagination/FindPartitionPager.java b/modules/cloudant/src/main/java/com/ibm/cloud/cloudant/features/pagination/FindPartitionPager.java index ca2467306..d34342d8b 100644 --- a/modules/cloudant/src/main/java/com/ibm/cloud/cloudant/features/pagination/FindPartitionPager.java +++ b/modules/cloudant/src/main/java/com/ibm/cloud/cloudant/features/pagination/FindPartitionPager.java @@ -14,7 +14,6 @@ package com.ibm.cloud.cloudant.features.pagination; import java.util.function.BiFunction; -import java.util.function.Function; import com.ibm.cloud.cloudant.v1.Cloudant; import com.ibm.cloud.cloudant.v1.model.FindResult; import com.ibm.cloud.cloudant.v1.model.PostPartitionFindOptions; @@ -27,16 +26,6 @@ final class FindPartitionPager extends FindBasePageIterator optionsToBuilderFunction() { - return PostPartitionFindOptions::newBuilder; - } - - @Override - Function builderToOptionsFunction() { - return Builder::build; - } - @Override BiFunction bookmarkSetter() { return Builder::bookmark; @@ -47,9 +36,4 @@ BiFunction> nextRequ return Cloudant::postPartitionFind; } - @Override - Function limitGetter() { - return PostPartitionFindOptions::limit; - } - } diff --git a/modules/cloudant/src/main/java/com/ibm/cloud/cloudant/features/pagination/KeyPageIterator.java b/modules/cloudant/src/main/java/com/ibm/cloud/cloudant/features/pagination/KeyPageIterator.java index 8f3e8be7d..503db47af 100644 --- a/modules/cloudant/src/main/java/com/ibm/cloud/cloudant/features/pagination/KeyPageIterator.java +++ b/modules/cloudant/src/main/java/com/ibm/cloud/cloudant/features/pagination/KeyPageIterator.java @@ -63,11 +63,6 @@ final void setNextPageOptions(B builder, R result) { nextKeyIdSetter().ifPresent(f -> f.apply(builder, nextId)); } - @Override - Long getPageSizeFromOptionsLimit(O opts) { - return super.getPageSizeFromOptionsLimit(opts) + 1; - } - abstract Optional checkBoundary(I penultimateItem, I lastItem); } diff --git a/modules/cloudant/src/main/java/com/ibm/cloud/cloudant/features/pagination/OptionsHandler.java b/modules/cloudant/src/main/java/com/ibm/cloud/cloudant/features/pagination/OptionsHandler.java index 13523e438..85d623966 100644 --- a/modules/cloudant/src/main/java/com/ibm/cloud/cloudant/features/pagination/OptionsHandler.java +++ b/modules/cloudant/src/main/java/com/ibm/cloud/cloudant/features/pagination/OptionsHandler.java @@ -17,6 +17,7 @@ import java.util.HashMap; import java.util.Map; import java.util.Optional; +import java.util.function.BiFunction; import java.util.function.Function; import java.util.function.Supplier; import com.ibm.cloud.cloudant.v1.model.PostAllDocsOptions; @@ -72,10 +73,22 @@ O optionsFromBuilder(B builder) { abstract void validate(O options); - O clone(O options) { + private O clone(O options) { return this.optionsFromBuilder(this.builderFromOptions(options)); } + B applyLimit(B builder, Long newLimit) { + return this.limitSetter().apply(builder, newLimit); + } + + abstract BiFunction limitSetter(); + + abstract Function limitGetter(); + + Long getPageSizeFromOptionsLimit(O opts) { + return Optional.ofNullable(limitGetter().apply(opts)).orElse(MAX_LIMIT); + } + static final PostAllDocsOptions duplicate(PostAllDocsOptions opts) { return POST_ALL_DOCS.clone(opts); } @@ -142,13 +155,46 @@ private static void validateOptionsAbsent(final Map> options } } + private abstract static class KeyOptionsHandler + extends OptionsHandler { + + KeyOptionsHandler(Function builderToOptions, Function optionsToBuilder) { + super(builderToOptions, optionsToBuilder); + } + + @Override + Long getPageSizeFromOptionsLimit(O opts) { + return super.getPageSizeFromOptionsLimit(opts) + 1; + } + + } + + private abstract static class BookmarkOptionsHandler + extends OptionsHandler { + + BookmarkOptionsHandler(Function builderToOptions, Function optionsToBuilder) { + super(builderToOptions, optionsToBuilder); + } + + } + private static final class AllDocsOptionsHandler - extends OptionsHandler { + extends KeyOptionsHandler { private AllDocsOptionsHandler() { super(PostAllDocsOptions.Builder::build, PostAllDocsOptions::newBuilder); } + @Override + Function limitGetter() { + return PostAllDocsOptions::limit; + } + + @Override + BiFunction limitSetter() { + return PostAllDocsOptions.Builder::limit; + } + @Override void validate(PostAllDocsOptions options) { validateLimit(options::limit); @@ -158,12 +204,22 @@ void validate(PostAllDocsOptions options) { } private static final class DesignDocsOptionsHandler - extends OptionsHandler { + extends KeyOptionsHandler { private DesignDocsOptionsHandler() { super(PostDesignDocsOptions.Builder::build, PostDesignDocsOptions::newBuilder); } + @Override + Function limitGetter() { + return PostDesignDocsOptions::limit; + } + + @Override + BiFunction limitSetter() { + return PostDesignDocsOptions.Builder::limit; + } + @Override void validate(PostDesignDocsOptions options) { validateLimit(options::limit); @@ -173,12 +229,22 @@ void validate(PostDesignDocsOptions options) { } private static final class FindOptionsHandler - extends OptionsHandler { + extends BookmarkOptionsHandler { private FindOptionsHandler() { super(PostFindOptions.Builder::build, PostFindOptions::newBuilder); } + @Override + Function limitGetter() { + return PostFindOptions::limit; + } + + @Override + BiFunction limitSetter() { + return PostFindOptions.Builder::limit; + } + @Override void validate(PostFindOptions options) { validateLimit(options::limit); @@ -187,12 +253,22 @@ void validate(PostFindOptions options) { } private static final class PartitionAllDocsOptionsHandler - extends OptionsHandler { + extends KeyOptionsHandler { private PartitionAllDocsOptionsHandler() { super(PostPartitionAllDocsOptions.Builder::build, PostPartitionAllDocsOptions::newBuilder); } + @Override + Function limitGetter() { + return PostPartitionAllDocsOptions::limit; + } + + @Override + BiFunction limitSetter() { + return PostPartitionAllDocsOptions.Builder::limit; + } + @Override void validate(PostPartitionAllDocsOptions options) { validateLimit(options::limit); @@ -202,12 +278,22 @@ void validate(PostPartitionAllDocsOptions options) { } private static final class PartitionFindOptionsHandler - extends OptionsHandler { + extends BookmarkOptionsHandler { private PartitionFindOptionsHandler() { super(PostPartitionFindOptions.Builder::build, PostPartitionFindOptions::newBuilder); } + @Override + Function limitGetter() { + return PostPartitionFindOptions::limit; + } + + @Override + BiFunction limitSetter() { + return PostPartitionFindOptions.Builder::limit; + } + @Override void validate(PostPartitionFindOptions options) { validateLimit(options::limit); @@ -216,12 +302,22 @@ void validate(PostPartitionFindOptions options) { } private static final class PartitionSearchOptionsHandler - extends OptionsHandler { + extends BookmarkOptionsHandler { private PartitionSearchOptionsHandler() { super(PostPartitionSearchOptions.Builder::build, PostPartitionSearchOptions::newBuilder); } + @Override + Function limitGetter() { + return PostPartitionSearchOptions::limit; + } + + @Override + BiFunction limitSetter() { + return PostPartitionSearchOptions.Builder::limit; + } + @Override void validate(PostPartitionSearchOptions options) { validateLimit(options::limit); @@ -230,12 +326,22 @@ void validate(PostPartitionSearchOptions options) { } private static final class PartitionViewOptionsHandler - extends OptionsHandler { + extends KeyOptionsHandler { private PartitionViewOptionsHandler() { super(PostPartitionViewOptions.Builder::build, PostPartitionViewOptions::newBuilder); } + @Override + Function limitGetter() { + return PostPartitionViewOptions::limit; + } + + @Override + BiFunction limitSetter() { + return PostPartitionViewOptions.Builder::limit; + } + @Override void validate(PostPartitionViewOptions options) { validateLimit(options::limit); @@ -245,12 +351,22 @@ void validate(PostPartitionViewOptions options) { } private static final class SearchOptionsHandler - extends OptionsHandler { + extends BookmarkOptionsHandler { private SearchOptionsHandler() { super(PostSearchOptions.Builder::build, PostSearchOptions::newBuilder); } + @Override + Function limitGetter() { + return PostSearchOptions::limit; + } + + @Override + BiFunction limitSetter() { + return PostSearchOptions.Builder::limit; + } + @Override void validate(PostSearchOptions options) { validateLimit(options::limit); @@ -266,12 +382,22 @@ void validate(PostSearchOptions options) { } private static final class ViewOptionsHandler - extends OptionsHandler { + extends KeyOptionsHandler { private ViewOptionsHandler() { super(PostViewOptions.Builder::build, PostViewOptions::newBuilder); } + @Override + Function limitGetter() { + return PostViewOptions::limit; + } + + @Override + BiFunction limitSetter() { + return PostViewOptions.Builder::limit; + } + @Override void validate(PostViewOptions options) { validateLimit(options::limit); diff --git a/modules/cloudant/src/main/java/com/ibm/cloud/cloudant/features/pagination/SearchPageIterator.java b/modules/cloudant/src/main/java/com/ibm/cloud/cloudant/features/pagination/SearchPageIterator.java index c7ac7a858..ad41d31fd 100644 --- a/modules/cloudant/src/main/java/com/ibm/cloud/cloudant/features/pagination/SearchPageIterator.java +++ b/modules/cloudant/src/main/java/com/ibm/cloud/cloudant/features/pagination/SearchPageIterator.java @@ -14,7 +14,6 @@ package com.ibm.cloud.cloudant.features.pagination; import java.util.function.BiFunction; -import java.util.function.Function; import com.ibm.cloud.cloudant.v1.Cloudant; import com.ibm.cloud.cloudant.v1.model.PostSearchOptions; import com.ibm.cloud.cloudant.v1.model.PostSearchOptions.Builder; @@ -27,16 +26,6 @@ final class SearchPageIterator extends SearchBasePageIterator optionsToBuilderFunction() { - return PostSearchOptions::newBuilder; - } - - @Override - Function builderToOptionsFunction() { - return Builder::build; - } - @Override BiFunction bookmarkSetter() { return Builder::bookmark; @@ -47,9 +36,4 @@ BiFunction> nextRequestFu return Cloudant::postSearch; } - @Override - Function limitGetter() { - return PostSearchOptions::limit; - } - } diff --git a/modules/cloudant/src/main/java/com/ibm/cloud/cloudant/features/pagination/SearchPartitionPageIterator.java b/modules/cloudant/src/main/java/com/ibm/cloud/cloudant/features/pagination/SearchPartitionPageIterator.java index 6b599d734..4c77ccfce 100644 --- a/modules/cloudant/src/main/java/com/ibm/cloud/cloudant/features/pagination/SearchPartitionPageIterator.java +++ b/modules/cloudant/src/main/java/com/ibm/cloud/cloudant/features/pagination/SearchPartitionPageIterator.java @@ -14,7 +14,6 @@ package com.ibm.cloud.cloudant.features.pagination; import java.util.function.BiFunction; -import java.util.function.Function; import com.ibm.cloud.cloudant.v1.Cloudant; import com.ibm.cloud.cloudant.v1.model.PostPartitionSearchOptions; import com.ibm.cloud.cloudant.v1.model.PostPartitionSearchOptions.Builder; @@ -27,16 +26,6 @@ final class SearchPartitionPageIterator extends SearchBasePageIterator optionsToBuilderFunction() { - return PostPartitionSearchOptions::newBuilder; - } - - @Override - Function builderToOptionsFunction() { - return Builder::build; - } - @Override BiFunction bookmarkSetter() { return Builder::bookmark; @@ -47,9 +36,4 @@ BiFunction> next return Cloudant::postPartitionSearch; } - @Override - Function limitGetter() { - return PostPartitionSearchOptions::limit; - } - } diff --git a/modules/cloudant/src/main/java/com/ibm/cloud/cloudant/features/pagination/ViewPageIterator.java b/modules/cloudant/src/main/java/com/ibm/cloud/cloudant/features/pagination/ViewPageIterator.java index 2bb58f78e..284942165 100644 --- a/modules/cloudant/src/main/java/com/ibm/cloud/cloudant/features/pagination/ViewPageIterator.java +++ b/modules/cloudant/src/main/java/com/ibm/cloud/cloudant/features/pagination/ViewPageIterator.java @@ -15,7 +15,6 @@ import java.util.Optional; import java.util.function.BiFunction; -import java.util.function.Function; import com.ibm.cloud.cloudant.v1.Cloudant; import com.ibm.cloud.cloudant.v1.model.PostViewOptions; import com.ibm.cloud.cloudant.v1.model.PostViewOptions.Builder; @@ -28,16 +27,6 @@ final class ViewPageIterator extends ViewBasePageIterator optionsToBuilderFunction() { - return PostViewOptions::newBuilder; - } - - @Override - Function builderToOptionsFunction() { - return Builder::build; - } - @Override BiFunction> nextRequestFunction() { return Cloudant::postView; @@ -53,9 +42,4 @@ Optional> nextKeyIdSetter() { return Optional.of(Builder::startKeyDocId); } - @Override - Function limitGetter() { - return PostViewOptions::limit; - } - } diff --git a/modules/cloudant/src/main/java/com/ibm/cloud/cloudant/features/pagination/ViewPartitionPageIterator.java b/modules/cloudant/src/main/java/com/ibm/cloud/cloudant/features/pagination/ViewPartitionPageIterator.java index 3663aa957..c434a6687 100644 --- a/modules/cloudant/src/main/java/com/ibm/cloud/cloudant/features/pagination/ViewPartitionPageIterator.java +++ b/modules/cloudant/src/main/java/com/ibm/cloud/cloudant/features/pagination/ViewPartitionPageIterator.java @@ -15,7 +15,6 @@ import java.util.Optional; import java.util.function.BiFunction; -import java.util.function.Function; import com.ibm.cloud.cloudant.v1.Cloudant; import com.ibm.cloud.cloudant.v1.model.PostPartitionViewOptions; import com.ibm.cloud.cloudant.v1.model.PostPartitionViewOptions.Builder; @@ -28,16 +27,6 @@ final class ViewPartitionPageIterator extends ViewBasePageIterator optionsToBuilderFunction() { - return PostPartitionViewOptions::newBuilder; - } - - @Override - Function builderToOptionsFunction() { - return Builder::build; - } - @Override BiFunction> nextRequestFunction() { return Cloudant::postPartitionView; @@ -53,9 +42,4 @@ Optional> nextKeyIdSetter() { return Optional.of(Builder::startKeyDocId); } - @Override - Function limitGetter() { - return PostPartitionViewOptions::limit; - } - } diff --git a/modules/cloudant/src/test/java/com/ibm/cloud/cloudant/features/pagination/BasePageIteratorTest.java b/modules/cloudant/src/test/java/com/ibm/cloud/cloudant/features/pagination/BasePageIteratorTest.java index 3d8e6e5b4..fa501385f 100644 --- a/modules/cloudant/src/test/java/com/ibm/cloud/cloudant/features/pagination/BasePageIteratorTest.java +++ b/modules/cloudant/src/test/java/com/ibm/cloud/cloudant/features/pagination/BasePageIteratorTest.java @@ -59,15 +59,6 @@ Cloudant getClient() { * Implicitly tests that limit gets applied per page, otherwise the default would be used and * page counts would be wrong. */ - @Override - protected Function optionsToBuilderFunction() { - return PostViewOptions::newBuilder; - } - - @Override - protected Function builderToOptionsFunction() { - return Builder::build; - } @Override protected Function> itemsGetter() { @@ -100,11 +91,6 @@ protected BiFunction> nextReq }; } - @Override - protected Function limitGetter() { - return PostViewOptions::limit; - } - } private static class ThrowingTestPager extends TestPager { diff --git a/modules/cloudant/src/test/java/com/ibm/cloud/cloudant/features/pagination/BookmarkPageIteratorTest.java b/modules/cloudant/src/test/java/com/ibm/cloud/cloudant/features/pagination/BookmarkPageIteratorTest.java index 5de76065b..3e129c475 100644 --- a/modules/cloudant/src/test/java/com/ibm/cloud/cloudant/features/pagination/BookmarkPageIteratorTest.java +++ b/modules/cloudant/src/test/java/com/ibm/cloud/cloudant/features/pagination/BookmarkPageIteratorTest.java @@ -59,21 +59,6 @@ protected BiFunction> nextReq }; } - @Override - protected Function limitGetter() { - return PostFindOptions::limit; - } - - @Override - protected Function optionsToBuilderFunction() { - return PostFindOptions::newBuilder; - } - - @Override - protected Function builderToOptionsFunction() { - return Builder::build; - } - @Override protected Function> itemsGetter() { return TestResult::getRows; diff --git a/modules/cloudant/src/test/java/com/ibm/cloud/cloudant/features/pagination/KeyPageIteratorTest.java b/modules/cloudant/src/test/java/com/ibm/cloud/cloudant/features/pagination/KeyPageIteratorTest.java index da3b6b5cc..77bf8c92a 100644 --- a/modules/cloudant/src/test/java/com/ibm/cloud/cloudant/features/pagination/KeyPageIteratorTest.java +++ b/modules/cloudant/src/test/java/com/ibm/cloud/cloudant/features/pagination/KeyPageIteratorTest.java @@ -62,11 +62,6 @@ protected BiFunction> nextReq }; } - @Override - protected Function limitGetter() { - return PostViewOptions::limit; - } - @Override protected BiFunction nextKeySetter() { return Builder::startKey; @@ -87,16 +82,6 @@ protected Function nextKeyIdGetter() { return String::valueOf; } - @Override - protected Function optionsToBuilderFunction() { - return PostViewOptions::newBuilder; - } - - @Override - protected Function builderToOptionsFunction() { - return Builder::build; - } - @Override protected Function> itemsGetter() { return TestResult::getRows; @@ -113,18 +98,25 @@ Optional checkBoundary(Integer penultimateItem, Integer lastItem) { @Test void testDefaultPageSize() { TestKeyPager pager = new TestKeyPager(mockClient, getRequiredTestOptionsBuilder().build()); + int expectedPageSize = Math.toIntExact(OptionsHandler.MAX_LIMIT + 1); // Assert the default limit as page size - Assert.assertEquals(pager.pageSize, 201, + Assert.assertEquals(pager.pageSize, expectedPageSize, "The page size should be one more than the default limit."); + Assert.assertEquals(pager.nextPageOptionsRef.get().limit(), expectedPageSize, + "The request limit should equal the page size."); } // Test page size limit (+1) @Test void testLimitPageSize() { - TestKeyPager pager = new TestKeyPager(mockClient, getDefaultTestOptions(42)); + int testPageSize = 42; + TestKeyPager pager = new TestKeyPager(mockClient, getDefaultTestOptions(testPageSize)); + int expectedPageSize = testPageSize + 1; // Assert the limit provided as page size Assert.assertEquals(pager.pageSize, 43, "The page size should be one more than the supplied limit."); + Assert.assertEquals(pager.nextPageOptionsRef.get().limit(), expectedPageSize, + "The request limit should equal the page size."); } // Test all items on page when no more pages From 728b65cf8966d89e7f6c032595d71ad0aeda6bbf Mon Sep 17 00:00:00 2001 From: Rich Ellis Date: Thu, 3 Jul 2025 12:19:01 +0100 Subject: [PATCH 3/6] fix: NPEs for reduced/grouped rows with no ID --- .../features/pagination/ViewBasePageIterator.java | 12 +++++------- 1 file changed, 5 insertions(+), 7 deletions(-) diff --git a/modules/cloudant/src/main/java/com/ibm/cloud/cloudant/features/pagination/ViewBasePageIterator.java b/modules/cloudant/src/main/java/com/ibm/cloud/cloudant/features/pagination/ViewBasePageIterator.java index ab15baa2d..be44480de 100644 --- a/modules/cloudant/src/main/java/com/ibm/cloud/cloudant/features/pagination/ViewBasePageIterator.java +++ b/modules/cloudant/src/main/java/com/ibm/cloud/cloudant/features/pagination/ViewBasePageIterator.java @@ -43,15 +43,13 @@ final Function nextKeyIdGetter() { @Override final Optional checkBoundary(ViewResultRow penultimateItem, ViewResultRow lastItem) { - String pId = penultimateItem.getId(); - String lId = lastItem.getId(); + Optional pId = Optional.ofNullable(penultimateItem.getId()); + Optional lId = Optional.ofNullable(lastItem.getId()); if (pId.equals(lId)) { // ID's are the same, check the keys - Object pKey = penultimateItem.getKey(); - Object lKey = lastItem.getKey(); - // Check reference equality first (e.g. null) - // Then check values - if (pKey == lKey || pKey != null && pKey.equals(lKey)) { + Optional pKey = Optional.ofNullable(penultimateItem.getKey()); + Optional lKey = Optional.ofNullable(lastItem.getKey()); + if (pKey.equals(lKey)) { // Identical keys, set an error message return Optional.of( String.format( From b9fe12aa6357943720871cf32a2de13cdb7f406c Mon Sep 17 00:00:00 2001 From: Rich Ellis Date: Thu, 3 Jul 2025 17:52:16 +0100 Subject: [PATCH 4/6] test: fix option provider --- .../cloudant/features/pagination/PaginationTestHelpers.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/modules/cloudant/src/test/java/com/ibm/cloud/cloudant/features/pagination/PaginationTestHelpers.java b/modules/cloudant/src/test/java/com/ibm/cloud/cloudant/features/pagination/PaginationTestHelpers.java index fee039864..f1624ff88 100644 --- a/modules/cloudant/src/test/java/com/ibm/cloud/cloudant/features/pagination/PaginationTestHelpers.java +++ b/modules/cloudant/src/test/java/com/ibm/cloud/cloudant/features/pagination/PaginationTestHelpers.java @@ -431,7 +431,7 @@ public Iterator viewLikeOptions() { @DataProvider(name = "viewOptions") public Iterator viewOptions() { - return getIteratorFor(viewLikeOptions); + return getIteratorFor(viewOptions); } static class TestDesignDocsResultRow extends DocsResultRow { From 9cef070ec2ec7d86de5cfa2f3b7366231a5f105e Mon Sep 17 00:00:00 2001 From: Rich Ellis Date: Thu, 3 Jul 2025 17:53:01 +0100 Subject: [PATCH 5/6] fix: pagination view option handling --- .../features/pagination/OptionsHandler.java | 317 +++++++----------- .../pagination/OptionsValidationTest.java | 33 ++ 2 files changed, 155 insertions(+), 195 deletions(-) diff --git a/modules/cloudant/src/main/java/com/ibm/cloud/cloudant/features/pagination/OptionsHandler.java b/modules/cloudant/src/main/java/com/ibm/cloud/cloudant/features/pagination/OptionsHandler.java index 85d623966..6e049ce81 100644 --- a/modules/cloudant/src/main/java/com/ibm/cloud/cloudant/features/pagination/OptionsHandler.java +++ b/modules/cloudant/src/main/java/com/ibm/cloud/cloudant/features/pagination/OptionsHandler.java @@ -15,11 +15,11 @@ import java.util.Collections; import java.util.HashMap; +import java.util.List; import java.util.Map; import java.util.Optional; import java.util.function.BiFunction; import java.util.function.Function; -import java.util.function.Supplier; import com.ibm.cloud.cloudant.v1.model.PostAllDocsOptions; import com.ibm.cloud.cloudant.v1.model.PostDesignDocsOptions; import com.ibm.cloud.cloudant.v1.model.PostFindOptions; @@ -57,10 +57,15 @@ abstract class OptionsHandler { private final Function builderToOptions; private final Function optionsToBuilder; + private final Function limitGetter; + private final BiFunction limitSetter; - private OptionsHandler(Function builderToOptions, Function optionsToBuilder) { + private OptionsHandler(Function builderToOptions, Function optionsToBuilder, + Function limitGetter, BiFunction limitSetter) { this.builderToOptions = builderToOptions; this.optionsToBuilder = optionsToBuilder; + this.limitGetter = limitGetter; + this.limitSetter = limitSetter; } B builderFromOptions(O options) { @@ -71,65 +76,63 @@ O optionsFromBuilder(B builder) { return this.builderToOptions.apply(builder); } - abstract void validate(O options); + protected void validate(O options) { + validateLimit(options); + } - private O clone(O options) { + private O copy(O options) { return this.optionsFromBuilder(this.builderFromOptions(options)); } B applyLimit(B builder, Long newLimit) { - return this.limitSetter().apply(builder, newLimit); + return this.limitSetter.apply(builder, newLimit); } - abstract BiFunction limitSetter(); - - abstract Function limitGetter(); - Long getPageSizeFromOptionsLimit(O opts) { - return Optional.ofNullable(limitGetter().apply(opts)).orElse(MAX_LIMIT); + return Optional.ofNullable(this.limitGetter.apply(opts)).orElse(MAX_LIMIT); } static final PostAllDocsOptions duplicate(PostAllDocsOptions opts) { - return POST_ALL_DOCS.clone(opts); + return POST_ALL_DOCS.copy(opts); } static final PostDesignDocsOptions duplicate(PostDesignDocsOptions opts) { - return POST_DESIGN_DOCS.clone(opts); + return POST_DESIGN_DOCS.copy(opts); } static final PostFindOptions duplicate(PostFindOptions opts) { - return POST_FIND.clone(opts); + return POST_FIND.copy(opts); } static final PostPartitionAllDocsOptions duplicate(PostPartitionAllDocsOptions opts) { - return POST_PARTITION_ALL_DOCS.clone(opts); + return POST_PARTITION_ALL_DOCS.copy(opts); } static final PostPartitionFindOptions duplicate(PostPartitionFindOptions opts) { - return POST_PARTITION_FIND.clone(opts); + return POST_PARTITION_FIND.copy(opts); } static final PostPartitionSearchOptions duplicate(PostPartitionSearchOptions opts) { - return POST_PARTITION_SEARCH.clone(opts); + return POST_PARTITION_SEARCH.copy(opts); } static final PostPartitionViewOptions duplicate(PostPartitionViewOptions opts) { - return POST_PARTITION_VIEW.clone(opts); + return POST_PARTITION_VIEW.copy(opts); } static final PostSearchOptions duplicate(PostSearchOptions opts) { - return POST_SEARCH.clone(opts); + return POST_SEARCH.copy(opts); } static final PostViewOptions duplicate(PostViewOptions opts) { - return POST_VIEW.clone(opts); + return POST_VIEW.copy(opts); } - private static void validateLimit(Supplier limitSupplier) { + protected void validateLimit(O opts) { // If limit is set check it is within range // Else it is unset and we will set the valid default value later - if (optionIsPresent(limitSupplier)) { - Long limit = limitSupplier.get(); + if (optionIsPresent(opts, this.limitGetter)) { + Long limit = this.limitGetter.apply(opts); if (limit > MAX_LIMIT) { throw new IllegalArgumentException(String.format( "The provided limit %d exceeds the maximum page size value of %d.", limit, MAX_LIMIT)); @@ -142,24 +145,48 @@ private static void validateLimit(Supplier limitSupplier) { } } - private static boolean optionIsPresent(final Supplier optionSupplier) { - return Optional.ofNullable(optionSupplier.get()).isPresent(); + protected Optional getOptionalOption(final O opts, final Function optionGetter) { + return Optional.ofNullable(optionGetter.apply(opts)); } - private static void validateOptionsAbsent(final Map> options) { - for (Map.Entry> option : options.entrySet()) { - if (optionIsPresent(option.getValue())) { + protected boolean optionIsPresent(final O opts, final Function optionGetter) { + return getOptionalOption(opts, optionGetter).isPresent(); + } + + protected void validateOptionsAbsent(final O opts, + final Map> optionGetters, final String messageReason) { + for (Map.Entry> optionGetter : optionGetters.entrySet()) { + if (optionIsPresent(opts, optionGetter.getValue())) { throw new IllegalArgumentException( - String.format("The option '%s' is invalid when using pagination.", option.getKey())); + String.format("The option '%s' is invalid %s", optionGetter.getKey(), messageReason)); } } } - private abstract static class KeyOptionsHandler - extends OptionsHandler { + protected void validateOptionsAbsent(final O options, + final Map> optionGetters) { + validateOptionsAbsent(options, optionGetters, "when using pagination."); + } + + private abstract static class KeyOptionsHandler extends OptionsHandler { + + protected final Function keyGetter; + private final Function> keysGetter; + private final String keyErrorMsg = + keyErrorMessage(new StringBuilder("when using pagination. ")); + + protected KeyOptionsHandler(Function builderToOptions, Function optionsToBuilder, + Function limitGetter, BiFunction limitSetter, Function keyGetter, + Function> keysGetter) { + super(builderToOptions, optionsToBuilder, limitGetter, limitSetter); + this.keyGetter = keyGetter; + this.keysGetter = keysGetter; + } - KeyOptionsHandler(Function builderToOptions, Function optionsToBuilder) { - super(builderToOptions, optionsToBuilder); + protected String keyErrorMessage(StringBuilder baseMessage) { + // Default for all docs style, override for views + return baseMessage.append("No need to paginate as 'key' returns a single result for an ID.") + .toString(); } @Override @@ -167,63 +194,57 @@ Long getPageSizeFromOptionsLimit(O opts) { return super.getPageSizeFromOptionsLimit(opts) + 1; } - } - - private abstract static class BookmarkOptionsHandler - extends OptionsHandler { - - BookmarkOptionsHandler(Function builderToOptions, Function optionsToBuilder) { - super(builderToOptions, optionsToBuilder); + @Override + protected void validate(O options) { + validateOptionsAbsent(options, Collections.singletonMap("keys", this.keysGetter)); + validateOptionsAbsent(options, Collections.singletonMap("key", this.keyGetter), + this.keyErrorMsg); + super.validate(options); } - } - private static final class AllDocsOptionsHandler - extends KeyOptionsHandler { + private abstract static class ViewsOptionsHandler extends KeyOptionsHandler { - private AllDocsOptionsHandler() { - super(PostAllDocsOptions.Builder::build, PostAllDocsOptions::newBuilder); + protected ViewsOptionsHandler(Function builderToOptions, Function optionsToBuilder, + Function limitGetter, BiFunction limitSetter, + Function keyGetter, Function> keysGetter) { + super(builderToOptions, optionsToBuilder, limitGetter, limitSetter, keyGetter, keysGetter); } @Override - Function limitGetter() { - return PostAllDocsOptions::limit; + protected String keyErrorMessage(StringBuilder baseMessage) { + return baseMessage.append("Use startKey and endKey instead.").toString(); } - @Override - BiFunction limitSetter() { - return PostAllDocsOptions.Builder::limit; - } + } - @Override - void validate(PostAllDocsOptions options) { - validateLimit(options::limit); - validateOptionsAbsent(Collections.singletonMap("keys", options::keys)); + private abstract static class BookmarkOptionsHandler extends OptionsHandler { + + BookmarkOptionsHandler(Function builderToOptions, Function optionsToBuilder, + Function limitGetter, BiFunction limitSetter) { + super(builderToOptions, optionsToBuilder, limitGetter, limitSetter); } } - private static final class DesignDocsOptionsHandler - extends KeyOptionsHandler { + private static final class AllDocsOptionsHandler + extends KeyOptionsHandler { - private DesignDocsOptionsHandler() { - super(PostDesignDocsOptions.Builder::build, PostDesignDocsOptions::newBuilder); + private AllDocsOptionsHandler() { + super(PostAllDocsOptions.Builder::build, PostAllDocsOptions::newBuilder, + PostAllDocsOptions::limit, PostAllDocsOptions.Builder::limit, PostAllDocsOptions::key, + PostAllDocsOptions::keys); } - @Override - Function limitGetter() { - return PostDesignDocsOptions::limit; - } + } - @Override - BiFunction limitSetter() { - return PostDesignDocsOptions.Builder::limit; - } + private static final class DesignDocsOptionsHandler + extends KeyOptionsHandler { - @Override - void validate(PostDesignDocsOptions options) { - validateLimit(options::limit); - validateOptionsAbsent(Collections.singletonMap("keys", options::keys)); + private DesignDocsOptionsHandler() { + super(PostDesignDocsOptions.Builder::build, PostDesignDocsOptions::newBuilder, + PostDesignDocsOptions::limit, PostDesignDocsOptions.Builder::limit, + PostDesignDocsOptions::key, PostDesignDocsOptions::keys); } } @@ -232,47 +253,19 @@ private static final class FindOptionsHandler extends BookmarkOptionsHandler { private FindOptionsHandler() { - super(PostFindOptions.Builder::build, PostFindOptions::newBuilder); - } - - @Override - Function limitGetter() { - return PostFindOptions::limit; - } - - @Override - BiFunction limitSetter() { - return PostFindOptions.Builder::limit; - } - - @Override - void validate(PostFindOptions options) { - validateLimit(options::limit); + super(PostFindOptions.Builder::build, PostFindOptions::newBuilder, PostFindOptions::limit, + PostFindOptions.Builder::limit); } } - private static final class PartitionAllDocsOptionsHandler - extends KeyOptionsHandler { + private static final class PartitionAllDocsOptionsHandler extends + KeyOptionsHandler { private PartitionAllDocsOptionsHandler() { - super(PostPartitionAllDocsOptions.Builder::build, PostPartitionAllDocsOptions::newBuilder); - } - - @Override - Function limitGetter() { - return PostPartitionAllDocsOptions::limit; - } - - @Override - BiFunction limitSetter() { - return PostPartitionAllDocsOptions.Builder::limit; - } - - @Override - void validate(PostPartitionAllDocsOptions options) { - validateLimit(options::limit); - validateOptionsAbsent(Collections.singletonMap("keys", options::keys)); + super(PostPartitionAllDocsOptions.Builder::build, PostPartitionAllDocsOptions::newBuilder, + PostPartitionAllDocsOptions::limit, PostPartitionAllDocsOptions.Builder::limit, + PostPartitionAllDocsOptions::key, PostPartitionAllDocsOptions::keys); } } @@ -281,71 +274,29 @@ private static final class PartitionFindOptionsHandler extends BookmarkOptionsHandler { private PartitionFindOptionsHandler() { - super(PostPartitionFindOptions.Builder::build, PostPartitionFindOptions::newBuilder); - } - - @Override - Function limitGetter() { - return PostPartitionFindOptions::limit; - } - - @Override - BiFunction limitSetter() { - return PostPartitionFindOptions.Builder::limit; - } - - @Override - void validate(PostPartitionFindOptions options) { - validateLimit(options::limit); + super(PostPartitionFindOptions.Builder::build, PostPartitionFindOptions::newBuilder, + PostPartitionFindOptions::limit, PostPartitionFindOptions.Builder::limit); } } - private static final class PartitionSearchOptionsHandler - extends BookmarkOptionsHandler { + private static final class PartitionSearchOptionsHandler extends + BookmarkOptionsHandler { private PartitionSearchOptionsHandler() { - super(PostPartitionSearchOptions.Builder::build, PostPartitionSearchOptions::newBuilder); - } - - @Override - Function limitGetter() { - return PostPartitionSearchOptions::limit; - } - - @Override - BiFunction limitSetter() { - return PostPartitionSearchOptions.Builder::limit; - } - - @Override - void validate(PostPartitionSearchOptions options) { - validateLimit(options::limit); + super(PostPartitionSearchOptions.Builder::build, PostPartitionSearchOptions::newBuilder, + PostPartitionSearchOptions::limit, PostPartitionSearchOptions.Builder::limit); } } private static final class PartitionViewOptionsHandler - extends KeyOptionsHandler { + extends ViewsOptionsHandler { private PartitionViewOptionsHandler() { - super(PostPartitionViewOptions.Builder::build, PostPartitionViewOptions::newBuilder); - } - - @Override - Function limitGetter() { - return PostPartitionViewOptions::limit; - } - - @Override - BiFunction limitSetter() { - return PostPartitionViewOptions.Builder::limit; - } - - @Override - void validate(PostPartitionViewOptions options) { - validateLimit(options::limit); - validateOptionsAbsent(Collections.singletonMap("keys", options::keys)); + super(PostPartitionViewOptions.Builder::build, PostPartitionViewOptions::newBuilder, + PostPartitionViewOptions::limit, PostPartitionViewOptions.Builder::limit, + PostPartitionViewOptions::key, PostPartitionViewOptions::keys); } } @@ -354,54 +305,30 @@ private static final class SearchOptionsHandler extends BookmarkOptionsHandler { private SearchOptionsHandler() { - super(PostSearchOptions.Builder::build, PostSearchOptions::newBuilder); - } - - @Override - Function limitGetter() { - return PostSearchOptions::limit; + super(PostSearchOptions.Builder::build, PostSearchOptions::newBuilder, + PostSearchOptions::limit, PostSearchOptions.Builder::limit); } @Override - BiFunction limitSetter() { - return PostSearchOptions.Builder::limit; - } - - @Override - void validate(PostSearchOptions options) { - validateLimit(options::limit); - Map> invalidOptions = new HashMap<>(5); - invalidOptions.put("counts", options::counts); - invalidOptions.put("groupField", options::groupField); - invalidOptions.put("groupLimit", options::groupLimit); - invalidOptions.put("groupSort", options::groupSort); - invalidOptions.put("ranges", options::ranges); - validateOptionsAbsent(invalidOptions); + protected void validate(PostSearchOptions options) { + Map> invalidOptions = new HashMap<>(5, 1); + invalidOptions.put("counts", PostSearchOptions::counts); + invalidOptions.put("groupField", PostSearchOptions::groupField); + invalidOptions.put("groupLimit", PostSearchOptions::groupLimit); + invalidOptions.put("groupSort", PostSearchOptions::groupSort); + invalidOptions.put("ranges", PostSearchOptions::ranges); + validateOptionsAbsent(options, invalidOptions); + super.validate(options); } } private static final class ViewOptionsHandler - extends KeyOptionsHandler { + extends ViewsOptionsHandler { private ViewOptionsHandler() { - super(PostViewOptions.Builder::build, PostViewOptions::newBuilder); - } - - @Override - Function limitGetter() { - return PostViewOptions::limit; - } - - @Override - BiFunction limitSetter() { - return PostViewOptions.Builder::limit; - } - - @Override - void validate(PostViewOptions options) { - validateLimit(options::limit); - validateOptionsAbsent(Collections.singletonMap("keys", options::keys)); + super(PostViewOptions.Builder::build, PostViewOptions::newBuilder, PostViewOptions::limit, + PostViewOptions.Builder::limit, PostViewOptions::key, PostViewOptions::keys); } } diff --git a/modules/cloudant/src/test/java/com/ibm/cloud/cloudant/features/pagination/OptionsValidationTest.java b/modules/cloudant/src/test/java/com/ibm/cloud/cloudant/features/pagination/OptionsValidationTest.java index 8484721f2..f2ebd1450 100644 --- a/modules/cloudant/src/test/java/com/ibm/cloud/cloudant/features/pagination/OptionsValidationTest.java +++ b/modules/cloudant/src/test/java/com/ibm/cloud/cloudant/features/pagination/OptionsValidationTest.java @@ -115,4 +115,37 @@ public void testValidationExceptionForFacetedSearch(String optionName, Object op }); } + @Test(dataProvider = "allDocsOptions", dataProviderClass = PaginationTestHelpers.class) + public void testValidationExceptionForAllDocsKey(OptionsProvider provider) + throws Exception { + provider.setRequiredOpts(); + provider.set("key", "foo"); + Assert.assertThrows("There should be a validation exception", IllegalArgumentException.class, + () -> { + try { + provider.handler.validate(provider.build()); + } catch (IllegalArgumentException e) { + Assert.assertTrue(e.getMessage().endsWith( + "The option 'key' is invalid when using pagination. No need to paginate as 'key' returns a single result for an ID.")); + throw e; + } + }); + } + + @Test(dataProvider = "viewOptions", dataProviderClass = PaginationTestHelpers.class) + public void testValidationExceptionForViewsKey(OptionsProvider provider) + throws Exception { + provider.setRequiredOpts(); + provider.set("key", new Object()); + Assert.assertThrows("There should be a validation exception", IllegalArgumentException.class, + () -> { + try { + provider.handler.validate(provider.build()); + } catch (IllegalArgumentException e) { + Assert.assertTrue(e.getMessage().endsWith( + "The option 'key' is invalid when using pagination. Use startKey and endKey instead.")); + throw e; + } + }); + } } From e8b3a4d93c75693382350584b3efc3ead18d5776 Mon Sep 17 00:00:00 2001 From: Rich Ellis Date: Fri, 4 Jul 2025 12:13:22 +0100 Subject: [PATCH 6/6] fix: remove skip from subsequent pages --- .../features/pagination/BasePageIterator.java | 3 + .../features/pagination/OptionsHandler.java | 122 ++++++++++++++++-- .../pagination/BookmarkPageIteratorTest.java | 23 +++- .../pagination/KeyPageIteratorTest.java | 22 +++- 4 files changed, 160 insertions(+), 10 deletions(-) diff --git a/modules/cloudant/src/main/java/com/ibm/cloud/cloudant/features/pagination/BasePageIterator.java b/modules/cloudant/src/main/java/com/ibm/cloud/cloudant/features/pagination/BasePageIterator.java index 091170c31..05d413719 100644 --- a/modules/cloudant/src/main/java/com/ibm/cloud/cloudant/features/pagination/BasePageIterator.java +++ b/modules/cloudant/src/main/java/com/ibm/cloud/cloudant/features/pagination/BasePageIterator.java @@ -63,8 +63,11 @@ List 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; diff --git a/modules/cloudant/src/main/java/com/ibm/cloud/cloudant/features/pagination/OptionsHandler.java b/modules/cloudant/src/main/java/com/ibm/cloud/cloudant/features/pagination/OptionsHandler.java index 6e049ce81..96ae005e7 100644 --- a/modules/cloudant/src/main/java/com/ibm/cloud/cloudant/features/pagination/OptionsHandler.java +++ b/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 extends OptionsHandler< protected final Function keyGetter; private final Function> keysGetter; + protected final Function skipGetter; private final String keyErrorMsg = keyErrorMessage(new StringBuilder("when using pagination. ")); protected KeyOptionsHandler(Function builderToOptions, Function optionsToBuilder, Function limitGetter, BiFunction limitSetter, Function keyGetter, - Function> keysGetter) { + Function> keysGetter, Function 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 extends KeyOptionsHandler { protected ViewsOptionsHandler(Function builderToOptions, Function optionsToBuilder, Function limitGetter, BiFunction limitSetter, - Function keyGetter, Function> keysGetter) { - super(builderToOptions, optionsToBuilder, limitGetter, limitSetter, keyGetter, keysGetter); + Function keyGetter, Function> keysGetter, + Function 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,7 +274,17 @@ 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(); } } @@ -252,11 +292,27 @@ private DesignDocsOptionsHandler() { private static final class FindOptionsHandler extends BookmarkOptionsHandler { + private final Function 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,7 +321,18 @@ 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(); } } @@ -273,11 +340,28 @@ private PartitionAllDocsOptionsHandler() { private static final class PartitionFindOptionsHandler extends BookmarkOptionsHandler { + private final Function 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(); } } diff --git a/modules/cloudant/src/test/java/com/ibm/cloud/cloudant/features/pagination/BookmarkPageIteratorTest.java b/modules/cloudant/src/test/java/com/ibm/cloud/cloudant/features/pagination/BookmarkPageIteratorTest.java index 3e129c475..a04740b0f 100644 --- a/modules/cloudant/src/test/java/com/ibm/cloud/cloudant/features/pagination/BookmarkPageIteratorTest.java +++ b/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 { + class TestBookmarkPager + extends BookmarkPageIterator { 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 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(); + } + } diff --git a/modules/cloudant/src/test/java/com/ibm/cloud/cloudant/features/pagination/KeyPageIteratorTest.java b/modules/cloudant/src/test/java/com/ibm/cloud/cloudant/features/pagination/KeyPageIteratorTest.java index 77bf8c92a..2c81c0acf 100644 --- a/modules/cloudant/src/test/java/com/ibm/cloud/cloudant/features/pagination/KeyPageIteratorTest.java +++ b/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 { + class TestKeyPager + extends KeyPageIterator { protected TestKeyPager(Cloudant client, PostViewOptions options) { super(client, options, OptionsHandler.POST_VIEW); @@ -289,4 +290,23 @@ Optional checkBoundary(Integer penultimateItem, Integer lastItem) { Assert.assertFalse(pager.hasNext(), "hasNext() should return false."); } + @Test + void testSkipRemovedForSubsequentPages() { + int pageSize = 3; + long expectedSkip = 17; + PageSupplier 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(); + } + }