Skip to content
This repository was archived by the owner on Nov 23, 2025. It is now read-only.

Feature/vehicle service#5

Closed
Akith-002 wants to merge 3 commits intodevfrom
feature/vehicle-service
Closed

Feature/vehicle service#5
Akith-002 wants to merge 3 commits intodevfrom
feature/vehicle-service

Conversation

@Akith-002
Copy link
Copy Markdown

@Akith-002 Akith-002 commented Oct 31, 2025

Summary by CodeRabbit

Release Notes

  • Bug Fixes

    • Improved error handling for health endpoint responses.
    • Enhanced gateway error responses with detailed logging.
  • Improvements

    • Unified API service routing configuration across all endpoints.
    • Added comprehensive request logging including method, path, and host details.

@Akith-002 Akith-002 requested a review from RandithaK October 31, 2025 18:39
@coderabbitai
Copy link
Copy Markdown

coderabbitai bot commented Oct 31, 2025

Walkthrough

The gateway routing logic is refactored to use per-service path-prefix routing with conditional authentication middleware and header injection. Proxy request logging, custom error handling, and health endpoint resilience are added. All services in the configuration have unified strip_prefix values.

Changes

Cohort / File(s) Summary
Gateway routing and observability
cmd/gateway/main.go
Refactor per-service routing with dynamic path-prefix handling and conditional auth middleware; add proxy request logging (method, path, host, headers); implement custom ErrorHandler for reverse proxy with error logging and 502 Bad Gateway responses; add error handling for health endpoint writes; rename env variable to envURL to avoid shadowing the url package import
Service configuration
config.yaml
Normalize strip_prefix values across all services (auth, users, vehicles, appointments, services, projects, time-logs, payments, invoices, admin, ai) from service-specific paths (e.g., /api/v1/auth) to common /api/v1 prefix

Sequence Diagram

sequenceDiagram
    autonumber
    participant Client
    participant Gateway
    participant AuthMiddleware
    participant ReverseProxy
    participant Service

    Client->>Gateway: HTTP Request to /api/v1/users/123
    Gateway->>Gateway: Match pathPrefix "/api/v1/users"
    Gateway->>Gateway: Check auth_required for service
    
    alt Auth Required
        Gateway->>AuthMiddleware: Apply auth middleware
        AuthMiddleware->>AuthMiddleware: Validate credentials
    end
    
    Gateway->>Gateway: Strip prefix "/api/v1"<br/>(path becomes "/users/123")
    Gateway->>ReverseProxy: Forward request
    ReverseProxy->>Gateway: Log: method, path, host, headers
    ReverseProxy->>Service: Proxy to service
    
    alt Success
        Service-->>ReverseProxy: 200 OK
        ReverseProxy-->>Client: Response
    else Proxy Error
        ReverseProxy->>Gateway: CustomErrorHandler triggered
        Gateway->>Gateway: Log target, error, path, method
        Gateway-->>Client: 502 Bad Gateway
    end
Loading

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~22 minutes

  • Per-service routing logic in cmd/gateway/main.go: Verify that pathPrefix matching, conditional auth middleware application, and dual route registration (pathPrefix + "/\*" and pathPrefix) function correctly across all services
  • Custom ErrorHandler implementation: Ensure error logging captures all necessary context and 502 responses are generated appropriately
  • Configuration consistency: Verify that the unified /api/v1 strip_prefix aligns with the new routing logic in the gateway

Poem

🐰 The gateway routes with nimble feet,
Each service path now clean and neat,
With logs that shine and errors caught,
Strip prefixes merged—a unified thought!
Health checks steady, errors clear,
A hopping good update is here! ✨

Pre-merge checks and finishing touches

❌ Failed checks (2 warnings)
Check name Status Explanation Resolution
Title Check ⚠️ Warning The pull request title "Feature/vehicle service" is misleading and does not accurately represent the primary changes in the changeset. While the configuration changes do touch the vehicle service among many others, the main objectives of this PR are infrastructure-level improvements: refactoring the gateway's routing logic, implementing enhanced proxy logging and error handling, and generalizing the strip_prefix configuration across all services from specific paths to a common "/api/v1" prefix. The title suggests a vehicle service feature implementation, which is not what the changeset delivers. A teammate scanning the history would misunderstand the actual purpose of these changes.
Docstring Coverage ⚠️ Warning Docstring coverage is 66.67% which is insufficient. The required threshold is 80.00%. You can run @coderabbitai generate docstrings to improve docstring coverage.
✅ Passed checks (1 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
✨ Finishing touches
  • 📝 Generate docstrings
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch feature/vehicle-service

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link
Copy Markdown

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 1746c22 and acabe0f.

📒 Files selected for processing (2)
  • cmd/gateway/main.go (5 hunks)
  • config.yaml (2 hunks)

Comment on lines +106 to +111
logger.Info("proxying request",
"method", req.Method,
"original_path", req.URL.Path,
"target_host", target.Host,
"headers", req.Header,
)
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🔴 Critical

Remove sensitive header logging

Dumping the entire req.Header leaks bearer tokens and other sensitive credentials to the gateway logs, creating a serious security and compliance exposure. Please drop the header dump (or at least explicitly whitelist non-sensitive headers) before we ship.

-		logger.Info("proxying request",
-			"method", req.Method,
-			"original_path", req.URL.Path,
-			"target_host", target.Host,
-			"headers", req.Header,
-		)
+		logger.Info("proxying request",
+			"method", req.Method,
+			"original_path", req.URL.Path,
+			"target_host", target.Host,
+		)
📝 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.

Suggested change
logger.Info("proxying request",
"method", req.Method,
"original_path", req.URL.Path,
"target_host", target.Host,
"headers", req.Header,
)
logger.Info("proxying request",
"method", req.Method,
"original_path", req.URL.Path,
"target_host", target.Host,
)
🤖 Prompt for AI Agents
In cmd/gateway/main.go around lines 106 to 111, the logger currently dumps the
entire req.Header which can leak sensitive values (bearer tokens, cookies,
etc.); remove the req.Header dump and instead log only a safe, explicit
whitelist of headers (for example: Content-Type, Accept, User-Agent,
X-Request-ID) or build a filtered copy that redacts/omits any Authorization,
Cookie, Set-Cookie, and other sensitive headers before logging; implement the
whitelist/filtering when calling logger.Info so no raw headers are emitted.

@Akith-002 Akith-002 changed the base branch from main to dev November 5, 2025 05:47
@RandithaK RandithaK closed this Nov 8, 2025
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants