From 7fa1fc7938187e678fdc6f33d572fb1011b8ddfb Mon Sep 17 00:00:00 2001 From: Alex Shnitman Date: Sun, 15 Mar 2026 20:53:13 +0200 Subject: [PATCH] code review fixes --- .github/workflows/main.yml | 39 ++- Dockerfile | 7 +- app/dl_formats.py | 12 +- app/main.py | 102 ++++-- app/tests/test_dl_formats.py | 21 ++ app/ytdl.py | 43 ++- ui/angular.json | 4 +- ui/src/app/app.config.ts | 2 +- ui/src/app/app.html | 52 +-- ui/src/app/app.sass | 67 +--- ui/src/app/app.ts | 319 +++++++++++------- ui/src/app/components/index.ts | 4 +- .../components/master-checkbox.component.ts | 8 +- .../components/slave-checkbox.component.ts | 12 +- ui/src/app/pipes/speed.pipe.spec.ts | 15 + ui/src/app/pipes/speed.pipe.ts | 38 +-- ui/src/app/services/downloads.service.ts | 142 +++----- ui/src/app/services/index.ts | 1 - ui/src/app/services/speed.service.ts | 39 --- ui/src/styles.sass | 19 ++ 20 files changed, 498 insertions(+), 448 deletions(-) create mode 100644 app/tests/test_dl_formats.py create mode 100644 ui/src/app/pipes/speed.pipe.spec.ts delete mode 100644 ui/src/app/services/speed.service.ts diff --git a/.github/workflows/main.yml b/.github/workflows/main.yml index 5e4a8aa..dd4a57c 100644 --- a/.github/workflows/main.yml +++ b/.github/workflows/main.yml @@ -6,13 +6,50 @@ on: - 'master' jobs: + quality-checks: + runs-on: ubuntu-latest + steps: + - name: Checkout + uses: actions/checkout@v6 + - name: Set up Node.js + uses: actions/setup-node@v6 + with: + node-version: lts/* + - name: Enable pnpm + run: corepack enable + - name: Install frontend dependencies + working-directory: ui + run: pnpm install --frozen-lockfile + - name: Run frontend lint + working-directory: ui + run: pnpm run lint + - name: Build frontend + working-directory: ui + run: pnpm run build + - name: Set up Python + uses: actions/setup-python@v6 + with: + python-version: '3.13' + - name: Run backend smoke checks + run: python -m compileall app + - name: Run backend tests + run: python -m unittest discover -s app/tests -p "test_*.py" + - name: Run Trivy filesystem scan + uses: aquasecurity/trivy-action@0.33.1 + with: + scan-type: fs + scan-ref: . + format: table + severity: CRITICAL,HIGH + dockerhub-build-push: + needs: quality-checks runs-on: ubuntu-latest steps: - name: Get current date id: date - run: echo "::set-output name=date::$(date +'%Y.%m.%d')" + run: echo "date=$(date +'%Y.%m.%d')" >> "$GITHUB_OUTPUT" - name: Checkout uses: actions/checkout@v6 diff --git a/Dockerfile b/Dockerfile index 4dff7c0..cad8d56 100644 --- a/Dockerfile +++ b/Dockerfile @@ -63,11 +63,12 @@ ENV PUID=1000 ENV PGID=1000 ENV UMASK=022 -ENV DOWNLOAD_DIR /downloads -ENV STATE_DIR /downloads/.metube -ENV TEMP_DIR /downloads +ENV DOWNLOAD_DIR=/downloads +ENV STATE_DIR=/downloads/.metube +ENV TEMP_DIR=/downloads VOLUME /downloads EXPOSE 8081 +HEALTHCHECK --interval=30s --timeout=5s --start-period=20s --retries=3 CMD curl -fsS "http://localhost:8081/" || exit 1 # Add build-time argument for version ARG VERSION=dev diff --git a/app/dl_formats.py b/app/dl_formats.py index e229366..7933451 100644 --- a/app/dl_formats.py +++ b/app/dl_formats.py @@ -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( { diff --git a/app/main.py b/app/main.py index 6e3cbbf..ff31190 100644 --- a/app/main.py +++ b/app/main.py @@ -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): diff --git a/app/tests/test_dl_formats.py b/app/tests/test_dl_formats.py new file mode 100644 index 0000000..5a1e92f --- /dev/null +++ b/app/tests/test_dl_formats.py @@ -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() diff --git a/app/ytdl.py b/app/ytdl.py index e738fe9..aa5df38 100644 --- a/app/ytdl.py +++ b/app/ytdl.py @@ -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'} diff --git a/ui/angular.json b/ui/angular.json index 6f0fddb..11245d1 100644 --- a/ui/angular.json +++ b/ui/angular.json @@ -33,9 +33,7 @@ "node_modules/@ng-select/ng-select/themes/default.theme.css", "src/styles.sass" ], - "scripts": [ - "node_modules/bootstrap/dist/js/bootstrap.bundle.min.js" - ], + "scripts": [], "serviceWorker": "ngsw-config.json", "browser": "src/main.ts", "polyfills": [ diff --git a/ui/src/app/app.config.ts b/ui/src/app/app.config.ts index b5fb6c0..6642eac 100644 --- a/ui/src/app/app.config.ts +++ b/ui/src/app/app.config.ts @@ -1,4 +1,4 @@ -import { ApplicationConfig, provideBrowserGlobalErrorListeners, isDevMode, provideZonelessChangeDetection, provideZoneChangeDetection } from '@angular/core'; +import { ApplicationConfig, provideBrowserGlobalErrorListeners, isDevMode, provideZoneChangeDetection } from '@angular/core'; import { provideServiceWorker } from '@angular/service-worker'; import { provideHttpClient, withInterceptorsFromDi } from '@angular/common/http'; diff --git a/ui/src/app/app.html b/ui/src/app/app.html index a54dddb..8ef50f8 100644 --- a/ui/src/app/app.html +++ b/ui/src/app/app.html @@ -49,22 +49,22 @@ -->