refactor(protocol): extract shared body filter helpers#855
refactor(protocol): extract shared body filter helpers#855eoinfennessy wants to merge 4 commits into
Conversation
) Deduplicate ~50 lines of near-identical logic between request_body_filter.rs and response_body_filter.rs into a new body_helpers module with shared helpers for size-limit checking, stream-buffer accumulation, chunk suppression, and buffer release. A BodyFilterOutput struct with take_from/write_back centralises the 7-field context destructure and writeback that both callers performed identically. Signed-off-by: Eoin Fennessy <efenness@redhat.com>
| @@ -0,0 +1,336 @@ | |||
| // SPDX-License-Identifier: MIT | |||
There was a problem hiding this comment.
Couldn't we have this all in super (i.e. mod.rs) and private, so that they can "just" be used from the other two "sub"modules? The root::A::* & root::B::* depending on root::C::things is maybe only weird to me tho. Feels more natural to the module visibility model afaict, no?
There was a problem hiding this comment.
Yeah, this is much better. Thanks for this lesson about module visibility :)
Done in c5c9e78
praxis-bot
left a comment
praxis-bot
left a comment
There was a problem hiding this comment.
Review Summary
Clean refactoring that faithfully extracts shared body filter logic. The field round-trip through BodyFilterOutput::take_from/write_back matches the 7 fields previously destructured inline by both callers, and the filter_context! macro confirms the context construction path is symmetric. The 4 helper functions preserve exact behavior. No correctness issues found.
| Severity | Count |
|---|---|
| Critical | 0 |
| Large | 0 |
| Medium | 1 |
Reviewed with Claude Opus 4.6
| assert_eq!(ctx.filter_metadata.get("key").map(String::as_str), Some("val")); | ||
| assert_eq!(ctx.cached_executed_filter_indices, vec![true, false]); | ||
| assert_eq!(ctx.cached_body_done_indices, vec![false, true]); | ||
| } |
There was a problem hiding this comment.
[Medium] This test only verifies 4 of 7 BodyFilterOutput fields with non-trivial values. upstream, extensions, and filter_state are all empty/default, so their transfer through write_back is not meaningfully verified.
Populate these with real values and assert them after writeback. For example:
let output = BodyFilterOutput {
cluster: Some(Arc::from("test-cluster")),
upstream: Some(Upstream { address: Arc::from("10.0.0.1:80"), .. }),
extensions: {
let mut ext = RequestExtensions::new();
ext.insert(42_u32);
ext
},
filter_metadata: HashMap::from([("key".to_owned(), "val".to_owned())]),
filter_state: HashMap::from([
(0_usize, Box::new(99_i32) as Box<dyn std::any::Any + Send + Sync>)
]),
executed_filter_indices: vec![true, false],
body_done_indices: vec![false, true],
};
output.write_back(&mut ctx);
assert!(ctx.upstream.is_some(), "upstream should transfer");
assert_eq!(ctx.extensions.get::<u32>(), Some(&42), "extensions should transfer");
assert_eq!(ctx.filter_state.len(), 1, "filter_state should transfer");Move body filter helpers from a separate `body_helpers` submodule into `mod.rs` so child modules access them from their parent rather than importing from a sibling. Functions become private (no longer `pub(super)`), matching Rust's visibility model. Signed-off-by: Eoin Fennessy <efenness@redhat.com>
Signed-off-by: Eoin Fennessy <efenness@redhat.com>
d75a7ff to
da43fda
Compare
4 participants
Summary
check_body_size_limit,accumulate_stream_buffer,suppress_stream_buffer_chunk,release_stream_buffer) fromrequest_body_filter.rsandresponse_body_filter.rsinto a newbody_helpersmoduleBodyFilterOutputstruct withtake_from/write_backmethods to centralise the 7-field context destructure and writeback that both callers performed identicallyCloses #830
Test plan
body_helpers::testspasspraxis-proxy-protocoltest suite passes (401 tests)