fix(dbcore): NumericQuery range crashes on a null value#6849
Open
chuenchen309 wants to merge 3 commits into
Open
fix(dbcore): NumericQuery range crashes on a null value#6849chuenchen309 wants to merge 3 commits into
chuenchen309 wants to merge 3 commits into
Conversation
NumericQuery.match() compares the field value against rangemin/rangemax
without a None guard, so a range query against a nullable numeric field
raises instead of returning False:
NumericQuery("rg_track_gain", "-10..0").match(Item())
TypeError: '<' not supported between instances of 'NoneType' and 'int'
Nullable types (NullFloat, NullInteger) use None as their null value, unlike
Integer/Float whose null is 0 -- which is why year queries never hit this.
Affected fields include album_id (None for every singleton) and the whole
rg_*/r128_* family (None until ReplayGain has been run, i.e. by default).
The query's own col_clause() defines the correct answer: `rg_track_gain >= ?
AND rg_track_gain <= ?` against NULL is never true in SQL, so the row is
quietly excluded. match() should agree, and now does. Point queries were
already fine.
Reachable from two callers, and the first has no SQL path at all:
- library/models.py:1203 -- Item.destination() evaluates `paths:` config
rules through match() unconditionally. A rule like `rg_track_gain:0..`
crashes on any item without ReplayGain.
- dbcore/db.py:805 -- the Results slow path, used whenever clause() returns
None (any flexattr subquery forces this).
This is the sibling of beetbox#3461: 2293d2f fixed the identical None-comparison
crash on the sort side (sort.py deliberately groups nulls to match SQLite's
NULL ordering); the query side was missed.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #6849 +/- ##
=======================================
Coverage 75.63% 75.63%
=======================================
Files 163 163
Lines 21312 21314 +2
Branches 3360 3361 +1
=======================================
+ Hits 16119 16121 +2
Misses 4401 4401
Partials 792 792
🚀 New features to boost your workflow:
|
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
A range query against a nullable numeric field raises instead of returning
False:NumericQuery.match()compares the value againstrangemin/rangemaxwith noNoneguard. Nullable types (NullFloat,NullInteger) useNoneas their null value — unlikeInteger/Float, whose null is0, which is whyyearqueries never hit this and the bug stayed hidden. Affected fields:album_id(Nonefor every singleton) and the wholerg_*/r128_*family (Noneuntil ReplayGain has been run, i.e. by default). Point queries (rg_track_gain:5) were already fine — only the range branches.What the correct behaviour is
The query's own
col_clause()defines it:rg_track_gain >= ? AND rg_track_gain <= ?againstNULLis never true in SQL, so the row is quietly excluded.match()should agree — this is the classic SQL-path/match-path divergence, andtest_fast_vs_slowin the test file exists precisely to pin that contract (it just never covered a nullable field).Reachability
Two callers, and the first has no SQL path at all:
library/models.py:1203—Item.destination()evaluatespaths:config rules throughmatch()unconditionally. A rule likerg_track_gain:0..crashes on any item lacking ReplayGain. Verified end-to-end:dbcore/db.py:805— theResultsslow path, used wheneverclause()returnsNone(any flexattr subquery forces this).beet ls "rg_track_gain:0.. someflex:x"raises, while the same data through a fast SQL-only query returns[]cleanly.Precedent
This is the sibling of #3461. Commit
2293d2f6("Fix crash when sorting by a nullable field missing on some items") fixed the identicalNone-comparison crash on the sort side —sort.pydeliberately groups nulls together "matching SQLite's default ordering of NULLs", and its comment namesNullInteger/NullFloatexplicitly. The query side was missed.Tests
Three cases (
-10..0,0..,..0) asserting a range never matches a null value. All fail onmasterwith theTypeErrorand pass here. No existing test fed a nullable field toNumericQuery—test/dbcore/test_query.pyonly usesyear.pytest test/dbcore/test_query.py— 146 passed (143 baseline + 3 new)pytest test/test_library.py test/dbcore/— 518 passed, no regressionsruff check/ruff format --check/mypyclean; changelog checked withdocstrfmt,sphinx-lint, and the unreleased-section check.AI disclosure: drafted with Claude Code (Opus 4.8), including the root-cause trace and the tests. I ran the repro and the test suite locally and reviewed the diff before opening.