Avoid panics #392

elegaanz · 2018-12-26T19:11:52Z

Use Result as much as possible
Display errors instead of panicking

TODO (maybe in another PR? this one is already quite big):

Find a way to merge Ructe/ErrorPage types, so that we can have routes returning Result<X, ErrorPage> instead of panicking when we have an Error
Display more details about the error, to make it easier to debug

(sorry, this isn't going to be fun to review, the diff is huge, but it is always the same changes)

Use Result everywhere

trinity-1686a · 2018-12-26T21:01:12Z

(sorry, this isn't going to be fun to review, the diff is huge, but it is always the same changes)

I guess it wasn't fun to write either

igalic

first pass, not done yet

igalic · 2018-12-26T22:41:03Z

plume-common/src/activity_pub/mod.rs

@@ -120,7 +120,7 @@ pub fn broadcast<S: sign::Signer, A: Activity, T: inbox::WithInbox + Actor>(

    let mut act = serde_json::to_value(act).expect("activity_pub::broadcast: serialization error");
    act["@context"] = context();
-    let signed = act.sign(sender);
+    let signed = act.sign(sender).expect("activity_pub::broadcast: signature error");


i thought we're trying to avoid panics?

Ahah, right… It actually doesn't add a panic, we just panic "outside" of the function instead of "inside" because it wouldn't make much sense to return a Result from broadcast as it is always the main function of its thread.

igalic · 2018-12-26T22:42:59Z

plume-common/src/activity_pub/request.rs

@@ -105,7 +105,7 @@ pub fn headers() -> HeaderMap {
    headers
 }

-pub fn signature<S: Signer>(signer: &S, headers: &HeaderMap) -> HeaderValue {
+pub fn signature<S: Signer>(signer: &S, headers: &HeaderMap) -> Result<HeaderValue, ()> {


can you briefly explain this return signature?
maybe then I'll also understand the above expect()

It says that it can either success with a HeaderValue, or fail with (), so we don't have much details about the error, we just know it failed, but it is probably enough to know that something failed in this function (or maybe not?)

igalic · 2018-12-26T22:47:42Z

plume-models/src/blogs.rs

+                .and_then(|instance| Blog::find_by_name(conn, actor, instance.id))
+                .or_else(|_| Blog::fetch_from_webfinger(conn, fqn))
+        } else { // local blog
+            Blog::find_local(conn, actor)


wow! that's some really nice refactoring! 👍

plume-models/src/blogs.rs

igalic · 2018-12-26T23:10:04Z

plume-models/src/comments.rs

        note.object_props
            .set_tag_link_vec(
                mentions
                    .into_iter()
-                    .map(|m| Mention::build_activity(conn, &m))
+                    .filter_map(|m| Mention::build_activity(conn, &m).ok())


plume-models/src/comments.rs

codecov · 2018-12-27T12:10:07Z

Codecov Report

Merging #392 into master will increase coverage by 0.08%.
The diff coverage is 19.5%.

@@            Coverage Diff             @@
##           master     #392      +/-   ##
==========================================
+ Coverage   27.76%   27.85%   +0.08%     
==========================================
  Files          63       63              
  Lines        6302     7260     +958     
==========================================
+ Hits         1750     2022     +272     
- Misses       4552     5238     +686

trinity-1686a

It seems good.
Some Result could/should be Result<()> instead.
The ? syntax is really helpful with error handling

trinity-1686a · 2018-12-28T15:58:56Z

plume-models/src/blogs.rs

-    pub fn delete(&self, conn: &Connection, searcher: &Searcher) {
-        for post in Post::get_for_blog(conn, &self) {
-            post.delete(&(conn, searcher));
+    pub fn delete(&self, conn: &Connection, searcher: &Searcher) -> Result<usize> {


I could see a use for knowing how many posts were deleted, and at first at assumed it's what it was. But after re-reading it's telling how many blogs where deleted, so the Ok(_) will only be 1 any, am I right?

Yes, this result is not used anywhere, it just to have the right return type without having to .map(|_| ()), but maybe it's misleading?

I think it is, even more because we did not document any function (nor what they return)

Ok, I'll change it then. 👍

trinity-1686a · 2018-12-28T16:00:18Z

plume-models/src/instance.rs

    }

    insert!(instances, NewInstance);
    get!(instances);
    find_by!(instances, find_by_domain, public_domain as &str);

-    pub fn toggle_block(&self, conn: &Connection) {
+    pub fn toggle_block(&self, conn: &Connection) -> Result<usize> {


If I'm not mistaken, the usize is going to be how many entries were affected, which is one?

Yes, but #392 (comment)

trinity-1686a · 2018-12-28T16:00:35Z

plume-models/src/instance.rs

@@ -138,7 +130,7 @@ impl Instance {
        open_registrations: bool,
        short_description: SafeString,
        long_description: SafeString,
-    ) {
+    ) -> Result<usize> {


If I'm not mistaken, the usize is going to be how many entries were affected, which is one?

trinity-1686a · 2018-12-28T16:24:05Z

plume-models/src/posts.rs

                    ),
                    published: true,
                    license: license,
                    // FIXME: This is wrong: with this logic, we may use the display URL as the AP ID. We need two different fields
-                    ap_url: article.object_props.url_string().unwrap_or_else(|_|
+                    ap_url: article.object_props.url_string().unwrap_or(


I don't know if there are other places where unwrap_or_else(|_| ..) was replaced with unwrap_or(..), if so this comment also apply to them. I think it would be better to or_else(|_| ..)?, I think you already did it at other places

trinity-1686a · 2018-12-28T16:27:48Z

plume-models/src/tags.rs

    }

-    pub fn delete(&self, conn: &Connection) {
+    pub fn delete(&self, conn: &Connection) -> Result<usize> {


Same about returning Result<()> because it will be 1

trinity-1686a · 2018-12-28T16:28:31Z

plume-models/src/users.rs

    }

-    pub fn delete(&self, conn: &Connection, searcher: &Searcher) {
+    pub fn delete(&self, conn: &Connection, searcher: &Searcher) -> Result<usize> {


same about Result<()>

trinity-1686a · 2018-12-28T16:29:01Z

plume-models/src/users.rs

    }

-    pub fn grant_admin_rights(&self, conn: &Connection) {
+    pub fn grant_admin_rights(&self, conn: &Connection) -> Result<usize> {


Same about Result<()>

trinity-1686a · 2018-12-28T16:29:08Z

plume-models/src/users.rs

    }

-    pub fn revoke_admin_rights(&self, conn: &Connection) {
+    pub fn revoke_admin_rights(&self, conn: &Connection) -> Result<usize> {


Same about Result<()>

trinity-1686a · 2018-12-28T16:35:26Z

src/main.rs

+    let searcher = match UnmanagedSearcher::open(&"search_index") {
+        Err(Error::Search(e)) => match e {
+            SearcherError::WriteLockAcquisitionError => panic!(
+r#"Your search index is locked. Plume can't start. To fix this issue, run:


I should have made this message from the start. Maybe add something about making sure no other instance of plume is already running here?

Just Result<()>

trinity-1686a

👍

elegaanz added 2 commits December 26, 2018 11:40

Avoid panics in plume-models as much as possible

b0b1e20

Use Result everywhere

Display errors instead of crashing (as much as possible)

af69022

elegaanz added C: Enhancement New feature or request S: Ready for review This PR is ready to be reviewed A: Backend Code running on the server labels Dec 26, 2018

trinity-1686a self-requested a review December 26, 2018 21:02

The test should panic when something goes wrong

48cc6bf

igalic reviewed Dec 26, 2018

View reviewed changes

elegaanz added 2 commits December 27, 2018 11:47

Update plm too

ea9a85a

is_some becomes is_ok, not is_err

ca3f3a9

trinity-1686a previously approved these changes Dec 28, 2018

View reviewed changes

elegaanz added 2 commits December 28, 2018 18:45

Use or_else instead of unwrap_or

b28c5c4

Update search lock message

53aac78

elegaanz dismissed trinity-1686a’s stale review via 53aac78 December 28, 2018 17:47

Don't return Result<usize> when it doesn't make sense

ae493c0

Just Result<()>

trinity-1686a approved these changes Dec 28, 2018

View reviewed changes

elegaanz merged commit 80a4dae into master Dec 29, 2018

elegaanz deleted the result branch December 29, 2018 08:36

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Avoid panics #392

Avoid panics #392

elegaanz commented Dec 26, 2018 •

edited

Loading

trinity-1686a commented Dec 26, 2018

igalic left a comment

igalic Dec 26, 2018

elegaanz Dec 27, 2018

igalic Dec 26, 2018

elegaanz Dec 27, 2018

igalic Dec 26, 2018

igalic Dec 26, 2018

codecov bot commented Dec 27, 2018 •

edited

Loading

trinity-1686a left a comment

trinity-1686a Dec 28, 2018

elegaanz Dec 28, 2018

trinity-1686a Dec 28, 2018

elegaanz Dec 28, 2018

trinity-1686a Dec 28, 2018

elegaanz Dec 28, 2018

trinity-1686a Dec 28, 2018

trinity-1686a Dec 28, 2018

trinity-1686a Dec 28, 2018

trinity-1686a Dec 28, 2018

trinity-1686a Dec 28, 2018

trinity-1686a Dec 28, 2018 •

edited

Loading

trinity-1686a Dec 28, 2018

trinity-1686a left a comment

Avoid panics #392

Avoid panics #392

Conversation

elegaanz commented Dec 26, 2018 • edited Loading

trinity-1686a commented Dec 26, 2018

igalic left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Dec 27, 2018 • edited Loading

Codecov Report

trinity-1686a left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

trinity-1686a Dec 28, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

trinity-1686a left a comment

Choose a reason for hiding this comment

elegaanz commented Dec 26, 2018 •

edited

Loading

codecov bot commented Dec 27, 2018 •

edited

Loading

trinity-1686a Dec 28, 2018 •

edited

Loading