Add transaction for creating an order#198
Conversation
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
I am requesting changes because your current transaction setup breaks an important atomicity requirement from the task. Right now, the INSERT INTO Orders runs before START TRANSACTION (near the top of task.sql), so creating the order is not part of the same transaction as inserting into OrderItems and updating Products.WarehouseAmount. This means the order could exist without its item or stock change if an error happens later, which violates the business rule that these operations should succeed or roll back together [CHECKLIST ITEM #17].
To fix this, move START TRANSACTION above the INSERT INTO Orders statement so that the order, its items, and the warehouse update are all inside one transaction. After that, keep using LAST_INSERT_ID() for OrderItems.OrderID, but now it will correctly refer to the order created within the same transaction. These changes are critical to ensure the script behaves correctly and consistently under failure scenarios and concurrent usage. Once you adjust the transaction boundaries, your solution will better match the intended design and should be ready to pass.
✨ Please rate this review in the Platform's notification center1. I will be grateful for your feedback! ✨
Important
Fix the issues, commit the changes, and then re-request my review using the Re-request review button2 in the Reviewers tab.
Tip
If you have any comments about this review, please leave a message in the Add a comment section below and mention me with the @mateacademy-ai-mentor tag3.
I will read your message during my next review of your pull request. Please do not reply directly to my comments, as that feature is not supported yet.
Caution
Do not close the pull request until the review is complete. Closing it early can lead to unexpected behavior and disrupt the review process.
Footnotes
|
|
||
| -- Start the transaction | ||
| USE ShopDB; | ||
| INSERT INTO Orders (CustomerID, Date) VALUES (1, '2023-01-01'); |
There was a problem hiding this comment.
The INSERT INTO Orders statement is executed before START TRANSACTION, so creating the order is not part of the atomic transaction with the order item insertion and warehouse update. According to the business rules and checklist item #17, consider moving this insert inside the transaction so that order creation, OrderItems, and product stock updates either all succeed or all roll back together.
There was a problem hiding this comment.
There's a conflict here, and I'd like to clarify the correct approach.
I originally placed INSERT INTO Orders outside the transaction on purpose, because the automated test in test.sh (GitHub Actions) requires it. There's an explicit check:
insertIntoOrders=$(echo $afterTransactionStart | grep "INSERT INTO Orders" || true)
if [ -z "$insertIntoOrders" ]; then
echo "checked if Orders are updated outside of the transaction - ok"
else
echo "error: Orders are updated inside of the transaction"
exit 1
fi
This test looks for INSERT INTO Orders after START TRANSACTION and fails with exit 1 if it finds it. It requires order creation to be outside the transaction.
Your review asks for the opposite — to move INSERT INTO Orders inside the transaction. If I do that, the CI test will fail.
|
|
||
| -- And some data should be created inside the transaction | ||
|
|
||
| INSERT INTO OrderItems (ProductID, Count, OrderID) VALUES (1, 1, LAST_INSERT_ID()); |
There was a problem hiding this comment.
LAST_INSERT_ID() here depends on the previous insert into Orders, which currently happens outside the transaction. After moving the Orders insert into the transaction, this will correctly link the OrderItems row to the newly created order as part of the same atomic operation.
No description provided.