From 6e256255814cbeaf458b5e5bcf43366939dc56ed Mon Sep 17 00:00:00 2001
From: Carlos O'Ryan <coryan@google.com>
Date: Wed, 21 Jun 2023 21:15:46 +0000
Subject: [PATCH 1/2] fix(common): pagination must support empty pages

Some services return empty pages with a non-empty pagination token. Our
code would stop such iterations immediately. The correct behavior is to
retrieve the next page until some non-empty range is returned.
---
 google/cloud/internal/pagination_range.h      |  7 +--
 .../cloud/internal/pagination_range_test.cc   | 57 +++++++++++++++++++
 2 files changed, 60 insertions(+), 4 deletions(-)

diff --git a/google/cloud/internal/pagination_range.h b/google/cloud/internal/pagination_range.h
index 6608e04ada5bb..5810aa485893b 100644
--- a/google/cloud/internal/pagination_range.h
+++ b/google/cloud/internal/pagination_range.h
@@ -93,8 +93,7 @@ class PagedStreamReader {
    *   successful end of stream.
    */
   typename StreamReader<T>::result_type GetNext() {
-    if (current_ == page_.end()) {
-      if (last_page_) return Status{};
+    while (current_ == page_.end() && !last_page_) {
       request_.set_page_token(std::move(token_));
       auto response = loader_(request_);
       if (!response.ok()) return std::move(response).status();
@@ -102,9 +101,9 @@ class PagedStreamReader {
       if (token_.empty()) last_page_ = true;
       page_ = extractor_(*std::move(response));
       current_ = page_.begin();
-      if (current_ == page_.end()) return Status{};
     }
-    return std::move(*current_++);
+    if (current_ != page_.end()) return std::move(*current_++);
+    return Status{};
   }
 
  private:
diff --git a/google/cloud/internal/pagination_range_test.cc b/google/cloud/internal/pagination_range_test.cc
index 5dc1a54c7f334..7241e9134521f 100644
--- a/google/cloud/internal/pagination_range_test.cc
+++ b/google/cloud/internal/pagination_range_test.cc
@@ -197,6 +197,63 @@ TYPED_TEST(PaginationRangeTest, TwoPages) {
   EXPECT_THAT(names, ElementsAre("p1", "p2", "p3", "p4"));
 }
 
+TYPED_TEST(PaginationRangeTest, EmptyAndFullPages) {
+  using ResponseType = TypeParam;
+  MockRpc<ResponseType> mock;
+  EXPECT_CALL(mock, Loader)
+      .WillOnce([](Request const& request) {
+        EXPECT_EQ(CurrentOptions().get<StringOption>(), "EmptyAndFullPages");
+        EXPECT_TRUE(request.testonly_page_token.empty());
+        ResponseType response;
+        response.testonly_set_page_token("t1");
+        return response;
+      })
+      .WillOnce([](Request const& request) {
+        EXPECT_EQ(CurrentOptions().get<StringOption>(), "EmptyAndFullPages");
+        EXPECT_EQ("t1", request.testonly_page_token);
+        ResponseType response;
+        response.testonly_set_page_token("t2");
+        return response;
+      })
+      .WillOnce([](Request const& request) {
+        EXPECT_EQ(CurrentOptions().get<StringOption>(), "EmptyAndFullPages");
+        EXPECT_EQ("t2", request.testonly_page_token);
+        ResponseType response;
+        response.testonly_items.push_back(Item{"p1"});
+        response.testonly_items.push_back(Item{"p2"});
+        response.testonly_set_page_token("t3");
+        return response;
+      })
+      .WillOnce([](Request const& request) {
+        EXPECT_EQ(CurrentOptions().get<StringOption>(), "EmptyAndFullPages");
+        EXPECT_EQ("t3", request.testonly_page_token);
+        ResponseType response;
+        response.testonly_set_page_token("t4");
+        return response;
+      })
+      .WillOnce([](Request const& request) {
+        EXPECT_EQ(CurrentOptions().get<StringOption>(), "EmptyAndFullPages");
+        EXPECT_EQ("t4", request.testonly_page_token);
+        ResponseType response;
+        response.testonly_items.push_back(Item{"p3"});
+        response.testonly_items.push_back(Item{"p4"});
+        response.testonly_set_page_token("");
+        return response;
+      });
+
+  OptionsSpan span(Options{}.set<StringOption>("EmptyAndFullPages"));
+  auto range = MakePaginationRange<ItemRange>(
+      Request{}, [&mock](Request const& r) { return mock.Loader(r); },
+      [](ResponseType const& r) { return r.testonly_items; });
+  OptionsSpan overlay(Options{}.set<StringOption>("uh-oh"));
+  std::vector<std::string> names;
+  for (auto& p : range) {
+    if (!p) break;
+    names.push_back(p->data);
+  }
+  EXPECT_THAT(names, ElementsAre("p1", "p2", "p3", "p4"));
+}
+
 TYPED_TEST(PaginationRangeTest, TwoPagesWithError) {
   using ResponseType = TypeParam;
   MockRpc<ResponseType> mock;

From b5ab14012aa5ee757d2b7c3216656fccee76345f Mon Sep 17 00:00:00 2001
From: Carlos O'Ryan <coryan@google.com>
Date: Thu, 22 Jun 2023 00:34:31 +0000
Subject: [PATCH 2/2] Address review comments

---
 google/cloud/internal/pagination_range.h | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/google/cloud/internal/pagination_range.h b/google/cloud/internal/pagination_range.h
index 5810aa485893b..4f65031ae8c49 100644
--- a/google/cloud/internal/pagination_range.h
+++ b/google/cloud/internal/pagination_range.h
@@ -102,8 +102,8 @@ class PagedStreamReader {
       page_ = extractor_(*std::move(response));
       current_ = page_.begin();
     }
-    if (current_ != page_.end()) return std::move(*current_++);
-    return Status{};
+    if (current_ == page_.end()) return Status{};
+    return std::move(*current_++);
   }
 
  private: