Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Ruby] Fix RepeatedField#last, #first inconsistencies #9722

Merged
merged 2 commits into from
Apr 6, 2022

Conversation

lucthev
Copy link
Contributor

@lucthev lucthev commented Apr 1, 2022

RepeatedField#last(n) has an off-by-one error resulting in one more element being returned than requested.

While I was at it, I also fixed the following inconsistencies with Array:

  • last(n) where n > size should return all elements; the current implementation wraps around such that e.g. last(size + 2) == last(2)
  • last(n) where n < 0 should raise an ArgumentError

@fowles fowles added the ruby label Apr 2, 2022
@fowles fowles self-requested a review April 2, 2022 17:12
@fowles
Copy link
Member

fowles commented Apr 2, 2022

This looks good. I will merge it after our test suite finishes

@fowles
Copy link
Member

fowles commented Apr 5, 2022

Looks like this is failing tests:

Failure: test_last(RepeatedFieldTest)
/tmp/protobuf/protobuf/ruby/tests/repeated_field_test.rb:85:in `test_last'
     82:     end
     83:     assert_equal "negative array size", err.message
     84:     assert_equal [], m.repeated_int32.last(0)
  => 85:     assert_equal [-11], m.repeated_int32.last(1)
     86:     assert_equal [-10, -11], m.repeated_int32.last(2)
     87:     assert_equal [-10, -11], m.repeated_int32.last(3)
     88:   end
org/jruby/RubyKernel.java:1189:in `catch'
org/jruby/RubyKernel.java:1184:in `catch'
org/jruby/RubyKernel.java:1189:in `catch'
org/jruby/RubyKernel.java:1184:in `catch'
<[-11]> expected but was
<[]>****

@fowles
Copy link
Member

fowles commented Apr 5, 2022

Looks like the failure is in jruby. Jason?

@JasonLunn
Copy link
Contributor

It appears that the fix for this issue has uncovered a separate bug in the JRuby implementation of the at (aka []) method of RubyRepeatedField. It appears that the implementation is making assumptions here about the range values that don't hold if you use negative indices to count back from the end of the sequence.

@lucthev - if you're comfortable working in Java, you can fix both issues in the same PR. If not, please open a separate ticket for the JRuby-specific issue and I can work up a patch.

@fowles
Copy link
Member

fowles commented Apr 6, 2022

rerunning test suite. Will come back tomorrow to see how it went

@lucthev
Copy link
Contributor Author

lucthev commented Apr 6, 2022

Thanks for the reviews! I had a stab at fixing the issue in RubyRepeatedField#[] but there's a lot of odd edge-cases (and I'm not very familiar with Java) so I didn't get very far. I've opened #9751 to track that issue just for completeness (it's pretty niche anyways).

In the meantime, I've worked around that in this PR by not using a negative index.

@JasonLunn JasonLunn merged commit 05df37d into protocolbuffers:main Apr 6, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants