Add solution for Challenge 13 by Onkar-25#1407
Conversation
WalkthroughThis PR introduces a SQLite-backed ProductStore implementation in Go for inventory management. It defines a Product struct and ProductStore type with database initialization, CRUD operations (create, read, update, delete), product listing with optional category filtering, and a transactional batch inventory update capability with proper commit/rollback semantics. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Tip Issue Planner is now in beta. Read the docs and try it out! Share your feedback on Discord. 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: 2
🧹 Nitpick comments (3)
challenge-13/submissions/Onkar-25/solution-template.go (3)
5-5: Remove the commented-out import.
// "errors"is a dead-code leftover and should be deleted.🧹 Proposed fix
- //"errors"
44-48:return db, err—erris alwaysnilhere; preferreturn db, nilfor clarity.If
db.Execat line 42 fails, the function returns at line 45. By line 48errcan only benil, so returning it is a no-op but confusing to readers.🧹 Proposed fix
- return db,err + return db, nil
97-101:UpdateProduct— pre-existence check viaGetProductis a redundant round-trip.The
GetProductcall at line 98 is unnecessary: if the product doesn't exist, the subsequentUPDATEwill simply affect 0 rows, which is already caught by therowsAffected == 0check at line 117. The extra select also introduces a TOCTOU window (the row could be deleted between the check and the update).Consider removing the
GetProductcall and relying solely on therowsAffected == 0guard, returning a"product with ID %d not found"message there instead:♻️ Proposed simplification
func (ps *ProductStore) UpdateProduct(product *Product) error { - _,err := ps.GetProduct(product.ID) - if err != nil{ - return err - } - query:= ` UPDATE products SET name = ?,price = ?,quantity = ?,category=? WHERE id = ? ` - result,err:= ps.db.Exec(query,product.Name,product.Price,product.Quantity,product.Category,product.ID) + result, err := ps.db.Exec(query,product.Name,product.Price,product.Quantity,product.Category,product.ID) if err != nil { return fmt.Errorf("failed to update product: %w", err) } rowsAffected, err := result.RowsAffected() if err != nil { return fmt.Errorf("failed to get rows affected: %w", err) } if rowsAffected == 0 { - return fmt.Errorf("no rows were updated") + return fmt.Errorf("product with ID %d not found", product.ID) } return nil
| if category != ""{ | ||
| query = query + `WHERE category = ?` | ||
| } | ||
|
|
||
| rows, err := ps.db.Query(query,category) |
There was a problem hiding this comment.
ListProducts crashes with "expected 0 arguments, got 1" when category is empty.
When category == "", the WHERE clause is omitted so the query has no ? placeholder, but category is still unconditionally passed as an argument to db.Query. The "sql: expected 0 arguments, got 1" error occurs when you pass arguments to a SQL query that has no placeholders — the number of parameters provided must match the number of ? markers in the statement.
Every call to ListProducts("") will return an error instead of the full product list.
🐛 Proposed fix — use variadic args
- query:= `SELECT *
- FROM products
- `
- if category != ""{
- query = query + `WHERE category = ?`
- }
-
- rows, err := ps.db.Query(query,category)
+ query := `SELECT * FROM products`
+ var args []interface{}
+ if category != "" {
+ query += ` WHERE category = ?`
+ args = append(args, category)
+ }
+
+ rows, err := ps.db.Query(query, args...)| rows, err := ps.db.Query(query,category) | ||
| if err != nil{ | ||
| return nil , err | ||
| } | ||
|
|
||
| var product []*Product | ||
|
|
||
| for rows.Next(){ | ||
| p:= &Product{} | ||
| err := rows.Scan(&p.ID,&p.Name,&p.Price,&p.Quantity,&p.Category) | ||
| if err!=nil{ | ||
| return nil, err | ||
| } | ||
| product = append(product,p) | ||
| } | ||
| return product,nil |
There was a problem hiding this comment.
ListProducts leaks the sql.Rows handle and silently swallows iteration errors.
Two issues in combination:
-
Missing
defer rows.Close()— ifrows.Scan()fails and the function returns early, the*sql.Rowsis never closed. This holds the underlying database connection open, exhausting the connection pool over time. -
Missing
rows.Err()check — afterrows.Next()returnsfalse, the caller must checkrows.Err()to detect any driver-level error that terminated the iteration. Without it, partial results can be returned with anilerror.
🐛 Proposed fix
rows, err := ps.db.Query(query, args...)
if err != nil{
return nil , err
}
+ defer rows.Close()
var product []*Product
for rows.Next(){
p:= &Product{}
err := rows.Scan(&p.ID,&p.Name,&p.Price,&p.Quantity,&p.Category)
if err!=nil{
return nil, err
}
product = append(product,p)
}
- return product,nil
+ if err := rows.Err(); err != nil {
+ return nil, err
+ }
+ return product, nil1 participant
Challenge 13 Solution
Submitted by: @Onkar-25
Challenge: Challenge 13
Description
This PR contains my solution for Challenge 13.
Changes
challenge-13/submissions/Onkar-25/solution-template.goTesting
Thank you for reviewing my submission! 🚀