diff --git a/crates/pm/src/cmd/view.rs b/crates/pm/src/cmd/view.rs index d8355e9922..e5bfaad79b 100644 --- a/crates/pm/src/cmd/view.rs +++ b/crates/pm/src/cmd/view.rs @@ -27,10 +27,8 @@ pub async fn view(package_spec: &str) -> Result<()> { tracing::debug!("Fetched package info: {full_manifest:?}"); // Resolve version and get full VersionManifest (with all display fields) - let version_list: Vec = full_manifest.versions.clone(); let resolved_version = utoo_ruborist::resolver::version::resolve_target_version( - &full_manifest.dist_tags, - &version_list, + (&full_manifest).into(), version_spec, ) .map_err(|e| { diff --git a/crates/ruborist/src/lib.rs b/crates/ruborist/src/lib.rs index 0ea3b3b42f..3ccd6937c5 100644 --- a/crates/ruborist/src/lib.rs +++ b/crates/ruborist/src/lib.rs @@ -41,7 +41,9 @@ pub mod graph { /// Package manifest types. pub mod manifest { - pub use crate::model::manifest::{CoreVersionManifest, Dist, FullManifest, VersionManifest}; + pub use crate::model::manifest::{ + CoreVersionManifest, Dist, FullManifest, VersionManifest, VersionsRef, + }; pub use crate::model::package_json::{ DepsView, EnginesView, IdentityView, PackageInstallView, PackageJson, PublishConfig, ScriptsView, diff --git a/crates/ruborist/src/model/manifest.rs b/crates/ruborist/src/model/manifest.rs index dea63dc7bc..e56168ee05 100644 --- a/crates/ruborist/src/model/manifest.rs +++ b/crates/ruborist/src/model/manifest.rs @@ -9,6 +9,44 @@ use std::sync::Arc; use serde::{Deserialize, Deserializer, Serialize}; use serde_json::Value; +/// Borrowed view of the data needed to resolve a version spec — a slice of +/// available versions plus a dist-tag map. +/// +/// The version-resolution logic ([`crate::resolver::version::resolve_target_version`]) +/// only needs read access to these two pieces of data; everything else on a +/// `FullManifest` (raw bytes, time map, maintainers, …) is irrelevant. By +/// borrowing them through a unified view we can serve the same resolver from +/// multiple in-memory shapes (a freshly-fetched `FullManifest`, a 304-cached +/// `VersionsInfo`, a disk-loaded `Versions`) without cloning data or +/// duplicating the resolution code. +/// +/// The lifetime parameter ties the view to whatever the caller is holding, +/// so the borrow checker statically rejects any attempt to keep the view +/// alive past its source. In practice every call site uses the view inside +/// a single function body — the lifetime never escapes. +/// +/// Construct via the `From` impls (defined alongside each source type): +/// ```ignore +/// // From a freshly-fetched manifest: +/// let view = VersionsRef::from(&*full_manifest); +/// // From the 304-path versions cache: +/// let view = VersionsRef::from(&*versions_info); +/// ``` +#[derive(Debug, Clone, Copy)] +pub struct VersionsRef<'a> { + pub versions: &'a [String], + pub dist_tags: &'a HashMap, +} + +impl<'a> From<&'a FullManifest> for VersionsRef<'a> { + fn from(m: &'a FullManifest) -> Self { + Self { + versions: &m.versions, + dist_tags: &m.dist_tags, + } + } +} + /// Skip on error - try to deserialize, return None if fails. /// This handles malformed npm registry data gracefully. fn skip_on_error<'de, D, T>(deserializer: D) -> Result, D::Error> diff --git a/crates/ruborist/src/resolver/registry.rs b/crates/ruborist/src/resolver/registry.rs index 6edeb9069c..145923160c 100644 --- a/crates/ruborist/src/resolver/registry.rs +++ b/crates/ruborist/src/resolver/registry.rs @@ -137,14 +137,12 @@ pub fn resolve_from_manifest( manifest: &FullManifest, spec: &str, ) -> Result> { - let version_list: Vec = manifest.versions.clone(); - - if version_list.is_empty() { + if manifest.versions.is_empty() { return Err(ResolveError::NoVersions(manifest.name.clone())); } // Resolve version using shared logic - let resolved_version = resolve_target_version(&manifest.dist_tags, &version_list, spec) + let resolved_version = resolve_target_version(manifest.into(), spec) .map_err(|e| ResolveError::Version(format!("{}@{}: {}", manifest.name, spec, e)))?; // Get manifest for resolved version (lazy: parse from Value on demand) diff --git a/crates/ruborist/src/resolver/version.rs b/crates/ruborist/src/resolver/version.rs index 63c6046abe..8ca56bd8a7 100644 --- a/crates/ruborist/src/resolver/version.rs +++ b/crates/ruborist/src/resolver/version.rs @@ -1,10 +1,10 @@ //! Version resolution from dist-tags and version lists. -use std::collections::HashMap; +use crate::model::manifest::VersionsRef; use super::semver::{matches, max_satisfying}; -/// Resolve target version from dist-tags and version list. +/// Resolve target version from a [`VersionsRef`] view. /// /// This is the core version resolution logic that matches npm's behavior: /// 1. If spec is a dist-tag, return the tagged version @@ -12,13 +12,14 @@ use super::semver::{matches, max_satisfying}; /// 3. Otherwise, find the maximum satisfying version /// /// # Arguments -/// * `dist_tags` - Map of tag names to versions (e.g., {"latest": "1.2.3"}) -/// * `version_list` - List of all available versions +/// * `view` - Borrowed view of the versions list + dist-tag map. Construct via +/// `VersionsRef::from(&full_manifest)` or `VersionsRef::from(&versions_info)`. /// * `spec` - Version specification to resolve /// /// # Examples /// ``` /// use std::collections::HashMap; +/// use utoo_ruborist::manifest::VersionsRef; /// use utoo_ruborist::resolver::version::resolve_target_version; /// /// let mut dist_tags = HashMap::new(); @@ -27,15 +28,17 @@ use super::semver::{matches, max_satisfying}; /// let versions = vec!["1.0.0".to_string(), "1.2.3".to_string(), "1.5.0".to_string()]; /// /// // Prefer latest when it satisfies the spec -/// let result = resolve_target_version(&dist_tags, &versions, "^1.0.0"); +/// let view = VersionsRef { versions: &versions, dist_tags: &dist_tags }; +/// let result = resolve_target_version(view, "^1.0.0"); /// assert_eq!(result, Ok("1.2.3".to_string())); /// ``` -pub fn resolve_target_version( - dist_tags: &HashMap, - version_list: &[String], - spec: &str, -) -> Result { - if version_list.is_empty() { +pub fn resolve_target_version(view: VersionsRef<'_>, spec: &str) -> Result { + let VersionsRef { + versions, + dist_tags, + } = view; + + if versions.is_empty() { return Err(ResolveError::NoVersionsAvailable); } @@ -54,12 +57,12 @@ pub fn resolve_target_version( latest.to_string() }) .or_else(|| { - max_satisfying(version_list.iter().map(|s| s.as_str()), spec).map(|v| v.to_string()) + max_satisfying(versions.iter().map(|s| s.as_str()), spec).map(|v| v.to_string()) }); version.ok_or_else(|| ResolveError::NoMatchingVersion { spec: spec.to_string(), - available_count: version_list.len(), + available_count: versions.len(), }) } @@ -95,8 +98,17 @@ impl std::error::Error for ResolveError {} #[cfg(test)] mod tests { + use std::collections::HashMap; + use super::*; + fn view<'a>(versions: &'a [String], dist_tags: &'a HashMap) -> VersionsRef<'a> { + VersionsRef { + versions, + dist_tags, + } + } + #[test] fn test_resolve_dist_tag() { let mut dist_tags = HashMap::new(); @@ -111,11 +123,11 @@ mod tests { // Resolve dist-tag directly assert_eq!( - resolve_target_version(&dist_tags, &versions, "latest"), + resolve_target_version(view(&versions, &dist_tags), "latest"), Ok("1.2.3".to_string()) ); assert_eq!( - resolve_target_version(&dist_tags, &versions, "beta"), + resolve_target_version(view(&versions, &dist_tags), "beta"), Ok("2.0.0-beta.1".to_string()) ); } @@ -133,7 +145,7 @@ mod tests { // Should prefer latest (1.5.0) over max_satisfying (1.9.0) assert_eq!( - resolve_target_version(&dist_tags, &versions, "^1.0.0"), + resolve_target_version(view(&versions, &dist_tags), "^1.0.0"), Ok("1.5.0".to_string()) ); } @@ -151,7 +163,7 @@ mod tests { // latest doesn't satisfy ^2.0.0, should use max_satisfying (2.1.0) assert_eq!( - resolve_target_version(&dist_tags, &versions, "^2.0.0"), + resolve_target_version(view(&versions, &dist_tags), "^2.0.0"), Ok("2.1.0".to_string()) ); } @@ -161,7 +173,7 @@ mod tests { let dist_tags = HashMap::new(); let versions = vec!["1.0.0".to_string()]; - let result = resolve_target_version(&dist_tags, &versions, "^2.0.0"); + let result = resolve_target_version(view(&versions, &dist_tags), "^2.0.0"); assert!(result.is_err()); assert!(matches!( result, @@ -174,7 +186,7 @@ mod tests { let dist_tags = HashMap::new(); let versions: Vec = vec![]; - let result = resolve_target_version(&dist_tags, &versions, "^1.0.0"); + let result = resolve_target_version(view(&versions, &dist_tags), "^1.0.0"); assert_eq!(result, Err(ResolveError::NoVersionsAvailable)); } } diff --git a/crates/ruborist/src/service/cache.rs b/crates/ruborist/src/service/cache.rs index 7587b04f87..a983f4e447 100644 --- a/crates/ruborist/src/service/cache.rs +++ b/crates/ruborist/src/service/cache.rs @@ -41,7 +41,7 @@ use std::sync::{Arc, LazyLock}; use dashmap::DashMap; use serde::{Deserialize, Serialize}; -use crate::model::manifest::{CoreVersionManifest, FullManifest}; +use crate::model::manifest::{CoreVersionManifest, FullManifest, VersionsRef}; /// Lightweight versions info, persisted by `ManifestStore` for ETag validation. #[derive(Debug, Clone, Serialize, Deserialize)] @@ -51,6 +51,12 @@ pub struct VersionsInfo { pub last_updated: u64, } +impl<'a> From<&'a VersionsInfo> for VersionsRef<'a> { + fn from(info: &'a VersionsInfo) -> Self { + VersionsRef::from(&info.versions) + } +} + /// Version list and dist-tags. #[derive(Debug, Clone, Serialize, Deserialize)] pub struct Versions { @@ -59,6 +65,15 @@ pub struct Versions { pub dist_tags: HashMap, } +impl<'a> From<&'a Versions> for VersionsRef<'a> { + fn from(v: &'a Versions) -> Self { + Self { + versions: &v.version_list, + dist_tags: &v.dist_tags, + } + } +} + // ============================================================================ // Memory cache (lock-free reads via DashMap) // ============================================================================ diff --git a/crates/ruborist/src/service/registry.rs b/crates/ruborist/src/service/registry.rs index 0022a72e5e..2e0f6b720f 100644 --- a/crates/ruborist/src/service/registry.rs +++ b/crates/ruborist/src/service/registry.rs @@ -245,6 +245,15 @@ impl UnifiedRegistry { .map_err(RegistryError)? { manifest::FetchManifestResult::Ok(manifest, new_etag) => { + // Build a `VersionsInfo` strictly for the disk-persist + // task. We do NOT also fill the in-memory + // `versions_info` cache slot on the 200 path: readers + // (`resolve_via_full_manifest::Full`) now go through + // `VersionsRef::from(&Arc)` for this + // case, so the `full_manifests` slot is the single + // source of truth in memory. The `versions_info` slot + // is reserved for the 304 path (or disk-loaded warm + // cache from a previous run). let versions_info = Arc::new(VersionsInfo { versions: Versions { version_list: manifest.versions.clone(), @@ -255,8 +264,6 @@ impl UnifiedRegistry { }); self.cache .set_full_manifest(name.to_string(), Arc::new(manifest)); - self.cache - .set_versions(name.to_string(), Arc::clone(&versions_info)); // Fire-and-forget: store may spawn its own task. self.store.store_versions(name, versions_info); } @@ -277,6 +284,10 @@ impl UnifiedRegistry { .await .map_err(RegistryError)?; + // Same shape as the 200 branch: only the + // canonical `full_manifests` slot is filled in + // memory; the disk-persist task gets its own + // `Arc`. let versions_info = Arc::new(VersionsInfo { versions: Versions { version_list: manifest.versions.clone(), @@ -287,8 +298,6 @@ impl UnifiedRegistry { }); self.cache .set_full_manifest(name.to_string(), Arc::new(manifest)); - self.cache - .set_versions(name.to_string(), Arc::clone(&versions_info)); self.store.store_versions(name, versions_info); } } @@ -432,9 +441,8 @@ impl UnifiedRegistry { if full.versions.is_empty() { return Err(RegistryError(anyhow!("No versions available for {}", name))); } - let resolved_version = - resolve_target_version(&full.dist_tags, &full.versions, spec) - .map_err(|e| RegistryError(anyhow!("{}@{}: {}", name, spec, e)))?; + let resolved_version = resolve_target_version((&*full).into(), spec) + .map_err(|e| RegistryError(anyhow!("{}@{}: {}", name, spec, e)))?; let core = full.get_core_version(&resolved_version).ok_or_else(|| { RegistryError(anyhow!( "Version {} not found in manifest for {}", @@ -454,12 +462,8 @@ impl UnifiedRegistry { let versions_info = self.cache.get_versions(name).ok_or_else(|| { RegistryError(anyhow!("Versions cache not found for {}", name)) })?; - let resolved_version = resolve_target_version( - &versions_info.versions.dist_tags, - &versions_info.versions.version_list, - spec, - ) - .map_err(|e| RegistryError(anyhow!("{}@{}: {}", name, spec, e)))?; + let resolved_version = resolve_target_version((&*versions_info).into(), spec) + .map_err(|e| RegistryError(anyhow!("{}@{}: {}", name, spec, e)))?; let manifest = manifest::fetch_version_manifest(manifest::FetchVersionManifestOptions { registry_url: &self.registry_url, diff --git a/crates/ruborist/src/traits/registry.rs b/crates/ruborist/src/traits/registry.rs index df8adac00f..48e29ad920 100644 --- a/crates/ruborist/src/traits/registry.rs +++ b/crates/ruborist/src/traits/registry.rs @@ -157,10 +157,9 @@ pub trait RegistryClient { ) -> impl Future, Self::Error>> { async move { let manifest = self.fetch_full_manifest(name).await?; - let version_list: Vec = manifest.versions.clone(); // Resolve version using shared logic - let resolved_version = resolve_target_version(&manifest.dist_tags, &version_list, spec) + let resolved_version = resolve_target_version((&*manifest).into(), spec) .map_err(|e| RegistryError(anyhow::anyhow!("{}@{}: {}", name, spec, e)))?; manifest @@ -227,9 +226,8 @@ pub trait RegistryClient { fetch_spec ); let full_manifest = self.fetch_full_manifest(&fetch_name).await?; - let version_list: Vec = full_manifest.versions.clone(); - if version_list.is_empty() { + if full_manifest.versions.is_empty() { return Err(RegistryError(anyhow::anyhow!( "No versions available for {}", fetch_name @@ -238,7 +236,7 @@ pub trait RegistryClient { } let resolved_version = - resolve_target_version(&full_manifest.dist_tags, &version_list, &fetch_spec) + resolve_target_version((&*full_manifest).into(), &fetch_spec) .map_err(|e| RegistryError(anyhow::anyhow!("{}@{}: {}", name, spec, e)))?; let version_manifest = full_manifest