Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub. |
![sentry[bot]](https://avatars.githubusercontent.com/in/12637?s=60&v=4){kind=small}
| for k, v := range m { | ||
| if f, ok := v.(float64); ok { | ||
| if id, _ := strconv.Atoi(k); id > 0 { | ||
| quantities[id] = int(f) // Go panic: assignment to entry in nil map |
There was a problem hiding this comment.
Bug: The quantities map is declared but not initialized before assignment, causing a panic during checkout processing.
Severity: CRITICAL | Confidence: 1.00
🔍 Detailed Analysis
The Go server will panic when processing a normal checkout request due to an assignment to an uninitialized map. The quantities map is declared as var quantities map[int]int but not initialized using make before values are assigned to its entries at go/cmd/server/main.go:514. This occurs when o.ValidateInventory is "true" and o.Cart contains items, which are standard conditions for a user proceeding through checkout with items in their cart. The panic will crash the server's request handler.
💡 Suggested Fix
Initialize the quantities map using quantities = make(map[int]int) before attempting to assign values to its entries.
🤖 Prompt for AI Agent
Review the code at the location below. A potential bug has been identified by an AI
agent.
Verify if this is a real issue. If it is, propose a fix; if not, explain why it's not
valid.
Location: go/cmd/server/main.go#L514
Potential issue: The Go server will panic when processing a normal checkout request due
to an assignment to an uninitialized map. The `quantities` map is declared as `var
quantities map[int]int` but not initialized using `make` before values are assigned to
its entries at `go/cmd/server/main.go:514`. This occurs when `o.ValidateInventory` is
"true" and `o.Cart` contains items, which are standard conditions for a user proceeding
through checkout with items in their cart. The panic will crash the server's request
handler.
Did we get this right? 👍 / 👎 to inform future reviews.
Codecov Report✅ All modified and coverable lines are covered by tests. ❌ Your project check has failed because the head coverage (24.62%) is below the target coverage (35.00%). You can increase the head coverage or adjust the target coverage. Additional details and impacted files@@ Coverage Diff @@
## master #1089 +/- ##
=======================================
Coverage 24.62% 24.62%
=======================================
Files 41 41
Lines 1202 1202
Branches 137 137
=======================================
Hits 296 296
Misses 881 881
Partials 25 25
|
|
@sentry review |
giortzisg
left a comment
giortzisg
left a comment
There was a problem hiding this comment.
Had a look at the go server implementation and left some comments
| // wrap mux with a 404 logger | ||
| logged := http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { | ||
| rw := &statusRecorder{ResponseWriter: w, status: 200} | ||
| mux.ServeHTTP(rw, r) | ||
| if rw.status == http.StatusNotFound { | ||
| log.Printf("404 Not Found: %s %s", r.Method, r.URL.Path) | ||
| } | ||
| }) |
There was a problem hiding this comment.
I noticed that we also trace 404 requests on the dashboard. We should probably skip these by setting the status code correctly to 404 (Just included 404 ignores with version 0.38.0, so the SDK version also needs to be bumped)
| mux.HandleFunc("/product/0/info", func(w http.ResponseWriter, r *http.Request) { | ||
| // Let Sentry HTTP middleware handle transaction creation automatically | ||
| // This is crucial for N+1 detection to work properly | ||
| logger := sentry.NewLogger(r.Context()) |
There was a problem hiding this comment.
for the purpose of the demo I would imagine it makes sense to use slog, rather than our own logger, since most people would default to that.
| querySpan := sentry.StartSpan(ctx, "get_inventory", sentry.WithDescription("db.query")) | ||
| defer querySpan.Finish() | ||
|
|
||
| sqlSpan := sentry.StartSpan(querySpan.Context(), "db", sentry.WithDescription("SELECT * FROM inventory WHERE productId")) |
There was a problem hiding this comment.
I don't really know if it makes sense to showcase sql transactions for Go, since we currently don't support a db integration (although we will very soon).
On the same note, is the flask app creating fake spans or is it actually integrating with a db?
There was a problem hiding this comment.
Flask uses the Sql integration. Yeah the purpose here is to showcase the n+1 performance capability. Are go devs not tracking db queries right now in Sentry?
There was a problem hiding this comment.
Not yet, we have an sql integration PR that's almost ready, but currently it's getting blocked by a core refactoring we are doing. I guess it's ok for now, but we should use the actual integration when we release it.
|
Also another consideration to showcase the product even more, we introduced exception groups from v0.36.0 so it would be nice if we could use a third party library like https://github.com/go-errors/errors, so that our nested errors would have their own unique stack traces. |
3 participants
Two main functions to review are getProducts() and getInventory()