From 486010ed3630b33dbe2d7ff8eb52a837835cfab9 Mon Sep 17 00:00:00 2001 From: joshmc Date: Mon, 16 Nov 2020 19:48:17 +0000 Subject: [PATCH] ISSUE-16 - Remove usage of `ManifestMetadata`: * Add functions to extract licence and repository from `cargo_metadata::PackageId` * Remove `ManifestMetadata` from display * Add testing Signed-off-by: joshmc --- cargo-geiger/src/format/display.rs | 45 +++++++--------- cargo-geiger/src/format/pattern.rs | 3 -- cargo-geiger/src/format/table.rs | 5 +- .../src/format/table/handle_text_tree_line.rs | 17 ++---- cargo-geiger/src/mapping.rs | 15 ++++-- cargo-geiger/src/mapping/krates.rs | 53 +++++++++++++++---- cargo-geiger/src/scan/default/table.rs | 1 - cargo-geiger/src/scan/forbid/table.rs | 20 +------ 8 files changed, 81 insertions(+), 78 deletions(-) diff --git a/cargo-geiger/src/format/display.rs b/cargo-geiger/src/format/display.rs index 23f356f3..81d9cf96 100644 --- a/cargo-geiger/src/format/display.rs +++ b/cargo-geiger/src/format/display.rs @@ -1,18 +1,18 @@ use crate::format::pattern::Pattern; use crate::format::Chunk; - use crate::mapping::{ - CargoMetadataParameters, GetPackageNameFromCargoMetadataPackageId, + CargoMetadataParameters, GetLicenceFromCargoMetadataPackageId, + GetPackageNameFromCargoMetadataPackageId, GetPackageVersionFromCargoMetadataPackageId, + GetRepositoryFromCargoMetadataPackageId, }; -use cargo::core::manifest::ManifestMetadata; + use std::fmt; pub struct Display<'a> { pub cargo_metadata_parameters: &'a CargoMetadataParameters<'a>, pub pattern: &'a Pattern, pub package: &'a cargo_metadata::PackageId, - pub metadata: &'a ManifestMetadata, } impl<'a> fmt::Display for Display<'a> { @@ -20,7 +20,13 @@ impl<'a> fmt::Display for Display<'a> { for chunk in &self.pattern.0 { match *chunk { Chunk::License => { - if let Some(ref license) = self.metadata.license { + if let Some(ref license) = self + .cargo_metadata_parameters + .krates + .get_licence_from_cargo_metadata_package_id( + self.package, + ) + { (write!(fmt, "{}", license))? } } @@ -42,7 +48,13 @@ impl<'a> fmt::Display for Display<'a> { } Chunk::Raw(ref s) => (fmt.write_str(s))?, Chunk::Repository => { - if let Some(ref repository) = self.metadata.repository { + if let Some(ref repository) = self + .cargo_metadata_parameters + .krates + .get_repository_from_cargo_metadata_package_id( + self.package, + ) + { (write!(fmt, "{}", repository))? } } @@ -59,7 +71,6 @@ pub mod display_tests { use crate::format::pattern::Pattern; use crate::format::Chunk; - use cargo::core::manifest::ManifestMetadata; use cargo_metadata::{CargoOpt, MetadataCommand}; use krates::Builder as KratesBuilder; use rstest::*; @@ -69,7 +80,7 @@ pub mod display_tests { expected_formatted_string, case( Pattern(vec![Chunk::License]), - "licence_string" + "Apache-2.0/MIT" ), case( Pattern(vec![Chunk::Package]), @@ -81,7 +92,7 @@ pub mod display_tests { ), case( Pattern(vec![Chunk::Repository]), - "repository_string" + "https://github.com/rust-secure-code/cargo-geiger" ) )] fn display_format_fmt_test( @@ -100,21 +111,6 @@ pub mod display_tests { let package_id = metadata.root_package().unwrap().id.clone(); - let manifest_metadata = ManifestMetadata { - authors: vec![], - keywords: vec![], - categories: vec![], - license: Some(String::from("licence_string")), - license_file: None, - description: None, - readme: None, - homepage: None, - repository: Some(String::from("repository_string")), - documentation: None, - badges: Default::default(), - links: None, - }; - let display = Display { cargo_metadata_parameters: &CargoMetadataParameters { krates: &krates, @@ -122,7 +118,6 @@ pub mod display_tests { }, pattern: &input_pattern, package: &package_id, - metadata: &manifest_metadata, }; assert_eq!(format!("{}", display), expected_formatted_string); diff --git a/cargo-geiger/src/format/pattern.rs b/cargo-geiger/src/format/pattern.rs index f29572bf..fbddd682 100644 --- a/cargo-geiger/src/format/pattern.rs +++ b/cargo-geiger/src/format/pattern.rs @@ -4,7 +4,6 @@ use crate::format::{Chunk, RawChunk}; use super::display::Display; use crate::mapping::CargoMetadataParameters; -use cargo::core::manifest::ManifestMetadata; use std::error::Error; #[derive(Debug, PartialEq)] @@ -14,14 +13,12 @@ impl Pattern { pub fn display<'a>( &'a self, cargo_metadata_parameters: &'a CargoMetadataParameters, - metadata: &'a ManifestMetadata, package: &'a cargo_metadata::PackageId, ) -> Display<'a> { Display { cargo_metadata_parameters, pattern: self, package, - metadata, } } diff --git a/cargo-geiger/src/format/table.rs b/cargo-geiger/src/format/table.rs index acff4800..125f1ca9 100644 --- a/cargo-geiger/src/format/table.rs +++ b/cargo-geiger/src/format/table.rs @@ -4,6 +4,7 @@ mod total_package_counts; use crate::format::emoji_symbols::EmojiSymbols; use crate::format::print_config::{colorize, PrintConfig}; use crate::format::CrateDetectionStatus; +use crate::mapping::CargoMetadataParameters; use crate::scan::GeigerContext; use crate::tree::TextTreeLine; @@ -13,8 +14,6 @@ use handle_text_tree_line::{ }; use total_package_counts::TotalPackageCounts; -use crate::mapping::CargoMetadataParameters; -use cargo::core::package::PackageSet; use cargo_geiger_serde::{Count, CounterBlock}; use std::collections::HashSet; use std::path::PathBuf; @@ -33,7 +32,6 @@ pub const UNSAFE_COUNTERS_HEADER: [&str; 6] = [ pub fn create_table_from_text_tree_lines( cargo_metadata_parameters: &CargoMetadataParameters, - package_set: &PackageSet, table_parameters: &TableParameters, text_tree_lines: Vec, ) -> (Vec, u64) { @@ -67,7 +65,6 @@ pub fn create_table_from_text_tree_lines( &emoji_symbols, &mut handle_package_parameters, package_id, - package_set, &mut table_lines, table_parameters, tree_vines, diff --git a/cargo-geiger/src/format/table/handle_text_tree_line.rs b/cargo-geiger/src/format/table/handle_text_tree_line.rs index 850815a6..64a7854b 100644 --- a/cargo-geiger/src/format/table/handle_text_tree_line.rs +++ b/cargo-geiger/src/format/table/handle_text_tree_line.rs @@ -1,7 +1,7 @@ use crate::format::emoji_symbols::EmojiSymbols; use crate::format::print_config::colorize; use crate::format::{get_kind_group_name, CrateDetectionStatus, SymbolKind}; -use crate::mapping::{CargoMetadataParameters, ToPackage}; +use crate::mapping::CargoMetadataParameters; use crate::scan::unsafe_stats; use super::total_package_counts::TotalPackageCounts; @@ -9,7 +9,6 @@ use super::TableParameters; use super::{table_row, table_row_empty}; use cargo::core::dependency::DepKind; -use cargo::core::package::PackageSet; use std::collections::HashSet; pub struct HandlePackageParameters<'a> { @@ -33,13 +32,11 @@ pub fn handle_text_tree_line_extra_deps_group( table_lines.push(format!("{}{}{}", table_row_empty(), tree_vines, name)); } -#[allow(clippy::too_many_arguments)] pub fn handle_text_tree_line_package( cargo_metadata_parameters: &CargoMetadataParameters, emoji_symbols: &EmojiSymbols, handle_package_parameters: &mut HandlePackageParameters, package_id: cargo_metadata::PackageId, - package_set: &PackageSet, table_lines: &mut Vec, table_parameters: &TableParameters, tree_vines: String, @@ -48,9 +45,6 @@ pub fn handle_text_tree_line_package( .visited_package_ids .insert(package_id.clone()); - let package = - package_id.to_package(cargo_metadata_parameters.krates, package_set); - let package_metrics = match table_parameters .geiger_context .package_id_to_metrics @@ -99,11 +93,10 @@ pub fn handle_text_tree_line_package( let package_name = colorize( format!( "{}", - table_parameters.print_config.format.display( - cargo_metadata_parameters, - package.manifest().metadata(), - &package_id - ) + table_parameters + .print_config + .format + .display(cargo_metadata_parameters, &package_id) ), &crate_detection_status, ); diff --git a/cargo-geiger/src/mapping.rs b/cargo-geiger/src/mapping.rs index 5ceb49b7..3c3507ca 100644 --- a/cargo-geiger/src/mapping.rs +++ b/cargo-geiger/src/mapping.rs @@ -4,7 +4,6 @@ mod metadata; use ::krates::Krates; use cargo::core::dependency::DepKind; -use cargo::core::manifest::ManifestMetadata; use cargo::core::{Package, PackageId, PackageSet, Resolve}; use cargo_metadata::{DependencyKind, Metadata}; use std::collections::HashSet; @@ -28,12 +27,11 @@ pub trait DepsNotReplaced { )>; } -pub trait GetManifestMetadataFromCargoMetadataPackageId { - fn get_manifest_metadata_from_cargo_metadata_package_id( +pub trait GetLicenceFromCargoMetadataPackageId { + fn get_licence_from_cargo_metadata_package_id( &self, package_id: &cargo_metadata::PackageId, - package_set: &PackageSet, - ) -> ManifestMetadata; + ) -> Option; } pub trait GetPackageNameFromCargoMetadataPackageId { @@ -50,6 +48,13 @@ pub trait GetPackageVersionFromCargoMetadataPackageId { ) -> cargo_metadata::Version; } +pub trait GetRepositoryFromCargoMetadataPackageId { + fn get_repository_from_cargo_metadata_package_id( + &self, + package_id: &cargo_metadata::PackageId, + ) -> Option; +} + pub trait GetRoot { fn get_root(&self) -> PathBuf; } diff --git a/cargo-geiger/src/mapping/krates.rs b/cargo-geiger/src/mapping/krates.rs index df360fb5..623019da 100644 --- a/cargo-geiger/src/mapping/krates.rs +++ b/cargo-geiger/src/mapping/krates.rs @@ -1,21 +1,19 @@ use crate::mapping::{ - GetManifestMetadataFromCargoMetadataPackageId, + GetLicenceFromCargoMetadataPackageId, GetPackageNameFromCargoMetadataPackageId, - GetPackageVersionFromCargoMetadataPackageId, ToPackage, + GetPackageVersionFromCargoMetadataPackageId, + GetRepositoryFromCargoMetadataPackageId, }; -use cargo::core::manifest::ManifestMetadata; -use cargo::core::PackageSet; use krates::Krates; -impl GetManifestMetadataFromCargoMetadataPackageId for Krates { - fn get_manifest_metadata_from_cargo_metadata_package_id( +impl GetLicenceFromCargoMetadataPackageId for Krates { + fn get_licence_from_cargo_metadata_package_id( &self, package_id: &cargo_metadata::PackageId, - package_set: &PackageSet, - ) -> ManifestMetadata { - let package = package_id.to_package(self, package_set); - package.manifest().metadata().clone() + ) -> Option { + let package = self.node_for_kid(package_id); + package.unwrap().krate.clone().license } } @@ -39,6 +37,16 @@ impl GetPackageVersionFromCargoMetadataPackageId for Krates { } } +impl GetRepositoryFromCargoMetadataPackageId for Krates { + fn get_repository_from_cargo_metadata_package_id( + &self, + package_id: &cargo_metadata::PackageId, + ) -> Option { + let package = self.node_for_kid(package_id); + package.unwrap().krate.clone().repository + } +} + #[cfg(test)] mod krates_tests { use super::*; @@ -47,6 +55,17 @@ mod krates_tests { use krates::Builder as KratesBuilder; use rstest::*; + #[rstest] + fn get_licence_from_cargo_metadata_package_id_test() { + let (krates, metadata) = construct_krates_and_metadata(); + let package = metadata.root_package().unwrap(); + let licence_option = + krates.get_licence_from_cargo_metadata_package_id(&package.id); + assert!(licence_option.is_some()); + let licence = licence_option.unwrap(); + assert_eq!(licence, String::from("Apache-2.0/MIT")) + } + #[rstest] fn get_package_name_from_cargo_metadata_package_id_test() { let (krates, metadata) = construct_krates_and_metadata(); @@ -65,6 +84,20 @@ mod krates_tests { assert_eq!(package_version, package.version); } + #[rstest] + fn get_repository_from_cargo_metadata_package_id_test() { + let (krates, metadata) = construct_krates_and_metadata(); + let package = metadata.root_package().unwrap(); + let repository_option = + krates.get_repository_from_cargo_metadata_package_id(&package.id); + assert!(repository_option.is_some()); + let repository = repository_option.unwrap(); + assert_eq!( + repository, + String::from("https://github.com/rust-secure-code/cargo-geiger") + ); + } + fn construct_krates_and_metadata() -> (Krates, Metadata) { let metadata = MetadataCommand::new() .manifest_path("./Cargo.toml") diff --git a/cargo-geiger/src/scan/default/table.rs b/cargo-geiger/src/scan/default/table.rs index de594f4e..32893a93 100644 --- a/cargo-geiger/src/scan/default/table.rs +++ b/cargo-geiger/src/scan/default/table.rs @@ -66,7 +66,6 @@ pub fn scan_to_table( let (mut table_lines, mut warning_count) = create_table_from_text_tree_lines( cargo_metadata_parameters, - package_set, &table_parameters, text_tree_lines, ); diff --git a/cargo-geiger/src/scan/forbid/table.rs b/cargo-geiger/src/scan/forbid/table.rs index 8ae8761b..3366f46d 100644 --- a/cargo-geiger/src/scan/forbid/table.rs +++ b/cargo-geiger/src/scan/forbid/table.rs @@ -3,9 +3,7 @@ use crate::format::pattern::Pattern; use crate::format::print_config::PrintConfig; use crate::format::{get_kind_group_name, SymbolKind}; use crate::graph::Graph; -use crate::mapping::{ - CargoMetadataParameters, GetManifestMetadataFromCargoMetadataPackageId, -}; +use crate::mapping::CargoMetadataParameters; use crate::tree::traversal::walk_dependency_tree; use crate::tree::TextTreeLine; @@ -67,7 +65,6 @@ pub fn scan_forbid_to_table( &emoji_symbols, &geiger_ctx, package_id, - package_set, print_config, &mut scan_output_lines, tree_vines, @@ -112,31 +109,19 @@ fn construct_key_lines(emoji_symbols: &EmojiSymbols) -> Vec { fn format_package_name( cargo_metadata_parameters: &CargoMetadataParameters, package_id: &cargo_metadata::PackageId, - package_set: &PackageSet, pattern: &Pattern, ) -> String { format!( "{}", - pattern.display( - cargo_metadata_parameters, - &cargo_metadata_parameters - .krates - .get_manifest_metadata_from_cargo_metadata_package_id( - package_id, - package_set - ), - &package_id - ) + pattern.display(cargo_metadata_parameters, &package_id) ) } -#[allow(clippy::too_many_arguments)] fn handle_package_text_tree_line( cargo_metadata_parameters: &CargoMetadataParameters, emoji_symbols: &EmojiSymbols, geiger_ctx: &GeigerContext, package_id: cargo_metadata::PackageId, - package_set: &PackageSet, print_config: &PrintConfig, scan_output_lines: &mut Vec, tree_vines: String, @@ -147,7 +132,6 @@ fn handle_package_text_tree_line( let name = format_package_name( cargo_metadata_parameters, &package_id, - package_set, &print_config.format, ); let package_metrics = geiger_ctx.package_id_to_metrics.get(&package_id);