Repository: beetbox/beets
Pull request: Reworked #4709 after latest release
## Description

Fixes #4709 SQL use of Double Quoted Strings.

Replacing #5236




- [X] Changelog. (Add an entry to `docs/changelog.rst` to the bottom of one of the lists near the top of the document.)


Problem statement:
Avoid using double-quoted string literals in SQL queries
### Problem

With a default beets installation (via pkg or ports) on FreeBSD (13.1), any action requiring interaction with the SQLite database will cause beets to crash.

This is due to Double-quoted String Literals being disabled by default in the 3.41.0,1 version of SQLite - enabling DQS in the make options for the SQLite port corrects this issue and allows beets to complete database functions normally.

The maintainers of the SQLite port on FreeBSD have since re-enabled DQS by default but are planning to disable DQS by default no later than 20240101.


Running this command in verbose (`-vv`) mode:

```sh
$ beet -vv ls
```

Led to this problem:

```
Traceback (most recent call last):
  File "/usr/local/bin/beet", line 33, in <module>
    sys.exit(load_entry_point('beets==1.6.0', 'console_scripts', 'beet')())
             ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/usr/local/lib/python3.11/site-packages/beets/ui/__init__.py", line 1285, in main
    _raw_main(args)
  File "/usr/local/lib/python3.11/site-packages/beets/ui/__init__.py", line 1272, in _raw_main
    subcommand.func(lib, suboptions, subargs)
  File "/usr/local/lib/python3.11/site-packages/beets/ui/commands.py", line 1089, in list_func
    list_items(lib, decargs(args), opts.album)
  File "/usr/local/lib/python3.11/site-packages/beets/ui/commands.py", line 1084, in list_items
    for item in lib.items(query):
                ^^^^^^^^^^^^^^^^
  File "/usr/local/lib/python3.11/site-packages/beets/library.py", line 1529, in items
    return self._fetch(Item, query, sort or self.get_default_item_sort())
           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/usr/local/lib/python3.11/site-packages/beets/library.py", line 1503, in _fetch
    return super()._fetch(
           ^^^^^^^^^^^^^^^
  File "/usr/local/lib/python3.11/site-packages/beets/dbcore/db.py", line 1093, in _fetch
    rows = tx.query(sql, subvals)
           ^^^^^^^^^^^^^^^^^^^^^^
  File "/usr/local/lib/python3.11/site-packages/beets/dbcore/db.py", line 858, in query
    cursor = self.db._connection().execute(statement, subvals)
             ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
sqlite3.OperationalError: no such column:
```


### Setup

* OS: FreeBSD 13.1
* Python version: 3.9 and 3.11
* beets version: 1.6.0
* Turning off plugins made problem go away (yes/no): no


My configuration (output of `beet config`) is:

```yaml
library: library.db
directory: /mnt/Music/
import:
   copy: yes
   move: no
```

Hints from issue discussion:
Wow; thank you for pointing this out! This looks very serious. For a little more background on the issue, the SQLite docs discuss the syntax compatibility problem:
https://www.sqlite.org/quirks.html#double_quoted_string_literals_are_accepted

Unfortunately, it is going to be tricky in general to track down where we are using this incorrect syntax. Based on the clue from this error message, suggesting that the sting literal in question is empty, I found one such instance:
https://github.com/beetbox/beets/blob/e51206ae168552983d8b4292d8af49da43fb0886/beets/library.py#L274

That should be `''` instead of `""`. But it's hard to tell if that's the only issue. We should start here, though, and hopefully run the whole test suite with DQS disabled to see if there are more similar crashes.
> [...] and hopefully run the whole test suite with DQS disabled to see if there are more similar crashes.

Unfortunately, `sqlite3_db_config` does not appear to be exposed in any way in Python, so we can't disable DQS at runtime :/
> > [...] and hopefully run the whole test suite with DQS disabled to see if there are more similar crashes.
> 
> Unfortunately, `sqlite3_db_config` does not appear to be exposed in any way in Python, so we can't disable DQS at runtime :/

I'm not a Python programmer and I find the namespace and pypi more confusing than dealing with any other languages repositories, but I came across this:

https://docs.python.org/3/library/sqlite3.html#sqlite3.Connection.setconfig

Which seems to expose the thing that's needed for this to work on FreeBSD.

```python
con.setconfig(sqlite3.SQLITE_DBCONFIG_DQS_DDL, 1)
con.setconfig(sqlite3.SQLITE_DBCONFIG_DQS_DML, 1)
```

That should work on all version of sqlite3 if I'm reading the docs correctly.

UPDATE: Of course, `import sqlite3` isn't the same as the docs for this `import sqlite3`.. This is one of the bigger issues I have with python. There's no way for me to look at a script and figure out which of the things `import sqlite3` means.

Nevermind, whatever is satisfying `import sqlite3` for the `beets` project doesn't expose `Connection.setconfig()`. Yay!
FreeBSD appears to have disabled DQS again as foretold. Is there any change I can locally apply to get my setup running again?  Looks like the setconfig bits aren't it, yeah?
> That should work on all version of sqlite3 if I'm reading the docs correctly.
> 
> UPDATE: Of course, `import sqlite3` isn't the same as the docs for this `import sqlite3`.. This is one of the bigger issues I have with python. There's no way for me to look at a script and figure out which of the things `import sqlite3` means.
> 
> Nevermind, whatever is satisfying `import sqlite3` for the `beets` project doesn't expose `Connection.setconfig()`. Yay!

The problem is likely that `setconfig` was only added in Python 3.12. But we should try to use it conditionally there, which would get us a check by CI which in turn should be sufficient to track down any remaining DQS usage.

EDIT: I'll open a PR for this later.
A quick look throughthe code I found the reference @sampsyo found, as well as the following 2 references in [beets/dbcore/query.py](https://github.com/beetbox/beets/blob/master/beets/dbcore/query.py#L1011-1012).

I am having trouble testing on windows running python 3.12.3, so looking to help out. I'll start a discussion about windows testing
If you want to get things working locally on FreeBSD while this issue is still unresolved, you can build sqlite3 from ports with DQS enabled via `make config `.
> If you want to get things working locally on FreeBSD while this issue is still unresolved, you can build sqlite3 from ports with DQS enabled via `make config `.

@FST777 my system is using binary packages and I'm not a fan of mixing ports and packages. but that said, there is a less risky approach, turns out. I built a pyenv of python 3.12 last night, installed beets from git, and manually applied https://github.com/beetbox/beets/pull/5235 , setting the values to 1 instead. on freebsd 13.1, that magically got beets up and running again.

Commit message: Reworked #4709 after latest release

Patch to review:
diff --git a/beets/dbcore/query.py b/beets/dbcore/query.py
index f8cf7fe4c2..5088b1e776 100644
--- a/beets/dbcore/query.py
+++ b/beets/dbcore/query.py
@@ -1040,8 +1040,8 @@ def order_clause(self) -> str:
         if self.case_insensitive:
             field = (
                 "(CASE "
-                'WHEN TYPEOF({0})="text" THEN LOWER({0}) '
-                'WHEN TYPEOF({0})="blob" THEN LOWER({0}) '
+                "WHEN TYPEOF({0})='text' THEN LOWER({0}) "
+                "WHEN TYPEOF({0})='blob' THEN LOWER({0}) "
                 "ELSE {0} END)".format(self.field)
             )
         else:
diff --git a/beets/library.py b/beets/library.py
index e8e0f0f83c..1f099e2d02 100644
--- a/beets/library.py
+++ b/beets/library.py
@@ -312,11 +312,8 @@ def order_clause(self):
         order = "ASC" if self.ascending else "DESC"
         field = "albumartist" if self.album else "artist"
         collate = "COLLATE NOCASE" if self.case_insensitive else ""
-        return (
-            "(CASE {0}_sort WHEN NULL THEN {0} "
-            'WHEN "" THEN {0} '
-            "ELSE {0}_sort END) {1} {2}"
-        ).format(field, collate, order)
+
+        return f"COALESCE(NULLIF({field}_sort, ''), {field}) {collate} {order}"
 
     def sort(self, objs):
         if self.album:
diff --git a/docs/changelog.rst b/docs/changelog.rst
index 33a4b5f94d..e48ec3e6d4 100644
--- a/docs/changelog.rst
+++ b/docs/changelog.rst
@@ -40,6 +40,8 @@ Bug fixes:
   issues in the future.
   :bug:`5289`
 * :doc:`plugins/discogs`: Fix the ``TypeError`` when there is no description.
+* Fix double-quoted string literals in SQL queries
+  :bug:`4709`
 
 For packagers:
 
diff --git a/test/test_library.py b/test/test_library.py
index 9b29505a3d..c480442101 100644
--- a/test/test_library.py
+++ b/test/test_library.py
@@ -59,7 +59,7 @@ def test_store_changes_database_value(self):
         self.i.store()
         new_year = (
             self.lib._connection()
-            .execute("select year from items where " 'title="the title"')
+            .execute("select year from items where title = ?", (self.i.title,))
             .fetchone()["year"]
         )
         assert new_year == 1987
@@ -70,7 +70,7 @@ def test_store_only_writes_dirty_fields(self):
         self.i.store()
         new_genre = (
             self.lib._connection()
-            .execute("select genre from items where " 'title="the title"')
+            .execute("select genre from items where title = ?", (self.i.title,))
             .fetchone()["genre"]
         )
         assert new_genre == original_genre
@@ -104,7 +104,8 @@ def test_item_add_inserts_row(self):
         new_grouping = (
             self.lib._connection()
             .execute(
-                "select grouping from items " 'where composer="the composer"'
+                "select grouping from items where composer = ?",
+                (self.i.composer,),
             )
             .fetchone()["grouping"]
         )
@@ -118,7 +119,8 @@ def test_library_add_path_inserts_row(self):
         new_grouping = (
             self.lib._connection()
             .execute(
-                "select grouping from items " 'where composer="the composer"'
+                "select grouping from items where composer = ?",
+                (self.i.composer,),
             )
             .fetchone()["grouping"]
         )


Code review comments:
diff_hunk: @@ -40,6 +40,8 @@ Bug fixes:
   issues in the future.
   :bug:`5289`
 * :doc:`plugins/discogs`: Fix the ``TypeError`` when there is no description.
+* Fix double-quoted string literals in SQL queries
path: docs/changelog.rst
text: @user1:
```suggestion
* Remove single quotes from all SQL queries
```

@author:
Updated