Improve error message when database directory is not writable#6294
Improve error message when database directory is not writable#6294VedantMadane wants to merge 6 commits into
Conversation
|
Thank you for the PR! The changelog has not been updated, so here is a friendly reminder to check if you need to add an entry. |
There was a problem hiding this comment.
Hey - I've found 2 issues, and left some high level feedback:
- The special-case
readonly/unable to openbranch currently assumes a directory permissions issue; consider broadening the message to mention both file and directory permissions (or phrasing it more generically) since these errors can also arise from file-level ACLs or missing paths. - When constructing
db_dir = os.path.dirname(dbpath), it may be safer to normalize the path (e.g., viaos.path.abspath) and handle the case wheredbpathhas no directory component, so that the resulting message about the directory being writable is always meaningful.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The special-case `readonly`/`unable to open` branch currently assumes a directory permissions issue; consider broadening the message to mention both file and directory permissions (or phrasing it more generically) since these errors can also arise from file-level ACLs or missing paths.
- When constructing `db_dir = os.path.dirname(dbpath)`, it may be safer to normalize the path (e.g., via `os.path.abspath`) and handle the case where `dbpath` has no directory component, so that the resulting message about the directory being writable is always meaningful.
## Individual Comments
### Comment 1
<location> `beets/ui/__init__.py:1513-1518` </location>
<code_context>
+ # Check for permission-related errors and provide a helpful message
+ error_str = str(db_error).lower()
+ dbpath_display = util.displayable_path(dbpath)
+ if "readonly" in error_str or "unable to open" in error_str:
+ db_dir = os.path.dirname(dbpath)
+ raise UserError(
+ f"database file {dbpath_display} could not be opened due to a "
+ f"permissions error. Please check that the directory "
+ f"{util.displayable_path(db_dir)} is writable."
+ )
raise UserError(
</code_context>
<issue_to_address>
**issue (bug_risk):** The permission-specific branch may misdiagnose non-permission errors as permission issues.
Relying on the generic substring `"unable to open"` and mapping it directly to a permissions error is brittle. This message can also arise from non-permission issues (missing directory, malformed path, unavailable filesystem). Please either narrow the pattern to permission-specific cases, include the original `db_error` in the message for context, or soften the wording so it doesn’t claim permissions are the only cause.
</issue_to_address>
### Comment 2
<location> `beets/ui/__init__.py:1512-1522` </location>
<code_context>
+ dbpath_display = util.displayable_path(dbpath)
+ if "readonly" in error_str or "unable to open" in error_str:
+ db_dir = os.path.dirname(dbpath)
+ raise UserError(
+ f"database file {dbpath_display} could not be opened due to a "
+ f"permissions error. Please check that the directory "
+ f"{util.displayable_path(db_dir)} is writable."
+ )
raise UserError(
</code_context>
<issue_to_address>
**suggestion:** The permission-focused message drops the original SQLite error details, reducing debuggability.
In this branch, the original `db_error` isn’t surfaced, unlike in the generic error path. Please include it here as well (e.g., by appending `f" (original error: {db_error})"`) so callers retain the underlying SQLite details for troubleshooting.
```suggestion
dbpath_display = util.displayable_path(dbpath)
if "readonly" in error_str or "unable to open" in error_str:
db_dir = os.path.dirname(dbpath)
raise UserError(
f"database file {dbpath_display} could not be opened due to a "
f"permissions error. Please check that the directory "
f"{util.displayable_path(db_dir)} is writable "
f"(original error: {db_error})."
)
raise UserError(
f"database file {dbpath_display} could not be opened: {db_error}"
)
```
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #6294 +/- ##
=======================================
Coverage 70.18% 70.18%
=======================================
Files 147 147
Lines 18680 18688 +8
Branches 3042 3044 +2
=======================================
+ Hits 13111 13117 +6
- Misses 4927 4928 +1
- Partials 642 643 +1
🚀 New features to boost your workflow:
|
|
readonly errors only occur on write operations, not during database open. I've pushed a fix that removes the Also refined the error message to be more specific about the directory-not-writable-on-create scenario. |
Resolved conflict in docs/changelog.rst by keeping both bug fix entries: - Database permission error message improvement (PR beetbox#6294) - ArtResizer OSError handling (from master)
f1fa526 to
29b4ee3
Compare
|
Maybe it'd be worth addressing this error message as part of this PR as well? This happens whenever you try to write to the database while it is already being written. You can re-produce it by opening two terminal windows:
|
c63bbfe to
61b677c
Compare
|
@VedantMadane what do you think about this? Also note that CI is complaining about some lint issues.
|
|
@VedantMadane I'm waiting for your response to my comments above. |
|
Please mark it ready once you responded to my comments above @VedantMadane. |
5f4c0cd to
a15df48
Compare
|
@snejus Sorry for the delayed response! Here's what I've done:
Should be green now — please take another look when you get a chance. |
There was a problem hiding this comment.
Pull request overview
PR try make beets give nicer UserError when SQLite fail open DB because permissions (like directory not writable). It also add tests for new message.
Changes:
- Add special-case handling in
ui._open_libraryfor SQLite “unable to open” errors to suggest checking file/dir write permissions. - Add UI init tests that assert the improved error message and keep fallback message for other OperationalError cases.
- Change DB core default timeout and update changelog entry (plus some packaging metadata edits).
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
beets/ui/__init__.py |
Add permission-hint UserError when SQLite open fail. |
test/ui/test_ui_init.py |
Add tests for new database-open error message behavior. |
beets/dbcore/db.py |
Adjust DB init defaults (timeout) and tweak migration base. |
docs/changelog.rst |
Add unreleased note about improved DB open error message + timeout + typo fix. |
pyproject.toml |
Modify project version / classifiers / dependency constraints, plus ruff ignore for new test. |
Comments suppressed due to low confidence (2)
beets/dbcore/db.py:1122
- grug see Database timeout default changed to 30s, but Library always passes timeout from beets.config['timeout'] (default in config_default.yaml still 5.0). so this change likely not actually increase default busy timeout for normal beets usage. either bump config default / docs, or wire new default into config path.
def __init__(self, path, timeout: float = 30.0):
if sqlite3.threadsafety == 0:
raise RuntimeError(
"sqlite3 must be compiled with multi-threading support"
)
docs/changelog.rst:20
- grug see docs/changelog drop whole 2.9.0 section + many entries from Unreleased. looks like accidental conflict revert. changelog should not lose past release notes; please restore removed sections and only add new bugfix entry for #1676.
Unreleased
----------
New features
~~~~~~~~~~~~
- :ref:`import-cmd` Use ffprobe to recognize format of any import music file
that has no extension. If the file cannot be recognized as a music file, leave
it alone. :bug:`4881`
- Query: Add ``has_cover_art`` computed field to query items by embedded cover
art presence. Users can now search for tracks with or without embedded artwork
using ``beet list has_cover_art:true`` or ``beet list has_cover_art:false``.
Ah I see, this makes sense! Would you mind rebasing your PR and fixing the issues in CI? Please address the review comments from Copilot as well! |
a15df48 to
7026749
Compare
ccceeb0 to
a3fd17e
Compare
- Add specific error handling for SQLite read-only and permission denied errors. - Ensure UI doesn't crash when database is unavailable. - Update changelog and tests. Signed-off-by: Vedant Madane <vedantnm@gmail.com>
a3fd17e to
90cf13c
Compare
|
@snejus Thanks for the patience and the review notes. I have rebased onto latest If anything in the rebased diff looks off vs what you reviewed before, I am happy to re-walk it. |
Adds better error handling for SQLite OperationalError when the database file or directory is not writable. When the error message contains 'readonly' or 'unable to open', provides a helpful message suggesting the user check directory permissions. Also fixes a typo in the error message ('cannot not' to 'could not'). Closes #1676