Repository: denoland/deno
Pull request: fix(npm): don't panic when peer dep is not in snapshot

Instead of panicking, create the missing node gracefully.

Fixes https://github.com/denoland/deno/issues/32443

Changed files:
filename: libs/npm/resolution/graph.rs
patch: @@ -428,10 +428,10 @@ impl Graph {
           //   package-b@1.0.0_package-c@1.0.0__package-b@1.0.0
           //                                    ^ attempting to resolve this
           // In this case, we go up the ancestors to see if we can find a matching nv.
-          let id = ancestor_ids
+          let maybe_id = ancestor_ids
             .iter()
             .find(|id| id.nv == pkg_id.nv && packages.contains_key(id))
-            .unwrap_or_else(|| {
+            .or_else(|| {
               // If a matching nv is not found in the ancestors, then we fall
               // back to searching the entire collection of packages for a matching
               // nv and just select the first one even though that might not be exactly
@@ -442,15 +442,22 @@ impl Graph {
                 .collect::<Vec<_>>();
               packages_with_same_nv.sort();
               if packages_with_same_nv.is_empty() {
-                // we verify in other places that references in a snapshot
-                // should be valid (ex. when creating a snapshot), so we should
-                // never get here and if so that indicates a bug elsewhere
-                panic!("not found package id: {}", pkg_id.as_serialized());
+                None
               } else {
-                packages_with_same_nv.remove(0)
+                Some(packages_with_same_nv.remove(0))
               }
             });
-          packages.get(id).unwrap()
+          match maybe_id {
+            Some(id) => packages.get(id).unwrap(),
+            None => {
+              // The package NV is not in the snapshot. This can happen
+              // with lockfiles from older versions where a peer dep NV
+              // referenced in an NpmPackageId was not included as a
+              // package. Leave this node with no children.
+              graph.resolved_node_ids.set(node_id, graph_resolved_id);
+              return node_id;
+            }
+          }
         }
       };
       for (name, child_id) in &resolution.dependencies {
@@ -6450,6 +6457,89 @@ mod test {
     let _graph = Graph::from_snapshot(snapshot);
   }
 
+  /// Builds a snapshot where package-b@1.0.0 has a peer dep on
+  /// package-c@2.0.0, but package-c@2.0.0 is not in the packages list.
+  fn snapshot_with_missing_peer_dep_nv() -> SerializedNpmResolutionSnapshot {
+    SerializedNpmResolutionSnapshot {
+      root_packages: HashMap::from([(
+        PackageReq::from_str("package-a").unwrap(),
+        NpmPackageId::from_serialized("package-a@1.0.0").unwrap(),
+      )]),
+      packages: Vec::from([
+        crate::resolution::SerializedNpmResolutionSnapshotPackage {
+          id: NpmPackageId::from_serialized("package-a@1.0.0").unwrap(),
+          system: Default::default(),
+          dependencies: HashMap::from([(
+            "package-b".into(),
+            NpmPackageId::from_serialized("package-b@1.0.0_package-c@2.0.0")
+              .unwrap(),
+          )]),
+          optional_peer_dependencies: Default::default(),
+          optional_dependencies: HashSet::new(),
+          extra: None,
+          is_deprecated: false,
+          dist: Some(crate::registry::NpmPackageVersionDistInfo {
+            tarball: "https://example.com/package-a@1.0.0.tgz".to_string(),
+            shasum: None,
+            integrity: None,
+          }),
+          has_bin: false,
+          has_scripts: false,
+        },
+        crate::resolution::SerializedNpmResolutionSnapshotPackage {
+          id: NpmPackageId::from_serialized("package-b@1.0.0_package-c@2.0.0")
+            .unwrap(),
+          system: Default::default(),
+          dependencies: HashMap::new(),
+          optional_peer_dependencies: Default::default(),
+          optional_dependencies: HashSet::new(),
+          extra: None,
+          is_deprecated: false,
+          dist: Some(crate::registry::NpmPackageVersionDistInfo {
+            tarball: "https://example.com/package-b@1.0.0.tgz".to_string(),
+            shasum: None,
+            integrity: None,
+          }),
+          has_bin: false,
+          has_scripts: false,
+        },
+      ]),
+    }
+  }
+
+  #[test]
+  fn graph_from_snapshot_peer_dep_not_in_packages_no_panic() {
+    // When a peer dep NV embedded in an NpmPackageId (e.g.
+    // package-b@1.0.0_package-c@2.0.0) references a package that doesn't
+    // exist in the snapshot, from_snapshot should not panic. It should
+    // create a node for the missing package with no children.
+    let snapshot = snapshot_with_missing_peer_dep_nv();
+    let snapshot = NpmResolutionSnapshot::new(snapshot.into_valid().unwrap());
+    let graph = Graph::from_snapshot(snapshot);
+
+    // Should have 3 nodes: package-a, package-b, and the missing package-c
+    assert_eq!(graph.nodes.len(), 3);
+
+    // package-a root should exist and have package-b as a child
+    let pkg_a_nv = PackageNv::from_str("package-a@1.0.0").unwrap();
+    let &pkg_a_node_id = graph.root_packages.get(&pkg_a_nv).unwrap();
+    let pkg_a_node = graph.nodes.get(&pkg_a_node_id).unwrap();
+    assert_eq!(pkg_a_node.children.len(), 1);
+    assert!(pkg_a_node.children.contains_key("package-b"));
+
+    // package-b should have no children (package-c is a peer dep, not a child)
+    let &pkg_b_node_id = pkg_a_node.children.get("package-b").unwrap();
+    let pkg_b_node = graph.nodes.get(&pkg_b_node_id).unwrap();
+    assert!(pkg_b_node.children.is_empty());
+
+    // package-c (the missing peer dep) should exist as a node with no children
+    let pkg_b_resolved = graph.resolved_node_ids.get(pkg_b_node_id).unwrap();
+    assert_eq!(pkg_b_resolved.peer_dependencies.len(), 1);
+    let pkg_c_node_id = pkg_b_resolved.peer_dependencies[0];
+    let pkg_c_node = graph.nodes.get(&pkg_c_node_id).unwrap();
+    assert!(pkg_c_node.children.is_empty());
+  }
+
   #[tokio::test]
   async fn link_packages() {
     let api = TestNpmRegistryApi::default();