Add solution for Challenge 14 by Kosench #845
Conversation
WalkthroughAdds a self-contained Go in-memory microservice-style implementation: User, Product, Order domain models; UserService and ProductService servers; an OrderService orchestrating cross-service order creation; gRPC-like request/response types; HTTP handlers; client stubs; and logging/auth interceptors. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant OrderService
participant UserService
participant ProductService
Client->>OrderService: CreateOrder(userID, productID, quantity)
rect rgb(200,220,240)
Note over OrderService,UserService: Validate user
OrderService->>UserService: ValidateUser(userID)
UserService-->>OrderService: (ok / error)
end
rect rgb(220,240,200)
Note over OrderService,ProductService: Fetch product & check inventory
OrderService->>ProductService: GetProduct(productID)
ProductService-->>OrderService: (*Product / error)
OrderService->>ProductService: CheckInventory(productID, quantity)
ProductService-->>OrderService: (available / error)
end
rect rgb(240,220,200)
Note over OrderService: Create and store order
OrderService->>OrderService: compute total, increment ID, store in map
end
OrderService-->>Client: (*Order / error)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
Possibly related PRs
Pre-merge checks❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
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: 6
🧹 Nitpick comments (2)
challenge-14/submissions/Kosench/solution-template.go (2)
31-46: Consider using integer cents for monetary values.Using
float64forPriceandTotalcan lead to floating-point precision issues in financial calculations (e.g.,0.1 + 0.2 != 0.3). A common practice is to store monetary values as integer cents and convert for display.For a challenge solution this is acceptable, but worth noting for production code.
439-456: Context is ignored in HTTP client calls.The
ctxparameter is not used, so request cancellation and timeouts won't work. Usehttp.NewRequestWithContextinstead ofhttp.Get.func (c *UserServiceClient) GetUser(ctx context.Context, userID int64) (*User, error) { - resp, err := http.Get(fmt.Sprintf("%s/user/get?id=%d", c.baseURL, userID)) + req, err := http.NewRequestWithContext(ctx, http.MethodGet, + fmt.Sprintf("%s/user/get?id=%d", c.baseURL, userID), nil) + if err != nil { + return nil, err + } + resp, err := http.DefaultClient.Do(req) if err != nil { return nil, err }Apply the same pattern to
ValidateUser,GetProduct, andCheckInventory.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
challenge-14/submissions/Kosench/solution-template.go(1 hunks)
🔇 Additional comments (3)
challenge-14/submissions/Kosench/solution-template.go (3)
60-91: LGTM!The
UserServiceServerimplementation is clean. The in-memory map is initialized once and only read afterward, making it safe for concurrent access without synchronization.
93-124: LGTM!The
ProductServiceServerfollows the same safe pattern asUserServiceServerwith read-only map access after initialization.
532-547: Registration stubs and main are appropriate for the challenge.The empty registration functions and minimal main are reasonable placeholders for this challenge context.
| mux.HandleFunc("/user/get", func(w http.ResponseWriter, r *http.Request) { | ||
| userIDStr := r.URL.Query().Get("id") | ||
| userID, _ := strconv.ParseInt(userIDStr, 10, 64) | ||
|
|
||
| user, err := userServer.GetUser(r.Context(), userID) | ||
| if err != nil { | ||
| if status.Code(err) == codes.NotFound { | ||
| http.Error(w, err.Error(), http.StatusNotFound) | ||
| } else { | ||
| http.Error(w, err.Error(), http.StatusInternalServerError) | ||
| } | ||
| return | ||
| } | ||
|
|
||
| w.Header().Set("Content-Type", "application/json") | ||
| json.NewEncoder(w).Encode(user) | ||
| }) |
There was a problem hiding this comment.
Handle strconv.ParseInt errors to avoid silent failures.
Ignoring the parse error causes invalid input like ?id=abc to silently default to 0, which could return unexpected results if a user with ID 0 existed.
mux.HandleFunc("/user/get", func(w http.ResponseWriter, r *http.Request) {
userIDStr := r.URL.Query().Get("id")
- userID, _ := strconv.ParseInt(userIDStr, 10, 64)
+ userID, err := strconv.ParseInt(userIDStr, 10, 64)
+ if err != nil {
+ http.Error(w, "invalid user id", http.StatusBadRequest)
+ return
+ }The same pattern applies to lines 310-311, 354-355, 372-375.
📝 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.
| mux.HandleFunc("/user/get", func(w http.ResponseWriter, r *http.Request) { | |
| userIDStr := r.URL.Query().Get("id") | |
| userID, _ := strconv.ParseInt(userIDStr, 10, 64) | |
| user, err := userServer.GetUser(r.Context(), userID) | |
| if err != nil { | |
| if status.Code(err) == codes.NotFound { | |
| http.Error(w, err.Error(), http.StatusNotFound) | |
| } else { | |
| http.Error(w, err.Error(), http.StatusInternalServerError) | |
| } | |
| return | |
| } | |
| w.Header().Set("Content-Type", "application/json") | |
| json.NewEncoder(w).Encode(user) | |
| }) | |
| mux.HandleFunc("/user/get", func(w http.ResponseWriter, r *http.Request) { | |
| userIDStr := r.URL.Query().Get("id") | |
| userID, err := strconv.ParseInt(userIDStr, 10, 64) | |
| if err != nil { | |
| http.Error(w, "invalid user id", http.StatusBadRequest) | |
| return | |
| } | |
| user, err := userServer.GetUser(r.Context(), userID) | |
| if err != nil { | |
| if status.Code(err) == codes.NotFound { | |
| http.Error(w, err.Error(), http.StatusNotFound) | |
| } else { | |
| http.Error(w, err.Error(), http.StatusInternalServerError) | |
| } | |
| return | |
| } | |
| w.Header().Set("Content-Type", "application/json") | |
| json.NewEncoder(w).Encode(user) | |
| }) |
🤖 Prompt for AI Agents
In challenge-14/submissions/Kosench/solution-template.go around lines 291-307
(and similarly at 310-311, 354-355, 372-375), the code ignores errors from
strconv.ParseInt which lets invalid ids (e.g. ?id=abc) silently become 0; update
each handler to check the ParseInt error and return a 400 Bad Request
(http.Error) with a descriptive message when parsing fails, and only proceed to
call server methods when the parsed id is valid.
| // Connect to both services and return an OrderService | ||
| func ConnectToServices(userServiceAddr, productServiceAddr string) (*OrderService, error) { | ||
| // TODO: Implement this function | ||
| // Hint: create gRPC connections with interceptors, create clients, return OrderService | ||
| userConn, err := grpc.Dial( | ||
| userServiceAddr, | ||
| grpc.WithTransportCredentials(insecure.NewCredentials()), | ||
| grpc.WithUnaryInterceptor(AuthInterceptor)) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("failed to connect to user service: %w", err) | ||
| } | ||
|
|
||
| productConn, err := grpc.Dial( | ||
| productServiceAddr, | ||
| grpc.WithTransportCredentials(insecure.NewCredentials()), | ||
| grpc.WithUnaryInterceptor(AuthInterceptor)) | ||
| if err != nil { | ||
| userConn.Close() | ||
| return nil, fmt.Errorf("failed to connect to product service %w", err) | ||
| } | ||
|
|
||
| userClient := NewUserServiceClient(userConn) | ||
| productClient := NewProductServiceClient(productConn) | ||
|
|
||
| return NewOrderService(userClient, productClient), nil | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
🧩 Analysis chain
🌐 Web query:
grpc.Dial deprecated grpc.NewClient go 2024 2025
💡 Result:
Short answer: Yes — in grpc-go the old grpc.Dial (and DialContext) were deprecated in favor of grpc.NewClient. Use NewClient (and its context-aware variants / options) because it constructs a ClientConn without immediately doing I/O and it defaults to the "dns" resolver (Dial historically used "passthrough"). See the package docs and migration/discussion threads for details and behavioral differences. [1][2][3]
Sources
- grpc package docs (pkg.go.dev) — Dial/DialContext marked Deprecated; guidance to use NewClient. [1]
- grpc-go documentation / anti-patterns — explains differences (default resolver, Dial does immediate connect, NewClient starts idle). [2]
- grpc/grpc-go issue tracking (migration/docs to NewClient). [3]
🌐 Web query:
gRPC-go grpc.Dial vs grpc.NewClient deprecation documentation
💡 Result:
Short answer: grpc.Dial / DialContext are deprecated in gRPC‑Go in favor of grpc.NewClient. NewClient (introduced in v1.63) creates a ClientConn that starts IDLE (does not perform immediate I/O), defaults to the "dns" resolver, and uses ClientOptions (not DialOptions). Dial/DialContext historically started connecting immediately, used the "passthrough" default resolver, and exposed DialOptions like WithBlock/WithTimeout; DialContext currently calls NewClient then exits idle. See the docs and issues for details and migration guidance. [1][2][3]
Sources:
- pkg.go.dev grpc docs (Dial deprecated; DialContext behavior & resolver difference). [1]
- grpc-go Documentation/anti-patterns (explains NewClient vs Dial differences and rationale). [2]
- grpc/grpc-go issue tracking (migration and doc updates to prefer NewClient). [3]
🌐 Web query:
gRPC go connection close lifecycle management best practices
💡 Result:
Here are concise, actionable best practices for managing gRPC (Go) connection lifecycle — client and server — with sources.
Client-side
- Reuse a single grpc.ClientConn (don’t dial per RPC); close it when the app/component is shutting down. [1][2]
- Dial with context/timeouts (grpc.DialContext) and use backoff/retry policies; avoid WithTimeout (deprecated). Use connection pooling only if needed. [2][3]
- Use keepalive settings only when necessary (to avoid idle closes by proxies); tune KEEPALIVE_TIME / TIMEOUT and KEEPALIVE_WITHOUT_CALLS carefully. [4]
- Handle context cancellation/timeouts per RPC (context.WithTimeout / deadlines) so calls fail fast and don’t hang while shutting down. [3]
- Log and surface transport errors (e.g., "transport is closing") and increase MaxConnectionAge/MaxConnectionAgeGrace if servers rotate connections. [2]
Server-side
- Implement graceful shutdown: mark unhealthy (deregister from LB), stop accepting new work, allow in-flight RPCs to finish within a deadline, then force-stop. Use Server.GracefulStop() with a timeout fallback to Stop(). [5][1]
- Use health checks so load balancers stop sending traffic during shutdown windows. Delay shutdown until deregistration completes. [5]
- Configure server keepalive and MaxConnectionIdle/MaxConnectionAge to control connection lifetimes and prompt DNS/load-balancer re-resolution when needed. [4]
Operational / reliability
- Test shutdown and connection-failure scenarios (streaming + unary) under load and high latency. Watch for EOF/transport errors and tune window/flow-control if necessary. [2][6]
- Emit metrics for active RPCs, connections, and shutdown progress; use these to gate forced shutdown. [5]
- Use libraries/frameworks (lifecycle managers like fx) or a shutdown coordinator to centralize closing order: LB deregistration → stop accepting → wait for inflight → close resources → close ClientConn(s). [6]
Quick checklist for shutdown order
- Mark service unhealthy / deregister from LB. [5]
- Stop accepting new RPCs. [1]
- Wait for in-flight RPCs with a bounded timeout. [1][5]
- Close resources (DBs, queues) and client connections (ClientConn.Close()). [2][5]
- Force-stop if timeout expires. [1]
Sources
[1] gRPC: Graceful shutdown docs and Go examples. (grpc.io). (grpc.io)
[2] grpc-go repository README / issues (reuse ClientConn, debug “transport is closing”, connection errors). (github.com)
[3] Go contexts + Dial/Call timeout guidance (StackOverflow / grpc-go docs). (stackoverflow.com)
[4] gRPC Keepalive config and server connection timeouts. (grpc.io)
[5] gRPC performance & graceful-stop recommendations (grpc.io guides/examples). (grpc.io)
[6] grpc-go issue and high-latency behavior (flow-control/window tuning). (github.com)
If you want, I can produce a small Go shutdown coordinator example (client + server) implementing these steps.
Citations:
- 1: https://grpc.io/docs/guides/server-graceful-stop/?utm_source=openai
- 2: https://github.com/grpc/grpc-go?utm_source=openai
- 3: https://stackoverflow.com/questions/63955472/how-should-i-implement-a-timeout-in-a-grpc-call?utm_source=openai
- 4: https://grpc.io/docs/guides/keepalive/?utm_source=openai
- 5: https://grpc.io/docs/guides/performance/?utm_source=openai
- 6: gRPC connection closing with unexpected EOF on high-latency connections grpc/grpc-go#5358
Replace deprecated grpc.Dial with grpc.NewClient and implement connection lifecycle management.
grpc.Dialis deprecated in favor ofgrpc.NewClient(introduced in grpc-go v1.63)- The gRPC connections (
userConn,productConn) are never closed—add aClosemethod toOrderServiceto manage cleanup - Line 419 error message is malformed (missing colon before
%w)
Fix the error message:
- return nil, fmt.Errorf("failed to connect to product service %w", err)
+ return nil, fmt.Errorf("failed to connect to product service: %w", err)Add connection lifecycle management to OrderService:
+func (s *OrderService) Close() error {
+ if err := s.userClient.Close(); err != nil {
+ return err
+ }
+ return s.productClient.Close()
+}Consider migrating to grpc.NewClient when possible for consistency with current best practices.
Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In challenge-14/submissions/Kosench/solution-template.go around lines 401 to
426, replace the deprecated grpc.Dial usage with grpc.NewClient (or the current
constructor in your grpc version) to create user and product clients, fix the
malformed error message on line 419 by adding the missing colon before %w, and
add connection lifecycle management: store the underlying connection objects on
the OrderService struct, ensure any connection opened is closed on error paths,
and implement an OrderService.Close() method that closes both connections (and
is called by callers when done).
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (5)
challenge-14/submissions/Kosench/solution-template.go (5)
527-567: HTTP client calls ignore context cancellation and timeouts.Same issue as
UserServiceClient: the methods receive acontext.Contextbut usehttp.Get, ignoring context cancellation and deadlines.Apply the same fix as suggested for
UserServiceClient—usehttp.NewRequestWithContextandhttp.DefaultClient.Do:func (c *ProductServiceClient) GetProduct(ctx context.Context, productID int64) (*Product, error) { - resp, err := http.Get(fmt.Sprintf("%s/product/get?id=%d", c.baseURL, productID)) + req, err := http.NewRequestWithContext(ctx, "GET", fmt.Sprintf("%s/product/get?id=%d", c.baseURL, productID), nil) + if err != nil { + return nil, err + } + resp, err := http.DefaultClient.Do(req) if err != nil { return nil, err }Apply the same pattern to
CheckInventory.
345-361: Handlestrconv.ParseInterror to avoid silent failures.Line 347 ignores the parse error, causing invalid input like
?id=abcto silently default to0. This was flagged in a previous review but remains unfixed in this handler.Apply this diff:
mux.HandleFunc("/user/validate", func(w http.ResponseWriter, r *http.Request) { userIDStr := r.URL.Query().Get("id") - userID, _ := strconv.ParseInt(userIDStr, 10, 64) + userID, err := strconv.ParseInt(userIDStr, 10, 64) + if err != nil { + http.Error(w, "invalid user id", http.StatusBadRequest) + return + } valid, err := userServer.ValidateUser(r.Context(), userID)
389-405: Handlestrconv.ParseInterror to avoid silent failures.Line 391 ignores the parse error, causing invalid input to silently default to
0. This was flagged in a previous review but remains unfixed.Apply this diff:
mux.HandleFunc("/product/get", func(w http.ResponseWriter, r *http.Request) { productIDStr := r.URL.Query().Get("id") - productID, _ := strconv.ParseInt(productIDStr, 10, 64) + productID, err := strconv.ParseInt(productIDStr, 10, 64) + if err != nil { + http.Error(w, "invalid product id", http.StatusBadRequest) + return + } product, err := productServer.GetProduct(r.Context(), productID)
407-425: Handlestrconv.ParseInterrors to avoid silent failures.Lines 409 and 411 ignore parse errors, causing invalid input to silently default to
0. This was flagged in a previous review but remains unfixed.Apply this diff:
mux.HandleFunc("/product/check-inventory", func(w http.ResponseWriter, r *http.Request) { productIDStr := r.URL.Query().Get("id") - productID, _ := strconv.ParseInt(productIDStr, 10, 64) + productID, err := strconv.ParseInt(productIDStr, 10, 64) + if err != nil { + http.Error(w, "invalid product id", http.StatusBadRequest) + return + } quantityStr := r.URL.Query().Get("quantity") - quantity, _ := strconv.ParseInt(quantityStr, 10, 32) + quantity, err := strconv.ParseInt(quantityStr, 10, 32) + if err != nil { + http.Error(w, "invalid quantity", http.StatusBadRequest) + return + } available, err := productServer.CheckInventory(r.Context(), productID, int32(quantity))
449-456: Missing colon in error message.Line 455: the error format string is missing a colon before
%w, which was flagged in a previous review but remains unfixed.Apply this diff:
productConn, err := grpc.NewClient( productServiceAddr, grpc.WithTransportCredentials(insecure.NewCredentials()), grpc.WithUnaryInterceptor(AuthInterceptor)) if err != nil { userConn.Close() - return nil, fmt.Errorf("failed to connect to product service %w", err) + return nil, fmt.Errorf("failed to connect to product service: %w", err) }
🧹 Nitpick comments (1)
challenge-14/submissions/Kosench/solution-template.go (1)
476-512: HTTP client calls ignore context cancellation and timeouts.The methods receive a
context.Contextbut usehttp.Getinstead of creating requests withhttp.NewRequestWithContext. This means context cancellation and deadlines are not respected, potentially causing requests to hang.Apply this pattern to respect context:
func (c *UserServiceClient) GetUser(ctx context.Context, userID int64) (*User, error) { - resp, err := http.Get(fmt.Sprintf("%s/user/get?id=%d", c.baseURL, userID)) + req, err := http.NewRequestWithContext(ctx, "GET", fmt.Sprintf("%s/user/get?id=%d", c.baseURL, userID), nil) + if err != nil { + return nil, err + } + resp, err := http.DefaultClient.Do(req) if err != nil { return nil, err }Apply the same pattern to
ValidateUser(and the ProductServiceClient methods).
2 participants
Challenge 14 Solution
Submitted by: @Kosench
Challenge: Challenge 14
Description
This PR contains my solution for Challenge 14.
Changes
challenge-14/submissions/Kosench/solution-template.goTesting
Thank you for reviewing my submission! 🚀