From 676e42968e06d474108119db3a8d63bfe15d82a7 Mon Sep 17 00:00:00 2001 From: Daniel Yudelevich <4537920+yudelevi@users.noreply.github.com> Date: Wed, 7 Oct 2026 17:11:09 -0700 Subject: [PATCH] Make mpat check show what upstream changed, and where it matters A CI log for a dependency bump buried the one drifted patch in a table of every watched symbol, and the drift itself said little more than "body", so the reviewer still had to dig up the upstream change by hand. Hide ok rows behind a count (--all restores them), print the changed fields under each row and in the GitLab description, and anchor GitLab findings to the declaring call rather than the first string that happens to quote the target. mpat diff and check --diff now render the upstream source change in the style of a ty diagnostic. The lock keeps hashes only, so the locked side is downloaded from the package index the environment already uses and cached; nothing upstream gets committed. Index credentials are only ever sent to the index host, because private registries redirect downloads to signed object-storage URLs. Fetch failures degrade to a one-line notice and never change the exit code. A shared depends_on used to report its drift once, under whichever patch declared it first. Every declaration using a drifted target is now listed, and each gets its own GitLab finding. A symbol deleted upstream used to abort collection with exit 2 and an empty Code Quality report. Collection now defers the lookup and check() reports it as missing; outside collection it still raises. Docstring-only upstream edits get their own status so they are not triaged like code changes. The lock gains an optional code_hash; old locks keep reporting body until relocked, so the format version stays 1. --- CHANGELOG.md | 34 ++++ README.md | 9 +- docs/site/pages/cli.md | 98 +++++++++-- docs/site/pages/lockfile.md | 1 + pyproject.toml | 2 +- src/mpat/__init__.py | 2 +- src/mpat/_api.py | 27 ++- src/mpat/_cli.py | 123 ++++++++++--- src/mpat/_fingerprint.py | 80 +++++++-- src/mpat/_gitlab.py | 73 ++++++-- src/mpat/_lock.py | 41 ++++- src/mpat/_registry.py | 1 + src/mpat/_render.py | 107 ++++++++++++ src/mpat/_upstream.py | 337 ++++++++++++++++++++++++++++++++++++ tests/conftest.py | 104 +++++++++++ tests/test_api.py | 9 + tests/test_cli.py | 251 ++++++++++++++++++++++++++- tests/test_fingerprint.py | 47 ++++- tests/test_gitlab.py | 67 ++++++- tests/test_render.py | 86 +++++++++ tests/test_upstream.py | 138 +++++++++++++++ tests/test_version.py | 2 +- uv.lock | 2 +- 23 files changed, 1544 insertions(+), 97 deletions(-) create mode 100644 src/mpat/_render.py create mode 100644 src/mpat/_upstream.py create mode 100644 tests/test_render.py create mode 100644 tests/test_upstream.py 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" },