Add solution for Challenge 13 by blxxdclxud#1558
Add solution for Challenge 13 by blxxdclxud#1558github-actions[bot] merged 2 commits intoRezaSi:mainfrom
Conversation
WalkthroughA new Go file introduces a complete SQLite-backed product inventory system featuring a Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 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. 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
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 852d9240-5dad-4e06-9c10-ff7e1def749c
📒 Files selected for processing (1)
challenge-13/submissions/blxxdclxud/solution-template.go
| ctx, _ := context.WithTimeout(context.Background(), 5 * time.Second) | ||
| if err = db.PingContext(ctx); err != nil { | ||
| return nil, err | ||
| } |
There was a problem hiding this comment.
Context cancel function leaked.
The cancel function from context.WithTimeout is discarded. This can leak resources associated with the context. Always call the cancel function when the context is no longer needed.
🛠️ Proposed fix
- ctx, _ := context.WithTimeout(context.Background(), 5 * time.Second)
+ ctx, cancel := context.WithTimeout(context.Background(), 5 * time.Second)
+ defer cancel()
if err = db.PingContext(ctx); err != nil {
return nil, err
}📝 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.
| ctx, _ := context.WithTimeout(context.Background(), 5 * time.Second) | |
| if err = db.PingContext(ctx); err != nil { | |
| return nil, err | |
| } | |
| ctx, cancel := context.WithTimeout(context.Background(), 5 * time.Second) | |
| defer cancel() | |
| if err = db.PingContext(ctx); err != nil { | |
| return nil, err | |
| } |
| query := "UPDATE products SET name=$2, price=$3, quantity=$4, category=$5 WHERE id = $1" | ||
| _, err := ps.db.Exec(query, product.Name, product.Price, product.Quantity, product.Category, product.ID) | ||
| return err |
There was a problem hiding this comment.
Critical: Parameter order mismatch will cause incorrect updates.
The query uses $1 for id in the WHERE clause, but the parameters are passed with product.Name first. This means $1 receives the name instead of the ID, causing the update to either fail or modify incorrect rows.
🐛 Proposed fix
query := "UPDATE products SET name=$2, price=$3, quantity=$4, category=$5 WHERE id = $1"
- _, err := ps.db.Exec(query, product.Name, product.Price, product.Quantity, product.Category, product.ID)
+ _, err := ps.db.Exec(query, product.ID, product.Name, product.Price, product.Quantity, product.Category)
return err📝 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.
| query := "UPDATE products SET name=$2, price=$3, quantity=$4, category=$5 WHERE id = $1" | |
| _, err := ps.db.Exec(query, product.Name, product.Price, product.Quantity, product.Category, product.ID) | |
| return err | |
| query := "UPDATE products SET name=$2, price=$3, quantity=$4, category=$5 WHERE id = $1" | |
| _, err := ps.db.Exec(query, product.ID, product.Name, product.Price, product.Quantity, product.Category) | |
| return err |
| // DeleteProduct removes a product by ID | ||
| func (ps *ProductStore) DeleteProduct(id int64) error { | ||
| // TODO: Delete the product from the database | ||
| // TODO: Return an error if the product doesn't exist | ||
| query := "DELETE FROM products WHERE id = $1" | ||
| _, err := ps.db.Exec(query, id) | ||
| return err | ||
| } |
There was a problem hiding this comment.
Missing "product not found" error when deleting non-existent product.
Per the TODO comment, this method should return an error if the product doesn't exist. Currently, deleting a non-existent ID silently succeeds.
🐛 Proposed fix
func (ps *ProductStore) DeleteProduct(id int64) error {
// TODO: Delete the product from the database
// TODO: Return an error if the product doesn't exist
query := "DELETE FROM products WHERE id = $1"
- _, err := ps.db.Exec(query, id)
- return err
+ result, err := ps.db.Exec(query, id)
+ if err != nil {
+ return err
+ }
+ rowsAffected, err := result.RowsAffected()
+ if err != nil {
+ return err
+ }
+ if rowsAffected == 0 {
+ return sql.ErrNoRows
+ }
+ return nil
}📝 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.
| // DeleteProduct removes a product by ID | |
| func (ps *ProductStore) DeleteProduct(id int64) error { | |
| // TODO: Delete the product from the database | |
| // TODO: Return an error if the product doesn't exist | |
| query := "DELETE FROM products WHERE id = $1" | |
| _, err := ps.db.Exec(query, id) | |
| return err | |
| } | |
| // DeleteProduct removes a product by ID | |
| func (ps *ProductStore) DeleteProduct(id int64) error { | |
| // TODO: Delete the product from the database | |
| // TODO: Return an error if the product doesn't exist | |
| query := "DELETE FROM products WHERE id = $1" | |
| result, err := ps.db.Exec(query, id) | |
| if err != nil { | |
| return err | |
| } | |
| rowsAffected, err := result.RowsAffected() | |
| if err != nil { | |
| return err | |
| } | |
| if rowsAffected == 0 { | |
| return sql.ErrNoRows | |
| } | |
| return nil | |
| } |
| query := "SELECT * from products WHERE category = $1" | ||
| if category == "" { | ||
| query = "SELECT * from products" | ||
| } | ||
| var res []*Product | ||
| rows, err := ps.db.Query(query, category) | ||
| if err != nil { | ||
| return nil, err | ||
| } |
There was a problem hiding this comment.
Bug: Parameter passed to query without placeholders when category is empty.
When category == "", the query is set to "SELECT * from products" (no placeholders), but ps.db.Query(query, category) still passes the category parameter. This can cause unexpected behavior or errors.
🐛 Proposed fix
- query := "SELECT * from products WHERE category = $1"
- if category == "" {
- query = "SELECT * from products"
- }
var res []*Product
- rows, err := ps.db.Query(query, category)
+ var rows *sql.Rows
+ var err error
+ if category == "" {
+ rows, err = ps.db.Query("SELECT * FROM products")
+ } else {
+ rows, err = ps.db.Query("SELECT * FROM products WHERE category = $1", category)
+ }
if err != nil {
return nil, err
}
+ defer rows.Close()📝 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.
| query := "SELECT * from products WHERE category = $1" | |
| if category == "" { | |
| query = "SELECT * from products" | |
| } | |
| var res []*Product | |
| rows, err := ps.db.Query(query, category) | |
| if err != nil { | |
| return nil, err | |
| } | |
| var res []*Product | |
| var rows *sql.Rows | |
| var err error | |
| if category == "" { | |
| rows, err = ps.db.Query("SELECT * FROM products") | |
| } else { | |
| rows, err = ps.db.Query("SELECT * FROM products WHERE category = $1", category) | |
| } | |
| if err != nil { | |
| return nil, err | |
| } | |
| defer rows.Close() |
| tx, _ := ps.db.BeginTx(context.Background(), nil) | ||
| defer tx.Rollback() |
There was a problem hiding this comment.
Transaction start error is ignored, risking nil pointer panic.
If BeginTx fails, tx will be nil and the subsequent defer tx.Rollback() will panic.
🐛 Proposed fix
- tx, _ := ps.db.BeginTx(context.Background(), nil)
+ tx, err := ps.db.BeginTx(context.Background(), nil)
+ if err != nil {
+ return err
+ }
defer tx.Rollback()📝 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.
| tx, _ := ps.db.BeginTx(context.Background(), nil) | |
| defer tx.Rollback() | |
| tx, err := ps.db.BeginTx(context.Background(), nil) | |
| if err != nil { | |
| return err | |
| } | |
| defer tx.Rollback() |
| tx.Commit() | ||
| return nil |
There was a problem hiding this comment.
Commit error is ignored.
If the transaction commit fails (e.g., constraint violation, I/O error), the error is silently discarded. The function returns nil even though the updates may not have persisted.
🐛 Proposed fix
- tx.Commit()
- return nil
+ return tx.Commit()📝 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.
| tx.Commit() | |
| return nil | |
| return tx.Commit() |
|
🎉 Auto-merged! This PR was automatically merged after 2 days with all checks passing. Thank you for your contribution, @blxxdclxud! |
2 participants
Challenge 13 Solution
Submitted by: @blxxdclxud
Challenge: Challenge 13
Description
This PR contains my solution for Challenge 13.
Changes
challenge-13/submissions/blxxdclxud/solution-template.goTesting
Thank you for reviewing my submission! 🚀