diff --git a/CHANGELOG.md b/CHANGELOG.md index 5a61c69..fbc0b28 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,5 +1,39 @@ # Changelog +## 0.3.2 + +- `mpat check` lists only the targets that are not `ok` and ends with an + `N ok` count, so a CI log shows the drift instead of burying it in a table + of every watched symbol. `--all` restores the full table. `--json`, + `--gitlab` and the exit code are unchanged. +- A target deleted upstream is reported as `missing`. Before, `patch()` and + `watch()` resolved their target while `mpat check` was collecting, so the + deletion aborted the run with exit 2, no table and an empty GitLab report. + Outside collection a missing target still raises `TargetNotFound`. +- Rows with a changed signature, value, location or kind list the change under + them, e.g. `value_repr: 16 -> 2`, and the GitLab description carries the + same text. `--json` gains a `changes` list. +- GitLab Code Quality findings point at the line of the `patch()` or `watch()` + call instead of the first line quoting the target, which could be an + unrelated string. `--json` gains `declared_line`. +- A change confined to docstrings reports the new `docstring` status instead of + `body`, with its own footer line and `minor` GitLab severity. It still exits 1. + The lock gains a `code_hash` field for this; the first `mpat lock` after the + upgrade lists every function and class as changed (`~`) while it records it, + and until then a docstring change keeps reporting `body`. +- `mpat diff` and `mpat check --diff` show how a drifted target's upstream + source changed, as a diff in the style of a `ty` diagnostic, with the locked + and installed line numbers side by side. The lock holds hashes, not source, + so the locked side comes from the release archive on the package index + (`UV_DEFAULT_INDEX`, `UV_INDEX_URL`, `PIP_INDEX_URL`, else PyPI), cached in + `MPAT_CACHE_DIR`. Index credentials never follow a redirect to another + host. A fetch failure prints why and leaves the exit code alone. `mpat diff` + without a target covers every drifted target, and now imports the + configured modules like `mpat check`. +- A target used by several declarations, such as a shared `depends_on`, lists + the others under its row and in `--json` (`used_by`), and GitLab gets one + finding per declaration, so every patch that depends on the drift is marked. + ## 0.3.1 - A watch can name a file shipped inside an installed package: diff --git a/README.md b/README.md index 4f35d05..da16457 100644 --- a/README.md +++ b/README.md @@ -120,11 +120,14 @@ next to it. `mpat.toml` wins when both exist. ``` mpat lock # fingerprint everything, write mpat.lock, commit it -mpat check # exit 1 unless every target is ok +mpat check # exit 1 unless every target is ok; lists only the ones that are not +mpat check --all # list ok targets too +mpat check --diff # show each drifted target's upstream source change mpat check --json mpat check --gitlab # GitLab Code Quality report mpat show somelib.client.Client.request mpat diff somelib.client.Client.request +mpat diff # every drifted target ``` `mpat lock` and `mpat check` import the modules listed in `[tool.mpat] modules` @@ -181,7 +184,8 @@ no_source = false An entry whose `role` or `parent` no longer matches the declaration is reported `unlocked`: the audit trail changed meaning, so it has to be re-locked. -`mpat check` reports one row per target with one of these statuses: +`mpat check` reports one row per target with one of these statuses, and +counts the `ok` ones instead of listing them unless given `--all`: | Status | Meaning | | --- | --- | @@ -189,6 +193,7 @@ An entry whose `role` or `parent` no longer matches the declaration is reported | `moved` | The target now resolves elsewhere, or its source file changed. | | `signature` | The signature changed, or the target switched between sync and async. | | `body` | The source hash changed. | +| `docstring` | The source hash changed, but only in docstrings. | | `value` | A watched constant's value changed. | | `no_source` | The locked source is no longer readable, so only the signature is still comparable. | | `missing` | The target no longer exists upstream. | diff --git a/docs/site/pages/cli.md b/docs/site/pages/cli.md index ff6627a..64aca58 100644 --- a/docs/site/pages/cli.md +++ b/docs/site/pages/cli.md @@ -49,16 +49,32 @@ mpat: somelib._speedups.encode: no source available, only the signature is locke ## `mpat check` Compares `mpat.lock` against the installed upstream and prints one row per -target. This is the command for CI and for dependency-bump merge requests. +target that is not `ok`. This is the command for CI and for dependency-bump +merge requests. ``` $ mpat check # somelib body patch somelib.client.Client.request myapp/patches.py 2.3.1 -> 2.5.0 Works around somelib#123. Delete when fixed. -ok depends_on somelib.client.Client._send myapp/patches.py 2.3.1 -> 2.5.0 Works around somelib#123. Delete when fixed. value watch somelib.settings.MAX_RETRIES myapp/patches.py 2.3.1 -> 2.5.0 see somelib#98 2 drifted: read the upstream change, then fix or delete the patch and run 'mpat lock' +1 ok (--all lists them) +``` + +`ok` targets are only counted, so a clean run prints a single line. `--all` +lists them in the table too. + +A target used by more than one declaration lists the others under its row, as +`used by: myapp/retry.py:40 (patch somelib.retry.backoff)`, so a shared +dependency that drifted points at every patch that needs a look. + +`--diff` adds the [`mpat diff`](#mpat-diff) block under each drifted row. It +downloads the locked release, so it is opt-in; add it to the CI job to get the +upstream change in the log: + +```yaml +- uv run mpat check --diff ``` The columns are status, role, target, the file the declaration came from, the @@ -78,6 +94,7 @@ With no declarations at all it prints `no declarations found` and exits 0. | `moved` | The target now resolves elsewhere, or its source file changed. | | `signature` | The signature changed, or the target switched between sync and async. | | `body` | The source hash changed. | +| `docstring` | The source hash changed, but only in docstrings. | | `content` | A watched file's bytes changed. | | `value` | A watched constant's value changed. | | `no_source` | The locked source is no longer readable, so only the signature is still comparable. | @@ -88,8 +105,19 @@ With no declarations at all it prints `no declarations found` and exits 0. Drift beats `review`: a target that both drifted and is overdue reports the drift. +A row with a changed signature, value, location or kind lists the change under +it, so the table says what moved without a separate `mpat diff`: + +``` +signature patch somelib.client.Client.request myapp/patches.py 2.3.1 -> 2.5.0 + signature: (self, method, url, **kwargs) -> (self, method, url, *, timeout, **kwargs) +``` + +Hash changes are not listed; `body` and `content` already say that much, and +`mpat diff TARGET` shows the hashes. + A row whose target currently has no readable source is marked, whatever its -status: +status, including `ok` rows listed with `--all`: ``` ok [signature-only] watch somelib._speedups.encode myapp/patches.py @@ -106,13 +134,17 @@ lines with a non-zero count are printed. ``` 1 unlocked: run 'mpat lock' 2 drifted: read the upstream change, then fix or delete the patch and run 'mpat lock' +1 docstring-only: skim the upstream docs change, then run 'mpat lock' 1 stale: run 'mpat lock' to prune 1 due for review +3 ok (--all lists them) ``` `unlocked` and `stale` mean the lockfile is out of date with your code, so the answer is `mpat lock`. `drifted` means upstream changed under you, so the answer -is to read the upstream change first. +is to read the upstream change first. `docstring` still fails the check, because +a docstring edit can announce a behaviour change, but it is counted apart from +the drift that needs a code review. ### `--json` @@ -128,7 +160,10 @@ $ mpat check --json "locked_version": "2.3.1", "current_version": "2.5.0", "no_source": false, - "dist": "somelib" + "dist": "somelib", + "declared_line": 12, + "changes": [], + "used_by": [] } ] ``` @@ -159,7 +194,12 @@ $ mpat check --gitlab ``` `missing` and `moved` are `blocker`, the fingerprint mismatches and `unlocked` -are `major`, `no_source` and `review` are `minor`, `stale` is `info`. The +are `major`, `no_source`, `docstring` and `review` are `minor`, `stale` is +`info`. The description ends with the same changed fields the table lists. +Every other declaration that uses a drifted target gets its own issue at its own +line, so the merge request marks each patch that depends on it. +`lines.begin` is the line of the `patch()` or `watch()` call, or for a +configuration table and a `stale` entry the first line quoting the target. The fingerprint is a hash of the path, role and target, so the same drift keeps the same identity between runs and GitLab does not report it twice. @@ -217,22 +257,50 @@ or against the version the lockfile recorded. ## `mpat diff` -Prints the drift status of one locked target, then every fingerprint field whose -locked value differs from the installed one. It reads `mpat.lock` and resolves -the target; it does not import `[tool.mpat] modules`. +Prints the drift status of one locked target, every fingerprint field whose +locked value differs from the installed one, and then how its upstream source +changed. ``` $ mpat diff somelib.client.Client.request -status: signature -signature: (self, method, url, **kwargs) -> (self, method, url, *, timeout=None, **kwargs) +status: body source_hash: sha256:0684635… -> sha256:9b1c7ee… dist_version: 2.3.1 -> 2.5.0 + +body[patch] somelib.client.Client.request: the upstream body changed (2.3.1 -> 2.5.0) + --> somelib/client.py:92 + | +88 90 | def request(self, method, url, **kwargs): +89 91 | self.log(method) +90 | - return self._send(method, url, **kwargs) + 92 | + return self._send(method, url, timeout=self.timeout, **kwargs) + | +info: declared here + --> myapp/patches.py:12 +info: also used by myapp/retry.py:40 (patch somelib.retry.backoff) ``` -`ok` prints only the status line. A target that no longer exists prints -`status: missing` and every locked field against `(missing)`. The lock records -hashes, not source, so a `body` change shows the two hashes; read the upstream -diff between the two `dist_version` values for the actual change. +The two gutter columns are the line numbers in the locked and the installed +file. `mpat diff` with no target prints the block for every drifted target. + +The lock records hashes, not source, so the locked side is downloaded: the +archive of the locked `dist_version` of the target's distribution, a wheel if +there is one and the sdist otherwise. The index is the first of +`UV_DEFAULT_INDEX`, `UV_INDEX_URL` and `PIP_INDEX_URL` that is set, else PyPI. +Credentials in the index URL, or in `~/.netrc` for its host, are sent to that +host only, never to a host a download redirects to. Archives are cached in +`MPAT_CACHE_DIR`, default `~/.cache/mpat`; point it at a CI cache path to keep +them across jobs. The downloaded definition is hashed again, and a line warns +when it does not match the lock, which happens when a release was rebuilt or +ships per-platform code. + +Only `body`, `content`, `docstring`, `signature`, `moved` and `missing` get a +block; `missing` and `moved` show the locked definition as removed. When the +archive cannot be fetched the block says `info: upstream diff unavailable` and +why, and the exit code does not change. + +`mpat diff` imports `[tool.mpat] modules` like `mpat check`, so the block knows +where the target is declared and what else uses it. Exits 0 on `ok`, 1 on any other status, 2 when the target is not in `mpat.lock`. diff --git a/docs/site/pages/lockfile.md b/docs/site/pages/lockfile.md index 479c498..a9ee099 100644 --- a/docs/site/pages/lockfile.md +++ b/docs/site/pages/lockfile.md @@ -70,6 +70,7 @@ no_source = false | `dist` | The distribution that installs the target's top-level package | | `dist_version` | The installed version of that distribution | | `no_source` | True when the target should have had a source hash and did not | +| `code_hash` | `sha256:` of the same AST node with every docstring removed, so a docstring-only change is told apart from a code change | Fields with a `None` value are omitted from the table. Entries for constants carry `value_repr` instead of `source_hash` and `source_file`; entries for diff --git a/pyproject.toml b/pyproject.toml index d580853..11728ed 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -1,6 +1,6 @@ [project] name = "mpat" -version = "0.3.1" +version = "0.3.2" description = "Lockfile-based upstream drift tracking for Python monkeypatches" readme = "README.md" license = "MIT" diff --git a/src/mpat/__init__.py b/src/mpat/__init__.py index c75d755..4776e6f 100644 --- a/src/mpat/__init__.py +++ b/src/mpat/__init__.py @@ -15,7 +15,7 @@ ) from mpat._until import Probe, Until, Version -__version__ = "0.3.1" +__version__ = "0.3.2" __all__ = [ "AlreadyPatched", diff --git a/src/mpat/_api.py b/src/mpat/_api.py index 28d84f2..e89a24a 100644 --- a/src/mpat/_api.py +++ b/src/mpat/_api.py @@ -75,12 +75,13 @@ def __call__(self, *args: Any, **kwargs: Any) -> Any: ... F = TypeVar("F", bound=Replacement) +T = TypeVar("T") log = logging.getLogger("mpat") -def _caller_file() -> str: +def _caller() -> tuple[str, int]: frame = inspect.stack()[_DECLARATION_FRAME_DEPTH] - return os.path.abspath(frame.filename) + return os.path.abspath(frame.filename), frame.lineno def _allow() -> tuple[str, ...]: @@ -211,6 +212,16 @@ def _check_kind(fn: Replacement, resolved: Resolved) -> None: ) +def _resolve_declared(resolver: Callable[[str], T], target: str) -> T | None: + """While collecting, a target deleted upstream is left for check() to report as missing.""" + try: + return resolver(target) + except TargetNotFound: + if collect_mode(): + return None + raise + + def _resolve_patchable(canonical: str) -> Resolved: resolved = resolve_attribute(canonical) check_supported(resolved) @@ -257,8 +268,8 @@ def patch( raise ValueError(f"on_drift must be one of {ON_DRIFT_VALUES}, got {on_drift!r}") canonical = canonical_target(target) _check_declaration_forbidden(canonical, depends_on) - resolved = None if when_imported else _resolve_patchable(canonical) - declared_in = _caller_file() + resolved = None if when_imported else _resolve_declared(_resolve_patchable, canonical) + declared_in, declared_line = _caller() def decorator(fn: F) -> F: if resolved is not None: @@ -272,6 +283,7 @@ def decorator(fn: F) -> F: review_by=review_by, note=note, declared_in=declared_in, + declared_line=declared_line, identity=(declared_in, fn.__qualname__), ) register(decl) @@ -331,8 +343,8 @@ def watch( ) -> None: canonical = canonical_target(target) _check_declaration_forbidden(canonical, depends_on) - resolved = resolve(canonical) - declared_in = _caller_file() + resolved = _resolve_declared(resolve, canonical) + declared_in, declared_line = _caller() decl = Declaration( target=canonical, role=ROLE_WATCH, @@ -342,11 +354,12 @@ def watch( review_by=review_by, note=note, declared_in=declared_in, + declared_line=declared_line, identity=(declared_in, f"watch:{canonical}"), track_value=track_value, ) register(decl) - if collect_mode(): + if resolved is None or collect_mode(): return _handle_drift(decl, resolved) decl.status = STATUS_WATCHED diff --git a/src/mpat/_cli.py b/src/mpat/_cli.py index c1b6e56..9141c0c 100644 --- a/src/mpat/_cli.py +++ b/src/mpat/_cli.py @@ -14,12 +14,14 @@ from mpat._fingerprint import ( BODY, CONTENT, + DOCSTRING, MOVED, NO_SOURCE, OK, SIGNATURE, VALUE, Fingerprint, + changed_fields, compare, fingerprint, ) @@ -32,6 +34,7 @@ UNLOCKED, CheckResult, Lock, + Usage, build_lock, check, lock_path, @@ -39,7 +42,9 @@ write_lock, ) from mpat._registry import collect_declarations +from mpat._render import DIFF_STATUSES, diff_block from mpat._targets import ResolvedFile, resolve +from mpat._upstream import UpstreamUnavailable, installed_source, locked_source EXIT_OK = 0 EXIT_DRIFT = 1 @@ -58,6 +63,9 @@ _NO_SOURCE_NOTICE = "no source available, only the signature is locked" _NO_DIST = "(no distribution)" _MISSING_CELL = "(missing)" +_OK_HIDDEN_HINT = " (--all lists them)" +_CHANGE_INDENT = " " +_NOTHING_TO_DIFF = "no drifted targets to diff" _DRIFT_STATUSES = (MISSING, MOVED, SIGNATURE, BODY, CONTENT, VALUE, NO_SOURCE) _FOOTERS: tuple[tuple[tuple[str, ...], str], ...] = ( ((UNLOCKED,), "unlocked: run 'mpat lock'"), @@ -65,6 +73,7 @@ _DRIFT_STATUSES, "drifted: read the upstream change, then fix or delete the patch and run 'mpat lock'", ), + ((DOCSTRING,), "docstring-only: skim the upstream docs change, then run 'mpat lock'"), ((STALE,), "stale: run 'mpat lock' to prune"), ((REVIEW,), "due for review"), ) @@ -122,22 +131,48 @@ def _status_cell(result: CheckResult) -> str: return result.status + (_NO_SOURCE_MARKER if result.no_source else "") -def _print_footer(results: Sequence[CheckResult]) -> None: +def _print_footer(results: Sequence[CheckResult], *, ok_hidden: bool, after_rows: bool) -> None: lines = [ f"{count} {advice}" for statuses, advice in _FOOTERS if (count := sum(1 for r in results if r.status in statuses)) ] + if ok_count := sum(1 for r in results if r.status == OK): + lines.append(f"{ok_count} {OK}" + (_OK_HIDDEN_HINT if ok_hidden else "")) if lines: - print() + if after_rows: + print() print("\n".join(lines)) -def _print_table(results: Sequence[CheckResult]) -> None: - status_width = max([_STATUS_WIDTH, *(len(_status_cell(r)) for r in results)]) - target_width = max((len(r.target) for r in results), default=0) - source_width = max((len(r.declared_in) for r in results), default=0) - ordered = sorted(results, key=lambda r: (r.dist or "", r.target)) +def _usage_cell(usage: Usage) -> str: + where = ( + usage.declared_in + if usage.declared_line is None + else f"{usage.declared_in}:{usage.declared_line}" + ) + return f"{where} ({usage.role} {usage.target})" + + +def _source_block(result: CheckResult, lock: Lock) -> list[str]: + entry = lock.entries.get(result.target) + if result.status not in DIFF_STATUSES or entry is None: + return [] + try: + old, unavailable = locked_source(entry), None + except UpstreamUnavailable as exc: + old, unavailable = None, str(exc) + return diff_block(result, old=old, new=installed_source(result.target), unavailable=unavailable) + + +def _print_table( + results: Sequence[CheckResult], *, show_ok: bool, diff_from: Lock | None = None +) -> None: + shown = results if show_ok else [r for r in results if r.status != OK] + status_width = max([_STATUS_WIDTH, *(len(_status_cell(r)) for r in shown)]) + target_width = max((len(r.target) for r in shown), default=0) + source_width = max((len(r.declared_in) for r in shown), default=0) + ordered = sorted(shown, key=lambda r: (r.dist or "", r.target)) previous: str | None = None for r in ordered: group = r.dist or _NO_DIST @@ -155,18 +190,26 @@ def _print_table(results: Sequence[CheckResult]) -> None: f"{r.target:<{target_width}} {r.declared_in:<{source_width}}{versions}{note}" ) print(row.rstrip()) - _print_footer(ordered) + for change in r.changes: + print(f"{_CHANGE_INDENT}{change}".rstrip()) + block = _source_block(r, diff_from) if diff_from is not None else [] + if not block: + for usage in r.used_by: + print(f"{_CHANGE_INDENT}used by: {_usage_cell(usage)}") + for line in block: + print(f"{_CHANGE_INDENT}{line}".rstrip()) + _print_footer(results, ok_hidden=not show_ok, after_rows=bool(ordered)) + + +def _check(config: Config, lock: Lock) -> list[CheckResult]: + declarations = collect_declarations(config, apply=False) + return check(declarations, lock, root=config.root, today=date.today()) def cmd_check(args: argparse.Namespace) -> int: config = _require_config() - declarations = collect_declarations(config, apply=False) - results = check( - declarations, - _existing_lock(config.root), - root=config.root, - today=date.today(), - ) + lock = _existing_lock(config.root) + results = _check(config, lock) if args.json: print(json.dumps([dataclasses.asdict(r) for r in results], indent=_JSON_INDENT)) elif args.gitlab: @@ -179,7 +222,7 @@ def cmd_check(args: argparse.Namespace) -> int: elif not results: print(_NO_DECLARATIONS) else: - _print_table(results) + _print_table(results, show_ok=args.all, diff_from=lock if args.diff else None) return EXIT_OK if all(r.status == OK for r in results) else EXIT_DRIFT @@ -200,17 +243,30 @@ def cmd_show(args: argparse.Namespace) -> int: def _diff_lines(locked: Fingerprint, current: Fingerprint | None) -> list[str]: - lines: list[str] = [] - for name, before in dataclasses.asdict(locked).items(): - after = _MISSING_CELL if current is None else getattr(current, name) - if before != after: - lines.append(f"{name}: {before} -> {after}") - return lines + if current is None: + return [ + f"{name}: {before} -> {_MISSING_CELL}" + for name, before in dataclasses.asdict(locked).items() + ] + return [ + f"{name}: {before} -> {after}" + for name, before, after in changed_fields(locked=locked, current=current) + ] + + +def _diff_all(config: Config, lock: Lock) -> int: + results = _check(config, lock) + blocks = [block for r in results if (block := _source_block(r, lock))] + print("\n\n".join("\n".join(block) for block in blocks) if blocks else _NOTHING_TO_DIFF) + return EXIT_OK if all(r.status == OK for r in results) else EXIT_DRIFT def cmd_diff(args: argparse.Namespace) -> int: config = _require_config() - entry = _existing_lock(config.root).entries.get(args.target) + lock = _existing_lock(config.root) + if args.target is None: + return _diff_all(config, lock) + entry = lock.entries.get(args.target) if entry is None: raise MpatError(f"{args.target}: not in {LOCK_FILENAME}") try: @@ -221,6 +277,11 @@ def cmd_diff(args: argparse.Namespace) -> int: print(f"status: {status}") for line in _diff_lines(entry.fingerprint, current): print(line) + result = next((r for r in _check(config, lock) if r.target == args.target), None) + block = _source_block(result, lock) if result is not None else [] + if block: + print() + print("\n".join(block)) return EXIT_OK if status == OK else EXIT_DRIFT @@ -237,6 +298,16 @@ def _parser() -> argparse.ArgumentParser: action="store_true", help="print a GitLab Code Quality report of every non-ok target", ) + check_parser.add_argument( + "--all", + action="store_true", + help="list ok targets in the table too (default: only their count)", + ) + check_parser.add_argument( + "--diff", + action="store_true", + help="show the upstream source change under each drifted row (fetches the locked release)", + ) check_parser.add_argument( "--repo-root", type=Path, @@ -247,8 +318,10 @@ def _parser() -> argparse.ArgumentParser: show_parser = sub.add_parser("show", help="print fingerprint and source of a target") show_parser.add_argument("target") show_parser.set_defaults(fn=cmd_show) - diff_parser = sub.add_parser("diff", help="show which locked fields of a target changed") - diff_parser.add_argument("target") + diff_parser = sub.add_parser( + "diff", help="show how a target, or every drifted target, changed upstream" + ) + diff_parser.add_argument("target", nargs="?") diff_parser.set_defaults(fn=cmd_diff) return parser diff --git a/src/mpat/_fingerprint.py b/src/mpat/_fingerprint.py index 2afc276..b7c44ee 100644 --- a/src/mpat/_fingerprint.py +++ b/src/mpat/_fingerprint.py @@ -1,13 +1,14 @@ from __future__ import annotations import ast +import copy import functools import hashlib import inspect import os import sys from collections.abc import Iterator, Mapping -from dataclasses import dataclass +from dataclasses import dataclass, fields from importlib import metadata from pathlib import Path from typing import Any @@ -28,12 +29,16 @@ CONTENT = "content" VALUE = "value" NO_SOURCE = "no_source" +DOCSTRING = "docstring" HASH_PREFIX = "sha256:" SCALARS = (int, str, bool, float, type(None)) _DEFS = (ast.FunctionDef, ast.AsyncFunctionDef, ast.ClassDef) +SourceNode = ast.Module | ast.FunctionDef | ast.AsyncFunctionDef | ast.ClassDef _BLOCKS = (ast.If, ast.Try, ast.With, ast.For, ast.While) _ASYNC_KINDS = (ASYNC, ASYNCGEN) +_UNREADABLE_FIELDS = frozenset({"source_hash", "code_hash", "dist", "dist_version", "no_source"}) +_DOCSTRING_OWNERS = (ast.Module, *_DEFS) @dataclass(frozen=True) @@ -48,6 +53,7 @@ class Fingerprint: dist: str | None = None dist_version: str | None = None no_source: bool = False + code_hash: str | None = None def _iter_defs( @@ -64,11 +70,11 @@ def _iter_defs( yield from _iter_defs(handler.body) -def _first_line(node: ast.FunctionDef | ast.AsyncFunctionDef | ast.ClassDef) -> int: +def first_line(node: ast.FunctionDef | ast.AsyncFunctionDef | ast.ClassDef) -> int: return min([node.lineno, *(d.lineno for d in node.decorator_list)]) -def _definition_node( +def definition_node( tree: ast.Module, qualname: str, *, lineno: int | None = None ) -> ast.FunctionDef | ast.AsyncFunctionDef | ast.ClassDef | None: if "" in qualname: @@ -84,19 +90,19 @@ def _definition_node( candidates = [n for n in _iter_defs(body) if n.name == leaf] if not candidates: return None - at_line = [n for n in candidates if _first_line(n) == lineno] + at_line = [n for n in candidates if first_line(n) == lineno] return at_line[0] if at_line else candidates[0] -def _hash(node: ast.AST) -> str: - return _hash_bytes(ast.unparse(node).encode()) +def hash_node(node: ast.AST) -> str: + return hash_bytes(ast.unparse(node).encode()) -def _hash_bytes(data: bytes) -> str: +def hash_bytes(data: bytes) -> str: return HASH_PREFIX + hashlib.sha256(data).hexdigest() -def _source_file(obj: Any) -> str | None: +def source_path(obj: Any) -> str | None: try: file = inspect.getsourcefile(obj) except TypeError: @@ -109,19 +115,34 @@ def _source_file(obj: Any) -> str | None: _SOURCE_FAILURES = (OSError, UnicodeDecodeError, SyntaxError, ValueError) -def _source_hash(obj: Any, file: str) -> str | None: +def source_node(obj: Any, file: str) -> SourceNode | None: try: tree = ast.parse(Path(file).read_text(encoding="utf-8")) except _SOURCE_FAILURES: return None if inspect.ismodule(obj): - return _hash(tree) + return tree qualname = getattr(obj, "__qualname__", None) if not qualname: return None code = getattr(obj, "__code__", None) - node = _definition_node(tree, qualname, lineno=getattr(code, "co_firstlineno", None)) - return None if node is None else _hash(node) + return definition_node(tree, qualname, lineno=getattr(code, "co_firstlineno", None)) + + +def _is_docstring(stmt: ast.stmt) -> bool: + return ( + isinstance(stmt, ast.Expr) + and isinstance(stmt.value, ast.Constant) + and isinstance(stmt.value.value, str) + ) + + +def _without_docstrings(node: ast.AST) -> ast.AST: + stripped = copy.deepcopy(node) + for owner in ast.walk(stripped): + if isinstance(owner, _DOCSTRING_OWNERS) and owner.body and _is_docstring(owner.body[0]): + owner.body = owner.body[1:] or [ast.Pass()] + return stripped def _kind(obj: Any) -> str: @@ -168,7 +189,7 @@ def _dist(top: str) -> tuple[str | None, str | None]: return names[0], metadata.version(names[0]) -def _relative_source(file: str, top: str) -> str: +def relative_source(file: str, top: str) -> str: module = sys.modules.get(top) module_file = getattr(module, "__file__", None) search_path = [Path(entry) for entry in getattr(module, "__path__", [])] @@ -199,7 +220,7 @@ def _fingerprint_file(resolved: ResolvedFile) -> Fingerprint: return Fingerprint( kind=KIND_FILE, resolved=resolved.target, - source_hash=_hash_bytes(resolved.read_bytes()), + source_hash=hash_bytes(resolved.read_bytes()), source_file=resolved.target, dist=dist, dist_version=dist_version, @@ -230,8 +251,9 @@ def fingerprint(resolved: Resolved | ResolvedFile, *, track_value: bool = True) value_repr = None hash_target = inspect.unwrap(raw_target) if raw_target is not None else None - file = _source_file(hash_target) if hash_target is not None else None - source_hash = _source_hash(hash_target, file) if file else None + file = source_path(hash_target) if hash_target is not None else None + node = source_node(hash_target, file) if file else None + source_hash = hash_node(node) if node is not None else None expects_source = hash_target is not None return Fingerprint( kind=kind, @@ -240,10 +262,28 @@ def fingerprint(resolved: Resolved | ResolvedFile, *, track_value: bool = True) signature=_signature(obj) if kind == KIND_FUNCTION else None, source_hash=source_hash, value_repr=value_repr, - source_file=_relative_source(file, top) if file else None, + source_file=relative_source(file, top) if file else None, dist=dist, dist_version=dist_version, no_source=expects_source and source_hash is None, + code_hash=hash_node(_without_docstrings(node)) if node is not None else None, + ) + + +def changed_fields(*, locked: Fingerprint, current: Fingerprint) -> list[tuple[str, Any, Any]]: + return [ + (f.name, before, after) + for f in fields(Fingerprint) + if (before := getattr(locked, f.name)) != (after := getattr(current, f.name)) + ] + + +def readable_changes(*, locked: Fingerprint, current: Fingerprint) -> tuple[str, ...]: + """The changed fields a reviewer can act on; hashes and versions are shown elsewhere.""" + return tuple( + f"{name}: {before} -> {after}" + for name, before, after in changed_fields(locked=locked, current=current) + if name not in _UNREADABLE_FIELDS ) @@ -261,5 +301,9 @@ def compare(*, locked: Fingerprint, current: Fingerprint) -> str: if locked.source_hash is not None and current.source_hash is None: return NO_SOURCE if locked.source_hash != current.source_hash: - return CONTENT if locked.kind == KIND_FILE else BODY + if locked.kind == KIND_FILE: + return CONTENT + if locked.code_hash is not None and locked.code_hash == current.code_hash: + return DOCSTRING + return BODY return OK diff --git a/src/mpat/_gitlab.py b/src/mpat/_gitlab.py index c70f2b6..c168a3c 100644 --- a/src/mpat/_gitlab.py +++ b/src/mpat/_gitlab.py @@ -5,8 +5,8 @@ from pathlib import Path from typing import Any -from mpat._fingerprint import BODY, CONTENT, MOVED, NO_SOURCE, OK, SIGNATURE, VALUE -from mpat._lock import MISSING, REVIEW, STALE, UNLOCKED, CheckResult +from mpat._fingerprint import BODY, CONTENT, DOCSTRING, MOVED, NO_SOURCE, OK, SIGNATURE, VALUE +from mpat._lock import MISSING, REVIEW, STALE, UNLOCKED, CheckResult, Usage SEVERITY_BLOCKER = "blocker" SEVERITY_MAJOR = "major" @@ -27,6 +27,7 @@ VALUE: SEVERITY_MAJOR, UNLOCKED: SEVERITY_MAJOR, NO_SOURCE: SEVERITY_MINOR, + DOCSTRING: SEVERITY_MINOR, REVIEW: SEVERITY_MINOR, STALE: SEVERITY_INFO, } @@ -38,6 +39,7 @@ SIGNATURE: "the upstream signature changed", BODY: "the upstream body changed", CONTENT: "the upstream file changed", + DOCSTRING: "only the upstream docstrings changed", VALUE: "the upstream value changed", NO_SOURCE: "upstream source is unavailable, only the signature is locked", UNLOCKED: f"not in the lock; run '{_CHECK_NAME_PREFIX} lock'", @@ -85,17 +87,42 @@ def _declaration_line(path: Path, target: str, cache: dict[Path, str]) -> int: return _line_in(cache[path], target) +def describe(status: str) -> str: + return _DESCRIPTIONS.get(status, _UNKNOWN_DESCRIPTION) + + +def _what(result: CheckResult, *, note: str = "") -> str: + what = describe(result.status) + noted = f" ({note})" if note else "" + changes = "".join(f"; {change}" for change in result.changes) + return f"{what}{noted}{changes}" + + def _description(result: CheckResult) -> str: - what = _DESCRIPTIONS.get(result.status, _UNKNOWN_DESCRIPTION) - note = f" ({result.note})" if result.note else "" - return f"{result.role} {result.target}: {what}{note}" + return f"{result.role} {result.target}: {_what(result, note=result.note)}" -def _fingerprint(path: str, result: CheckResult) -> str: - identity = _FINGERPRINT_SEPARATOR.join((path, result.role, result.target)) +def _usage_description(result: CheckResult, usage: Usage) -> str: + return f"{usage.role} {usage.target} depends on {result.target}: {_what(result)}" + + +def _fingerprint(*parts: str) -> str: + identity = _FINGERPRINT_SEPARATOR.join(parts) return hashlib.sha256(identity.encode("utf-8")).hexdigest() +def _issue( + result: CheckResult, *, description: str, fingerprint: str, path: str, line: int +) -> dict[str, Any]: + return { + "description": description, + "check_name": f"{_CHECK_NAME_PREFIX}/{result.status}", + "fingerprint": fingerprint, + "severity": _SEVERITIES.get(result.status, _DEFAULT_SEVERITY), + "location": {"path": path, "lines": {"begin": line}}, + } + + def code_quality_report( results: Sequence[CheckResult], *, root: Path, base: Path | None = None ) -> list[dict[str, Any]]: @@ -103,7 +130,8 @@ def code_quality_report( `location.path` is resolved against the repository root, not against `root`, because GitLab reads it relative to the checkout: a `mpat check` run in - `services/api` must still report `services/api/src/patches.py`. + `services/api` must still report `services/api/src/patches.py`. Every other + declaration using a drifted target gets its own issue at its own line. """ base = base or repo_root(root) or root cache: dict[Path, str] = {} @@ -113,15 +141,24 @@ def code_quality_report( continue path = _report_path(result.declared_in, root=root, base=base) report.append( - { - "description": _description(result), - "check_name": f"{_CHECK_NAME_PREFIX}/{result.status}", - "fingerprint": _fingerprint(path, result), - "severity": _SEVERITIES.get(result.status, _DEFAULT_SEVERITY), - "location": { - "path": path, - "lines": {"begin": _declaration_line(base / path, result.target, cache)}, - }, - } + _issue( + result, + description=_description(result), + fingerprint=_fingerprint(path, result.role, result.target), + path=path, + line=result.declared_line or _declaration_line(base / path, result.target, cache), + ) ) + for usage in result.used_by: + usage_path = _report_path(usage.declared_in, root=root, base=base) + report.append( + _issue( + result, + description=_usage_description(result, usage), + fingerprint=_fingerprint(usage_path, result.role, result.target, usage.target), + path=usage_path, + line=usage.declared_line + or _declaration_line(base / usage_path, usage.target, cache), + ) + ) return report diff --git a/src/mpat/_lock.py b/src/mpat/_lock.py index 9f609b6..3bc5324 100644 --- a/src/mpat/_lock.py +++ b/src/mpat/_lock.py @@ -6,6 +6,7 @@ import tempfile import tomllib import warnings +from collections import defaultdict from collections.abc import Sequence from dataclasses import dataclass from datetime import date @@ -16,7 +17,7 @@ from mpat._config import find_project_root from mpat._errors import LockError, TargetNotFound -from mpat._fingerprint import OK, Fingerprint, compare, fingerprint +from mpat._fingerprint import OK, Fingerprint, compare, fingerprint, readable_changes from mpat._registry import ROLE_DEPENDS, Declaration from mpat._targets import resolve @@ -41,6 +42,7 @@ "dist", "dist_version", "parent", + "code_hash", ) _TRACK_VALUE_KEY = "track_value" _BOOL_FIELDS = ("is_async", "no_source", _TRACK_VALUE_KEY) @@ -73,6 +75,14 @@ class Wanted: track_value: bool +@dataclass(frozen=True) +class Usage: + declared_in: str + declared_line: int | None + role: str + target: str + + @dataclass(frozen=True) class CheckResult: target: str @@ -84,6 +94,9 @@ class CheckResult: current_version: str | None = None no_source: bool = False dist: str | None = None + declared_line: int | None = None + changes: tuple[str, ...] = () + used_by: tuple[Usage, ...] = () def lock_path(root: Path) -> Path: @@ -251,14 +264,33 @@ def _status(w: Wanted, entry: LockEntry, current: Fingerprint | None, today: dat return result +def _users(declarations: Sequence[Declaration]) -> dict[str, list[Declaration]]: + users: dict[str, list[Declaration]] = defaultdict(list) + for decl in declarations: + for target in dict.fromkeys((decl.target, *decl.depends_on)): + users[target].append(decl) + return users + + def check( declarations: Sequence[Declaration], lock: Lock, *, root: Path, today: date ) -> list[CheckResult]: results: list[CheckResult] = [] seen: set[str] = set() + users = _users(declarations) for w in expand(declarations): seen.add(w.target) declared_in = _declared_in(w.decl, root) + used_by = tuple( + Usage( + declared_in=_declared_in(d, root), + declared_line=d.declared_line, + role=d.role, + target=d.target, + ) + for d in users[w.target] + if d is not w.decl + ) entry = lock.entries.get(w.target) current = _current(w.target, track_value=w.track_value) if entry is not None and ( @@ -274,6 +306,8 @@ def check( note=w.decl.note, declared_in=declared_in, dist=current.dist if current else None, + declared_line=w.decl.declared_line, + used_by=used_by, ) ) continue @@ -288,6 +322,11 @@ def check( current_version=current.dist_version if current else None, no_source=current.no_source if current else False, dist=current.dist if current else entry.fingerprint.dist, + declared_line=w.decl.declared_line, + changes=readable_changes(locked=entry.fingerprint, current=current) + if current + else (), + used_by=used_by, ) ) results.extend( diff --git a/src/mpat/_registry.py b/src/mpat/_registry.py index 682fc66..1fad792 100644 --- a/src/mpat/_registry.py +++ b/src/mpat/_registry.py @@ -44,6 +44,7 @@ class Declaration: identity: tuple[str, str] status: str = field(default=STATUS_REGISTERED) track_value: bool = True + declared_line: int | None = None _declarations: dict[tuple[str, str], Declaration] = {} diff --git a/src/mpat/_render.py b/src/mpat/_render.py new file mode 100644 index 0000000..05c2e7a --- /dev/null +++ b/src/mpat/_render.py @@ -0,0 +1,107 @@ +"""ty-style blocks showing how a drifted target's upstream source changed.""" + +from __future__ import annotations + +from difflib import SequenceMatcher +from typing import NamedTuple + +from mpat._fingerprint import BODY, CONTENT, DOCSTRING, MOVED, SIGNATURE +from mpat._gitlab import describe +from mpat._lock import MISSING, CheckResult, Usage +from mpat._upstream import Source + +DIFF_STATUSES = (BODY, CONTENT, DOCSTRING, SIGNATURE, MOVED, MISSING) +CONTEXT_LINES = 3 + +_KEEP = " " +_REMOVED = "-" +_ADDED = "+" +_HUNK_GAP = "..." +_REBUILT = "warning: the downloaded source does not match mpat.lock" + + +def _where(path: str, line: int | None) -> str: + return path if line is None else f"{path}:{line}" + + +def _usage(usage: Usage) -> str: + return f"{_where(usage.declared_in, usage.declared_line)} ({usage.role} {usage.target})" + + +def _header(result: CheckResult) -> str: + versions = "" + if ( + result.locked_version + and result.current_version + and result.locked_version != result.current_version + ): + versions = f" ({result.locked_version} -> {result.current_version})" + return f"{result.status}[{result.role}] {result.target}: {describe(result.status)}{versions}" + + +class _Row(NamedTuple): + old: int | None + new: int | None + marker: str + text: str + + +def _hunks(old: Source, new: Source | None) -> list[list[_Row]]: + """A gone target is one hunk of removed lines.""" + if new is None: + return [[_Row(old.start + i, None, _REMOVED, text) for i, text in enumerate(old.lines)]] + hunks: list[list[_Row]] = [] + matcher = SequenceMatcher(a=old.lines, b=new.lines, autojunk=False) + for group in matcher.get_grouped_opcodes(CONTEXT_LINES): + hunk: list[_Row] = [] + for tag, i1, i2, j1, j2 in group: + if tag == "equal": + hunk.extend( + _Row(old.start + i, new.start + j, _KEEP, old.lines[i]) + for i, j in zip(range(i1, i2), range(j1, j2), strict=True) + ) + continue + hunk.extend(_Row(old.start + i, None, _REMOVED, old.lines[i]) for i in range(i1, i2)) + hunk.extend(_Row(None, new.start + j, _ADDED, new.lines[j]) for j in range(j1, j2)) + hunks.append(hunk) + return hunks + + +def _location(rows: list[_Row], old: Source, new: Source | None) -> str: + """Point at the first added line in the installed file, else the first removed one.""" + added = next((r.new for r in rows if r.marker == _ADDED), None) + if new is not None and added is not None: + return _where(new.path, added) + return _where(old.path, next(r.old for r in rows if r.marker == _REMOVED)) + + +def diff_block( + result: CheckResult, *, old: Source | None, new: Source | None, unavailable: str | None = None +) -> list[str]: + lines = [_header(result)] + hunks = _hunks(old, new) if old is not None and unavailable is None else [] + rows = [row for hunk in hunks for row in hunk] + width = max( + (len(str(n)) for row in rows for n in (row.old, row.new) if n is not None), default=0 + ) + pad = 2 * width + 1 + arrow = f"{' ' * (pad - 1)}-->" + gutter = f"{' ' * pad} |" + if unavailable is not None: + lines.append(f"info: upstream diff unavailable: {unavailable}") + if old is not None and rows: + lines.extend((f"{arrow} {_location(rows, old, new)}", gutter)) + for index, hunk in enumerate(hunks): + if index: + lines.append(f"{' ' * pad} {_HUNK_GAP}") + for row in hunk: + o = "" if row.old is None else str(row.old) + n = "" if row.new is None else str(row.new) + lines.append(f"{o:>{width}} {n:>{width}} | {row.marker} {row.text}".rstrip()) + lines.append(gutter) + if not old.matches_lock: + lines.append(_REBUILT) + lines.append("info: declared here") + lines.append(f"{arrow} {_where(result.declared_in, result.declared_line)}") + lines.extend(f"info: also used by {_usage(u)}" for u in result.used_by) + return lines diff --git a/src/mpat/_upstream.py b/src/mpat/_upstream.py new file mode 100644 index 0000000..b80eb49 --- /dev/null +++ b/src/mpat/_upstream.py @@ -0,0 +1,337 @@ +"""Fetch the upstream source a lock entry was taken from, for `mpat diff`.""" + +from __future__ import annotations + +import ast +import base64 +import http.client +import inspect +import json +import netrc +import os +import tarfile +import tempfile +import urllib.error +import urllib.parse +import urllib.request +import zipfile +from dataclasses import dataclass +from html.parser import HTMLParser +from http.client import HTTPMessage +from pathlib import Path +from typing import IO, Any + +from packaging.utils import ( + InvalidSdistFilename, + InvalidWheelFilename, + canonicalize_name, + parse_sdist_filename, + parse_wheel_filename, +) +from packaging.version import InvalidVersion, Version + +from mpat._errors import MpatError, TargetNotFound +from mpat._fingerprint import ( + KIND_FILE, + KIND_MODULE, + SourceNode, + definition_node, + first_line, + hash_bytes, + hash_node, + relative_source, + source_node, + source_path, +) +from mpat._lock import LockEntry +from mpat._targets import ResolvedFile, resolve + +INDEX_ENV_VARS = ("UV_DEFAULT_INDEX", "UV_INDEX_URL", "PIP_INDEX_URL") +DEFAULT_INDEX = "https://pypi.org/simple" +CACHE_ENV = "MPAT_CACHE_DIR" +XDG_CACHE_ENV = "XDG_CACHE_HOME" +TIMEOUT_SECONDS = 10 + +_SIMPLE_JSON = "application/vnd.pypi.simple.v1+json" +_ACCEPT = f"{_SIMPLE_JSON}, text/html;q=0.1" +_PURE_WHEEL_TAG = "py3-none-any" +_SDIST_SUFFIXES = (".tar.gz", ".zip") +_INIT = "__init__.py" +_PY = ".py" +_AUTHORIZATION = "Authorization" + + +class UpstreamUnavailable(MpatError): + pass + + +@dataclass(frozen=True) +class Source: + path: str + start: int + lines: tuple[str, ...] + matches_lock: bool = True + + +@dataclass(frozen=True) +class _Link: + filename: str + url: str + + +class _Anchors(HTMLParser): + def __init__(self) -> None: + super().__init__() + self.hrefs: list[str] = [] + + def handle_starttag(self, tag: str, attrs: list[tuple[str, str | None]]) -> None: + href = dict(attrs).get("href") + if tag == "a" and href: + self.hrefs.append(href) + + +def _index_url() -> str: + return next( + (os.environ[name] for name in INDEX_ENV_VARS if os.environ.get(name)), DEFAULT_INDEX + ) + + +def _cache_dir() -> Path: + if explicit := os.environ.get(CACHE_ENV): + return Path(explicit) + if xdg := os.environ.get(XDG_CACHE_ENV): + return Path(xdg) / "mpat" + try: + return Path.home() / ".cache" / "mpat" + except RuntimeError: + return Path(tempfile.gettempdir()) / "mpat" + + +def _without_credentials(url: str) -> tuple[str, str | None]: + """Split userinfo off the index URL; urllib never sends it on its own.""" + parts = urllib.parse.urlsplit(url) + if parts.username is None: + return url, None + host = parts.hostname or "" + netloc = host if parts.port is None else f"{host}:{parts.port}" + user = urllib.parse.unquote(parts.username) + password = urllib.parse.unquote(parts.password or "") + token = base64.b64encode(f"{user}:{password}".encode()).decode() + return urllib.parse.urlunsplit(parts._replace(netloc=netloc)), f"Basic {token}" + + +def _netrc_auth(host: str) -> str | None: + try: + found = netrc.netrc().authenticators(host) + except (FileNotFoundError, netrc.NetrcParseError): + return None + if found is None: + return None + login, _, password = found + return "Basic " + base64.b64encode(f"{login}:{password or ''}".encode()).decode() + + +class _KeepAuthOnHost(urllib.request.HTTPRedirectHandler): + """Private indexes redirect downloads to signed object-storage URLs; the index + credentials must not travel there, and storage rejects a request carrying both.""" + + def __init__(self, host: str | None) -> None: + super().__init__() + self.host = host + + def redirect_request( + self, + req: urllib.request.Request, + fp: IO[bytes], + code: int, + msg: str, + headers: HTTPMessage, + newurl: str, + ) -> urllib.request.Request | None: + new = super().redirect_request(req, fp, code, msg, headers, newurl) + if new is not None and urllib.parse.urlsplit(newurl).hostname != self.host: + new.remove_header(_AUTHORIZATION) + return new + + +def _get( + url: str, *, auth: str | None, auth_host: str | None, accept: str | None = None +) -> tuple[bytes, str]: + request = urllib.request.Request(url) + if accept: + request.add_header("Accept", accept) + if auth and urllib.parse.urlsplit(url).hostname == auth_host: + request.add_header(_AUTHORIZATION, auth) + opener = urllib.request.build_opener(_KeepAuthOnHost(auth_host)) + try: + with opener.open(request, timeout=TIMEOUT_SECONDS) as response: + return response.read(), response.headers.get_content_type() + except (urllib.error.URLError, http.client.HTTPException, OSError, ValueError) as exc: + raise UpstreamUnavailable(f"cannot fetch {url}: {exc}") from exc + + +def _links(body: bytes, content_type: str, *, page: str) -> list[_Link]: + if content_type == _SIMPLE_JSON: + try: + files: list[dict[str, Any]] = json.loads(body)["files"] + except (ValueError, KeyError, TypeError) as exc: + raise UpstreamUnavailable(f"unreadable index page {page}: {exc}") from exc + pairs = [(f["filename"], f["url"]) for f in files] + else: + parser = _Anchors() + parser.feed(body.decode("utf-8", errors="replace")) + pairs = [(urllib.parse.urlsplit(h).path.rsplit("/", 1)[-1], h) for h in parser.hrefs] + return [ + _Link( + filename=urllib.parse.unquote(name), + url=urllib.parse.urldefrag(urllib.parse.urljoin(page, url)).url, + ) + for name, url in pairs + ] + + +def _archive_version(filename: str) -> tuple[Version, bool, bool] | None: + """(version, is_wheel, is_pure) for a distribution filename, None for anything else.""" + try: + if filename.endswith(".whl"): + _, version, _, tags = parse_wheel_filename(filename) + return version, True, any(str(tag) == _PURE_WHEEL_TAG for tag in tags) + if filename.endswith(_SDIST_SUFFIXES): + _, version = parse_sdist_filename(filename) + return version, False, False + except (InvalidWheelFilename, InvalidSdistFilename, InvalidVersion): + return None + return None + + +def _pick(links: list[_Link], version: Version) -> _Link | None: + """Prefer a pure wheel, then any wheel, then an sdist, of exactly `version`.""" + ranked: list[tuple[bool, bool, str, _Link]] = [] + for link in links: + parsed = _archive_version(link.filename) + if parsed is None: + continue + found, is_wheel, is_pure = parsed + if found == version: + ranked.append((not is_wheel, not is_pure, link.filename, link)) + return min(ranked, key=lambda r: r[:3])[-1] if ranked else None + + +def _archive(dist: str, version: str) -> Path: + try: + wanted = Version(version) + except InvalidVersion as exc: + raise UpstreamUnavailable(f"{dist} {version}: {exc}") from exc + index, auth = _without_credentials(_index_url()) + index_host = urllib.parse.urlsplit(index).hostname + auth = auth or (_netrc_auth(index_host) if index_host else None) + page = f"{index.rstrip('/')}/{canonicalize_name(dist)}/" + body, content_type = _get(page, auth=auth, auth_host=index_host, accept=_ACCEPT) + link = _pick(_links(body, content_type, page=page), wanted) + if link is None: + raise UpstreamUnavailable(f"no archive for {dist} {version} on {index}") + cached = _cache_dir() / link.filename + if cached.is_file(): + return cached + data, _ = _get(link.url, auth=auth, auth_host=index_host) + try: + cached.parent.mkdir(parents=True, exist_ok=True) + with tempfile.NamedTemporaryFile(dir=cached.parent, delete=False) as fh: + fh.write(data) + os.replace(fh.name, cached) + except OSError as exc: + raise UpstreamUnavailable( + f"cannot cache {link.filename} in {cached.parent}: {exc}" + ) from exc + return cached + + +def _member(archive: Path, source_file: str) -> bytes: + suffix = "/" + source_file + try: + if archive.name.endswith((".whl", ".zip")): + with zipfile.ZipFile(archive) as zf: + name = next( + (n for n in zf.namelist() if n == source_file or n.endswith(suffix)), None + ) + if name is not None: + return zf.read(name) + else: + with tarfile.open(archive) as tf: + member = next( + (m for m in tf.getmembers() if m.isfile() and m.name.endswith(suffix)), None + ) + extracted = tf.extractfile(member) if member is not None else None + if extracted is not None: + return extracted.read() + except (zipfile.BadZipFile, tarfile.TarError, OSError) as exc: + raise UpstreamUnavailable(f"unreadable archive {archive.name}: {exc}") from exc + raise UpstreamUnavailable(f"{source_file} is not in {archive.name}") + + +def _module_of(source_file: str) -> str: + path = source_file.removesuffix(_PY) + if source_file.endswith("/" + _INIT): + path = path.removesuffix("/" + _INIT.removesuffix(_PY)) + return path.replace("/", ".") + + +def _span(lines: list[str], node: SourceNode) -> tuple[int, tuple[str, ...]]: + if isinstance(node, ast.Module): + return 1, tuple(lines) + start = first_line(node) + return start, tuple(lines[start - 1 : node.end_lineno]) + + +def locked_source(entry: LockEntry) -> Source: + locked = entry.fingerprint + if not (locked.dist and locked.dist_version and locked.source_file): + raise UpstreamUnavailable(f"{entry.target}: no distribution recorded in the lock") + data = _member(_archive(locked.dist, locked.dist_version), locked.source_file) + if locked.kind == KIND_FILE: + return Source( + path=locked.source_file, + start=1, + lines=tuple(data.decode("utf-8", errors="replace").splitlines()), + matches_lock=hash_bytes(data) == locked.source_hash, + ) + text = data.decode("utf-8", errors="replace") + try: + tree = ast.parse(text) + except (SyntaxError, ValueError) as exc: + raise UpstreamUnavailable(f"cannot parse {locked.source_file}: {exc}") from exc + module = _module_of(locked.source_file) + if locked.kind == KIND_MODULE: + node: SourceNode | None = tree + elif locked.resolved.startswith(module + "."): + node = definition_node(tree, locked.resolved.removeprefix(module + ".")) + else: + raise UpstreamUnavailable(f"{locked.resolved} is not defined in {locked.source_file}") + if node is None: + raise UpstreamUnavailable(f"{locked.resolved} not found in {locked.source_file}") + start, lines = _span(text.splitlines(), node) + return Source( + path=locked.source_file, + start=start, + lines=lines, + matches_lock=hash_node(node) == locked.source_hash, + ) + + +def installed_source(target: str) -> Source | None: + """The installed definition of `target`, or None when it is gone or has no source.""" + try: + resolved = resolve(target) + except TargetNotFound: + return None + if isinstance(resolved, ResolvedFile): + text = resolved.read_bytes().decode("utf-8", errors="replace") + return Source(path=target, start=1, lines=tuple(text.splitlines())) + obj = resolved.obj.fget if isinstance(resolved.obj, property) else resolved.obj + unwrapped = inspect.unwrap(obj) + file = source_path(unwrapped) + node = source_node(unwrapped, file) if file else None + if file is None or node is None: + return None + start, lines = _span(Path(file).read_text(encoding="utf-8").splitlines(), node) + return Source(path=relative_source(file, target.split(".")[0]), start=start, lines=lines) diff --git a/tests/conftest.py b/tests/conftest.py index ce97ff7..0cc35d8 100644 --- a/tests/conftest.py +++ b/tests/conftest.py @@ -1,7 +1,12 @@ import importlib +import json import shutil import sys +import tarfile +import threading +import zipfile from collections.abc import Iterator +from http.server import BaseHTTPRequestHandler, ThreadingHTTPServer from pathlib import Path import pytest @@ -13,6 +18,7 @@ FIXTURE_ROOT = Path(__file__).parent / "fake_upstream" PKG = "fakeup" PATCH_MODULE = "app_patches" +POLL_SECONDS = 0.01 class Upstream: @@ -28,6 +34,34 @@ def purge(self) -> None: del sys.modules[name] importlib.invalidate_caches() + def install_dist(self, version: str) -> None: + for old in self.root.glob(f"{PKG}-*.dist-info"): + shutil.rmtree(old) + info = self.root / f"{PKG}-{version}.dist-info" + info.mkdir() + (info / "METADATA").write_text(f"Metadata-Version: 2.1\nName: {PKG}\nVersion: {version}\n") + (info / "top_level.txt").write_text(f"{PKG}\n") + importlib.invalidate_caches() + + def sources(self) -> Iterator[Path]: + return ( + p for p in sorted(self.pkg.rglob("*")) if p.is_file() and "__pycache__" not in p.parts + ) + + def build_wheel(self, dest: Path, version: str) -> Path: + wheel = dest / f"{PKG}-{version}-py3-none-any.whl" + with zipfile.ZipFile(wheel, "w") as zf: + for path in self.sources(): + zf.write(path, path.relative_to(self.root).as_posix()) + return wheel + + def build_sdist(self, dest: Path, version: str) -> Path: + sdist = dest / f"{PKG}-{version}.tar.gz" + with tarfile.open(sdist, "w:gz") as tf: + for path in self.sources(): + tf.add(path, f"{PKG}-{version}/src/{path.relative_to(self.root).as_posix()}") + return sdist + def edit(self, rel: str, old: str, new: str) -> None: path = self.pkg / rel text = path.read_text() @@ -36,6 +70,65 @@ def edit(self, rel: str, old: str, new: str) -> None: self.purge() +class Index: + """A package index on localhost serving whatever archives sit in `files`.""" + + JSON = "application/vnd.pypi.simple.v1+json" + HTML = "text/html" + + def __init__(self, root: Path) -> None: + self.files = root / "files" + self.files.mkdir(parents=True) + self.content_type = self.JSON + self.requests: list[str] = [] + self.headers: list[dict[str, str]] = [] + self.redirect_files_to: str | None = None + index = self + + class Handler(BaseHTTPRequestHandler): + def do_GET(self) -> None: + index.requests.append(self.path) + index.headers.append(dict(self.headers)) + path, _, query = self.path.partition("?") + if index.redirect_files_to and path.startswith("/files/") and not query: + self.send_response(302) + self.send_header("Location", f"{index.redirect_files_to}{path}?signed=1") + self.end_headers() + return + body, content_type = index.respond(path) + if body is None: + self.send_error(404) + return + self.send_response(200) + self.send_header("Content-Type", content_type) + self.send_header("Content-Length", str(len(body))) + self.end_headers() + self.wfile.write(body) + + def log_message(self, format: str, *args: object) -> None: + _ = format, args + + self.server = ThreadingHTTPServer(("127.0.0.1", 0), Handler) + self.url = f"http://127.0.0.1:{self.server.server_port}/simple" + threading.Thread( + target=self.server.serve_forever, kwargs={"poll_interval": POLL_SECONDS}, daemon=True + ).start() + + def respond(self, path: str) -> tuple[bytes | None, str]: + if path == f"/simple/{PKG}/": + names = sorted(p.name for p in self.files.iterdir()) + if self.content_type == self.JSON: + files = [{"filename": n, "url": f"../../files/{n}", "hashes": {}} for n in names] + return json.dumps({"name": PKG, "files": files}).encode(), self.JSON + links = "".join(f'{n}' for n in names) + return f"{links}".encode(), self.HTML + if path.startswith("/files/"): + file = self.files / path.removeprefix("/files/") + if file.is_file(): + return file.read_bytes(), "application/octet-stream" + return None, "" + + def forget_patch_module(upstream: Upstream) -> None: """Undo an in-process `mpat lock`, which the real CLI does in its own process.""" sys.modules.pop(PATCH_MODULE, None) @@ -52,6 +145,17 @@ def upstream(tmp_path: Path, monkeypatch: pytest.MonkeyPatch) -> Iterator[Upstre up.purge() +@pytest.fixture +def index(tmp_path: Path, monkeypatch: pytest.MonkeyPatch) -> Iterator[Index]: + idx = Index(tmp_path / "index") + for name in ("UV_DEFAULT_INDEX", "UV_INDEX_URL"): + monkeypatch.delenv(name, raising=False) + monkeypatch.setenv("PIP_INDEX_URL", idx.url) + monkeypatch.setenv("MPAT_CACHE_DIR", str(tmp_path / "cache")) + yield idx + idx.server.shutdown() + + @pytest.fixture(autouse=True) def _isolate(tmp_path: Path, monkeypatch: pytest.MonkeyPatch) -> Iterator[None]: before = set(sys.modules) diff --git a/tests/test_api.py b/tests/test_api.py index 1fede43..c6a2bce 100644 --- a/tests/test_api.py +++ b/tests/test_api.py @@ -572,3 +572,12 @@ def test_apply_overrides_warns_on_drift_before_assigning(upstream, tmp_path): upstream.edit("__init__.py", "LIMIT = 16", "def LIMIT():\n return 16") with pytest.warns(UpstreamDriftWarning, match="moved"), pytest.raises(UnsupportedTarget): mpat.apply_overrides() + + +def test_deleted_patch_target_still_raises_outside_collection(upstream): + upstream.edit("core.py", "def add(", "def put(") + with pytest.raises(TargetNotFound): + + @mpat.patch("fakeup.core.Store.add") + def add(original, self, item): + return original(self, item) diff --git a/tests/test_cli.py b/tests/test_cli.py index 87102f9..7b62066 100644 --- a/tests/test_cli.py +++ b/tests/test_cli.py @@ -152,7 +152,7 @@ def test_check_marks_no_source_rows(tmp_path, monkeypatch, capsys): monkeypatch.syspath_prepend(str(root)) _cli.main(["lock"]) capsys.readouterr() - assert _cli.main(["check"]) == _cli.EXIT_OK + assert _cli.main(["check", "--all"]) == _cli.EXIT_OK assert "[signature-only]" in capsys.readouterr().out @@ -209,12 +209,47 @@ def test_check_groups_rows_by_distribution(tmp_path, upstream, monkeypatch, caps monkeypatch.syspath_prepend(str(tmp_path)) _cli.main(["lock"]) capsys.readouterr() - assert _cli.main(["check"]) == _cli.EXIT_OK + assert _cli.main(["check", "--all"]) == _cli.EXIT_OK lines = capsys.readouterr().out.splitlines() assert "# packaging" in lines assert "" in lines +def test_check_hides_ok_rows_by_default(tmp_path, upstream, monkeypatch, capsys): + root = project(tmp_path, upstream) + monkeypatch.syspath_prepend(str(root)) + _cli.main(["lock"]) + capsys.readouterr() + assert _cli.main(["check"]) == _cli.EXIT_DRIFT + out = capsys.readouterr().out + assert "fakeup.core.greet" in out + assert "fakeup.REGISTRY" not in out + assert "1 ok (--all lists them)" in out.splitlines() + + +def test_check_all_lists_ok_rows(tmp_path, upstream, monkeypatch, capsys): + root = project(tmp_path, upstream) + monkeypatch.syspath_prepend(str(root)) + _cli.main(["lock"]) + capsys.readouterr() + assert _cli.main(["check", "--all"]) == _cli.EXIT_DRIFT + out = capsys.readouterr().out + assert "fakeup.REGISTRY" in out + assert "1 ok" in out.splitlines() + + +def test_check_all_ok_prints_only_the_count(tmp_path, upstream, monkeypatch, capsys): + (tmp_path / "pyproject.toml").write_text( + '[project]\nname = "x"\n[tool.mpat]\nmodules = ["app_patches"]\n' + ) + (tmp_path / "app_patches.py").write_text('import mpat\n\nmpat.watch("fakeup.core.greet")\n') + monkeypatch.syspath_prepend(str(tmp_path)) + _cli.main(["lock"]) + capsys.readouterr() + assert _cli.main(["check"]) == _cli.EXIT_OK + assert capsys.readouterr().out == "1 ok (--all lists them)\n" + + CONFIG_WATCHES = """ [[tool.mpat.watch]] target = "fakeup.core.greet" @@ -255,7 +290,7 @@ def test_config_watch_table_prints_pyproject_as_source(tmp_path, upstream, capsy config_only_project(tmp_path) _cli.main(["lock"]) capsys.readouterr() - assert _cli.main(["check"]) == _cli.EXIT_OK + assert _cli.main(["check", "--all"]) == _cli.EXIT_OK assert "pyproject.toml" in capsys.readouterr().out @@ -323,7 +358,8 @@ def test_mpat_toml_watch_locks_with_mpat_toml_as_source(tmp_path, upstream, caps assert _cli.main(["lock"]) == _cli.EXIT_OK lock = _lock.read_lock(_lock.lock_path(tmp_path)) assert lock.entries["fakeup.core.greet"].declared_in == "mpat.toml" - assert _cli.main(["check"]) == _cli.EXIT_OK + capsys.readouterr() + assert _cli.main(["check", "--all"]) == _cli.EXIT_OK assert "mpat.toml" in capsys.readouterr().out @@ -373,3 +409,210 @@ def test_diff_unlocked_target_is_a_usage_error(tmp_path, upstream, monkeypatch, monkeypatch.syspath_prepend(str(root)) assert _cli.main(["diff", "fakeup.core.greet"]) == _cli.EXIT_USAGE assert "not in mpat.lock" in capsys.readouterr().err + + +def test_check_lists_changed_fields_under_the_row(tmp_path, upstream, monkeypatch, capsys): + root = project(tmp_path, upstream) + monkeypatch.syspath_prepend(str(root)) + _cli.main(["lock"]) + capsys.readouterr() + upstream.edit("__init__.py", "LIMIT = 16", "LIMIT = 2") + upstream.edit("core.py", 'def greet(name, punct="!")', 'def greet(name, punct="?", loud=False)') + assert _cli.main(["check"]) == _cli.EXIT_DRIFT + lines = capsys.readouterr().out.splitlines() + assert " value_repr: 16 -> 2" in lines + assert any(line.startswith(" signature: (name, punct='!') -> ") for line in lines) + assert not any("source_hash" in line for line in lines) + + +def test_check_json_carries_changes(tmp_path, upstream, monkeypatch, capsys): + root = project(tmp_path, upstream) + monkeypatch.syspath_prepend(str(root)) + _cli.main(["lock"]) + capsys.readouterr() + upstream.edit("__init__.py", "LIMIT = 16", "LIMIT = 2") + _cli.main(["check", "--json"]) + by_target = {r["target"]: r for r in json.loads(capsys.readouterr().out)} + assert by_target["fakeup.LIMIT"]["changes"] == ["value_repr: 16 -> 2"] + assert by_target["fakeup.REGISTRY"]["changes"] == [] + + +def test_check_footer_for_docstring_only_drift(tmp_path, upstream, monkeypatch, capsys): + root = project(tmp_path, upstream) + monkeypatch.syspath_prepend(str(root)) + _cli.main(["lock"]) + upstream.edit( + "core.py", + 'def greet(name, punct="!"):\n', + 'def greet(name, punct="!"):\n """Say hi."""\n', + ) + capsys.readouterr() + assert _cli.main(["check"]) == _cli.EXIT_DRIFT + out = capsys.readouterr().out + assert any(line.startswith("docstring patch") for line in out.splitlines()) + assert "1 docstring-only: skim the upstream docs change, then run 'mpat lock'" in out + assert "drifted" not in out + + +def test_check_reports_a_deleted_patch_target_as_missing(tmp_path, upstream, monkeypatch, capsys): + (tmp_path / "pyproject.toml").write_text( + '[project]\nname = "x"\n[tool.mpat]\nmodules = ["app_patches"]\n' + ) + (tmp_path / "app_patches.py").write_text( + textwrap.dedent( + """ + import mpat + + @mpat.patch("fakeup.core.Store.add") + def add(original, self, item): + return original(self, item) + + mpat.watch("fakeup.core.Store.double") + """ + ) + ) + monkeypatch.syspath_prepend(str(tmp_path)) + monkeypatch.chdir(tmp_path) + _cli.main(["lock"]) + upstream.edit("core.py", "def add(", "def put(") + upstream.edit("core.py", "def double(", "def twice(") + _registry.reset() + sys.modules.pop("app_patches", None) + capsys.readouterr() + assert _cli.main(["check"]) == _cli.EXIT_DRIFT + lines = capsys.readouterr().out.splitlines() + assert any(line.startswith("missing patch fakeup.core.Store.add") for line in lines) + assert any(line.startswith("missing watch fakeup.core.Store.double") for line in lines) + assert _cli.main(["check", "--gitlab"]) == _cli.EXIT_DRIFT + issues = json.loads(capsys.readouterr().out) + assert [i["check_name"] for i in issues] == ["mpat/missing", "mpat/missing"] + + +def test_check_against_a_lock_written_before_code_hash(tmp_path, upstream, monkeypatch, capsys): + root = project(tmp_path, upstream) + monkeypatch.syspath_prepend(str(root)) + _cli.main(["lock"]) + path = _lock.lock_path(root) + path.write_text( + "".join( + line for line in path.read_text().splitlines(keepends=True) if "code_hash" not in line + ) + ) + capsys.readouterr() + assert _cli.main(["check", "--all"]) == _cli.EXIT_DRIFT + out = capsys.readouterr().out + assert "body" not in out + assert "1 ok" in out.splitlines() + + +SHARED_DEPENDENCY = """ +import mpat + + +@mpat.patch("fakeup.core.greet", depends_on=["fakeup.LIMIT"]) +def shout(original, name, punct="!"): + return original(name, punct).upper() + + +@mpat.patch("fakeup.core.Store.add", depends_on=["fakeup.LIMIT"]) +def add(original, self, item): + return original(self, item) + + +mpat.watch("fakeup.DEBUG") + + +@mpat.patch("fakeup.core.Store.double", depends_on=["fakeup.DEBUG"]) +def double(original, x): + return original(x) +""" + + +def shared_dependency_project(tmp_path: Path) -> Path: + (tmp_path / "pyproject.toml").write_text( + '[project]\nname = "x"\n[tool.mpat]\nmodules = ["app_patches"]\n' + ) + (tmp_path / "app_patches.py").write_text(SHARED_DEPENDENCY) + return tmp_path + + +def test_check_lists_every_declaration_using_a_drifted_target( + tmp_path, upstream, monkeypatch, capsys +): + root = shared_dependency_project(tmp_path) + monkeypatch.syspath_prepend(str(root)) + _cli.main(["lock"]) + upstream.edit("__init__.py", "LIMIT = 16", "LIMIT = 2") + upstream.edit("__init__.py", "DEBUG = False", "DEBUG = True") + capsys.readouterr() + assert _cli.main(["check"]) == _cli.EXIT_DRIFT + lines = capsys.readouterr().out.splitlines() + assert " used by: app_patches.py:10 (patch fakeup.core.Store.add)" in lines + assert " used by: app_patches.py:18 (patch fakeup.core.Store.double)" in lines + + +def drifted_with_index(tmp_path, upstream, index, monkeypatch) -> Path: + root = project(tmp_path, upstream) + monkeypatch.syspath_prepend(str(root)) + upstream.install_dist("1.0") + _cli.main(["lock"]) + upstream.build_wheel(index.files, "1.0") + upstream.edit("core.py", 'f"hi {name}{punct}"', 'f"hey {name}{punct}"') + upstream.install_dist("1.1") + _registry.reset() + sys.modules.pop("app_patches", None) + return root + + +def test_check_diff_shows_the_upstream_change_under_the_row( + tmp_path, upstream, index, monkeypatch, capsys +): + drifted_with_index(tmp_path, upstream, index, monkeypatch) + capsys.readouterr() + assert _cli.main(["check", "--diff"]) == _cli.EXIT_DRIFT + lines = capsys.readouterr().out.splitlines() + row = next(i for i, line in enumerate(lines) if line.startswith("body patch")) + assert lines[row + 1] == ( + " body[patch] fakeup.core.greet: the upstream body changed (1.0 -> 1.1)" + ) + assert ' 5 | - return f"hi {name}{punct}"' in lines + assert ' 5 | + return f"hey {name}{punct}"' in lines + assert " info: also used by" not in "\n".join(lines) + + +def test_check_without_diff_never_fetches(tmp_path, upstream, index, monkeypatch, capsys): + drifted_with_index(tmp_path, upstream, index, monkeypatch) + _cli.main(["check"]) + _cli.main(["check", "--json"]) + _cli.main(["check", "--json", "--diff"]) + _cli.main(["check", "--gitlab", "--diff"]) + assert index.requests == [] + + +def test_check_diff_survives_an_unreachable_index(tmp_path, upstream, index, monkeypatch, capsys): + drifted_with_index(tmp_path, upstream, index, monkeypatch) + index.server.shutdown() + index.server.server_close() + capsys.readouterr() + assert _cli.main(["check", "--diff"]) == _cli.EXIT_DRIFT + assert "info: upstream diff unavailable: cannot fetch" in capsys.readouterr().out + + +def test_diff_target_adds_the_block(tmp_path, upstream, index, monkeypatch, capsys): + drifted_with_index(tmp_path, upstream, index, monkeypatch) + capsys.readouterr() + assert _cli.main(["diff", "fakeup.core.greet"]) == _cli.EXIT_DRIFT + out = capsys.readouterr().out + assert out.startswith("status: body\n") + assert "body[patch] fakeup.core.greet: the upstream body changed (1.0 -> 1.1)" in out + + +def test_diff_without_target_shows_every_drifted_block( + tmp_path, upstream, index, monkeypatch, capsys +): + drifted_with_index(tmp_path, upstream, index, monkeypatch) + capsys.readouterr() + assert _cli.main(["diff"]) == _cli.EXIT_DRIFT + out = capsys.readouterr().out + assert out.startswith("body[patch] fakeup.core.greet:") + assert "fakeup.LIMIT" not in out diff --git a/tests/test_fingerprint.py b/tests/test_fingerprint.py index 7baf844..f5f6b9e 100644 --- a/tests/test_fingerprint.py +++ b/tests/test_fingerprint.py @@ -280,7 +280,7 @@ def test_conditional_definition_hashes_the_live_branch(upstream): nodes = [n for n in ast.walk(tree) if isinstance(n, ast.FunctionDef) and n.name == "twice"] dead, live = sorted(nodes, key=lambda n: n.lineno) assert "dead" in ast.unparse(dead) - assert fingerprint_of("fakeup.core.twice").source_hash == fp._hash(live) + assert fingerprint_of("fakeup.core.twice").source_hash == fp.hash_node(live) def test_decorated_conditional_definition_hashes_the_live_branch(upstream): @@ -289,4 +289,47 @@ def test_decorated_conditional_definition_hashes_the_live_branch(upstream): tree = ast.parse((upstream.pkg / "core.py").read_text()) nodes = [n for n in ast.walk(tree) if isinstance(n, ast.FunctionDef) and n.name == "thrice"] _, live = sorted(nodes, key=lambda n: n.lineno) - assert fingerprint_of("fakeup.core.thrice").source_hash == fp._hash(live) + assert fingerprint_of("fakeup.core.thrice").source_hash == fp.hash_node(live) + + +def test_docstring_only_change_is_its_own_status(upstream): + before = fingerprint_of("fakeup.core.greet") + upstream.edit( + "core.py", + 'def greet(name, punct="!"):\n', + 'def greet(name, punct="!"):\n """Say hi."""\n', + ) + after = fingerprint_of("fakeup.core.greet") + assert after.source_hash != before.source_hash + assert after.code_hash == before.code_hash + assert fp.compare(locked=before, current=after) == fp.DOCSTRING + + +def test_nested_docstrings_are_ignored_by_code_hash(upstream): + before = fingerprint_of("fakeup.core.Store") + upstream.edit( + "core.py", + " def add(self, item):\n", + ' def add(self, item):\n """Add one."""\n', + ) + assert fp.compare(locked=before, current=fingerprint_of("fakeup.core.Store")) == fp.DOCSTRING + + +def test_docstring_and_code_change_is_body(upstream): + before = fingerprint_of("fakeup.core.greet") + upstream.edit( + "core.py", + 'def greet(name, punct="!"):\n return f"hi', + 'def greet(name, punct="!"):\n """Say hi."""\n return f"hey', + ) + assert fp.compare(locked=before, current=fingerprint_of("fakeup.core.greet")) == fp.BODY + + +def test_lock_without_code_hash_reports_body(upstream): + before = dataclasses.replace(fingerprint_of("fakeup.core.greet"), code_hash=None) + upstream.edit( + "core.py", + 'def greet(name, punct="!"):\n', + 'def greet(name, punct="!"):\n """Say hi."""\n', + ) + assert fp.compare(locked=before, current=fingerprint_of("fakeup.core.greet")) == fp.BODY diff --git a/tests/test_gitlab.py b/tests/test_gitlab.py index f81b389..1176829 100644 --- a/tests/test_gitlab.py +++ b/tests/test_gitlab.py @@ -35,7 +35,9 @@ def test_report_lists_only_non_ok_targets(tmp_path, upstream, monkeypatch, capsy issues = report(capsys) assert [i["check_name"] for i in issues] == ["mpat/value"] issue = issues[0] - assert issue["description"] == "watch fakeup.LIMIT: the upstream value changed (issue #7)" + assert issue["description"] == ( + "watch fakeup.LIMIT: the upstream value changed (issue #7); value_repr: 16 -> 2" + ) assert issue["severity"] == "major" assert issue["location"] == {"path": "app_patches.py", "lines": {"begin": 4}} @@ -109,3 +111,66 @@ def test_json_and_gitlab_are_mutually_exclusive(tmp_path, capsys): assert exc.code == _cli.EXIT_USAGE else: raise AssertionError("expected argparse to reject both flags") + + +def test_line_is_the_declaration_not_the_first_mention(tmp_path, upstream, monkeypatch, capsys): + (tmp_path / "pyproject.toml").write_text( + '[project]\nname = "x"\n[tool.mpat]\nmodules = ["app_patches"]\n' + ) + (tmp_path / "app_patches.py").write_text( + "import mpat\n" + "\n" + "UNRELATED = 'fakeup.core.greet'\n" + "\n" + "\n" + "@mpat.patch(\n" + " 'fakeup.core.greet',\n" + " depends_on=['fakeup.LIMIT'],\n" + ")\n" + "def shout(original, name, punct='!'):\n" + " return original(name, punct).upper()\n" + ) + monkeypatch.syspath_prepend(str(tmp_path)) + monkeypatch.chdir(tmp_path) + assert _cli.main(["check", "--gitlab"]) == _cli.EXIT_DRIFT + lines = { + i["description"].split(":")[0]: i["location"]["lines"]["begin"] for i in report(capsys) + } + assert lines == {"patch fakeup.core.greet": 6, "depends_on fakeup.LIMIT": 6} + + +def test_every_user_of_a_drifted_target_gets_its_own_finding( + tmp_path, upstream, monkeypatch, capsys +): + (tmp_path / "pyproject.toml").write_text( + '[project]\nname = "x"\n[tool.mpat]\nmodules = ["app_patches"]\n' + ) + (tmp_path / "app_patches.py").write_text( + "import mpat\n" + "\n" + "\n" + "@mpat.patch('fakeup.core.greet', depends_on=['fakeup.LIMIT'])\n" + "def shout(original, name, punct='!'):\n" + " return original(name, punct)\n" + "\n" + "\n" + "@mpat.patch('fakeup.core.Store.add', depends_on=['fakeup.LIMIT'])\n" + "def add(original, self, item):\n" + " return original(self, item)\n" + ) + monkeypatch.syspath_prepend(str(tmp_path)) + monkeypatch.chdir(tmp_path) + _cli.main(["lock"]) + upstream.edit("__init__.py", "LIMIT = 16", "LIMIT = 2") + capsys.readouterr() + assert _cli.main(["check", "--gitlab"]) == _cli.EXIT_DRIFT + issues = report(capsys) + assert [(i["description"], i["location"]["lines"]["begin"]) for i in issues] == [ + ("depends_on fakeup.LIMIT: the upstream value changed; value_repr: 16 -> 2", 4), + ( + "patch fakeup.core.Store.add depends on fakeup.LIMIT: " + "the upstream value changed; value_repr: 16 -> 2", + 9, + ), + ] + assert len({i["fingerprint"] for i in issues}) == 2 diff --git a/tests/test_render.py b/tests/test_render.py new file mode 100644 index 0000000..813b67d --- /dev/null +++ b/tests/test_render.py @@ -0,0 +1,86 @@ +from mpat import _render +from mpat._lock import CheckResult, Usage +from mpat._upstream import Source + +RESULT = CheckResult( + target="somelib.client.Client.request", + role="patch", + status="body", + declared_in="myapp/patches.py", + declared_line=12, + locked_version="2.3.1", + current_version="2.5.0", + used_by=( + Usage(declared_in="myapp/retry.py", declared_line=40, role="patch", target="somelib.retry"), + ), +) +OLD = Source( + path="somelib/client.py", + start=88, + lines=( + " def request(self, method, url, **kwargs):", + " self.log(method)", + " return self._send(method, url, **kwargs)", + ), +) +NEW = Source( + path="somelib/client.py", + start=90, + lines=( + " def request(self, method, url, **kwargs):", + " self.log(method)", + " return self._send(method, url, timeout=self.timeout, **kwargs)", + ), +) + + +def test_body_block(): + assert _render.diff_block(RESULT, old=OLD, new=NEW) == [ + "body[patch] somelib.client.Client.request: the upstream body changed (2.3.1 -> 2.5.0)", + " --> somelib/client.py:92", + " |", + "88 90 | def request(self, method, url, **kwargs):", + "89 91 | self.log(method)", + "90 | - return self._send(method, url, **kwargs)", + " 92 | + return self._send(method, url, timeout=self.timeout, **kwargs)", + " |", + "info: declared here", + " --> myapp/patches.py:12", + "info: also used by myapp/retry.py:40 (patch somelib.retry)", + ] + + +def test_missing_target_shows_the_old_definition_removed(): + result = CheckResult(target="pkg.f", role="watch", status="missing", declared_in="p.py") + old = Source(path="pkg/m.py", start=9, lines=("def f():", " return 1")) + assert _render.diff_block(result, old=old, new=None) == [ + "missing[watch] pkg.f: no longer exists upstream", + " --> pkg/m.py:9", + " |", + " 9 | - def f():", + "10 | - return 1", + " |", + "info: declared here", + " --> p.py", + ] + + +def test_hunks_are_separated_and_far_context_is_dropped(): + old = Source(path="m.py", start=1, lines=tuple(f"x{i}" for i in range(12))) + new = Source(path="m.py", start=1, lines=("y0", *old.lines[1:11], "y11")) + lines = _render.diff_block(RESULT, old=old, new=new) + assert " 1 | - x0" in lines + assert " ..." in lines + assert not any(line.endswith("x5") for line in lines) + + +def test_rebuilt_archive_warns(): + old = Source(path=OLD.path, start=OLD.start, lines=OLD.lines, matches_lock=False) + assert "warning: the downloaded source does not match mpat.lock" in _render.diff_block( + RESULT, old=old, new=NEW + ) + + +def test_unavailable_upstream_is_one_info_line(): + lines = _render.diff_block(RESULT, old=None, new=NEW, unavailable="cannot fetch x") + assert lines[1] == "info: upstream diff unavailable: cannot fetch x" diff --git a/tests/test_upstream.py b/tests/test_upstream.py new file mode 100644 index 0000000..d99152f --- /dev/null +++ b/tests/test_upstream.py @@ -0,0 +1,138 @@ +import base64 + +import pytest + +from mpat import _upstream +from mpat._fingerprint import fingerprint +from mpat._lock import LockEntry +from mpat._registry import ROLE_PATCH +from mpat._targets import resolve + +GREET = "fakeup.core.greet" +OLD_RETURN = ' return f"hi {name}{punct}"' +NEW_RETURN = ' return f"hey {name}{punct}"' + + +def lock_entry(target: str) -> LockEntry: + return LockEntry( + target=target, + role=ROLE_PATCH, + declared_in="app_patches.py", + fingerprint=fingerprint(resolve(target)), + ) + + +def bump(upstream) -> None: + upstream.edit("core.py", OLD_RETURN, NEW_RETURN) + upstream.install_dist("1.1") + + +@pytest.fixture +def locked(upstream, index) -> LockEntry: + upstream.install_dist("1.0") + entry = lock_entry(GREET) + assert entry.fingerprint.dist_version == "1.0" + return entry + + +@pytest.mark.parametrize("content_type", ["json", "html"]) +def test_locked_source_from_a_wheel(upstream, index, locked, content_type): + index.content_type = index.JSON if content_type == "json" else index.HTML + upstream.build_wheel(index.files, "1.0") + bump(upstream) + source = _upstream.locked_source(locked) + assert source.path == "fakeup/core.py" + assert source.start == 4 + assert source.lines == ('def greet(name, punct="!"):', OLD_RETURN) + assert source.matches_lock + + +def test_locked_source_falls_back_to_the_sdist(upstream, index, locked): + upstream.build_sdist(index.files, "1.0") + bump(upstream) + assert _upstream.locked_source(locked).lines[-1] == OLD_RETURN + + +def test_rebuilt_archive_is_flagged(upstream, index, locked): + bump(upstream) + upstream.build_wheel(index.files, "1.0") + source = _upstream.locked_source(locked) + assert source.lines[-1] == NEW_RETURN + assert not source.matches_lock + + +def test_archive_is_cached(upstream, index, locked): + upstream.build_wheel(index.files, "1.0") + bump(upstream) + _upstream.locked_source(locked) + _upstream.locked_source(locked) + assert sum(r.startswith("/files/") for r in index.requests) == 1 + + +def test_missing_version_is_unavailable(upstream, index, locked): + upstream.build_wheel(index.files, "0.9") + with pytest.raises(_upstream.UpstreamUnavailable, match="no archive for fakeup 1.0"): + _upstream.locked_source(locked) + + +def test_unreachable_index_is_unavailable(upstream, index, locked, monkeypatch): + index.server.shutdown() + index.server.server_close() + with pytest.raises(_upstream.UpstreamUnavailable): + _upstream.locked_source(locked) + + +def test_target_without_distribution_is_unavailable(upstream, index): + entry = lock_entry(GREET) + with pytest.raises(_upstream.UpstreamUnavailable, match="no distribution"): + _upstream.locked_source(entry) + + +def test_index_credentials_are_sent_as_basic_auth(upstream, index, locked, monkeypatch): + monkeypatch.setenv("PIP_INDEX_URL", index.url.replace("http://", "http://me:s3cret@")) + upstream.build_wheel(index.files, "1.0") + _upstream.locked_source(locked) + expected = "Basic " + base64.b64encode(b"me:s3cret").decode() + assert index.headers[0]["Authorization"] == expected + + +def test_file_watch_diffs_the_whole_file(upstream, index): + (upstream.pkg / "static" / "widget.js").write_text("a\nb\n") + upstream.install_dist("1.0") + entry = lock_entry("fakeup/static/widget.js") + upstream.build_wheel(index.files, "1.0") + (upstream.pkg / "static" / "widget.js").write_text("a\nc\n") + source = _upstream.locked_source(entry) + assert source.lines == ("a", "b") + assert source.matches_lock + + +def test_installed_source(upstream): + bump(upstream) + source = _upstream.installed_source(GREET) + assert source is not None + assert (source.path, source.start, source.lines[-1]) == ("fakeup/core.py", 4, NEW_RETURN) + + +def test_installed_source_of_a_deleted_target(upstream): + upstream.edit("core.py", "def add(", "def put(") + assert _upstream.installed_source("fakeup.core.Store.add") is None + + +def test_credentials_do_not_follow_a_redirect_to_another_host(upstream, index, locked, monkeypatch): + monkeypatch.setenv("PIP_INDEX_URL", index.url.replace("http://", "http://me:s3cret@")) + index.redirect_files_to = f"http://localhost:{index.server.server_port}" + upstream.build_wheel(index.files, "1.0") + bump(upstream) + assert _upstream.locked_source(locked).lines[-1] == OLD_RETURN + by_path = dict(zip(index.requests, index.headers, strict=True)) + assert "Authorization" in by_path["/files/fakeup-1.0-py3-none-any.whl"] + assert "Authorization" not in by_path["/files/fakeup-1.0-py3-none-any.whl?signed=1"] + + +def test_unwritable_cache_is_unavailable(upstream, index, locked, monkeypatch, tmp_path): + (tmp_path / "blocker").write_text("") + monkeypatch.setenv("MPAT_CACHE_DIR", str(tmp_path / "blocker" / "cache")) + upstream.build_wheel(index.files, "1.0") + with pytest.raises(_upstream.UpstreamUnavailable, match="cannot cache"): + _upstream.locked_source(locked) diff --git a/tests/test_version.py b/tests/test_version.py index 7f478f4..6ce1be4 100644 --- a/tests/test_version.py +++ b/tests/test_version.py @@ -6,4 +6,4 @@ def test_version_matches_pyproject(): data = tomllib.loads((Path(__file__).parents[1] / "pyproject.toml").read_text()) - assert data["project"]["version"] == mpat.__version__ == "0.3.1" + assert data["project"]["version"] == mpat.__version__ == "0.3.2" diff --git a/uv.lock b/uv.lock index ea1890f..817fe18 100644 --- a/uv.lock +++ b/uv.lock @@ -412,7 +412,7 @@ wheels = [ [[package]] name = "mpat" -version = "0.3.1" +version = "0.3.2" source = { editable = "." } dependencies = [ { name = "packaging" },