bug#67142: 29.1; with-sqlite-transaction commits on exception rather than rolling back

["J.P." <jp@neverwas.me>]

[-- Attachment #1: Type: text/plain, Size: 332 bytes --]

"J.P." <jp@neverwas.me> writes:

> Not obvious on account of the indentation, but it seems the second
>
>   (funcall ,func-var)
>
> snuck outside the `if' form with that fix, making body run twice if you
> have SQLite.

I guess folks are too busy to get excited about fixing this. Here's a
patch in case that ever changes. Thanks.


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-Run-successful-body-once-in-with-sqlite-transaction.patch --]
[-- Type: text/x-patch, Size: 3783 bytes --]

From e9319447a1403736cd8aac766d29ba2d98591eff Mon Sep 17 00:00:00 2001
From: "F. Jason Park" <jp@neverwas.me>
Date: Sun, 10 Mar 2024 20:13:42 -0700
Subject: [PATCH] Run successful body once in with-sqlite-transaction

* lisp/sqlite.el (with-sqlite-transaction): Tuck misplaced body of
else form back into feature-test control structure whence it escaped.
* test/lisp/sqlite-tests.el: New file to accompany
test/src/sqlite-tests.el.
(Bug#67142)
---
 lisp/sqlite.el            |  7 +++---
 test/lisp/sqlite-tests.el | 51 +++++++++++++++++++++++++++++++++++++++
 2 files changed, 55 insertions(+), 3 deletions(-)
 create mode 100644 test/lisp/sqlite-tests.el

diff --git a/lisp/sqlite.el b/lisp/sqlite.el
index 46e35ac18d8..efc5997fb5c 100644
--- a/lisp/sqlite.el
+++ b/lisp/sqlite.el
@@ -32,7 +32,8 @@ with-sqlite-transaction
 If BODY completes normally, commit the changes and return
 the value of BODY.
 If BODY signals an error, or transaction commit fails, roll
-back the transaction changes."
+back the transaction changes before allowing the signal to
+propagate."
   (declare (indent 1) (debug (form body)))
   (let ((db-var (gensym))
         (func-var (gensym))
@@ -48,8 +49,8 @@ with-sqlite-transaction
                  (setq ,res-var (funcall ,func-var))
                  (setq ,commit-var (sqlite-commit ,db-var))
                  ,res-var)
-             (or ,commit-var (sqlite-rollback ,db-var))))
-         (funcall ,func-var))))
+             (or ,commit-var (sqlite-rollback ,db-var)))
+         (funcall ,func-var)))))
 
 (provide 'sqlite)
 
diff --git a/test/lisp/sqlite-tests.el b/test/lisp/sqlite-tests.el
new file mode 100644
index 00000000000..d4892a27efc
--- /dev/null
+++ b/test/lisp/sqlite-tests.el
@@ -0,0 +1,51 @@
+;;; sqlite-tests.el --- Tests for sqlite.el  -*- lexical-binding: t; -*-
+
+;; Copyright (C) 2024 Free Software Foundation, Inc.
+
+;; This file is part of GNU Emacs.
+
+;; GNU Emacs is free software: you can redistribute it and/or modify
+;; it under the terms of the GNU General Public License as published by
+;; the Free Software Foundation, either version 3 of the License, or
+;; (at your option) any later version.
+
+;; GNU Emacs is distributed in the hope that it will be useful,
+;; but WITHOUT ANY WARRANTY; without even the implied warranty of
+;; MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+;; GNU General Public License for more details.
+
+;; You should have received a copy of the GNU General Public License
+;; along with GNU Emacs.  If not, see <https://www.gnu.org/licenses/>.
+
+;;; Commentary:
+
+;;; Code:
+(require 'sqlite)
+
+(ert-deftest with-sqlite-transaction ()
+  (skip-unless (sqlite-available-p))
+  (let ((db (sqlite-open)))
+    (sqlite-execute db "create table test (a)")
+    (should
+     (eql 42 (with-sqlite-transaction db
+               (sqlite-execute db "insert into test values (1)")
+               (should (equal '((1)) (sqlite-select db "select * from test")))
+               42)))
+    ;; Body runs exactly once.
+    (should (equal '((1)) (sqlite-select db "select * from test")))))
+
+(ert-deftest with-sqlite-transaction/rollback ()
+  (skip-unless (sqlite-available-p))
+  (let ((db (sqlite-open)))
+    (sqlite-execute db "create table test (a)")
+    (should (equal '(sqlite-error
+                     ("SQL logic error" "no such function: fake" 1 1))
+                   (should-error
+                    (with-sqlite-transaction db
+                      (sqlite-execute db "insert into test values (1)")
+                      (sqlite-execute db "insert into test values (fake(2))")
+                      42))))
+    ;; First insertion (a=1) rolled back.
+    (should-not (sqlite-select db "select * from test"))))
+
+;;; sqlite-tests.el ends here
-- 
2.44.0