Merge pull request #106 from seapagan/fix/transaction-rollback-issue-104

Fix transaction rollback bug (Issue #104)

Author: seapagan

docs/guide/transactions.md

@@ -9,16 +9,8 @@ with db:
     # If an exception occurs, the transaction will be rolled back
 ```
 
-> [!WARNING]
-> **Known Issue:** Transaction rollback is currently broken. Changes made
-> inside a transaction are NOT rolled back when an exception occurs.
->
-> **Status:** This is being tracked in [issue #104](https://github.com/seapagan/sqliter-py/issues/104).
->
-> **Workaround:** Do not rely on transaction rollback for data integrity until
-> this is fixed. All changes are committed immediately.
->
-> Using the context manager will automatically commit the transaction at the end
-> (unless an exception occurs), regardless of the `auto_commit` setting. The
-> `close()` method will also be called when the context manager exits, so you
-> do not need to call it manually.
+Using the context manager will automatically commit the transaction at the end
+(unless an exception occurs), regardless of the `auto_commit` setting. If an
+exception occurs, all changes made within the transaction block are rolled back.
+The `close()` method will also be called when the context manager exits, so you
+do not need to call it manually.

docs/tui-demo/errors.md

@@ -114,8 +114,8 @@ print("\nAttempting to create product with invalid data...")
 
 try:
     # Wrong types: price should be float, quantity should be int
-    invalid_product = Product(name="Invalid Widget", price="free", quantity="lots")
-    db.insert(invalid_product)
+    # ValidationError is raised by Pydantic during model instantiation
+    Product(name="Invalid Widget", price="free", quantity="lots")
 except ValidationError as e:
     print(f"\nCaught error: {type(e).__name__}")
     print(f"Message: {e}")
@@ -316,12 +316,12 @@ try:
         raise ValueError("Invalid operation")
 except ValueError as e:
     print(f"Transaction failed: {e}")
-    # Note: Changes are NOT rolled back due to bug (issue #104)
+    print("Changes rolled back automatically")
 
-# Verify balance unchanged
+# Verify balance unchanged (rollback restored original value)
 reloaded = db.get(Account, account.pk)
 if reloaded is not None:
-    print(f"Balance: {reloaded.balance}")  # Was 100.0
+    print(f"Balance: {reloaded.balance}")  # Still 100.0
 ```
 
 ## Error Handling Best Practices

docs/tui-demo/transactions.md

@@ -43,8 +43,8 @@ db.close()
 
 ### What Happens
 
-- Both updates succeed or both fail (intended behaviour)
-- If an error occurs, changes should be rolled back (see warning below about current limitations)
+- Both updates succeed or both fail
+- If an error occurs, changes are rolled back
 - Database remains in a consistent state
 
 ## Transaction Rollback
@@ -83,12 +83,6 @@ db.close()
 # --8<-- [end:transaction-rollback]
 ```
 
-!!! warning
-    **Known Issue:** Transaction rollback is currently broken in SQLiter.
-    The `update()`, `insert()`, and `delete()` methods use nested context
-    managers that commit prematurely. This demo shows the expected behavior,
-    but actual rollback may not work correctly until this is fixed.
-
 ### Rollback Behavior
 
 - All changes within the transaction should be undone

sqliter/query/query.py

@@ -601,10 +601,10 @@ def _execute_query(
             self.db._log_sql(sql, values)  # noqa: SLF001
 
         try:
-            with self.db.connect() as conn:
-                cursor = conn.cursor()
-                cursor.execute(sql, values)
-                return cursor.fetchall() if not fetch_one else cursor.fetchone()
+            conn = self.db.connect()
+            cursor = conn.cursor()
+            cursor.execute(sql, values)
+            return cursor.fetchall() if not fetch_one else cursor.fetchone()
         except sqlite3.Error as exc:
             raise RecordFetchError(self.table_name) from exc
 
@@ -911,12 +911,16 @@ def delete(self) -> int:
             self.db._log_sql(sql, values)  # noqa: SLF001
 
         try:
-            with self.db.connect() as conn:
-                cursor = conn.cursor()
-                cursor.execute(sql, values)
-                deleted_count = cursor.rowcount
-                self.db._maybe_commit()  # noqa: SLF001
-                self.db._cache_invalidate_table(self.table_name)  # noqa: SLF001
-                return deleted_count
+            conn = self.db.connect()
+            cursor = conn.cursor()
+            cursor.execute(sql, values)
+            deleted_count = cursor.rowcount
+            self.db._maybe_commit()  # noqa: SLF001
+            self.db._cache_invalidate_table(self.table_name)  # noqa: SLF001
         except sqlite3.Error as exc:
+            # Rollback implicit transaction if not in user-managed transaction
+            if not self.db._in_transaction and self.db.conn:  # noqa: SLF001
+                self.db.conn.rollback()
             raise RecordDeletionError(self.table_name) from exc
+        else:
+            return deleted_count

sqliter/sqliter.py

@@ -888,12 +888,15 @@ def insert(
         """  # noqa: S608
 
         try:
-            with self.connect() as conn:
-                cursor = conn.cursor()
-                cursor.execute(insert_sql, values)
-                self._maybe_commit()
+            conn = self.connect()
+            cursor = conn.cursor()
+            cursor.execute(insert_sql, values)
+            self._maybe_commit()
 
         except sqlite3.IntegrityError as exc:
+            # Rollback implicit transaction if not in user-managed transaction
+            if not self._in_transaction and self.conn:
+                self.conn.rollback()
             # Check for foreign key constraint violation
             if "FOREIGN KEY constraint failed" in str(exc):
                 fk_operation = "insert"
@@ -903,6 +906,9 @@ def insert(
                 ) from exc
             raise RecordInsertionError(table_name) from exc
         except sqlite3.Error as exc:
+            # Rollback implicit transaction if not in user-managed transaction
+            if not self._in_transaction and self.conn:
+                self.conn.rollback()
             raise RecordInsertionError(table_name) from exc
         else:
             self._cache_invalidate_table(table_name)
@@ -936,10 +942,10 @@ def get(
         """  # noqa: S608
 
         try:
-            with self.connect() as conn:
-                cursor = conn.cursor()
-                cursor.execute(select_sql, (primary_key_value,))
-                result = cursor.fetchone()
+            conn = self.connect()
+            cursor = conn.cursor()
+            cursor.execute(select_sql, (primary_key_value,))
+            result = cursor.fetchone()
 
             if result:
                 result_dict = {
@@ -990,18 +996,26 @@ def update(self, model_instance: BaseDBModel) -> None:
         """  # noqa: S608
 
         try:
-            with self.connect() as conn:
-                cursor = conn.cursor()
-                cursor.execute(update_sql, (*values, primary_key_value))
+            conn = self.connect()
+            cursor = conn.cursor()
+            cursor.execute(update_sql, (*values, primary_key_value))
 
-                # Check if any rows were updated
-                if cursor.rowcount == 0:
-                    raise RecordNotFoundError(primary_key_value)
+            # Check if any rows were updated
+            if cursor.rowcount == 0:
+                raise RecordNotFoundError(primary_key_value)  # noqa: TRY301
 
-                self._maybe_commit()
-                self._cache_invalidate_table(table_name)
+            self._maybe_commit()
+            self._cache_invalidate_table(table_name)
 
+        except RecordNotFoundError:
+            # Rollback implicit transaction if not in user-managed transaction
+            if not self._in_transaction and self.conn:
+                self.conn.rollback()
+            raise
         except sqlite3.Error as exc:
+            # Rollback implicit transaction if not in user-managed transaction
+            if not self._in_transaction and self.conn:
+                self.conn.rollback()
             raise RecordUpdateError(table_name) from exc
 
     def delete(
@@ -1026,15 +1040,23 @@ def delete(
         """  # noqa: S608
 
         try:
-            with self.connect() as conn:
-                cursor = conn.cursor()
-                cursor.execute(delete_sql, (primary_key_value,))
+            conn = self.connect()
+            cursor = conn.cursor()
+            cursor.execute(delete_sql, (primary_key_value,))
 
-                if cursor.rowcount == 0:
-                    raise RecordNotFoundError(primary_key_value)
-                self._maybe_commit()
-                self._cache_invalidate_table(table_name)
+            if cursor.rowcount == 0:
+                raise RecordNotFoundError(primary_key_value)  # noqa: TRY301
+            self._maybe_commit()
+            self._cache_invalidate_table(table_name)
+        except RecordNotFoundError:
+            # Rollback implicit transaction if not in user-managed transaction
+            if not self._in_transaction and self.conn:
+                self.conn.rollback()
+            raise
         except sqlite3.IntegrityError as exc:
+            # Rollback implicit transaction if not in user-managed transaction
+            if not self._in_transaction and self.conn:
+                self.conn.rollback()
             # Check for foreign key constraint violation (RESTRICT)
             if "FOREIGN KEY constraint failed" in str(exc):
                 fk_operation = "delete"
@@ -1044,6 +1066,9 @@ def delete(
                 ) from exc
             raise RecordDeletionError(table_name) from exc
         except sqlite3.Error as exc:
+            # Rollback implicit transaction if not in user-managed transaction
+            if not self._in_transaction and self.conn:
+                self.conn.rollback()
             raise RecordDeletionError(table_name) from exc
 
     def select(

sqliter/tui/demos/errors.py

@@ -102,10 +102,8 @@ class Product(BaseDBModel):
 
     try:
         # Wrong types: price should be float, quantity should be int
-        invalid_product = Product(
-            name="Invalid Widget", price="free", quantity="lots"
-        )
-        db.insert(invalid_product)
+        # ValidationError is raised by Pydantic during model instantiation
+        Product(name="Invalid Widget", price="free", quantity="lots")
     except ValidationError as e:
         output.write(f"\nCaught error: {type(e).__name__}\n")
         output.write(f"Message: {e}\n")

sqliter/tui/demos/transactions.py

@@ -51,9 +51,8 @@ class Account(BaseDBModel):
 def _run_rollback() -> str:
     """Demonstrate transaction rollback behavior.
 
-    NOTE: This demo currently shows a BUG in SQLiter's transaction handling.
-    The value should be restored to 10 after rollback, but it's not.
-    See: https://github.com/seapagan/sqliter-py/issues/104
+    When an exception occurs inside a `with db:` block, all changes made
+    within that transaction are automatically rolled back.
     """
     output = io.StringIO()
 
@@ -85,7 +84,6 @@ class Item(BaseDBModel):
         except RuntimeError:
             output.write("Error occurred - transaction rolled back\n")
             # Verify rollback with NEW connection
-            # BUG: This shows 5 instead of 10 - rollback doesn't work!
             db2 = SqliterDB(db_filename=db_path)
             try:
                 restored = db2.get(Item, item.pk)
@@ -98,9 +96,11 @@ class Item(BaseDBModel):
                     if restored_quantity == expected_quantity:
                         output.write("✓ Rollback worked correctly\n")
                     else:
-                        output.write(
-                            "✗ BUG: Rollback failed (expected 10, got 5)\n"
+                        msg = (
+                            f"✗ Rollback failed: expected {expected_quantity}, "
+                            f"got {restored_quantity}\n"
                         )
+                        output.write(msg)
             finally:
                 db2.close()
     finally: