Conversation
…stgres fix: Update response transformers to validate response size and improve XML/CSV rendering refactor: Streamline CORS configuration in main server initialization chore: Improve authentication middleware logging and token extraction feat: Introduce bearer token extraction utility in account service refactor: Enhance retry logic with configurable delays and logging fix: Update unified pagination to use general-purpose base64 engine feat: Improve websocket logger with structured logging and tests docs: Add production readiness blueprint outlining actionable work streams
|
Caution Review failedThe pull request is closed. WalkthroughClarifies WebSocket admin access in docs; adds a production-readiness blueprint; propagates tenant_id via request extensions; moves WebSocket slot reservation to post-handshake with atomic reservation and 503 on exhaustion; migrates error logging to tracing; enforces response size limits and adds XML/CSV escaping; enforces memory limits and snapshot pruning; adds retry backoff hook and CORS origin centralization. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Client
participant WSHandler as WS Handler
participant Pool as ConnPool
participant Guard as ConnectionGuard
Client->>WSHandler: HTTP Upgrade (WebSocket)
WSHandler->>WSHandler: Validate origin & auth (may use WS_LOGS_ADMIN_USER)
alt auth/origin fail
WSHandler->>Client: 401/403
else pass
WSHandler->>WSHandler: Perform HTTP upgrade (handshake)
WSHandler->>Pool: Read active count
loop atomic reservation
WSHandler->>Pool: compare_exchange increment
alt capacity reached
WSHandler->>Client: Close + 503 ServiceUnavailable
else reservation succeeds
WSHandler->>Guard: Create guard and spawn session task
Guard->>Pool: Ensure decrement on drop
WSHandler->>Client: WebSocket established
end
end
end
sequenceDiagram
autonumber
participant Req
participant Auth as Auth Middleware
participant Ext as Extensions
participant Ctrl as ControllerContext
Req->>Auth: Incoming request
Auth->>Auth: Decode & validate JWT
alt invalid
Auth->>Req: 401
else valid
Auth->>Ext: Insert tenant_id into extensions
Auth->>Req: Forward
Req->>Ctrl: ControllerContext::from_request
Ctrl->>Ext: Read optional tenant_id
Ctrl->>Ctrl: Return context { pool, tenant_id }
end
sequenceDiagram
autonumber
participant Client
participant Handler
participant Serializer
participant Validator
Client->>Handler: Request with Accept header
Handler->>Handler: Negotiate response format
Handler->>Serializer: Render payload
Serializer->>Validator: Check size <= MAX_RESPONSE_SIZE_BYTES
alt exceeds size
Validator->>Handler: Return error
Handler->>Client: Error response
else okay
Validator->>Handler: Return body
Handler->>Client: 200 + body
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes
Possibly related PRs
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (3)
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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/services/functional_patterns.rs (1)
673-685: Misleading API: backoff strategy methods don't affect execution.Both
exponential_backoffandfibonacci_backoffmethods only setdelay_msbut don't change the actual backoff strategy. Theexecutemethod always uses exponential backoff (saturating_mul(2)at line 714), regardless of which configuration method was called. This creates a misleading API where callingfibonacci_backoffsuggests Fibonacci backoff will be used, but exponential backoff is always applied.Consider one of these solutions:
Solution 1: Store the backoff strategy
+#[derive(Clone, Copy)] +enum BackoffStrategy { + Exponential, + Fibonacci, +} + pub struct Retry<T> { operation: Box<dyn Fn() -> ServiceResult<T> + Send + Sync>, max_attempts: usize, delay_ms: u64, + strategy: BackoffStrategy, }Then modify
executeto use the stored strategy.Solution 2: Remove misleading methods
If only exponential backoff is supported, remove
fibonacci_backoffand rename/removeexponential_backoffsince it's the default.Solution 3: Use
execute_with_backoffDocument that users should call
execute_with_backoffwith a custom function for non-exponential strategies, and remove these configuration methods.src/functional/immutable_state.rs (1)
672-717: Restore named index after auto pruning.When the auto branch drops snapshots, the indexes of surviving named snapshots shift but
named_snapshotsnever gets rebuilt. Named lookups will therefore point at the wrong snapshot or panic. Please ensure the index map is refreshed whenever pruning removes elements.- // Remove oldest automatic snapshots if over limit (keep newest ones) - if !is_named && auto_count > self.max_auto_snapshots { + let mut named_index_dirty = false; + + // Remove oldest automatic snapshots if over limit (keep newest ones) + if !is_named && auto_count > self.max_auto_snapshots { // Count automatic snapshots and remove oldest ones let to_remove = auto_count - self.max_auto_snapshots; let mut removed = 0; self.snapshots.retain(|s| { // Keep all named snapshots if s.name.is_some() { return true; } // Remove oldest automatic snapshots if removed < to_remove { removed += 1; false } else { true } }); + named_index_dirty = true; } // Remove oldest named snapshots if over limit (keep newest ones) if is_named && named_count > self.max_named_snapshots { // Find and remove oldest named snapshots let to_remove = named_count - self.max_named_snapshots; let mut removed = 0; self.snapshots.retain(|s| { // Keep all automatic snapshots if s.name.is_none() { return true; } // Remove oldest named snapshots if removed < to_remove { removed += 1; false } else { true } }); - // Rebuild named_snapshots index after potential removals - self.rebuild_named_index(); + named_index_dirty = true; + } + + if named_index_dirty { + self.rebuild_named_index(); }
🧹 Nitpick comments (12)
docs/PRODUCTION_FOUNDATION_BLUEPRINT.md (4)
3-3: Use heading syntax instead of emphasis for the date.Per Markdown best practices (MD036), dates should not be emphasized but can be included as a heading or in plain text.
-_Date: October 29, 2025_ +**Date:** October 29, 2025Also, consider updating the date to October 30, 2025 (current date) unless this intentionally reflects when the plan was drafted.
29-29: Add hyphen to compound adjective.Line 29 references "focused suites"; the preceding phrase should use a hyphen to join the modifier and noun.
- Author focused suites for: + Author focused suites for:Change to: "Author unit-test suites for:" or similar if referring to a compound adjective grouping.
65-65: Add hyphen to compound adjective."Graceful" and "fail-open" should be separated; the phrase reads: "…degrade gracefully (fail-open)…"
This is grammatically acceptable as written; no change needed here.
94-94: Correct.github/capitalization.The official name of the GitHub platform directory is
.GitHub/, not.github/.-No `.github/` directory or CI workflows exist. +No `.GitHub/` directory or CI workflows exist.Also apply on line 101:
-Add `.github/workflows/ci.yml` with lint, test (Rust MSRV/stable matrix), build, and security audit jobs; configure test services and coverage upload. +Add `.GitHub/workflows/ci.yml` with lint, test (Rust MSRV/stable matrix), build, and security audit jobs; configure test services and coverage upload.Also applies to: 101-101
src/error.rs (1)
9-9: Clean up unused log imports after tracing migration.The migration to
tracinginlog_with_levelis correct, but line 9 still imports log macros (log_error,log_info,log_warn,debug) that are no longer used in this method.Consider removing the unused log macro imports:
-use log::{debug, error as log_error, info as log_info, warn as log_warn, Level}; +use log::Level;Note: The
error_loggingmodule (lines 454-520) still uses thelogcrate, so a complete migration would require updating that module as well for consistency.Also applies to: 13-13, 268-271
src/services/account_service.rs (1)
67-98: Strengthen JWT format validation in bearer token extraction.The current validation checks if the token contains a dot, but a valid JWT should have exactly two dots (separating header, payload, and signature).
Apply this diff to improve the validation:
- } else if !trimmed.contains('.') { + } else if trimmed.matches('.').count() != 2 { Err(ServiceError::unauthorized( "Invalid token format".to_string(), ))This ensures the token follows the standard JWT structure of
header.payload.signature.src/utils/ws_logger.rs (2)
152-157: Consider using current span context instead of event metadata name.The
span_nameis extracted fromevent.metadata().name(), which gives the event's name, not the current span's name. The namedctxparameter could be used to get the actual span context.If you want the current span's name, consider using the context:
let span_name = ctx .event_span(event) .map(|span| span.name().to_string()) .unwrap_or_else(|| metadata.name().to_string());However, if event metadata name is intentional, the current implementation is fine.
334-403: Good test coverage for LogBroadcaster.The tests cover the main scenarios for the broadcaster: sending/receiving, multiple receivers, and no receivers. The LogFormat tests are also appropriate.
Consider adding tests for
format_log_eventto verify the JSON and Text formatting logic, especially the new span and fields handling.src/middleware/auth_middleware.rs (1)
112-115: Scope the tenant_id extension with a newtype.Storing the tenant ID as a bare
Stringin request extensions risks collisions with any other middleware that also inserts aString. Please wrap it in a dedicatedTenantIdnewtype (and read that type back incontroller_context) to avoid clobbering unrelated data and make intent explicit.src/functional/response_transformers.rs (3)
611-611: Consider using ContentType helper methods for consistency.Lines 611, 622, and 635 use raw header tuples while other format branches use
header::ContentType::json()andheader::ContentType::plaintext()helper methods.For consistency and readability, consider using helpers where available:
// For XML: builder.insert_header(header::ContentType(mime::APPLICATION_XML)); // Or if not available: builder.insert_header((header::CONTENT_TYPE, HeaderValue::from_static("application/xml; charset=utf-8")));This makes the code more uniform and easier to maintain.
Also applies to: 622-622, 635-635
574-577: Consider extracting size validation to reduce duplication.The size validation logic and error message are repeated identically across all six format branches. While straightforward, this duplication makes future changes (e.g., adjusting the limit or error format) more error-prone.
Extract a helper function:
fn validate_response_size(payload: &[u8]) -> Result<(), serde_json::Error> { if payload.len() > MAX_RESPONSE_SIZE_BYTES { return Err(serde_json::Error::io(std::io::Error::new( std::io::ErrorKind::Other, format!( "Response size {} exceeds maximum of {} bytes", payload.len(), MAX_RESPONSE_SIZE_BYTES ), ))); } Ok(()) }Then use it in each branch:
let payload = serde_json::to_vec(&envelope)?; validate_response_size(&payload)?; builder.insert_header(header::ContentType::json()); Ok(builder.body(payload))Also applies to: 584-586, 593-595, 607-609, 618-620, 631-633
682-700: Consider simplifying wildcard matching with a priority list.The explicit if-else chain for
*/*handling is correct but verbose. A priority-based approach would be more maintainable.if entry.media_type == "*/*" { // Priority order for wildcard: prefer text/JSON formats over binary const WILDCARD_PRIORITY: &[ResponseFormat] = &[ ResponseFormat::Json, ResponseFormat::JsonPretty, ResponseFormat::Text, ResponseFormat::Xml, ResponseFormat::Csv, ResponseFormat::MessagePack, ]; for &format in WILDCARD_PRIORITY { if allowed.contains(&format) { return Some(format); } } return allowed.first().copied(); }This makes the priority order explicit and easier to adjust.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (14)
.env.example(1 hunks)docs/PRODUCTION_FOUNDATION_BLUEPRINT.md(1 hunks)src/api/controller_context.rs(1 hunks)src/api/ws_controller.rs(3 hunks)src/error.rs(2 hunks)src/functional/immutable_state.rs(8 hunks)src/functional/query_builders.rs(1 hunks)src/functional/response_transformers.rs(7 hunks)src/main.rs(1 hunks)src/middleware/auth_middleware.rs(4 hunks)src/services/account_service.rs(6 hunks)src/services/functional_patterns.rs(3 hunks)src/unified_pagination.rs(3 hunks)src/utils/ws_logger.rs(5 hunks)
🧰 Additional context used
🧬 Code graph analysis (7)
src/main.rs (1)
src/middleware/ws_security.rs (1)
get_allowed_origins(85-107)
src/services/account_service.rs (3)
src/error.rs (2)
unauthorized(155-160)internal_server_error(162-167)src/services/functional_patterns.rs (1)
conn(76-76)src/models/refresh_token.rs (1)
revoke(77-81)
src/functional/query_builders.rs (2)
src/models/tenant.rs (2)
query(555-555)filter(439-595)src/api/tenant_controller.rs (1)
filter(316-397)
src/api/ws_controller.rs (1)
src/middleware/ws_security.rs (1)
from_header(56-69)
src/api/controller_context.rs (1)
src/error.rs (1)
internal_server_error(162-167)
src/functional/immutable_state.rs (1)
src/functional/state_transitions.rs (1)
create_test_tenant(583-591)
src/middleware/auth_middleware.rs (2)
src/api/controller_context.rs (4)
req(21-23)req(33-35)tenant_id(70-72)tenant_id(185-187)src/utils/token_utils.rs (1)
verify_token(42-52)
🪛 LanguageTool
docs/PRODUCTION_FOUNDATION_BLUEPRINT.md
[grammar] ~29-~29: Use a hyphen to join words.
Context: ...re/, validators/, utils/). - Author focused suites for: - account_service...
(QB_NEW_EN_HYPHEN)
[grammar] ~65-~65: Use a hyphen to join words.
Context: ...ation steps in deployment guides. - Rate limiting middleware should degrade grace...
(QB_NEW_EN_HYPHEN)
[uncategorized] ~94-~94: The official name of this software platform is spelled with a capital “H”.
Context: ...r; Postgres/Redis services absent. - No .github/ directory or CI workflows exist. - Do...
(GITHUB)
[uncategorized] ~101-~101: The official name of this software platform is spelled with a capital “H”.
Context: ...tgres/Redis/test-runner pattern). - Add .github/workflows/ci.yml with lint, test (Rust...
(GITHUB)
🪛 markdownlint-cli2 (0.18.1)
docs/PRODUCTION_FOUNDATION_BLUEPRINT.md
3-3: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
🔇 Additional comments (14)
docs/PRODUCTION_FOUNDATION_BLUEPRINT.md (1)
1-197: Comprehensive and actionable production roadmap.The blueprint is well-structured with clear phases, current baselines grounded in code references, and concrete target deliverables. The cross-cutting notes and open questions provide good context for execution.
Suggestions for enhanced clarity:
- Phase 1 frontloads security hardening before expanding test coverage—this sequencing is sound and should be emphasized in release comms.
- The integration harness pattern (
tests/integration/{api,common}/) and fixture helpers will significantly improve test maintainability; consider adding a comment about testcontainers setup preferences.- Open questions are well-framed; suggest documenting decisions in a follow-up ADR once stakeholder input is gathered.
src/unified_pagination.rs (3)
31-31: LGTM: Clean import refactoring.The updated import statement correctly brings the
Enginetrait into scope, which is required for calling.encode()and.decode()methods on the base64 engines. This refactor improves code clarity by enabling shorter, more readable paths throughout the file.
39-39: LGTM: Consistent usage of import alias.The shortened path
general_purpose::STANDARD.decode()correctly leverages the import alias from line 31, improving readability while maintaining identical functionality.
109-109: LGTM: Explicit trait import enhances clarity.While the
Enginetrait is technically available viause super::*;on line 108, explicitly importing it here is good practice. This makes the module's dependency on the trait clear and improves code maintainability.src/functional/query_builders.rs (1)
143-144: LGTM with caveats: changes are correct, but codebase migration is incomplete.The
.escape('\\')additions to bothContains(line 144) andNotContains(line 152) operators are technically correct and properly pair withescape_like_pattern()for robust Postgres LIKE handling.However, verification reveals 6 remaining unescaped LIKE usages that expose wildcard injection vulnerabilities:
src/models/tenant.rs(lines 459, 466, 473):format!("%{}%", field_filter.value)src/models/person.rs(lines 131, 145, 159):format!("%{}%", email/name/phone)These legacy patterns lack both pattern escaping and the ESCAPE clause. Clarify whether this PR is a partial update or if the migration to the secure pattern should include these files. If in scope, migrate these to use
escape_like_pattern()and.escape('\\').src/services/functional_patterns.rs (2)
720-752: LGTM!The
execute_with_backoffimplementation correctly allows custom backoff strategies. The signatureF: Fn(usize, u64) -> u64provides flexibility where the first parameter is the attempt number and the second is the current delay. The implementation properly calculates the delay before logging and sleeping, maintaining clear semantics.Note: The same blocking sleep concern mentioned for the
executemethod applies here.
687-718: No issues found—implementation is appropriate for synchronous contexts.Verification confirms all
Retryusages are in synchronous functions (logout,refresh,me) withinaccount_service.rs. Sinceverify_token_with_retryis a sync function and all its callers are sync, the use ofstd::thread::sleepis appropriate and poses no problems. The exponential backoff implementation is correct withsaturating_mul(2)preventing overflow.src/services/account_service.rs (2)
285-285: LGTM! Consistent token extraction.The refactoring to use
extract_bearer_tokenimproves code consistency and reduces duplication across the logout, refresh, and me operations.Also applies to: 361-361, 514-514
459-472: LGTM! Atomic token rotation prevents orphaned states.The transaction ensures that both the revoke and create operations succeed or fail together, preventing security issues where an old token is revoked but no new token is created.
src/utils/ws_logger.rs (3)
200-222: LGTM! Structured JSON logging with span and fields.The enhanced JSON format provides rich structured data for downstream log aggregation and analysis. The filtering of duplicate message fields is correct.
229-248: LGTM! Comprehensive field capture for structured logging.The visitor now captures all fields into a structured map, enabling richer JSON output while maintaining backward compatibility with the message field.
315-329: Idempotent initialization via error message matching.The error message matching makes initialization idempotent, which is useful for tests and potential hot-reloads. However, string matching on error messages is fragile if the upstream library changes error text.
This is a pragmatic solution, but if tracing_subscriber provides a more stable API to check initialization status, that would be preferable.
.env.example (1)
40-43: LGTM! Clear security documentation.The updated documentation explicitly states the behavior when
WS_LOGS_ADMIN_USERis unset, which helps operators make informed security decisions.src/main.rs (1)
133-140: LGTM! Centralized CORS origin management.Moving the allowed origins logic to
middleware::ws_security::get_allowed_origins()provides a single source of truth for CORS configuration, improving maintainability and consistency across the application.
- Add state rollback in ImmutableStateManager to restore previous state when memory limits are exceeded, preventing corruption - Improve CSV field escaping to handle carriage return characters (\r) in `escape_csv_field` - Implement recursive JSON to XML conversion in `json_to_xml` function for proper XML structure and use it in render_response, replacing simple JSON string conversion - Bind WebSocket connection guard to local variable in ws_controller to ensure proper lifetime and counter decrement when task completes
…unctional modules - Modified snapshot history test to use create_test_state helper instead of default state constructor, improving test maintainability - Corrected payload assertion in response transformers test to check for "test" key, fixing likely typo in expected output validation
1 participant
…stgres
fix: Update response transformers to validate response size and improve XML/CSV rendering
refactor: Streamline CORS configuration in main server initialization
chore: Improve authentication middleware logging and token extraction
feat: Introduce bearer token extraction utility in account service
refactor: Enhance retry logic with configurable delays and logging
fix: Update unified pagination to use general-purpose base64 engine
feat: Improve websocket logger with structured logging and tests
docs: Add production readiness blueprint outlining actionable work streams
Summary by CodeRabbit
Documentation
New Features
Improvements