Merge pull request #7431 from Turbo87/less-panic-more-qms

Return `anyhow::Result` from more functions instead of panicking

Author: Turbo87

crates_io_index/repo.rs

@@ -13,7 +13,7 @@ pub struct RepositoryConfig {
 }
 
 impl RepositoryConfig {
-    pub fn from_environment() -> Self {
+    pub fn from_environment() -> anyhow::Result<Self> {
         let username = dotenvy::var("GIT_HTTP_USER");
         let password = dotenvy::var("GIT_HTTP_PWD").map(SecretString::from);
         let http_url = dotenvy::var("GIT_REPO_URL");
@@ -38,30 +38,30 @@ impl RepositoryConfig {
                     String::from_utf8(key).expect("failed to convert the ssh key to a string");
                 let credentials = Credentials::Ssh { key: key.into() };
 
-                Self {
+                Ok(Self {
                     index_location,
                     credentials,
-                }
+                })
             }
             (Ok(username), Ok(password), Ok(http_url), Err(_), Err(_)) => {
                 let index_location = Url::parse(&http_url).expect("failed to parse GIT_REPO_URL");
                 let credentials = Credentials::Http { username, password };
 
-                Self {
+                Ok(Self {
                     index_location,
                     credentials,
-                }
+                })
             }
             (_, _, Ok(http_url), _, _) => {
                 let index_location = Url::parse(&http_url).expect("failed to parse GIT_REPO_URL");
                 let credentials = Credentials::Missing;
 
-                Self {
+                Ok(Self {
                     index_location,
                     credentials,
-                }
+                })
             }
-            _ => panic!("must have `GIT_REPO_URL` defined"),
+            _ => Err(anyhow!("must have `GIT_REPO_URL` defined")),
         }
     }
 }

src/admin/git_import.rs

@@ -30,7 +30,7 @@ pub struct Opts {
 pub fn run(opts: Opts) -> anyhow::Result<()> {
     let mut conn = db::oneoff_connection()?;
     println!("fetching git repo");
-    let config = RepositoryConfig::from_environment();
+    let config = RepositoryConfig::from_environment()?;
     let repo = Repository::open(&config)?;
     repo.reset_head()?;
     println!("HEAD is at {}", repo.head_oid()?);

src/admin/migrate.rs

@@ -16,8 +16,8 @@ pub struct Opts;
 
 pub fn run(_opts: Opts) -> Result<(), Error> {
     let config = crate::config::DatabasePools::full_from_environment(
-        &crate::config::Base::from_environment(),
-    );
+        &crate::config::Base::from_environment()?,
+    )?;
 
     // TODO: Refactor logic so that we can also check things from App::new() here.
     // If the app will panic due to bad configuration, it is better to error in the release phase

src/admin/upload_index.rs

@@ -18,7 +18,7 @@ pub fn run(opts: Opts) -> anyhow::Result<()> {
     let storage = Storage::from_environment();
 
     println!("fetching git repo");
-    let config = RepositoryConfig::from_environment();
+    let config = RepositoryConfig::from_environment()?;
     let repo = Repository::open(&config)?;
     repo.reset_head()?;
     println!("HEAD is at {}", repo.head_oid()?);

src/bin/background-worker.rs

@@ -32,7 +32,7 @@ use std::sync::Arc;
 use std::thread::sleep;
 use std::time::{Duration, Instant};
 
-fn main() {
+fn main() -> anyhow::Result<()> {
     let _sentry = crates_io::sentry::init();
 
     // Initialize logging
@@ -42,7 +42,7 @@ fn main() {
 
     info!("Booting runner");
 
-    let config = config::Server::default();
+    let config = config::Server::from_environment()?;
 
     if config.db.are_all_read_only() {
         loop {
@@ -65,7 +65,7 @@ fn main() {
     }
 
     let clone_start = Instant::now();
-    let repository_config = RepositoryConfig::from_environment();
+    let repository_config = RepositoryConfig::from_environment()?;
     let repository = Repository::open(&repository_config).expect("Failed to clone index");
 
     let clone_duration = clone_start.elapsed();

src/bin/server.rs

@@ -18,15 +18,15 @@ use tower::Layer;
 
 const CORE_THREADS: usize = 4;
 
-fn main() -> Result<(), Box<dyn std::error::Error>> {
+fn main() -> anyhow::Result<()> {
     let _sentry = crates_io::sentry::init();
 
     // Initialize logging
     crates_io::util::tracing::init();
 
     let _span = info_span!("server.run");
 
-    let config = crates_io::config::Server::default();
+    let config = crates_io::config::Server::from_environment()?;
     let client = Client::new();
     let app = Arc::new(App::new(config, Some(client)));
 

src/config/balance_capacity.rs

@@ -10,14 +10,14 @@ pub struct BalanceCapacityConfig {
 }
 
 impl BalanceCapacityConfig {
-    pub fn from_environment() -> Self {
-        Self {
+    pub fn from_environment() -> anyhow::Result<Self> {
+        Ok(Self {
             report_only: env::var("WEB_CAPACITY_REPORT_ONLY").is_ok(),
             log_total_at_count: env_optional("WEB_CAPACITY_LOG_TOTAL_AT_COUNT").unwrap_or(50),
             // The following are a percentage of `db_capacity`
             log_at_percentage: env_optional("WEB_CAPACITY_LOG_PCT").unwrap_or(50),
             throttle_at_percentage: env_optional("WEB_CAPACITY_THROTTLE_PCT").unwrap_or(70),
             dl_only_at_percentage: env_optional("WEB_CAPACITY_DL_ONLY_PCT").unwrap_or(80),
-        }
+        })
     }
 }

src/config/base.rs

@@ -9,12 +9,12 @@ pub struct Base {
 }
 
 impl Base {
-    pub fn from_environment() -> Self {
+    pub fn from_environment() -> anyhow::Result<Self> {
         let env = match dotenvy::var("HEROKU") {
             Ok(_) => Env::Production,
             _ => Env::Development,
         };
 
-        Self { env }
+        Ok(Self { env })
     }
 }

src/config/database_pools.rs

@@ -63,7 +63,7 @@ impl DatabasePools {
     /// # Panics
     ///
     /// This function panics if `DB_OFFLINE=leader` but `READ_ONLY_REPLICA_URL` is unset.
-    pub fn full_from_environment(base: &Base) -> Self {
+    pub fn full_from_environment(base: &Base) -> anyhow::Result<Self> {
         let leader_url = env("DATABASE_URL").into();
         let follower_url = dotenvy::var("READ_ONLY_REPLICA_URL").map(Into::into).ok();
         let read_only_mode = dotenvy::var("READ_ONLY_MODE").is_ok();
@@ -110,7 +110,7 @@ impl DatabasePools {
 
         let enforce_tls = base.env == Env::Production;
 
-        match dotenvy::var("DB_OFFLINE").as_deref() {
+        Ok(match dotenvy::var("DB_OFFLINE").as_deref() {
             // The actual leader is down, use the follower in read-only mode as the primary and
             // don't configure a replica.
             Ok("leader") => Self {
@@ -165,6 +165,6 @@ impl DatabasePools {
                 helper_threads,
                 enforce_tls,
             },
-        }
+        })
     }
 }

src/config/sentry.rs

@@ -1,4 +1,5 @@
 use crate::env_optional;
+use anyhow::Context;
 use sentry::types::Dsn;
 use sentry::IntoDsn;
 
@@ -10,22 +11,25 @@ pub struct SentryConfig {
 }
 
 impl SentryConfig {
-    pub fn from_environment() -> Self {
+    pub fn from_environment() -> anyhow::Result<Self> {
         let dsn = dotenvy::var("SENTRY_DSN_API")
             .ok()
             .into_dsn()
-            .expect("SENTRY_DSN_API is not a valid Sentry DSN value");
+            .context("SENTRY_DSN_API is not a valid Sentry DSN value")?;
 
-        let environment = dsn.as_ref().map(|_| {
-            dotenvy::var("SENTRY_ENV_API")
-                .expect("SENTRY_ENV_API must be set when using SENTRY_DSN_API")
-        });
+        let environment = match dsn {
+            None => None,
+            Some(_) => Some(
+                dotenvy::var("SENTRY_ENV_API")
+                    .context("SENTRY_ENV_API must be set when using SENTRY_DSN_API")?,
+            ),
+        };
 
-        Self {
+        Ok(Self {
             dsn,
             environment,
             release: dotenvy::var("HEROKU_SLUG_COMMIT").ok(),
             traces_sample_rate: env_optional("SENTRY_TRACES_SAMPLE_RATE").unwrap_or(0.0),
-        }
+        })
     }
 }

src/config/server.rs

@@ -65,7 +65,7 @@ pub struct Server {
     pub content_security_policy: Option<HeaderValue>,
 }
 
-impl Default for Server {
+impl Server {
     /// Returns a default value for the application's config
     ///
     /// Sets the following default values:
@@ -102,7 +102,7 @@ impl Default for Server {
     /// # Panics
     ///
     /// This function panics if the Server configuration is invalid.
-    fn default() -> Self {
+    pub fn from_environment() -> anyhow::Result<Self> {
         let docker = dotenvy::var("DEV_DOCKER").is_ok();
         let heroku = dotenvy::var("HEROKU").is_ok();
 
@@ -114,7 +114,7 @@ impl Default for Server {
 
         let port = env_optional("PORT").unwrap_or(8888);
 
-        let allowed_origins = AllowedOrigins::from_default_env();
+        let allowed_origins = AllowedOrigins::from_default_env()?;
         let page_offset_ua_blocklist = match env_optional::<String>("WEB_PAGE_OFFSET_UA_BLOCKLIST")
         {
             None => vec![],
@@ -128,11 +128,10 @@ impl Default for Server {
                 Some(s) => s
                     .split(',')
                     .map(parse_cidr_block)
-                    .collect::<Result<_, _>>()
-                    .unwrap(),
+                    .collect::<Result<_, _>>()?,
             };
 
-        let base = Base::from_environment();
+        let base = Base::from_environment()?;
         let excluded_crate_names = match env_optional::<String>("EXCLUDED_CRATE_NAMES") {
             None => vec![],
             Some(s) if s.is_empty() => vec![],
@@ -176,8 +175,8 @@ impl Default for Server {
             cdn_domain = storage.cdn_prefix.as_ref().map(|cdn_prefix| format!("https://{cdn_prefix}")).unwrap_or_default()
         );
 
-        Server {
-            db: DatabasePools::full_from_environment(&base),
+        Ok(Server {
+            db: DatabasePools::full_from_environment(&base)?,
             storage,
             base,
             ip,
@@ -221,11 +220,11 @@ impl Default for Server {
             ),
             cdn_user_agent: dotenvy::var("WEB_CDN_USER_AGENT")
                 .unwrap_or_else(|_| "Amazon CloudFront".into()),
-            balance_capacity: BalanceCapacityConfig::from_environment(),
+            balance_capacity: BalanceCapacityConfig::from_environment()?,
             serve_dist: true,
             serve_html: true,
-            content_security_policy: Some(content_security_policy.parse().unwrap()),
-        }
+            content_security_policy: Some(content_security_policy.parse()?),
+        })
     }
 }
 
@@ -291,13 +290,13 @@ fn parse_traffic_patterns(patterns: &str) -> impl Iterator<Item = (&str, &str)>
 pub struct AllowedOrigins(Vec<String>);
 
 impl AllowedOrigins {
-    pub fn from_default_env() -> Self {
+    pub fn from_default_env() -> anyhow::Result<Self> {
         let allowed_origins = env("WEB_ALLOWED_ORIGINS")
             .split(',')
             .map(ToString::to_string)
             .collect();
 
-        Self(allowed_origins)
+        Ok(Self(allowed_origins))
     }
 
     pub fn contains(&self, value: &HeaderValue) -> bool {

src/db.rs

@@ -174,9 +174,9 @@ pub fn oneoff_connection_with_config(
     PgConnection::establish(&url)
 }
 
-pub fn oneoff_connection() -> ConnectionResult<PgConnection> {
-    let config = config::DatabasePools::full_from_environment(&config::Base::from_environment());
-    oneoff_connection_with_config(&config)
+pub fn oneoff_connection() -> anyhow::Result<PgConnection> {
+    let config = config::DatabasePools::full_from_environment(&config::Base::from_environment()?)?;
+    oneoff_connection_with_config(&config).map_err(Into::into)
 }
 
 pub fn connection_url(config: &config::DatabasePools, url: &str) -> String {

src/sentry/mod.rs

@@ -10,8 +10,14 @@ use std::sync::Arc;
 ///
 /// `HEROKU_SLUG_COMMIT`, if present, will be used as the `release` property
 /// on all events.
-pub fn init() -> ClientInitGuard {
-    let config = SentryConfig::from_environment();
+pub fn init() -> Option<ClientInitGuard> {
+    let config = match SentryConfig::from_environment() {
+        Ok(config) => config,
+        Err(error) => {
+            warn!(%error, "Failed to read Sentry configuration from environment");
+            return None;
+        }
+    };
 
     let traces_sampler = move |ctx: &TransactionContext| -> f32 {
         if let Some(sampled) = ctx.sampled() {
@@ -55,5 +61,5 @@ pub fn init() -> ClientInitGuard {
         ..Default::default()
     };
 
-    sentry::init(opts)
+    Some(sentry::init(opts))
 }