diff --git a/CHANGELOG.md b/CHANGELOG.md index a0567905b..4fd62ad31 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -7,6 +7,17 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ## [Unreleased] +### Fixed + +- `apm update` now re-checks a transitive dependency's own semver range + against the remote at any depth, not just for direct dependencies. + Previously, `download_callback` only ran for a dependency whose install + path didn't already exist, so a transitive dependency already present on + disk (e.g. `pkg1 -> pkg2 -> pkg3`, all constrained by `^1.0.0`) never had + its own range re-evaluated -- publishing a new matching version of `pkg3` + was silently ignored by `apm update` in `pkg1` even though `pkg2`'s + manifest allowed it. (by @nadav-y) + ### Added - Corporate proxy and internal-CA users can now use Python-based APM HTTPS paths diff --git a/src/apm_cli/deps/apm_resolver.py b/src/apm_cli/deps/apm_resolver.py index 14d18cf8c..0cd51572a 100644 --- a/src/apm_cli/deps/apm_resolver.py +++ b/src/apm_cli/deps/apm_resolver.py @@ -80,6 +80,7 @@ def __init__( download_callback: DownloadCallback | None = None, max_parallel: int | None = None, auth_resolver: object | None = None, + update_refs: bool = False, ): """Initialize the resolver with maximum recursion depth. @@ -97,11 +98,18 @@ def __init__( for parity-testing against the legacy sequential path -- this is a diagnostic knob, not a user toggle. auth_resolver: Optional auth resolver for marketplace dependency resolution. + update_refs: True for ``apm update`` / ``--refresh``. When set, + ``_should_force_recheck`` gives ``download_callback`` a + chance to run for a semver-ranged dep even when its + install path already exists, at any depth -- see that + method's docstring for why this can't be scoped to direct + deps only. """ self.max_depth = max_depth self._apm_modules_dir: Path | None = apm_modules_dir self._project_root: Path | None = None self._download_callback = download_callback + self._update_refs = update_refs # Whether ``download_callback`` accepts ``parent_pkg`` (added in #857). # Detected once via signature inspection so legacy callbacks that # predate the field still work without raising a silent TypeError @@ -944,6 +952,29 @@ def _load_work_item(self, item): except (ValueError, FileNotFoundError) as exc: return (item, None, exc) + def _should_force_recheck(self, dep_ref: DependencyReference) -> bool: + """True when *dep_ref* must reach ``download_callback`` even though + its install path already exists on disk. + + ``download_callback`` (resolve.py) already has a + ``_force_semver_resolve`` fallthrough for this case, but this + method's caller (``_try_load_dependency_package``) only invokes the + callback when ``not install_path.exists()`` -- so that fallthrough + never fires for a dep whose path is already there. The existing + pre-purge (``_purge_cached_semver_paths_for_update``) works around + this for **direct** deps by deleting their install path up front, + but a transitive dep's path can't be pre-purged (its existence + isn't known until its parent's manifest is read, mid-resolution). + Widening this gate instead covers any depth uniformly. Mirrors + ``_force_semver_resolve`` exactly -- keep the two in sync. + """ + return ( + self._update_refs + and not dep_ref.is_local + and not getattr(dep_ref, "artifactory_prefix", None) + and getattr(dep_ref, "ref_kind", None) == "semver" + ) + def _try_load_dependency_package( self, dep_ref: DependencyReference, @@ -996,8 +1027,9 @@ def _try_load_dependency_package( # Get the canonical install path for this dependency install_path = dep_ref.get_install_path(self._apm_modules_dir) - # If package doesn't exist locally, try to download it - if not install_path.exists(): + # If package doesn't exist locally, try to download it. Also fall + # through for a forced semver re-check (see _should_force_recheck). + if not install_path.exists() or self._should_force_recheck(dep_ref): if self._download_callback is not None: unique_key = self._download_dedup_key(dep_ref, parent_pkg) # Avoid re-downloading the same logical (dep_ref, anchor) pair diff --git a/src/apm_cli/install/phases/resolve.py b/src/apm_cli/install/phases/resolve.py index d95b9721a..601cc45e6 100644 --- a/src/apm_cli/install/phases/resolve.py +++ b/src/apm_cli/install/phases/resolve.py @@ -196,11 +196,10 @@ def _purge_cached_semver_paths_for_update( and rewrites the lockfile with the latest matching tag. Matches npm / cargo / bundler: ``--update`` is the explicit re-resolve trigger and must not be swallowed by the on-disk cache. Scoped to - direct deps to avoid disturbing transitive cached content; the - resolver re-walks transitives naturally once a direct dep's - callback rewrites its ref. Local and proxy deps are excluded (their - semver semantics belong to a different resolver path). Registry semver - deps are included: their callback also gates on install_path.exists(). + direct deps -- a transitive dep's own range is covered separately by + ``APMDependencyResolver._should_force_recheck``. Local and proxy deps + are excluded (different resolver path). Registry semver deps are + included: their callback also gates on install_path.exists(). """ from contextlib import suppress @@ -770,6 +769,7 @@ def download_callback(dep_ref, modules_dir, parent_chain="", parent_pkg=None): apm_modules_dir=ctx.apm_modules_dir, download_callback=download_callback, auth_resolver=ctx.auth_resolver, + update_refs=update_refs, ) # Resolver reads ``/apm.yml``. Preserve the original diff --git a/tests/unit/deps/test_apm_resolver_edge_cases.py b/tests/unit/deps/test_apm_resolver_edge_cases.py index ca9fc3281..ef23bbef3 100644 --- a/tests/unit/deps/test_apm_resolver_edge_cases.py +++ b/tests/unit/deps/test_apm_resolver_edge_cases.py @@ -642,3 +642,142 @@ def legacy_cb(dep_ref, modules_dir, parent_chain=""): resolver = APMDependencyResolver(apm_modules_dir=mods, download_callback=legacy_cb) resolver._try_load_dependency_package(ref) assert len(call_log) == 1 + + +class TestShouldForceRecheck: + """``_should_force_recheck`` decides whether download_callback gets a + chance to run for a dep whose install path already exists on disk -- + the fix for transitive semver deps never being re-evaluated against the + remote during ``apm update`` unless something *above* them in the tree + also happened to change. Mirrors download_callback's own + ``_force_semver_resolve`` predicate (see install/phases/resolve.py) + exactly -- kept in sync deliberately, since without this widened gate + that check is otherwise unreachable for any dep whose path exists.""" + + def _dep(self, *, is_local=False, artifactory_prefix=None, ref_kind="semver"): + ref = MagicMock() + ref.is_local = is_local + ref.artifactory_prefix = artifactory_prefix + ref.ref_kind = ref_kind + return ref + + def test_false_when_update_refs_is_false(self) -> None: + resolver = APMDependencyResolver(update_refs=False) + assert resolver._should_force_recheck(self._dep()) is False + + def test_true_for_semver_dep_when_update_refs_true(self) -> None: + resolver = APMDependencyResolver(update_refs=True) + assert resolver._should_force_recheck(self._dep()) is True + + def test_false_for_literal_ref_kind(self) -> None: + """An exact/literal ref (e.g. a pinned tag) has nowhere else to + resolve to -- no point re-checking it.""" + resolver = APMDependencyResolver(update_refs=True) + assert resolver._should_force_recheck(self._dep(ref_kind="literal")) is False + + def test_false_for_none_ref_kind(self) -> None: + resolver = APMDependencyResolver(update_refs=True) + assert resolver._should_force_recheck(self._dep(ref_kind=None)) is False + + def test_false_for_local_dep(self) -> None: + resolver = APMDependencyResolver(update_refs=True) + assert resolver._should_force_recheck(self._dep(is_local=True)) is False + + def test_false_for_artifactory_proxied_dep(self) -> None: + resolver = APMDependencyResolver(update_refs=True) + assert resolver._should_force_recheck(self._dep(artifactory_prefix="proxy")) is False + + def test_default_constructor_is_update_refs_false(self) -> None: + """Callers that don't pass update_refs (e.g. plain apm install) + must not accidentally force re-checks.""" + resolver = APMDependencyResolver() + assert resolver._should_force_recheck(self._dep()) is False + + +class TestTryLoadDependencyPackageForceRecheck: + """End-to-end through _try_load_dependency_package: an existing install + path must still invoke download_callback when _should_force_recheck is + True -- this is what makes a transitive dependency's own semver range + get re-evaluated during apm update, not just direct dependencies.""" + + def _semver_dep_ref(self, install_path: Path, key: str = "org/pkg"): + ref = MagicMock() + ref.is_local = False + ref.local_path = None + ref.artifactory_prefix = None + ref.ref_kind = "semver" + ref.repo_url = key + ref.get_install_path.return_value = install_path + ref.get_unique_key.return_value = key + ref.get_display_name.return_value = key + return ref + + def test_existing_path_calls_callback_when_update_refs_true(self, tmp_path: Path) -> None: + mods = tmp_path / "apm_modules" + pkg_dir = mods / "org" / "pkg" + pkg_dir.mkdir(parents=True) + (pkg_dir / "apm.yml").write_text("name: pkg\nversion: 1.0.0\n") + + call_log: list = [] + + def cb(dep_ref, modules_dir, parent_chain="", parent_pkg=None): + call_log.append(dep_ref) + return pkg_dir # unchanged -- still exists, same content + + ref = self._semver_dep_ref(pkg_dir) + resolver = APMDependencyResolver( + apm_modules_dir=mods, download_callback=cb, update_refs=True + ) + result = resolver._try_load_dependency_package(ref) + + assert len(call_log) == 1 + assert result is not None + + def test_existing_path_skips_callback_when_update_refs_false(self, tmp_path: Path) -> None: + """Plain apm install (update_refs=False, the default) must not pay + the re-check cost -- this is the existing, unaffected fast path.""" + mods = tmp_path / "apm_modules" + pkg_dir = mods / "org" / "pkg" + pkg_dir.mkdir(parents=True) + (pkg_dir / "apm.yml").write_text("name: pkg\nversion: 1.0.0\n") + + call_log: list = [] + + def cb(dep_ref, modules_dir, parent_chain="", parent_pkg=None): + call_log.append(dep_ref) + return pkg_dir + + ref = self._semver_dep_ref(pkg_dir) + resolver = APMDependencyResolver( + apm_modules_dir=mods, download_callback=cb, update_refs=False + ) + result = resolver._try_load_dependency_package(ref) + + assert len(call_log) == 0 + assert result is not None + + def test_existing_path_skips_callback_for_literal_ref_even_with_update_refs( + self, tmp_path: Path + ) -> None: + """A dep pinned to a literal ref (not a semver range) has nothing to + re-resolve to -- re-checking it would be pure waste.""" + mods = tmp_path / "apm_modules" + pkg_dir = mods / "org" / "pkg" + pkg_dir.mkdir(parents=True) + (pkg_dir / "apm.yml").write_text("name: pkg\nversion: 1.0.0\n") + + call_log: list = [] + + def cb(dep_ref, modules_dir, parent_chain="", parent_pkg=None): + call_log.append(dep_ref) + return pkg_dir + + ref = self._semver_dep_ref(pkg_dir) + ref.ref_kind = "literal" + resolver = APMDependencyResolver( + apm_modules_dir=mods, download_callback=cb, update_refs=True + ) + result = resolver._try_load_dependency_package(ref) + + assert len(call_log) == 0 + assert result is not None