web: resolve relative album artpath before serving art - #7015
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #7015 +/- ##
==========================================
- Coverage 77.39% 77.25% -0.14%
==========================================
Files 163 163
Lines 21841 21844 +3
Branches 3370 3371 +1
==========================================
- Hits 16904 16876 -28
- Misses 4112 4146 +34
+ Partials 825 822 -3
🚀 New features to boost your workflow:
|
d5cb75c to
f43e9a5
Compare
Serene-Arc
left a comment
There was a problem hiding this comment.
Hi! Thanks for the PR. Overall, it looks good except for one thing. We're currently in the process of moving everything that we can to the more modern pathlib library in Python. If you could change the path stuff to use pathlib, that would be great.
| # to an absolute path so send_file doesn't look under the app root. | ||
| if not os.path.isabs(artpath): | ||
| artpath = os.path.join(g.lib.directory, artpath) | ||
| return flask.send_file(util.syspath(artpath)) |
There was a problem hiding this comment.
We are moving to pathlib instead of os functions where possible. These can be done with pathlib functions.
There was a problem hiding this comment.
Switched to pathlib here in c9105ee. Using the album.art_filepath property now and joining the relative case against the library dir with /. Kept os.fsdecode since that's the bytes to str step art_filepath itself does; pathlib won't take bytes and a plain .decode() would choke on non-utf8 filenames.
| rel = os.path.join(b"rel_art_dir", b"cover.png") | ||
| abspath = os.path.join(lib.directory, rel) | ||
| os.makedirs(os.path.dirname(syspath(abspath)), exist_ok=True) |
There was a problem hiding this comment.
More places where pathlib can be used instead.
There was a problem hiding this comment.
Same commit. os.makedirs/os.path.dirname are now abspath.parent.mkdir(parents=True, exist_ok=True), and the joins use /.
Address review on beetbox#7015: replace os.path.isabs/os.path.join (and the os.makedirs/os.path.dirname/open in the test) with pathlib, reusing the Album.art_filepath helper. os.fsdecode stays as the bytes->str bridge, matching beets' own model idiom.
|
Pushed the pathlib conversion in c9105ee, both the route and the test. The only os call left is os.fsdecode, which is the bytes to str bridge beets already uses in Album.art_filepath (pathlib doesn't accept bytes). Should be good for another look. |
Description
GET /album/<id>/artreturns a 500 when an album'sartpathis stored relative to the library directory.album_art()passesalbum.artpathstraight toflask.send_file(). beets storesartpathrelative to the music directory and only resolves it lazily, through themusic_dirContextVar, in the thread that set it. The web server runs withapp.run(threaded=True), so worker threads never set that ContextVar; there the path stays relative andsend_file()resolves it against the app root (beetsplug/web/), raisingFileNotFoundError, so Flask returns a 500. Absolute artpaths are unaffected.Fix
Resolve a relative artpath against
g.lib.directorybefore serving it, throughutil.syspath():Testing
Added a regression test that reproduces the failure from a separate thread (which, like the
threaded=Trueworkers, doesn't inherit the ContextVar) against an on-disk library, and asserts a 200 with the file's bytes. It fails without the fix and passes with it.To Do