From 1a4fccef060586e4ffb5091804b069b20d545c04 Mon Sep 17 00:00:00 2001 From: nitrobass24 Date: Tue, 2 Jun 2026 20:40:26 -0500 Subject: [PATCH 1/6] Refactor: decompose LftpJobStatusParser.__parse_jobs (#523) Hoist the 17 per-call-compiled regexes in __parse_jobs to module-level re.compile() constants (compiled once at import, not per parse() call) and split the ~410-line, complexity-48 method into focused helpers driven by a thin ordered dispatch loop: - _parse_pget_header_block, _parse_mirror_header, _parse_mirror_fl_header, _parse_filename_chunk, _build_chunk_transfer_state (header/data parsing) - _skip_mirror_noise, _skip_chunk_header, _skip_chmod_block, _skip_noise_line (match-and-ignore leaf helpers) - _is_valid_first_line, _is_orphan_progress_line (guard helpers) - _dispatch_job_line + __parse_jobs (thin loop) Strictly behavior-preserving: every compiled pattern is byte-identical to the original (verified), load-bearing branch order and lookahead/pop semantics on the shared lines list are unchanged, and the pget-vs-filename name-comparison asymmetry is preserved. The shared regex fragment strings move to module scope (mangling-safe) with the class attributes aliasing them, so _size_to_bytes, _eta_to_seconds, and the out-of-scope __parse_queue are unaffected. Every function now measures cyclomatic complexity <= 12 (max is 10); the # noqa: C901 on __parse_jobs is removed (now 5). __parse_queue is left as-is. The existing 66 tests pass unchanged (no assertions modified); adds 34 focused helper tests including a compile-once identity regression. Co-Authored-By: Claude Opus 4.8 (1M context) --- src/python/lftp/job_status_parser.py | 846 +++++++++--------- .../test_lftp/test_job_status_parser.py | 297 ++++++ 2 files changed, 742 insertions(+), 401 deletions(-) diff --git a/src/python/lftp/job_status_parser.py b/src/python/lftp/job_status_parser.py index 61cb7281..253cd1f3 100644 --- a/src/python/lftp/job_status_parser.py +++ b/src/python/lftp/job_status_parser.py @@ -13,24 +13,170 @@ class LftpJobStatusParserError(AppError): pass +# Shared regex fragments. +# +# These live at module scope (rather than as ``__``-mangled class attributes) +# so the pre-compiled patterns below can interpolate them without tripping over +# name mangling. The class attributes on ``LftpJobStatusParser`` simply alias +# these so ``_size_to_bytes``, ``_eta_to_seconds`` and ``__parse_queue`` keep +# resolving the same strings they always did. + +# python doesn't support partial inline-modified flags, so we need +# to capture all case-sensitive cases here +_SIZE_UNITS_REGEX = ( + r"b|B|" + r"k|kb|kib|K|Kb|KB|KiB|Kib|" + r"m|mb|mib|M|Mb|MB|MiB|Mib|" + r"g|gb|gib|G|Gb|GB|GiB|Gib" +) +_TIME_UNITS_REGEX = r"(?P\d+d)?(?P\d+h)?(?P\d+m)?(?P\d+s)?" + +_QUOTED_FILE_NAME_REGEX = r"`(?P.*)'" + +_QUEUE_DONE_REGEX = r"^\[(?P\d+)\]\sDone\s\(queue\s\(.+\)\)" + + +# Pre-compiled patterns used by __parse_jobs. +# +# These were previously compiled inside __parse_jobs on every parse() call, +# which runs on the controller thread every status poll. Compiling them once +# at import time is behavior-preserving (regex compilation is referentially +# transparent) and avoids the per-cycle recompile cost. + +# pget header +_RE_PGET_HEADER = re.compile( + r"^\[(?P\d+)\]\s+" + r"pget\s+" + r"(?P.*?)\s+" + r"(?P['\"]|)(?P.+)(?P=lq)\s+" # greedy on purpose + r"-o\s+" + r"(?P['\"]|)(?P.+)(?P=rq)$" # greedy on purpose +) + +# mirror header (downloading) +_RE_MIRROR_HEADER = re.compile( + r"^\[(?P\d+)\]\s+" + r"mirror\s+" + r"(?P.*?)\s+" + r"(?P['\"]|)(?P.+)(?P=lq)\s+" # greedy on purpose + r"(?P['\"]|)(?P.+)(?P=rq)\s+" # greedy on purpose + r"--\s+" + rf"(?P\d+\.?\d*\s?({_SIZE_UNITS_REGEX})?)" # size=0 has no units + r"\/" + rf"(?P\d+\.?\d*\s?({_SIZE_UNITS_REGEX})?)\s+" # size=0 has no units + r"\((?P\d+)%\)" + rf"(\s+(?P\d+\.?\d*\s?({_SIZE_UNITS_REGEX}))\/s)?$" +) + +# mirror header (connecting or receiving file list) +_RE_MIRROR_FL_HEADER = re.compile( + r"^\[(?P\d+)\]\s+" + r"mirror\s+" + r"(?P.*?)\s+" + r"(?P['\"]|)(?P.+)(?P=lq)\s+" # greedy on purpose + r"(?P['\"]|)(?P.+)(?P=rq)$" # greedy on purpose +) + +# Data patterns +_RE_FILENAME = re.compile(r"\\transfer\s" + _QUOTED_FILE_NAME_REGEX) + +_RE_CHUNK_AT = re.compile( + ( + r"^" + _QUOTED_FILE_NAME_REGEX + r"\s+" + r"at\s+" + r"\d+\s+" # this is NOT the local size + r"(?:\(\d+%\)\s+)?" # this is NOT the local percent + r"((?P\d+\.?\d*\s?({sz}))\/s\s+)?" + r"(eta:(?P{eta})\s+)?" + r"\s*\[(?P.*)\]$" + ).format(sz=_SIZE_UNITS_REGEX, eta=_TIME_UNITS_REGEX) +) + +_RE_CHUNK_AT2 = re.compile( + r"^" + _QUOTED_FILE_NAME_REGEX + r"\s+" + r"at\s+" + r"\d+\s+" # this is NOT the local size + r"(?:\(\d+%\))" # this is NOT the local percent +) + +_RE_CHUNK_GOT = re.compile( + ( + r"^" + _QUOTED_FILE_NAME_REGEX + r",\s+" + r"got\s+" + r"(?P\d+)\s+" + r"of\s+" + r"(?P\d+)\s+" + r"\((?P\d+)%\)" + r"(\s+(?P\d+\.?\d*\s?({sz}))\/s)?" + r"(\seta:(?P{eta}))?" + ).format(sz=_SIZE_UNITS_REGEX, eta=_TIME_UNITS_REGEX) +) + +_RE_CHUNK_HEADER = re.compile(r"\\chunk\s+\d+") + +_RE_CHMOD_HEADER = re.compile(r"chmod\s(?P.*)") + +_RE_CHMOD = re.compile(_QUOTED_FILE_NAME_REGEX + r"\s\[\]") + +_RE_MIRROR = re.compile( + ( + r"\\mirror\s" + _QUOTED_FILE_NAME_REGEX + r"\s+" + r"--\s+" + r"(?P\d+\.?\d*\s?({sz})?)" # size=0 has no units + r"\/" + r"(?P\d+\.?\d*\s?({sz})?)\s+" # size=0 has no units + r"\((?P\d+)%\)" + r"(\s+(?P\d+\.?\d*\s?({sz}))\/s)?$" + ).format(sz=_SIZE_UNITS_REGEX) +) + +_RE_MIRROR_EMPTY = re.compile(r"\\mirror\s" + _QUOTED_FILE_NAME_REGEX + r"\s*$") + +_RE_QUEUE_DONE = re.compile(_QUEUE_DONE_REGEX) + +# Orphan progress lines lftp can emit outside a job context, e.g.: +# "3.0K/s eta:3m [Receiving data]" +# "10M/s eta:1h2m [Making data connection]" +_RE_ORPHAN_PROGRESS = re.compile( + rf"^(?:\d+\.?\d*\s?({_SIZE_UNITS_REGEX}))\/s\s+" + rf"eta:({_TIME_UNITS_REGEX})\s+" + r"\[.*\]$" +) + +# Partial progress fragments from line-wrap (seen on Unraid), e.g.: +# "/s eta:25m [Receiving data]" (tail of "347.3K/s eta:25m ...") +_RE_PARTIAL_PROGRESS = re.compile( + r"^\/s\s+" + rf"eta:({_TIME_UNITS_REGEX})\s+" + r"\[.*\]$" +) + +# Chunk line-wrap fragments: long filenames cause lftp chunk progress +# lines to wrap, producing a tail fragment like: +# "tmos.7.1.DV.HDR.H.265-TheFarm.mkv' at 22283455338 (0%) 427.6K/s eta:28m [Receiving data]" +# These are the tail of a `filename' at (%) ... line where +# the leading backtick and start of the filename are on the previous line. +_RE_CHUNK_WRAP = re.compile( + r"^(?:[^`\\].*)?'\s+at\s+\d+\s+" + r"(?:\(\d+%\)\s+)?" + rf"(?:(?:\d+\.?\d*\s?({_SIZE_UNITS_REGEX}))\/s\s+)?" + rf"(?:eta:({_TIME_UNITS_REGEX})\s+)?" + r"\s*\[.*\]$" +) + + class LftpJobStatusParser: """ Parses the output of lftp's "jobs -v" command into a LftpJobStatus """ - # python doesn't support partial inline-modified flags, so we need - # to capture all case-sensitive cases here - __SIZE_UNITS_REGEX = ( - r"b|B|" - r"k|kb|kib|K|Kb|KB|KiB|Kib|" - r"m|mb|mib|M|Mb|MB|MiB|Mib|" - r"g|gb|gib|G|Gb|GB|GiB|Gib" - ) - __TIME_UNITS_REGEX = r"(?P\d+d)?(?P\d+h)?(?P\d+m)?(?P\d+s)?" - - __QUOTED_FILE_NAME_REGEX = r"`(?P.*)'" - - __QUEUE_DONE_REGEX = r"^\[(?P\d+)\]\sDone\s\(queue\s\(.+\)\)" + # Aliases to the module-level fragments (see above). Kept as class + # attributes so existing references via the (mangled) ``__`` names keep + # resolving unchanged. + __SIZE_UNITS_REGEX = _SIZE_UNITS_REGEX + __TIME_UNITS_REGEX = _TIME_UNITS_REGEX + __QUOTED_FILE_NAME_REGEX = _QUOTED_FILE_NAME_REGEX + __QUEUE_DONE_REGEX = _QUEUE_DONE_REGEX def __init__(self): self.logger = logging.getLogger("LftpJobStatusParser") @@ -121,142 +267,297 @@ def parse(self, output: str) -> list[LftpJobStatus]: raise LftpJobStatusParserError("Error parsing lftp job status") from e return statuses - def __parse_jobs(self, lines: list[str]) -> list[LftpJobStatus]: # noqa: C901 — complexity 48, lftp output parser - jobs: list[LftpJobStatus] = [] + @staticmethod + def _is_orphan_progress_line(line: str) -> bool: + """True if ``line`` is a known progress fragment lftp emits outside a + job context (orphan progress, line-wrap partials, or wrapped chunk + tails) that should be skipped rather than raised on.""" + return bool(_RE_ORPHAN_PROGRESS.match(line) or _RE_PARTIAL_PROGRESS.match(line) or _RE_CHUNK_WRAP.match(line)) - # Header patterns - # pget header - pget_header_pattern = ( - r"^\[(?P\d+)\]\s+" - r"pget\s+" - r"(?P.*?)\s+" - r"(?P['\"]|)(?P.+)(?P=lq)\s+" # greedy on purpose - r"-o\s+" - r"(?P['\"]|)(?P.+)(?P=rq)$" - ) # greedy on purpose - pget_header_m = re.compile(pget_header_pattern) - - # mirror header (downloading) - mirror_header_pattern = ( - r"^\[(?P\d+)\]\s+" - r"mirror\s+" - r"(?P.*?)\s+" - r"(?P['\"]|)(?P.+)(?P=lq)\s+" # greedy on purpose - r"(?P['\"]|)(?P.+)(?P=rq)\s+" # greedy on purpose - r"--\s+" - rf"(?P\d+\.?\d*\s?({LftpJobStatusParser.__SIZE_UNITS_REGEX})?)" # size=0 has no units - r"\/" - rf"(?P\d+\.?\d*\s?({LftpJobStatusParser.__SIZE_UNITS_REGEX})?)\s+" # size=0 has no units - r"\((?P\d+)%\)" - rf"(\s+(?P\d+\.?\d*\s?({LftpJobStatusParser.__SIZE_UNITS_REGEX}))\/s)?$" - ) - mirror_header_m = re.compile(mirror_header_pattern) - - # mirror header (connecting or receiving file list) - mirror_fl_header_pattern = ( - r"^\[(?P\d+)\]\s+" - r"mirror\s+" - r"(?P.*?)\s+" - r"(?P['\"]|)(?P.+)(?P=lq)\s+" # greedy on purpose - r"(?P['\"]|)(?P.+)(?P=rq)$" - ) # greedy on purpose - mirror_fl_header_m = re.compile(mirror_fl_header_pattern) - - # Data patterns - filename_pattern = r"\\transfer\s" + LftpJobStatusParser.__QUOTED_FILE_NAME_REGEX - filename_m = re.compile(filename_pattern) - - chunk_at_pattern = ( - r"^" + LftpJobStatusParser.__QUOTED_FILE_NAME_REGEX + r"\s+" - r"at\s+" - r"\d+\s+" # this is NOT the local size - r"(?:\(\d+%\)\s+)?" # this is NOT the local percent - r"((?P\d+\.?\d*\s?({sz}))\/s\s+)?" - r"(eta:(?P{eta})\s+)?" - r"\s*\[(?P.*)\]$" - ).format(sz=LftpJobStatusParser.__SIZE_UNITS_REGEX, eta=LftpJobStatusParser.__TIME_UNITS_REGEX) - chunk_at_m = re.compile(chunk_at_pattern) - - chunk_at2_pattern = ( - r"^" + LftpJobStatusParser.__QUOTED_FILE_NAME_REGEX + r"\s+" - r"at\s+" - r"\d+\s+" # this is NOT the local size - r"(?:\(\d+%\))" - ) # this is NOT the local percent - chunk_at2_m = re.compile(chunk_at2_pattern) - - chunk_got_pattern = ( - r"^" + LftpJobStatusParser.__QUOTED_FILE_NAME_REGEX + r",\s+" - r"got\s+" - r"(?P\d+)\s+" - r"of\s+" - r"(?P\d+)\s+" - r"\((?P\d+)%\)" - r"(\s+(?P\d+\.?\d*\s?({sz}))\/s)?" - r"(\seta:(?P{eta}))?" - ).format(sz=LftpJobStatusParser.__SIZE_UNITS_REGEX, eta=LftpJobStatusParser.__TIME_UNITS_REGEX) - chunk_got_m = re.compile(chunk_got_pattern) - - chunk_header_pattern = r"\\chunk\s+\d+" - chunk_header_m = re.compile(chunk_header_pattern) - - chmod_header_pattern = ( - r"chmod\s" - r"(?P.*)" + @staticmethod + def _is_valid_first_line(line: str, prev_job: "LftpJobStatus | None") -> bool: + """The first line of a job block must be a valid header; once a job is + in context any line is permitted (the cascade decides what to do).""" + return bool( + prev_job or _RE_PGET_HEADER.match(line) or _RE_MIRROR_HEADER.match(line) or _RE_MIRROR_FL_HEADER.match(line) ) - chmod_header_m = re.compile(chmod_header_pattern) - - chmod_pattern = LftpJobStatusParser.__QUOTED_FILE_NAME_REGEX + r"\s\[\]" - chmod_pattern_m = re.compile(chmod_pattern) - mirror_pattern = ( - r"\\mirror\s" + LftpJobStatusParser.__QUOTED_FILE_NAME_REGEX + r"\s+" - r"--\s+" - r"(?P\d+\.?\d*\s?({sz})?)" # size=0 has no units - r"\/" - r"(?P\d+\.?\d*\s?({sz})?)\s+" # size=0 has no units - r"\((?P\d+)%\)" - r"(\s+(?P\d+\.?\d*\s?({sz}))\/s)?$" - ).format(sz=LftpJobStatusParser.__SIZE_UNITS_REGEX) - mirror_m = re.compile(mirror_pattern) + @staticmethod + def _build_chunk_transfer_state(result_at, result_at2, result_got) -> "LftpJobStatus.TransferState": + """Build a TransferState from whichever of the three chunk-data regex + matches is present. Caller is responsible for the name-mismatch checks + (which differ between the pget header and the filename chunk).""" + if result_at: + speed = None + if result_at.group("speed"): + speed = LftpJobStatusParser._size_to_bytes(result_at.group("speed")) + eta = None + if result_at.group("eta"): + eta = LftpJobStatusParser._eta_to_seconds(result_at.group("eta")) + return LftpJobStatus.TransferState(None, None, None, speed, eta) + if result_at2: + return LftpJobStatus.TransferState(None, None, None, None, None) + # result_got + size_local = int(result_got.group("szlocal")) + size_remote = int(result_got.group("szremote")) + percent_local = int(result_got.group("pctlocal")) + speed = None + if result_got.group("speed"): + speed = LftpJobStatusParser._size_to_bytes(result_got.group("speed")) + eta = None + if result_got.group("eta"): + eta = LftpJobStatusParser._eta_to_seconds(result_got.group("eta")) + return LftpJobStatus.TransferState(size_local, size_remote, percent_local, speed, eta) + + def _parse_pget_header_block(self, result, lines: list[str]) -> LftpJobStatus: + """Parse a pget header line (already matched) plus its mandatory 'sftp' + line and optional data line into a RUNNING pget LftpJobStatus.""" + # Next line must be the sftp line + if len(lines) < 1 or "sftp" not in lines[0]: + raise ValueError(f"Missing the 'sftp' line for pget header '{result.string}'") + lines.pop(0) # pop the 'sftp' line + + # Data line may not exist + result_at = None + result_at2 = None + result_got = None + if lines: + line = lines.pop(0) # data line + result_at = _RE_CHUNK_AT.search(line) + result_at2 = _RE_CHUNK_AT2.search(line) + result_got = _RE_CHUNK_GOT.search(line) + + id_ = int(result.group("id")) + name = os.path.basename(os.path.normpath(result.group("remote"))) + flags = result.group("flags") + status = LftpJobStatus( + job_id=id_, job_type=LftpJobStatus.Type.PGET, state=LftpJobStatus.State.RUNNING, name=name, flags=flags + ) + if result_at: + if result.group("remote") != result_at.group("name"): + raise ValueError( + "Mismatch between pget names '{}' vs '{}'".format(result.group("remote"), result_at.group("name")) + ) + elif result_at2: + if result.group("remote") != result_at2.group("name"): + raise ValueError( + "Mismatch between pget names '{}' vs '{}'".format(result.group("remote"), result_at2.group("name")) + ) + elif result_got: + got_group_basename = os.path.basename(os.path.normpath(result_got.group("name"))) + if got_group_basename != name: + raise ValueError(f"Mismatch: filename '{name}' but chunk data for '{got_group_basename}'") - mirror_empty_pattern = r"\\mirror\s" + LftpJobStatusParser.__QUOTED_FILE_NAME_REGEX + r"\s*$" - mirror_empty_m = re.compile(mirror_empty_pattern) + if result_at or result_at2 or result_got: + transfer_state = LftpJobStatusParser._build_chunk_transfer_state(result_at, result_at2, result_got) + else: + # No data line at all + transfer_state = LftpJobStatus.TransferState(None, None, None, None, None) - queue_done_m = re.compile(LftpJobStatusParser.__QUEUE_DONE_REGEX) + status.total_transfer_state = transfer_state + return status - # Orphan progress lines lftp can emit outside a job context, e.g.: - # "3.0K/s eta:3m [Receiving data]" - # "10M/s eta:1h2m [Making data connection]" - orphan_progress_pattern = ( - rf"^(?:\d+\.?\d*\s?({LftpJobStatusParser.__SIZE_UNITS_REGEX}))\/s\s+" - rf"eta:({LftpJobStatusParser.__TIME_UNITS_REGEX})\s+" - r"\[.*\]$" + @staticmethod + def _parse_mirror_header(result) -> LftpJobStatus: + """Parse a downloading mirror header line (already matched) into a + RUNNING mirror LftpJobStatus with size/speed totals.""" + id_ = int(result.group("id")) + name = os.path.basename(os.path.normpath(result.group("remote"))) + flags = result.group("flags") + status = LftpJobStatus( + job_id=id_, job_type=LftpJobStatus.Type.MIRROR, state=LftpJobStatus.State.RUNNING, name=name, flags=flags ) - orphan_progress_m = re.compile(orphan_progress_pattern) - - # Partial progress fragments from line-wrap (seen on Unraid), e.g.: - # "/s eta:25m [Receiving data]" (tail of "347.3K/s eta:25m ...") - partial_progress_pattern = ( - r"^\/s\s+" - rf"eta:({LftpJobStatusParser.__TIME_UNITS_REGEX})\s+" - r"\[.*\]$" + size_local = LftpJobStatusParser._size_to_bytes(result.group("szlocal")) + size_remote = LftpJobStatusParser._size_to_bytes(result.group("szremote")) + percent_local = int(result.group("pctlocal")) + speed = None + if result.group("speed"): + speed = LftpJobStatusParser._size_to_bytes(result.group("speed")) + status.total_transfer_state = LftpJobStatus.TransferState( + size_local, + size_remote, + percent_local, + speed, + None, # eta ) - partial_progress_m = re.compile(partial_progress_pattern) - - # Chunk line-wrap fragments: long filenames cause lftp chunk progress - # lines to wrap, producing a tail fragment like: - # "tmos.7.1.DV.HDR.H.265-TheFarm.mkv' at 22283455338 (0%) 427.6K/s eta:28m [Receiving data]" - # These are the tail of a `filename' at (%) ... line where - # the leading backtick and start of the filename are on the previous line. - chunk_wrap_pattern = ( - r"^(?:[^`\\].*)?'\s+at\s+\d+\s+" - r"(?:\(\d+%\)\s+)?" - rf"(?:(?:\d+\.?\d*\s?({LftpJobStatusParser.__SIZE_UNITS_REGEX}))\/s\s+)?" - rf"(?:eta:({LftpJobStatusParser.__TIME_UNITS_REGEX})\s+)?" - r"\s*\[.*\]$" + return status + + @staticmethod + def _parse_mirror_fl_header(result, lines: list[str]) -> LftpJobStatus: + """Parse a connecting / receiving-file-list mirror header (already + matched) into a RUNNING mirror LftpJobStatus, popping the optional + 'Getting file list'/'cd ' follow-up line.""" + # There may be a 'Connecting' or 'cd' line ahead, but not always + if lines and (lines[0].startswith("Getting file list") or lines[0].startswith("cd ")): + lines.pop(0) # pop the connecting line + id_ = int(result.group("id")) + name = os.path.basename(os.path.normpath(result.group("remote"))) + flags = result.group("flags") + return LftpJobStatus( + job_id=id_, job_type=LftpJobStatus.Type.MIRROR, state=LftpJobStatus.State.RUNNING, name=name, flags=flags ) - chunk_wrap_m = re.compile(chunk_wrap_pattern) + + def _parse_filename_chunk(self, result, lines: list[str], prev_job: "LftpJobStatus | None") -> None: + """Parse a '\\transfer `name'' line (already matched) plus its + following chunk-data line, registering the active file transfer state + on ``prev_job``.""" + name = result.group("name") + if not lines: + raise ValueError(f"Missing chunk data for filename '{name}'") + line = lines.pop(0) + result_at = _RE_CHUNK_AT.search(line) + result_at2 = _RE_CHUNK_AT2.search(line) + result_got = _RE_CHUNK_GOT.search(line) + basename = os.path.basename(os.path.normpath(name)) + if result_at: + # filename is full path, but chunk name is only normpath + if result_at.group("name") != basename: + raise ValueError( + "Mismatch: filename '{}' but chunk data for '{}'".format(name, result_at.group("name")) + ) + elif result_at2: + # filename is full path, but chunk name is only normpath + if result_at2.group("name") != basename: + raise ValueError( + "Mismatch: filename '{}' but chunk data for '{}'".format(name, result_at2.group("name")) + ) + elif result_got: + if result_got.group("name") != basename: + raise ValueError( + "Mismatch: filename '{}' but chunk data for '{}'".format(name, result_got.group("name")) + ) + else: + raise ValueError(f"Missing chunk data for filename '{name}'") + file_status = LftpJobStatusParser._build_chunk_transfer_state(result_at, result_at2, result_got) + assert prev_job is not None + prev_job.add_active_file_transfer_state(name, file_status) + + @staticmethod + def _skip_mirror_noise(line: str, lines: list[str]) -> bool: + """Match-and-ignore a '\\mirror' line. Returns True if handled. + The downloading '\\mirror' line carries no extra follow-up; the empty + '\\mirror' line may be followed by one ignorable line.""" + if _RE_MIRROR.search(line): + return True + result = _RE_MIRROR_EMPTY.search(line) + if result: + name = result.group("name") + # One of these lines may follow, ignore it as well + # "Getting files list" + # "cd" + # ": " + # "mkdir" + if lines and ( + "Getting file list" in lines[0] + or lines[0].startswith("cd ") + or lines[0] == f"{name}:" + or lines[0].startswith("mkdir ") + ): + lines.pop(0) + return True + return False + + @staticmethod + def _skip_chunk_header(line: str, lines: list[str]) -> bool: + """Match-and-ignore a '\\chunk' line plus its optional backtick data + line. Returns True if handled.""" + if not _RE_CHUNK_HEADER.search(line): + return False + # Also need to ignore the next line (chunk data) + if lines and lines[0].startswith("`"): + lines.pop(0) + return True + + @staticmethod + def _skip_chmod_block(line: str, lines: list[str]) -> bool: + """Match-and-ignore a 'chmod' line plus its mandatory 'file:' line and + optional matching '`name' []' line. Returns True if handled.""" + result = _RE_CHMOD_HEADER.search(line) + if not result: + return False + name = result.group("name") + # Also ignore the next one or two lines + if not lines or not lines[0].startswith("file:"): + raise ValueError(f"Missing 'file:' line for chmod '{name}'") + lines.pop(0) + if lines: + result_chmod = _RE_CHMOD.search(lines[0]) + if result_chmod: + name_chmod = result_chmod.group("name") + if name != name_chmod: + raise ValueError(f"Mismatch in names chmod '{name}'") + lines.pop(0) + return True + + def _skip_noise_line(self, line: str, lines: list[str], prev_job: "LftpJobStatus | None") -> bool: + """Handle the tail of the dispatch cascade for a line that matched no + header/data/ignore branch: the queue 'Done' line, in-job unrecognized + fragments, and out-of-context orphan progress. Returns True if handled; + raises ValueError for a truly unrecognized line outside any job.""" + # Search for the Done line, but it better be the last line + if _RE_QUEUE_DONE.match(line): + if lines: + raise ValueError("There are more lines after the 'Done' line") + return True + + # If we're inside a job context, skip any unrecognized line. + # PTY line-wrapping can produce arbitrary fragments (filename tails, + # partial speed/eta strings like "eta:4m [Receiving data]" or + # "ta:4m [Receiving data]") that no fixed regex can anticipate. + if prev_job is not None: + self.logger.warning("Skipping unrecognized line inside job context: '%s'", line) + return True + + # Outside a job context, check for known orphan progress lines + if LftpJobStatusParser._is_orphan_progress_line(line): + self.logger.warning("Skipping orphan lftp progress line: '%s'", line) + return True + + # Truly unrecognized line outside any job — raise so the caller + # can track consecutive errors and decide whether to propagate + raise ValueError(f"Unable to parse line '{line}'") + + def _dispatch_job_line(self, line: str, lines: list[str], prev_job: "LftpJobStatus | None"): + """Run the ordered dispatch cascade for a single popped line. Returns + the LftpJobStatus to push (and adopt as the new ``prev_job``) when the + line started a new job, or None when the line was data/noise that left + ``prev_job`` unchanged. ORDER IS LOAD-BEARING.""" + # Search for pget header + result = _RE_PGET_HEADER.search(line) + if result: + return self._parse_pget_header_block(result, lines) + + # Search for mirror header + result = _RE_MIRROR_HEADER.search(line) + if result: + return LftpJobStatusParser._parse_mirror_header(result) + + # Search for mirror connecting header + # Note: this must be after the more restrictive mirror header above + result = _RE_MIRROR_FL_HEADER.search(line) + if result: + return LftpJobStatusParser._parse_mirror_fl_header(result, lines) + + # Search for filename + result = _RE_FILENAME.search(line) + if result: + self._parse_filename_chunk(result, lines, prev_job) + return None + + # Search for but ignore "\mirror" / "\chunk" / "chmod" lines + if ( + LftpJobStatusParser._skip_mirror_noise(line, lines) + or LftpJobStatusParser._skip_chunk_header(line, lines) + or LftpJobStatusParser._skip_chmod_block(line, lines) + ): + return None + + # Done line / in-job skip / orphan skip / raise + self._skip_noise_line(line, lines, prev_job) + return None + + def __parse_jobs(self, lines: list[str]) -> list[LftpJobStatus]: + jobs: list[LftpJobStatus] = [] prev_job: LftpJobStatus | None = None while lines: @@ -265,273 +566,16 @@ def __parse_jobs(self, lines: list[str]) -> list[LftpJobStatus]: # noqa: C901 # First line must be a valid job header. # Exception: skip known orphan progress lines that lftp emits # outside a job context (e.g. "3.0K/s eta:3m [Receiving data]"). - if not ( - prev_job or pget_header_m.match(line) or mirror_header_m.match(line) or mirror_fl_header_m.match(line) - ): - if orphan_progress_m.match(line) or partial_progress_m.match(line) or chunk_wrap_m.match(line): + if not LftpJobStatusParser._is_valid_first_line(line, prev_job): + if LftpJobStatusParser._is_orphan_progress_line(line): self.logger.warning("Skipping orphan lftp progress line: '%s'", line) continue raise ValueError(f"First line is not a matching header '{line}'") - # Search for pget header - result = pget_header_m.search(line) - if result: - # Next line must be the sftp line - if len(lines) < 1 or "sftp" not in lines[0]: - raise ValueError(f"Missing the 'sftp' line for pget header '{line}'") - lines.pop(0) # pop the 'sftp' line - - # Data line may not exist - result_at = None - result_at2 = None - result_got = None - if lines: - line = lines.pop(0) # data line - result_at = chunk_at_m.search(line) - result_at2 = chunk_at2_m.search(line) - result_got = chunk_got_m.search(line) - - id_ = int(result.group("id")) - name = os.path.basename(os.path.normpath(result.group("remote"))) - flags = result.group("flags") - type_ = LftpJobStatus.Type.PGET - status = LftpJobStatus( - job_id=id_, job_type=type_, state=LftpJobStatus.State.RUNNING, name=name, flags=flags - ) - if result_at: - if result.group("remote") != result_at.group("name"): - raise ValueError( - "Mismatch between pget names '{}' vs '{}'".format( - result.group("remote"), result_at.group("name") - ) - ) - size_local = None - percent_local = None - speed = None - if result_at.group("speed"): - speed = LftpJobStatusParser._size_to_bytes(result_at.group("speed")) - eta = None - if result_at.group("eta"): - eta = LftpJobStatusParser._eta_to_seconds(result_at.group("eta")) - transfer_state = LftpJobStatus.TransferState( - size_local, - None, # size remote - percent_local, - speed, - eta, - ) - elif result_at2: - if result.group("remote") != result_at2.group("name"): - raise ValueError( - "Mismatch between pget names '{}' vs '{}'".format( - result.group("remote"), result_at2.group("name") - ) - ) - transfer_state = LftpJobStatus.TransferState(None, None, None, None, None) - elif result_got: - got_group_basename = os.path.basename(os.path.normpath(result_got.group("name"))) - if got_group_basename != name: - raise ValueError(f"Mismatch: filename '{name}' but chunk data for '{got_group_basename}'") - size_local = int(result_got.group("szlocal")) - size_remote = int(result_got.group("szremote")) - percent_local = int(result_got.group("pctlocal")) - speed = None - if result_got.group("speed"): - speed = LftpJobStatusParser._size_to_bytes(result_got.group("speed")) - eta = None - if result_got.group("eta"): - eta = LftpJobStatusParser._eta_to_seconds(result_got.group("eta")) - transfer_state = LftpJobStatus.TransferState(size_local, size_remote, percent_local, speed, eta) - else: - # No data line at all - transfer_state = LftpJobStatus.TransferState(None, None, None, None, None) - - status.total_transfer_state = transfer_state + status = self._dispatch_job_line(line, lines, prev_job) + if status is not None: jobs.append(status) prev_job = status - continue - - # Search for mirror header - result = mirror_header_m.search(line) - if result: - id_ = int(result.group("id")) - name = os.path.basename(os.path.normpath(result.group("remote"))) - flags = result.group("flags") - type_ = LftpJobStatus.Type.MIRROR - status = LftpJobStatus( - job_id=id_, job_type=type_, state=LftpJobStatus.State.RUNNING, name=name, flags=flags - ) - size_local = LftpJobStatusParser._size_to_bytes(result.group("szlocal")) - size_remote = LftpJobStatusParser._size_to_bytes(result.group("szremote")) - percent_local = int(result.group("pctlocal")) - speed = None - if result.group("speed"): - speed = LftpJobStatusParser._size_to_bytes(result.group("speed")) - transfer_state = LftpJobStatus.TransferState( - size_local, - size_remote, - percent_local, - speed, - None, # eta - ) - status.total_transfer_state = transfer_state - jobs.append(status) - prev_job = status - # Continue the outer loop - continue - - # Search for mirror connecting header - # Note: this must be after the more restrictive mirror header above - result = mirror_fl_header_m.search(line) - if result: - # There may be a 'Connecting' or 'cd' line ahead, but not always - if lines and (lines[0].startswith("Getting file list") or lines[0].startswith("cd ")): - lines.pop(0) # pop the connecting line - id_ = int(result.group("id")) - name = os.path.basename(os.path.normpath(result.group("remote"))) - flags = result.group("flags") - type_ = LftpJobStatus.Type.MIRROR - status = LftpJobStatus( - job_id=id_, job_type=type_, state=LftpJobStatus.State.RUNNING, name=name, flags=flags - ) - jobs.append(status) - prev_job = status - # Continue the outer loop - continue - - # Search for filename - result = filename_m.search(line) - if result: - name = result.group("name") - if not lines: - raise ValueError(f"Missing chunk data for filename '{name}'") - line = lines.pop(0) - result_at = chunk_at_m.search(line) - result_at2 = chunk_at2_m.search(line) - result_got = chunk_got_m.search(line) - if result_at: - # filename is full path, but chunk name is only normpath - if result_at.group("name") != os.path.basename(os.path.normpath(name)): - raise ValueError( - "Mismatch: filename '{}' but chunk data for '{}'".format(name, result_at.group("name")) - ) - size_local = None - percent_local = None - speed = None - if result_at.group("speed"): - speed = LftpJobStatusParser._size_to_bytes(result_at.group("speed")) - eta = None - if result_at.group("eta"): - eta = LftpJobStatusParser._eta_to_seconds(result_at.group("eta")) - file_status = LftpJobStatus.TransferState(size_local, None, percent_local, speed, eta) - assert prev_job is not None - prev_job.add_active_file_transfer_state(name, file_status) - elif result_at2: - # filename is full path, but chunk name is only normpath - if result_at2.group("name") != os.path.basename(os.path.normpath(name)): - raise ValueError( - "Mismatch: filename '{}' but chunk data for '{}'".format(name, result_at2.group("name")) - ) - file_status = LftpJobStatus.TransferState(None, None, None, None, None) - assert prev_job is not None - prev_job.add_active_file_transfer_state(name, file_status) - elif result_got: - if result_got.group("name") != os.path.basename(os.path.normpath(name)): - raise ValueError( - "Mismatch: filename '{}' but chunk data for '{}'".format(name, result_got.group("name")) - ) - size_local = int(result_got.group("szlocal")) - size_remote = int(result_got.group("szremote")) - percent_local = int(result_got.group("pctlocal")) - speed = None - if result_got.group("speed"): - speed = LftpJobStatusParser._size_to_bytes(result_got.group("speed")) - eta = None - if result_got.group("eta"): - eta = LftpJobStatusParser._eta_to_seconds(result_got.group("eta")) - file_status = LftpJobStatus.TransferState(size_local, size_remote, percent_local, speed, eta) - assert prev_job is not None - prev_job.add_active_file_transfer_state(name, file_status) - else: - raise ValueError(f"Missing chunk data for filename '{name}'") - # Continue the outer loop - continue - - # Search for but ignore "\mirror" line - result = mirror_m.search(line) - if result: - # Continue the outer loop - continue - result = mirror_empty_m.search(line) - if result: - name = result.group("name") - # One of these lines may follow, ignore it as well - # "Getting files list" - # "cd" - # ": " - # "mkdir" - if lines: - if ( - "Getting file list" in lines[0] - or lines[0].startswith("cd ") - or lines[0] == f"{name}:" - or lines[0].startswith("mkdir ") - ): - lines.pop(0) - # Continue the outer loop - continue - - # Search for but ignore "\chunk" line - result = chunk_header_m.search(line) - if result: - # Also need to ignore the next line (chunk data) - if lines and lines[0].startswith("`"): - lines.pop(0) - # Continue the outer loop - continue - - # Search for but ignore "chmod" line - result = chmod_header_m.search(line) - if result: - name = result.group("name") - # Also ignore the next one or two lines - if not lines or not lines[0].startswith("file:"): - raise ValueError(f"Missing 'file:' line for chmod '{name}'") - lines.pop(0) - if lines: - result_chmod = chmod_pattern_m.search(lines[0]) - if result_chmod: - name_chmod = result_chmod.group("name") - if name != name_chmod: - raise ValueError(f"Mismatch in names chmod '{name}'") - lines.pop(0) - # Continue the outer loop - continue - - # Search for the Done line, but it better be the last line - result = queue_done_m.match(line) - if result: - if lines: - raise ValueError("There are more lines after the 'Done' line") - # Continue the outer loop - continue - - # If we're inside a job context, skip any unrecognized line. - # PTY line-wrapping can produce arbitrary fragments (filename tails, - # partial speed/eta strings like "eta:4m [Receiving data]" or - # "ta:4m [Receiving data]") that no fixed regex can anticipate. - if prev_job is not None: - self.logger.warning("Skipping unrecognized line inside job context: '%s'", line) - continue - - # Outside a job context, check for known orphan progress lines - if orphan_progress_m.match(line) or partial_progress_m.match(line) or chunk_wrap_m.match(line): - self.logger.warning("Skipping orphan lftp progress line: '%s'", line) - continue - - # Truly unrecognized line outside any job — raise so the caller - # can track consecutive errors and decide whether to propagate - raise ValueError(f"Unable to parse line '{line}'") return jobs @staticmethod diff --git a/src/python/tests/unittests/test_lftp/test_job_status_parser.py b/src/python/tests/unittests/test_lftp/test_job_status_parser.py index d9d25fc4..54458d40 100644 --- a/src/python/tests/unittests/test_lftp/test_job_status_parser.py +++ b/src/python/tests/unittests/test_lftp/test_job_status_parser.py @@ -2,6 +2,7 @@ import unittest +import lftp.job_status_parser as jsp from lftp import LftpJobStatus, LftpJobStatusParser, LftpJobStatusParserError @@ -1686,3 +1687,299 @@ def test_queue_header_immediately_followed_by_commands_queued(self): ] self.assertEqual(len(golden), len(statuses)) self.assertEqual(golden, statuses) + + +# noinspection PyPep8 +class TestLftpJobStatusParserHelpers(unittest.TestCase): + """Focused unit tests for the helpers extracted from __parse_jobs (#523). + + These exercise each isolated lftp-format branch directly, without having to + drive the whole dispatch loop, which is the point of the decomposition. + """ + + def setUp(self): + self.maxDiff = None + self.parser = LftpJobStatusParser() + + # --- module-level pre-compiled regex constants (compile once) ------------- + + def test_module_regexes_are_compiled_once(self): + """The hoisted _RE_* constants must be the same compiled object across + two parse() calls (regression for the per-call recompile smell).""" + import re as _re + + re_names = [ + "_RE_PGET_HEADER", + "_RE_MIRROR_HEADER", + "_RE_MIRROR_FL_HEADER", + "_RE_FILENAME", + "_RE_CHUNK_AT", + "_RE_CHUNK_AT2", + "_RE_CHUNK_GOT", + "_RE_CHUNK_HEADER", + "_RE_CHMOD_HEADER", + "_RE_CHMOD", + "_RE_MIRROR", + "_RE_MIRROR_EMPTY", + "_RE_QUEUE_DONE", + "_RE_ORPHAN_PROGRESS", + "_RE_PARTIAL_PROGRESS", + "_RE_CHUNK_WRAP", + ] + for name in re_names: + self.assertTrue(hasattr(jsp, name), f"missing module regex {name}") + self.assertIsInstance(getattr(jsp, name), _re.Pattern, name) + + before = {name: id(getattr(jsp, name)) for name in re_names} + self.parser.parse("[0] mirror -c /remote/path/show /local/path/ -- 500M/1G (50%) 10M/s") + self.parser.parse("3.0K/s eta:3m [Receiving data]") + after = {name: id(getattr(jsp, name)) for name in re_names} + self.assertEqual(before, after, "compiled regexes must not be re-created per parse()") + + # --- _parse_mirror_header ------------------------------------------------- + + def test_parse_mirror_header(self): + line = "[1] mirror -c /remote/path/show /local/path/ -- 500M/1G (50%) 10M/s" + result = jsp._RE_MIRROR_HEADER.search(line) + status = LftpJobStatusParser._parse_mirror_header(result) + self.assertEqual(1, status.id) + self.assertEqual("show", status.name) + self.assertEqual(LftpJobStatus.Type.MIRROR, status.type) + self.assertEqual(LftpJobStatus.State.RUNNING, status.state) + self.assertEqual( + LftpJobStatus.TransferState(500 * 1024 * 1024, 1024 * 1024 * 1024, 50, 10 * 1024 * 1024, None), + status.total_transfer_state, + ) + + def test_parse_mirror_header_no_speed(self): + line = "[2] mirror -c /remote/path/abc /local/path/ -- 0/1.1k (0%)" + result = jsp._RE_MIRROR_HEADER.search(line) + status = LftpJobStatusParser._parse_mirror_header(result) + self.assertEqual("abc", status.name) + self.assertEqual(LftpJobStatus.TransferState(0, 1126, 0, None, None), status.total_transfer_state) + + # --- _parse_mirror_fl_header ---------------------------------------------- + + def test_parse_mirror_fl_header_pops_getting_file_list(self): + line = "[1] mirror -c /remote/path/show /local/path/" + lines = ["Getting file list (25) [Receiving data]", "next"] + result = jsp._RE_MIRROR_FL_HEADER.search(line) + status = LftpJobStatusParser._parse_mirror_fl_header(result, lines) + self.assertEqual("show", status.name) + self.assertEqual(LftpJobStatus.Type.MIRROR, status.type) + # The 'Getting file list' follow-up must be consumed + self.assertEqual(["next"], lines) + + def test_parse_mirror_fl_header_pops_cd_line(self): + line = "[2] mirror -c /remote/path/a /local/path/" + lines = ["cd `/remote/path/a' [Connecting...]"] + result = jsp._RE_MIRROR_FL_HEADER.search(line) + LftpJobStatusParser._parse_mirror_fl_header(result, lines) + self.assertEqual([], lines) + + def test_parse_mirror_fl_header_no_follow_up(self): + line = "[2] mirror -c /remote/path/a /local/path/" + lines = ["[3] mirror -c /remote/path/b /local/path/"] + result = jsp._RE_MIRROR_FL_HEADER.search(line) + LftpJobStatusParser._parse_mirror_fl_header(result, lines) + # Unrelated next header must NOT be popped + self.assertEqual(["[3] mirror -c /remote/path/b /local/path/"], lines) + + # --- _parse_pget_header_block --------------------------------------------- + + def test_parse_pget_header_block_with_at_line(self): + line = "[1] pget -c /tmp/test_lftp/remote/c -o /tmp/test_lftp/local/" + lines = [ + "sftp://someone:@localhost/home/someone", + "`/tmp/test_lftp/remote/c' at 4585 (3%) 1.2K/s eta:2m [Receiving data]", + ] + result = jsp._RE_PGET_HEADER.search(line) + status = self.parser._parse_pget_header_block(result, lines) + self.assertEqual("c", status.name) + self.assertEqual(LftpJobStatus.Type.PGET, status.type) + self.assertEqual(LftpJobStatus.TransferState(None, None, None, 1228, 120), status.total_transfer_state) + self.assertEqual([], lines) + + def test_parse_pget_header_block_missing_sftp_line_raises(self): + line = "[1] pget -c /tmp/test_lftp/remote/c -o /tmp/test_lftp/local/" + lines = ["this line lacks the protocol token"] + result = jsp._RE_PGET_HEADER.search(line) + with self.assertRaises(ValueError): + self.parser._parse_pget_header_block(result, lines) + + def test_parse_pget_header_block_no_data_line(self): + line = "[4] pget -c /tmp/test_lftp/remote/d -o /tmp/test_lftp/local/" + lines = ["sftp://someone:@localhost/home/someone"] + result = jsp._RE_PGET_HEADER.search(line) + status = self.parser._parse_pget_header_block(result, lines) + self.assertEqual("d", status.name) + self.assertEqual(LftpJobStatus.TransferState(None, None, None, None, None), status.total_transfer_state) + + def test_parse_pget_header_block_name_mismatch_raises(self): + line = "[1] pget -c /tmp/test_lftp/remote/c -o /tmp/test_lftp/local/" + lines = [ + "sftp://someone:@localhost/home/someone", + "`/tmp/test_lftp/remote/WRONG' at 10 (0%) [Receiving data]", + ] + result = jsp._RE_PGET_HEADER.search(line) + with self.assertRaises(ValueError): + self.parser._parse_pget_header_block(result, lines) + + # --- _parse_filename_chunk ------------------------------------------------ + + def test_parse_filename_chunk_registers_active_file(self): + prev_job = LftpJobStatus( + job_id=1, job_type=LftpJobStatus.Type.MIRROR, state=LftpJobStatus.State.RUNNING, name="a", flags="-c" + ) + line = "\\transfer `aa'" + lines = ["`aa' at 315 (1%) 90b/s eta:4m [Receiving data]"] + result = jsp._RE_FILENAME.search(line) + self.parser._parse_filename_chunk(result, lines, prev_job) + self.assertEqual( + [("aa", LftpJobStatus.TransferState(None, None, None, 90, 240))], + prev_job.get_active_file_transfer_states(), + ) + self.assertEqual([], lines) + + def test_parse_filename_chunk_missing_data_raises(self): + prev_job = LftpJobStatus( + job_id=1, job_type=LftpJobStatus.Type.MIRROR, state=LftpJobStatus.State.RUNNING, name="a", flags="-c" + ) + line = "\\transfer `aa'" + lines: list[str] = [] + result = jsp._RE_FILENAME.search(line) + with self.assertRaises(ValueError): + self.parser._parse_filename_chunk(result, lines, prev_job) + + def test_parse_filename_chunk_name_mismatch_raises(self): + prev_job = LftpJobStatus( + job_id=1, job_type=LftpJobStatus.Type.MIRROR, state=LftpJobStatus.State.RUNNING, name="a", flags="-c" + ) + line = "\\transfer `aa'" + lines = ["`bb' at 10 (0%) [Receiving data]"] + result = jsp._RE_FILENAME.search(line) + with self.assertRaises(ValueError): + self.parser._parse_filename_chunk(result, lines, prev_job) + + # --- _build_chunk_transfer_state ------------------------------------------ + + def test_build_chunk_transfer_state_got(self): + result_got = jsp._RE_CHUNK_GOT.search("`ab', got 13733 of 25165824 (0%) 4.0K/s eta:1h45m") + state = LftpJobStatusParser._build_chunk_transfer_state(None, None, result_got) + self.assertEqual(LftpJobStatus.TransferState(13733, 25165824, 0, 4096, 6300), state) + + def test_build_chunk_transfer_state_at2(self): + result_at2 = jsp._RE_CHUNK_AT2.search("`aa' at 59 (59%)") + state = LftpJobStatusParser._build_chunk_transfer_state(None, result_at2, None) + self.assertEqual(LftpJobStatus.TransferState(None, None, None, None, None), state) + + # --- skip helpers --------------------------------------------------------- + + def test_skip_chmod_block_two_liner(self): + line = "chmod Space.Trek.S08E04.sfv" + lines = ["file:/local/path/Space.Trek/Space.Trek.S08E04", "next"] + self.assertTrue(LftpJobStatusParser._skip_chmod_block(line, lines)) + self.assertEqual(["next"], lines) + + def test_skip_chmod_block_three_liner(self): + line = "chmod Space.Trek.S23E03.720p.r06" + lines = [ + "file:/local/path/Space.Trek.S23E03.720p", + "`Space.Trek.S23E03.720p.r06' []", + "next", + ] + self.assertTrue(LftpJobStatusParser._skip_chmod_block(line, lines)) + self.assertEqual(["next"], lines) + + def test_skip_chmod_block_missing_file_line_raises(self): + line = "chmod something" + lines = ["not a file line"] + with self.assertRaises(ValueError): + LftpJobStatusParser._skip_chmod_block(line, lines) + + def test_skip_chmod_block_not_a_chmod_line(self): + line = "\\transfer `aa'" + lines: list[str] = [] + self.assertFalse(LftpJobStatusParser._skip_chmod_block(line, lines)) + + def test_skip_mirror_noise_empty_pops_follow_up(self): + line = "\\mirror `Sample'" + lines = ["Sample:", "next"] + self.assertTrue(LftpJobStatusParser._skip_mirror_noise(line, lines)) + self.assertEqual(["next"], lines) + + def test_skip_mirror_noise_downloading_no_follow_up(self): + line = "\\mirror `ba' -- 23k/263k (8%) 6.9 KiB/s" + lines = ["\\transfer `ba/baa'"] + self.assertTrue(LftpJobStatusParser._skip_mirror_noise(line, lines)) + self.assertEqual(["\\transfer `ba/baa'"], lines) + + def test_skip_mirror_noise_not_a_mirror_line(self): + self.assertFalse(LftpJobStatusParser._skip_mirror_noise("chmod foo", [])) + + def test_skip_chunk_header_pops_backtick_line(self): + line = "\\chunk 0-6291456" + lines = ["`ab' at 4362 (0%) 1.1K/s eta:92m [Receiving data]", "next"] + self.assertTrue(LftpJobStatusParser._skip_chunk_header(line, lines)) + self.assertEqual(["next"], lines) + + def test_skip_chunk_header_no_data_line(self): + line = "\\chunk 5077" + lines: list[str] = [] + self.assertTrue(LftpJobStatusParser._skip_chunk_header(line, lines)) + self.assertEqual([], lines) + + def test_skip_chunk_header_not_a_chunk_line(self): + self.assertFalse(LftpJobStatusParser._skip_chunk_header("chmod foo", [])) + + # --- _skip_noise_line ----------------------------------------------------- + + def test_skip_noise_line_done_last(self): + line = "[0] Done (queue (sftp://someone:@localhost))" + self.assertTrue(self.parser._skip_noise_line(line, [], None)) + + def test_skip_noise_line_done_not_last_raises(self): + line = "[0] Done (queue (sftp://someone:@localhost))" + with self.assertRaises(ValueError): + self.parser._skip_noise_line(line, ["leftover"], None) + + def test_skip_noise_line_inside_job_skips_anything(self): + prev_job = LftpJobStatus( + job_id=1, job_type=LftpJobStatus.Type.MIRROR, state=LftpJobStatus.State.RUNNING, name="a", flags="-c" + ) + self.assertTrue(self.parser._skip_noise_line("completely unexpected garbage", [], prev_job)) + + def test_skip_noise_line_orphan_outside_job_skips(self): + self.assertTrue(self.parser._skip_noise_line("3.0K/s eta:3m [Receiving data]", [], None)) + + def test_skip_noise_line_unrecognized_outside_job_raises(self): + with self.assertRaises(ValueError): + self.parser._skip_noise_line("completely unexpected garbage", [], None) + + # --- guard helpers -------------------------------------------------------- + + def test_is_valid_first_line_header(self): + self.assertTrue( + LftpJobStatusParser._is_valid_first_line( + "[1] mirror -c /remote/path/show /local/path/ -- 500M/1G (50%) 10M/s", None + ) + ) + + def test_is_valid_first_line_in_job_context(self): + prev_job = LftpJobStatus( + job_id=1, job_type=LftpJobStatus.Type.MIRROR, state=LftpJobStatus.State.RUNNING, name="a", flags="-c" + ) + self.assertTrue(LftpJobStatusParser._is_valid_first_line("anything goes now", prev_job)) + + def test_is_valid_first_line_bad_first_line(self): + self.assertFalse(LftpJobStatusParser._is_valid_first_line("garbage", None)) + + def test_is_orphan_progress_line(self): + self.assertTrue(LftpJobStatusParser._is_orphan_progress_line("3.0K/s eta:3m [Receiving data]")) + self.assertTrue(LftpJobStatusParser._is_orphan_progress_line("/s eta:25m [Receiving data]")) + self.assertTrue( + LftpJobStatusParser._is_orphan_progress_line( + "tmos.7.1.DV.HDR.H.265-TheFarm.mkv' at 22283455338 (0%) 427.6K/s eta:28m [Receiving data]" + ) + ) + self.assertFalse(LftpJobStatusParser._is_orphan_progress_line("garbage")) From 318608fa952e36f9552854d23f8549545faa5834 Mon Sep 17 00:00:00 2001 From: nitrobass24 Date: Tue, 2 Jun 2026 20:46:14 -0500 Subject: [PATCH 2/6] Refactor: extract capabilities module + selection service from ViewFileService (#524) Split the ViewFileService god service incrementally and behavior-preservingly: - Extract the status->capability state machine into a pure, separately-tested module view-file-capabilities.ts. The four deletion/extraction allow-lists that were duplicated copy-paste arrays are now named constants (single source of truth): QUEUEABLE_STATUSES, STOPPABLE_STATUSES, LOCAL_ACTION_STATUSES (shared verbatim by isExtractable + isLocallyDeletable), REMOTELY_DELETABLE_STATUSES, VALIDATABLE_STATUSES. Pure mapState() + deriveCapabilities() reproduce the exact switch (incl. DEFAULT->STOPPED-when-both-sizes>0 and unknown->DEFAULT) and the exact capability predicates, keyed off RAW nullable sizes for isValidatable / validateTooltip. createViewFile now calls them and spreads the result; its signature, percentDownloaded math, pairName resolution, and ViewFile shape are byte-for-byte identical. - Extract ViewFileSelectionService owning checkedSet, lastCheckedKey, and the checked$ stream. It is input-driven (callers pass display-order key lists) with no back-reference to ViewFileService, so the dependency stays acyclic (view-file-sort.service already injects ViewFileService). ViewFileService keeps toggleCheck/shiftCheck/checkAll/uncheckAll/checked$ as thin delegations; updateCheckedState still re-spreads only flipped rows (issue #521 identity); buildViewFromModelFiles uses selection.isChecked + selection.pruneRemoved. ViewFileService's public API and exports are unchanged, so file-list.component, file-options.component, view-file-sort.service and app.config need no edits. Existing view-file.service.spec.ts passes with zero assertion changes; added view-file-capabilities.spec.ts (34) and view-file-selection.service.spec.ts (16). DEFERRED: the ViewFileCommandService split (createAction/queue/.../bulk*). That slice straddles diffing-owned prevModelFiles and selection-owned check state; a clean extraction needs a ModelFile-resolver + checked-snapshot threaded in or a back-reference, both larger seams than a strictly behavior-preserving pass warrants here. Recommend as a follow-up. Co-Authored-By: Claude Opus 4.8 (1M context) --- .../files/view-file-capabilities.spec.ts | 189 ++++++++++++++++++ .../services/files/view-file-capabilities.ts | 165 +++++++++++++++ .../files/view-file-selection.service.spec.ts | 165 +++++++++++++++ .../files/view-file-selection.service.ts | 115 +++++++++++ .../app/services/files/view-file.service.ts | 186 +++-------------- 5 files changed, 667 insertions(+), 153 deletions(-) create mode 100644 src/angular/src/app/services/files/view-file-capabilities.spec.ts create mode 100644 src/angular/src/app/services/files/view-file-capabilities.ts create mode 100644 src/angular/src/app/services/files/view-file-selection.service.spec.ts create mode 100644 src/angular/src/app/services/files/view-file-selection.service.ts diff --git a/src/angular/src/app/services/files/view-file-capabilities.spec.ts b/src/angular/src/app/services/files/view-file-capabilities.spec.ts new file mode 100644 index 00000000..0332d13b --- /dev/null +++ b/src/angular/src/app/services/files/view-file-capabilities.spec.ts @@ -0,0 +1,189 @@ +import { describe, it, expect } from 'vitest'; + +import { ModelFileState } from '../../models/model-file'; +import { ViewFileStatus } from '../../models/view-file'; +import { + mapState, + deriveCapabilities, + QUEUEABLE_STATUSES, + STOPPABLE_STATUSES, + LOCAL_ACTION_STATUSES, + REMOTELY_DELETABLE_STATUSES, + VALIDATABLE_STATUSES, + VALIDATE_UNAVAILABLE_TOOLTIP, +} from './view-file-capabilities'; + +describe('view-file-capabilities', () => { + describe('mapState', () => { + // Every 1:1 state -> status mapping (sizes irrelevant for these). + const directCases: [ModelFileState, ViewFileStatus][] = [ + [ModelFileState.QUEUED, ViewFileStatus.QUEUED], + [ModelFileState.DOWNLOADING, ViewFileStatus.DOWNLOADING], + [ModelFileState.DOWNLOADED, ViewFileStatus.DOWNLOADED], + [ModelFileState.DELETED, ViewFileStatus.DELETED], + [ModelFileState.EXTRACTING, ViewFileStatus.EXTRACTING], + [ModelFileState.EXTRACTED, ViewFileStatus.EXTRACTED], + [ModelFileState.EXTRACT_FAILED, ViewFileStatus.EXTRACT_FAILED], + [ModelFileState.VALIDATING, ViewFileStatus.VALIDATING], + [ModelFileState.VALIDATED, ViewFileStatus.VALIDATED], + [ModelFileState.CORRUPT, ViewFileStatus.CORRUPT], + ]; + + for (const [state, expected] of directCases) { + it(`maps ${state} -> ${expected}`, () => { + expect(mapState(state, 0, 0)).toBe(expected); + expect(mapState(state, 100, 200)).toBe(expected); + }); + } + + it('maps DEFAULT with both sizes > 0 to STOPPED', () => { + expect(mapState(ModelFileState.DEFAULT, 50, 200)).toBe(ViewFileStatus.STOPPED); + }); + + it('maps DEFAULT with local_size = 0 to DEFAULT', () => { + expect(mapState(ModelFileState.DEFAULT, 0, 200)).toBe(ViewFileStatus.DEFAULT); + }); + + it('maps DEFAULT with remote_size = 0 to DEFAULT', () => { + expect(mapState(ModelFileState.DEFAULT, 50, 0)).toBe(ViewFileStatus.DEFAULT); + }); + + it('maps DEFAULT with both sizes = 0 to DEFAULT', () => { + expect(mapState(ModelFileState.DEFAULT, 0, 0)).toBe(ViewFileStatus.DEFAULT); + }); + + it('falls through to DEFAULT for an unknown state', () => { + expect(mapState('something-unexpected' as ModelFileState, 100, 200)).toBe(ViewFileStatus.DEFAULT); + }); + }); + + describe('deriveCapabilities', () => { + // isQueueable: in QUEUEABLE_STATUSES with remoteSize > 0, OR CORRUPT. + it('marks queueable statuses queueable only when remoteSize > 0', () => { + for (const status of QUEUEABLE_STATUSES) { + expect(deriveCapabilities(status, 0, 100, 0, 100).isQueueable).toBe(true); + expect(deriveCapabilities(status, 0, 0, 0, 0).isQueueable).toBe(false); + } + }); + + it('marks CORRUPT queueable even when remoteSize is 0', () => { + expect(deriveCapabilities(ViewFileStatus.CORRUPT, 0, 0, 0, 0).isQueueable).toBe(true); + }); + + it('does not mark a non-queueable status queueable', () => { + expect(deriveCapabilities(ViewFileStatus.DOWNLOADING, 0, 100, 0, 100).isQueueable).toBe(false); + }); + + it('marks stoppable statuses stoppable', () => { + for (const status of STOPPABLE_STATUSES) { + expect(deriveCapabilities(status, 0, 0, 0, 0).isStoppable).toBe(true); + } + expect(deriveCapabilities(ViewFileStatus.DEFAULT, 0, 0, 0, 0).isStoppable).toBe(false); + }); + + it('marks isExtractable / isLocallyDeletable for local-action statuses only when localSize > 0', () => { + for (const status of LOCAL_ACTION_STATUSES) { + const onWithLocal = deriveCapabilities(status, 100, 0, 100, 0); + expect(onWithLocal.isExtractable).toBe(true); + expect(onWithLocal.isLocallyDeletable).toBe(true); + + const offNoLocal = deriveCapabilities(status, 0, 100, 0, 100); + expect(offNoLocal.isExtractable).toBe(false); + expect(offNoLocal.isLocallyDeletable).toBe(false); + } + }); + + it('does not mark non-local-action statuses extractable / locally-deletable', () => { + const caps = deriveCapabilities(ViewFileStatus.QUEUED, 100, 100, 100, 100); + expect(caps.isExtractable).toBe(false); + expect(caps.isLocallyDeletable).toBe(false); + }); + + it('keeps isExtractable and isLocallyDeletable in lockstep (shared allow-list)', () => { + for (const status of Object.values(ViewFileStatus)) { + const caps = deriveCapabilities(status, 100, 100, 100, 100); + expect(caps.isExtractable).toBe(caps.isLocallyDeletable); + } + }); + + it('marks isRemotelyDeletable for remote-deletable statuses only when remoteSize > 0', () => { + for (const status of REMOTELY_DELETABLE_STATUSES) { + expect(deriveCapabilities(status, 0, 100, 0, 100).isRemotelyDeletable).toBe(true); + expect(deriveCapabilities(status, 0, 0, 0, 0).isRemotelyDeletable).toBe(false); + } + }); + + it('marks DELETED remotely-deletable with remoteSize > 0 (delete-from-remote after local delete)', () => { + expect(deriveCapabilities(ViewFileStatus.DELETED, 0, 100, 0, 100).isRemotelyDeletable).toBe(true); + }); + + it('marks isValidatable only when status allows AND both raw sizes are non-null', () => { + for (const status of VALIDATABLE_STATUSES) { + expect(deriveCapabilities(status, 100, 100, 100, 100).isValidatable).toBe(true); + // raw null on either side disables validation + expect(deriveCapabilities(status, 100, 0, 100, null).isValidatable).toBe(false); + expect(deriveCapabilities(status, 0, 100, null, 100).isValidatable).toBe(false); + expect(deriveCapabilities(status, 0, 0, null, null).isValidatable).toBe(false); + } + }); + + it('does not mark a non-validatable status validatable even with both sizes present', () => { + const caps = deriveCapabilities(ViewFileStatus.DEFAULT, 100, 100, 100, 100); + expect(caps.isValidatable).toBe(false); + expect(caps.validateTooltip).toBeNull(); + }); + + it('keys isValidatable off RAW null-ness, not coalesced sizes', () => { + // Coalesced sizes are both > 0, but raw remote is null -> not validatable. + const caps = deriveCapabilities(ViewFileStatus.DOWNLOADED, 100, 0, 100, null); + expect(caps.isValidatable).toBe(false); + }); + + it('shows the unavailable tooltip when validatable-status but raw remote_size is null', () => { + const caps = deriveCapabilities(ViewFileStatus.DOWNLOADED, 100, 0, 100, null); + expect(caps.validateTooltip).toBe(VALIDATE_UNAVAILABLE_TOOLTIP); + }); + + it('shows the unavailable tooltip when both raw sizes are null', () => { + const caps = deriveCapabilities(ViewFileStatus.DOWNLOADED, 0, 0, null, null); + expect(caps.validateTooltip).toBe(VALIDATE_UNAVAILABLE_TOOLTIP); + }); + + it('does not show a tooltip when only the raw local_size is null (remote present)', () => { + const caps = deriveCapabilities(ViewFileStatus.DOWNLOADED, 0, 100, null, 100); + expect(caps.isValidatable).toBe(false); + expect(caps.validateTooltip).toBeNull(); + }); + + it('does not show a tooltip when the file is validatable', () => { + const caps = deriveCapabilities(ViewFileStatus.DOWNLOADED, 100, 100, 100, 100); + expect(caps.isValidatable).toBe(true); + expect(caps.validateTooltip).toBeNull(); + }); + + it('does not show a tooltip for a non-validatable status with null remote', () => { + const caps = deriveCapabilities(ViewFileStatus.QUEUED, 100, 0, 100, null); + expect(caps.validateTooltip).toBeNull(); + }); + }); + + describe('allow-list constants are the single source of truth', () => { + it('shares the verbatim LOCAL_ACTION_STATUSES list for extraction and local deletion', () => { + // isExtractable and isLocallyDeletable both gate on this list; assert the + // exact membership so a divergence would fail here rather than silently. + expect([...LOCAL_ACTION_STATUSES]).toEqual([ + ViewFileStatus.DEFAULT, + ViewFileStatus.STOPPED, + ViewFileStatus.DOWNLOADED, + ViewFileStatus.EXTRACTED, + ViewFileStatus.EXTRACT_FAILED, + ViewFileStatus.VALIDATED, + ViewFileStatus.CORRUPT, + ]); + }); + + it('REMOTELY_DELETABLE_STATUSES is LOCAL_ACTION_STATUSES plus DELETED', () => { + expect([...REMOTELY_DELETABLE_STATUSES]).toEqual([...LOCAL_ACTION_STATUSES, ViewFileStatus.DELETED]); + }); + }); +}); diff --git a/src/angular/src/app/services/files/view-file-capabilities.ts b/src/angular/src/app/services/files/view-file-capabilities.ts new file mode 100644 index 00000000..0a40c84b --- /dev/null +++ b/src/angular/src/app/services/files/view-file-capabilities.ts @@ -0,0 +1,165 @@ +import { ModelFileState } from '../../models/model-file'; +import { ViewFileStatus } from '../../models/view-file'; + +/** + * Pure status/capability state machine for view files. + * + * This is the single source of truth for: + * - mapping a backend {@link ModelFileState} to a {@link ViewFileStatus}, and + * - deriving the six action-capability flags + the validate tooltip from a + * status and the file's sizes. + * + * The allow-lists below are named constants so each set of eligible statuses + * is declared exactly once (the deletion/extraction lists used to be duplicated + * copy-paste arrays kept in sync by hand — issue #524 part 2). + */ + +/** Statuses from which a file can be (re-)queued for download. */ +export const QUEUEABLE_STATUSES: readonly ViewFileStatus[] = [ + ViewFileStatus.DEFAULT, + ViewFileStatus.STOPPED, + ViewFileStatus.DELETED, +]; + +/** Statuses from which an in-flight transfer can be stopped. */ +export const STOPPABLE_STATUSES: readonly ViewFileStatus[] = [ + ViewFileStatus.QUEUED, + ViewFileStatus.DOWNLOADING, +]; + +/** + * Statuses that permit local-side actions (extraction and local deletion). + * Both isExtractable and isLocallyDeletable gate on this verbatim list. + */ +export const LOCAL_ACTION_STATUSES: readonly ViewFileStatus[] = [ + ViewFileStatus.DEFAULT, + ViewFileStatus.STOPPED, + ViewFileStatus.DOWNLOADED, + ViewFileStatus.EXTRACTED, + ViewFileStatus.EXTRACT_FAILED, + ViewFileStatus.VALIDATED, + ViewFileStatus.CORRUPT, +]; + +/** Statuses from which the remote copy can be deleted. */ +export const REMOTELY_DELETABLE_STATUSES: readonly ViewFileStatus[] = [ + ViewFileStatus.DEFAULT, + ViewFileStatus.STOPPED, + ViewFileStatus.DOWNLOADED, + ViewFileStatus.EXTRACTED, + ViewFileStatus.EXTRACT_FAILED, + ViewFileStatus.VALIDATED, + ViewFileStatus.CORRUPT, + ViewFileStatus.DELETED, +]; + +/** Statuses from which a checksum validation can be requested. */ +export const VALIDATABLE_STATUSES: readonly ViewFileStatus[] = [ + ViewFileStatus.DOWNLOADED, + ViewFileStatus.EXTRACTED, + ViewFileStatus.EXTRACT_FAILED, + ViewFileStatus.VALIDATED, + ViewFileStatus.CORRUPT, +]; + +export const VALIDATE_UNAVAILABLE_TOOLTIP = 'Remote file not available for checksum comparison'; + +/** + * The six capability flags plus the validate tooltip derived for a view file. + */ +export interface ViewFileCapabilities { + isQueueable: boolean; + isStoppable: boolean; + isExtractable: boolean; + isLocallyDeletable: boolean; + isRemotelyDeletable: boolean; + isValidatable: boolean; + validateTooltip: string | null; +} + +/** + * Map a backend model-file state to the view status. + * + * The only non-1:1 case is DEFAULT: a file in DEFAULT with both a local and a + * remote size present is a partially-downloaded file that was stopped, so it + * surfaces as STOPPED. Unknown states fall through to DEFAULT. + * + * @param state the backend model-file state + * @param localSize coalesced (non-null) local size in bytes + * @param remoteSize coalesced (non-null) remote size in bytes + */ +export function mapState(state: ModelFileState, localSize: number, remoteSize: number): ViewFileStatus { + switch (state) { + case ModelFileState.DEFAULT: + return localSize > 0 && remoteSize > 0 ? ViewFileStatus.STOPPED : ViewFileStatus.DEFAULT; + case ModelFileState.QUEUED: + return ViewFileStatus.QUEUED; + case ModelFileState.DOWNLOADING: + return ViewFileStatus.DOWNLOADING; + case ModelFileState.DOWNLOADED: + return ViewFileStatus.DOWNLOADED; + case ModelFileState.DELETED: + return ViewFileStatus.DELETED; + case ModelFileState.EXTRACTING: + return ViewFileStatus.EXTRACTING; + case ModelFileState.EXTRACTED: + return ViewFileStatus.EXTRACTED; + case ModelFileState.EXTRACT_FAILED: + return ViewFileStatus.EXTRACT_FAILED; + case ModelFileState.VALIDATING: + return ViewFileStatus.VALIDATING; + case ModelFileState.VALIDATED: + return ViewFileStatus.VALIDATED; + case ModelFileState.CORRUPT: + return ViewFileStatus.CORRUPT; + default: + return ViewFileStatus.DEFAULT; + } +} + +/** + * Derive the action-capability flags and validate tooltip for a view file. + * + * isValidatable and validateTooltip key off the RAW nullable sizes + * (`modelFile.local_size`/`modelFile.remote_size`), not the coalesced numeric + * sizes — a file can be validatable only when both sizes are actually known, + * and the "remote not available" tooltip is shown specifically when the remote + * size is null. The other flags gate on the coalesced numeric sizes. + * + * @param status the resolved view status + * @param localSize coalesced (non-null) local size in bytes + * @param remoteSize coalesced (non-null) remote size in bytes + * @param localSizeRaw the raw, possibly-null local size from the model file + * @param remoteSizeRaw the raw, possibly-null remote size from the model file + */ +export function deriveCapabilities( + status: ViewFileStatus, + localSize: number, + remoteSize: number, + localSizeRaw: number | null, + remoteSizeRaw: number | null, +): ViewFileCapabilities { + const isQueueable = + (QUEUEABLE_STATUSES.includes(status) && remoteSize > 0) || status === ViewFileStatus.CORRUPT; + const isStoppable = STOPPABLE_STATUSES.includes(status); + const isExtractable = LOCAL_ACTION_STATUSES.includes(status) && localSize > 0; + const isLocallyDeletable = LOCAL_ACTION_STATUSES.includes(status) && localSize > 0; + const isRemotelyDeletable = REMOTELY_DELETABLE_STATUSES.includes(status) && remoteSize > 0; + const isValidatable = + VALIDATABLE_STATUSES.includes(status) && localSizeRaw != null && remoteSizeRaw != null; + + let validateTooltip: string | null = null; + if (!isValidatable && VALIDATABLE_STATUSES.includes(status) && remoteSizeRaw == null) { + validateTooltip = VALIDATE_UNAVAILABLE_TOOLTIP; + } + + return { + isQueueable, + isStoppable, + isExtractable, + isLocallyDeletable, + isRemotelyDeletable, + isValidatable, + validateTooltip, + }; +} diff --git a/src/angular/src/app/services/files/view-file-selection.service.spec.ts b/src/angular/src/app/services/files/view-file-selection.service.spec.ts new file mode 100644 index 00000000..ad18d2ba --- /dev/null +++ b/src/angular/src/app/services/files/view-file-selection.service.spec.ts @@ -0,0 +1,165 @@ +import { describe, it, expect, beforeEach } from 'vitest'; + +import { ViewFileSelectionService } from './view-file-selection.service'; + +describe('ViewFileSelectionService', () => { + let service: ViewFileSelectionService; + + beforeEach(() => { + service = new ViewFileSelectionService(); + }); + + function latestChecked(): Set { + let result = new Set(); + service.checked$.subscribe((s) => (result = s)); + return result; + } + + it('starts with an empty checked set', () => { + expect(latestChecked().size).toBe(0); + expect(service.snapshot().size).toBe(0); + expect(service.isChecked('a')).toBe(false); + }); + + // --- toggle --- + + it('toggle adds a key when absent and removes it when present', () => { + service.toggle('a'); + expect(service.isChecked('a')).toBe(true); + expect(latestChecked().has('a')).toBe(true); + + service.toggle('a'); + expect(service.isChecked('a')).toBe(false); + expect(latestChecked().has('a')).toBe(false); + }); + + it('toggle emits a fresh defensive copy each time', () => { + const emissions: Set[] = []; + service.checked$.subscribe((s) => emissions.push(s)); + const initial = emissions.length; + + service.toggle('a'); + service.toggle('b'); + + expect(emissions.length).toBe(initial + 2); + // Distinct set instances, not the internal mutable set. + expect(emissions[emissions.length - 1]).not.toBe(emissions[emissions.length - 2]); + expect(emissions[emissions.length - 1].has('a')).toBe(true); + expect(emissions[emissions.length - 1].has('b')).toBe(true); + }); + + // --- shiftRange --- + + it('shiftRange falls back to toggle when there is no prior anchor', () => { + service.shiftRange('b', ['a', 'b', 'c']); + expect(service.snapshot()).toEqual(new Set(['b'])); + }); + + it('shiftRange selects the inclusive forward range from the anchor', () => { + service.toggle('a'); // anchor = a + service.shiftRange('c', ['a', 'b', 'c', 'd']); + expect(service.snapshot()).toEqual(new Set(['a', 'b', 'c'])); + }); + + it('shiftRange selects the inclusive backward range from the anchor', () => { + service.toggle('d'); // anchor = d + service.shiftRange('b', ['a', 'b', 'c', 'd']); + expect(service.snapshot()).toEqual(new Set(['b', 'c', 'd'])); + }); + + it('shiftRange moves the anchor to the latest key for chained ranges', () => { + service.toggle('a'); // anchor = a + service.shiftRange('c', ['a', 'b', 'c', 'd', 'e']); // anchor now c + service.shiftRange('e', ['a', 'b', 'c', 'd', 'e']); // c..e + expect(service.snapshot()).toEqual(new Set(['a', 'b', 'c', 'd', 'e'])); + }); + + it('shiftRange falls back to toggle when the anchor is no longer in the list', () => { + service.toggle('z'); // anchor = z, but z not in the list below + service.shiftRange('b', ['a', 'b', 'c']); + // z stayed checked (from its own toggle), b toggled on via fallback + expect(service.snapshot()).toEqual(new Set(['z', 'b'])); + }); + + it('shiftRange falls back to toggle when the target key is not in the list', () => { + service.toggle('a'); // anchor = a + service.shiftRange('zzz', ['a', 'b', 'c']); + // zzz not present -> fallback toggles zzz on + expect(service.snapshot()).toEqual(new Set(['a', 'zzz'])); + }); + + // --- checkAll --- + + it('checkAll checks every provided key', () => { + service.checkAll(['a', 'b', 'c']); + expect(service.snapshot()).toEqual(new Set(['a', 'b', 'c'])); + expect(latestChecked()).toEqual(new Set(['a', 'b', 'c'])); + }); + + it('checkAll unions with the existing selection', () => { + service.toggle('x'); + service.checkAll(['a', 'b']); + expect(service.snapshot()).toEqual(new Set(['x', 'a', 'b'])); + }); + + // --- uncheckAll --- + + it('uncheckAll clears the set and the anchor', () => { + service.toggle('a'); // anchor = a + service.checkAll(['b', 'c']); + service.uncheckAll(); + expect(service.snapshot().size).toBe(0); + expect(latestChecked().size).toBe(0); + + // Anchor was cleared: a subsequent shiftRange falls back to a plain toggle. + service.shiftRange('c', ['a', 'b', 'c']); + expect(service.snapshot()).toEqual(new Set(['c'])); + }); + + // --- pruneRemoved --- + + it('pruneRemoved drops removed keys, returns true, and re-emits checked$', () => { + service.checkAll(['keep', 'gone']); + const emissions: Set[] = []; + service.checked$.subscribe((s) => emissions.push(s)); + const before = emissions.length; + + const changed = service.pruneRemoved(['gone']); + + expect(changed).toBe(true); + expect(service.isChecked('gone')).toBe(false); + expect(service.isChecked('keep')).toBe(true); + expect(emissions.length).toBe(before + 1); + expect(emissions[emissions.length - 1].has('gone')).toBe(false); + }); + + it('pruneRemoved returns false and does NOT re-emit when no key was checked', () => { + service.checkAll(['keep']); + const emissions: Set[] = []; + service.checked$.subscribe((s) => emissions.push(s)); + const before = emissions.length; + + const changed = service.pruneRemoved(['never-checked']); + + expect(changed).toBe(false); + expect(emissions.length).toBe(before); // no re-emission + }); + + it('pruneRemoved does not disturb the shift anchor', () => { + service.toggle('a'); // anchor = a + service.checkAll(['b', 'c']); + service.pruneRemoved(['b']); + // anchor still 'a' -> range a..c selects a, c (b was removed but reselected in range) + service.shiftRange('c', ['a', 'b', 'c']); + expect(service.snapshot()).toEqual(new Set(['a', 'b', 'c'])); + }); + + // --- snapshot defensiveness --- + + it('snapshot returns a copy that does not mutate internal state', () => { + service.toggle('a'); + const snap = service.snapshot(); + snap.add('b'); + expect(service.isChecked('b')).toBe(false); + }); +}); diff --git a/src/angular/src/app/services/files/view-file-selection.service.ts b/src/angular/src/app/services/files/view-file-selection.service.ts new file mode 100644 index 00000000..e0346a8f --- /dev/null +++ b/src/angular/src/app/services/files/view-file-selection.service.ts @@ -0,0 +1,115 @@ +import { Injectable } from '@angular/core'; +import { BehaviorSubject, Observable } from 'rxjs'; + +/** + * Owns the multi-select "checked" state for view files: the set of checked file + * keys, the last-checked anchor for shift-range selection, and the {@link checked$} + * stream that consumers observe. + * + * This service is deliberately *input-driven*: every range-based operation takes + * the current list of file keys (in display order) as an argument rather than + * reaching back into {@link ViewFileService}. ViewFileService depends on this + * service, never the reverse — that keeps the dependency acyclic + * (`view-file-sort.service` already injects ViewFileService, so a back-reference + * here would form a DI cycle). + * + * Keys are the composite `pairId:name` keys produced by `fileKey`/`viewFileKey`. + */ +@Injectable({ providedIn: 'root' }) +export class ViewFileSelectionService { + private readonly checkedSet = new Set(); + private readonly checkedSubject = new BehaviorSubject>(new Set()); + private lastCheckedKey: string | null = null; + + readonly checked$: Observable> = this.checkedSubject.asObservable(); + + /** Whether the given key is currently checked. */ + isChecked(key: string): boolean { + return this.checkedSet.has(key); + } + + /** A defensive copy of the current checked-key set. */ + snapshot(): Set { + return new Set(this.checkedSet); + } + + /** + * Toggle a single key's checked state and make it the shift-range anchor. + * Emits the updated set on {@link checked$}. + */ + toggle(key: string): void { + if (this.checkedSet.has(key)) { + this.checkedSet.delete(key); + } else { + this.checkedSet.add(key); + } + this.lastCheckedKey = key; + this.emit(); + } + + /** + * Check every key in the contiguous range between the last anchor and the + * given key (inclusive) within {@param filteredKeys} (the display-order key + * list). Falls back to a plain {@link toggle} when there is no prior anchor or + * either endpoint is not present in the list. Updates the anchor to the given + * key. Emits the updated set on {@link checked$}. + */ + shiftRange(key: string, filteredKeys: readonly string[]): void { + if (this.lastCheckedKey == null) { + this.toggle(key); + return; + } + const lastIdx = filteredKeys.indexOf(this.lastCheckedKey); + const currIdx = filteredKeys.indexOf(key); + if (lastIdx < 0 || currIdx < 0) { + this.toggle(key); + return; + } + const start = Math.min(lastIdx, currIdx); + const end = Math.max(lastIdx, currIdx); + for (let i = start; i <= end; i++) { + this.checkedSet.add(filteredKeys[i]); + } + this.lastCheckedKey = key; + this.emit(); + } + + /** Check every key in the given list. Emits the updated set on {@link checked$}. */ + checkAll(filteredKeys: readonly string[]): void { + for (const key of filteredKeys) { + this.checkedSet.add(key); + } + this.emit(); + } + + /** Clear all checked keys and the shift anchor. Emits the (empty) set. */ + uncheckAll(): void { + this.checkedSet.clear(); + this.lastCheckedKey = null; + this.emit(); + } + + /** + * Drop every key in {@param removedKeys} from the checked set, re-emitting + * {@link checked$} only when at least one key was actually present (matching + * the original in-loop pruning guard). Does NOT touch the shift anchor. + * + * @returns whether any checked key was removed (and thus checked$ re-emitted). + */ + pruneRemoved(removedKeys: readonly string[]): boolean { + let changed = false; + for (const key of removedKeys) { + if (this.checkedSet.delete(key)) { + changed = true; + } + } + if (changed) { + this.emit(); + } + return changed; + } + + private emit(): void { + this.checkedSubject.next(new Set(this.checkedSet)); + } +} diff --git a/src/angular/src/app/services/files/view-file.service.ts b/src/angular/src/app/services/files/view-file.service.ts index 38b51ba8..cfb144fb 100644 --- a/src/angular/src/app/services/files/view-file.service.ts +++ b/src/angular/src/app/services/files/view-file.service.ts @@ -6,9 +6,11 @@ import { LoggerService } from '../utils/logger.service'; import { ModelFileService } from './model-file.service'; import { PathPairsService } from '../settings/path-pairs.service'; import { WebReaction } from '../utils/rest.service'; -import { ModelFile, ModelFileState } from '../../models/model-file'; -import { ViewFile, ViewFileStatus } from '../../models/view-file'; +import { ModelFile } from '../../models/model-file'; +import { ViewFile } from '../../models/view-file'; import { fileKey } from './file-key'; +import { mapState, deriveCapabilities } from './view-file-capabilities'; +import { ViewFileSelectionService } from './view-file-selection.service'; /** * Coalescing window (ms) for batching incremental SSE model-file emissions before @@ -41,6 +43,7 @@ export class ViewFileService { private readonly modelFileService = inject(ModelFileService); private readonly pathPairsService = inject(PathPairsService); private readonly coalesceMs = inject(VIEW_FILE_COALESCE_MS); + private readonly selection = inject(ViewFileSelectionService); private pairNameMap = new Map(); private files: ViewFile[] = []; @@ -53,13 +56,9 @@ export class ViewFileService { private filterCriteria: ViewFileFilterCriteria | null = null; private sortComparator: ViewFileComparator | null = null; - private checkedSet = new Set(); - private readonly checkedSubject = new BehaviorSubject>(new Set()); - private lastCheckedKey: string | null = null; - readonly files$: Observable = this.filesSubject.asObservable(); readonly filteredFiles$: Observable = this.filteredFilesSubject.asObservable(); - readonly checked$ = this.checkedSubject.asObservable(); + readonly checked$ = this.selection.checked$; constructor() { this.pathPairsService.pairs$.subscribe((pairs) => { @@ -178,58 +177,36 @@ export class ViewFileService { } toggleCheck(file: ViewFile): void { - const key = viewFileKey(file); - if (this.checkedSet.has(key)) { - this.checkedSet.delete(key); - } else { - this.checkedSet.add(key); - } - this.lastCheckedKey = key; + this.selection.toggle(viewFileKey(file)); this.updateCheckedState(); } shiftCheck(file: ViewFile): void { - if (this.lastCheckedKey == null) { - this.toggleCheck(file); - return; - } - const filtered = this.filteredFilesSubject.getValue(); - const lastIdx = filtered.findIndex(f => viewFileKey(f) === this.lastCheckedKey); - const currIdx = filtered.findIndex(f => viewFileKey(f) === viewFileKey(file)); - if (lastIdx < 0 || currIdx < 0) { - this.toggleCheck(file); - return; - } - const start = Math.min(lastIdx, currIdx); - const end = Math.max(lastIdx, currIdx); - for (let i = start; i <= end; i++) { - this.checkedSet.add(viewFileKey(filtered[i])); - } - this.lastCheckedKey = viewFileKey(file); + const filteredKeys = this.filteredFilesSubject.getValue().map(viewFileKey); + this.selection.shiftRange(viewFileKey(file), filteredKeys); this.updateCheckedState(); } checkAll(): void { - const filtered = this.filteredFilesSubject.getValue(); - for (const f of filtered) { - this.checkedSet.add(viewFileKey(f)); - } + const filteredKeys = this.filteredFilesSubject.getValue().map(viewFileKey); + this.selection.checkAll(filteredKeys); this.updateCheckedState(); } uncheckAll(): void { - this.checkedSet.clear(); - this.lastCheckedKey = null; + this.selection.uncheckAll(); this.updateCheckedState(); } + // Re-spread only the rows whose derived isChecked flips, preserving object + // identity for unchanged rows so OnPush/ngOnChanges can skip them. isChecked + // stays strictly derived from the selection service's checked set. The + // checked$ emission itself is owned by ViewFileSelectionService — this method + // only reconciles the diffing-owned `this.files` array and re-pushes the view. private updateCheckedState(): void { - // Only replace rows whose derived isChecked actually flips, preserving object - // identity for unchanged rows so OnPush/ngOnChanges can skip them. isChecked - // stays strictly derived from checkedSet. let changed = false; const nextFiles = this.files.map(f => { - const isChecked = this.checkedSet.has(viewFileKey(f)); + const isChecked = this.selection.isChecked(viewFileKey(f)); if (isChecked === f.isChecked) { return f; } @@ -239,7 +216,6 @@ export class ViewFileService { if (changed) { this.files = nextFiles; } - this.checkedSubject.next(new Set(this.checkedSet)); this.pushViewFiles(); } @@ -264,7 +240,7 @@ export class ViewFileService { action: (f: ViewFile) => Observable, concurrency = Infinity ): Observable { - const checked = this.files.filter(f => this.checkedSet.has(viewFileKey(f)) && filter(f)); + const checked = this.files.filter(f => this.selection.isChecked(viewFileKey(f)) && filter(f)); if (checked.length === 0) { return of([]); } @@ -331,7 +307,7 @@ export class ViewFileService { const index = this.indices.get(key)!; const oldViewFile = newViewFiles[index]; const newViewFile = createViewFile(modelFiles.get(key)!, this.pairNameMap, oldViewFile.isSelected); - newViewFiles[index] = { ...newViewFile, isChecked: this.checkedSet.has(key) }; + newViewFiles[index] = { ...newViewFile, isChecked: this.selection.isChecked(key) }; if (this.sortComparator != null && this.sortComparator(oldViewFile, newViewFile) !== 0) { reSort = true; } @@ -341,27 +317,21 @@ export class ViewFileService { for (const key of addedKeys) { reSort = true; const viewFile = createViewFile(modelFiles.get(key)!, this.pairNameMap); - newViewFiles.push({ ...viewFile, isChecked: this.checkedSet.has(key) }); + newViewFiles.push({ ...viewFile, isChecked: this.selection.isChecked(key) }); this.indices.set(viewFileKey(viewFile), newViewFiles.length - 1); } // Do the removes (no re-sort required). Filter out every removed key in a // single O(n) pass instead of findIndex+splice per key (O(n*m)). filter is // stable, so the surviving order is identical to the splice-loop result. - let checkedChanged = false; if (removedKeys.length > 0) { updateIndices = true; const removed = new Set(removedKeys); - for (const key of removedKeys) { - if (this.checkedSet.delete(key)) { - checkedChanged = true; - } - } + // Drop any checked entries for removed files; the selection service + // re-emits checked$ iff at least one was actually present. + this.selection.pruneRemoved(removedKeys); newViewFiles = newViewFiles.filter((v) => !removed.has(viewFileKey(v))); } - if (checkedChanged) { - this.checkedSubject.next(new Set(this.checkedSet)); - } if (reSort && this.sortComparator != null) { this.logger.debug('Re-sorting view files'); @@ -463,98 +433,14 @@ function createViewFile(modelFile: ModelFile, pairNameMap: Map, percentDownloaded = 100; } - let status: ViewFileStatus; - switch (modelFile.state) { - case ModelFileState.DEFAULT: - if (localSize > 0 && remoteSize > 0) { - status = ViewFileStatus.STOPPED; - } else { - status = ViewFileStatus.DEFAULT; - } - break; - case ModelFileState.QUEUED: - status = ViewFileStatus.QUEUED; - break; - case ModelFileState.DOWNLOADING: - status = ViewFileStatus.DOWNLOADING; - break; - case ModelFileState.DOWNLOADED: - status = ViewFileStatus.DOWNLOADED; - break; - case ModelFileState.DELETED: - status = ViewFileStatus.DELETED; - break; - case ModelFileState.EXTRACTING: - status = ViewFileStatus.EXTRACTING; - break; - case ModelFileState.EXTRACTED: - status = ViewFileStatus.EXTRACTED; - break; - case ModelFileState.EXTRACT_FAILED: - status = ViewFileStatus.EXTRACT_FAILED; - break; - case ModelFileState.VALIDATING: - status = ViewFileStatus.VALIDATING; - break; - case ModelFileState.VALIDATED: - status = ViewFileStatus.VALIDATED; - break; - case ModelFileState.CORRUPT: - status = ViewFileStatus.CORRUPT; - break; - default: - status = ViewFileStatus.DEFAULT; - } - - const isQueueable = - ([ViewFileStatus.DEFAULT, ViewFileStatus.STOPPED, ViewFileStatus.DELETED].includes(status) && remoteSize > 0) || - status === ViewFileStatus.CORRUPT; - const isStoppable = [ViewFileStatus.QUEUED, ViewFileStatus.DOWNLOADING].includes(status); - const isExtractable = - [ - ViewFileStatus.DEFAULT, - ViewFileStatus.STOPPED, - ViewFileStatus.DOWNLOADED, - ViewFileStatus.EXTRACTED, - ViewFileStatus.EXTRACT_FAILED, - ViewFileStatus.VALIDATED, - ViewFileStatus.CORRUPT, - ].includes(status) && localSize > 0; - const isLocallyDeletable = - [ - ViewFileStatus.DEFAULT, - ViewFileStatus.STOPPED, - ViewFileStatus.DOWNLOADED, - ViewFileStatus.EXTRACTED, - ViewFileStatus.EXTRACT_FAILED, - ViewFileStatus.VALIDATED, - ViewFileStatus.CORRUPT, - ].includes(status) && localSize > 0; - const isRemotelyDeletable = - [ - ViewFileStatus.DEFAULT, - ViewFileStatus.STOPPED, - ViewFileStatus.DOWNLOADED, - ViewFileStatus.EXTRACTED, - ViewFileStatus.EXTRACT_FAILED, - ViewFileStatus.VALIDATED, - ViewFileStatus.CORRUPT, - ViewFileStatus.DELETED, - ].includes(status) && remoteSize > 0; - const validatableStatuses = [ - ViewFileStatus.DOWNLOADED, - ViewFileStatus.EXTRACTED, - ViewFileStatus.EXTRACT_FAILED, - ViewFileStatus.VALIDATED, - ViewFileStatus.CORRUPT, - ]; - const isValidatable = - validatableStatuses.includes(status) && modelFile.local_size != null && modelFile.remote_size != null; - - let validateTooltip: string | null = null; - if (!isValidatable && validatableStatuses.includes(status) && modelFile.remote_size == null) { - validateTooltip = 'Remote file not available for checksum comparison'; - } + const status = mapState(modelFile.state, localSize, remoteSize); + const capabilities = deriveCapabilities( + status, + localSize, + remoteSize, + modelFile.local_size, + modelFile.remote_size, + ); return { name: modelFile.name, @@ -571,13 +457,7 @@ function createViewFile(modelFile: ModelFile, pairNameMap: Map, isArchive: modelFile.is_extractable, isSelected, isChecked: false, - isQueueable, - isStoppable, - isExtractable, - isLocallyDeletable, - isRemotelyDeletable, - isValidatable, - validateTooltip, + ...capabilities, localCreatedTimestamp: modelFile.local_created_timestamp, localModifiedTimestamp: modelFile.local_modified_timestamp, remoteCreatedTimestamp: modelFile.remote_created_timestamp, From de96bcc886b9d09036a41443cba0a6fb6eb94876 Mon Sep 17 00:00:00 2001 From: nitrobass24 Date: Tue, 2 Jun 2026 20:59:54 -0500 Subject: [PATCH 3/6] Refactor: decouple controller commands/persist-sync; type config options (#525) Address the four strictly behavior-preserving smells in #525: (a) Move Command/Action/ICallback, CommandProcessWrapper, and MAX_CONCURRENT_COMMAND_PROCESSES into controller/commands.py. Controller re-exports them as class attributes (identity preserved), and CommandPipeline imports them directly, removing the TYPE_CHECKING + lazy 'from .controller import Controller' imports and the controller_cls plumbing threaded through dispatch. (b) Extract sync_persist into a PersistSync collaborator (controller/persist_sync.py) injected into both CommandPipeline and ModelUpdater at construction, removing the placeholder lambda and post-hoc re-patch in Controller.__init__. ModelUpdater keeps a thin sync_persist_to_all_builders() delegating to PersistSync.sync(). (c) Move conditional-disable metadata (disabledWhen/overrideNote) onto the option definitions in options-list.ts; the component applies one generic applyDisableRules transform. Static wrappers retained so existing specs pass unchanged. (d) Type config access: ConfigValuePath is derived from the Config model, IOption.valuePath is typed against it, and the 'as unknown as' casts in config.service.ts and settings-page.component.ts are removed via a typed getConfigValue accessor and config section index signatures. (e) Documented the Angular mutating-service result/error contract in CLAUDE.md and confirmed IntegrationsService conforms; the ConfigService/AutoQueueService tap migration is explicitly deferred (not behavior-preserving against unchanged specs). New focused tests: test_commands.py, test_persist_sync.py, options-list.spec.ts, settings-page.disable-rules.spec.ts. All safety-net suites pass unchanged. Co-Authored-By: Claude Opus 4.8 (1M context) --- CLAUDE.md | 11 +++ src/angular/src/app/models/config.ts | 32 +++++-- .../app/pages/settings/options-list.spec.ts | 27 ++++++ .../src/app/pages/settings/options-list.ts | 45 ++++++++- .../pages/settings/settings-page.component.ts | 68 ++++++++------ .../settings-page.disable-rules.spec.ts | 55 +++++++++++ .../app/services/settings/config.service.ts | 20 ++-- src/python/controller/command_pipeline.py | 84 +++++++---------- src/python/controller/commands.py | 71 ++++++++++++++ src/python/controller/controller.py | 77 ++++----------- src/python/controller/model_updater.py | 51 ++-------- src/python/controller/persist_sync.py | 70 ++++++++++++++ .../test_controller/test_commands.py | 78 ++++++++++++++++ .../test_controller/test_persist_sync.py | 93 +++++++++++++++++++ 14 files changed, 581 insertions(+), 201 deletions(-) create mode 100644 src/angular/src/app/pages/settings/options-list.spec.ts create mode 100644 src/angular/src/app/pages/settings/settings-page.disable-rules.spec.ts create mode 100644 src/python/controller/commands.py create mode 100644 src/python/controller/persist_sync.py create mode 100644 src/python/tests/unittests/test_controller/test_commands.py create mode 100644 src/python/tests/unittests/test_controller/test_persist_sync.py diff --git a/CLAUDE.md b/CLAUDE.md index 5836532c..286f484f 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -276,6 +276,17 @@ When you see these, open an issue for the extraction rather than working around - **Mechanical checks** (Python): ruff's `C901` (cyclomatic complexity) with `max-complexity = 12`. Enforced in CI; existing outliers are annotated with `# noqa: C901`. Test files (`tests/**`) are excluded via `per-file-ignores` in `pyproject.toml`. - **Ratchet pattern**: when a bound exists but the codebase has outliers, set the threshold just above the worst and drop it over time (same pattern we use for `--max-warnings` in `ng lint`). +### Angular mutating-service contract + +Services that mutate a `BehaviorSubject` from a REST/HTTP call should follow **one** documented contract so callers can reason about side effects: + +- **Update the subject inside the returned observable's pipeline (`tap`/`map`), not via a second internal `.subscribe()`.** A second internal subscribe fires the side effect independently of the caller, runs the request a second time when the source is cold, and makes "when does the store update?" caller-independent and surprising. Folding the mutation into the returned pipeline means the store updates exactly when (and as many times as) the caller subscribes. +- **Return a typed result and recover errors with `catchError`** so the caller always gets a value (or a typed failure) rather than an error notification. Map HTTP responses to a small typed result/`WebReaction`. + +`IntegrationsService` (`src/angular/src/app/services/settings/integrations.service.ts`) is the reference implementation: `create`/`update`/`remove`/`test` use `tap`/`map` to update `instancesSubject` and `catchError` to map failures to a typed result, with no second subscribe. + +**Known deviation (tracked):** `ConfigService.set` and `AutoQueueService.add/remove` still use the legacy `const obs = sendRequest(url); obs.subscribe({next: ...mutate...}); return obs;` pattern (a second internal subscribe). Migrating them to `tap` is **deferred**: their unit specs call `set()`/`add()`/`remove()` *without* subscribing and assert the `BehaviorSubject` mutated, which the current caller-independent subscribe makes pass. Because `RestService.sendRequest` uses `shareReplay(1)`, switching to `tap` defers the mutation to caller subscription and is **not** behavior-preserving against those unchanged tests. The migration must ship in its own PR that also updates those specs to subscribe. + ## GitHub Repository - **Repo**: github.com/nitrobass24/seedsync diff --git a/src/angular/src/app/models/config.ts b/src/angular/src/app/models/config.ts index 933c7cec..1d1b9d1b 100644 --- a/src/angular/src/app/models/config.ts +++ b/src/angular/src/app/models/config.ts @@ -3,13 +3,24 @@ * Note: Naming convention matches that used in the JSON. */ -export interface General { +/** + * The value any single config field may hold. Every section field is one of + * these, which lets a section be read as a string-keyed record for the + * dynamic section/option access used by ConfigService.set without an + * `as unknown as` cast. + */ +export type ConfigValue = string | number | boolean | null; + +/** Common shape of every config section: a string-keyed bag of ConfigValues. */ +export type ConfigSection = Record; + +export interface General extends ConfigSection { log_level: string | null; verbose: boolean | null; exclude_patterns: string | null; } -export interface Lftp { +export interface Lftp extends ConfigSection { remote_address: string | null; remote_username: string | null; remote_password: string | null; @@ -34,7 +45,7 @@ export interface Lftp { net_reconnect_interval_multiplier: number | null; } -export interface Controller { +export interface Controller extends ConfigSection { interval_ms_remote_scan: number | null; interval_ms_local_scan: number | null; interval_ms_downloading_scan: number | null; @@ -44,23 +55,23 @@ export interface Controller { use_staging: boolean | null; } -export interface Web { +export interface Web extends ConfigSection { port: number | null; api_key: string | null; } -export interface AutoQueue { +export interface AutoQueue extends ConfigSection { enabled: boolean | null; patterns_only: boolean | null; auto_extract: boolean | null; auto_delete_remote: boolean | null; } -export interface Logging { +export interface Logging extends ConfigSection { log_format: string | null; } -export interface Notifications { +export interface Notifications extends ConfigSection { webhook_url: string | null; notify_on_download_start: boolean | null; notify_on_download_complete: boolean | null; @@ -72,7 +83,7 @@ export interface Notifications { telegram_chat_id: string | null; } -export interface Validate { +export interface Validate extends ConfigSection { enabled: boolean | null; algorithm: string | null; auto_validate: boolean | null; @@ -82,7 +93,10 @@ export interface Validate { /** Sentinel value the backend uses to mask sensitive fields in API responses. */ export const REDACTED_SENTINEL = '********'; -export interface Config { +// Index signature lets a Config be read as a record of sections keyed by a +// runtime string (used by ConfigService.set), without an `as unknown as` cast. +// Each section already satisfies ConfigSection, so this is type-accurate. +export interface Config extends Record { general: General; lftp: Lftp; controller: Controller; diff --git a/src/angular/src/app/pages/settings/options-list.spec.ts b/src/angular/src/app/pages/settings/options-list.spec.ts new file mode 100644 index 00000000..680a6bbf --- /dev/null +++ b/src/angular/src/app/pages/settings/options-list.spec.ts @@ -0,0 +1,27 @@ +import { describe, it, expect } from 'vitest'; +import { getConfigValue } from './options-list'; +import { Config, DEFAULT_CONFIG } from '../../models/config'; + +describe('getConfigValue', () => { + const config: Config = { + ...DEFAULT_CONFIG, + lftp: { ...DEFAULT_CONFIG.lftp, remote_path: '/remote', remote_port: 22 }, + web: { ...DEFAULT_CONFIG.web, api_key: 'secret' }, + }; + + it('reads a string field by typed path', () => { + expect(getConfigValue(config, ['lftp', 'remote_path'])).toBe('/remote'); + }); + + it('reads a numeric field by typed path', () => { + expect(getConfigValue(config, ['lftp', 'remote_port'])).toBe(22); + }); + + it('reads a field from another section', () => { + expect(getConfigValue(config, ['web', 'api_key'])).toBe('secret'); + }); + + it('returns null for a field whose value is null', () => { + expect(getConfigValue(config, ['lftp', 'local_path'])).toBeNull(); + }); +}); diff --git a/src/angular/src/app/pages/settings/options-list.ts b/src/angular/src/app/pages/settings/options-list.ts index 53caf498..bf8863cf 100644 --- a/src/angular/src/app/pages/settings/options-list.ts +++ b/src/angular/src/app/pages/settings/options-list.ts @@ -1,13 +1,46 @@ -import { OptionType } from './option.component'; +import { Config } from '../../models/config'; +import { OptionType, OptionValue } from './option.component'; + +/** + * A [section, option] path into the Config model. The union over Config's + * sections keeps each tuple's second element constrained to keys of that + * section, so a typo or a renamed field is a compile error rather than a + * silently-broken magic string. + */ +export type ConfigValuePath = { + [S in keyof Config]: [S, keyof Config[S]]; +}[keyof Config]; + +/** + * Conditions under which an option is rendered disabled. The component maps + * each flag to a runtime predicate; co-locating the condition with the option + * definition keeps the disable rules next to the options they affect. + */ +export type OptionDisabledWhen = 'pairsEnabled' | 'validateDisabled'; export interface IOption { type: OptionType; label: string; - valuePath: [string, string]; + valuePath: ConfigValuePath; description: string | null; disabled?: boolean; choices?: string[]; requiresRestart?: boolean; + /** When set, the option is disabled while the named condition holds. */ + disabledWhen?: OptionDisabledWhen; + /** Description shown in place of the default while disabledWhen is active. */ + overrideNote?: string; +} + +/** Note shown on options that Path Pairs overrides once any pair is enabled. */ +export const OVERRIDE_NOTE = 'Overridden by Path Pairs when any pair is enabled'; + +/** Read a config value by its typed [section, option] path. */ +export function getConfigValue(config: Config, path: ConfigValuePath): OptionValue { + const [section, option] = path; + const sectionObj = config[section]; + if (!sectionObj) return null; + return sectionObj[option as string] ?? null; } export interface IOptionsContext { @@ -54,6 +87,8 @@ export const OPTIONS_CONTEXT_SERVER: IOptionsContext = { valuePath: ['lftp', 'remote_path'], description: 'Path to your files on the remote server', requiresRestart: true, + disabledWhen: 'pairsEnabled', + overrideNote: OVERRIDE_NOTE, }, { type: OptionType.Text, @@ -61,6 +96,8 @@ export const OPTIONS_CONTEXT_SERVER: IOptionsContext = { valuePath: ['lftp', 'local_path'], description: 'Downloaded files are placed here', requiresRestart: true, + disabledWhen: 'pairsEnabled', + overrideNote: OVERRIDE_NOTE, }, { type: OptionType.Text, @@ -215,6 +252,8 @@ export const OPTIONS_CONTEXT_AUTOQUEUE: IOptionsContext = { valuePath: ['autoqueue', 'enabled'], description: null, requiresRestart: true, + disabledWhen: 'pairsEnabled', + overrideNote: OVERRIDE_NOTE, }, { type: OptionType.Checkbox, @@ -441,6 +480,7 @@ export const OPTIONS_CONTEXT_VALIDATE: IOptionsContext = { label: 'Auto-validate after download', valuePath: ['validate', 'auto_validate'], description: 'Automatically validate files when download completes. Requires post-download validation above.', + disabledWhen: 'validateDisabled', }, { type: OptionType.Select, @@ -448,6 +488,7 @@ export const OPTIONS_CONTEXT_VALIDATE: IOptionsContext = { valuePath: ['validate', 'algorithm'], description: 'Checksum algorithm used for both inline transfer verification and post-download validation', choices: ['md5', 'sha1', 'sha256'], + disabledWhen: 'validateDisabled', }, ], }; diff --git a/src/angular/src/app/pages/settings/settings-page.component.ts b/src/angular/src/app/pages/settings/settings-page.component.ts index 2363b8c2..8910a518 100644 --- a/src/angular/src/app/pages/settings/settings-page.component.ts +++ b/src/angular/src/app/pages/settings/settings-page.component.ts @@ -18,7 +18,10 @@ import { OptionComponent, OptionValue } from './option.component'; import { PathPairsComponent } from './path-pairs.component'; import { IntegrationsComponent } from './integrations.component'; import { + ConfigValuePath, IOptionsContext, + OVERRIDE_NOTE, + getConfigValue, OPTIONS_CONTEXT_SERVER, OPTIONS_CONTEXT_DISCOVERY, OPTIONS_CONTEXT_CONNECTIONS, @@ -86,7 +89,9 @@ export class SettingsPageComponent implements OnInit { ); private badValueNotifs = new Map(); - private static readonly OVERRIDE_NOTE = 'Overridden by Path Pairs when any pair is enabled'; + // Retained for the unit spec, which reads this via a statics cast. The + // canonical note now lives next to the option definitions in options-list.ts. + private static readonly OVERRIDE_NOTE = OVERRIDE_NOTE; ngOnInit(): void { this.connectedService.connected$.pipe( @@ -121,47 +126,54 @@ export class SettingsPageComponent implements OnInit { }); } - private static buildServerContext(hasEnabledPairs: boolean): IOptionsContext { + /** + * Apply the per-option disable rules carried by each option definition + * (disabledWhen/overrideNote). One generic transform replaces the former + * per-context builders: an option is disabled when its disabledWhen flag is + * currently active, and its description is swapped for overrideNote when one + * is provided. + */ + private static applyDisableRules( + context: IOptionsContext, + active: { pairsEnabled: boolean; validateDisabled: boolean }, + ): IOptionsContext { return { - ...OPTIONS_CONTEXT_SERVER, - options: OPTIONS_CONTEXT_SERVER.options.map((option) => { - if (hasEnabledPairs && (option.valuePath[1] === 'remote_path' || option.valuePath[1] === 'local_path')) { - return { ...option, description: SettingsPageComponent.OVERRIDE_NOTE, disabled: true }; + ...context, + options: context.options.map((option) => { + if (option.disabledWhen && active[option.disabledWhen]) { + return option.overrideNote !== undefined + ? { ...option, disabled: true, description: option.overrideNote } + : { ...option, disabled: true }; } return option; }), }; } + private static buildServerContext(hasEnabledPairs: boolean): IOptionsContext { + return SettingsPageComponent.applyDisableRules(OPTIONS_CONTEXT_SERVER, { + pairsEnabled: hasEnabledPairs, + validateDisabled: false, + }); + } + private static buildAutoqueueContext(hasEnabledPairs: boolean): IOptionsContext { - return { - ...OPTIONS_CONTEXT_AUTOQUEUE, - options: OPTIONS_CONTEXT_AUTOQUEUE.options.map((option) => { - if (hasEnabledPairs && option.valuePath[1] === 'enabled') { - return { ...option, description: SettingsPageComponent.OVERRIDE_NOTE, disabled: true }; - } - return option; - }), - }; + return SettingsPageComponent.applyDisableRules(OPTIONS_CONTEXT_AUTOQUEUE, { + pairsEnabled: hasEnabledPairs, + validateDisabled: false, + }); } private static buildValidateContext(validateEnabled: boolean): IOptionsContext { - return { - ...OPTIONS_CONTEXT_VALIDATE, - options: OPTIONS_CONTEXT_VALIDATE.options.map((option) => { - if (!validateEnabled && (option.valuePath[1] === 'auto_validate' || option.valuePath[1] === 'algorithm')) { - return { ...option, disabled: true }; - } - return option; - }), - }; + return SettingsPageComponent.applyDisableRules(OPTIONS_CONTEXT_VALIDATE, { + pairsEnabled: false, + validateDisabled: !validateEnabled, + }); } - getOptionValue(config: Config | null, valuePath: [string, string]): OptionValue { + getOptionValue(config: Config | null, valuePath: ConfigValuePath): OptionValue { if (!config) return null; - const section = (config as unknown as Record | undefined>)[valuePath[0]]; - if (!section) return null; - return section[valuePath[1]] ?? null; + return getConfigValue(config, valuePath); } onSetConfig(section: string, option: string, value: OptionValue, requiresRestart?: boolean): void { diff --git a/src/angular/src/app/pages/settings/settings-page.disable-rules.spec.ts b/src/angular/src/app/pages/settings/settings-page.disable-rules.spec.ts new file mode 100644 index 00000000..8cb68e70 --- /dev/null +++ b/src/angular/src/app/pages/settings/settings-page.disable-rules.spec.ts @@ -0,0 +1,55 @@ +import '@angular/compiler'; +import { describe, it, expect } from 'vitest'; +import { SettingsPageComponent } from './settings-page.component'; +import { IOptionsContext } from './options-list'; + +// buildValidateContext is a private static reached the same way the existing +// settings-page spec reaches buildServerContext/buildAutoqueueContext. This +// focused spec covers the validate ('validateDisabled') branch of the generic +// applyDisableRules transform, which the existing spec does not exercise. +interface ValidateStatics { + buildValidateContext(validateEnabled: boolean): IOptionsContext; +} +const statics = SettingsPageComponent as unknown as ValidateStatics; +const buildValidateContext = (enabled: boolean): IOptionsContext => statics.buildValidateContext(enabled); + +describe('SettingsPageComponent.buildValidateContext', () => { + it('disables auto_validate and algorithm when validation is disabled', () => { + const ctx = buildValidateContext(false); + const autoValidate = ctx.options.find((o) => o.valuePath[1] === 'auto_validate')!; + const algorithm = ctx.options.find((o) => o.valuePath[1] === 'algorithm')!; + + expect(autoValidate.disabled).toBe(true); + expect(algorithm.disabled).toBe(true); + }); + + it('keeps the original description on disabled validate options (no override note)', () => { + const ctx = buildValidateContext(false); + const autoValidate = ctx.options.find((o) => o.valuePath[1] === 'auto_validate')!; + + // validateDisabled options carry no overrideNote, so the description is unchanged. + expect(autoValidate.description).toBe( + 'Automatically validate files when download completes. Requires post-download validation above.', + ); + }); + + it('does not disable auto_validate and algorithm when validation is enabled', () => { + const ctx = buildValidateContext(true); + const autoValidate = ctx.options.find((o) => o.valuePath[1] === 'auto_validate')!; + const algorithm = ctx.options.find((o) => o.valuePath[1] === 'algorithm')!; + + expect(autoValidate.disabled).toBeFalsy(); + expect(algorithm.disabled).toBeFalsy(); + }); + + it('never disables the other validate options', () => { + const ctx = buildValidateContext(false); + const others = ctx.options.filter( + (o) => o.valuePath[1] !== 'auto_validate' && o.valuePath[1] !== 'algorithm', + ); + + for (const option of others) { + expect(option.disabled).toBeFalsy(); + } + }); +}); diff --git a/src/angular/src/app/services/settings/config.service.ts b/src/angular/src/app/services/settings/config.service.ts index eb51db6c..c954caf9 100644 --- a/src/angular/src/app/services/settings/config.service.ts +++ b/src/angular/src/app/services/settings/config.service.ts @@ -5,13 +5,17 @@ import { ConnectedService } from '../utils/connected.service'; import { LoggerService } from '../utils/logger.service'; import { RestService, WebReaction } from '../utils/rest.service'; import { StreamDispatchService } from '../base/stream-dispatch.service'; -import { Config, REDACTED_SENTINEL } from '../../models/config'; +import { Config, ConfigSection, ConfigValue, REDACTED_SENTINEL } from '../../models/config'; -/** Value a config field may take (matches OptionComponent's value shape). */ -export type ConfigValue = string | number | boolean | null; +// ConfigValue is re-exported for callers that import it from this service. +export type { ConfigValue }; -/** Config treated as a string-keyed record for dynamic section/option access. */ -type ConfigRecord = Record>; +/** + * Config viewed as a record of string-keyed sections, for dynamic + * section/option access by runtime strings. Config carries a matching index + * signature, so it is assignable to this with no cast. + */ +type ConfigRecord = Record; /** Sentinel value sent to the backend when the user clears a text field. */ export const EMPTY_VALUE_SENTINEL = '__empty__'; @@ -60,8 +64,8 @@ export class ConfigService { set(section: string, option: string, value: ConfigValue): Observable { const valueStr = String(value ?? ''); const currentConfig = this.configSubject.getValue(); - const configRecord = currentConfig as unknown as ConfigRecord; - if (!currentConfig || !(section in currentConfig) || !(option in configRecord[section])) { + const configRecord: ConfigRecord | null = currentConfig; + if (!currentConfig || !configRecord || !(section in currentConfig) || !(option in configRecord[section])) { return of({ success: false, data: null, @@ -79,7 +83,7 @@ export class ConfigService { if (reaction.success) { const config = this.configSubject.getValue(); if (config) { - const configRecord = config as unknown as ConfigRecord; + const configRecord: ConfigRecord = config; const newConfig = { ...config, [section]: { ...configRecord[section], [option]: value } }; this.configSubject.next(newConfig); // Propagate API key changes to the SSE stream immediately. set() is diff --git a/src/python/controller/command_pipeline.py b/src/python/controller/command_pipeline.py index f3b25c98..c802977f 100644 --- a/src/python/controller/command_pipeline.py +++ b/src/python/controller/command_pipeline.py @@ -13,15 +13,12 @@ import os from collections.abc import Callable from queue import Queue -from typing import TYPE_CHECKING - -if TYPE_CHECKING: - from .controller import Controller from common import AppError, AppProcess, Context, MultiprocessingLogger from lftp import LftpError from model import ModelError, ModelFile +from .commands import MAX_CONCURRENT_COMMAND_PROCESSES, Command, CommandProcessWrapper from .controller_persist import ControllerPersist from .delete import DeleteLocalProcess, DeleteRemoteProcess from .exclude_patterns import parse_exclude_patterns @@ -65,10 +62,10 @@ def __init__( self.sync_persist_callback = sync_persist_callback # The command queue - self.command_queue: Queue[Controller.Command] = Queue() + self.command_queue: Queue[Command] = Queue() # Keep track of active command processes (shared) - self.active_command_processes: list[Controller.CommandProcessWrapper] = [] + self.active_command_processes: list[CommandProcessWrapper] = [] # Keep track of active move processes (staging -> final, shared) self.active_move_processes: list[MoveProcess] = [] @@ -82,25 +79,19 @@ def __init__( # validate worker is surfaced once at ERROR rather than every cycle. self.__reported_dead_workers: set[int] = set() - def queue(self, command: Controller.Command) -> None: + def queue(self, command: Command) -> None: """Put a command on the queue for processing.""" self.command_queue.put(command) def step(self): - """Process commands from queue. - - References Controller.Command, Controller.Command.Action, - Controller.MAX_CONCURRENT_COMMAND_PROCESSES, and - Controller.CommandProcessWrapper which remain in Controller. - """ - from .controller import Controller + """Process commands from queue.""" - def _notify_failure(_command: Controller.Command, _msg: str): + def _notify_failure(_command: Command, _msg: str): self._logger.warning(f"Command failed. {_msg}") for _callback in _command.callbacks: _callback.on_failure(_msg) - deferred: list[Controller.Command] = [] + deferred: list[Command] = [] while not self.command_queue.empty(): command = self.command_queue.get() @@ -117,7 +108,7 @@ def _notify_failure(_command: Controller.Command, _msg: str): _notify_failure(command, f"File '{command.filename}' not found") continue - success = self._dispatch_command(command, file, pc, deferred, _notify_failure, Controller) + success = self._dispatch_command(command, file, pc, deferred, _notify_failure) if not success: continue @@ -130,25 +121,20 @@ def _notify_failure(_command: Controller.Command, _msg: str): def _dispatch_command( self, - command: Controller.Command, + command: Command, file: ModelFile, pc: PairContext, - deferred: list[Controller.Command], - _notify_failure: Callable[[Controller.Command, str], None], - controller_cls: type[Controller], + deferred: list[Command], + _notify_failure: Callable[[Command, str], None], ) -> bool: """Dispatch a command to the appropriate handler. Returns True on success.""" - Action = controller_cls.Command.Action + Action = Command.Action handlers = { Action.QUEUE: lambda: self._handle_queue(command, file, pc, _notify_failure), Action.STOP: lambda: self._handle_stop(command, file, pc, _notify_failure), Action.EXTRACT: lambda: self._handle_extract(command, file, pc, _notify_failure), - Action.DELETE_LOCAL: lambda: self._handle_delete_local( - command, file, pc, deferred, _notify_failure, controller_cls - ), - Action.DELETE_REMOTE: lambda: self._handle_delete_remote( - command, file, pc, deferred, _notify_failure, controller_cls - ), + Action.DELETE_LOCAL: lambda: self._handle_delete_local(command, file, pc, deferred, _notify_failure), + Action.DELETE_REMOTE: lambda: self._handle_delete_remote(command, file, pc, deferred, _notify_failure), Action.VALIDATE: lambda: self._handle_validate(command, file, pc, _notify_failure), } handler = handlers.get(command.action) @@ -158,10 +144,10 @@ def _dispatch_command( def _handle_queue( self, - command: Controller.Command, + command: Command, file: ModelFile, pc: PairContext, - _notify_failure: Callable[[Controller.Command, str], None], + _notify_failure: Callable[[Command, str], None], ) -> bool: """Handle the QUEUE action. Returns True on success, False on failure.""" if file.remote_size is None: @@ -177,10 +163,10 @@ def _handle_queue( def _handle_stop( self, - command: Controller.Command, + command: Command, file: ModelFile, pc: PairContext, - _notify_failure: Callable[[Controller.Command, str], None], + _notify_failure: Callable[[Command, str], None], ) -> bool: """Handle the STOP action. Returns True on success, False on failure.""" if file.state not in (ModelFile.State.DOWNLOADING, ModelFile.State.QUEUED): @@ -195,10 +181,10 @@ def _handle_stop( def _handle_extract( self, - command: Controller.Command, + command: Command, file: ModelFile, pc: PairContext, - _notify_failure: Callable[[Controller.Command, str], None], + _notify_failure: Callable[[Command, str], None], ) -> bool: """Handle the EXTRACT action. Returns True on success, False on failure.""" if file.state not in ( @@ -223,15 +209,14 @@ def _handle_extract( def _handle_delete_local( self, - command: Controller.Command, + command: Command, file: ModelFile, pc: PairContext, - deferred: list[Controller.Command], - _notify_failure: Callable[[Controller.Command, str], None], - controller_cls: type[Controller], + deferred: list[Command], + _notify_failure: Callable[[Command, str], None], ) -> bool: """Handle the DELETE_LOCAL action. Returns True on success, False on failure/deferred.""" - if len(self.active_command_processes) >= controller_cls.MAX_CONCURRENT_COMMAND_PROCESSES: + if len(self.active_command_processes) >= MAX_CONCURRENT_COMMAND_PROCESSES: self._logger.debug( "Deferring %s for '%s': %d active processes at cap", command.action, @@ -270,22 +255,21 @@ def post_callback(delete_path: str = delete_path, _pc: PairContext = pc) -> None if delete_path != _pc.local_path: _pc.active_scan_process.force_scan() - command_wrapper = controller_cls.CommandProcessWrapper(process=process, post_callback=post_callback) + command_wrapper = CommandProcessWrapper(process=process, post_callback=post_callback) self.active_command_processes.append(command_wrapper) command_wrapper.process.start() return True def _handle_delete_remote( self, - command: Controller.Command, + command: Command, file: ModelFile, pc: PairContext, - deferred: list[Controller.Command], - _notify_failure: Callable[[Controller.Command, str], None], - controller_cls: type[Controller], + deferred: list[Command], + _notify_failure: Callable[[Command, str], None], ) -> bool: """Handle the DELETE_REMOTE action. Returns True on success, False on failure/deferred.""" - if len(self.active_command_processes) >= controller_cls.MAX_CONCURRENT_COMMAND_PROCESSES: + if len(self.active_command_processes) >= MAX_CONCURRENT_COMMAND_PROCESSES: self._logger.debug( "Deferring %s for '%s': %d active processes at cap", command.action, @@ -320,7 +304,7 @@ def _handle_delete_remote( file_name=file.name, ) process.set_mp_log_queue(self._mp_logger.queue, self._mp_logger.log_level) - command_wrapper = controller_cls.CommandProcessWrapper( + command_wrapper = CommandProcessWrapper( process=process, post_callback=pc.remote_scan_process.force_scan ) self.active_command_processes.append(command_wrapper) @@ -329,10 +313,10 @@ def _handle_delete_remote( def _handle_validate( self, - command: Controller.Command, + command: Command, file: ModelFile, pc: PairContext, - _notify_failure: Callable[[Controller.Command, str], None], + _notify_failure: Callable[[Command, str], None], ) -> bool: """Handle the VALIDATE action. Returns True on success, False on failure.""" if not self._context.config.validate.enabled: @@ -378,7 +362,7 @@ def cleanup(self): Cleanup the list of active commands and do any callbacks :return: """ - still_active_processes: list[Controller.CommandProcessWrapper] = [] + still_active_processes: list[CommandProcessWrapper] = [] for command_process in self.active_command_processes: if command_process.process.is_alive(): still_active_processes.append(command_process) @@ -539,7 +523,7 @@ def spawn_move_process(self, file_name: str, pc: PairContext): self.active_move_processes.append(process) self._logger.info(f"Spawned move process for {file_name} (staging -> local)") - def _get_pair_context_for_command(self, command: Controller.Command) -> PairContext | None: + def _get_pair_context_for_command(self, command: Command) -> PairContext | None: """Find the pair context for a command based on pair_id.""" return self.find_pair_by_id(command.pair_id) diff --git a/src/python/controller/commands.py b/src/python/controller/commands.py new file mode 100644 index 00000000..d3474e18 --- /dev/null +++ b/src/python/controller/commands.py @@ -0,0 +1,71 @@ +# Copyright 2017, Inderpreet Singh, All rights reserved. + +"""Command types shared between Controller and CommandPipeline. + +Extracted from controller.py to break the bidirectional Controller <-> +CommandPipeline coupling. Both modules import these symbols directly from +here; Controller additionally re-exports them as class attributes so that +existing `Controller.Command`, `Controller.CommandProcessWrapper`, and +`Controller.MAX_CONCURRENT_COMMAND_PROCESSES` references keep resolving to +the same objects (identity preserved). +""" + +from __future__ import annotations + +from abc import ABC, abstractmethod +from collections.abc import Callable +from enum import Enum + +from common import AppOneShotProcess + + +class Command: + """ + Class by which clients of Controller can request Actions to be executed + Supports callbacks by which clients can be notified of action success/failure + Note: callbacks will be executed in Controller thread, so any heavy computation + should be moved out of the callback + """ + + class Action(Enum): + QUEUE = 0 + STOP = 1 + EXTRACT = 2 + DELETE_LOCAL = 3 + DELETE_REMOTE = 4 + VALIDATE = 5 + + class ICallback(ABC): + """Command callback interface""" + + @abstractmethod + def on_success(self): + """Called on successful completion of action""" + pass + + @abstractmethod + def on_failure(self, error: str): + """Called on action failure""" + pass + + def __init__(self, action: Action, filename: str, pair_id: str | None = None): + self.action = action + self.filename = filename + self.pair_id = pair_id + self.callbacks: list[Command.ICallback] = [] + + def add_callback(self, callback: ICallback): + self.callbacks.append(callback) + + +class CommandProcessWrapper: + """ + Wraps any one-shot command processes launched by the controller + """ + + def __init__(self, process: AppOneShotProcess, post_callback: Callable[[], None]): + self.process = process + self.post_callback = post_callback + + +MAX_CONCURRENT_COMMAND_PROCESSES = 8 diff --git a/src/python/controller/controller.py b/src/python/controller/controller.py index d513284f..a146d309 100644 --- a/src/python/controller/controller.py +++ b/src/python/controller/controller.py @@ -4,19 +4,14 @@ import os import threading -from abc import ABC, abstractmethod from collections.abc import Callable, Iterator -from enum import Enum -from typing import TYPE_CHECKING -if TYPE_CHECKING: - pass - -from common import AppOneShotProcess, AppProcess, Constants, Context, MultiprocessingLogger +from common import AppProcess, Constants, Context, MultiprocessingLogger from lftp import Lftp from model import IModelListener, Model, ModelFile from .command_pipeline import CommandPipeline +from .commands import MAX_CONCURRENT_COMMAND_PROCESSES, Command, CommandProcessWrapper from .controller_persist import ControllerPersist # my libs @@ -25,6 +20,7 @@ from .model_registry import ModelRegistry from .model_updater import ModelUpdater from .pair_context import ControllerError, PairContext, configure_lftp, validate_config +from .persist_sync import PersistSync from .scan import ActiveScanner, LocalScanner, RemoteScanner, ScannerProcess from .validate import ValidateProcess @@ -34,54 +30,12 @@ class Controller: Top-level class that controls the behaviour of the app """ - class Command: - """ - Class by which clients of Controller can request Actions to be executed - Supports callbacks by which clients can be notified of action success/failure - Note: callbacks will be executed in Controller thread, so any heavy computation - should be moved out of the callback - """ - - class Action(Enum): - QUEUE = 0 - STOP = 1 - EXTRACT = 2 - DELETE_LOCAL = 3 - DELETE_REMOTE = 4 - VALIDATE = 5 - - class ICallback(ABC): - """Command callback interface""" - - @abstractmethod - def on_success(self): - """Called on successful completion of action""" - pass - - @abstractmethod - def on_failure(self, error: str): - """Called on action failure""" - pass - - def __init__(self, action: Action, filename: str, pair_id: str | None = None): - self.action = action - self.filename = filename - self.pair_id = pair_id - self.callbacks: list[Controller.Command.ICallback] = [] - - def add_callback(self, callback: ICallback): - self.callbacks.append(callback) - - class CommandProcessWrapper: - """ - Wraps any one-shot command processes launched by the controller - """ - - def __init__(self, process: AppOneShotProcess, post_callback: Callable[[], None]): - self.process = process - self.post_callback = post_callback - - MAX_CONCURRENT_COMMAND_PROCESSES = 8 + # Re-exported from .commands so existing Controller.Command, + # Controller.CommandProcessWrapper, and Controller.MAX_CONCURRENT_COMMAND_PROCESSES + # references resolve to the same objects shared with CommandPipeline. + Command = Command + CommandProcessWrapper = CommandProcessWrapper + MAX_CONCURRENT_COMMAND_PROCESSES = MAX_CONCURRENT_COMMAND_PROCESSES def __init__(self, context: Context, persist: ControllerPersist): self.__context = context @@ -113,9 +67,11 @@ def __init__(self, context: Context, persist: ControllerPersist): self.__validate_process = ValidateProcess() self.__validate_process.set_mp_log_queue(self.__mp_logger.queue, self.__mp_logger.log_level) + # Persist sync is a standalone collaborator so the pipeline and updater + # share the exact same instance with no construction-order placeholder. + self.__persist_sync = PersistSync(self.__pair_contexts, self.__persist) + # Command pipeline owns the queue, active processes, and move state. - # Use a lambda placeholder for sync_persist_callback; it will be - # replaced once the ModelUpdater is created (chicken-and-egg). self.__pipeline = CommandPipeline( pair_contexts=self.__pair_contexts, registry=self.__registry, @@ -126,7 +82,7 @@ def __init__(self, context: Context, persist: ControllerPersist): extract_process=self.__extract_process, validate_process=self.__validate_process, logger=self.logger, - sync_persist_callback=lambda: None, + sync_persist_callback=self.__persist_sync.sync, ) # Model updater owns the per-cycle update loop @@ -140,12 +96,11 @@ def __init__(self, context: Context, persist: ControllerPersist): context=self.__context, password=self.__password, logger=self.logger, + persist_sync=self.__persist_sync, ) - # Now wire the real callback into the pipeline - self.__pipeline.sync_persist_callback = self.__updater.sync_persist_to_all_builders # Seed each builder with filtered persist state - self.__updater.sync_persist_to_all_builders() + self.__persist_sync.sync() # Flag for hot-reloading LFTP tuning settings (set from REST thread) self.__needs_lftp_reconfigure = threading.Event() diff --git a/src/python/controller/model_updater.py b/src/python/controller/model_updater.py index db4da647..51799723 100644 --- a/src/python/controller/model_updater.py +++ b/src/python/controller/model_updater.py @@ -22,7 +22,8 @@ from .extract import ExtractCompletedResult, ExtractFailedResult, ExtractProcess, ExtractStatus, ExtractStatusResult from .model_registry import ModelRegistry from .pair_context import PairContext -from .persist_keys import KEY_SEP, persist_key, strip_persist_key +from .persist_keys import persist_key, strip_persist_key +from .persist_sync import PersistSync from .validate import ( ValidateCompletedResult, ValidateFailedResult, @@ -50,6 +51,7 @@ def __init__( context: Context, password: str | None, logger: logging.Logger, + persist_sync: PersistSync | None = None, ): self._pair_contexts = pair_contexts self._persist = persist @@ -60,6 +62,10 @@ def __init__( self._context = context self._password = password self._logger = logger + # Share the same PersistSync instance with the CommandPipeline when one + # is injected; otherwise build one over the same pair_contexts/persist so + # standalone use (and unit tests) behaves identically. + self._persist_sync = persist_sync or PersistSync(pair_contexts, persist) def update(self) -> None: # Grab the latest extract results (shared) @@ -437,45 +443,4 @@ def _detect_lftp_completions(self, pc: PairContext, lftp_statuses: list[LftpJobS def sync_persist_to_all_builders(self): """Push current persist state to all pair model builders, filtered by pair_id.""" - namespaced_prefixes = tuple( - f"{other_pc.pair_id}{sep}" for other_pc in self._pair_contexts if other_pc.pair_id for sep in (KEY_SEP, ":") - ) - for pc in self._pair_contexts: - pc.model_builder.set_downloaded_files( - self._filter_keys_for_pair(self._persist.downloaded_file_names, pc.pair_id, namespaced_prefixes) - ) - pc.model_builder.set_extracted_files( - self._filter_keys_for_pair(self._persist.extracted_file_names, pc.pair_id, namespaced_prefixes) - ) - pc.model_builder.set_extract_failed_files( - self._filter_keys_for_pair(self._persist.extract_failed_file_names, pc.pair_id, namespaced_prefixes) - ) - pc.model_builder.set_validated_files( - self._filter_keys_for_pair(self._persist.validated_file_names, pc.pair_id, namespaced_prefixes) - ) - pc.model_builder.set_corrupt_files( - self._filter_keys_for_pair(self._persist.corrupt_file_names, pc.pair_id, namespaced_prefixes) - ) - - @staticmethod - def _filter_keys_for_pair(keys: set[str], pair_id: str | None, namespaced_prefixes: tuple[str, ...]) -> set[str]: - """Filter and strip persist keys that belong to a specific pair. - - For pairs with a pair_id, matches keys with the current separator or - legacy colon prefix. For the default pair (pair_id=None), matches keys - that don't start with any other pair's prefix. - """ - result: set[str] = set() - if pair_id: - prefix = f"{pair_id}{KEY_SEP}" - legacy_prefix = f"{pair_id}:" - for key in keys: - if key.startswith(prefix): - result.add(key[len(prefix) :]) - elif key.startswith(legacy_prefix): - result.add(key[len(legacy_prefix) :]) - else: - for key in keys: - if not key.startswith(namespaced_prefixes): - result.add(key) - return result + self._persist_sync.sync() diff --git a/src/python/controller/persist_sync.py b/src/python/controller/persist_sync.py new file mode 100644 index 00000000..04000e5f --- /dev/null +++ b/src/python/controller/persist_sync.py @@ -0,0 +1,70 @@ +# Copyright 2017, Inderpreet Singh, All rights reserved. + +"""Persist-state synchronization to per-pair model builders. + +Extracted from ModelUpdater so the same collaborator can be injected into +both CommandPipeline (which clears persist entries on extract/validate) and +ModelUpdater (which pushes persist state every update cycle), removing the +construction-order placeholder lambda that previously stood in for the +callback. The logic is moved verbatim — this is a structural extraction, +not a refactor. +""" + +from __future__ import annotations + +from .controller_persist import ControllerPersist +from .pair_context import PairContext +from .persist_keys import KEY_SEP + + +class PersistSync: + """Push current persist state to all pair model builders, filtered by pair_id.""" + + def __init__(self, pair_contexts: list[PairContext], persist: ControllerPersist): + self._pair_contexts = pair_contexts + self._persist = persist + + def sync(self): + """Push current persist state to all pair model builders, filtered by pair_id.""" + namespaced_prefixes = tuple( + f"{other_pc.pair_id}{sep}" for other_pc in self._pair_contexts if other_pc.pair_id for sep in (KEY_SEP, ":") + ) + for pc in self._pair_contexts: + pc.model_builder.set_downloaded_files( + self._filter_keys_for_pair(self._persist.downloaded_file_names, pc.pair_id, namespaced_prefixes) + ) + pc.model_builder.set_extracted_files( + self._filter_keys_for_pair(self._persist.extracted_file_names, pc.pair_id, namespaced_prefixes) + ) + pc.model_builder.set_extract_failed_files( + self._filter_keys_for_pair(self._persist.extract_failed_file_names, pc.pair_id, namespaced_prefixes) + ) + pc.model_builder.set_validated_files( + self._filter_keys_for_pair(self._persist.validated_file_names, pc.pair_id, namespaced_prefixes) + ) + pc.model_builder.set_corrupt_files( + self._filter_keys_for_pair(self._persist.corrupt_file_names, pc.pair_id, namespaced_prefixes) + ) + + @staticmethod + def _filter_keys_for_pair(keys: set[str], pair_id: str | None, namespaced_prefixes: tuple[str, ...]) -> set[str]: + """Filter and strip persist keys that belong to a specific pair. + + For pairs with a pair_id, matches keys with the current separator or + legacy colon prefix. For the default pair (pair_id=None), matches keys + that don't start with any other pair's prefix. + """ + result: set[str] = set() + if pair_id: + prefix = f"{pair_id}{KEY_SEP}" + legacy_prefix = f"{pair_id}:" + for key in keys: + if key.startswith(prefix): + result.add(key[len(prefix) :]) + elif key.startswith(legacy_prefix): + result.add(key[len(legacy_prefix) :]) + else: + for key in keys: + if not key.startswith(namespaced_prefixes): + result.add(key) + return result diff --git a/src/python/tests/unittests/test_controller/test_commands.py b/src/python/tests/unittests/test_controller/test_commands.py new file mode 100644 index 00000000..8cea5465 --- /dev/null +++ b/src/python/tests/unittests/test_controller/test_commands.py @@ -0,0 +1,78 @@ +# Copyright 2017, Inderpreet Singh, All rights reserved. + +import unittest +from unittest.mock import MagicMock + +from controller import commands +from controller.commands import MAX_CONCURRENT_COMMAND_PROCESSES, Command, CommandProcessWrapper +from controller.controller import Controller + + +class TestCommand(unittest.TestCase): + def test_action_members_and_values(self): + # The wire/dispatch contract depends on these exact members and values. + self.assertEqual(0, Command.Action.QUEUE.value) + self.assertEqual(1, Command.Action.STOP.value) + self.assertEqual(2, Command.Action.EXTRACT.value) + self.assertEqual(3, Command.Action.DELETE_LOCAL.value) + self.assertEqual(4, Command.Action.DELETE_REMOTE.value) + self.assertEqual(5, Command.Action.VALIDATE.value) + self.assertEqual(6, len(list(Command.Action))) + + def test_command_init_defaults(self): + command = Command(Command.Action.QUEUE, "file.txt") + self.assertEqual(Command.Action.QUEUE, command.action) + self.assertEqual("file.txt", command.filename) + self.assertIsNone(command.pair_id) + self.assertEqual([], command.callbacks) + + def test_command_init_with_pair_id(self): + command = Command(Command.Action.STOP, "file.txt", pair_id="abc") + self.assertEqual("abc", command.pair_id) + + def test_add_callback_appends(self): + command = Command(Command.Action.QUEUE, "file.txt") + cb1 = MagicMock() + cb2 = MagicMock() + command.add_callback(cb1) + command.add_callback(cb2) + self.assertEqual([cb1, cb2], command.callbacks) + + def test_icallback_is_abstract(self): + with self.assertRaises(TypeError): + Command.ICallback() # type: ignore[abstract] + + +class TestCommandProcessWrapper(unittest.TestCase): + def test_stores_process_and_callback(self): + process = MagicMock() + post_callback = MagicMock() + wrapper = CommandProcessWrapper(process=process, post_callback=post_callback) + self.assertIs(process, wrapper.process) + self.assertIs(post_callback, wrapper.post_callback) + + +class TestConcurrencyCap(unittest.TestCase): + def test_max_concurrent_command_processes(self): + self.assertEqual(8, MAX_CONCURRENT_COMMAND_PROCESSES) + + +class TestControllerReExportIdentity(unittest.TestCase): + """Lock the re-export: Controller.* must be the SAME objects as commands.*. + + Every Controller.Command / .CommandProcessWrapper / + .MAX_CONCURRENT_COMMAND_PROCESSES reference in tests, auto_queue.py, and the + web handler resolves through these attributes — identity is the contract. + """ + + def test_controller_command_is_commands_command(self): + self.assertIs(commands.Command, Controller.Command) + + def test_controller_process_wrapper_is_commands_wrapper(self): + self.assertIs(commands.CommandProcessWrapper, Controller.CommandProcessWrapper) + + def test_controller_cap_matches(self): + self.assertEqual(commands.MAX_CONCURRENT_COMMAND_PROCESSES, Controller.MAX_CONCURRENT_COMMAND_PROCESSES) + + def test_controller_action_is_commands_action(self): + self.assertIs(commands.Command.Action, Controller.Command.Action) diff --git a/src/python/tests/unittests/test_controller/test_persist_sync.py b/src/python/tests/unittests/test_controller/test_persist_sync.py new file mode 100644 index 00000000..aab26624 --- /dev/null +++ b/src/python/tests/unittests/test_controller/test_persist_sync.py @@ -0,0 +1,93 @@ +# Copyright 2017, Inderpreet Singh, All rights reserved. + +import unittest +from unittest.mock import MagicMock + +from controller.persist_keys import KEY_SEP +from controller.persist_sync import PersistSync + + +class TestPersistSync(unittest.TestCase): + """Mirrors the assertions in test_model_updater to prove the extracted + PersistSync.sync() runs the identical pair-filtering logic.""" + + def _make_pair_context(self, pair_id): + pc = MagicMock() + pc.pair_id = pair_id + return pc + + def _make_persist(self, downloaded=None, extracted=None, extract_failed=None, validated=None, corrupt=None): + persist = MagicMock() + persist.downloaded_file_names = downloaded or set() + persist.extracted_file_names = extracted or set() + persist.extract_failed_file_names = extract_failed or set() + persist.validated_file_names = validated or set() + persist.corrupt_file_names = corrupt or set() + return persist + + def test_filters_keys_by_pair_id_prefix(self): + pc_abc = self._make_pair_context("abc") + pc_xyz = self._make_pair_context("xyz") + persist = self._make_persist( + downloaded={f"abc{KEY_SEP}movie.mkv", f"xyz{KEY_SEP}show.avi"}, + extracted={f"abc{KEY_SEP}movie.mkv"}, + ) + + PersistSync([pc_abc, pc_xyz], persist).sync() + + pc_abc.model_builder.set_downloaded_files.assert_called_once_with({"movie.mkv"}) + pc_abc.model_builder.set_extracted_files.assert_called_once_with({"movie.mkv"}) + pc_xyz.model_builder.set_downloaded_files.assert_called_once_with({"show.avi"}) + pc_xyz.model_builder.set_extracted_files.assert_called_once_with(set()) + + def test_none_pair_id_gets_unprefixed_keys(self): + pc_default = self._make_pair_context(None) + pc_abc = self._make_pair_context("abc") + persist = self._make_persist(downloaded={"plain_file.txt", f"abc{KEY_SEP}namespaced.mkv"}) + + PersistSync([pc_default, pc_abc], persist).sync() + + pc_default.model_builder.set_downloaded_files.assert_called_once_with({"plain_file.txt"}) + pc_abc.model_builder.set_downloaded_files.assert_called_once_with({"namespaced.mkv"}) + + def test_handles_legacy_colon_separator_keys(self): + pc_abc = self._make_pair_context("abc") + persist = self._make_persist(downloaded={"abc:legacy_file.mkv"}) + + PersistSync([pc_abc], persist).sync() + + pc_abc.model_builder.set_downloaded_files.assert_called_once_with({"legacy_file.mkv"}) + + def test_default_pair_excludes_legacy_colon_keys(self): + pc_default = self._make_pair_context(None) + pc_abc = self._make_pair_context("abc") + persist = self._make_persist(downloaded={"abc:legacy.mkv", "plain.txt"}) + + PersistSync([pc_default, pc_abc], persist).sync() + + pc_default.model_builder.set_downloaded_files.assert_called_once_with({"plain.txt"}) + pc_abc.model_builder.set_downloaded_files.assert_called_once_with({"legacy.mkv"}) + + def test_all_persist_categories_are_distributed(self): + pc_abc = self._make_pair_context("abc") + persist = self._make_persist( + downloaded={f"abc{KEY_SEP}file.mkv"}, + extracted={f"abc{KEY_SEP}file.mkv"}, + extract_failed={f"abc{KEY_SEP}bad.zip"}, + validated={f"abc{KEY_SEP}good.mkv"}, + corrupt={f"abc{KEY_SEP}corrupt.mkv"}, + ) + + PersistSync([pc_abc], persist).sync() + + pc_abc.model_builder.set_downloaded_files.assert_called_once_with({"file.mkv"}) + pc_abc.model_builder.set_extracted_files.assert_called_once_with({"file.mkv"}) + pc_abc.model_builder.set_extract_failed_files.assert_called_once_with({"bad.zip"}) + pc_abc.model_builder.set_validated_files.assert_called_once_with({"good.mkv"}) + pc_abc.model_builder.set_corrupt_files.assert_called_once_with({"corrupt.mkv"}) + + def test_filter_keys_for_pair_static(self): + result = PersistSync._filter_keys_for_pair( + {f"abc{KEY_SEP}a.mkv", f"xyz{KEY_SEP}b.mkv"}, "abc", (f"xyz{KEY_SEP}", "xyz:") + ) + self.assertEqual({"a.mkv"}, result) From 293936af266ecea788ea34e89166bf56c58c1855 Mon Sep 17 00:00:00 2001 From: nitrobass24 Date: Tue, 2 Jun 2026 21:10:45 -0500 Subject: [PATCH 4/6] Type-annotate job_status_parser helpers for strict pyright (#523) The #523 decomposition added helpers without parameter type annotations; CI's strict pyright (Python Type Check) requires them. Annotate the regex-match params (re.Match[str]) and add an assert where the caller guarantees a non-None match. No behavior change; 100 parser tests still pass. Co-Authored-By: Claude Opus 4.8 (1M context) --- src/python/lftp/job_status_parser.py | 19 +++++++++++++------ 1 file changed, 13 insertions(+), 6 deletions(-) diff --git a/src/python/lftp/job_status_parser.py b/src/python/lftp/job_status_parser.py index 253cd1f3..477cb2eb 100644 --- a/src/python/lftp/job_status_parser.py +++ b/src/python/lftp/job_status_parser.py @@ -283,7 +283,11 @@ def _is_valid_first_line(line: str, prev_job: "LftpJobStatus | None") -> bool: ) @staticmethod - def _build_chunk_transfer_state(result_at, result_at2, result_got) -> "LftpJobStatus.TransferState": + def _build_chunk_transfer_state( + result_at: "re.Match[str] | None", + result_at2: "re.Match[str] | None", + result_got: "re.Match[str] | None", + ) -> "LftpJobStatus.TransferState": """Build a TransferState from whichever of the three chunk-data regex matches is present. Caller is responsible for the name-mismatch checks (which differ between the pget header and the filename chunk).""" @@ -297,7 +301,8 @@ def _build_chunk_transfer_state(result_at, result_at2, result_got) -> "LftpJobSt return LftpJobStatus.TransferState(None, None, None, speed, eta) if result_at2: return LftpJobStatus.TransferState(None, None, None, None, None) - # result_got + # result_got (one of the three is guaranteed non-None by the caller) + assert result_got is not None size_local = int(result_got.group("szlocal")) size_remote = int(result_got.group("szremote")) percent_local = int(result_got.group("pctlocal")) @@ -309,7 +314,7 @@ def _build_chunk_transfer_state(result_at, result_at2, result_got) -> "LftpJobSt eta = LftpJobStatusParser._eta_to_seconds(result_got.group("eta")) return LftpJobStatus.TransferState(size_local, size_remote, percent_local, speed, eta) - def _parse_pget_header_block(self, result, lines: list[str]) -> LftpJobStatus: + def _parse_pget_header_block(self, result: "re.Match[str]", lines: list[str]) -> LftpJobStatus: """Parse a pget header line (already matched) plus its mandatory 'sftp' line and optional data line into a RUNNING pget LftpJobStatus.""" # Next line must be the sftp line @@ -358,7 +363,7 @@ def _parse_pget_header_block(self, result, lines: list[str]) -> LftpJobStatus: return status @staticmethod - def _parse_mirror_header(result) -> LftpJobStatus: + def _parse_mirror_header(result: "re.Match[str]") -> LftpJobStatus: """Parse a downloading mirror header line (already matched) into a RUNNING mirror LftpJobStatus with size/speed totals.""" id_ = int(result.group("id")) @@ -383,7 +388,7 @@ def _parse_mirror_header(result) -> LftpJobStatus: return status @staticmethod - def _parse_mirror_fl_header(result, lines: list[str]) -> LftpJobStatus: + def _parse_mirror_fl_header(result: "re.Match[str]", lines: list[str]) -> LftpJobStatus: """Parse a connecting / receiving-file-list mirror header (already matched) into a RUNNING mirror LftpJobStatus, popping the optional 'Getting file list'/'cd ' follow-up line.""" @@ -397,7 +402,9 @@ def _parse_mirror_fl_header(result, lines: list[str]) -> LftpJobStatus: job_id=id_, job_type=LftpJobStatus.Type.MIRROR, state=LftpJobStatus.State.RUNNING, name=name, flags=flags ) - def _parse_filename_chunk(self, result, lines: list[str], prev_job: "LftpJobStatus | None") -> None: + def _parse_filename_chunk( + self, result: "re.Match[str]", lines: list[str], prev_job: "LftpJobStatus | None" + ) -> None: """Parse a '\\transfer `name'' line (already matched) plus its following chunk-data line, registering the active file transfer state on ``prev_job``.""" From 542ab511ecbc080fa6df47599e064da746b185d8 Mon Sep 17 00:00:00 2001 From: nitrobass24 Date: Tue, 2 Jun 2026 21:18:21 -0500 Subject: [PATCH 5/6] Make config types actually typo-safe; add missing remote_python_path (#525 review) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Review found #525's AC#4 "removed the casts" by adding index signatures (Config extends Record<...>, sections extends ConfigSection), which collapses keyof to string and lets typos compile — a type-safety regression, the opposite of the AC. Remove the index signatures so Config/sections keep their specific keys and ConfigValuePath becomes a real typo-checked discriminated union; keep a single localized record cast at the genuinely-dynamic access sites (ConfigService.set, getConfigValue). This immediately surfaced a latent bug the index signature was masking: the options list references ['lftp','remote_python_path'] (a real backend key, config.py:268) but the Lftp model never declared it. Added the field + default. Co-Authored-By: Claude Opus 4.8 (1M context) --- src/angular/src/app/models/config.ts | 37 +++++++++---------- .../src/app/pages/settings/options-list.ts | 6 ++- .../services/settings/config.service.spec.ts | 1 + .../app/services/settings/config.service.ts | 6 ++- 4 files changed, 27 insertions(+), 23 deletions(-) diff --git a/src/angular/src/app/models/config.ts b/src/angular/src/app/models/config.ts index 1d1b9d1b..f48176a5 100644 --- a/src/angular/src/app/models/config.ts +++ b/src/angular/src/app/models/config.ts @@ -3,24 +3,24 @@ * Note: Naming convention matches that used in the JSON. */ -/** - * The value any single config field may hold. Every section field is one of - * these, which lets a section be read as a string-keyed record for the - * dynamic section/option access used by ConfigService.set without an - * `as unknown as` cast. - */ +/** The value any single config field may hold. */ export type ConfigValue = string | number | boolean | null; -/** Common shape of every config section: a string-keyed bag of ConfigValues. */ +/** + * A config section read as a string-keyed bag of ConfigValues. Sections do NOT + * `extends` this (that would collapse `keyof Section` to `string` and defeat the + * typo-catching ConfigValuePath in options-list.ts); it is only used as an + * explicit, localized cast target at the few dynamic section/option access sites. + */ export type ConfigSection = Record; -export interface General extends ConfigSection { +export interface General { log_level: string | null; verbose: boolean | null; exclude_patterns: string | null; } -export interface Lftp extends ConfigSection { +export interface Lftp { remote_address: string | null; remote_username: string | null; remote_password: string | null; @@ -28,6 +28,7 @@ export interface Lftp extends ConfigSection { remote_path: string | null; local_path: string | null; remote_path_to_scan_script: string | null; + remote_python_path: string | null; use_ssh_key: boolean | null; num_max_parallel_downloads: number | null; num_max_parallel_files_per_download: number | null; @@ -45,7 +46,7 @@ export interface Lftp extends ConfigSection { net_reconnect_interval_multiplier: number | null; } -export interface Controller extends ConfigSection { +export interface Controller { interval_ms_remote_scan: number | null; interval_ms_local_scan: number | null; interval_ms_downloading_scan: number | null; @@ -55,23 +56,23 @@ export interface Controller extends ConfigSection { use_staging: boolean | null; } -export interface Web extends ConfigSection { +export interface Web { port: number | null; api_key: string | null; } -export interface AutoQueue extends ConfigSection { +export interface AutoQueue { enabled: boolean | null; patterns_only: boolean | null; auto_extract: boolean | null; auto_delete_remote: boolean | null; } -export interface Logging extends ConfigSection { +export interface Logging { log_format: string | null; } -export interface Notifications extends ConfigSection { +export interface Notifications { webhook_url: string | null; notify_on_download_start: boolean | null; notify_on_download_complete: boolean | null; @@ -83,7 +84,7 @@ export interface Notifications extends ConfigSection { telegram_chat_id: string | null; } -export interface Validate extends ConfigSection { +export interface Validate { enabled: boolean | null; algorithm: string | null; auto_validate: boolean | null; @@ -93,10 +94,7 @@ export interface Validate extends ConfigSection { /** Sentinel value the backend uses to mask sensitive fields in API responses. */ export const REDACTED_SENTINEL = '********'; -// Index signature lets a Config be read as a record of sections keyed by a -// runtime string (used by ConfigService.set), without an `as unknown as` cast. -// Each section already satisfies ConfigSection, so this is type-accurate. -export interface Config extends Record { +export interface Config { general: General; lftp: Lftp; controller: Controller; @@ -121,6 +119,7 @@ export const DEFAULT_LFTP: Lftp = { remote_path: null, local_path: null, remote_path_to_scan_script: null, + remote_python_path: null, use_ssh_key: null, num_max_parallel_downloads: null, num_max_parallel_files_per_download: null, diff --git a/src/angular/src/app/pages/settings/options-list.ts b/src/angular/src/app/pages/settings/options-list.ts index bf8863cf..e5ad5f12 100644 --- a/src/angular/src/app/pages/settings/options-list.ts +++ b/src/angular/src/app/pages/settings/options-list.ts @@ -1,4 +1,4 @@ -import { Config } from '../../models/config'; +import { Config, ConfigSection } from '../../models/config'; import { OptionType, OptionValue } from './option.component'; /** @@ -38,7 +38,9 @@ export const OVERRIDE_NOTE = 'Overridden by Path Pairs when any pair is enabled' /** Read a config value by its typed [section, option] path. */ export function getConfigValue(config: Config, path: ConfigValuePath): OptionValue { const [section, option] = path; - const sectionObj = config[section]; + // path is a typo-checked ConfigValuePath; the read itself is dynamic, so the + // string-indexed section access is localized here through ConfigSection. + const sectionObj = config[section] as unknown as ConfigSection; if (!sectionObj) return null; return sectionObj[option as string] ?? null; } diff --git a/src/angular/src/app/services/settings/config.service.spec.ts b/src/angular/src/app/services/settings/config.service.spec.ts index 044b33d5..cb85f535 100644 --- a/src/angular/src/app/services/settings/config.service.spec.ts +++ b/src/angular/src/app/services/settings/config.service.spec.ts @@ -21,6 +21,7 @@ function makeConfig(overrides: Partial = {}): Config { remote_path: "/remote", local_path: "/local", remote_path_to_scan_script: null, + remote_python_path: null, use_ssh_key: false, num_max_parallel_downloads: 1, num_max_parallel_files_per_download: 1, diff --git a/src/angular/src/app/services/settings/config.service.ts b/src/angular/src/app/services/settings/config.service.ts index c954caf9..ddcdce07 100644 --- a/src/angular/src/app/services/settings/config.service.ts +++ b/src/angular/src/app/services/settings/config.service.ts @@ -64,7 +64,9 @@ export class ConfigService { set(section: string, option: string, value: ConfigValue): Observable { const valueStr = String(value ?? ''); const currentConfig = this.configSubject.getValue(); - const configRecord: ConfigRecord | null = currentConfig; + // Dynamic section/option access by runtime string: localize the record cast + // here (the typed Config keeps its specific keys for static callers). + const configRecord = currentConfig as unknown as ConfigRecord | null; if (!currentConfig || !configRecord || !(section in currentConfig) || !(option in configRecord[section])) { return of({ success: false, @@ -83,7 +85,7 @@ export class ConfigService { if (reaction.success) { const config = this.configSubject.getValue(); if (config) { - const configRecord: ConfigRecord = config; + const configRecord = config as unknown as ConfigRecord; const newConfig = { ...config, [section]: { ...configRecord[section], [option]: value } }; this.configSubject.next(newConfig); // Propagate API key changes to the SSE stream immediately. set() is From 00046d8ce5ef2449d0c8afceacf3e013fd9664b9 Mon Sep 17 00:00:00 2001 From: nitrobass24 Date: Tue, 2 Jun 2026 21:20:35 -0500 Subject: [PATCH 6/6] Apply ruff format to command_pipeline.py (#525) The #525 edit left command_pipeline.py not ruff-format clean (CI's Python Lint runs ruff format --check). Format-only; tests unchanged. Co-Authored-By: Claude Opus 4.8 (1M context) --- src/python/controller/command_pipeline.py | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/src/python/controller/command_pipeline.py b/src/python/controller/command_pipeline.py index c802977f..99be7b66 100644 --- a/src/python/controller/command_pipeline.py +++ b/src/python/controller/command_pipeline.py @@ -304,9 +304,7 @@ def _handle_delete_remote( file_name=file.name, ) process.set_mp_log_queue(self._mp_logger.queue, self._mp_logger.log_level) - command_wrapper = CommandProcessWrapper( - process=process, post_callback=pc.remote_scan_process.force_scan - ) + command_wrapper = CommandProcessWrapper(process=process, post_callback=pc.remote_scan_process.force_scan) self.active_command_processes.append(command_wrapper) command_wrapper.process.start() return True