feat: Address pagination API gap with aligned approach by sichanyoo · Pull Request #345 · smithy-lang/smithy-ruby

sichanyoo · 2026-06-17T18:02:29Z

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

Background & Decisions

Responses are Struct subclasses, and Ruby's Struct includes Enumerable by default. Without this fix, calling .each on a paginated response silently iterated struct field values instead of pages, a possible silent data-loss bug. This PR fixes the gap by adding .each to PageableResponse (delegating to each_page), undefining each on Smithy::Schema::Structure as a safety net, and introducing a PageEnumerator class that exposes only safe enumeration methods (map, select, flat_map, reduce,first, take, lazy) while blocking dangerous ones (count, sort, to_a, etc.) that would silently trigger full pagination with real API calls and real cost.

We chose a custom PageEnumerator over mixing Enumerable into the response because starting with a narrow surface and expanding later is non-breaking, whereas removing a dangerous method after customers depend on it is breaking.

.each iterates pages rather than items because items is optional in Smithy's @paginated trait. About 21% of paginated operations at AWS have no items defined currently, and some operations return multiple result collections, making page iteration the only universally safe default.

sichanyoo

Comments for additional context

sichanyoo · 2026-06-19T16:22:19Z

@@ -0,0 +1,40 @@
+# frozen_string_literal: true


This is the custom enumerator with limited API surface to safeguard against unsafe Enumerable methods

sichanyoo · 2026-06-19T16:23:37Z

@@ -4,6 +4,10 @@ module Smithy
  module Schema
    # A module mixed into Structs that provides utility methods for Structure shapes.
    module Structure


Add each undef to prevent silent data loss in the case pageable_response plugin doesn't run for whatever reason (e.g., customer customization on plugin list that leaves it out)

Probably better if you turn the above into a code comment for future us.

Added it as inline comment.

sichanyoo · 2026-06-19T16:25:44Z

@@ -36,6 +36,13 @@ def initialize(response)
    # This yields one response object per API call made. The SDK retrieves additional
    # pages of data to complete the request.
    #


Updates the PageableResponse module that gets added to Response to return the custom enumerator when not give a block, and if given a block, enumerate using the custom enumerator and yield results. As it has been, PageableResponse#each will paginate over pages instead of items due to uniqueness of AWS background discussed offline.

jterapin

The overall approach looks good but curious what we can do about the enumerator being created at every method call. 🤔

Also - I was wondering if the PR description could contain overall info about the motivation of this approach and what decisions we make.

We should also have wangrch take a look when he comes back.

jterapin · 2026-06-22T19:13:33Z

+      def enum
+        Enumerator.new(&@block)
+      end


The current PageEnumerator creates a new Enumerator on every method call (map, select, first, etc.) via the private enum method. Is there a way to prevent this or was this intentional?

The allocation cost is negligible compared to the API calls each iteration triggers & returning brand new enumerator each time I think has a subtle benefit of thread safety with interleaved paging operations when it's used concurrently. But changing it to reuse enumerator is a one line change so if this is based on rubyist convention / you feel strongly for reusing, we can do that

jterapin · 2026-06-22T19:18:27Z

+      def reduce(*, &) = enum.reduce(*, &)
+
+      def first(val = (no_arg = true
+                       nil))


Fix alignment.

Can we do: def first(*args) if we are just forwarding args?

Good catch! Changed as suggested 👍

jterapin · 2026-06-22T19:32:09Z

@@ -107,16 +126,29 @@ def each_page(&)
        end


What about returning self here to enable chaining?

Q: What could be the usecase for returning self to allow chaining here? If user doesn't provide a block, they get enumerator they can chain to already, and if user provides a block, whatever the user needs to do would be specified within that block. Fwiw, V3 also returns nil instead of self for each when user provides a block: https://github.com/aws/aws-sdk-ruby/blob/313cb58ad76dac97c27ff6d63b99eaadd5ad4d00/gems/aws-sdk-core/lib/aws-sdk-core/pageable_response.rb#L188-L196

jterapin · 2026-06-22T19:34:41Z

@@ -4,6 +4,10 @@ module Smithy
  module Schema
    # A module mixed into Structs that provides utility methods for Structure shapes.
    module Structure


Probably better if you turn the above into a code comment for future us.

jterapin · 2026-06-22T19:38:14Z

+require_relative 'smithy-client/paginators/page_enumerator'
+require_relative 'smithy-client/paginators/pageable_response'


Ruby convention: a directory paginators/ implies a module Paginators. But PageEnumerator and PageableResponse don't live under a Smithy::Client::Paginators module

Feels longwinded if we add a new module so maybe we should keep this flat.

I see! Updated to keep the paginator file structure flat.

jterapin · 2026-06-22T19:45:16Z

-          response = response.next_page
-          @paginator.items(response.data).each(&)
+      # @return [PageEnumerator, nil]
+      def each_item(&block)


If the API does not define items and customer calls .each_item, they'll get a NotImplementedError. Should we document this?

Good point, added a @raise doc comment.

richardwang1124

Looks good overall, can take a second pass review once the comments are addressed.

richardwang1124 · 2026-06-26T16:11:10Z

+      def each(&block)
+        return each_page unless block
+
+        each_page(&block)
+      end


I think this could be one line? Maybe

def each(&block) each_page(&block) end

Good catch! Changed as suggested 👍

sichanyoo

Address initial review comments.

sichanyoo · 2026-06-26T18:12:03Z

+      def enum
+        Enumerator.new(&@block)
+      end


The allocation cost is negligible compared to the API calls each iteration triggers & returning brand new enumerator each time I think has a subtle benefit of thread safety with interleaved paging operations when it's used concurrently. But changing it to reuse enumerator is a one line change so if this is based on rubyist convention / you feel strongly for reusing, we can do that

sichanyoo · 2026-06-26T18:17:09Z

+      def reduce(*, &) = enum.reduce(*, &)
+
+      def first(val = (no_arg = true
+                       nil))


Good catch! Changed as suggested 👍

sichanyoo · 2026-06-26T18:24:46Z

@@ -107,16 +126,29 @@ def each_page(&)
        end


Q: What could be the usecase for returning self to allow chaining here? If user doesn't provide a block, they get enumerator they can chain to already, and if user provides a block, whatever the user needs to do would be specified within that block. Fwiw, V3 also returns nil instead of self for each when user provides a block: https://github.com/aws/aws-sdk-ruby/blob/313cb58ad76dac97c27ff6d63b99eaadd5ad4d00/gems/aws-sdk-core/lib/aws-sdk-core/pageable_response.rb#L188-L196

sichanyoo · 2026-06-26T18:27:54Z

+      def each(&block)
+        return each_page unless block
+
+        each_page(&block)
+      end


Good catch! Changed as suggested 👍

sichanyoo · 2026-06-26T18:30:02Z

-          response = response.next_page
-          @paginator.items(response.data).each(&)
+      # @return [PageEnumerator, nil]
+      def each_item(&block)


Good point, added a @raise doc comment.

sichanyoo · 2026-06-26T18:33:41Z

+require_relative 'smithy-client/paginators/page_enumerator'
+require_relative 'smithy-client/paginators/pageable_response'


I see! Updated to keep the paginator file structure flat.

sichanyoo · 2026-06-26T18:37:05Z

@@ -4,6 +4,10 @@ module Smithy
  module Schema
    # A module mixed into Structs that provides utility methods for Structure shapes.
    module Structure


Added it as inline comment.

sichanyoo added 2 commits June 17, 2026 11:01

Address pagination API gap with aligned approach.

31619f8

Fix .except bug in existing code.

3b6e61d

sichanyoo commented Jun 19, 2026

View reviewed changes

sichanyoo requested review from jterapin and richardwang1124 June 19, 2026 16:37

jterapin reviewed Jun 22, 2026

View reviewed changes

richardwang1124 reviewed Jun 26, 2026

View reviewed changes

Address initial review comments.

83bb57a

sichanyoo commented Jun 26, 2026

View reviewed changes

sichanyoo added 2 commits June 26, 2026 11:57

addres jruby rbs issue in CI

4c72adb

Fix spec helper path.

bc955be

sichanyoo requested review from jterapin and richardwang1124 June 26, 2026 20:37

		require_relative 'smithy-client/paginators/page_enumerator'
		require_relative 'smithy-client/paginators/pageable_response'

Uh oh!

Conversation

sichanyoo commented Jun 17, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Background & Decisions

Uh oh!

sichanyoo left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jterapin left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

richardwang1124 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sichanyoo left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

sichanyoo commented Jun 17, 2026 •

edited

Loading