diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 516d59b1b..a241a0926 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -29,6 +29,10 @@ jobs: version: '1.60.3' - run: smithy --version + - name: Pin rdoc for JRuby + if: startsWith(matrix.ruby, 'jruby') + run: echo 'gem "rdoc", "< 8.0"' >> Gemfile + - name: Setup Ruby uses: ruby/setup-ruby@v1 with: diff --git a/.github/workflows/test.yml b/.github/workflows/test.yml index a6cf284f0..918a92c3e 100644 --- a/.github/workflows/test.yml +++ b/.github/workflows/test.yml @@ -22,6 +22,10 @@ jobs: steps: - uses: actions/checkout@v4 + - name: Pin rdoc for JRuby + if: startsWith(matrix.ruby, 'jruby') + run: echo 'gem "rdoc", "< 8.0"' >> Gemfile + - name: Setup Ruby uses: ruby/setup-ruby@v1 with: diff --git a/gems/smithy-client/lib/smithy-client.rb b/gems/smithy-client/lib/smithy-client.rb index 48aca2f52..254e67b76 100644 --- a/gems/smithy-client/lib/smithy-client.rb +++ b/gems/smithy-client/lib/smithy-client.rb @@ -26,6 +26,7 @@ require_relative 'smithy-client/log_param_formatter' require_relative 'smithy-client/managed_file' require_relative 'smithy-client/networking_error' +require_relative 'smithy-client/page_enumerator' require_relative 'smithy-client/pageable_response' require_relative 'smithy-client/param_converter' require_relative 'smithy-client/param_validator' diff --git a/gems/smithy-client/lib/smithy-client/page_enumerator.rb b/gems/smithy-client/lib/smithy-client/page_enumerator.rb new file mode 100644 index 000000000..cdac11b8f --- /dev/null +++ b/gems/smithy-client/lib/smithy-client/page_enumerator.rb @@ -0,0 +1,36 @@ +# frozen_string_literal: true + +module Smithy + module Client + # A controlled enumerator for paginated results. Exposes only safe + # enumeration methods that are meaningful for page/item iteration, + # preventing accidental / wasteful full-pagination calls like #count or #sort. + class PageEnumerator + def initialize(&block) + @block = block + end + + def each(&consumer) + return self unless consumer + + enum.each(&consumer) + end + + def map(&) = enum.map(&) + def select(&) = enum.select(&) + def filter(&) = enum.select(&) + def flat_map(&) = enum.flat_map(&) + def reduce(*, &) = enum.reduce(*, &) + def first(*) = enum.first(*) + + def take(val) = enum.take(val) + def lazy = enum.lazy + + private + + def enum + Enumerator.new(&@block) + end + end + end +end diff --git a/gems/smithy-client/lib/smithy-client/pageable_response.rb b/gems/smithy-client/lib/smithy-client/pageable_response.rb index 970fb9103..8080cf65f 100644 --- a/gems/smithy-client/lib/smithy-client/pageable_response.rb +++ b/gems/smithy-client/lib/smithy-client/pageable_response.rb @@ -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. # + # When called without a block, `each_page` returns a {PageEnumerator} that supports + # chaining safe enumeration methods: + # + # weather.list_cities.each_page.map { |page| page.items.map(&:name) } + # weather.list_cities.each_page.first(3) + # weather.list_cities.each_page.flat_map { |page| page.items } + # # If the operation allows for it, a selected item can be enumerated using # `each_item`: # @@ -96,9 +103,21 @@ def next_page(params = {}) end # Yields the current and each following response to the given block. + # When called without a block, returns a {PageEnumerator}. # @yieldparam [Response] response - # @return [Enumerable, nil] Returns a new Enumerable if no block is given. - def each_page(&) + # @return [PageEnumerator, nil] + def each_page(&block) + unless block + return PageEnumerator.new do |y| + response = self + y << response + until response.last_page? + response = response.next_page + y << response + end + end + end + response = self yield(response) until response.last_page? @@ -107,16 +126,28 @@ def each_page(&) end end - # Yields the current and each following item to the given block. + # Yields the current and each following response to the given block. + # When called without a block, returns a {PageEnumerator}. + # This is an alias for {#each_page}. + # @yieldparam [Response] response + # @return [PageEnumerator, nil] + def each(&) + each_page(&) + end + + # Yields each item across all pages to the given block. + # When called without a block, returns a {PageEnumerator}. # @yieldparam [Object] item - # @return [Enumerable, nil] Returns a new Enumerable if no block is given. - def each_item(&) - response = self - @paginator.items(response.data).each(&) - until response.last_page? - response = response.next_page - @paginator.items(response.data).each(&) + # @return [PageEnumerator, nil] + # @raise [NotImplementedError] if the service API does not define items. + def each_item(&block) + unless block + return PageEnumerator.new do |y| + each_page { |page| @paginator.items(page.data).each { |item| y << item } } + end end + + each_page { |page| @paginator.items(page.data).each(&block) } end private @@ -130,7 +161,7 @@ def next_page_params(params) prev_tokens = @paginator.prev_tokens(context.params) # Remove all previous tokens from original params # Sometimes a token can be nil and merge would not include it. - new_params = context[:original_params].except(*prev_tokens) + new_params = context[:original_params].except(*prev_tokens.keys) new_params.merge!(@paginator.next_tokens(data).merge(params)) end end diff --git a/gems/smithy-client/spec/smithy-client/page_enumerator_spec.rb b/gems/smithy-client/spec/smithy-client/page_enumerator_spec.rb new file mode 100644 index 000000000..98738a76c --- /dev/null +++ b/gems/smithy-client/spec/smithy-client/page_enumerator_spec.rb @@ -0,0 +1,95 @@ +# frozen_string_literal: true + +require_relative '../spec_helper' + +module Smithy + module Client + describe PageEnumerator do + let(:enumerator) do + PageEnumerator.new do |y| + y << 'page1' + y << 'page2' + y << 'page3' + end + end + + describe '#each' do + it 'yields each value when a block is given' do + results = [] + enumerator.each { |v| results << v } # rubocop:disable Style/MapIntoArray + expect(results).to eq %w[page1 page2 page3] + end + + it 'returns self when no block is given' do + expect(enumerator.each).to be(enumerator) + end + end + + describe '#map' do + it 'transforms each value' do + expect(enumerator.map(&:upcase)).to eq %w[PAGE1 PAGE2 PAGE3] + end + end + + describe '#select' do + it 'filters values' do + expect(enumerator.select { |v| v.include?('2') }).to eq ['page2'] + end + end + + describe '#filter' do + it 'filters values (alias for select)' do + expect(enumerator.filter { |v| v.include?('3') }).to eq ['page3'] + end + end + + describe '#flat_map' do + it 'maps and flattens values' do + expect(enumerator.flat_map { |v| [v, v] }).to eq %w[page1 page1 page2 page2 page3 page3] + end + end + + describe '#reduce' do + it 'accumulates values' do + expect(enumerator.reduce('') { |acc, v| acc + v }).to eq 'page1page2page3' + end + + it 'supports an initial value' do + expect(enumerator.reduce([]) { |acc, v| acc + [v] }).to eq %w[page1 page2 page3] + end + end + + describe '#first' do + it 'returns the first value with no argument' do + expect(enumerator.first).to eq 'page1' + end + + it 'returns the first n values with an argument' do + expect(enumerator.first(2)).to eq %w[page1 page2] + end + end + + describe '#take' do + it 'returns the first n values' do + expect(enumerator.take(2)).to eq %w[page1 page2] + end + end + + describe '#lazy' do + it 'returns a lazy enumerator' do + lazy = enumerator.lazy + expect(lazy).to be_a(Enumerator::Lazy) + expect(lazy.take(1).to_a).to eq ['page1'] + end + end + + describe 'blocked methods' do + %i[count sort min max tally to_a sum zip].each do |method| + it "does not respond to ##{method}" do + expect(enumerator.respond_to?(method)).to be false + end + end + end + end + end +end diff --git a/gems/smithy-client/spec/smithy-client/plugins/pageable_response_spec.rb b/gems/smithy-client/spec/smithy-client/plugins/pageable_response_spec.rb index cc87811cc..e694a0f5c 100644 --- a/gems/smithy-client/spec/smithy-client/plugins/pageable_response_spec.rb +++ b/gems/smithy-client/spec/smithy-client/plugins/pageable_response_spec.rb @@ -246,6 +246,181 @@ module Plugins expect(response.last_page?).to be true expect { response.next_page }.to raise_error(LastPageError) end + + context '#each' do + it 'yields pages when a block is given' do + client.stub_responses( + :get_foos, + { next_token: 'next_token', foos: %w[foo1 foo2] }, + { next_token: nil, foos: ['foo3'] } + ) + + pages = [] + client.get_foos.each { |page| pages << page.foos } # rubocop:disable Style/MapIntoArray + expect(pages).to eq [%w[foo1 foo2], ['foo3']] + end + + it 'returns a PageEnumerator when no block is given' do + client.stub_responses( + :get_foos, + { next_token: nil, foos: %w[foo1 foo2] } + ) + + result = client.get_foos.each + expect(result).to be_a(Smithy::Client::PageEnumerator) + end + + it 'supports chaining map on the PageEnumerator' do + client.stub_responses( + :get_foos, + { next_token: 'next_token', foos: %w[foo1 foo2] }, + { next_token: nil, foos: ['foo3'] } + ) + + result = client.get_foos.each.map(&:foos) + expect(result).to eq [%w[foo1 foo2], ['foo3']] + end + + it 'supports chaining flat_map on the PageEnumerator' do + client.stub_responses( + :get_foos, + { next_token: 'next_token', foos: %w[foo1 foo2] }, + { next_token: nil, foos: ['foo3'] } + ) + + result = client.get_foos.each.flat_map(&:foos) + expect(result).to eq %w[foo1 foo2 foo3] + end + + it 'supports first on the PageEnumerator' do + client.stub_responses( + :get_foos, + { next_token: 'next_token', foos: %w[foo1 foo2] }, + { next_token: 'next_token2', foos: ['foo3'] }, + { next_token: nil, foos: ['foo4'] } + ) + + result = client.get_foos.each.first + expect(result.foos).to eq %w[foo1 foo2] + end + + it 'supports first(n) on the PageEnumerator' do + client.stub_responses( + :get_foos, + { next_token: 'next_token', foos: %w[foo1 foo2] }, + { next_token: 'next_token2', foos: ['foo3'] }, + { next_token: nil, foos: ['foo4'] } + ) + + result = client.get_foos.each.first(2) + expect(result.size).to eq 2 + expect(result[0].foos).to eq %w[foo1 foo2] + expect(result[1].foos).to eq ['foo3'] + end + end + + context '#each_page without block' do + it 'returns a PageEnumerator' do + client.stub_responses( + :get_foos, + { next_token: nil, foos: %w[foo1 foo2] } + ) + + result = client.get_foos.each_page + expect(result).to be_a(Smithy::Client::PageEnumerator) + end + + it 'supports chaining map' do + client.stub_responses( + :get_foos, + { next_token: 'next_token', foos: %w[foo1 foo2] }, + { next_token: nil, foos: ['foo3'] } + ) + + result = client.get_foos.each_page.map(&:foos) + expect(result).to eq [%w[foo1 foo2], ['foo3']] + end + end + + context '#each_item without block' do + it 'returns a PageEnumerator' do + client.stub_responses( + :get_foos, + { next_token: nil, foos: %w[foo1 foo2] } + ) + + result = client.get_foos.each_item + expect(result).to be_a(Smithy::Client::PageEnumerator) + end + + it 'supports chaining map' do + client.stub_responses( + :get_foos, + { next_token: 'next_token', foos: %w[foo1 foo2] }, + { next_token: nil, foos: ['foo3'] } + ) + + result = client.get_foos.each_item.map(&:upcase) + expect(result).to eq %w[FOO1 FOO2 FOO3] + end + + it 'supports first' do + client.stub_responses( + :get_foos, + { next_token: 'next_token', foos: %w[foo1 foo2] }, + { next_token: nil, foos: ['foo3'] } + ) + + expect(client.get_foos.each_item.first).to eq 'foo1' + end + + it 'supports first(n)' do + client.stub_responses( + :get_foos, + { next_token: 'next_token', foos: %w[foo1 foo2] }, + { next_token: nil, foos: ['foo3'] } + ) + + expect(client.get_foos.each_item.first(2)).to eq %w[foo1 foo2] + end + + it 'supports select' do + client.stub_responses( + :get_foos, + { next_token: 'next_token', foos: %w[foo1 foo2] }, + { next_token: nil, foos: ['foo3'] } + ) + + result = client.get_foos.each_item.select { |item| item.include?('1') } + expect(result).to eq ['foo1'] + end + end + + context 'blocked methods' do + it 'does not expose dangerous methods on PageEnumerator' do + client.stub_responses( + :get_foos, + { next_token: nil, foos: %w[foo1 foo2] } + ) + + enumerator = client.get_foos.each + %i[count sort min max tally to_a sum].each do |method| + expect(enumerator.respond_to?(method)).to be false + end + end + end + + context 'delegator safety' do + it 'does not forward .map to struct field iteration' do + client.stub_responses( + :get_foos, + { next_token: nil, foos: %w[foo1 foo2] } + ) + + response = client.get_foos + expect { response.map { |x| x } }.to raise_error(NoMethodError) + end + end end end end diff --git a/gems/smithy-client/spec/smithy-client/plugins/stub_responses_spec.rb b/gems/smithy-client/spec/smithy-client/plugins/stub_responses_spec.rb index 2a07255df..df1f5f535 100644 --- a/gems/smithy-client/spec/smithy-client/plugins/stub_responses_spec.rb +++ b/gems/smithy-client/spec/smithy-client/plugins/stub_responses_spec.rb @@ -171,7 +171,7 @@ module Plugins data = { string: 'new string' } client.stub_responses(:operation, data) response = client.operation - expect(response.data).not_to include(default_stub_data.except(:string)) + expect(response.data.to_h).not_to include(default_stub_data.except(:string)) end it 'can stub multiple responses' do diff --git a/gems/smithy-schema/lib/smithy-schema/structure.rb b/gems/smithy-schema/lib/smithy-schema/structure.rb index 3990736c4..7683b374c 100644 --- a/gems/smithy-schema/lib/smithy-schema/structure.rb +++ b/gems/smithy-schema/lib/smithy-schema/structure.rb @@ -4,6 +4,12 @@ module Smithy module Schema # A module mixed into Structs that provides utility methods for Structure shapes. module Structure + def self.included(base) + # 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) + base.undef_method(:each) if base.is_a?(Class) && base.method_defined?(:each) + end + # Deeply converts the Struct into a hash. Structure members that # are `nil` are omitted from the resultant hash. # @return [Hash, Structure] diff --git a/gems/smithy-schema/spec/smithy-schema/structure_spec.rb b/gems/smithy-schema/spec/smithy-schema/structure_spec.rb index 7e84220d1..1df29015d 100644 --- a/gems/smithy-schema/spec/smithy-schema/structure_spec.rb +++ b/gems/smithy-schema/spec/smithy-schema/structure_spec.rb @@ -40,6 +40,16 @@ module Schema ) end + describe '#each' do + it 'is undefined to prevent silent struct field iteration' do + expect(subject.respond_to?(:each)).to be false + end + + it 'raises NoMethodError when called' do + expect { subject.each }.to raise_error(NoMethodError) + end + end + describe '#to_h' do it 'serializes nested structs to a hash' do expected = {