THRIFT-6290: Rust: skip string/binary fields without heap-allocating

[lbarthon]

Client: rs

JIRA: https://issues.apache.org/jira/browse/THRIFT-6290

skip(TType::String) allocated a Vec the size of the field via read_bytes() and immediately dropped it. Binary and compact now read the length prefix and discard the payload through a 256-byte stack buffer (skip_binary / discard_exact). Negative binary sizes and max_string_size still apply. Custom TInputProtocol impls keep the allocating default.

• Did you create an Apache Jira ticket? (Request account here, not required for trivial changes)
• If a ticket exists: Does your pull request title follow the pattern "THRIFT-NNNN: describe my issue"?
• Did you squash your changes to a single commit? (not required, but preferred)
• Did you do your best to avoid breaking changes? If one was needed, did you label the Jira ticket with "Breaking-Change"?
• If your change does not involve any code, include [skip ci] anywhere in the commit message to free up build resources.

[lbarthon]

cc @Jens-G when you have a moment 🙏

[Jens-G]

Code review

Found 1 issue:

1. There is no JIRA ticket for this change, and the PR title and the commit subject do not start with THRIFT-NNNN:. The change adds a method to the public TInputProtocol trait and changes how the binary and compact protocols skip string and binary fields, so it is not a trivial one (AGENTS.md says a ticket is "Required for all non-trivial PRs" and "All significant changes need a JIRA ticket.")

thrift/AGENTS.md

Lines 27 to 29 in 7d6be86

	|---|---|---|
	| Significant changes | [Apache JIRA — THRIFT project](https://issues.apache.org/jira/browse/THRIFT) | Required for all non-trivial PRs |
	| Minor / quick fixes | GitHub Issues | Typos, trivial compiler warnings, etc. |

thrift/AGENTS.md

Lines 48 to 50 in 7d6be86

	- One commit per issue (squash before submitting).
	- All significant changes need a JIRA ticket.
	- Provide tests for every submitted change.

One suggestion, below the bar for the list above but verified:

• None of the new tests notices if skip() goes back to read_bytes(), or if the skip_binary forward in Box<P> or in TStoredInputProtocol is dropped, because both paths consume the same bytes and return the same errors. A test transport that records the largest buffer it is asked to fill does notice: while skipping a 1024-byte string it sees a single 1024-byte request on the old path and no request over 256 bytes on the new one. I checked this against all three changes.

thrift/lib/rs/src/protocol/mod.rs

Lines 1213 to 1224 in 7d6be86

	#[test]
	fn must_skip_large_binary_field_through_boxed_protocol() {
	let payload = vec![0xCDu8; 1024];
	let data = build_struct_with_unknown_binary_then_i64(&payload, 9);
	let mut proto: Box<dyn TInputProtocol> =
	Box::new(TBinaryInputProtocol::new(Cursor::new(data), true));
	let (first, second) = skip_unknown_and_read_i64_fields(&mut proto).unwrap();
	assert_eq!(first, 42);
	assert_eq!(second, Some(9));
	}

thrift/lib/rs/src/protocol/mod.rs

Lines 407 to 411 in 7d6be86

	fn skip_binary(&mut self) -> crate::Result<()> {
	(**self).skip_binary()
	}

🤖 Generated with Claude Code

_{- If this code review was useful, please react with 👍. Otherwise, react with 👎.}

[lbarthon]

@Jens-G Filed THRIFT-6290 and updated the PR title and commit subject to start with it:
https://issues.apache.org/jira/browse/THRIFT-6290

On the test note: agreed that the current tests would still pass if skip() went back to read_bytes(), or if the skip_binary forward on Box

or TStoredInputProtocol were dropped. Happy to add a transport that records the largest buffer it is asked to fill, and assert that skipping a 1024-byte string never asks for more than 256, if you want that in this PR.

[Jens-G]

Thanks — THRIFT-6290 settles the ticket point.

Yes please, add the recording transport in this PR. "Skipping a 1024-byte string never asks for more than 256 bytes" is exactly the two-state signal the current tests are missing: they pass either way, because the old and the new path consume the same bytes and return the same errors.

Worth covering all three places the forward can be lost, not just the protocols themselves: TBinaryInputProtocol / TCompactInputProtocol, the Box<P> impl, and TStoredInputProtocol. If a skip_binary forward is dropped in either wrapper it silently falls back to the allocating default, and nothing else in the suite would notice.

🤖 Generated with Claude Code

[lbarthon]

@Jens-G Added that recording transport in e54863c.

Skipping a 1024-byte string now asserts the transport is never asked to fill more than 256 bytes, on TBinaryInputProtocol, TCompactInputProtocol, Box<dyn TInputProtocol>, and TStoredInputProtocol. Dropping either skip_binary forward fails only that wrapper's test with a 1024-byte read; pointing the protocol methods back at read_bytes() fails all four the same way.