chore: refine comments, add logging for sqlx/anyhow errors

Author: abonander

.github/workflows/rust.yml

@@ -49,6 +49,7 @@ jobs:
       uses: actions/checkout@v2
       with:
         repository: gothinkster/realworld
+        # FIXME: we actually require this PR for the tests to pass: https://github.com/gothinkster/realworld/pull/490
         ref: a3855ae3a346e7bcb809dcddcc057bd13edc0c19
         path: realworld
     - name: Run Realworld Postman collection

Cargo.toml

@@ -34,7 +34,6 @@ sha2 = "0.9.8"
 
 time = "0.2"
 
-
 uuid = { version = "0.8", features = ["serde"] }
 
 # Utility Crates

src/http/articles/mod.rs

@@ -144,7 +144,9 @@ async fn create_article(
     // Never specified unless you count just showing them sorted in the examples:
     // https://realworld-docs.netlify.app/docs/specs/backend-specs/api-response-format#single-article
     //
-    // However, it is required by the Postman collection.
+    // However, it is required by the Postman collection. To their credit, the Realworld authors
+    // have acknowledged this oversight and are willing to loosen the requirement:
+    // https://github.com/gothinkster/realworld/issues/839#issuecomment-1002806224
     req.article.tag_list.sort();
 
     // For fun, this is how we combine several operations into a single query for brevity.
@@ -424,7 +426,6 @@ async fn favorite_article(
     .await?
     .ok_or(Error::NotFound)?;
 
-    // It didn't really seem necessary to put this in a transaction.
     Ok(Json(ArticleBody {
         article: article_by_id(&ctx.db, auth_user.user_id, article_id).await?,
     }))
@@ -438,6 +439,8 @@ async fn unfavorite_article(
 ) -> Result<Json<ArticleBody>> {
     // The Realworld spec doesn't say what to do if the user calls this on an article
     // that they haven't favorited. I've chosen to just do nothing as that's the easiest.
+    //
+    // The Postman collection doesn't test that case.
 
     let article_id = sqlx::query_scalar!(
         r#"
@@ -458,7 +461,6 @@ async fn unfavorite_article(
     .await?
     .ok_or(Error::NotFound)?;
 
-    // It didn't really seem necessary to put this in a transaction.
     Ok(Json(ArticleBody {
         article: article_by_id(&ctx.db, auth_user.user_id, article_id).await?,
     }))
@@ -492,6 +494,10 @@ async fn get_tags(ctx: Extension<ApiContext>) -> Result<Json<TagsBody>> {
 // End handler functions.
 // Begin utility functions.
 
+// This is used in a few different places so it makes sense to extract into its own function.
+//
+// I usually throw stuff like this at the bottom of the file but other engineers like
+// to put these kinds of functions in their own modules. Po-tay-to po-tah-to.
 async fn article_by_id(
     e: impl Executor<'_, Database = Postgres>,
     user_id: Uuid,
@@ -538,6 +544,8 @@ async fn article_by_id(
 /// Convert a title string to a slug for identifying an article.
 ///
 /// E.g. `slugify("Doctests are the Bee's Knees") == "doctests-are-the-bees-knees"`
+///
+// (Sadly, doctests are not run on private functions it seems.)
 fn slugify(string: &str) -> String {
     const QUOTE_CHARS: &[char] = &['\'', '"'];
 

src/http/error.rs

@@ -36,8 +36,8 @@ pub enum Error {
     ///
     /// For a good API, the other status codes should also ideally map to some sort of JSON body
     /// that the frontend can deal with, but I do admit sometimes I've just gotten lazy and
-    /// returned a plain error message if I can get away with just using distinct status codes
-    /// without deviating too much from their specifications.
+    /// returned a plain error message if there were few enough error modes for a route
+    /// that the frontend could infer the error from the status code alone.
     #[error("error in the request body")]
     UnprocessableEntity {
         errors: HashMap<Cow<'static, str>, Vec<Cow<'static, str>>>,
@@ -48,12 +48,15 @@ pub enum Error {
     /// Via the generated `From<sqlx::Error> for Error` impl,
     /// this allows using `?` on database calls in handler functions without a manual mapping step.
     ///
+    /// I highly recommend creating an error type like this if only to make handler function code
+    /// nicer; code in Actix-web projects that we started before I settled on this pattern is
+    /// filled with `.map_err(ErrInternalServerError)?` which is a *ton* of unnecessary noise.
+    ///
     /// The actual error message isn't returned to the client for security reasons.
     /// It should be logged instead.
     ///
     /// Note that this could also contain database constraint errors, which should usually
-    /// be transformed into client errors (e.g. `422 Unprocessable Entity`).
-    ///
+    /// be transformed into client errors (e.g. `422 Unprocessable Entity` or `409 Conflict`).
     /// See `ResultExt` below for a convenient way to do this.
     #[error("an error occurred with the database")]
     Sqlx(#[from] sqlx::Error),
@@ -62,7 +65,8 @@ pub enum Error {
     ///
     /// `anyhow::Error` is used in a few places to capture context and backtraces
     /// on unrecoverable (but technically non-fatal) errors which could be highly useful for
-    /// debugging.
+    /// debugging. We use it a lot in our code for background tasks or making API calls
+    /// to external services so we can use `.context()` to refine the logged error.
     ///
     /// Via the generated `From<anyhow::Error> for Error` impl, this allows the
     /// use of `?` in handler functions to automatically convert `anyhow::Error` into a response.
@@ -124,7 +128,7 @@ impl IntoResponse for Error {
                     errors: HashMap<Cow<'static, str>, Vec<Cow<'static, str>>>,
                 }
 
-                (StatusCode::UNPROCESSABLE_ENTITY, Json(Errors { errors })).into_response()
+                return (StatusCode::UNPROCESSABLE_ENTITY, Json(Errors { errors })).into_response();
             }
             Self::Unauthorized => (
                 self.status_code(),
@@ -137,17 +141,30 @@ impl IntoResponse for Error {
                 //
                 // However, at Launchbadge we try to adhere to web standards wherever possible,
                 // if nothing else than to try to act as a vanguard of sanity on the web.
-                //
-                // "Your scientists were so preoccupied with whether or not they *could*,
-                //  they didn't stop to think if they *should*."
                 [(WWW_AUTHENTICATE, HeaderValue::from_static("Token"))]
                     .into_iter()
                     .collect::<HeaderMap>(),
                 self.to_string(),
             )
                 .into_response(),
-            other => (other.status_code(), other.to_string()).into_response(),
+
+            Self::Sqlx(e) => {
+                // TODO: we probably want to use `tracing` instead
+                // so that this gets linked to the HTTP request by `TraceLayer`.
+                log::error!("SQLx error: {:?}", e);
+            }
+
+            Self::Anyhow(e) => {
+                // TODO: we probably want to use `tracing` instead
+                // so that this gets linked to the HTTP request by `TraceLayer`.
+                log::error!("Generic error: {:?}", e);
+            }
+
+            // Other errors get mapped normally.
+            _ => (),
         }
+
+        (self.status_code(), self.to_string()).into_response()
     }
 }
 

src/http/profiles.rs

@@ -89,7 +89,7 @@ async fn follow_user(
     // because you were too clever. You can always improve performance later if the
     // implementation proves to be a bottleneck.
     //
-    // Readability is also tantamount if you need to onboard more devs to the project.
+    // Readability is also paramount if you need to onboard more devs to the project.
     //
     // Trust me, I've learned this the hard way.
 

src/http/types.rs

@@ -5,9 +5,19 @@ use time::{Format, OffsetDateTime};
 
 /// `OffsetDateTime` provides RFC-3339 (ISO-8601 subset) serialization, but the default
 /// `serde::Serialize` implementation produces array of integers, which is great for binary
-/// serialization but very difficult to consume when returned from an API.
+/// serialization, but infeasible to consume when returned from an API, and certainly
+/// not human-readable.
 ///
 /// With this wrapper type, we override this to provide the serialization format we want.
+///
+/// `chrono::DateTime` doesn't need this treatment, but Chrono sadly seems to have stagnated,
+/// and has a few more papercuts than I'd like:
+///
+/// * Having to import both `DateTime` and `Utc` everywhere gets annoying quickly.
+/// * lack of `const fn` constructors anywhere (especially for `chrono::Duration`)
+/// * `cookie::CookieBuilder` (used by Actix-web and `tower-cookies`) bakes-in `time::Duration`
+///   for setting the expiration
+///     * not really Chrono's fault but certainly doesn't help.
 #[derive(sqlx::Type)]
 pub struct Timestamptz(pub OffsetDateTime);
 
@@ -31,6 +41,14 @@ impl<'de> Deserialize<'de> for Timestamptz {
         //
         // We could deserialize a borrowed `&str` directly but certain deserialization modes
         // of `serde_json` don't support that, so we'd be forced to always deserialize `String`.
+        //
+        // `serde_with` has a helper for this but it can be a bit overkill to bring in
+        // just for one type: https://docs.rs/serde_with/latest/serde_with/#displayfromstr
+        //
+        // We'd still need to implement `Display` and `FromStr`, but those are much simpler
+        // to work with.
+        //
+        // However, I also wanted to demonstrate that it was possible to do this with Serde alone.
         impl Visitor<'_> for StrVisitor {
             type Value = Timestamptz;
 

src/http/users.rs

@@ -63,7 +63,7 @@ async fn create_user(
 ) -> Result<Json<UserBody<User>>> {
     let password_hash = hash_password(req.user.password).await?;
 
-    // We like using queries inline in our request handlers as it's easier to understand the
+    // I personally prefer using queries inline in request handlers as it's easier to understand the
     // query's semantics in the wider context of where it's invoked.
     //
     // Sometimes queries just get too darn big, though. In that case it may be a good idea