Merge pull request #588 from afranchuk/prefer-local-wildcards

Prefer local wildcard audits to remote (imported) audits.

src/resolver.rs

@@ -341,6 +341,34 @@ pub enum RequiredEntry {
     },
 }
 
+#[derive(Debug, Copy, Clone, Eq, PartialEq)]
+enum DeltaEdgeFreshness {
+    // All information requried for this delta edge is already stored within the store's
+    // supply-chain. Using this edge will require no changes to imports.lock.
+    Stale,
+    // This edge originates from an already-imported or local wildcard audit, however will require
+    // importing fresh publisher information into imports.lock.
+    FreshPublisher,
+    // This edge is fully fresh, and will require adding the audit entry to imports.lock to use.
+    Fresh,
+}
+
+impl DeltaEdgeFreshness {
+    fn new(is_fresh_audit: bool, is_fresh_publisher: bool) -> Self {
+        if is_fresh_audit {
+            DeltaEdgeFreshness::Fresh
+        } else if is_fresh_publisher {
+            DeltaEdgeFreshness::FreshPublisher
+        } else {
+            DeltaEdgeFreshness::Stale
+        }
+    }
+
+    fn is_fresh(&self) -> bool {
+        self != &DeltaEdgeFreshness::Stale
+    }
+}
+
 /// A directed edge in the graph of audits. This may be forward or backwards,
 /// depending on if we're searching from "roots" (forward) or the target (backward).
 /// The source isn't included because that's implicit in the Node.
@@ -355,7 +383,7 @@ struct DeltaEdge<'a> {
     origin: DeltaEdgeOrigin,
     /// Whether or not the edge is a "fresh import", and should be
     /// de-prioritized to avoid unnecessary imports.lock updates.
-    is_fresh_import: bool,
+    freshness: DeltaEdgeFreshness,
 }
 
 #[derive(Debug, Copy, Clone, Eq, PartialEq)]
@@ -1142,12 +1170,13 @@ impl<'a> AuditGraph<'a> {
             };
 
             let criteria = criteria_mapper.criteria_from_list(&entry.criteria);
+            let freshness = DeltaEdgeFreshness::new(entry.is_fresh_import, false);
 
             forward_audits.entry(from_ver).or_default().push(DeltaEdge {
                 version: Some(to_ver),
                 criteria: criteria.clone(),
                 origin: origin.clone(),
-                is_fresh_import: entry.is_fresh_import,
+                freshness,
             });
             backward_audits
                 .entry(Some(to_ver))
@@ -1156,7 +1185,7 @@ impl<'a> AuditGraph<'a> {
                     version: from_ver,
                     criteria,
                     origin,
-                    is_fresh_import: entry.is_fresh_import,
+                    freshness,
                 });
         }
 
@@ -1177,19 +1206,20 @@ impl<'a> AuditGraph<'a> {
                         audit_index,
                         publisher_index,
                     };
-                    let is_fresh_import = entry.is_fresh_import || publisher.is_fresh_import;
+                    let freshness =
+                        DeltaEdgeFreshness::new(entry.is_fresh_import, publisher.is_fresh_import);
 
                     forward_audits.entry(from_ver).or_default().push(DeltaEdge {
                         version: to_ver,
                         criteria: criteria.clone(),
                         origin: origin.clone(),
-                        is_fresh_import,
+                        freshness,
                     });
                     backward_audits.entry(to_ver).or_default().push(DeltaEdge {
                         version: from_ver,
                         criteria,
                         origin,
-                        is_fresh_import,
+                        freshness,
                     });
                 }
             }
@@ -1203,18 +1233,23 @@ impl<'a> AuditGraph<'a> {
                     let to_ver = Some(&publisher.version);
                     let criteria = criteria_mapper.criteria_from_list(&entry.criteria);
                     let origin = DeltaEdgeOrigin::Trusted { publisher_index };
+                    // While the import freshness is technically based on the publisher being a
+                    // fresh import, we use this as the _audit_ freshness here because a trusted
+                    // entry should be put at the same caveat level ordering as other audits (which
+                    // is determined from freshness later on).
+                    let freshness = DeltaEdgeFreshness::new(publisher.is_fresh_import, false);
 
                     forward_audits.entry(from_ver).or_default().push(DeltaEdge {
                         version: to_ver,
                         criteria: criteria.clone(),
                         origin: origin.clone(),
-                        is_fresh_import: publisher.is_fresh_import,
+                        freshness,
                     });
                     backward_audits.entry(to_ver).or_default().push(DeltaEdge {
                         version: from_ver,
                         criteria,
                         origin,
-                        is_fresh_import: publisher.is_fresh_import,
+                        freshness,
                     });
                 }
             }
@@ -1226,19 +1261,19 @@ impl<'a> AuditGraph<'a> {
             let to_ver = Some(&unpublished.version);
             let criteria = criteria_mapper.all_criteria();
             let origin = DeltaEdgeOrigin::Unpublished { unpublished_index };
-            let is_fresh_import = unpublished.is_fresh_import;
+            let freshness = DeltaEdgeFreshness::new(unpublished.is_fresh_import, false);
 
             forward_audits.entry(from_ver).or_default().push(DeltaEdge {
                 version: to_ver,
                 criteria: criteria.clone(),
                 origin: origin.clone(),
-                is_fresh_import,
+                freshness,
             });
             backward_audits.entry(to_ver).or_default().push(DeltaEdge {
                 version: from_ver,
                 criteria,
                 origin,
-                is_fresh_import,
+                freshness,
             });
         }
 
@@ -1255,13 +1290,13 @@ impl<'a> AuditGraph<'a> {
                     version: to_ver,
                     criteria: criteria.clone(),
                     origin: origin.clone(),
-                    is_fresh_import: false,
+                    freshness: DeltaEdgeFreshness::Stale,
                 });
                 backward_audits.entry(to_ver).or_default().push(DeltaEdge {
                     version: from_ver,
                     criteria,
                     origin,
-                    is_fresh_import: false,
+                    freshness: DeltaEdgeFreshness::Stale,
                 });
             }
         }
@@ -1449,6 +1484,7 @@ fn search_for_path(
         NonImportableAudit,
         PreferredExemption,
         PreferredUnpublished,
+        FreshPublisher,
         FreshImport,
         Exemption,
         Unpublished,
@@ -1579,16 +1615,19 @@ fn search_for_path(
                 DeltaEdgeOrigin::Unpublished { .. } => match mode {
                     // When preferring exemptions, prefer existing unpublished
                     // entries to avoid imports.lock churn.
-                    SearchMode::PreferExemptions if !edge.is_fresh_import => {
+                    SearchMode::PreferExemptions if !edge.freshness.is_fresh() => {
                         CaveatLevel::PreferredUnpublished
                     }
                     SearchMode::PreferExemptions => CaveatLevel::Unpublished,
                     // Otherwise, prefer fresh to avoid outdated versions.
-                    _ if edge.is_fresh_import => CaveatLevel::PreferredUnpublished,
+                    _ if edge.freshness.is_fresh() => CaveatLevel::PreferredUnpublished,
                     _ => CaveatLevel::Unpublished,
                 },
-                _ if edge.is_fresh_import => CaveatLevel::FreshImport,
-                _ => CaveatLevel::None,
+                _ => match edge.freshness {
+                    DeltaEdgeFreshness::Stale => CaveatLevel::None,
+                    DeltaEdgeFreshness::FreshPublisher => CaveatLevel::FreshPublisher,
+                    DeltaEdgeFreshness::Fresh => CaveatLevel::FreshImport,
+                },
             };
 
             queue.push(Node {

src/tests/import.rs

@@ -1934,3 +1934,123 @@ fn import_criteria_map_aggregated_error() {
 
     insta::assert_snapshot!(output);
 }
+
+#[test]
+fn existing_import_kept_despite_local_wildcard_audit() {
+    // (Pass) An existing imported audit is still kept if a local wildcard audit accounts for a
+    // crate.
+
+    let _enter = TEST_RUNTIME.enter();
+    let mock = MockMetadata::simple();
+
+    let metadata = mock.metadata();
+    let (mut config, mut audits, mut imports) = builtin_files_full_audited(&metadata);
+
+    audits.audits.remove("third-party2");
+
+    audits.wildcard_audits.insert(
+        "third-party2".to_owned(),
+        vec![wildcard_audit(1, SAFE_TO_DEPLOY)],
+    );
+
+    let foreign_audits = AuditsFile {
+        criteria: SortedMap::new(),
+        audits: [(
+            "third-party2".to_owned(),
+            vec![full_audit(ver(DEFAULT_VER), SAFE_TO_DEPLOY)],
+        )]
+        .into(),
+        wildcard_audits: SortedMap::new(),
+        trusted: SortedMap::new(),
+    };
+
+    imports
+        .audits
+        .insert(FOREIGN.to_owned(), foreign_audits.clone());
+
+    config.imports.insert(
+        FOREIGN.to_owned(),
+        crate::format::RemoteImport {
+            url: vec![FOREIGN_URL.to_owned()],
+            ..Default::default()
+        },
+    );
+
+    let cfg = mock_cfg(&metadata);
+
+    let mut network = Network::new_mock();
+    MockRegistryBuilder::new()
+        .user(1, "user1", "User One")
+        .package(
+            "third-party2",
+            &[reg_published_by(ver(DEFAULT_VER), Some(1), "2022-12-15")],
+        )
+        .serve(&mut network);
+    network.mock_serve_toml(FOREIGN_URL, &foreign_audits);
+
+    let store = Store::mock_online(&cfg, config, audits, imports, &network, true).unwrap();
+
+    let output = get_imports_file_changes_prune(&metadata, &store);
+    insta::assert_snapshot!(output);
+}
+
+#[test]
+fn local_wildcard_audit_preferred_to_fresh_import() {
+    // (Pass) If a local wildcard audit accounts for a crate, a freshly imported audit should not
+    // be preferred.
+
+    let _enter = TEST_RUNTIME.enter();
+    let mock = MockMetadata::simple();
+
+    let metadata = mock.metadata();
+    let (mut config, mut audits, imports) = builtin_files_full_audited(&metadata);
+
+    audits.audits.remove("third-party2");
+
+    audits.wildcard_audits.insert(
+        "third-party2".to_owned(),
+        vec![wildcard_audit(1, SAFE_TO_DEPLOY)],
+    );
+
+    let foreign_audits = AuditsFile {
+        criteria: SortedMap::new(),
+        audits: [(
+            "third-party2".to_owned(),
+            vec![full_audit(ver(DEFAULT_VER), SAFE_TO_DEPLOY)],
+        )]
+        .into(),
+        // This isn't necessary (and overlaps with the imported audit), but we have it to also show
+        // that local wildcard audits are preferred to remote ones.
+        wildcard_audits: [(
+            "third-party2".to_owned(),
+            vec![wildcard_audit(1, SAFE_TO_DEPLOY)],
+        )]
+        .into(),
+        trusted: SortedMap::new(),
+    };
+
+    config.imports.insert(
+        FOREIGN.to_owned(),
+        crate::format::RemoteImport {
+            url: vec![FOREIGN_URL.to_owned()],
+            ..Default::default()
+        },
+    );
+
+    let cfg = mock_cfg(&metadata);
+
+    let mut network = Network::new_mock();
+    MockRegistryBuilder::new()
+        .user(1, "user1", "User One")
+        .package(
+            "third-party2",
+            &[reg_published_by(ver(DEFAULT_VER), Some(1), "2022-12-15")],
+        )
+        .serve(&mut network);
+    network.mock_serve_toml(FOREIGN_URL, &foreign_audits);
+
+    let store = Store::mock_online(&cfg, config, audits, imports, &network, true).unwrap();
+
+    let output = get_imports_file_changes_noprune(&metadata, &store);
+    insta::assert_snapshot!(output);
+}

src/tests/snapshots/cargo_vet__tests__import__existing_import_kept_despite_local_wildcard_audit.snap

@@ -0,0 +1,9 @@
+---
+source: src/tests/import.rs
+expression: output
+---
+
+ [[audits.peer-company.audits.third-party2]]
+ criteria = "safe-to-deploy"
+ version = "10.0.0"
+

src/tests/snapshots/cargo_vet__tests__import__local_wildcard_audit_preferred_to_fresh_import.snap

@@ -0,0 +1,14 @@
+---
+source: src/tests/import.rs
+expression: output
+---
++
++[[publisher.third-party2]]
++version = "10.0.0"
++when = "2022-12-15"
++user-id = 1
++user-login = "user1"
++user-name = "User One"
++
++[audits.peer-company.audits]
+