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/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 096a35a6db..ca699746b6 100644 --- a/docs/changelog.rst +++ b/docs/changelog.rst @@ -58,6 +58,19 @@ 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 + 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" 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