chore: refactor affiliation analysis, experiment with macro (#154)

hipcheck/src/analysis/analysis.rs

@@ -36,7 +36,7 @@ pub trait AnalysisProvider:
 	fn activity_analysis(&self) -> Arc<HCAnalysisReport>;
 
 	/// Returns result of affiliation analysis
-	fn affiliation_analysis(&self) -> Result<Arc<AnalysisReport>>;
+	fn affiliation_analysis(&self) -> Arc<HCAnalysisReport>;
 
 	/// Returns result of binary analysis
 	fn binary_analysis(&self) -> Result<Arc<AnalysisReport>>;
@@ -185,93 +185,69 @@ pub fn activity_analysis(db: &dyn AnalysisProvider) -> Arc<HCAnalysisReport> {
 		Ok(results) => results,
 	};
 	let value = results.time_since_last_commit.num_weeks() as u64;
-	let hc_value = HCBasicValue::from(value);
 	Arc::new(HCAnalysisReport {
-		outcome: HCAnalysisOutcome::Completed(HCAnalysisValue::Basic(hc_value)),
+		outcome: HCAnalysisOutcome::Completed(HCAnalysisValue::Basic(value.into())),
 		concerns: vec![],
 	})
 }
 
-pub fn affiliation_analysis(db: &dyn AnalysisProvider) -> Result<Arc<AnalysisReport>> {
-	if db.affiliation_active() {
-		let results = db.affiliation_metric();
-
-		match results {
-			Err(err) => Ok(Arc::new(AnalysisReport::None {
-				outcome: AnalysisOutcome::Error(err),
-			})),
-			Ok(results) => {
-				let affiliated_iter = results
-					.affiliations
-					.iter()
-					.filter(|a| a.affiliated_type.is_affiliated());
-
-				let value = affiliated_iter.clone().count() as u64;
-				let threshold = db.affiliation_count_threshold();
-				let results_score = score_by_threshold(value, threshold);
-
-				let mut contributor_freq_map = HashMap::new();
-
-				for affiliation in affiliated_iter {
-					let commit_view =
-						db.contributors_for_commit(Arc::clone(&affiliation.commit))?;
-
-					let contributor = match affiliation.affiliated_type {
-						AffiliatedType::Author => String::from(&commit_view.author.name),
-						AffiliatedType::Committer => String::from(&commit_view.committer.name),
-						AffiliatedType::Neither => String::from("Neither"),
-						AffiliatedType::Both => String::from("Both"),
-					};
-
-					let count_commits_for = |contributor| {
-						db.commits_for_contributor(Arc::clone(contributor))
-							.into_iter()
-							.count() as i64
-					};
-
-					let author_commits = count_commits_for(&commit_view.author);
-					let committer_commits = count_commits_for(&commit_view.committer);
-
-					let commit_count = match affiliation.affiliated_type {
-						AffiliatedType::Neither => 0,
-						AffiliatedType::Both => author_commits + committer_commits,
-						AffiliatedType::Author => author_commits,
-						AffiliatedType::Committer => committer_commits,
-					};
+pub fn affiliation_analysis(db: &dyn AnalysisProvider) -> Arc<HCAnalysisReport> {
+	let results = match db.affiliation_metric() {
+		Err(err) => return Arc::new(HCAnalysisReport::generic_error(err, vec![])),
+		Ok(results) => results,
+	};
 
-					// Add string representation of affiliated contributor with count of associated commits
-					contributor_freq_map.insert(contributor, commit_count);
-				}
+	let affiliated_iter = results
+		.affiliations
+		.iter()
+		.filter(|a| a.affiliated_type.is_affiliated());
+
+	let value = affiliated_iter.clone().count() as u64;
+
+	let mut contributor_freq_map = HashMap::new();
+
+	for affiliation in affiliated_iter {
+		let commit_view = match db.contributors_for_commit(Arc::clone(&affiliation.commit)) {
+			Err(err) => return Arc::new(HCAnalysisReport::generic_error(err, vec![])),
+			Ok(cv) => cv,
+		};
+
+		let contributor = match affiliation.affiliated_type {
+			AffiliatedType::Author => String::from(&commit_view.author.name),
+			AffiliatedType::Committer => String::from(&commit_view.committer.name),
+			AffiliatedType::Neither => String::from("Neither"),
+			AffiliatedType::Both => String::from("Both"),
+		};
+
+		let count_commits_for = |contributor| {
+			db.commits_for_contributor(Arc::clone(contributor))
+				.into_iter()
+				.count() as i64
+		};
+
+		let author_commits = count_commits_for(&commit_view.author);
+		let committer_commits = count_commits_for(&commit_view.committer);
+
+		let commit_count = match affiliation.affiliated_type {
+			AffiliatedType::Neither => 0,
+			AffiliatedType::Both => author_commits + committer_commits,
+			AffiliatedType::Author => author_commits,
+			AffiliatedType::Committer => committer_commits,
+		};
+
+		// Add string representation of affiliated contributor with count of associated commits
+		contributor_freq_map.insert(contributor, commit_count);
+	}
 
-				let concerns = contributor_freq_map
-					.into_iter()
-					.map(|(contributor, count)| Concern::Affiliation { contributor, count })
-					.collect();
+	let concerns = contributor_freq_map
+		.into_iter()
+		.map(|(contributor, count)| Concern::Affiliation { contributor, count })
+		.collect();
 
-				if results_score == 0 {
-					let msg = format!("{} affiliated <= {} affiliated", value, threshold);
-					Ok(Arc::new(AnalysisReport::Affiliation {
-						value,
-						threshold,
-						outcome: AnalysisOutcome::Pass(msg),
-						concerns,
-					}))
-				} else {
-					let msg = format!("{} affiliated > {} affiliated", value, threshold);
-					Ok(Arc::new(AnalysisReport::Affiliation {
-						value,
-						threshold,
-						outcome: AnalysisOutcome::Fail(msg),
-						concerns,
-					}))
-				}
-			}
-		}
-	} else {
-		Ok(Arc::new(AnalysisReport::None {
-			outcome: AnalysisOutcome::Skipped,
-		}))
-	}
+	Arc::new(HCAnalysisReport {
+		outcome: HCAnalysisOutcome::Completed(HCAnalysisValue::Basic(value.into())),
+		concerns,
+	})
 }
 
 pub fn binary_analysis(db: &dyn AnalysisProvider) -> Result<Arc<AnalysisReport>> {

hipcheck/src/analysis/report_builder.rs

@@ -48,35 +48,26 @@ pub fn build_report(session: &Session, scoring: &ScoringResults) -> Result<Repor
 
 	// Affiliation analysis.
 
-	extract_results(
-		&mut builder,
-		&scoring.results.affiliation,
-		|builder, output| {
-			match output.as_ref() {
-				AnalysisReport::Affiliation {
-					value,
-					threshold,
-					concerns,
-					..
-				} => {
-					let analysis = Analysis::affiliation(*value, *threshold);
-					builder.add_analysis(analysis, concerns.clone())?;
-				}
-				_ => {
-					return Err(hc_error!(
-						"phase name does not match {} analysis",
-						crate::analysis::score::AFFILIATION_PHASE
-					))
-				}
+	if let Some(stored) = &scoring.results.affiliation {
+		match &stored.result {
+			Ok(analysis) => {
+				let Predicate::Threshold(pred) = analysis.as_ref();
+				let HCBasicValue::Unsigned(value) = pred.value else {
+					return Err(hc_error!("affiliation analysis has a non-u64 value"));
+				};
+				let HCBasicValue::Unsigned(thresh) = pred.threshold else {
+					return Err(hc_error!("affiliation analysis has a non-u64 value"));
+				};
+				builder.add_analysis(
+					Analysis::affiliation(value, thresh),
+					stored.concerns.clone(),
+				)?;
 			}
-
-			Ok(())
-		},
-		|builder, error| {
-			builder.add_errored_analysis(AnalysisIdent::Affiliation, error);
-			Ok(())
-		},
-	)?;
+			Err(error) => {
+				builder.add_errored_analysis(AnalysisIdent::Affiliation, error);
+			}
+		}
+	};
 
 	// Binary Analysis
 

hipcheck/src/analysis/result.rs

@@ -112,10 +112,16 @@ pub enum HCCompositeValue {
 }
 
 /// The set of possible predicates for deciding if a source passed an analysis.
-pub trait HCPredicate: Display + std::fmt::Debug + std::any::Any + 'static {
+pub trait HCPredicate: Display + std::fmt::Debug {
 	fn pass(&self) -> Result<bool>;
 }
 
+pub struct ThresholdSpec {
+	pub threshold: HCBasicValue,
+	pub units: Option<String>,
+	pub ordering: Ordering,
+}
+
 /// This predicate determines analysis pass/fail by whether a returned value was
 /// greater than, less than, or equal to a target value.
 #[derive(Debug, Clone, Eq, PartialEq)]

hipcheck/src/analysis/score.rs

@@ -100,7 +100,7 @@ impl AltAnalysisResults {
 #[derive(Debug, Default)]
 pub struct AnalysisResults {
 	pub activity: Option<HCStoredResult>,
-	pub affiliation: Option<Result<Arc<AnalysisReport>>>,
+	pub affiliation: Option<HCStoredResult>,
 	pub binary: Option<Result<Arc<AnalysisReport>>>,
 	pub churn: Option<Result<Arc<AnalysisReport>>>,
 	pub entropy: Option<Result<Arc<AnalysisReport>>>,
@@ -185,36 +185,9 @@ pub fn phase_outcome<P: AsRef<String>>(
 		ACTIVITY_PHASE => Err(hc_error!(
 			"activity analysis does not use this infrastructure"
 		)),
-		AFFILIATION_PHASE => match &db.affiliation_analysis().unwrap().as_ref() {
-			AnalysisReport::None {
-				outcome: AnalysisOutcome::Skipped,
-			} => Ok(Arc::new(ScoreResult::default())),
-			AnalysisReport::None {
-				outcome: AnalysisOutcome::Error(msg),
-			} => Ok(Arc::new(ScoreResult {
-				count: 0,
-				score: 0,
-				outcome: AnalysisOutcome::Error(msg.clone()),
-			})),
-			AnalysisReport::Affiliation {
-				outcome: AnalysisOutcome::Pass(msg),
-				..
-			} => Ok(Arc::new(ScoreResult {
-				count: db.affiliation_weight(),
-				score: 0,
-				outcome: AnalysisOutcome::Pass(msg.to_string()),
-			})),
-			AnalysisReport::Affiliation {
-				outcome: AnalysisOutcome::Fail(msg),
-				..
-			} => Ok(Arc::new(ScoreResult {
-				count: db.affiliation_weight(),
-				score: 1,
-				outcome: AnalysisOutcome::Fail(msg.to_string()),
-			})),
-			_ => Err(hc_error!("phase name does not match analysis")),
-		},
-
+		AFFILIATION_PHASE => Err(hc_error!(
+			"affiliation analysis does not use this infrastructure"
+		)),
 		BINARY_PHASE => match &db.binary_analysis().unwrap().as_ref() {
 			AnalysisReport::None {
 				outcome: AnalysisOutcome::Skipped,
@@ -581,6 +554,29 @@ pub fn add_tree_edge(
 	score_tree
 }
 
+macro_rules! run_and_score_threshold_analysis {
+	($res:ident, $p:ident, $tree: ident, $phase:ident, $a:expr, $w:expr, $spec:ident, $node:ident) => {{
+		update_phase($p, $phase)?;
+		let analysis_result =
+			ThresholdPredicate::from_analysis(&$a, $spec.threshold, $spec.units, $spec.ordering);
+		$res.table.insert($phase.to_owned(), analysis_result);
+		let (an_score, outcome) = $res.table.get($phase).unwrap().score();
+		let score_result = Arc::new(ScoreResult {
+			count: $w,
+			score: an_score,
+			outcome,
+		});
+		let output = score_result.outcome.clone();
+		match add_node_and_edge_with_score(score_result, $tree, $phase, $node) {
+			Ok(score_tree_inc) => {
+				$tree = score_tree_inc;
+			}
+			_ => return Err(hc_error!("failed to complete {} scoring.", $phase)),
+		};
+		output
+	}};
+}
+
 pub fn score_results(phase: &mut Phase, db: &dyn ScoringProvider) -> Result<ScoringResults> {
 	/*
 	Scoring should be performed by the construction of a "score tree" where scores are the
@@ -816,37 +812,23 @@ pub fn score_results(phase: &mut Phase, db: &dyn ScoringProvider) -> Result<Scor
 			score_tree = add_tree_edge(score_tree_updated, commit_node, attacks_node);
 
 			/*===NEW_PHASE===*/
-			update_phase(phase, AFFILIATION_PHASE)?;
-			let affiliation_analysis = db.affiliation_analysis()?;
-			match affiliation_analysis.as_ref() {
-				AnalysisReport::None {
-					outcome: AnalysisOutcome::Skipped,
-				} => results.affiliation = None,
-				AnalysisReport::None {
-					outcome: AnalysisOutcome::Error(err),
-				} => results.affiliation = Some(Err(err.clone())),
-				_ => results.affiliation = Some(Ok(affiliation_analysis)),
-			}
-			let score_result = db
-				.phase_outcome(Arc::new(AFFILIATION_PHASE.to_string()))
-				.unwrap();
-			score.affiliation = score_result.outcome.clone();
-			match add_node_and_edge_with_score(
-				score_result,
+			let spec = ThresholdSpec {
+				threshold: HCBasicValue::from(db.affiliation_count_threshold()),
+				units: Some("affiliated".to_owned()),
+				ordering: Ordering::Less,
+			};
+			score.affiliation = run_and_score_threshold_analysis!(
+				alt_results,
+				phase,
 				score_tree,
 				AFFILIATION_PHASE,
-				commit_node,
-			) {
-				Ok(score_tree_inc) => {
-					score_tree = score_tree_inc;
-				}
-				_ => {
-					return Err(hc_error!(
-						"failed to complete {} scoring.",
-						AFFILIATION_PHASE
-					))
-				}
-			}
+				db.affiliation_analysis(),
+				db.affiliation_weight(),
+				spec,
+				commit_node
+			);
+			// This will be removed once results is deprecated in favor of alt_results
+			results.affiliation = Some(alt_results.table.get(AFFILIATION_PHASE).unwrap().clone());
 
 			/*===NEW_PHASE===*/
 			update_phase(phase, CHURN_PHASE)?;