Dependencies update and tests #62
Conversation
Removed RabbitMQ and its related components (broker, publisher, subscriber, rmq) in favor of NATS for messaging. Simplified the codebase by replacing redundant queue handling logic, updated route handling, and improved error reporting and response handling with NATS integration. Updated versioning and build scripts accordingly.
WalkthroughIntroduces a NATS-based request proxy, optional Prometheus metrics, and a new observability subsystem. Expands routing with conditional CORS and a /metrics endpoint. Adds tests across handlers, routes, events, and responses. Updates CI to include tests, bumps crate version and Docker base/tag, refactors signals handling, and exposes modules via lib.rs. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Axum as Axum Router
participant Proxy as handlers::proxy
participant NATS as NATS Client
participant Broker as NATS Subject "http"
Client->>Axum: HTTP request
Axum->>Proxy: Route match (method, path, body, headers)
Proxy->>Proxy: Build HttpReq (MsgPack), trace & metrics start
Proxy->>NATS: request("http", payload, headers)
NATS->>Broker: Publish request
Broker-->>NATS: MsgPack reply (headers: code?)
NATS-->>Proxy: Response
Proxy->>Proxy: Decode, map code→Status, record metrics
Proxy-->>Axum: IntoResponse
Axum-->>Client: HTTP response
sequenceDiagram
participant Main as main
participant Obs as observability::init_observability
participant Routes as routes::build_routes
participant Signals as signals::server_shutdown_notify
Main->>Obs: initialize tracing + metrics
Main->>Routes: build_router(allowed_origins, enable_cors, nats, metrics)
Main->>Main: start server (graceful shutdown)
Signals-->>Main: notify shutdown
Main->>Obs: shutdown_observability
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 (
|
# Conflicts: # .github/workflows/main.yml # Cargo.lock # Cargo.toml # Dockerfile # Makefile # src/conf.rs # src/handlers.rs
soulgarden
changed the title
- Add enable_cors configuration field support with conditional middleware - Add comprehensive observability system with tracing and metrics infrastructure - Implement structured JSON logging with request correlation and route templates - Add Prometheus metrics collection for HTTP and NATS requests - Optimize log output: single informative log per request with timing - Update tests for new build_routes signature and configuration structure
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 30
🔭 Outside diff range comments (3)
src/events.rs (1)
64-83: Avoid intermediate String allocations when building byte vectorsUse .as_bytes().to_vec() where possible. It’s simpler and avoids extra String allocations.
Apply this diff:
- scheme: match uri.scheme() { - None => "".to_string().into_bytes(), - Some(s) => s.as_str().to_string().into_bytes(), - }, - path: uri.path().to_string().into_bytes(), + scheme: match uri.scheme() { + None => Vec::new(), + Some(s) => s.as_str().as_bytes().to_vec(), + }, + path: uri.path().as_bytes().to_vec(), query_string: match uri.path_and_query() { None => "".to_string().into_bytes(), Some(path_and_query) => match path_and_query.query() { None => "".to_string().into_bytes(), Some(q) => q.to_string().into_bytes(), }, }, hash: &[], - host: match uri.host() { - None => "".to_string().into_bytes(), - Some(h) => h.to_string().into_bytes(), - }, + host: match uri.host() { + None => Vec::new(), + Some(h) => h.as_bytes().to_vec(), + },src/conf.rs (2)
37-39: Error messages hardcode "config.json"; include the actual path for clarityWhen
CFG_PATHis set, the error still says "config.json", which is misleading. Use thepathvalue in all error messages.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 at '{}', {e}", path), })?; @@ - .map_err(|e| ConfError { - message: format!("can't read config.json file, {e}"), - })?; + .map_err(|e| ConfError { + message: format!("can't read config file at '{}', {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).map_err(|e| ConfError { + message: format!("can't parse config file at '{}', {e}", path), + })?;Also applies to: 47-49, 51-53
8-17: Implement std::error::Error for ConfErrorThis will integrate better with error handling and
?propagation chains.Apply this diff:
#[derive(Debug, Clone)] pub struct ConfError { pub message: String, } impl fmt::Display for ConfError { fn fmt(&self, f: &mut fmt::Formatter) -> std::fmt::Result { write!(f, "ConfError: {}", self.message) } } + +impl std::error::Error for ConfError {}
📜 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 (15)
Cargo.toml(3 hunks)src/conf.rs(2 hunks)src/events.rs(2 hunks)src/handlers.rs(7 hunks)src/lib.rs(1 hunks)src/main.rs(4 hunks)src/metrics.rs(1 hunks)src/observability.rs(1 hunks)src/responses/errors.rs(1 hunks)src/responses/statuses.rs(1 hunks)src/routes.rs(1 hunks)src/signals.rs(1 hunks)tests/handlers_test.rs(1 hunks)tests/integration_test.rs(1 hunks)tests/routes_test.rs(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (8)
tests/handlers_test.rs (1)
src/handlers.rs (2)
health_check(30-45)not_found(47-54)
src/observability.rs (2)
src/conf.rs (2)
fmt(14-16)new(34-56)src/metrics.rs (1)
new(20-36)
tests/routes_test.rs (1)
src/routes.rs (2)
build_routes(19-180)request(165-167)
src/metrics.rs (2)
src/conf.rs (1)
new(34-56)src/events.rs (1)
new(43-86)
src/handlers.rs (3)
src/events.rs (1)
new(43-86)src/metrics.rs (1)
new(20-36)src/routes.rs (1)
request(165-167)
tests/integration_test.rs (3)
src/routes.rs (2)
build_routes(19-180)request(165-167)src/conf.rs (1)
new(34-56)src/metrics.rs (1)
new(20-36)
src/main.rs (3)
src/observability.rs (2)
init_observability(6-60)shutdown_observability(62-64)src/routes.rs (1)
build_routes(19-180)src/signals.rs (1)
listen_signals(7-40)
src/routes.rs (2)
src/metrics.rs (1)
new(20-36)src/handlers.rs (4)
health_check(30-45)metrics_handler(56-88)proxy(96-226)not_found(47-54)
⏰ 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: test
- GitHub Check: test
🔇 Additional comments (25)
src/signals.rs (1)
11-24: Robust signal registration and non-panicking flow look goodReplacing unwrap with a fallible registration and spawning per-signal listeners is correct and prevents crashes if one registration fails.
src/responses/errors.rs (1)
15-80: Good, focused serialization tests for Error and ErrorsThe tests exercise single, multiple, empty, and empty-fields cases and are straightforward and stable.
src/events.rs (2)
70-75: Correctly avoid potential panic on missing path/querySwitching from unwrap to nested matches on path_and_query() and .query() is the right defensive fix.
89-157: No missing rmp-serde dependency—tests will compile as-isThe project’s Cargo.toml already declares
rmp-serde = "^1.1"under
[dependencies], sormp_serde::to_vecis available to both the library and its tests. No changes to Cargo.toml are required.src/responses/statuses.rs (1)
1-16: Adding Deserialize derive is correct and unblocks round-trip testsDeriving Deserialize across these types aligns with the JSON API usage and keeps types symmetrical.
src/conf.rs (3)
118-127: Consider keepingenable_corsdefaulted while still testing missing required fieldsWith
enable_corsdefaulted, this test remains valid (other required fields are still missing). No action needed if you apply the default.
129-136: LGTM:ConfErrorDisplay formatting test is preciseClear assertion that guards the human-readable error formatting.
138-146: LGTM:NatsConfcloning behaves as expectedCloning semantics are validated properly.
src/lib.rs (2)
2-2: Good hardening:#![forbid(unsafe_code)]This is a sensible crate-level guarantee.
4-11: Public modules export looks consistent with the PR surfaceExposing conf, routes, handlers, metrics, observability, responses, and signals aligns with the new API.
tests/handlers_test.rs (1)
17-25: LGTM: not_found handler contract is verifiedStatus code and JSON:API content type are properly asserted.
tests/routes_test.rs (2)
139-156:test_route_methodsmay block up to the NATS request timeoutGiven the handler uses NATS request timeouts, this test can still take hundreds of ms per run. If stability is a concern, consider mocking the proxy or feature-gating NATS-dependent tests.
Would you like me to provide a mockable
proxyhandler behind a feature flag for tests, or a test helper service that short-circuits without NATS?
23-44: LGTM: Health and Not Found route tests validate status and media typeThe basic routing contract looks solid and aligned with JSON:API expectations.
Also applies to: 46-67
src/routes.rs (4)
38-46: CORS layer composition looks correct; ensure preflight expectations in tests match default 204 behavior
tower_httpwill return 204 on preflight. Align tests accordingly (see routes_test.rs comment).
48-53: Conditional /metrics route based on presence of metrics is a good toggleThis keeps the surface minimal in environments that don’t expose Prometheus.
151-154: Layer ordering is sensible; CORS before Trace ensures preflights are traced at the outer layerNice attention to middleware ordering. Body size limit is applied inner-most which is also appropriate.
161-179: Great use of TraceLayer with MatchedPath to capture route templatesCapturing method, route template, and version adds valuable context to spans.
Cargo.toml (4)
3-3: LGTM! Version bump is appropriateThe version bump from 0.1.0 to 0.1.1 reflects the addition of new features (metrics, observability).
7-14: LGTM! Binary and library targets correctly configuredThe addition of both binary and library targets enables the crate to be used both as a standalone application and as a library, which is appropriate for the testing and modular architecture introduced in this PR.
36-46: LGTM! Observability dependencies are well chosenThe OpenTelemetry and Prometheus dependencies are industry-standard choices for observability. The versions are compatible and the feature flags are appropriate.
55-59: LGTM! Dev dependencies support the new test suiteThe dev dependencies are appropriate for the integration tests being added.
tokio-testfor async testing,tempfilefor test fixtures, andtowerwith "util" feature for test utilities are all good choices.src/main.rs (1)
87-89: LGTM! Proper shutdown sequenceThe shutdown sequence correctly closes the NATS client before shutting down observability, ensuring proper resource cleanup.
src/handlers.rs (3)
169-179: LGTM! Good fallback handling for status codesThe code properly handles missing or invalid status codes with appropriate logging and fallback to 500.
245-254: LGTM! Excellent error handling for serialization failuresThe fallback JSON string ensures the error response is always valid, even when MessagePack serialization fails.
155-167: Revisit Missing NATS Headers HandlingThe match on
response.headersin src/handlers.rs:155–167 currently treats a missing header map as a fatal error (500). Our grep/ast-grep didn’t find any other places where NATS replies might omit headers, and tests only verify HTTP response headers, not NATS ones. Before keeping this as a hard failure, confirm whether your NATS service truly always returns headers in every reply.If not, consider defaulting to an empty
HeaderMapand logging a warning instead of bailing out:- let headers = match response.headers { - Some(headers) => headers, - None => { - error!(request_id = %id, "NATS response missing headers"); - return create_error_response( - StatusCode::INTERNAL_SERVER_ERROR, - "500", - "Internal server error", - "Invalid NATS response format", - ) - .into_response(); - } - }; + let headers = response + .headers + .unwrap_or_else(|| { + warn!(request_id = %id, "NATS response missing headers – proceeding with defaults"); + HeaderMap::new() + });• If your NATS backend guarantees headers on every response, you can leave the fatal error in place.
• Otherwise, switch to the pattern above to avoid unnecessary 500s when headers are legitimately absent.
| #[derive(Debug, Deserialize)] | ||
| pub struct Conf { | ||
| pub listen_port: u16, | ||
| pub enable_cors: bool, | ||
| pub nats: NatsConf, | ||
| pub allowed_origins: Vec<String>, | ||
| pub is_debug: bool, | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Adding a required enable_cors boolean is a breaking change; default it to false to preserve backward compatibility
Existing deployments with config.json lacking enable_cors will now fail to parse. Make this field optional with a default to avoid breaking existing configs.
Apply this diff:
pub struct Conf {
pub listen_port: u16,
- pub enable_cors: bool,
+ #[serde(default)]
+ pub enable_cors: bool,
pub nats: NatsConf,
pub allowed_origins: Vec<String>,
pub is_debug: bool,
}🤖 Prompt for AI Agents
In src/conf.rs around lines 19 to 26, the new required field `enable_cors: bool`
is a breaking change; change it to use Serde's field default so missing configs
parse as false. Add the attribute #[serde(default)] to the `enable_cors` field
(or an explicit default function wired with #[serde(default = "default_false")]
returning false) so deserialization sets it to false when absent; keep the type
as bool and no other structural changes are needed.
| assert_eq!(test_conf.listen_port, 8080); | ||
| assert_eq!(test_conf.nats.host, "localhost:4222"); | ||
| assert_eq!(test_conf.allowed_origins, vec!["http://localhost:3000"]); | ||
| assert!(test_conf.is_debug); | ||
| } |
There was a problem hiding this comment.
Unit test mismatch: comparing Vec to Vec<&str> will not compile
Use String values on the right-hand side.
Apply this diff:
- assert_eq!(test_conf.allowed_origins, vec!["http://localhost:3000"]);
+ assert_eq!(
+ test_conf.allowed_origins,
+ vec!["http://localhost:3000".to_string()]
+ );📝 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.
| assert_eq!(test_conf.listen_port, 8080); | |
| assert_eq!(test_conf.nats.host, "localhost:4222"); | |
| assert_eq!(test_conf.allowed_origins, vec!["http://localhost:3000"]); | |
| assert!(test_conf.is_debug); | |
| } | |
| assert_eq!(test_conf.listen_port, 8080); | |
| assert_eq!(test_conf.nats.host, "localhost:4222"); | |
| assert_eq!( | |
| test_conf.allowed_origins, | |
| vec!["http://localhost:3000".to_string()] | |
| ); | |
| assert!(test_conf.is_debug); |
🤖 Prompt for AI Agents
In src/conf.rs around lines 76 to 80, the unit test compares
test_conf.allowed_origins (Vec<String>) to vec!["http://localhost:3000"]
(Vec<&str>) which fails to compile; change the RHS to own Strings by using
vec![String::from("http://localhost:3000")] or
vec!["http://localhost:3000".to_string()] so both sides are Vec<String>, and
update the assertion accordingly.
| let conf: Result<Conf, _> = serde_json::from_str(config_json); | ||
| assert!(conf.is_ok()); | ||
| let conf = conf.unwrap(); | ||
| assert_eq!(conf.listen_port, 8080); | ||
| assert_eq!(conf.nats.host, "localhost:4222"); | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Expand assertions to cover newly added enable_cors and other fields
The JSON contains enable_cors and allowed_origins, but the test doesn’t assert them.
Apply this diff:
let conf = conf.unwrap();
assert_eq!(conf.listen_port, 8080);
assert_eq!(conf.nats.host, "localhost:4222");
+ assert!(conf.enable_cors);
+ assert_eq!(
+ conf.allowed_origins,
+ vec!["http://localhost:3000".to_string()]
+ );
+ assert!(conf.is_debug);🤖 Prompt for AI Agents
In src/conf.rs around lines 95 to 100, the test parses config_json but only
asserts listen_port and nats.host; it must be expanded to assert the newly added
fields. Update the test to assert that conf.enable_cors equals the value present
in config_json and that conf.allowed_origins equals the expected list from
config_json, and add similar assertions for any other new fields added to Conf
so the parsed struct fully matches the input JSON.
| pub async fn metrics_handler( | ||
| Extension(metrics): Extension<Option<Arc<AppMetrics>>>, | ||
| ) -> impl IntoResponse { | ||
| match metrics { | ||
| Some(metrics) => match metrics.render().await { | ||
| Ok(body) => match Response::builder() | ||
| .status(StatusCode::OK) | ||
| .header("Content-Type", "text/plain; version=0.0.4; charset=utf-8") | ||
| .body(body) | ||
| { | ||
| Ok(response) => response.into_response(), | ||
| Err(e) => { | ||
| error!(error = %e, "failed to build metrics response"); | ||
| Response::builder() | ||
| .status(StatusCode::INTERNAL_SERVER_ERROR) | ||
| .body("Internal server error".to_string()) | ||
| .unwrap_or_else(|_| Response::new("Fatal error".to_string())) | ||
| .into_response() | ||
| } | ||
| }, | ||
| Err(_) => Response::builder() | ||
| .status(StatusCode::INTERNAL_SERVER_ERROR) | ||
| .body("Error rendering metrics".to_string()) | ||
| .unwrap_or_else(|_| Response::new("Error rendering metrics".to_string())) | ||
| .into_response(), | ||
| }, | ||
| None => Response::builder() | ||
| .status(StatusCode::SERVICE_UNAVAILABLE) | ||
| .body("Metrics not available".to_string()) | ||
| .unwrap_or_else(|_| Response::new("Metrics not available".to_string())) | ||
| .into_response(), | ||
| } | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Improve metrics handler error handling consistency
The metrics handler has inconsistent error handling with multiple unwrap_or_else calls that could be simplified.
pub async fn metrics_handler(
Extension(metrics): Extension<Option<Arc<AppMetrics>>>,
) -> impl IntoResponse {
match metrics {
Some(metrics) => match metrics.render().await {
- Ok(body) => match Response::builder()
+ Ok(body) => Response::builder()
.status(StatusCode::OK)
.header("Content-Type", "text/plain; version=0.0.4; charset=utf-8")
.body(body)
- {
- Ok(response) => response.into_response(),
- Err(e) => {
- error!(error = %e, "failed to build metrics response");
- Response::builder()
- .status(StatusCode::INTERNAL_SERVER_ERROR)
- .body("Internal server error".to_string())
- .unwrap_or_else(|_| Response::new("Fatal error".to_string()))
- .into_response()
- }
- },
- Err(_) => Response::builder()
+ .unwrap_or_else(|e| {
+ error!(error = %e, "failed to build metrics response");
+ Response::builder()
+ .status(StatusCode::INTERNAL_SERVER_ERROR)
+ .body("Internal server error".to_string())
+ .unwrap_or_else(|_| Response::new("Fatal error".to_string()))
+ })
+ .into_response(),
+ Err(e) => {
+ error!(error = %e, "failed to render metrics");
+ Response::builder()
.status(StatusCode::INTERNAL_SERVER_ERROR)
.body("Error rendering metrics".to_string())
.unwrap_or_else(|_| Response::new("Error rendering metrics".to_string()))
.into_response()
+ }
},
None => Response::builder()
.status(StatusCode::SERVICE_UNAVAILABLE)
.body("Metrics not available".to_string())
.unwrap_or_else(|_| Response::new("Metrics not available".to_string()))
.into_response(),
}
}📝 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.
| pub async fn metrics_handler( | |
| Extension(metrics): Extension<Option<Arc<AppMetrics>>>, | |
| ) -> impl IntoResponse { | |
| match metrics { | |
| Some(metrics) => match metrics.render().await { | |
| Ok(body) => match Response::builder() | |
| .status(StatusCode::OK) | |
| .header("Content-Type", "text/plain; version=0.0.4; charset=utf-8") | |
| .body(body) | |
| { | |
| Ok(response) => response.into_response(), | |
| Err(e) => { | |
| error!(error = %e, "failed to build metrics response"); | |
| Response::builder() | |
| .status(StatusCode::INTERNAL_SERVER_ERROR) | |
| .body("Internal server error".to_string()) | |
| .unwrap_or_else(|_| Response::new("Fatal error".to_string())) | |
| .into_response() | |
| } | |
| }, | |
| Err(_) => Response::builder() | |
| .status(StatusCode::INTERNAL_SERVER_ERROR) | |
| .body("Error rendering metrics".to_string()) | |
| .unwrap_or_else(|_| Response::new("Error rendering metrics".to_string())) | |
| .into_response(), | |
| }, | |
| None => Response::builder() | |
| .status(StatusCode::SERVICE_UNAVAILABLE) | |
| .body("Metrics not available".to_string()) | |
| .unwrap_or_else(|_| Response::new("Metrics not available".to_string())) | |
| .into_response(), | |
| } | |
| } | |
| pub async fn metrics_handler( | |
| Extension(metrics): Extension<Option<Arc<AppMetrics>>>, | |
| ) -> impl IntoResponse { | |
| match metrics { | |
| Some(metrics) => match metrics.render().await { | |
| Ok(body) => Response::builder() | |
| .status(StatusCode::OK) | |
| .header("Content-Type", "text/plain; version=0.0.4; charset=utf-8") | |
| .body(body) | |
| .unwrap_or_else(|e| { | |
| error!(error = %e, "failed to build metrics response"); | |
| Response::builder() | |
| .status(StatusCode::INTERNAL_SERVER_ERROR) | |
| .body("Internal server error".to_string()) | |
| .unwrap_or_else(|_| Response::new("Fatal error".to_string())) | |
| }) | |
| .into_response(), | |
| Err(e) => { | |
| error!(error = %e, "failed to render metrics"); | |
| Response::builder() | |
| .status(StatusCode::INTERNAL_SERVER_ERROR) | |
| .body("Error rendering metrics".to_string()) | |
| .unwrap_or_else(|_| Response::new("Error rendering metrics".to_string())) | |
| .into_response() | |
| } | |
| }, | |
| None => Response::builder() | |
| .status(StatusCode::SERVICE_UNAVAILABLE) | |
| .body("Metrics not available".to_string()) | |
| .unwrap_or_else(|_| Response::new("Metrics not available".to_string())) | |
| .into_response(), | |
| } | |
| } |
| @@ -0,0 +1,11 @@ | |||
| #![deny(warnings)] | |||
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Consider gating #![deny(warnings)] to avoid brittle builds
Denying warnings at the crate root can cause unrelated failures under newer toolchains. Prefer gating to non-test, non-dev builds or enforcing via CI.
Apply this diff:
-#![deny(warnings)]
+#![cfg_attr(all(not(test), not(debug_assertions)), deny(warnings))]📝 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.
| #![deny(warnings)] | |
| #![cfg_attr(all(not(test), not(debug_assertions)), deny(warnings))] |
🤖 Prompt for AI Agents
In src/lib.rs at line 1, the crate-level attribute `#![deny(warnings)]` is too
strict and can break builds under newer toolchains; instead gate this directive
so it only applies to non-test/non-dev builds (or remove it and enforce
warnings-as-errors in CI). Replace the unconditional deny with a conditional
attribute that applies only when not running tests or debug/dev builds (e.g.,
use cfg_attr to enable deny(warnings) for release/CI builds), or remove the
attribute and configure the CI toolchain to treat warnings as errors.
| async fn create_test_nats_client() -> Option<Arc<RwLock<Client>>> { | ||
| let options = async_nats::ConnectOptions::new() | ||
| .ping_interval(std::time::Duration::from_secs(10)) | ||
| .request_timeout(Some(std::time::Duration::from_secs(10))); | ||
|
|
||
| match options.connect("nats://localhost:4222").await { | ||
| Ok(client) => Some(Arc::new(RwLock::new(client))), | ||
| Err(_) => None, // NATS not available for testing | ||
| } | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Consider improving error handling in test helper
The create_test_nats_client() helper silently returns None when NATS is unavailable. Consider logging the error for debugging purposes.
async fn create_test_nats_client() -> Option<Arc<RwLock<Client>>> {
let options = async_nats::ConnectOptions::new()
.ping_interval(std::time::Duration::from_secs(10))
.request_timeout(Some(std::time::Duration::from_secs(10)));
match options.connect("nats://localhost:4222").await {
Ok(client) => Some(Arc::new(RwLock::new(client))),
- Err(_) => None, // NATS not available for testing
+ Err(e) => {
+ eprintln!("NATS connection failed: {}", e);
+ None // NATS not available for testing
+ }
}
}📝 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.
| async fn create_test_nats_client() -> Option<Arc<RwLock<Client>>> { | |
| let options = async_nats::ConnectOptions::new() | |
| .ping_interval(std::time::Duration::from_secs(10)) | |
| .request_timeout(Some(std::time::Duration::from_secs(10))); | |
| match options.connect("nats://localhost:4222").await { | |
| Ok(client) => Some(Arc::new(RwLock::new(client))), | |
| Err(_) => None, // NATS not available for testing | |
| } | |
| } | |
| async fn create_test_nats_client() -> Option<Arc<RwLock<Client>>> { | |
| let options = async_nats::ConnectOptions::new() | |
| .ping_interval(std::time::Duration::from_secs(10)) | |
| .request_timeout(Some(std::time::Duration::from_secs(10))); | |
| match options.connect("nats://localhost:4222").await { | |
| Ok(client) => Some(Arc::new(RwLock::new(client))), | |
| Err(e) => { | |
| eprintln!("NATS connection failed: {}", e); | |
| None // NATS not available for testing | |
| } | |
| } | |
| } |
🤖 Prompt for AI Agents
In tests/integration_test.rs around lines 223 to 232, the
create_test_nats_client() helper swallows the connection error and returns None
silently; change the Err(_) arm to capture the error (Err(err)) and log it
before returning None — e.g., call eprintln! or use the crate's test logger
(tracing::error! or log::error!) to print a clear message including err, so test
runs show why NATS connection failed while still returning None for unavailable
NATS.
| #[allow(dead_code)] | ||
| fn nats_available() -> bool { | ||
| // In real tests, you'd check if NATS is available | ||
| // For now, assume it's not available in test environments | ||
| false | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Remove or implement the nats_available() helper function
The nats_available() function is marked with #[allow(dead_code)] and always returns false. It's not used anywhere in the tests, making it dead code that should be removed or properly implemented if needed.
-// Helper function to skip integration tests if NATS is not available
-#[allow(dead_code)]
-fn nats_available() -> bool {
- // In real tests, you'd check if NATS is available
- // For now, assume it's not available in test environments
- false
-}Committable suggestion skipped: line range outside the PR's diff.
| let options = async_nats::ConnectOptions::new() | ||
| .ping_interval(std::time::Duration::from_secs(10)) | ||
| .request_timeout(Some(std::time::Duration::from_secs(10))); | ||
|
|
||
| match options.connect("nats://localhost:4222").await { | ||
| Ok(client) => Some(Arc::new(RwLock::new(client))), | ||
| Err(_) => None, // NATS not available for testing | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Reduce NATS request timeout to keep tests snappy when the server is unavailable
A 10s request timeout will slow down tests. 200–500ms is typically enough for unit/integration tests that intentionally skip when NATS isn’t available.
Apply this diff:
let options = async_nats::ConnectOptions::new()
.ping_interval(std::time::Duration::from_secs(10))
- .request_timeout(Some(std::time::Duration::from_secs(10)));
+ .request_timeout(Some(std::time::Duration::from_millis(250)));📝 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.
| let options = async_nats::ConnectOptions::new() | |
| .ping_interval(std::time::Duration::from_secs(10)) | |
| .request_timeout(Some(std::time::Duration::from_secs(10))); | |
| match options.connect("nats://localhost:4222").await { | |
| Ok(client) => Some(Arc::new(RwLock::new(client))), | |
| Err(_) => None, // NATS not available for testing | |
| } | |
| let options = async_nats::ConnectOptions::new() | |
| .ping_interval(std::time::Duration::from_secs(10)) | |
| .request_timeout(Some(std::time::Duration::from_millis(250))); | |
| match options.connect("nats://localhost:4222").await { | |
| Ok(client) => Some(Arc::new(RwLock::new(client))), | |
| Err(_) => None, // NATS not available for testing | |
| } |
🤖 Prompt for AI Agents
In tests/routes_test.rs around lines 13 to 20, the NATS request_timeout is set
to 10 seconds which slows tests when the server is unavailable; change
request_timeout to a short duration (e.g., 200–500 ms such as
Duration::from_millis(250)) so tests skip quickly when NATS is unreachable while
leaving the rest of the connect options intact.
| // CORS preflight should return 200 | ||
| assert_eq!(response.status(), StatusCode::OK); | ||
|
|
||
| // Check that CORS headers are present | ||
| assert!(response | ||
| .headers() | ||
| .get(header::ACCESS_CONTROL_ALLOW_ORIGIN) | ||
| .is_some()); | ||
| } else { |
There was a problem hiding this comment.
CORS preflight typically returns 204 No Content, not 200
tower_http::cors::CorsLayer responds to preflight requests with 204 by default. The test should expect 204 unless you’ve explicitly changed this behavior.
Apply this diff:
- // CORS preflight should return 200
- assert_eq!(response.status(), StatusCode::OK);
+ // CORS preflight should return 204 by default
+ assert_eq!(response.status(), StatusCode::NO_CONTENT);🤖 Prompt for AI Agents
In tests/routes_test.rs around lines 85 to 93, the CORS preflight assertion
expects 200 but tower_http::cors::CorsLayer responds with 204 No Content by
default; update the test to assert StatusCode::NO_CONTENT (204) instead of OK
(200) and keep the subsequent check for the ACCESS_CONTROL_ALLOW_ORIGIN header
unchanged (or adjust only if you've intentionally overridden the CORS preflight
status elsewhere).
| #[tokio::test] | ||
| async fn test_api_v1_routes_exist() { | ||
| let allowed_origins = vec!["http://localhost:3000".to_string()]; | ||
| if let Some(mock_client) = create_test_nats_client().await { | ||
| // Just verify that build_routes doesn't panic with various route configurations | ||
| let _router = build_routes(allowed_origins, true, mock_client, None); | ||
|
|
||
| // Test that common API routes would be registered | ||
| // In a complete test, you'd verify each route exists and has correct methods | ||
| assert!(true); // Placeholder | ||
| } else { | ||
| println!("Skipping test: NATS server not available"); | ||
| } | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
test_api_v1_routes_exist is a placeholder; assert a basic route to add value
You can cheaply assert that a known route responds without panicking, which adds coverage beyond assert!(true).
Proposed change outside the selected range:
// After building the router:
let request = Request::builder()
.uri("/api/v1/statuses")
.method(Method::GET)
.body(Body::empty())
.unwrap();
let response = _router.oneshot(request).await.unwrap();
assert_eq!(response.status(), StatusCode::OK);🤖 Prompt for AI Agents
In tests/routes_test.rs around lines 123 to 136, the test
test_api_v1_routes_exist currently only has assert!(true) and should instead
perform a real request against a known route; after building the router, create
an HTTP GET request to "/api/v1/statuses", send it through the router using
oneshot().await, and assert the response status is StatusCode::OK; ensure the
test imports Request, Method, Body, and StatusCode (e.g., from hyper) and awaits
the oneshot future, unwrapping results as needed so the test fails on errors
rather than silently passing.
Summary by CodeRabbit
New Features
Bug Fixes
Tests
Chores