Skip to content

THRIFT-6045: Add recursion-depth round-trip test for the Ruby library#3550

Merged
Jens-G merged 1 commit into
apache:masterfrom
Jens-G:rb-recursion-depth
Jun 3, 2026
Merged

THRIFT-6045: Add recursion-depth round-trip test for the Ruby library#3550
Jens-G merged 1 commit into
apache:masterfrom
Jens-G:rb-recursion-depth

Conversation

@Jens-G
Copy link
Copy Markdown
Member

@Jens-G Jens-G commented May 28, 2026

THRIFT-6045: Add a recursion-depth round-trip test for the Ruby library

This is a test-only change — it adds the regression test without the implementation; the limit itself is left to a follow-up (see the discussion below).

spec/recursion_depth_spec.rb drives the generated read/write path (Thrift::Struct#read/#write, Thrift::Union#read/#write) over recursive RecTree (struct), RecUnion (union) and RecError (exception) types added to ThriftSpec.thrift, across BinaryProtocol and (when the native extension is loaded) BinaryProtocolAccelerated.

  • Active (round-trip / within-limit): a chain at the limit round-trips, a wide shallow tree round-trips, a struct through a MultiplexedProtocol, and the exception round-trip.
  • Pending (limit-enforcement, over-limit write/read for struct, union and exception): the Ruby library does not bound recursion depth yet, so these are expected to fail until the limit is implemented, at which point RSpec flags them to be enabled.

The over-limit read payloads are hand-serialized with the real recursive field (id 1, list<self>) so a future guarded reader recurses through the generated path rather than skip().

Validated locally (Ruby 3.0.2, pure-Ruby): 10 examples, 0 failures, 5 pending. Implementation note for the follow-up: the native thrift_native C extension rebinds Thrift::Struct#read/#write, so a complete fix must also guard rb_thrift_struct_write/read (and the union equivalents) in C, not only the pure-Ruby path.

🤖 Generated with Claude Code

@Jens-G Jens-G requested a review from kpumuk as a code owner May 28, 2026 11:45
@mergeable mergeable Bot added the ruby Pull requests that update Ruby code label May 28, 2026
Copy link
Copy Markdown
Member

@kpumuk kpumuk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This PR is insufficient:

  • It only covers pure Ruby implementation, and not the native extension
  • It does not cover Union
  • There are no tests

I am currently working on https://issues.apache.org/jira/browse/THRIFT-5938, with recursion depth enforcement being a part of the problem. I can extract my change to merge it ahead of the configuration class

@Jens-G Jens-G marked this pull request as draft May 28, 2026 22:46
@Jens-G Jens-G force-pushed the rb-recursion-depth branch 2 times, most recently from 84c4704 to d7a6fff Compare May 30, 2026 21:35
@Jens-G Jens-G marked this pull request as ready for review May 30, 2026 21:40
@Jens-G
Copy link
Copy Markdown
Member Author

Jens-G commented May 30, 2026

This PR is insufficient:

It is way more than that. That#s hy I made it a draft. Different story.

I can extract my change to merge it ahead of the configuration class

Ok I only push the test then and leave it up to you, including the ticket.

@Jens-G Jens-G force-pushed the rb-recursion-depth branch from d7a6fff to fa9ce28 Compare May 30, 2026 22:16
@Jens-G Jens-G changed the title THRIFT-6045: Limit struct read/write recursion depth in Ruby library THRIFT-6045: Add recursion-depth round-trip test for the Ruby library May 30, 2026
Client: rb

Adds a round-trip regression test for struct/union/exception recursion depth
without the implementation; the limit itself is left to a follow-up.

spec/recursion_depth_spec.rb drives the generated read/write path over recursive
RecTree (struct), RecUnion (union) and RecError (exception) types added to
ThriftSpec.thrift, across BinaryProtocol and BinaryProtocolAccelerated. The
round-trip / within-limit cases (a chain at the limit, a wide shallow tree, a
struct through a MultiplexedProtocol, and the exception round-trip) are active.
The limit-enforcement cases (over-limit struct/union/exception write and read)
are marked pending: the Ruby library does not bound recursion depth yet, so they
are expected to fail until the limit is added, at which point RSpec flags them to
be enabled.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
@Jens-G Jens-G force-pushed the rb-recursion-depth branch from fa9ce28 to d15e539 Compare June 1, 2026 18:45
@Jens-G Jens-G merged commit 3822c70 into apache:master Jun 3, 2026
88 of 89 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

ruby Pull requests that update Ruby code

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants