Auto merge of #2072 - jtgeibel:fix/remove-some-unwraps, r=carols10cents

Remove some usage of `unwrap()`

This commit reduces the usage of unwrap within the request/response
lifecycle, and documents a few remaining cases.  Usage of unwrap in
tests, binaries, or server boot code is generally okay and was not
reviewed.

Remaining work:

* Some usage of unwrap in `git.rs` and `src/tasks/` background jobs.
   This code is not directly in the request/response lifecycle, but
  should be reviewed.
* Current usage in middleware seems okay, but could be documented.
  In most cases, such as with the handler in `AroundMiddleware`, the
  conduit API ensures a handler is set.

Author: bors

src/controllers/krate/search.rs

@@ -7,6 +7,7 @@ use crate::controllers::cargo_prelude::*;
 use crate::controllers::helpers::Paginate;
 use crate::models::{Crate, CrateBadge, CrateOwner, CrateVersions, OwnerKind, Version};
 use crate::schema::*;
+use crate::util::errors::{bad_request, ChainError};
 use crate::views::EncodableCrate;
 
 use crate::models::krate::{canon_crate_name, ALL_COLUMNS};
@@ -110,7 +111,7 @@ pub fn search(req: &mut dyn Request) -> AppResult<Response> {
             letter
                 .chars()
                 .next()
-                .unwrap()
+                .chain_error(|| bad_request("letter value must contain 1 character"))?
                 .to_lowercase()
                 .collect::<String>()
         );

src/controllers/user/me.rs

@@ -4,7 +4,7 @@ use crate::controllers::frontend_prelude::*;
 
 use crate::controllers::helpers::*;
 use crate::email;
-use crate::util::errors::AppError;
+use crate::util::errors::{AppError, ChainError};
 
 use crate::models::{
     CrateOwner, Email, Follow, NewEmail, OwnerKind, User, Version, VersionOwnerAction,
@@ -141,12 +141,10 @@ pub fn update_user(req: &mut dyn Request) -> AppResult<Response> {
     let user_update: UserUpdate =
         serde_json::from_str(&body).map_err(|_| bad_request("invalid json request"))?;
 
-    if user_update.user.email.is_none() {
-        return Err(bad_request("empty email rejected"));
-    }
-
-    let user_email = user_update.user.email.unwrap();
-    let user_email = user_email.trim();
+    let user_email = match &user_update.user.email {
+        Some(email) => email.trim(),
+        None => return Err(bad_request("empty email rejected")),
+    };
 
     if user_email == "" {
         return Err(bad_request("empty email rejected"));
@@ -199,7 +197,9 @@ pub fn regenerate_token_and_send(req: &mut dyn Request) -> AppResult<Response> {
     use diesel::update;
 
     let user = req.user()?;
-    let name = &req.params()["user_id"].parse::<i32>().ok().unwrap();
+    let name = &req.params()["user_id"]
+        .parse::<i32>()
+        .chain_error(|| bad_request("invalid user_id"))?;
     let conn = req.db_conn()?;
 
     // need to check if current user matches user to be updated

src/controllers/user/other.rs

@@ -2,6 +2,7 @@ use crate::controllers::frontend_prelude::*;
 
 use crate::models::{CrateOwner, OwnerKind, User};
 use crate::schema::{crate_owners, crates, users};
+use crate::util::errors::ChainError;
 use crate::views::EncodablePublicUser;
 
 /// Handles the `GET /users/:user_id` route.
@@ -28,7 +29,9 @@ pub fn show(req: &mut dyn Request) -> AppResult<Response> {
 pub fn stats(req: &mut dyn Request) -> AppResult<Response> {
     use diesel::dsl::sum;
 
-    let user_id = &req.params()["user_id"].parse::<i32>().ok().unwrap();
+    let user_id = &req.params()["user_id"]
+        .parse::<i32>()
+        .chain_error(|| bad_request("invalid user_id"))?;
     let conn = req.db_conn()?;
 
     let data = CrateOwner::by_owner_kind(OwnerKind::User)

src/middleware/block_traffic.rs

@@ -54,6 +54,7 @@ impl Handler for BlockTraffic {
                  Please open an issue at https://github.com/rust-lang/crates.io \
                  or email [email protected] \
                  and provide the request id {}",
+                // Heroku should always set this header
                 req.headers().find("X-Request-Id").unwrap()[0]
             );
             let mut headers = HashMap::new();

src/models/badge.rs

@@ -109,6 +109,7 @@ impl Queryable<badges::SqlType, Pg> for Badge {
 
 impl Badge {
     pub fn encodable(self) -> EncodableBadge {
+        // The serde attributes on Badge ensure it can be deserialized to EncodableBadge
         serde_json::from_value(serde_json::to_value(self).unwrap()).unwrap()
     }
 

src/models/keyword.rs

@@ -53,6 +53,7 @@ impl Keyword {
         if name.is_empty() {
             return false;
         }
+        // unwrap is okay because name is non-empty
         name.chars().next().unwrap().is_alphanumeric()
             && name
                 .chars()

src/models/team.rs

@@ -68,6 +68,10 @@ impl<'a> NewTeam<'a> {
 
 impl Team {
     /// Tries to create the Team in the DB (assumes a `:` has already been found).
+    ///
+    /// # Panics
+    ///
+    /// This function will panic if login contains less than 2 `:` characters.
     pub fn create_or_update(
         app: &App,
         conn: &PgConnection,
@@ -76,10 +80,11 @@ impl Team {
     ) -> AppResult<Self> {
         // must look like system:xxxxxxx
         let mut chunks = login.split(':');
+        // unwrap is okay, split on an empty string still has 1 chunk
         match chunks.next().unwrap() {
             // github:rust-lang:owners
             "github" => {
-                // Ok to unwrap since we know one ":" is contained
+                // unwrap is documented above as part of the calling contract
                 let org = chunks.next().unwrap();
                 let team = chunks.next().ok_or_else(|| {
                     cargo_err(

src/s3/Cargo.toml

@@ -6,6 +6,7 @@ authors = ["Alex Crichton <[email protected]>"]
 license = "MIT OR Apache-2.0"
 repository = "https://github.com/rust-lang/crates.io"
 description = "Interaction between crates.io and S3 for storing crate files"
+edition = "2018"
 
 [lib]
 

src/s3/error.rs

@@ -0,0 +1,33 @@
+use std::fmt;
+
+use openssl::error::ErrorStack;
+use reqwest::Error as ReqwestError;
+
+#[derive(Debug)]
+pub enum Error {
+    Openssl(ErrorStack),
+    Reqwest(ReqwestError),
+}
+
+impl From<ErrorStack> for Error {
+    fn from(stack: ErrorStack) -> Self {
+        Self::Openssl(stack)
+    }
+}
+
+impl From<ReqwestError> for Error {
+    fn from(error: ReqwestError) -> Self {
+        Self::Reqwest(error)
+    }
+}
+
+impl fmt::Display for Error {
+    fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
+        match self {
+            Self::Openssl(stack) => stack.fmt(f),
+            Self::Reqwest(error) => error.fmt(f),
+        }
+    }
+}
+
+impl std::error::Error for Error {}

src/s3/lib.rs

@@ -1,16 +1,15 @@
 #![warn(clippy::all, rust_2018_idioms)]
 
-extern crate base64;
-extern crate chrono;
-extern crate openssl;
-extern crate reqwest;
-
 use base64::encode;
 use chrono::prelude::Utc;
+use openssl::error::ErrorStack;
 use openssl::hash::MessageDigest;
 use openssl::pkey::PKey;
 use openssl::sign::Signer;
-use reqwest::header;
+use reqwest::{header, Body, Client, Response};
+
+mod error;
+pub use error::Error;
 
 #[derive(Clone, Debug)]
 pub struct Bucket {
@@ -40,20 +39,20 @@ impl Bucket {
 
     pub fn put<R: std::io::Read + Send + 'static>(
         &self,
-        client: &reqwest::Client,
+        client: &Client,
         path: &str,
         content: R,
         content_length: u64,
         content_type: &str,
         extra_headers: header::HeaderMap,
-    ) -> reqwest::Result<reqwest::Response> {
+    ) -> Result<Response, Error> {
         let path = if path.starts_with('/') {
             &path[1..]
         } else {
             path
         };
         let date = Utc::now().to_rfc2822();
-        let auth = self.auth("PUT", &date, path, "", content_type);
+        let auth = self.auth("PUT", &date, path, "", content_type)?;
         let url = self.url(path);
 
         client
@@ -62,23 +61,20 @@ impl Bucket {
             .header(header::CONTENT_TYPE, content_type)
             .header(header::DATE, date)
             .headers(extra_headers)
-            .body(reqwest::Body::sized(content, content_length))
+            .body(Body::sized(content, content_length))
             .send()?
             .error_for_status()
+            .map_err(Into::into)
     }
 
-    pub fn delete(
-        &self,
-        client: &reqwest::Client,
-        path: &str,
-    ) -> reqwest::Result<reqwest::Response> {
+    pub fn delete(&self, client: &Client, path: &str) -> Result<Response, Error> {
         let path = if path.starts_with('/') {
             &path[1..]
         } else {
             path
         };
         let date = Utc::now().to_rfc2822();
-        let auth = self.auth("DELETE", &date, path, "", "");
+        let auth = self.auth("DELETE", &date, path, "", "")?;
         let url = self.url(path);
 
         client
@@ -87,6 +83,7 @@ impl Bucket {
             .header(header::AUTHORIZATION, auth)
             .send()?
             .error_for_status()
+            .map_err(Into::into)
     }
 
     pub fn host(&self) -> String {
@@ -101,7 +98,14 @@ impl Bucket {
         )
     }
 
-    fn auth(&self, verb: &str, date: &str, path: &str, md5: &str, content_type: &str) -> String {
+    fn auth(
+        &self,
+        verb: &str,
+        date: &str,
+        path: &str,
+        md5: &str,
+        content_type: &str,
+    ) -> Result<String, ErrorStack> {
         let string = format!(
             "{verb}\n{md5}\n{ty}\n{date}\n{headers}{resource}",
             verb = verb,
@@ -112,12 +116,12 @@ impl Bucket {
             resource = format!("/{}/{}", self.name, path)
         );
         let signature = {
-            let key = PKey::hmac(self.secret_key.as_bytes()).unwrap();
-            let mut signer = Signer::new(MessageDigest::sha1(), &key).unwrap();
-            signer.update(string.as_bytes()).unwrap();
-            encode(&signer.sign_to_vec().unwrap()[..])
+            let key = PKey::hmac(self.secret_key.as_bytes())?;
+            let mut signer = Signer::new(MessageDigest::sha1(), &key)?;
+            signer.update(string.as_bytes())?;
+            encode(&signer.sign_to_vec()?[..])
         };
-        format!("AWS {}:{}", self.access_key, signature)
+        Ok(format!("AWS {}:{}", self.access_key, signature))
     }
 
     fn url(&self, path: &str) -> String {

src/uploaders.rs

@@ -1,5 +1,6 @@
 use conduit::Request;
 use flate2::read::GzDecoder;
+use openssl::error::ErrorStack;
 use openssl::hash::{Hasher, MessageDigest};
 use reqwest::header;
 
@@ -88,6 +89,11 @@ impl Uploader {
     /// Uploads a file using the configured uploader (either `S3`, `Local`).
     ///
     /// It returns the path of the uploaded file.
+    ///
+    /// # Panics
+    ///
+    /// This function can panic on an `Self::Local` during development.
+    /// Production and tests use `Self::S3` which should not panic.
     pub fn upload<R: std::io::Read + Send + 'static>(
         &self,
         client: &reqwest::Client,
@@ -135,7 +141,7 @@ impl Uploader {
         let mut body = Vec::new();
         LimitErrorReader::new(req.body(), maximums.max_upload_size).read_to_end(&mut body)?;
         verify_tarball(krate, vers, &body, maximums.max_unpack_size)?;
-        let checksum = hash(&body);
+        let checksum = hash(&body)?;
         let content_length = body.len() as u64;
         let content = Cursor::new(body);
         let mut extra_headers = header::HeaderMap::new();
@@ -221,8 +227,8 @@ fn verify_tarball(
     Ok(())
 }
 
-fn hash(data: &[u8]) -> Vec<u8> {
-    let mut hasher = Hasher::new(MessageDigest::sha256()).unwrap();
-    hasher.update(data).unwrap();
-    hasher.finish().unwrap().to_vec()
+fn hash(data: &[u8]) -> Result<Vec<u8>, ErrorStack> {
+    let mut hasher = Hasher::new(MessageDigest::sha256())?;
+    hasher.update(data)?;
+    Ok(hasher.finish()?.to_vec())
 }

src/util.rs

@@ -17,6 +17,13 @@ mod request_helpers;
 mod request_proxy;
 pub mod rfc3339;
 
+/// Serialize a value to JSON and build a status 200 Response
+///
+/// This helper sets appropriate values for `Content-Type` and `Content-Length`.
+///
+/// # Panics
+///
+/// This function will panic if serialization fails.
 pub fn json_response<T: Serialize>(t: &T) -> Response {
     let json = serde_json::to_string(t).unwrap();
     let mut headers = HashMap::new();