bug#65998: Sqlite-mode issue deleting records and closing database

bug#65998: Sqlite-mode issue deleting records and closing database

[Thomas Hilke via Bug reports for GNU Emacs, the Swiss army knife of text editors]

[-- Attachment #1.1: Type: text/plain, Size: 1590 bytes --]

Hi everyone,

I had the opportunity of using sqlite-mode recently, which is really
handy for quickly inspecting the content of a database. However, I
noticed two issues when using it from Windows:

- The sql query built by sqlite-mode-delete is syntactically correct,
  but misinterpreted by sqlite, and eventually do nothing. The query
  is of the form "REMOVE FROM table_name WHERE rowid = ? and
  'column_name_1' = ? and 'column_name_2' = ? and ..."  From
  https://www.sqlite.org/lang_keywords.html, if I understand
  correctly, the quoted column names in the WHERE clause are
  interpreted as strings from sqlite, and as a result not a single row
  is ever matched and deleted.

- The connection to the sqlite database (file) is never closed, even
  when the buffer is killed. As sqlite--db is a local variable,
  it's not even possible to close the connection by hand once the
  buffer is killed. That means that once a database file is opened
  with sqlite-mode-open-file, the file cannot be deleted unless
  emacs is closed (on Windows).

Attached are the modifications that makes it work for me. I didn't have
the opportunity to test it under linux or another environment.

I did not want to touch the other parts that were already working, but
note that sqlite support a collection of pragma statements that can be
used to inspect the schema of the database in a more structured way
than parsing the content of the sqlite_master table:
https://www.sqlite.org/lang_keywords.html.

I hope it helps, and thanks for all the great work!

Thomas Hilke

[-- Attachment #1.2: Type: text/html, Size: 10917 bytes --]

[-- Attachment #2: 0001-Remove-column-quoting-and-close-sqlite-db-on-buffer-.patch --]
[-- Type: application/octet-stream, Size: 1690 bytes --]

From fa7b7df9f174b2612a620223266ea4015644e177 Mon Sep 17 00:00:00 2001
From: Thomas Hilke <t.hilke@rollomatic.ch>
Date: Fri, 15 Sep 2023 10:30:25 +0200
Subject: [PATCH] Remove column quoting and close sqlite db on buffer kill

---
 lisp/sqlite-mode.el | 18 ++----------------
 1 file changed, 2 insertions(+), 16 deletions(-)

diff --git a/lisp/sqlite-mode.el b/lisp/sqlite-mode.el
index 8cb94485369..38e9f84b842 100644
--- a/lisp/sqlite-mode.el
+++ b/lisp/sqlite-mode.el
@@ -63,6 +63,7 @@
   (setq-local sqlite--db (sqlite-open file))
   (unless (sqlitep sqlite--db)
     (error "`sqlite-open' failed to open SQLite file"))
+  (add-hook 'kill-buffer-hook (lambda () (sqlite-close sqlite--db)) nil t)
   (sqlite-mode-list-tables))
 
 (defun sqlite-mode-list-tables ()
@@ -135,22 +136,7 @@
 
 (defun sqlite-mode--column-names (table)
   "Return a list of the column names for TABLE."
-  (let ((sql
-         (caar
-          (sqlite-select
-           sqlite--db
-           "select sql from sqlite_master where tbl_name = ? AND type = 'table'"
-           (list table)))))
-    (with-temp-buffer
-      (insert sql)
-      (mapcar #'string-trim
-              (split-string
-               ;; Extract the args to CREATE TABLE.  Point is
-               ;; currently at its end.
-               (buffer-substring
-                (1- (point))                          ; right before )
-                (1+ (progn (backward-sexp) (point)))) ; right after (
-               ",")))))
+  (mapcar (lambda (row) (nth 1 row)) (sqlite-select sqlite--db (format "pragma table_info(%s)" table))))
 
 (defun sqlite-mode-list-data ()
   "List the data from the table under point."
-- 
2.41.0.windows.3

bug#65998: Sqlite-mode issue deleting records and closing database

[Eli Zaretskii]

> Date: Fri, 15 Sep 2023 09:02:49 +0000
> From:  Thomas Hilke via "Bug reports for GNU Emacs,
>  the Swiss army knife of text editors" <bug-gnu-emacs@gnu.org>
> 
> I had the opportunity of using sqlite-mode recently, which is really
> handy for quickly inspecting the content of a database. However, I
> noticed two issues when using it from Windows:
> 
> - The sql query built by sqlite-mode-delete is syntactically correct,
>   but misinterpreted by sqlite, and eventually do nothing. The query
>   is of the form "REMOVE FROM table_name WHERE rowid = ? and
>   'column_name_1' = ? and 'column_name_2' = ? and ..."  From
>   https://www.sqlite.org/lang_keywords.html, if I understand
>   correctly, the quoted column names in the WHERE clause are
>   interpreted as strings from sqlite, and as a result not a single row
>   is ever matched and deleted.
> 
> - The connection to the sqlite database (file) is never closed, even
>   when the buffer is killed. As sqlite--db is a local variable,
>   it's not even possible to close the connection by hand once the
>   buffer is killed. That means that once a database file is opened
>   with sqlite-mode-open-file, the file cannot be deleted unless
>   emacs is closed (on Windows).
> 
> Attached are the modifications that makes it work for me. I didn't have
> the opportunity to test it under linux or another environment.

Thanks.  I installed on the emacs-29 branch the first part of your
patch, which closes the DB when the buffer is killed.  As for the
second part, I'd prefer that Lars or someone who knows SQL reviewed it
first, as I find it strange that Lars would code something so basic
which doesn't work at all.

Could someone who knows SQL please review and chime in?

bug#65998: Sqlite-mode issue deleting records and closing database

[Stefan Kangas]

Eli Zaretskii <eliz@gnu.org> writes:

>> - The sql query built by sqlite-mode-delete is syntactically correct,
>>   but misinterpreted by sqlite, and eventually do nothing. The query
>>   is of the form "REMOVE FROM table_name WHERE rowid = ? and
>>   'column_name_1' = ? and 'column_name_2' = ? and ..."  From
>>   https://www.sqlite.org/lang_keywords.html, if I understand
>>   correctly, the quoted column names in the WHERE clause are
>>   interpreted as strings from sqlite, and as a result not a single row
>>   is ever matched and deleted.
>
> Thanks.  I installed on the emacs-29 branch the first part of your
> patch, which closes the DB when the buffer is killed.  As for the
> second part, I'd prefer that Lars or someone who knows SQL reviewed it
> first, as I find it strange that Lars would code something so basic
> which doesn't work at all.
>
> Could someone who knows SQL please review and chime in?

Something like this in an SQL "REMOVE FROM table_name WHERE {foo}" clause

    'column_name_1' = ?

will check if the '?' part is equal to the string 'column_name_1', which
is probably not what we want.

Whereas this

    column_name_1 = ?

will instead check if the '?' part is equal to the value of the column
column_name_1 in table_name.

(The "?" is just a placeholder that will be filled in with an actual
value later.)

So without having tested the code or studied it in detail, the analysis
of the problem sounds right to me.

Don't we have unit tests in place for this stuff, though?  Perhaps we
should see this as an opportunity to add some...

bug#65998: Sqlite-mode issue deleting records and closing database

[Eli Zaretskii]

> From: Stefan Kangas <stefankangas@gmail.com>
> Date: Sat, 16 Sep 2023 07:07:51 -0700
> Cc: 65998@debbugs.gnu.org
> 
> Eli Zaretskii <eliz@gnu.org> writes:
> 
> >> - The sql query built by sqlite-mode-delete is syntactically correct,
> >>   but misinterpreted by sqlite, and eventually do nothing. The query
> >>   is of the form "REMOVE FROM table_name WHERE rowid = ? and
> >>   'column_name_1' = ? and 'column_name_2' = ? and ..."  From
> >>   https://www.sqlite.org/lang_keywords.html, if I understand
> >>   correctly, the quoted column names in the WHERE clause are
> >>   interpreted as strings from sqlite, and as a result not a single row
> >>   is ever matched and deleted.
> >
> > Thanks.  I installed on the emacs-29 branch the first part of your
> > patch, which closes the DB when the buffer is killed.  As for the
> > second part, I'd prefer that Lars or someone who knows SQL reviewed it
> > first, as I find it strange that Lars would code something so basic
> > which doesn't work at all.
> >
> > Could someone who knows SQL please review and chime in?
> 
> Something like this in an SQL "REMOVE FROM table_name WHERE {foo}" clause
> 
>     'column_name_1' = ?
> 
> will check if the '?' part is equal to the string 'column_name_1', which
> is probably not what we want.
> 
> Whereas this
> 
>     column_name_1 = ?
> 
> will instead check if the '?' part is equal to the value of the column
> column_name_1 in table_name.
> 
> (The "?" is just a placeholder that will be filled in with an actual
> value later.)
> 
> So without having tested the code or studied it in detail, the analysis
> of the problem sounds right to me.

Thanks, so I've now installed the other part on the emacs-29 branch,
and I'm closing the bug.

> Don't we have unit tests in place for this stuff, though?  Perhaps we
> should see this as an opportunity to add some...

I agree, patches are welcome.