Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 1 addition & 3 deletions crates/pm/src/cmd/view.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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<String> = 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| {
Expand Down
4 changes: 3 additions & 1 deletion crates/ruborist/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down
38 changes: 38 additions & 0 deletions crates/ruborist/src/model/manifest.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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<String, String>,
}

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<Option<T>, D::Error>
Expand Down
6 changes: 2 additions & 4 deletions crates/ruborist/src/resolver/registry.rs
Original file line number Diff line number Diff line change
Expand Up @@ -137,14 +137,12 @@ pub fn resolve_from_manifest<E: std::error::Error + 'static>(
manifest: &FullManifest,
spec: &str,
) -> Result<ResolvedPackage, ResolveError<E>> {
let version_list: Vec<String> = 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)
Expand Down
50 changes: 31 additions & 19 deletions crates/ruborist/src/resolver/version.rs
Original file line number Diff line number Diff line change
@@ -1,24 +1,25 @@
//! 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
/// 2. If 'latest' dist-tag satisfies the spec, prefer it (npm behavior)
/// 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();
Expand All @@ -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<String, String>,
version_list: &[String],
spec: &str,
) -> Result<String, ResolveError> {
if version_list.is_empty() {
pub fn resolve_target_version(view: VersionsRef<'_>, spec: &str) -> Result<String, ResolveError> {
let VersionsRef {
versions,
dist_tags,
} = view;

if versions.is_empty() {
return Err(ResolveError::NoVersionsAvailable);
}

Expand All @@ -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(),
})
}

Expand Down Expand Up @@ -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<String, String>) -> VersionsRef<'a> {
VersionsRef {
versions,
dist_tags,
}
}

#[test]
fn test_resolve_dist_tag() {
let mut dist_tags = HashMap::new();
Expand All @@ -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())
);
}
Expand All @@ -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())
);
}
Expand All @@ -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())
);
}
Expand All @@ -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,
Expand All @@ -174,7 +186,7 @@ mod tests {
let dist_tags = HashMap::new();
let versions: Vec<String> = 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));
}
}
17 changes: 16 additions & 1 deletion crates/ruborist/src/service/cache.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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)]
Expand All @@ -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 {
Expand All @@ -59,6 +65,15 @@ pub struct Versions {
pub dist_tags: HashMap<String, String>,
}

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)
// ============================================================================
Expand Down
30 changes: 17 additions & 13 deletions crates/ruborist/src/service/registry.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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<FullManifest>)` 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(),
Expand All @@ -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);
}
Expand All @@ -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<VersionsInfo>`.
let versions_info = Arc::new(VersionsInfo {
versions: Versions {
version_list: manifest.versions.clone(),
Expand All @@ -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);
}
}
Expand Down Expand Up @@ -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 {}",
Expand All @@ -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,
Expand Down
8 changes: 3 additions & 5 deletions crates/ruborist/src/traits/registry.rs
Original file line number Diff line number Diff line change
Expand Up @@ -157,10 +157,9 @@ pub trait RegistryClient {
) -> impl Future<Output = Result<Arc<CoreVersionManifest>, Self::Error>> {
async move {
let manifest = self.fetch_full_manifest(name).await?;
let version_list: Vec<String> = 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
Expand Down Expand Up @@ -227,9 +226,8 @@ pub trait RegistryClient {
fetch_spec
);
let full_manifest = self.fetch_full_manifest(&fetch_name).await?;
let version_list: Vec<String> = 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
Expand All @@ -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
Expand Down
Loading