code review fixes

This commit is contained in:
Alex Shnitman
2026-03-15 20:53:13 +02:00
parent 04959a6189
commit 7fa1fc7938
20 changed files with 498 additions and 448 deletions

View File

@@ -53,12 +53,12 @@ def get_format(download_type: str, codec: str, format: str, quality: str) -> str
if download_type == "audio":
if format not in AUDIO_FORMATS:
raise Exception(f"Unknown audio format {format}")
raise ValueError(f"Unknown audio format {format}")
return f"bestaudio[ext={format}]/bestaudio/best"
if download_type == "video":
if format not in ("any", "mp4", "ios"):
raise Exception(f"Unknown video format {format}")
raise ValueError(f"Unknown video format {format}")
vfmt, afmt = ("[ext=mp4]", "[ext=m4a]") if format in ("mp4", "ios") else ("", "")
vres = f"[height<={quality}]" if quality not in ("best", "worst") else ""
vcombo = vres + vfmt
@@ -71,12 +71,12 @@ def get_format(download_type: str, codec: str, format: str, quality: str) -> str
return f"bestvideo{codec_filter}{vcombo}+bestaudio{afmt}/bestvideo{vcombo}+bestaudio{afmt}/best{vcombo}"
return f"bestvideo{vcombo}+bestaudio{afmt}/best{vcombo}"
raise Exception(f"Unknown download_type {download_type}")
raise ValueError(f"Unknown download_type {download_type}")
def get_opts(
download_type: str,
codec: str,
_codec: str,
format: str,
quality: str,
ytdl_opts: dict,
@@ -96,8 +96,6 @@ def get_opts(
Returns:
dict: extended options
"""
del codec # kept for parity with get_format signature
download_type = (download_type or "video").strip().lower()
format = (format or "any").strip().lower()
opts = copy.deepcopy(ytdl_opts)
@@ -113,7 +111,7 @@ def get_opts(
}
)
if format not in ("wav") and "writethumbnail" not in opts:
if format != "wav" and "writethumbnail" not in opts:
opts["writethumbnail"] = True
postprocessors.append(
{

View File

@@ -16,25 +16,15 @@ import pathlib
import re
from watchfiles import DefaultFilter, Change, awatch
from ytdl import DownloadQueueNotifier, DownloadQueue
from ytdl import DownloadQueueNotifier, DownloadQueue, Download
from yt_dlp.version import __version__ as yt_dlp_version
log = logging.getLogger('main')
def parseLogLevel(logLevel):
match logLevel:
case 'DEBUG':
return logging.DEBUG
case 'INFO':
return logging.INFO
case 'WARNING':
return logging.WARNING
case 'ERROR':
return logging.ERROR
case 'CRITICAL':
return logging.CRITICAL
case _:
return None
if not isinstance(logLevel, str):
return None
return getattr(logging, logLevel.upper(), None)
# Configure logging before Config() uses it so early messages are not dropped.
# Only configure if no handlers are set (avoid clobbering hosting app settings).
@@ -71,7 +61,7 @@ class Config:
'KEYFILE': '',
'BASE_DIR': '',
'DEFAULT_THEME': 'auto',
'MAX_CONCURRENT_DOWNLOADS': 3,
'MAX_CONCURRENT_DOWNLOADS': '3',
'LOGLEVEL': 'INFO',
'ENABLE_ACCESSLOG': 'false',
}
@@ -181,7 +171,7 @@ class ObjectSerializer(json.JSONEncoder):
elif hasattr(obj, '__iter__') and not isinstance(obj, (str, bytes)):
try:
return list(obj)
except:
except Exception:
pass
# Fall back to default behavior
return json.JSONEncoder.default(self, obj)
@@ -280,6 +270,7 @@ class Notifier(DownloadQueueNotifier):
dqueue = DownloadQueue(config, Notifier())
app.on_startup.append(lambda app: dqueue.initialize())
app.on_cleanup.append(lambda app: Download.shutdown_manager())
class FileOpsFilter(DefaultFilter):
def __call__(self, change_type: int, path: str) -> bool:
@@ -330,12 +321,30 @@ async def watch_files():
if config.YTDL_OPTIONS_FILE:
app.on_startup.append(lambda app: watch_files())
async def _read_json_request(request: web.Request) -> dict:
try:
post = await request.json()
except json.JSONDecodeError as exc:
raise web.HTTPBadRequest(reason='Invalid JSON request body') from exc
if not isinstance(post, dict):
raise web.HTTPBadRequest(reason='JSON request body must be an object')
return post
@routes.post(config.URL_PREFIX + 'add')
async def add(request):
log.info("Received request to add download")
post = await request.json()
post = await _read_json_request(request)
post = _migrate_legacy_request(post)
log.info(f"Request data: {post}")
log.info(
"Add download request: type=%s quality=%s format=%s has_folder=%s auto_start=%s",
post.get('download_type'),
post.get('quality'),
post.get('format'),
bool(post.get('folder')),
post.get('auto_start'),
)
url = post.get('url')
download_type = post.get('download_type')
codec = post.get('codec')
@@ -415,7 +424,10 @@ async def add(request):
quality = 'best'
codec = 'auto'
playlist_item_limit = int(playlist_item_limit)
try:
playlist_item_limit = int(playlist_item_limit)
except (TypeError, ValueError) as exc:
raise web.HTTPBadRequest(reason='playlist_item_limit must be an integer') from exc
status = await dqueue.add(
url,
@@ -441,7 +453,7 @@ async def cancel_add(request):
@routes.post(config.URL_PREFIX + 'delete')
async def delete(request):
post = await request.json()
post = await _read_json_request(request)
ids = post.get('ids')
where = post.get('where')
if not ids or where not in ['queue', 'done']:
@@ -453,7 +465,7 @@ async def delete(request):
@routes.post(config.URL_PREFIX + 'start')
async def start(request):
post = await request.json()
post = await _read_json_request(request)
ids = post.get('ids')
log.info(f"Received request to start pending downloads for ids: {ids}")
status = await dqueue.start_pending(ids)
@@ -468,17 +480,23 @@ async def upload_cookies(request):
field = await reader.next()
if field is None or field.name != 'cookies':
return web.Response(status=400, text=serializer.encode({'status': 'error', 'msg': 'No cookies file provided'}))
max_size = 1_000_000 # 1MB limit
size = 0
with open(COOKIES_PATH, 'wb') as f:
while True:
chunk = await field.read_chunk()
if not chunk:
break
size += len(chunk)
if size > 1_000_000: # 1MB limit
os.remove(COOKIES_PATH)
return web.Response(status=400, text=serializer.encode({'status': 'error', 'msg': 'Cookie file too large (max 1MB)'}))
f.write(chunk)
content = bytearray()
while True:
chunk = await field.read_chunk()
if not chunk:
break
size += len(chunk)
if size > max_size:
return web.Response(status=400, text=serializer.encode({'status': 'error', 'msg': 'Cookie file too large (max 1MB)'}))
content.extend(chunk)
tmp_cookie_path = f"{COOKIES_PATH}.tmp"
with open(tmp_cookie_path, 'wb') as f:
f.write(content)
os.replace(tmp_cookie_path, COOKIES_PATH)
config.set_runtime_override('cookiefile', COOKIES_PATH)
log.info(f'Cookies file uploaded ({size} bytes)')
return web.Response(text=serializer.encode({'status': 'ok', 'msg': f'Cookies uploaded ({size} bytes)'}))
@@ -543,6 +561,22 @@ async def connect(sid, environ):
await sio.emit('ytdl_options_changed', serializer.encode(get_options_update_time()), to=sid)
def get_custom_dirs():
cache_ttl_seconds = 5
now = asyncio.get_running_loop().time()
cache_key = (
config.DOWNLOAD_DIR,
config.AUDIO_DOWNLOAD_DIR,
config.CUSTOM_DIRS_EXCLUDE_REGEX,
)
if (
hasattr(get_custom_dirs, "_cache_key")
and hasattr(get_custom_dirs, "_cache_value")
and hasattr(get_custom_dirs, "_cache_time")
and get_custom_dirs._cache_key == cache_key
and (now - get_custom_dirs._cache_time) < cache_ttl_seconds
):
return get_custom_dirs._cache_value
def recursive_dirs(base):
path = pathlib.Path(base)
@@ -579,10 +613,14 @@ def get_custom_dirs():
if config.DOWNLOAD_DIR != config.AUDIO_DOWNLOAD_DIR:
audio_download_dir = recursive_dirs(config.AUDIO_DOWNLOAD_DIR)
return {
result = {
"download_dir": download_dir,
"audio_download_dir": audio_download_dir
}
get_custom_dirs._cache_key = cache_key
get_custom_dirs._cache_time = now
get_custom_dirs._cache_value = result
return result
@routes.get(config.URL_PREFIX)
def index(request):

View File

@@ -0,0 +1,21 @@
import unittest
from app.dl_formats import get_format, get_opts
class DlFormatsTests(unittest.TestCase):
def test_audio_unknown_format_raises_value_error(self):
with self.assertRaises(ValueError):
get_format("audio", "auto", "invalid", "best")
def test_wav_does_not_enable_thumbnail_postprocessing(self):
opts = get_opts("audio", "auto", "wav", "best", {})
self.assertNotIn("writethumbnail", opts)
def test_mp3_enables_thumbnail_postprocessing(self):
opts = get_opts("audio", "auto", "mp3", "best", {})
self.assertTrue(opts.get("writethumbnail"))
if __name__ == "__main__":
unittest.main()

View File

@@ -229,6 +229,12 @@ class DownloadInfo:
class Download:
manager = None
@classmethod
def shutdown_manager(cls):
if cls.manager is not None:
cls.manager.shutdown()
cls.manager = None
def __init__(self, download_dir, temp_dir, output_template, output_template_chapter, quality, format, ytdl_opts, info):
self.download_dir = download_dir
self.temp_dir = temp_dir
@@ -568,20 +574,33 @@ class PersistentQueue:
log_prefix = f"PersistentQueue:{self.identifier} repair (sqlite3/file)"
log.debug(f"{log_prefix} started")
try:
result = subprocess.run(
f"sqlite3 {self.path} '.recover' | sqlite3 {self.path}.tmp",
recover_proc = subprocess.Popen(
["sqlite3", self.path, ".recover"],
stdout=subprocess.PIPE,
stderr=subprocess.PIPE,
text=True,
)
run_result = subprocess.run(
["sqlite3", f"{self.path}.tmp"],
stdin=recover_proc.stdout,
capture_output=True,
text=True,
shell=True,
timeout=60
timeout=60,
)
if result.stderr:
log.debug(f"{log_prefix} failed: {result.stderr}")
if recover_proc.stdout is not None:
recover_proc.stdout.close()
recover_stderr = recover_proc.stderr.read() if recover_proc.stderr is not None else ""
recover_proc.wait(timeout=60)
if run_result.stderr or recover_stderr:
error_text = " ".join(part for part in [recover_stderr.strip(), run_result.stderr.strip()] if part)
log.debug(f"{log_prefix} failed: {error_text}")
else:
shutil.move(f"{self.path}.tmp", self.path)
log.debug(f"{log_prefix}{result.stdout or " was successful, no output"}")
log.debug(f"{log_prefix}{run_result.stdout or ' was successful, no output'}")
except FileNotFoundError:
log.debug(f"{log_prefix} failed: 'sqlite3' was not found")
except subprocess.TimeoutExpired:
log.debug(f"{log_prefix} failed: sqlite recovery timed out")
class DownloadQueue:
def __init__(self, config, notifier):
@@ -629,7 +648,7 @@ class DownloadQueue:
if download.tmpfilename and os.path.isfile(download.tmpfilename):
try:
os.remove(download.tmpfilename)
except:
except OSError:
pass
download.info.status = 'error'
download.close()
@@ -898,7 +917,7 @@ class DownloadQueue:
async def start_pending(self, ids):
for id in ids:
if not self.pending.exists(id):
log.warn(f'requested start for non-existent download {id}')
log.warning(f'requested start for non-existent download {id}')
continue
dl = self.pending.get(id)
self.queue.put(dl)
@@ -915,7 +934,7 @@ class DownloadQueue:
await self.notifier.canceled(id)
continue
if not self.queue.exists(id):
log.warn(f'requested cancel for non-existent download {id}')
log.warning(f'requested cancel for non-existent download {id}')
continue
if self.queue.get(id).started():
self.queue.get(id).cancel()
@@ -927,7 +946,7 @@ class DownloadQueue:
async def clear(self, ids):
for id in ids:
if not self.done.exists(id):
log.warn(f'requested delete for non-existent download {id}')
log.warning(f'requested delete for non-existent download {id}')
continue
if self.config.DELETE_FILE_ON_TRASHCAN:
dl = self.done.get(id)
@@ -935,7 +954,7 @@ class DownloadQueue:
dldirectory, _ = self.__calc_download_path(dl.info.download_type, dl.info.folder)
os.remove(os.path.join(dldirectory, dl.info.filename))
except Exception as e:
log.warn(f'deleting file for download {id} failed with error message {e!r}')
log.warning(f'deleting file for download {id} failed with error message {e!r}')
self.done.delete(id)
await self.notifier.cleared(id)
return {'status': 'ok'}