refactor: Improve clarity of ConfigError variants

Resolves #931

Add new variants to replace all existing uses of `Unspecified`:
- `SetConfigBadFormat`
- `InternalError`
- `FileNotFound`
- `ConfigFileParseError`
- `EnvVarNotSet`

Signed-off-by: Cal Stepanian <[email protected]>

hipcheck-common/proto/hipcheck/v1/hipcheck.proto

@@ -108,6 +108,18 @@ enum ConfigurationStatus {
     CONFIGURATION_STATUS_UNRECOGNIZED_CONFIGURATION = 3;
     // The user provided a configuration item whose value is invalid.
     CONFIGURATION_STATUS_INVALID_CONFIGURATION_VALUE = 4;
+    // The JSON data sent by Hipcheck to the plugin was not recognized as the expected format.
+    CONFIGURATION_STATUS_SET_CONFIG_BAD_FORMAT = 5;
+    // The plugin encountered an internal error, probably due to incorrect assumptions.
+    CONFIGURATION_STATUS_INTERNAL_ERROR = 6;
+    // A necessary plugin input file was not found.
+    CONFIGURATION_STATUS_FILE_NOT_FOUND = 7;
+    // A configuration file could not be parsed correctly.
+    CONFIGURATION_STATUS_CONFIG_FILE_PARSE_ERROR = 8;
+    // An environment variable needed by the plugin was not set.
+    CONFIGURATION_STATUS_ENV_VAR_NOT_SET = 9;
+    // The plugin could not run a needed program.
+    CONFIGURATION_STATUS_MISSING_PROGRAM = 10;
 }
 
 /*===========================================================================

plugins/activity/src/main.rs

@@ -61,11 +61,12 @@ impl Plugin for ActivityPlugin {
 	const NAME: &'static str = "activity";
 
 	fn set_config(&self, config: Value) -> StdResult<(), ConfigError> {
-		let conf =
-			serde_json::from_value::<Config>(config).map_err(|e| ConfigError::Unspecified {
+		let conf = serde_json::from_value::<Config>(config).map_err(|e| {
+			ConfigError::SetConfigBadFormat {
 				message: e.to_string(),
-			})?;
-		CONFIG.set(conf).map_err(|_e| ConfigError::Unspecified {
+			}
+		})?;
+		CONFIG.set(conf).map_err(|_e| ConfigError::InternalError {
 			message: "config was already set".to_owned(),
 		})
 	}

plugins/affiliation/src/main.rs

@@ -50,13 +50,10 @@ impl TryFrom<RawConfig> for Config {
 		if let Some(ofv) = value.orgs_file_path {
 			// Get the Orgs file path and confirm it exists
 			let orgs_file = PathBuf::from(&ofv);
-			file::exists(&orgs_file).map_err(|_e| ConfigError::Unspecified {
-				// Print error with Debug for full context
-				message: format!("could not find an orgs file at path {:?}", ofv),
-			})?;
+			file::exists(&orgs_file).map_err(|_e| ConfigError::FileNotFound { file_path: ofv })?;
 			// Parse the Orgs file and construct an OrgSpec.
 			let orgs_spec =
-				OrgSpec::load_from(&orgs_file).map_err(|e| ConfigError::Unspecified {
+				OrgSpec::load_from(&orgs_file).map_err(|e| ConfigError::ConfigFileParseError {
 					// Print error with Debug for full context
 					message: format!("{:?}", e),
 				})?;
@@ -280,21 +277,21 @@ impl Plugin for AffiliationPlugin {
 
 	fn set_config(&self, config: Value) -> StdResult<(), ConfigError> {
 		let conf: Config = serde_json::from_value::<RawConfig>(config)
-			.map_err(|e| ConfigError::Unspecified {
+			.map_err(|e| ConfigError::SetConfigBadFormat {
 				message: e.to_string(),
 			})?
 			.try_into()?;
 
 		// Store the policy conf to be accessed only in the `default_policy_expr()` impl
 		self.policy_conf
 			.set(conf.count_threshold)
-			.map_err(|_| ConfigError::Unspecified {
+			.map_err(|_| ConfigError::InternalError {
 				message: "plugin was already configured".to_string(),
 			})?;
 
 		ORGSSPEC
 			.set(conf.orgs_spec)
-			.map_err(|_e| ConfigError::Unspecified {
+			.map_err(|_e| ConfigError::InternalError {
 				message: "orgs spec was already set".to_owned(),
 			})
 	}

plugins/binary/src/main.rs

@@ -80,26 +80,27 @@ impl Plugin for BinaryPlugin {
 	fn set_config(&self, config: Value) -> StdResult<(), ConfigError> {
 		// Deserialize and validate the config struct
 		let conf: Config = serde_json::from_value::<RawConfig>(config)
-			.map_err(|e| ConfigError::Unspecified {
+			.map_err(|e| ConfigError::SetConfigBadFormat {
 				message: e.to_string(),
 			})?
 			.try_into()?;
 
 		// Store the policy conf to be accessed only in the `default_policy_expr()` impl
 		self.policy_conf
 			.set(conf.opt_threshold)
-			.map_err(|_| ConfigError::Unspecified {
+			.map_err(|_| ConfigError::InternalError {
 				message: "plugin was already configured".to_string(),
 			})?;
 
 		// Use the langs file to create a SourceFileDetector and init the salsa db
-		let bfd =
-			BinaryFileDetector::load(conf.binary_file).map_err(|e| ConfigError::Unspecified {
+		let bfd = BinaryFileDetector::load(conf.binary_file).map_err(|e| {
+			ConfigError::ConfigFileParseError {
 				message: e.to_string_pretty_multiline(),
-			})?;
+			}
+		})?;
 
 		// Make the salsa db globally accessible
-		DETECTOR.set(bfd).map_err(|_e| ConfigError::Unspecified {
+		DETECTOR.set(bfd).map_err(|_e| ConfigError::InternalError {
 			message: "config was already set".to_owned(),
 		})
 	}

plugins/churn/src/main.rs

@@ -253,15 +253,15 @@ impl Plugin for ChurnPlugin {
 	fn set_config(&self, config: Value) -> StdResult<(), ConfigError> {
 		// Deserialize and validate the config struct
 		let conf: Config = serde_json::from_value::<RawConfig>(config)
-			.map_err(|e| ConfigError::Unspecified {
+			.map_err(|e| ConfigError::SetConfigBadFormat {
 				message: e.to_string(),
 			})?
 			.try_into()?;
 
 		// Store the PolicyExprConf to be accessed only in the `default_policy_expr()` impl
 		self.policy_conf
 			.set(conf.opt_policy)
-			.map_err(|_| ConfigError::Unspecified {
+			.map_err(|_| ConfigError::InternalError {
 				message: "plugin was already configured".to_string(),
 			})
 	}

plugins/entropy/src/main.rs

@@ -161,15 +161,15 @@ impl Plugin for EntropyPlugin {
 	fn set_config(&self, config: Value) -> StdResult<(), ConfigError> {
 		// Deserialize and validate the config struct
 		let conf: Config = serde_json::from_value::<RawConfig>(config)
-			.map_err(|e| ConfigError::Unspecified {
+			.map_err(|e| ConfigError::SetConfigBadFormat {
 				message: e.to_string(),
 			})?
 			.try_into()?;
 
 		// Store the PolicyExprConf to be accessed only in the `default_policy_expr()` impl
 		self.policy_conf
 			.set(conf.opt_policy)
-			.map_err(|_| ConfigError::Unspecified {
+			.map_err(|_| ConfigError::InternalError {
 				message: "plugin was already configured".to_string(),
 			})
 	}

plugins/git/src/main.rs

@@ -327,15 +327,16 @@ impl Plugin for GitPlugin {
 
 	fn set_config(&self, config: Value) -> std::result::Result<(), ConfigError> {
 		// Deserialize and validate the config struct
-		let conf: Config =
-			serde_json::from_value::<Config>(config).map_err(|e| ConfigError::Unspecified {
+		let conf: Config = serde_json::from_value::<Config>(config).map_err(|e| {
+			ConfigError::SetConfigBadFormat {
 				message: e.to_string(),
-			})?;
+			}
+		})?;
 		let cache_size = conf.commit_cache_size;
 
 		CACHE
 			.set(Mutex::new(LruCache::new(NonZero::new(cache_size).unwrap())))
-			.map_err(|_e| ConfigError::Unspecified {
+			.map_err(|_e| ConfigError::InternalError {
 				message: "config was already set".to_owned(),
 			})
 	}

plugins/github/src/main.rs

@@ -32,12 +32,11 @@ impl TryFrom<RawConfig> for Config {
 	type Error = ConfigError;
 	fn try_from(value: RawConfig) -> StdResult<Config, ConfigError> {
 		if let Some(atv) = value.api_token_var {
-			let api_token =
-				std::env::var(&atv).map_err(|_e| ConfigError::InvalidConfigValue {
-					field_name: "api-token-var".to_owned(),
-					value: atv.clone(),
-					reason: format!("Could not find an environment variable with the name \"{}\". This environment variable must contain a GitHub API token.", atv),
-				})?;
+			let api_token = std::env::var(&atv).map_err(|_e| ConfigError::EnvVarNotSet {
+				env_var_name: atv.clone(),
+				field_name: "api-token-var".to_owned(),
+				purpose: "This environment variable must contain a GitHub API token.".to_owned(),
+			})?;
 			Ok(Config { api_token })
 		} else {
 			Err(ConfigError::MissingRequiredConfig {
@@ -125,11 +124,11 @@ impl Plugin for GithubAPIPlugin {
 
 	fn set_config(&self, config: Value) -> StdResult<(), ConfigError> {
 		let conf: Config = serde_json::from_value::<RawConfig>(config)
-			.map_err(|e| ConfigError::Unspecified {
+			.map_err(|e| ConfigError::SetConfigBadFormat {
 				message: e.to_string(),
 			})?
 			.try_into()?;
-		CONFIG.set(conf).map_err(|_e| ConfigError::Unspecified {
+		CONFIG.set(conf).map_err(|_e| ConfigError::InternalError {
 			message: "config was already set".to_owned(),
 		})
 	}

plugins/identity/src/main.rs

@@ -109,13 +109,14 @@ impl Plugin for IdentityPlugin {
 
 	fn set_config(&self, config: Value) -> StdResult<(), ConfigError> {
 		// Deserialize the config struct
-		let conf =
-			serde_json::from_value::<Config>(config).map_err(|e| ConfigError::Unspecified {
+		let conf = serde_json::from_value::<Config>(config).map_err(|e| {
+			ConfigError::SetConfigBadFormat {
 				message: e.to_string(),
-			})?;
+			}
+		})?;
 		self.policy_conf
 			.set(conf.percent_threshold)
-			.map_err(|_| ConfigError::Unspecified {
+			.map_err(|_| ConfigError::InternalError {
 				message: "plugin was already configured".to_string(),
 			})?;
 		Ok(())

plugins/linguist/src/main.rs

@@ -38,13 +38,15 @@ impl Plugin for LinguistPlugin {
 
 	fn set_config(&self, config: Value) -> StdResult<(), ConfigError> {
 		let conf: Config =
-			serde_json::from_value(config).map_err(|e| ConfigError::Unspecified {
+			serde_json::from_value(config).map_err(|e| ConfigError::SetConfigBadFormat {
 				message: e.to_string(),
 			})?;
 		let sfd = match conf.langs_file {
-			Some(p) => SourceFileDetector::load(p).map_err(|e| ConfigError::Unspecified {
-				message: e.to_string_pretty_multiline(),
-			})?,
+			Some(p) => {
+				SourceFileDetector::load(p).map_err(|e| ConfigError::ConfigFileParseError {
+					message: e.to_string_pretty_multiline(),
+				})?
+			}
 			None => {
 				return Err(ConfigError::MissingRequiredConfig {
 					field_name: "langs-file".to_owned(),
@@ -53,7 +55,7 @@ impl Plugin for LinguistPlugin {
 				});
 			}
 		};
-		DETECTOR.set(sfd).map_err(|_e| ConfigError::Unspecified {
+		DETECTOR.set(sfd).map_err(|_e| ConfigError::InternalError {
 			message: "config was already set".to_owned(),
 		})
 	}

plugins/review/src/main.rs

@@ -75,11 +75,12 @@ impl Plugin for ReviewPlugin {
 	const NAME: &'static str = "review";
 
 	fn set_config(&self, config: Value) -> StdResult<(), ConfigError> {
-		let conf =
-			serde_json::from_value::<Config>(config).map_err(|e| ConfigError::Unspecified {
+		let conf = serde_json::from_value::<Config>(config).map_err(|e| {
+			ConfigError::SetConfigBadFormat {
 				message: e.to_string(),
-			})?;
-		CONFIG.set(conf).map_err(|_e| ConfigError::Unspecified {
+			}
+		})?;
+		CONFIG.set(conf).map_err(|_e| ConfigError::InternalError {
 			message: "config was already set".to_owned(),
 		})
 	}

plugins/typo/src/main.rs

@@ -48,7 +48,7 @@ impl TryFrom<RawConfig> for Config {
 		let typo_file = TypoFile::load_from(&typo_path).map_err(|e| {
 			// Print error with Debug for full context
 			log::error!("{:?}", e);
-			ConfigError::Unspecified {
+			ConfigError::ConfigFileParseError {
 				// Print error with Debug for full context
 				message: format!("{:?}", e),
 			}
@@ -117,21 +117,21 @@ impl Plugin for TypoPlugin {
 	fn set_config(&self, config: Value) -> StdResult<(), ConfigError> {
 		// Deserialize and validate the config struct
 		let conf: Config = serde_json::from_value::<RawConfig>(config)
-			.map_err(|e| ConfigError::Unspecified {
+			.map_err(|e| ConfigError::SetConfigBadFormat {
 				message: e.to_string(),
 			})?
 			.try_into()?;
 
 		// Store the policy conf to be accessed only in the `default_policy_expr()` impl
 		self.policy_conf
 			.set(conf.count_threshold)
-			.map_err(|_| ConfigError::Unspecified {
+			.map_err(|_| ConfigError::InternalError {
 				message: "plugin was already configured".to_string(),
 			})?;
 
 		TYPOFILE
 			.set(conf.typo_file)
-			.map_err(|_e| ConfigError::Unspecified {
+			.map_err(|_e| ConfigError::InternalError {
 				message: "config was already set".to_owned(),
 			})
 	}

sdk/rust/src/error.rs

@@ -138,6 +138,31 @@ pub enum ConfigError {
 
 	/// An unspecified error
 	Unspecified { message: String },
+
+	/// The JSON data sent by Hipcheck to the plugin was not recognized as the expected format.
+	SetConfigBadFormat { message: String },
+
+	/// The plugin encountered an error, probably due to incorrect assumptions.
+	InternalError { message: String },
+
+	/// A necessary plugin input file was not found.
+	FileNotFound { file_path: String },
+
+	/// A configuration file could not be parsed correctly.
+	ConfigFileParseError { message: String },
+
+	/// An environment variable needed by the plugin was not set.
+	EnvVarNotSet {
+		/// Name of the environment variable
+		env_var_name: String,
+		/// Config field that set the variable name
+		field_name: String,
+		/// Message describing what the environment variable should contain
+		purpose: String,
+	},
+
+	/// The plugin could not run a needed program.
+	MissingProgram { program_name: String },
 }
 
 impl From<ConfigError> for SetConfigurationResponse {
@@ -192,6 +217,34 @@ impl From<ConfigError> for SetConfigurationResponse {
 				status: ConfigurationStatus::Unspecified as i32,
 				message,
 			},
+			ConfigError::SetConfigBadFormat { message } => SetConfigurationResponse {
+				status: ConfigurationStatus::SetConfigBadFormat as i32,
+				message: format!("The JSON data sent by Hipcheck to the plugin was not recognized as the expected format: {message}"),
+			},
+			ConfigError::InternalError { message } => SetConfigurationResponse {
+				status: ConfigurationStatus::InternalError as i32,
+				message: format!("The plugin encountered an error, probably due to incorrect assumptions: {message}"),
+			},
+			ConfigError::FileNotFound { file_path } => SetConfigurationResponse {
+				status: ConfigurationStatus::FileNotFound as i32,
+				message: format!("File not found at path {file_path}"),
+			},
+			ConfigError::ConfigFileParseError { message } => SetConfigurationResponse {
+				status: ConfigurationStatus::ConfigFileParseError as i32,
+				message: format!("A configuration file could not be parsed correctly: {message}"),
+			},
+			ConfigError::EnvVarNotSet {
+				env_var_name,
+				field_name,
+				purpose,
+			} => SetConfigurationResponse {
+				status: ConfigurationStatus::EnvVarNotSet as i32,
+				message: format!("Could not find an environment variable with the name \"{env_var_name}\" (set by config field '{field_name}'). Purpose: {purpose}"),
+			},
+			ConfigError::MissingProgram { program_name } => SetConfigurationResponse {
+				status: ConfigurationStatus::MissingProgram as i32,
+				message: format!("The plugin could not run a needed program: {program_name}"),
+			},
 		}
 	}
 }