0.1.3 #63
0.1.3 #63
Conversation
WalkthroughVersion bump to 0.1.3 with CI/Makefile tagging updates. Dependencies pinned and upgraded (axum/tower/http/otel). Introduces SharedState, typed wrappers (Port, NatsHost, ClientIp, AuthToken, HttpMethod), client IP extraction, and refactors routes/handlers/events to use new types and state. Config deserialization now validates Port and NatsHost. Tests updated accordingly. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant C as Client
participant Ax as Axum Router
participant H as handler::proxy
participant S as SharedState
participant N as NATS Client
participant B as Backend (via NATS)
Note over Ax,H: New flow using SharedState, HttpRequestInfo, RequestContext
C->>Ax: HTTP request (method, path, headers, body)
Ax->>H: Route match (OriginalUri, Method, Headers, State, Body)
H->>H: extract_client_ip(headers)
H->>H: parse Authorization: Bearer <token>
H->>H: Build HttpRequestInfo + RequestContext
H->>H: Construct HttpReq (typed Uri, Method, AuthToken, ClientIp)
H->>S: Read NATS client (read lock)
H->>N: request_with_headers(HttpReq, req-id)
alt NATS success
N->>B: Deliver request
B-->>N: Reply (payload)
N-->>H: Response bytes
H-->>Ax: HTTP response (status/body)
else NATS error/timeout
N-->>H: Error
H-->>Ax: Error response (JSON)
end
Ax-->>C: HTTP response
Note over H: Metrics recorded via S.metrics
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
Tip 🔌 Remote MCP (Model Context Protocol) integration is now available!Pro plan users can now connect to remote MCP servers from the Integrations page. Connect with popular remote MCPs such as Notion and Linear to add more context to your reviews and chats. ✨ Finishing Touches
🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 34
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (16)
src/responses/errors.rs (2)
3-13: Consider deriving Debug/Clone/PartialEq/Eq on response DTOs.These types are frequently logged, cloned, and compared in tests; deriving them improves ergonomics without runtime cost.
Apply:
-#[derive(Serialize)] +#[derive(Serialize, Debug, Clone, PartialEq, Eq)] pub struct Errors { pub errors: Vec<Error>, } -#[derive(Serialize)] +#[derive(Serialize, Debug, Clone, PartialEq, Eq)] pub struct Error { pub code: String, pub title: String, pub detail: String, }
20-31: Prefer structural JSON assertions over substring checks in tests.Comparing JSON values avoids brittle string matching and ordering issues.
Apply:
- use serde_json; + use serde_json::{self, json}; @@ - let json = serde_json::to_string(&error).expect("Should serialize error"); - assert!(json.contains("\"code\":\"404\"")); - assert!(json.contains("\"title\":\"Not Found\"")); - assert!(json.contains("\"detail\":\"The requested resource was not found\"")); + let val = serde_json::to_value(&error).expect("Should serialize error"); + assert_eq!( + val, + json!({"code":"404","title":"Not Found","detail":"The requested resource was not found"}) + );Repeat similarly for the other tests if you agree.
src/responses/statuses.rs (1)
10-13: Optional: avoid raw identifierr#typewith serde rename.Raw idents are valid but slightly awkward. An alternative is a conventional field name with serde rename.
Example (illustrative):
#[derive(Serialize, Deserialize)] pub struct StatusesData { pub id: String, #[serde(rename = "type")] pub type_: String, pub attributes: Attributes, }No need to change if you prefer the current style.
.github/workflows/main.yml (1)
69-90: Align CI tagging with Makefile behaviorCI is currently only tagging the versioned image (
http2-0.1.3), while the Makefile builds and pushes both the versioned andhttp2-latesttags. To keep CI in sync with the Makefile:• In .github/workflows/main.yml – lines 77 and 88, update both the “Build” and “Build and push” steps to emit two tags:
${{ secrets.DOCKER_HUB_USERNAME }}/swup:http2-${{ env.VERSION }}${{ secrets.DOCKER_HUB_USERNAME }}/swup:http2-latestApply:
- name: Build if: ${{ env.DO_PUSH == 'false' }} id: docker_build uses: docker/build-push-action@v6 with: context: . file: Dockerfile push: false - tags: ${{ secrets.DOCKER_HUB_USERNAME }}/swup:http2-${{ env.VERSION }} + tags: | + ${{ secrets.DOCKER_HUB_USERNAME }}/swup:http2-${{ env.VERSION }} + ${{ secrets.DOCKER_HUB_USERNAME }}/swup:http2-latest cache-from: type=registry,ref=${{ secrets.DOCKER_HUB_USERNAME }}/swup:http2-latest - name: Build and push if: ${{ env.DO_PUSH == 'true' }} id: docker_build_push uses: docker/build-push-action@v6 with: context: . file: Dockerfile push: true - tags: ${{ secrets.DOCKER_HUB_USERNAME }}/swup:http2-${{ env.VERSION }} + tags: | + ${{ secrets.DOCKER_HUB_USERNAME }}/swup:http2-${{ env.VERSION }} + ${{ secrets.DOCKER_HUB_USERNAME }}/swup:http2-latest cache-from: type=registry,ref=${{ secrets.DOCKER_HUB_USERNAME }}/swup:http2-latest cache-to: type=registry,ref=${{ secrets.DOCKER_HUB_USERNAME }}/swup:http2-latest,mode=maxThis ensures CI builds and pushes both tags just like the Makefile does.
Cargo.toml (1)
38-43: OTel/tracing versions are aligned; consider making them optional features.Good pairing: opentelemetry 0.27 + tracing-opentelemetry 0.28 + subscriber 0.3. To keep the minimal runtime and build graph small in non-observability environments, consider gating OTel deps behind a cargo feature (e.g., features = ["observability"]) and defaulting it off.
If you want, I can draft a features stanza and cfg-gate the observability module.
src/routes.rs (2)
21-42: CORS: handle wildcard and preflight nuances; harden error path.
- If allowed_origins contains "", tower-http expects Any rather than a literal "" HeaderValue.
- Consider allow_credentials(true) if you rely on cookies/auth headers.
- Optionally add .vary([AC_REQUEST_HEADERS, ORIGIN]) to help caches.
- let cors_layer = if enable_cors { + let cors_layer = if enable_cors { let mut origins: Vec<HeaderValue> = Vec::new(); - for origin in allowed_origins { + let mut any = false; + for origin in allowed_origins { + if origin == "*" { + any = true; + continue; + } match HeaderValue::from_str(origin.as_str()) { Ok(header_value) => origins.push(header_value), Err(e) => { tracing::error!(error = %e, origin = %origin, "invalid CORS origin header value, skipping"); continue; } } } - Some( - CorsLayer::new() - .allow_origin(origins) + let mut layer = CorsLayer::new() .allow_methods([Method::GET, Method::POST, Method::DELETE, Method::PATCH]) - .allow_headers([header::CONTENT_TYPE, header::AUTHORIZATION]), - ) + .allow_headers([header::CONTENT_TYPE, header::AUTHORIZATION]); + layer = if any { layer.allow_origin(tower_http::cors::Any) } else { layer.allow_origin(origins) }; + Some(layer) } else { None };
44-115: Route table is verbose; consider nesting under API_V1 to reduce repetition.Nesting improves readability and makes version bumps a single-line change.
I can provide a follow-up patch using Router::nest(API_V1, Router::new()...).
src/conf.rs (1)
40-52: Error messages hardcode 'config.json' even when CFG_PATH is setUse the actual path in error messages to simplify troubleshooting.
Apply this diff:
- let file = File::open(path).map_err(|e| ConfError { - message: format!("can't open config.json file, {e}"), + let file = File::open(&path).map_err(|e| ConfError { + message: format!("can't open config file '{}', {e}", path), })?; @@ - buf_reader + buf_reader .read_to_string(&mut contents) .map_err(|e| ConfError { - message: format!("can't read config.json file, {e}"), + message: format!("can't read config file '{}', {e}", path), })?; @@ - let conf: Conf = serde_json::from_str(contents.as_str()).map_err(|e| ConfError { - message: format!("can't parse config.json file, {e}"), + let conf: Conf = serde_json::from_str(contents.as_str()).map_err(|e| ConfError { + message: format!("can't parse config file '{}', {e}", path), })?;tests/routes_test.rs (1)
96-106: Strengthen CORS assertionsSince the CorsLayer explicitly sets allowed methods and headers, assert ACCESS_CONTROL_ALLOW_METHODS and ACCESS_CONTROL_ALLOW_HEADERS as well (your integration tests already do this; duplicating here increases signal if those diverge).
src/main.rs (3)
4-4: Unused import with #![deny(warnings)] will fail the buildfutures::SinkExt isn’t used in this file; with deny(warnings) this becomes a hard error.
Apply:
-use futures::SinkExt;
34-42: Duplicate error logging linesYou print the same observability init error via println! and eprintln!, which is noisy.
Apply:
- println!("ERROR: Failed to initialize observability: {}", e); - eprintln!("ERROR: Failed to initialize observability: {}", e); + eprintln!("ERROR: Failed to initialize observability: {}", e);
88-93: Log message is misleading; not a thread join erroraxum::serve(...).await returns a server result, not a thread join error.
Apply:
- Err(e) => log::error!("thread join error {}", e), + Err(e) => log::error!("server error {}", e),src/handlers.rs (3)
238-276: Bug: Error responses are serialized as MessagePack but labeled JSONcreate_error_response uses rmp_serde::Serializer to produce MessagePack bytes while setting Content-Type to application/vnd.api+json. This violates the advertised content type and will break JSON:API clients.
Switch to serde_json to emit actual JSON:
- let mut buf = Vec::new(); - let mut se = Serializer::new(&mut buf); - - // Handle serialization failure gracefully - if let Err(e) = errors.serialize(&mut se) { - error!(error = %e, "failed to serialize error response"); - // Return a minimal fallback response as JSON string - let fallback_body = format!( - r#"{{"errors":[{{"code":"{}","title":"{}","detail":"Serialization failed"}}]}}"#, - code, title - ); - buf = fallback_body.into_bytes(); - } + let buf = match serde_json::to_vec(&errors) { + Ok(v) => v, + Err(e) => { + error!(error = %e, "failed to serialize error response"); + format!( + r#"{{"errors":[{{"code":"{}","title":"{}","detail":"Serialization failed"}}]}}"#, + code, title + ).into_bytes() + } + }; @@ - Err(e) => { + Err(e) => { error!(error = %e, "failed to build error response"); // Return minimal fallback response - this should never fail - Response::new(Full::from("Internal server error".as_bytes())) + let mut resp = Response::new(Full::from(br#"{"errors":[{"code":"500","title":"Internal Server Error"}]}"#.as_slice())); + resp.headers_mut().insert(CONTENT_TYPE, HeaderValue::from_static(JSON_API_TYPE)); + resp }
88-93: Instrumentation: consider skipping more heavy/PII paramsSkip shared_state and headers to avoid accidentally capturing large/PII data in spans.
Apply:
-#[instrument(skip(body), fields( +#[instrument(skip(body, headers, shared_state), fields(
280-303: Response builder fallbacks: ensure JSON API content-type on all pathsIn the Err branch you return a body without setting Content-Type. Set it to JSON API for consistency.
Apply:
- match Response::builder() + match Response::builder() .status(StatusCode::INTERNAL_SERVER_ERROR) .body(Full::from("Failed to build response".as_bytes())) { Ok(resp) => resp, Err(_) => { // Last resort fallback - this should never fail - Response::new(Full::from("Internal server error".as_bytes())) + let mut resp = Response::new(Full::from("Internal server error".as_bytes())); + resp.headers_mut().insert(CONTENT_TYPE, HeaderValue::from_static(JSON_API_TYPE)); + resp } }src/events.rs (1)
104-135: Strengthen tests: exercise HttpReq::new with a real http::Uri.Current tests build Uri manually and don’t validate the builder logic (scheme/host:port/path/query mapping). Add a test that uses HttpRequestInfo + RequestContext to cover authority() and query extraction.
Here’s a test you can add:
#[test] fn test_http_req_new_builds_uri_correctly() { let http_info = HttpRequestInfo { uri: "http://localhost:8000/api/v1/test?x=1&y=2".parse().unwrap(), method: HttpMethod::Post, matched_path: MatchedPath::new("/api/v1/test"), }; let context = RequestContext { client_ip: ClientIp::new("192.168.1.1".into()), authorization: AuthToken::new("Bearer secret".into()), user_values: HashMap::from([("u".into(), "v".into())]), }; let query_args = HashMap::from([("x".into(), "1".into()), ("y".into(), "2".into())]); let req = HttpReq::new(http_info, context, query_args, b"body"); assert_eq!(req.method, HttpMethod::Post); assert_eq!(req.r#type, "/api/v1/test"); assert_eq!(req.uri.scheme, "http"); assert_eq!(req.uri.host, "localhost:8000"); // relies on authority() assert_eq!(req.uri.path, "/api/v1/test"); assert_eq!(req.uri.query_string, "x=1&y=2"); assert_eq!(req.user_values.get("u").unwrap(), "v"); assert_eq!(req.body, b"body"); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (17)
.github/workflows/main.yml(2 hunks)Cargo.toml(3 hunks)Makefile(2 hunks)http2.iml(1 hunks)src/client_ip.rs(1 hunks)src/conf.rs(7 hunks)src/events.rs(5 hunks)src/handlers.rs(8 hunks)src/lib.rs(2 hunks)src/main.rs(3 hunks)src/responses/errors.rs(1 hunks)src/responses/statuses.rs(1 hunks)src/routes.rs(4 hunks)src/types.rs(1 hunks)tests/client_ip_test.rs(1 hunks)tests/integration_test.rs(9 hunks)tests/routes_test.rs(8 hunks)
🧰 Additional context used
🧬 Code graph analysis (10)
tests/client_ip_test.rs (2)
src/client_ip.rs (1)
extract_client_ip(14-64)src/types.rs (9)
new(18-24)new(45-47)new(85-87)new(192-200)as_str(55-57)as_str(96-98)as_str(203-205)is_empty(61-63)is_empty(102-104)
src/responses/errors.rs (1)
src/responses/mod.rs (1)
errors(1-1)
src/client_ip.rs (2)
src/events.rs (1)
new(54-96)src/types.rs (11)
new(18-24)new(45-47)new(85-87)new(192-200)empty(50-52)empty(90-92)as_str(55-57)as_str(96-98)as_str(203-205)is_empty(61-63)is_empty(102-104)
src/conf.rs (1)
src/types.rs (8)
new(18-24)new(45-47)new(85-87)new(192-200)as_u16(27-29)as_str(55-57)as_str(96-98)as_str(203-205)
tests/routes_test.rs (1)
src/routes.rs (1)
build_routes(16-174)
tests/integration_test.rs (2)
src/routes.rs (1)
build_routes(16-174)src/types.rs (4)
new(18-24)new(45-47)new(85-87)new(192-200)
src/handlers.rs (3)
src/client_ip.rs (1)
extract_client_ip(14-64)src/types.rs (10)
as_str(55-57)as_str(96-98)as_str(203-205)from_axum_method(140-151)empty(50-52)empty(90-92)new(18-24)new(45-47)new(85-87)new(192-200)src/events.rs (1)
new(54-96)
src/main.rs (4)
src/routes.rs (1)
build_routes(16-174)src/signals.rs (1)
listen_signals(7-40)src/conf.rs (1)
new(37-59)src/types.rs (4)
new(18-24)new(45-47)new(85-87)new(192-200)
src/events.rs (1)
src/types.rs (4)
new(18-24)new(45-47)new(85-87)new(192-200)
src/types.rs (2)
src/events.rs (1)
new(54-96)src/metrics.rs (1)
AppMetrics(6-17)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: build
- GitHub Check: test
🔇 Additional comments (23)
src/responses/errors.rs (1)
1-1: Serde derive import migration is correct.Switching to
use serde::Serialize;aligns with modern serde usage and removes theserde_derivedependency duplication. No behavioral change.http2.iml (1)
8-8: Added test source root entry looks fine (if you keep the file).The
testsfolder is correctly marked as a test source root.src/responses/statuses.rs (1)
1-1: Serde derive import migration is correct here as well.Using
serde::{Deserialize, Serialize}keeps derives in one crate and matches the dependency changes.Cargo.toml (4)
3-3: Version bump to 0.1.3: remember to tag and update changelog/release notes.If CI auto-tags, ignore. Otherwise, ensure a git tag v0.1.3 and CHANGELOG entry exist to match Cargo.toml.
Would you like me to open a release checklist item?
21-24: HTTP stack upgrades (axum 0.8/tower 0.5/http 1.1/http-body 1.0) look coherent.APIs used in routes (Router::with_state, MatchedPath via extensions, TraceLayer::make_span_with) are consistent with these versions. No concerns here.
Also applies to: 32-33
26-28: Dependency Usage ConfirmedThe
serde_bytes_wrappercrate is imported and used insrc/events.rs(line 7:use serde_bytes_wrapper::Bytes;), so this dependency is indeed required and should remain inCargo.toml.
46-48: No version mismatch detectedThe
metrics-exporter-prometheuscrate at v0.16.x specifies a dependency onmetricswithreq = "^0.24"(default features enabled), ensuring full compatibility withmetrics = "0.24"as used in your Cargo.toml.• Cargo.toml (lines 46–48):
metrics = "0.24" metrics-exporter-prometheus = "0.16"→ exporter v0.16 pulls in
metrics ^0.24by design, so no registry or trait mismatches will occur.No further action is required here.
src/lib.rs (1)
4-4: New modules exported (client_ip, types): LGTM.Public surface is minimal and deliberate. Ensure downstream crates rely on http2::client_ip and http2::types as intended.
Also applies to: 13-13
tests/client_ip_test.rs (1)
147-190: Add tests asserting we don’t trust CF/XFF unless explicitly configured.If you plan to gate header trust by proxies, encode those expectations here; otherwise add a prominent comment explaining the trust model (e.g., service is always behind Cloudflare/ingress that strips spoofed headers).
Would you like me to add tests that simulate a remote peer not in the trusted list once that feature lands?
src/routes.rs (2)
47-49: Conditional /metrics exposure via shared_state: good.This keeps the endpoint surface minimal in environments without metrics.
11-12: State management via with_state(SharedState): LGTM.The switch away from multiple Extension layers does reduce coupling—and the SharedState injection is correctly applied where needed:
- metrics_handler: takes
State<SharedState>- proxy: takes
State<SharedState>- health_check and not_found don’t require access to shared state, so their signatures without
State<SharedState>are intentional and correct.src/conf.rs (5)
22-24: Type-safe Port in Conf: good upgradeSwitching listen_port to Port with serde validation hardens config parsing and prevents invalid states at the boundary.
32-34: Type-safe NATS host in NatsConf: good upgradeValidating at deserialization time via NatsHost eliminates a whole class of runtime errors.
119-121: LGTM: JSON config round-trip with typed accessorsDeserializing listen_port and nats.host then asserting via as_u16/as_str is correct.
162-167: LGTM: Clone semantics verified on NatsConfEnsures the underlying string is preserved when cloning.
169-183: LGTM: Negative tests strengthen config validationValidating port != 0 and host must contain a port catches common misconfigurations early.
Also applies to: 185-199
tests/routes_test.rs (1)
121-135: BODY_SIZE Constant VerifiedThe
BODY_SIZEconstant is defined as1024 * 250(250 KiB) insrc/routes.rs(line 13), which matches the “250KB” limit described in the test comment. The test then correctly generates a 300 KiB payload to exceed that limit and assert a 413 response. No changes are needed here.src/main.rs (1)
76-79: Binding address from typed port: goodUsing conf.listen_port.as_u16() with explicit 0.0.0.0 keeps the intent clear and aligns with the Port wrapper.
tests/integration_test.rs (2)
52-62: LGTM: Typed Conf construction in testsConstructing Conf using Port and NatsHost mirrors production config parsing and prevents accidental drift.
140-158: CORS preflight coverage is solidValidating origin, methods, and headers covers the main axes of CORS behavior. Nice.
src/handlers.rs (1)
106-109: Client IP logging: double-check privacy postureRecording client IP in spans is useful, but it’s PII. Ensure your data retention and log levels are appropriate for your environment.
If needed, gate this behind a config flag or lower it to trace.
src/events.rs (1)
53-65: Query arg merge strategy overwrites duplicates; confirm intent.Using a single HashMap<String, Bytes> means only one value per key survives. If you need multi-value query params (e.g., ids=1&ids=2), consider HashMap<String, Vec>.
Do you expect multi-valued query parameters? If yes, I can propose a small refactor with parsing helper.
src/types.rs (1)
38-76: ClientIp/AuthToken newtype approach is solid.Transparent serde and small accessors keep the surface tight and explicit. Nice.
| serde_json = "1.0.132" | ||
| futures = "0.3.31" | ||
| libc = "0.2.164" | ||
| http = "1.1.0" | ||
| http-body = "1.0.1" | ||
| http-body-util = "0.1.2" |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Sanity check: orphan or unused new deps.
New pins http-body-util, uuid 1.11, rmp-serde 1.3, rmp 0.8.14, tokio-test 0.4.4, tempfile 3.14.0 all look fine. Let’s confirm they’re referenced to avoid unused dependency lint in future CI.
Also applies to: 49-55, 57-59
🏁 Script executed:
#!/bin/bash
# Look for symbols/usages:
rg -nP --type=rust -C2 '\b(http_body_util|uuid|rmp_serde|rmp|tokio_test|tempfile)\b'Length of output: 1222
🏁 Script executed:
#!/bin/bash
# Verify usage of rmp, tempfile, and tokio-test crates
rg -nP --type=rust -C2 '\brmp::'
rg -nP --type=rust -C2 '\btempfile::'
rg -nP --type=rust -C2 '\btokio_test::'Length of output: 116
Remove or reference orphaned dependencies
The crates rmp, tempfile, and tokio-test do not appear to be used anywhere in the codebase. To prevent unused-dependency errors in CI, please either remove these pins from Cargo.toml or add the corresponding usages.
• Unreferenced crates:
- rmp = "0.8.14" – no calls to
rmp::…found - tempfile = "3.14.0" – no calls to
tempfile::…found - tokio-test = "0.4.4" – no imports or uses of
tokio_test::…found
Locations to update:
- Cargo.toml (lines 49–55) – remove or reference
rmpandtempfile - Cargo.toml (lines 57–59) – remove or reference
tokio-test
🤖 Prompt for AI Agents
In Cargo.toml around lines 49–55 and 57–59, the crates rmp = "0.8.14", tempfile
= "3.14.0" and tokio-test = "0.4.4" are not referenced anywhere; remove their
dependency entries (or move tokio-test into [dev-dependencies] if it was
intended for tests) from Cargo.toml so CI won't fail on unused deps, then run
cargo check/cargo test to ensure nothing breaks and commit the updated
Cargo.toml (and Cargo.lock if it changes).
| <exclude-output /> | ||
| <content url="file://$MODULE_DIR$"> | ||
| <sourceFolder url="file://$MODULE_DIR$/src" isTestSource="false" /> | ||
| <sourceFolder url="file://$MODULE_DIR$/tests" isTestSource="true" /> |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
IDE metadata in VCS — recommend removing .iml from the repo.
JetBrains .iml files are machine/IDE-specific and tend to cause noisy diffs. Consider ignoring them; tests are discovered by Cargo without IDE config.
Add to .gitignore (outside this diff):
# JetBrains
*.iml
.idea/If you intentionally track it, the added tests source folder entry itself looks fine.
🤖 Prompt for AI Agents
In http2.iml around line 8, an IDE-specific JetBrains .iml file was committed
(adds a test sourceFolder); remove this file from version control and stop
tracking IDE metadata by adding patterns to .gitignore (e.g., *.iml and .idea/)
and committing that change, or if you intend to keep IDE files tracked, leave
the file but confirm that is intentional; ensure tests still run via Cargo
without the IDE config.
| @@ -1,3 +1,5 @@ | |||
| VERSION = 0.1.3 | |||
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Source VERSION from Cargo.toml to avoid drift.
Single source of truth prevents manual bumps in multiple places.
Apply:
-VERSION = 0.1.3
+# Derive version from Cargo.toml [package] section
+VERSION ?= $(shell awk 'BEGIN{in_pkg=0} /^\[package\]/{in_pkg=1; next} /^\[/{if(in_pkg){exit}} in_pkg && /version *= *"/{gsub(/.*version *= *"/,""); gsub(/".*/,""); print; exit}' Cargo.toml)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| VERSION = 0.1.3 | |
| # Derive version from Cargo.toml [package] section | |
| VERSION ?= $(shell awk 'BEGIN{in_pkg=0} /^\[package\]/{in_pkg=1; next} /^\[/{if(in_pkg){exit}} in_pkg && /version *= *"/{gsub(/.*version *= *"/,""); gsub(/".*/,""); print; exit}' Cargo.toml) |
🤖 Prompt for AI Agents
Makefile lines 1-1: replace the hardcoded "VERSION = 0.1.3" with a Makefile
variable that reads the version directly from Cargo.toml at build time (e.g.,
set VERSION using a shell command that extracts the value of the version =
"x.y.z" entry from Cargo.toml via sed/awk/grep), ensuring it strips surrounding
quotes and whitespace so the variable contains just the numeric version string;
update any targets that referenced the old static variable to use the new
VERSION variable (no other behavior changes).
| build: | ||
| docker build . -t soulgarden/swup:http2-0.1.1 --platform linux/amd64 | ||
| docker push soulgarden/swup:http2-0.1.1 | ||
| docker build . -t soulgarden/swup:http2-$(VERSION) -t soulgarden/swup:http2-latest --platform linux/amd64 | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Parameterize Docker Hub namespace; keep in sync with CI.
Hard-coding soulgarden diverges from CI’s ${{ secrets.DOCKER_HUB_USERNAME }}. Make it configurable via DOCKER_USER.
Apply:
+DOCKER_USER ?= soulgarden
@@
-build:
- docker build . -t soulgarden/swup:http2-$(VERSION) -t soulgarden/swup:http2-latest --platform linux/amd64
+build:
+ docker build . \
+ -t $(DOCKER_USER)/swup:http2-$(VERSION) \
+ -t $(DOCKER_USER)/swup:http2-latest \
+ --platform linux/amd64🤖 Prompt for AI Agents
In Makefile around lines 29 to 31, the docker image target hard-codes the Docker
Hub namespace "soulgarden" which diverges from CI where the username is
parameterized; update the target to use a DOCKER_USER variable (fallback to a
sensible default) for both tags and ensure VERSION is still used, e.g. replace
the literal namespace with $(DOCKER_USER) and document or export DOCKER_USER in
the Makefile so CI and local builds use the same value.
| push: | ||
| docker push soulgarden/swup:http2-$(VERSION) | ||
| docker push soulgarden/swup:http2-latest |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Add push to .PHONY and use the parameterized namespace here too.
Also, help text currently says “build and push” for the build target; consider updating to reflect the split.
Apply:
-.PHONY: help fmt lint test test-verbose check build
+.PHONY: help fmt lint test test-verbose check build push-push:
- docker push soulgarden/swup:http2-$(VERSION)
- docker push soulgarden/swup:http2-latest
+push:
+ docker push $(DOCKER_USER)/swup:http2-$(VERSION)
+ docker push $(DOCKER_USER)/swup:http2-latestOptional (adjust help description):
- @echo " build - Build and push Docker image"
+ @echo " build - Build Docker image(s)"
+ @echo " push - Push Docker image(s)"🤖 Prompt for AI Agents
In Makefile around lines 32 to 34, the push target is missing from the .PHONY
list and the docker push commands hardcode the soulgarden namespace; add "push"
to the .PHONY declaration and change both docker push lines to use the
VERSIONED_NAMESPACE/IMAGE pattern (replace soulgarden with the existing
parameterized namespace variable used elsewhere, e.g. $(NAMESPACE) or
$(REGISTRY_NAMESPACE)) so pushes follow the same configurable namespace; also
update the help text for the build target to say "build" (or "build" and
separate "push" if you keep a combined help) so it no longer incorrectly reads
"build and push".
| impl NatsHost { | ||
| /// Create a new NatsHost with basic validation | ||
| pub fn new(host: String) -> Result<Self, &'static str> { | ||
| if host.is_empty() { | ||
| Err("NATS host cannot be empty") | ||
| } else if !host.contains(':') { | ||
| Err("NATS host must include port (e.g., 'localhost:4222')") | ||
| } else { | ||
| Ok(NatsHost(host)) | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
🧹 Nitpick (assertive)
NatsHost validation is too weak (IPv6, schemes).
Checking only “contains(':')” rejects bracketed IPv6 or URLs with scheme (e.g., nats://host:4222) and accepts invalid forms. Prefer a stricter parser.
Two options:
- If you accept URLs: parse with url::Url and validate scheme == "nats".
- If you accept host:port: validate authority with http::uri::Authority::from_str or accept “[::1]:4222” forms.
I can propose a targeted validator once we confirm the accepted formats in conf.
🤖 Prompt for AI Agents
In src/types.rs around lines 190-201, replace the fragile host.contains(':')
check with proper parsing: attempt to parse the input as a URL first using
url::Url::parse — if parse succeeds require scheme == "nats" and that host and
port are present — otherwise fall back to parsing as a socket address using
std::net::SocketAddr::from_str (this accepts IPv4, IPv6 with brackets, and
port); return Err on parse failure or missing port, and Ok(NatsHost(host)) only
when one of these parsers validates successfully.
| #[cfg(test)] | ||
| mod tests { | ||
| use super::*; | ||
|
|
||
| #[test] | ||
| fn test_port_validation() { | ||
| assert!(Port::new(0).is_err()); | ||
| assert!(Port::new(8080).is_ok()); | ||
| assert_eq!(Port::new(8080).unwrap().as_u16(), 8080); | ||
| } | ||
|
|
||
| #[test] | ||
| fn test_client_ip() { | ||
| let ip = ClientIp::new("192.168.1.1".to_string()); | ||
| assert_eq!(ip.as_str(), "192.168.1.1"); | ||
| assert!(!ip.is_empty()); | ||
|
|
||
| let empty_ip = ClientIp::empty(); | ||
| assert!(empty_ip.is_empty()); | ||
| } | ||
|
|
||
| #[test] | ||
| fn test_auth_token() { | ||
| let token = AuthToken::new("Bearer abc123".to_string()); | ||
| assert_eq!(token.as_str(), "Bearer abc123"); | ||
| assert!(!token.is_empty()); | ||
|
|
||
| let empty_token = AuthToken::empty(); | ||
| assert!(empty_token.is_empty()); | ||
| } | ||
|
|
||
| #[test] | ||
| fn test_http_method_from_string() { | ||
| assert_eq!(HttpMethod::from_str("GET").unwrap(), HttpMethod::Get); | ||
| assert_eq!(HttpMethod::from_str("post").unwrap(), HttpMethod::Post); | ||
| assert!(HttpMethod::from_str("INVALID").is_err()); | ||
| } | ||
|
|
||
| #[test] | ||
| fn test_http_method_display() { | ||
| assert_eq!(HttpMethod::Get.to_string(), "GET"); | ||
| assert_eq!(HttpMethod::Post.to_string(), "POST"); | ||
| } | ||
|
|
||
| #[test] | ||
| fn test_nats_host_validation() { | ||
| assert!(NatsHost::new("".to_string()).is_err()); | ||
| assert!(NatsHost::new("localhost".to_string()).is_err()); // No port | ||
| assert!(NatsHost::new("localhost:4222".to_string()).is_ok()); | ||
| assert_eq!( | ||
| NatsHost::new("localhost:4222".to_string()) | ||
| .unwrap() | ||
| .as_str(), | ||
| "localhost:4222" | ||
| ); | ||
| } | ||
|
|
||
| #[test] | ||
| fn test_serde_serialization() { | ||
| let port = Port::new(8080).unwrap(); | ||
| let serialized = serde_json::to_string(&port).unwrap(); | ||
| assert_eq!(serialized, "8080"); | ||
|
|
||
| let client_ip = ClientIp::new("192.168.1.1".to_string()); | ||
| let serialized = serde_json::to_string(&client_ip).unwrap(); | ||
| assert_eq!(serialized, "\"192.168.1.1\""); | ||
|
|
||
| let method = HttpMethod::Get; | ||
| let serialized = serde_json::to_string(&method).unwrap(); | ||
| assert_eq!(serialized, "\"GET\""); | ||
| } | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Tests look good; add cases for TRACE/CONNECT and redaction.
Extend tests to cover the new methods and ensure AuthToken Debug is redacted.
Example additions:
#[test]
fn test_http_method_trace_connect() {
assert_eq!(HttpMethod::from_str("TRACE").unwrap(), HttpMethod::Trace);
assert_eq!(HttpMethod::from_str("CONNECT").unwrap(), HttpMethod::Connect);
assert_eq!(HttpMethod::Trace.to_string(), "TRACE");
assert_eq!(HttpMethod::Connect.to_string(), "CONNECT");
}
#[test]
fn test_auth_token_redaction() {
let t = AuthToken::new("secret-token-123".into());
assert_eq!(format!("{:?}", t), "AuthToken(\"***REDACTED***\")");
assert!(format!("{}", t).ends_with("…REDACTED"));
}🤖 Prompt for AI Agents
In src/types.rs around lines 220-291, tests are missing coverage for the TRACE
and CONNECT HTTP methods and for AuthToken redaction; add a test function that
verifies HttpMethod::from_str accepts "TRACE" and "CONNECT" and that their
Display outputs "TRACE" and "CONNECT", and add a test that constructs an
AuthToken("secret-token-123") and asserts Debug prints exactly
AuthToken("***REDACTED***") and Display contains the redaction suffix (ends with
"…REDACTED") to ensure sensitive tokens are not exposed.
| #[tokio::test] | ||
| async fn test_client_ip_extraction_integration() { |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Async not required; switch to plain #[test] to speed tests.
These tests don’t use async. Dropping tokio reduces overhead and avoids needing a runtime in integration tests.
-#[tokio::test]
-async fn test_client_ip_extraction_integration() {
+#[test]
+fn test_client_ip_extraction_integration() {
@@
-#[tokio::test]
-async fn test_real_world_scenarios() {
+#[test]
+fn test_real_world_scenarios() {
@@
-#[tokio::test]
-async fn test_security_scenarios() {
+#[test]
+fn test_security_scenarios() {🤖 Prompt for AI Agents
In tests/client_ip_test.rs around lines 5 to 6, the test is annotated with
#[tokio::test] but does not use async functionality; change it to a synchronous
test by replacing #[tokio::test] with the standard #[test] and remove the async
keyword from the function signature so the test runs without spawning a Tokio
runtime, speeding up execution and avoiding unnecessary async runtime setup.
| let shared_state = SharedState { | ||
| nats: mock_client, | ||
| metrics: None, | ||
| }; | ||
| let app = build_routes(allowed_origins, true, shared_state); | ||
|
|
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Factor out test app creation
Same DRY suggestion as routes tests: construct SharedState and Router via a helper to reduce repetition.
Proposed helper (in this file):
async fn build_test_app(allowed: Vec<String>) -> Option<axum::Router> {
if let Some(nats) = create_test_nats_client().await {
let state = SharedState { nats, metrics: None };
Some(build_routes(allowed, true, state))
} else {
None
}
}Then replace repeated blocks with a single call to build_test_app.
Also applies to: 67-74, 88-93, 124-131, 168-173, 203-208, 231-235
🤖 Prompt for AI Agents
In tests/integration_test.rs around lines 19 to 24 (and similarly at 67-74,
88-93, 124-131, 168-173, 203-208, 231-235), the test repeatedly constructs
SharedState and builds the Router inline; factor this into a single async helper
function (e.g., build_test_app) that creates the test NATS client, constructs
SharedState with metrics: None, and returns Some(build_routes(allowed, true,
state)) or None on failure, then replace each repeated block with a call to that
helper to reduce duplication and improve readability.
| let shared_state = SharedState { | ||
| nats: mock_client, | ||
| metrics: None, | ||
| }; | ||
| let app = build_routes(allowed_origins, true, shared_state); | ||
|
|
There was a problem hiding this comment.
🧹 Nitpick (assertive)
DRY the SharedState/app construction across tests
The repeated pattern creating SharedState and calling build_routes adds noise and maintenance burden.
Add a small helper in this file:
async fn build_test_app(allowed_origins: Vec<String>) -> Option<axum::Router> {
if let Some(mock_client) = create_test_nats_client().await {
let state = SharedState { nats: mock_client, metrics: None };
Some(build_routes(allowed_origins, true, state))
} else {
None
}
}Then replace blocks like:
- let shared_state = SharedState {
- nats: mock_client,
- metrics: None,
- };
- let app = build_routes(allowed_origins, true, shared_state);
+ let app = build_test_app(allowed_origins).await.expect("NATS unavailable");If you want to keep the graceful skip, unwrap_or_return in the test body instead of expect.
Also applies to: 55-60, 83-87, 115-120, 145-150, 164-168, 196-199
🤖 Prompt for AI Agents
In tests/routes_test.rs around lines 28-33 (also apply to ranges 55-60, 83-87,
115-120, 145-150, 164-168, 196-199), the SharedState and build_routes
construction is duplicated; add a small async helper fn
build_test_app(allowed_origins: Vec<String>) -> Option<axum::Router> that awaits
create_test_nats_client(), constructs SharedState { nats: mock_client, metrics:
None } and returns Some(build_routes(allowed_origins, true, state)) or None on
missing client, then replace each repeated block with a call to that helper and
in tests use unwrap_or_return to gracefully skip when None (instead of expect)
so tests keep existing skip behavior.
Summary by CodeRabbit
New Features
Refactor
Chores
Tests