add enum containing all successful route returns (#614)

* add enum containing all successful route returns

This enum derives `Responder`, so it can be used as route result.
We also implement `From`, so it can be converted

* disable clippy warning about this enum

There's no use trying to chase this warning.
The next warning will be about `Redirect`, which itself is 360 byte,
according to @fdb-hiroshima (thanks for the research!)

So, if anything, we can try to get Rocket to work on that!

* refactor routes/posts to only use one level of Result

* admin settings error is not an ErrorPage, so dont use Result<>

* refactor: use early return to remove indent of main logic

This diff is absolutely atrocious.
but the resulting readability is worth it

* refactor routes/post for early returns & RespondOrRedirect

* refactor routes/session for early returns & use RespondOrRedirect

* refactor routes/user -- add another field to enum

* refactor routes/blogs for early returns & RespondOrRedirect

* refactor routes/blogs for early returns & RespondOrRedirect

This is a final refactor of `pub fn update()` for readability.
We're removing at least one indentation level.

src/routes/blogs.rs

@@ -16,7 +16,7 @@ use plume_models::{
     blog_authors::*, blogs::*, instance::Instance, medias::*, posts::Post, safe_string::SafeString,
     users::User, Connection, PlumeRocket,
 };
-use routes::{errors::ErrorPage, Page};
+use routes::{errors::ErrorPage, Page, RespondOrRedirect};
 use template_utils::{IntoContext, Ructe};
 
 #[get("/~/<name>?<page>", rank = 2)]
@@ -84,10 +84,7 @@ fn valid_slug(title: &str) -> Result<(), ValidationError> {
 }
 
 #[post("/blogs/new", data = "<form>")]
-pub fn create(
+pub fn create(form: LenientForm<NewBlogForm>, rockets: PlumeRocket) -> RespondOrRedirect {
-    form: LenientForm<NewBlogForm>,
-    rockets: PlumeRocket,
-) -> Result<Flash<Redirect>, Ructe> {
     let slug = utils::make_actor_id(&form.title);
     let conn = &*rockets.conn;
     let intl = &rockets.intl.catalog;
@@ -111,7 +108,10 @@ pub fn create(
         );
     }
 
-    if errors.is_empty() {
+    if !errors.is_empty() {
+        return render!(blogs::new(&rockets.to_context(), &*form, errors)).into();
+    }
+
     let blog = Blog::insert(
         &*conn,
         NewBlog::new_local(
@@ -136,17 +136,15 @@ pub fn create(
     )
     .expect("blog::create: author error");
 
-        Ok(Flash::success(
+    Flash::success(
         Redirect::to(uri!(details: name = slug.clone(), page = _)),
         &i18n!(intl, "Your blog was successfully created!"),
-        ))
+    )
-    } else {
+    .into()
-        Err(render!(blogs::new(&rockets.to_context(), &*form, errors)))
-    }
 }
 
 #[post("/~/<name>/delete")]
-pub fn delete(name: String, rockets: PlumeRocket) -> Result<Flash<Redirect>, Ructe> {
+pub fn delete(name: String, rockets: PlumeRocket) -> RespondOrRedirect {
     let conn = &*rockets.conn;
     let blog = Blog::find_by_fqn(&rockets, &name).expect("blog::delete: blog not found");
 
@@ -158,19 +156,21 @@ pub fn delete(name: String, rockets: PlumeRocket) -> Result<Flash<Redirect>, Ruc
     {
         blog.delete(&conn, &rockets.searcher)
             .expect("blog::expect: deletion error");
-        Ok(Flash::success(
+        Flash::success(
             Redirect::to(uri!(super::instance::index)),
             i18n!(rockets.intl.catalog, "Your blog was deleted."),
-        ))
+        )
+        .into()
     } else {
         // TODO actually return 403 error code
-        Err(render!(errors::not_authorized(
+        render!(errors::not_authorized(
             &rockets.to_context(),
             i18n!(
                 rockets.intl.catalog,
                 "You are not allowed to delete this blog."
             )
-        )))
+        ))
+        .into()
     }
 }
 
@@ -236,16 +236,27 @@ pub fn update(
     name: String,
     form: LenientForm<EditForm>,
     rockets: PlumeRocket,
-) -> Result<Flash<Redirect>, Ructe> {
+) -> RespondOrRedirect {
     let conn = &*rockets.conn;
     let intl = &rockets.intl.catalog;
     let mut blog = Blog::find_by_fqn(&rockets, &name).expect("blog::update: blog not found");
-    if rockets
+    if !rockets
         .user
         .clone()
         .and_then(|u| u.is_author_in(&*conn, &blog).ok())
         .unwrap_or(false)
     {
+        // TODO actually return 403 error code
+        return render!(errors::not_authorized(
+            &rockets.to_context(),
+            i18n!(
+                rockets.intl.catalog,
+                "You are not allowed to edit this blog."
+            )
+        ))
+        .into();
+    }
+
     let user = rockets
         .user
         .clone()
@@ -324,16 +335,8 @@ pub fn update(
                 err
             ))
         })
-    } else {
+        .unwrap()
-        // TODO actually return 403 error code
+        .into()
-        Err(render!(errors::not_authorized(
-            &rockets.to_context(),
-            i18n!(
-                rockets.intl.catalog,
-                "You are not allowed to edit this blog."
-            )
-        )))
-    }
 }
 
 #[get("/~/<name>/outbox")]

src/routes/instance.rs

@@ -13,7 +13,7 @@ use plume_models::{
     admin::Admin, comments::Comment, db_conn::DbConn, headers::Headers, instance::*, posts::Post,
     safe_string::SafeString, users::User, Error, PlumeRocket, CONFIG,
 };
-use routes::{errors::ErrorPage, rocket_uri_macro_static_files, Page};
+use routes::{errors::ErrorPage, rocket_uri_macro_static_files, Page, RespondOrRedirect};
 use template_utils::{IntoContext, Ructe};
 
 #[get("/")]
@@ -114,10 +114,19 @@ pub fn update_settings(
     _admin: Admin,
     form: LenientForm<InstanceSettingsForm>,
     rockets: PlumeRocket,
-) -> Result<Flash<Redirect>, Ructe> {
+) -> RespondOrRedirect {
     let conn = &*rockets.conn;
-    form.validate()
+    if let Err(e) = form.validate() {
-        .and_then(|_| {
+        let local_inst =
+            Instance::get_local().expect("instance::update_settings: local instance error");
+        render!(instance::admin(
+            &rockets.to_context(),
+            local_inst,
+            form.clone(),
+            e
+        ))
+        .into()
+    } else {
         let instance =
             Instance::get_local().expect("instance::update_settings: local instance error");
         instance
@@ -129,21 +138,12 @@ pub fn update_settings(
                 form.long_description.clone(),
             )
             .expect("instance::update_settings: save error");
-            Ok(Flash::success(
+        Flash::success(
             Redirect::to(uri!(admin)),
             i18n!(rockets.intl.catalog, "Instance settings have been saved."),
-            ))
+        )
-        })
+        .into()
-        .or_else(|e| {
+    }
-            let local_inst =
-                Instance::get_local().expect("instance::update_settings: local instance error");
-            Err(render!(instance::admin(
-                &rockets.to_context(),
-                local_inst,
-                form.clone(),
-                e
-            )))
-        })
 }
 
 #[get("/admin/instances?<page>")]

src/routes/medias.rs

@@ -38,14 +38,16 @@ pub fn upload(
     ct: &ContentType,
     conn: DbConn,
 ) -> Result<Redirect, status::BadRequest<&'static str>> {
-    if ct.is_form_data() {
+    if !ct.is_form_data() {
+        return Ok(Redirect::to(uri!(new)));
+    }
+
     let (_, boundary) = ct
         .params()
         .find(|&(k, _)| k == "boundary")
         .ok_or_else(|| status::BadRequest(Some("No boundary")))?;
 
-        match Multipart::with_body(data.open(), boundary).save().temp() {
+    if let SaveResult::Full(entries) = Multipart::with_body(data.open(), boundary).save().temp() {
-            SaveResult::Full(entries) => {
         let fields = entries.fields;
 
         let filename = fields
@@ -105,9 +107,6 @@ pub fn upload(
         )
         .map_err(|_| status::BadRequest(Some("Error while saving media")))?;
         Ok(Redirect::to(uri!(details: id = media.id)))
-            }
-            SaveResult::Partial(_, _) | SaveResult::Error(_) => Ok(Redirect::to(uri!(new))),
-        }
     } else {
         Ok(Redirect::to(uri!(new)))
     }

src/routes/mod.rs

@@ -7,15 +7,51 @@ use rocket::{
         RawStr, Status,
     },
     request::{self, FromFormValue, FromRequest, Request},
-    response::NamedFile,
+    response::{Flash, NamedFile, Redirect},
     Outcome,
 };
 use std::path::{Path, PathBuf};
+use template_utils::Ructe;
 
 use plume_models::{posts::Post, Connection};
 
 const ITEMS_PER_PAGE: i32 = 12;
 
+/// Special return type used for routes that "cannot fail", and instead
+/// `Redirect`, or `Flash<Redirect>`, when we cannot deliver a `Ructe` Response
+#[allow(clippy::large_enum_variant)]
+#[derive(Responder)]
+pub enum RespondOrRedirect {
+    Response(Ructe),
+    FlashResponse(Flash<Ructe>),
+    Redirect(Redirect),
+    FlashRedirect(Flash<Redirect>),
+}
+
+impl From<Ructe> for RespondOrRedirect {
+    fn from(response: Ructe) -> Self {
+        RespondOrRedirect::Response(response)
+    }
+}
+
+impl From<Flash<Ructe>> for RespondOrRedirect {
+    fn from(response: Flash<Ructe>) -> Self {
+        RespondOrRedirect::FlashResponse(response)
+    }
+}
+
+impl From<Redirect> for RespondOrRedirect {
+    fn from(redirect: Redirect) -> Self {
+        RespondOrRedirect::Redirect(redirect)
+    }
+}
+
+impl From<Flash<Redirect>> for RespondOrRedirect {
+    fn from(redirect: Flash<Redirect>) -> Self {
+        RespondOrRedirect::FlashRedirect(redirect)
+    }
+}
+
 #[derive(Shrinkwrap, Copy, Clone, UriDisplayQuery)]
 pub struct Page(i32);
 

src/routes/posts.rs

@@ -26,7 +26,9 @@ use plume_models::{
     users::User,
     Error, PlumeRocket,
 };
-use routes::{comments::NewCommentForm, errors::ErrorPage, ContentLen, RemoteForm};
+use routes::{
+    comments::NewCommentForm, errors::ErrorPage, ContentLen, RemoteForm, RespondOrRedirect,
+};
 use template_utils::{IntoContext, Ructe};
 
 #[get("/~/<blog>/<slug>?<responding_to>", rank = 4)]
@@ -40,12 +42,18 @@ pub fn details(
     let user = rockets.user.clone();
     let blog = Blog::find_by_fqn(&rockets, &blog)?;
     let post = Post::find_by_slug(&*conn, &slug, blog.id)?;
-    if post.published
+    if !(post.published
         || post
             .get_authors(&*conn)?
             .into_iter()
-            .any(|a| a.id == user.clone().map(|u| u.id).unwrap_or(0))
+            .any(|a| a.id == user.clone().map(|u| u.id).unwrap_or(0)))
     {
+        return Ok(render!(errors::not_authorized(
+            &rockets.to_context(),
+            i18n!(rockets.intl.catalog, "This post isn't published yet.")
+        )));
+    }
+
     let comments = CommentTree::from_post(&*conn, &post, user.as_ref())?;
 
     let previous = responding_to.and_then(|r| Comment::get(&*conn, r).ok());
@@ -87,12 +95,6 @@ pub fn details(
             user.and_then(|u| u.is_following(&*conn, post.get_authors(&*conn).ok()?[0].id).ok()).unwrap_or(false),
             post.get_authors(&*conn)?[0].clone()
         )))
-    } else {
-        Ok(render!(errors::not_authorized(
-            &rockets.to_context(),
-            i18n!(rockets.intl.catalog, "This post isn't published yet.")
-        )))
-    }
 }
 
 #[get("/~/<blog>/<slug>", rank = 3)]
@@ -219,7 +221,7 @@ pub fn update(
     cl: ContentLen,
     form: LenientForm<NewPostForm>,
     rockets: PlumeRocket,
-) -> Result<Flash<Redirect>, Ructe> {
+) -> RespondOrRedirect {
     let conn = &*rockets.conn;
     let b = Blog::find_by_fqn(&rockets, &blog).expect("post::update: blog error");
     let mut post =
@@ -255,10 +257,11 @@ pub fn update(
             .expect("posts::update: is author in error")
         {
             // actually it's not "Ok"…
-            Ok(Flash::error(
+            Flash::error(
                 Redirect::to(uri!(super::blogs::details: name = blog, page = _)),
                 i18n!(&intl, "You are not allowed to publish on this blog."),
-            ))
+            )
+            .into()
         } else {
             let (content, mentions, hashtags) = utils::md_to_html(
                 form.content.to_string().as_ref(),
@@ -345,14 +348,15 @@ pub fn update(
                 }
             }
 
-            Ok(Flash::success(
+            Flash::success(
                 Redirect::to(uri!(details: blog = blog, slug = new_slug, responding_to = _)),
                 i18n!(intl, "Your article has been updated."),
-            ))
+            )
+            .into()
         }
     } else {
         let medias = Media::for_user(&*conn, user.id).expect("posts:update: medias error");
-        Err(render!(posts::new(
+        render!(posts::new(
             &rockets.to_context(),
             i18n!(intl, "Edit {0}"; &form.title),
             b,
@@ -363,7 +367,8 @@ pub fn update(
             errors.clone(),
             medias.clone(),
             cl.0
-        )))
+        ))
+        .into()
     }
 }
 
@@ -396,7 +401,7 @@ pub fn create(
     form: LenientForm<NewPostForm>,
     cl: ContentLen,
     rockets: PlumeRocket,
-) -> Result<Flash<Redirect>, Result<Ructe, ErrorPage>> {
+) -> Result<RespondOrRedirect, ErrorPage> {
     let conn = &*rockets.conn;
     let blog = Blog::find_by_fqn(&rockets, &blog_name).expect("post::create: blog error");;
     let slug = form.title.to_string().to_kebab_case();
@@ -429,7 +434,8 @@ pub fn create(
                     &rockets.intl.catalog,
                     "You are not allowed to publish on this blog."
                 ),
-            ));
+            )
+            .into());
         }
 
         let (content, mentions, hashtags) = utils::md_to_html(
@@ -530,10 +536,11 @@ pub fn create(
         Ok(Flash::success(
             Redirect::to(uri!(details: blog = blog_name, slug = slug, responding_to = _)),
             i18n!(&rockets.intl.catalog, "Your article has been saved."),
-        ))
+        )
+        .into())
     } else {
         let medias = Media::for_user(&*conn, user.id).expect("posts::create: medias error");
-        Err(Ok(render!(posts::new(
+        Ok(render!(posts::new(
             &rockets.to_context(),
             i18n!(rockets.intl.catalog, "New article"),
             blog,
@@ -544,7 +551,8 @@ pub fn create(
             errors.clone(),
             medias,
             cl.0
-        ))))
+        ))
+        .into())
     }
 }
 
@@ -627,14 +635,14 @@ pub fn remote_interact_post(
     blog_name: String,
     slug: String,
     remote: LenientForm<RemoteForm>,
-) -> Result<Result<Ructe, Redirect>, ErrorPage> {
+) -> Result<RespondOrRedirect, ErrorPage> {
     let target = Blog::find_by_fqn(&rockets, &blog_name)
         .and_then(|blog| Post::find_by_slug(&rockets.conn, &slug, blog.id))?;
     if let Some(uri) = User::fetch_remote_interact_uri(&remote.remote)
         .ok()
         .and_then(|uri| rt_format!(uri, uri = target.ap_url).ok())
     {
-        Ok(Err(Redirect::to(uri)))
+        Ok(Redirect::to(uri).into())
     } else {
         let mut errs = ValidationErrors::new();
         errs.add("remote", ValidationError {
@@ -643,13 +651,14 @@ pub fn remote_interact_post(
             params: HashMap::new(),
         });
         //could not get your remote url?
-        Ok(Ok(render!(posts::remote_interact(
+        Ok(render!(posts::remote_interact(
             &rockets.to_context(),
             target,
             super::session::LoginForm::default(),
             ValidationErrors::default(),
             remote.clone(),
             errs
-        ))))
+        ))
+        .into())
     }
 }

src/routes/session.rs

@@ -7,6 +7,7 @@ use rocket::{
     State,
 };
 use rocket_i18n::I18n;
+use routes::RespondOrRedirect;
 use std::{
     borrow::Cow,
     sync::{Arc, Mutex},
@@ -45,7 +46,7 @@ pub fn create(
     form: LenientForm<LoginForm>,
     mut cookies: Cookies,
     rockets: PlumeRocket,
-) -> Result<Flash<Redirect>, Ructe> {
+) -> RespondOrRedirect {
     let conn = &*rockets.conn;
     let user = User::find_by_email(&*conn, &form.email_or_name)
         .or_else(|_| User::find_by_fqn(&rockets, &form.email_or_name));
@@ -76,7 +77,10 @@ pub fn create(
         String::new()
     };
 
-    if errors.is_empty() {
+    if !errors.is_empty() {
+        return render!(session::login(&rockets.to_context(), None, &*form, errors)).into();
+    }
+
     cookies.add_private(
         Cookie::build(AUTH_COOKIE, user_id)
             .same_site(SameSite::Lax)
@@ -96,28 +100,20 @@ pub fn create(
         )
         .unwrap_or_else(|| "/".to_owned());
 
-        let uri = Uri::parse(&destination)
+    if let Ok(uri) = Uri::parse(&destination).map(IntoOwned::into_owned) {
-            .map(IntoOwned::into_owned)
+        Flash::success(
-            .map_err(|_| {
+            Redirect::to(uri),
+            i18n!(&rockets.intl.catalog, "You are now connected."),
+        )
+        .into()
+    } else {
         render!(session::login(
             &(conn, &rockets.intl.catalog, None, None),
             None,
             &*form,
             errors
         ))
-            })?;
+        .into()
-
-        Ok(Flash::success(
-            Redirect::to(uri),
-            i18n!(&rockets.intl.catalog, "You are now connected."),
-        ))
-    } else {
-        Err(render!(session::login(
-            &rockets.to_context(),
-            None,
-            &*form,
-            errors
-        )))
     }
 }
 

src/routes/user.rs

@@ -25,14 +25,14 @@ use plume_models::{
     users::*,
     Error, PlumeRocket,
 };
-use routes::{errors::ErrorPage, Page, RemoteForm};
+use routes::{errors::ErrorPage, Page, RemoteForm, RespondOrRedirect};
 use template_utils::{IntoContext, Ructe};
 
 #[get("/me")]
-pub fn me(user: Option<User>) -> Result<Redirect, Flash<Redirect>> {
+pub fn me(user: Option<User>) -> RespondOrRedirect {
     match user {
-        Some(user) => Ok(Redirect::to(uri!(details: name = user.username))),
+        Some(user) => Redirect::to(uri!(details: name = user.username)).into(),
-        None => Err(utils::requires_login("", uri!(me))),
+        None => utils::requires_login("", uri!(me)).into(),
     }
 }
 
@@ -190,7 +190,7 @@ pub fn follow_not_connected(
     name: String,
     remote_form: Option<LenientForm<RemoteForm>>,
     i18n: I18n,
-) -> Result<Result<Flash<Ructe>, Redirect>, ErrorPage> {
+) -> Result<RespondOrRedirect, ErrorPage> {
     let target = User::find_by_fqn(&rockets, &name)?;
     if let Some(remote_form) = remote_form {
         if let Some(uri) = User::fetch_remote_interact_uri(&remote_form)
@@ -207,7 +207,7 @@ pub fn follow_not_connected(
                 .ok()
             })
         {
-            Ok(Err(Redirect::to(uri)))
+            Ok(Redirect::to(uri).into())
         } else {
             let mut err = ValidationErrors::default();
             err.add("remote",
@@ -217,7 +217,7 @@ pub fn follow_not_connected(
                     params: HashMap::new(),
                 },
             );
-            Ok(Ok(Flash::new(
+            Ok(Flash::new(
                 render!(users::follow_remote(
                     &rockets.to_context(),
                     target,
@@ -228,10 +228,11 @@ pub fn follow_not_connected(
                 )),
                 "callback",
                 uri!(follow: name = name).to_string(),
-            )))
+            )
+            .into())
         }
     } else {
-        Ok(Ok(Flash::new(
+        Ok(Flash::new(
             render!(users::follow_remote(
                 &rockets.to_context(),
                 target,
@@ -243,7 +244,8 @@ pub fn follow_not_connected(
             )),
             "callback",
             uri!(follow: name = name).to_string(),
-        )))
+        )
+        .into())
     }
 }