Copilot commented on code in PR #3705:
URL: https://github.com/apache/thrift/pull/3705#discussion_r3721718541
##########
lib/rb/lib/thrift/transport/header_transport.rb:
##########
@@ -431,6 +431,8 @@ def bounded_inflate(compressed)
inflater.inflate(compressed, &append_chunk)
inflater.finish(&append_chunk)
buffer
+ rescue Zlib::DataError, Zlib::BufError
+ raise TransportException.new(TransportException::UNKNOWN, "Invalid
ZLIB payload")
Review Comment:
Consider capturing the rescued Zlib exception (e.g., `rescue ... => e`) and
re-raising with the original exception as the `cause`. This preserves debugging
context while still enforcing the transport-exception contract (stable
message/type), and makes production diagnosis of malformed/truncated frames
much easier.
##########
lib/rb/spec/header_transport_spec.rb:
##########
@@ -206,6 +206,28 @@ def framed(message)
expect(read_trans.read(1_000)).to eq("A" * 1_000)
end
+ {
+ "invalid" => "not-zlib",
+ "truncated" => Zlib::Deflate.deflate("valid payload")[0...-1]
+ }.each do |failure_type, payload|
Review Comment:
The spec intends to cover both `Zlib::DataError` and `Zlib::BufError`, but
the truncated payload may still raise `Zlib::DataError` depending on zlib
behavior/version, leaving the `Zlib::BufError` rescue path potentially
unexercised. Consider adding a targeted test that deterministically triggers
`Zlib::BufError` (or explicitly stubbing/injecting the inflater to raise it) so
both rescued exceptions are covered.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
