From c862604a3757d7fe13994be1938c15a366929647 Mon Sep 17 00:00:00 2001 From: Nick Ermakov Date: Sun, 19 Jul 2026 17:40:08 +0700 Subject: [PATCH 1/3] fix(dbcore): filter flex attributes out of store() main-table update `Model.store(fields=...)` used to walk every field name passed in and build an `UPDATE SET =?` clause for each dirty one, without checking whether the field was a fixed column or a flexible attribute. Callers such as `beet update` pass `library.Item._media_fields` when no explicit `fields=` is given, and a plugin that registers the same field via both `item_types` (typed flex attr) and `add_media_field()` lands in that set. For such fields there is no column, so the UPDATE crashed with `sqlite3.OperationalError: no such column: `. Filter the requested field set to fixed columns before building the UPDATE; flexible attributes are already persisted just below via the `_flex_table` INSERT path, so they keep round-tripping correctly. Adds a regression test that stores a typed flex attribute through the `fields=` argument and asserts the value survives a fresh load. Fixes #5580. --- beets/dbcore/db.py | 11 ++++++++++- docs/changelog.rst | 7 +++++++ test/dbcore/test_db.py | 21 +++++++++++++++++++++ 3 files changed, 38 insertions(+), 1 deletion(-) diff --git a/beets/dbcore/db.py b/beets/dbcore/db.py index e66f4548bf..f002146ed9 100755 --- a/beets/dbcore/db.py +++ b/beets/dbcore/db.py @@ -586,10 +586,19 @@ def store(self, fields: Iterable[str] | None = None) -> None: if fields is None: fields = self._fields + # Separate the requested fields into fixed (real columns in the main + # table) and flexible (persisted via ``_flex_table``). Callers such as + # ``beet update`` may pass a flex attribute through ``fields=`` -- for + # example, when a plugin registers it via both ``item_types`` and + # ``add_media_field``. Treating a flex attribute as a column here used + # to build ``UPDATE
SET =?`` and crash with + # ``sqlite3.OperationalError: no such column: `` (see #5580). + column_fields = [f for f in fields if f in self._fields] + # Build assignments for query. assignments = [] subvars: list[SQLiteType] = [] - for key in fields: + for key in column_fields: if key != "id" and key in self._dirty: self._dirty.remove(key) assignments.append(f"{key}=?") diff --git a/docs/changelog.rst b/docs/changelog.rst index 096a35a6db..9f7f4c8c3a 100644 --- a/docs/changelog.rst +++ b/docs/changelog.rst @@ -58,6 +58,13 @@ New features Bug fixes ~~~~~~~~~ +- Fixed a ``sqlite3.OperationalError: no such column`` crash in ``Model.store`` + when a field passed via the ``fields`` argument was a flexible attribute + rather than a fixed column. The fields set is now filtered to fixed columns + for the main-table ``UPDATE``; flexible attributes continue to be persisted + via the ``_flex_table`` path. This affected plugins that register fields via + both ``item_types`` and ``add_media_field`` and were later updated with + ``beet update``. :bug:`5580` - :doc:`plugins/edit`: Preserve missing album art paths when editing album metadata, instead of turning ``artpath: null`` into a path ending in ``None``. :bug:`2438` diff --git a/test/dbcore/test_db.py b/test/dbcore/test_db.py index 829919847a..fb33b49e33 100644 --- a/test/dbcore/test_db.py +++ b/test/dbcore/test_db.py @@ -281,6 +281,27 @@ def test_store_and_retrieve_flexattr(self): other_model = self.db._get(ModelFixture1, model.id) assert other_model.foo == "bar" + def test_store_with_flex_field_in_fields_argument(self): + """A flex attribute passed via ``fields=`` must not crash. + + Regression test for #5580: when a plugin registers a typed flex + attribute (via ``item_types``) and a caller such as ``beet update`` + passes its name through ``fields=`` to ``Model.store()``, the store + loop used to build ``UPDATE
SET =?`` and crash + with ``sqlite3.OperationalError: no such column: ``. + Flex attributes have no column in the main table; they live in the + ``_flex_table`` and must be persisted via the INSERT path. + """ + model = ModelFixture1() + model.add(self.db) + model.field_one = 42 + model.some_float_field = 1.5 + model.store(fields=["field_one", "some_float_field"]) + + other_model = self.db._get(ModelFixture1, model.id) + assert other_model.field_one == 42 + assert other_model.some_float_field == 1.5 + def test_delete_flexattr(self): model = ModelFixture1() model["foo"] = "bar" From bae9665e9dedd4b5dd157dc1793d051fc11b3ca1 Mon Sep 17 00:00:00 2001 From: Nick Ermakov Date: Sun, 19 Jul 2026 20:36:48 +0700 Subject: [PATCH 2/3] style(changelog): wrap entry to 72 cols (docstrfmt) CI's Check docs job failed on the original entry because docstrfmt enforces a 72-column wrap. Reformat with . --- docs/changelog.rst | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/docs/changelog.rst b/docs/changelog.rst index 9f7f4c8c3a..84fc808d1b 100644 --- a/docs/changelog.rst +++ b/docs/changelog.rst @@ -63,8 +63,8 @@ Bug fixes rather than a fixed column. The fields set is now filtered to fixed columns for the main-table ``UPDATE``; flexible attributes continue to be persisted via the ``_flex_table`` path. This affected plugins that register fields via - both ``item_types`` and ``add_media_field`` and were later updated with - ``beet update``. :bug:`5580` + both ``item_types`` and ``add_media_field`` and were later updated with ``beet + update``. :bug:`5580` - :doc:`plugins/edit`: Preserve missing album art paths when editing album metadata, instead of turning ``artpath: null`` into a path ending in ``None``. :bug:`2438` From ff410f76b74d5c7fca2e63f4e106efd772cdf4c5 Mon Sep 17 00:00:00 2001 From: Nick Ermakov Date: Sun, 19 Jul 2026 20:30:04 +0700 Subject: [PATCH 3/3] fix(embedart): retry compare without phash:colorspaces on empty IM7 output MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ImageMagick ≥7.1.1-44 sometimes emits empty stdout+stderr when invoked with `magick compare -define phash:colorspaces=sRGB,HCLp -metric PHASH`. The colorspaces define was added in a873a191b to make PHASH scores deterministic; on newer IM7 builds it silently produces no output (see ImageMagick/ImageMagick#5191). beets' IMBackend.compare() treated this as a parse error and returned None, which surfaced to users as "Error while checking art similarity; skipping" on every track during `beet import` (#6348). Fix: when the first compare pipeline exits cleanly (0 or 1) but produces no bytes on either stdout or stderr, retry the same pipeline WITHOUT the colorspaces define. The score is then less deterministic across IM builds but still usable for the compare_threshold gate. If the retry also yields no parseable output, compare() returns None (same as a genuine parse error). Implementation: - Extract the convert | compare subprocess orchestration into a `_run_compare_pipeline` helper returning a `CompareResult` dataclass that distinguishes the empty-output case from a real parse/failure. - Extract PHASH-output parsing (including the existing 7.1.1-44 "(diff)" paren extraction from #5650) into a static `_parse_compare_output` helper. - compare() calls the helper twice when the first run's `empty_output` flag is set, with a retry cmd that omits the define. Tests: two regression tests in ArtSimilarityTest (both fully mocked, no IM7 required): - test_compare_empty_output_triggers_retry: first call empty, retry parseable → similarity result reflects the retry score. - test_compare_empty_output_retry_also_fails: both empty → None (graceful). The existing 8 ArtSimilarityTest tests are unchanged and still pass — the empty-output path is only entered when both streams are empty AND the subprocess exited 0/1, so genuine parse errors and convert/compare failures short-circuit before the retry. Fixes #6348. --- beets/util/artresizer.py | 144 ++++++++++++++++++++++++++++------ docs/changelog.rst | 6 ++ test/plugins/test_embedart.py | 73 ++++++++++++++++- 3 files changed, 196 insertions(+), 27 deletions(-) diff --git a/beets/util/artresizer.py b/beets/util/artresizer.py index 0556ebf16e..10f3a4375d 100644 --- a/beets/util/artresizer.py +++ b/beets/util/artresizer.py @@ -11,6 +11,7 @@ import subprocess from abc import ABC, abstractmethod from contextlib import suppress +from dataclasses import dataclass from enum import Enum from itertools import chain from typing import TYPE_CHECKING, Any, ClassVar @@ -25,7 +26,7 @@ ) if TYPE_CHECKING: - from collections.abc import Mapping + from collections.abc import Mapping, Sequence PROXY_URL = "https://images.weserv.nl/" @@ -48,6 +49,21 @@ class LocalBackendNotAvailableError(Exception): pass +@dataclass +class CompareResult: + """Outcome of a single PHASH compare pipeline run. + + `score` is None when no parseable PHASH score was produced. The + `empty_output` flag distinguishes the #6348 case (ImageMagick ≥7.1.1-44 + emits empty stdout+stderr with the `phash:colorspaces` define) from a + genuine parse error or subprocess failure — only the empty-output + case warrants a retry without the define. + """ + + score: float | None + empty_output: bool = False + + # Singleton pattern that the typechecker understands: # https://peps.python.org/pep-0484/#support-for-singleton-types-in-unions class NotAvailable(Enum): @@ -373,6 +389,15 @@ def compare( "gray", "MIFF:-", ] + + # The `phash:colorspaces=sRGB,HCLp` define was added in a873a191b + # to make PHASH scores deterministic. ImageMagick ≥7.1.1-44 + # sometimes emits empty stdout+stderr when this define is passed + # (see ImageMagick/ImageMagick#5191), which beets interpreted as + # a parse error and surfaced to users as "Error while checking + # art similarity; skipping" on every track (#6348). We try the + # deterministic pipeline first; on empty output we retry without + # the define, trading determinism for a usable score. compare_cmd = [ *self.compare_cmd, "-define", @@ -382,6 +407,76 @@ def compare( "-", "null:", ] + result = self._run_compare_pipeline( + convert_cmd, compare_cmd, im1, im2, is_windows + ) + if result.empty_output: + log.debug( + "ImageMagick produced no output with phash:colorspaces " + "define; retrying without it (see #6348)" + ) + retry_compare_cmd = [ + *self.compare_cmd, + "-metric", + "PHASH", + "-", + "null:", + ] + result = self._run_compare_pipeline( + convert_cmd, retry_compare_cmd, im1, im2, is_windows + ) + + if result.score is None: + return None + + log.debug("ImageMagick compare score: {}", result.score) + return result.score <= compare_threshold + + @staticmethod + def _parse_compare_output( + returncode: int, stdout: bytes, stderr: bytes + ) -> float | None: + """Parse the output of `compare -metric PHASH` into a score. + + ImageMagick writes the score to stdout when the images are + identical (exit 0) and to stderr when they differ (exit 1). Any + other exit code, or output that is not parseable as a float + after the IM 7.1.1-44 "(diff)" paren extraction, returns None. + """ + if returncode: + if returncode != 1: + return None + out_str = stderr + else: + out_str = stdout + + # ImageMagick 7.1.1-44 outputs in a different format. + if b"(" in out_str and out_str.endswith(b")"): + # Extract diff from "... (diff)". + out_str = out_str[out_str.index(b"(") + 1 : -1] + + try: + return float(out_str) + except ValueError: + log.debug("IM output is not a number: {0!r}", out_str) + return None + + def _run_compare_pipeline( + self, + convert_cmd: Sequence[bytes | str], + compare_cmd: Sequence[bytes | str], + im1: bytes, + im2: bytes, + is_windows: bool, + ) -> CompareResult: + """Run the convert | compare pipeline once. + + Returns a `CompareResult` whose `score` is None when no parseable + PHASH score was produced, and whose `empty_output` flag indicates + whether the compare subprocess itself ran cleanly (exit 0 or 1) + but produced no usable output — the #6348 case, which warrants a + retry without the colorspaces define. + """ log.debug( "comparing images with pipeline {} | {}", convert_cmd, compare_cmd ) @@ -415,35 +510,32 @@ def compare( convert_proc, convert_stderr, ) - return None + return CompareResult(None, empty_output=False) # Check the compare output. stdout, stderr = compare_proc.communicate() - if compare_proc.returncode: - if compare_proc.returncode != 1: - log.debug( - "ImageMagick compare failed: {}, {}", - displayable_path(im2), - displayable_path(im1), - ) - return None - out_str = stderr - else: - out_str = stdout - - # ImageMagick 7.1.1-44 outputs in a different format. - if b"(" in out_str and out_str.endswith(b")"): - # Extract diff from "... (diff)". - out_str = out_str[out_str.index(b"(") + 1 : -1] - - try: - phash_diff = float(out_str) - except ValueError: - log.debug("IM output is not a number: {0!r}", out_str) - return None + if compare_proc.returncode and compare_proc.returncode != 1: + log.debug( + "ImageMagick compare failed: {}, {}", + displayable_path(im2), + displayable_path(im1), + ) + return CompareResult(None, empty_output=False) - log.debug("ImageMagick compare score: {}", phash_diff) - return phash_diff <= compare_threshold + score = self._parse_compare_output( + compare_proc.returncode, stdout, stderr + ) + # Detect the #6348 case: compare exited 0/1 but emitted no + # parseable bytes on the stream we read. We only treat the case + # where BOTH streams are empty as retry-worthy — non-empty but + # unparseable output is a genuine parse error. + empty_output = ( + score is None + and compare_proc.returncode in (0, 1) + and not stdout.strip() + and not stderr.strip() + ) + return CompareResult(score, empty_output=empty_output) @property def can_write_metadata(self) -> bool: diff --git a/docs/changelog.rst b/docs/changelog.rst index 84fc808d1b..ca699746b6 100644 --- a/docs/changelog.rst +++ b/docs/changelog.rst @@ -58,6 +58,12 @@ New features Bug fixes ~~~~~~~~~ +- :doc:`/plugins/embedart`: Fixed "Error while checking art similarity; + skipping" being logged on every track on ImageMagick ≥7.1.1-44. When the + ``phash:colorspaces=sRGB,HCLp`` define (added for score determinism) makes + ``magick compare -metric PHASH`` emit empty stdout+stderr, beets now retries + the compare without the define, trading determinism for a usable score. + :bug:`6348` - Fixed a ``sqlite3.OperationalError: no such column`` crash in ``Model.store`` when a field passed via the ``fields`` argument was a flexible attribute rather than a fixed column. The fields set is now filtered to fixed columns diff --git a/test/plugins/test_embedart.py b/test/plugins/test_embedart.py index 6ad91100be..ba1875339a 100644 --- a/test/plugins/test_embedart.py +++ b/test/plugins/test_embedart.py @@ -349,14 +349,32 @@ def _mock_popens( compare_stdout=b"", compare_stderr=b"", convert_status=0, + # When the first compare call produces empty/unparseable output, + # compare() retries without the phash:colorspaces define. These + # args configure that retry call (used by the #6348 regression + # tests). If None, no retry is mocked (legacy behavior). + retry_status=None, + retry_stdout=b"", + retry_stderr=b"", + retry_convert_status=0, ): mock_extract.return_value = b"extracted_path" - mock_subprocess.Popen.side_effect = [ + popens = [ # The `convert` call. self._popen(convert_status), # The `compare` call. self._popen(compare_status, compare_stdout, compare_stderr), ] + if retry_status is not None: + # The retry path runs a second convert + compare (without the + # phash:colorspaces define). + popens.extend( + [ + self._popen(retry_convert_status), + self._popen(retry_status, retry_stdout, retry_stderr), + ] + ) + mock_subprocess.Popen.side_effect = popens def test_compare_success_similar(self, mock_extract, mock_subprocess): self._mock_popens(mock_extract, mock_subprocess, 0, b"10", b"err") @@ -388,6 +406,59 @@ def test_compare_parsing_error_and_failure( self._mock_popens(mock_extract, mock_subprocess, 1, b"foo", b"bar") assert self._similarity(20) is None + def test_compare_empty_output_triggers_retry( + self, mock_extract, mock_subprocess + ): + """Reproduces #6348: ImageMagick ≥7.1.1-44 sometimes emits empty + stdout+stderr when invoked with the `phash:colorspaces=sRGB,HCLp` + define. compare() should retry the pipeline without the define + rather than returning None silently. + """ + # First call: empty stdout+stderr (the #6348 regression). + # Retry: a parseable score on stdout. + self._mock_popens( + mock_extract, + mock_subprocess, + compare_status=0, + compare_stdout=b"", + compare_stderr=b"", + retry_status=0, + retry_stdout=b"10", + retry_stderr=b"", + ) + # Score 10 vs threshold 20 → similar (within threshold). + assert self._similarity(20) + # And the threshold comparison actually fires in the other direction. + self._mock_popens( + mock_extract, + mock_subprocess, + compare_status=0, + compare_stdout=b"", + compare_stderr=b"", + retry_status=0, + retry_stdout=b"10", + retry_stderr=b"", + ) + assert not self._similarity(5) + + def test_compare_empty_output_retry_also_fails( + self, mock_extract, mock_subprocess + ): + """If both the define-based and the no-define retries produce no + parseable output, compare() returns None (same as a parse error). + """ + self._mock_popens( + mock_extract, + mock_subprocess, + compare_status=0, + compare_stdout=b"", + compare_stderr=b"", + retry_status=0, + retry_stdout=b"", + retry_stderr=b"", + ) + assert self._similarity(20) is None + def test_convert_failure(self, mock_extract, mock_subprocess): self._mock_popens(mock_extract, mock_subprocess, convert_status=1) assert self._similarity(20) is None