From f869cfa479c293ecd55c2f8ccbf981cd3bb41ecf Mon Sep 17 00:00:00 2001 From: Charles OuGuo Date: Tue, 26 Dec 2023 12:09:51 -0800 Subject: [PATCH] In Ruby repeated fields, each_index actually iterates over the index (#11767) Currently we're aliasing `each_index` to `each_with_index`, incorrectly passing both the index and the value of a repeated field to the block. What we want is to just pass the index. Luckily this is a method on Ruby arrays, so we just wrap the native Ruby array method. Fixes https://github.com/protocolbuffers/protobuf/issues/7806 Closes #11767 COPYBARA_INTEGRATE_REVIEW=https://github.com/protocolbuffers/protobuf/pull/11767 from shaldengeki:shaldengeki-repeated-field-each-index-returns-actual-index 874916c21db5e728ce84f5349a607c7ce42f6006 PiperOrigin-RevId: 593835025 --- .../v3.0.0/tests/repeated_field_test.rb | 14 ++++++++++++++ ruby/lib/google/protobuf/repeated_field.rb | 3 +-- ruby/tests/repeated_field_test.rb | 14 ++++++++++++++ 3 files changed, 29 insertions(+), 2 deletions(-) diff --git a/ruby/compatibility_tests/v3.0.0/tests/repeated_field_test.rb b/ruby/compatibility_tests/v3.0.0/tests/repeated_field_test.rb index 56fa7fe41b..ed12eaa8de 100755 --- a/ruby/compatibility_tests/v3.0.0/tests/repeated_field_test.rb +++ b/ruby/compatibility_tests/v3.0.0/tests/repeated_field_test.rb @@ -122,6 +122,20 @@ class RepeatedFieldTest < Test::Unit::TestCase end + def test_each_index + m = TestMessage.new + 5.times{|i| m.repeated_string << 'string' } + + expected = 0 + m.repeated_string.each_index do |idx| + assert_equal expected, idx + expected += 1 + assert_equal 'string', m.repeated_string[idx] + end + assert_equal 5, expected + end + + def test_empty? m = TestMessage.new assert_empty m.repeated_string diff --git a/ruby/lib/google/protobuf/repeated_field.rb b/ruby/lib/google/protobuf/repeated_field.rb index a48095b62e..b8c71e0655 100644 --- a/ruby/lib/google/protobuf/repeated_field.rb +++ b/ruby/lib/google/protobuf/repeated_field.rb @@ -94,7 +94,6 @@ module Google end # array aliases into enumerable - alias_method :each_index, :each_with_index alias_method :slice, :[] alias_method :values_at, :select alias_method :map, :collect @@ -145,7 +144,7 @@ module Google end - %w(collect! compact! delete_if fill flatten! insert reverse! + %w(collect! compact! delete_if each_index fill flatten! insert reverse! rotate! select! shuffle! sort! sort_by! uniq!).each do |method_name| define_array_wrapper_with_result_method(method_name) end diff --git a/ruby/tests/repeated_field_test.rb b/ruby/tests/repeated_field_test.rb index 8e49fdce35..293b4e42e1 100755 --- a/ruby/tests/repeated_field_test.rb +++ b/ruby/tests/repeated_field_test.rb @@ -143,6 +143,20 @@ class RepeatedFieldTest < Test::Unit::TestCase end + def test_each_index + m = TestMessage.new + 5.times{|i| m.repeated_string << 'string' } + + expected = 0 + m.repeated_string.each_index do |idx| + assert_equal expected, idx + expected += 1 + assert_equal 'string', m.repeated_string[idx] + end + assert_equal 5, expected + end + + def test_empty? m = TestMessage.new assert_empty m.repeated_string