Open
Add modular response compression middleware (Zstd + Gzip)#85
Conversation
Agent-Logs-Url: https://github.com/fermi-ad/extapi-acsys/sessions/7b6e8ebc-9cdf-4b62-98b5-e2d812093cb9 Co-authored-by: beauremus <3767966+beauremus@users.noreply.github.com>
Copilot created this pull request from a session on behalf of
beauremus
March 30, 2026 19:50
View session
beauremus
marked this pull request as ready for review
March 30, 2026 20:24
beauremus
requested review from
bstover74,
chowington,
d4vebr4cey,
mguzman04 and
rnethi1701
March 30, 2026 20:25
Contributor
There was a problem hiding this comment.
Pull request overview
Adds a dedicated GraphQL compression module and wires it into the Axum router to enable response compression (preferring Zstd with Gzip fallback) via tower_http::compression::CompressionLayer.
Changes:
- Enabled
tower-httpgzip + zstd compression features inCargo.toml(and updatedCargo.lockaccordingly). - Added
src/graphql/compression.rsexposing a singlecompression_layer()helper. - Applied the compression layer to the main GraphQL router and added a compression-focused test.
Reviewed changes
Copilot reviewed 3 out of 4 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| Cargo.toml | Enables tower-http compression feature flags for gzip/zstd. |
| Cargo.lock | Captures new transitive deps (notably zstd-related crates). |
| src/graphql/compression.rs | Introduces isolated compression_layer() construction. |
| src/graphql/mod.rs | Registers module, applies the layer to routers, and adds a compression test. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+483
to
+486
| check_compression("gzip").await; | ||
| check_compression("zstd").await; | ||
| check_compression("deflate").await; | ||
| } |
There was a problem hiding this comment.
test_compression asserts behavior for Accept-Encoding: deflate, but compression_layer() is only configured to enable Zstd and Gzip. If deflate isn't enabled by default, this test will fail (and it also contradicts the PR description). Consider either enabling deflate explicitly in compression_layer() or adjusting this assertion to expect no content-encoding (or to only test gzip/zstd).
Author
There was a problem hiding this comment.
Fixed in commit 46775e8. The check_compression closure now uses a match on the encoding name: "gzip" and "zstd" assert the content-encoding header is present and matches; any other encoding (including "deflate") asserts no content-encoding header is set, correctly reflecting that only Gzip and Zstd are enabled in compression_layer().
Code Coverage Report - 1,671 of 3,971 lines covered ( ⛔ 42.08%)
|
Code Coverage Report - 1,668 of 3,968 lines covered ( ⛔ 42.04%)
|
Parse the content-encoding header and assert only the encodings this middleware guarantees (gzip/zstd), with a clear expectation for unsupported encodings. Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
rneswold
approved these changes
Mar 31, 2026
rneswold
left a comment
rneswold
left a comment
Contributor
There was a problem hiding this comment.
Looks good! Loving the AI assistants!
jacob-curley-fnal
approved these changes
Apr 1, 2026
jacob-curley-fnal
left a comment
jacob-curley-fnal
left a comment
Contributor
There was a problem hiding this comment.
Had some questions about the strategies used in one of the test cases, but not a showstopper! And the questions are mostly for my curiosity, I haven't checked whether the suggestion would actually compile.
Comment on lines
+444
to
+494
| async fn test_compression() { | ||
| let site = mk_test_site(); | ||
| let query = r#"{ "query" : "{ __schema { types { name } } }" }"#; | ||
|
|
||
| // Helper to check compression | ||
| let check_compression = |encoding: &str| { | ||
| let mut site = site.clone(); | ||
| let query = query.to_string(); | ||
| let encoding = encoding.to_string(); | ||
| async move { | ||
| let response = site | ||
| .as_service() | ||
| .call( | ||
| Request::builder() | ||
| .method("POST") | ||
| .uri("/test") | ||
| .header("content-type", "application/json") | ||
| .header("accept-encoding", &encoding) | ||
| .body(Body::from(query)) | ||
| .unwrap(), | ||
| ) | ||
| .await | ||
| .unwrap(); | ||
|
|
||
| assert_eq!(response.status(), StatusCode::OK); | ||
| let header = response.headers().get("content-encoding"); | ||
| match encoding.as_str() { | ||
| "gzip" | "zstd" => { | ||
| let value = header | ||
| .expect("missing content-encoding header for supported encoding") | ||
| .to_str() | ||
| .expect("invalid content-encoding header value"); | ||
| assert_eq!(value, encoding); | ||
| } | ||
| _ => { | ||
| // For unsupported encodings (e.g., deflate), we expect no compression. | ||
| assert!( | ||
| header.is_none(), | ||
| "expected no content-encoding header for unsupported encoding {}, got {:?}", | ||
| encoding, | ||
| header | ||
| ); | ||
| } | ||
| } | ||
| } | ||
| }; | ||
|
|
||
| check_compression("gzip").await; | ||
| check_compression("zstd").await; | ||
| check_compression("deflate").await; | ||
| } |
Contributor
There was a problem hiding this comment.
Nit - questions about this test case:
1.) Why initialize query as a byte string, if we're just gonna call to_string on it later (and then call as_str on it after that)? Could it just be initialized as a &str out of the gate?
let query = "{ \"query\" : \"{ __schema { types { name } } }\" }";2.) Is Rust complaining about references to these local variables within the lambda? Is that why there's the async move block in there? Or would a reference work just fine? We only use the lambda within this same test function, so those variables should stay in scope, no? Would something like this work?:
Suggested change
| async fn test_compression() { | |
| let site = mk_test_site(); | |
| let query = r#"{ "query" : "{ __schema { types { name } } }" }"#; | |
| // Helper to check compression | |
| let check_compression = |encoding: &str| { | |
| let mut site = site.clone(); | |
| let query = query.to_string(); | |
| let encoding = encoding.to_string(); | |
| async move { | |
| let response = site | |
| .as_service() | |
| .call( | |
| Request::builder() | |
| .method("POST") | |
| .uri("/test") | |
| .header("content-type", "application/json") | |
| .header("accept-encoding", &encoding) | |
| .body(Body::from(query)) | |
| .unwrap(), | |
| ) | |
| .await | |
| .unwrap(); | |
| assert_eq!(response.status(), StatusCode::OK); | |
| let header = response.headers().get("content-encoding"); | |
| match encoding.as_str() { | |
| "gzip" | "zstd" => { | |
| let value = header | |
| .expect("missing content-encoding header for supported encoding") | |
| .to_str() | |
| .expect("invalid content-encoding header value"); | |
| assert_eq!(value, encoding); | |
| } | |
| _ => { | |
| // For unsupported encodings (e.g., deflate), we expect no compression. | |
| assert!( | |
| header.is_none(), | |
| "expected no content-encoding header for unsupported encoding {}, got {:?}", | |
| encoding, | |
| header | |
| ); | |
| } | |
| } | |
| } | |
| }; | |
| check_compression("gzip").await; | |
| check_compression("zstd").await; | |
| check_compression("deflate").await; | |
| } | |
| async fn test_compression() { | |
| let site = mk_test_site(); | |
| let query = "{ \"query\" : \"{ __schema { types { name } } }\" }"; // could get by with just a &str | |
| // Helper to check compression | |
| let check_compression = |encoding: &str| async { | |
| let response = site.clone() | |
| .as_service() | |
| .call( | |
| Request::builder() | |
| .method("POST") | |
| .uri("/test") | |
| .header("content-type", "application/json") | |
| .header("accept-encoding", encoding) // encoding is already a reference | |
| .body(Body::from(query.to_string())) // If the `to_string` is needed, but sometimes it's not - check whether `Body::from` will accept an &str | |
| .unwrap(), | |
| ) | |
| .await | |
| .unwrap(); | |
| assert_eq!(response.status(), StatusCode::OK); | |
| let header = response.headers().get("content-encoding"); | |
| match encoding { | |
| "gzip" | "zstd" => { | |
| let value = header | |
| .expect("missing content-encoding header for supported encoding") | |
| .to_str() | |
| .expect("invalid content-encoding header value"); | |
| assert_eq!(value, encoding); | |
| } | |
| _ => { | |
| // For unsupported encodings (e.g., deflate), we expect no compression. | |
| assert!( | |
| header.is_none(), | |
| "expected no content-encoding header for unsupported encoding {}, got {:?}", | |
| encoding, | |
| header | |
| ); | |
| } | |
| } | |
| }; | |
| check_compression("gzip").await; | |
| check_compression("zstd").await; | |
| check_compression("deflate").await; | |
| } |
Contributor
There was a problem hiding this comment.
r#"string"# isn't a byte string, it's a raw string (i.e. the compiler won't interpret escape characters in it.) But you're right, no need to make a string and then borrow a string slice.
Contributor
There was a problem hiding this comment.
Does the change compile?
I know you sometimes have to move data into closures when dealing with Futures because the .await may cause the future to make further progress on another thread's stack.
Contributor
There was a problem hiding this comment.
Yeah, that's a good question. I was wondering if the borrow checker would be smart enough to see that we're only passing references in this case, and they refer to data that will outlive the lambda. But it's not always able to spot that, so good to check before committing the change.
Looking at this again, the problem might stem from this test case going after 3 distinct things, which may be better to split out into their own test cases. Then the lambda could be promoted to a standalone function that gets called by each of the test cases. But just a suggestion!
This was causing a compile-time error when running `cargo test` with no default features enabled.
We don't conditionally include `set_device` so we don't need to decorate it with the underscore (which prevents "unused symbol" warnings.)
Remove chatty `info` and add a `warn` message when credentials aren't provided during a setting.
Bug fixes
Still debugging settings
This reverts commit 8b9b520.
The filtering of data was working, however I realized that it was using .partition_point() to split the vector into old and new data. The partition function is efficient but relies on the timestamps to be ascending. If DPM inserts a status, it inserts its own timestamp which may be out of order with the front-end's timestamps, resulting in it dropping data points. This commit makes the filtering function work correctly. An additional change was to improve the signature of the filtering function. Before, it passed a `&mut Vec<>` which made it easy to use various `Vec<>` methods. As a result, we were making 4 passes over the data! Now it takes an `Iterator<>` and we (can only) make one pass over the data.
Fix data filter
🚚 use newer module format
Contributor
|
I merged this PR's changes into |
These were due to a mistake when merging `main`.
Code Coverage Report - 3,133 of 6,388 lines covered ( ⛔ 49.05%)
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
compression-zstdandcompression-gzipfeatures totower-httpinCargo.tomlsrc/graphql/compression.rsmodule with acompression_layer()function returning aCompressionLayer(Zstd + Gzip)mod compressionand apply the layer increate_site()insrc/graphql/mod.rscontent-encodingassertion with match-based check — gzip/zstd expect the header, unsupported encodings (e.g. deflate) expect no header