Better error handling for OstatusController.

lib/pleroma/web/ostatus/ostatus_controller.ex

@@ -9,36 +9,42 @@ defmodule Pleroma.Web.OStatus.OStatusController do
   alias Pleroma.Web.ActivityPub.ActivityPubController
   alias Pleroma.Web.ActivityPub.ActivityPub
 
-  def feed_redirect(conn, %{"nickname" => nickname}) do
-    user = User.get_cached_by_nickname(nickname)
+  action_fallback(:errors)
 
-    case get_format(conn) do
-      "html" -> Fallback.RedirectController.redirector(conn, nil)
-      "activity+json" -> ActivityPubController.call(conn, :user)
-      _ -> redirect(conn, external: OStatus.feed_path(user))
+  def feed_redirect(conn, %{"nickname" => nickname}) do
+    with {_, %User{} = user} <- {:user, User.get_cached_by_nickname(nickname)} do
+      case get_format(conn) do
+        "html" -> Fallback.RedirectController.redirector(conn, nil)
+        "activity+json" -> ActivityPubController.call(conn, :user)
+        _ -> redirect(conn, external: OStatus.feed_path(user))
+      end
+    else
+      {:user, nil} -> {:error, :not_found}
     end
   end
 
   def feed(conn, %{"nickname" => nickname} = params) do
-    user = User.get_cached_by_nickname(nickname)
+    with {_, %User{} = user} <- {:user, User.get_cached_by_nickname(nickname)} do
+      query_params =
+        Map.take(params, ["max_id"])
+        |> Map.merge(%{"whole_db" => true, "actor_id" => user.ap_id})
 
-    query_params =
-      Map.take(params, ["max_id"])
-      |> Map.merge(%{"whole_db" => true, "actor_id" => user.ap_id})
+      activities =
+        ActivityPub.fetch_public_activities(query_params)
+        |> Enum.reverse()
 
-    activities =
-      ActivityPub.fetch_public_activities(query_params)
-      |> Enum.reverse()
+      response =
+        user
+        |> FeedRepresenter.to_simple_form(activities, [user])
+        |> :xmerl.export_simple(:xmerl_xml)
+        |> to_string
 
-    response =
-      user
-      |> FeedRepresenter.to_simple_form(activities, [user])
-      |> :xmerl.export_simple(:xmerl_xml)
-      |> to_string
-
-    conn
-    |> put_resp_content_type("application/atom+xml")
-    |> send_resp(200, response)
+      conn
+      |> put_resp_content_type("application/atom+xml")
+      |> send_resp(200, response)
+    else
+      {:user, nil} -> {:error, :not_found}
+    end
   end
 
   defp decode_or_retry(body) do
@@ -73,7 +79,8 @@ def object(conn, %{"uuid" => uuid}) do
       ActivityPubController.call(conn, :object)
     else
       with id <- o_status_url(conn, :object, uuid),
-           %Activity{} = activity <- Activity.get_create_activity_by_object_ap_id(id),
+           {_, %Activity{} = activity} <-
+             {:activity, Activity.get_create_activity_by_object_ap_id(id)},
            {_, true} <- {:public?, ActivityPub.is_public?(activity)},
            %User{} = user <- User.get_cached_by_ap_id(activity.data["actor"]) do
         case get_format(conn) do
@@ -82,16 +89,20 @@ def object(conn, %{"uuid" => uuid}) do
         end
       else
         {:public?, false} ->
-          conn
-          |> put_status(404)
-          |> json("Not found")
+          {:error, :not_found}
+
+        {:activity, nil} ->
+          {:error, :not_found}
+
+        e ->
+          e
       end
     end
   end
 
   def activity(conn, %{"uuid" => uuid}) do
     with id <- o_status_url(conn, :activity, uuid),
-         %Activity{} = activity <- Activity.get_by_ap_id(id),
+         {_, %Activity{} = activity} <- {:activity, Activity.get_by_ap_id(id)},
          {_, true} <- {:public?, ActivityPub.is_public?(activity)},
          %User{} = user <- User.get_cached_by_ap_id(activity.data["actor"]) do
       case get_format(conn) do
@@ -100,14 +111,18 @@ def activity(conn, %{"uuid" => uuid}) do
       end
     else
       {:public?, false} ->
-        conn
-        |> put_status(404)
-        |> json("Not found")
+        {:error, :not_found}
+
+      {:activity, nil} ->
+        {:error, :not_found}
+
+      e ->
+        e
     end
   end
 
   def notice(conn, %{"id" => id}) do
-    with %Activity{} = activity <- Repo.get(Activity, id),
+    with {_, %Activity{} = activity} <- {:activity, Repo.get(Activity, id)},
          {_, true} <- {:public?, ActivityPub.is_public?(activity)},
          %User{} = user <- User.get_cached_by_ap_id(activity.data["actor"]) do
       case get_format(conn) do
@@ -121,9 +136,13 @@ def notice(conn, %{"id" => id}) do
       end
     else
       {:public?, false} ->
-        conn
-        |> put_status(404)
-        |> json("Not found")
+        {:error, :not_found}
+
+      {:activity, nil} ->
+        {:error, :not_found}
+
+      e ->
+        e
     end
   end
 
@@ -139,4 +158,16 @@ defp represent_activity(conn, activity, user) do
     |> put_resp_content_type("application/atom+xml")
     |> send_resp(200, response)
   end
+
+  def errors(conn, {:error, :not_found}) do
+    conn
+    |> put_status(404)
+    |> text("Not found")
+  end
+
+  def errors(conn, _) do
+    conn
+    |> put_status(500)
+    |> text("Something went wrong")
+  end
 end

test/web/ostatus/ostatus_controller_test.exs

@@ -53,11 +53,21 @@ test "gets a feed", %{conn: conn} do
 
     conn =
       conn
+      |> put_req_header("content-type", "application/atom+xml")
       |> get("/users/#{user.nickname}/feed.atom")
 
     assert response(conn, 200) =~ note_activity.data["object"]["content"]
   end
 
+  test "returns 404 for a missing feed", %{conn: conn} do
+    conn =
+      conn
+      |> put_req_header("content-type", "application/atom+xml")
+      |> get("/users/nonexisting/feed.atom")
+
+    assert response(conn, 404)
+  end
+
   test "gets an object", %{conn: conn} do
     note_activity = insert(:note_activity)
     user = User.get_by_ap_id(note_activity.data["actor"])
@@ -90,6 +100,16 @@ test "404s on private objects", %{conn: conn} do
     assert response(conn, 404)
   end
 
+  test "404s on nonexisting objects", %{conn: conn} do
+    url = "/objects/123"
+
+    conn =
+      conn
+      |> get(url)
+
+    assert response(conn, 404)
+  end
+
   test "gets an activity", %{conn: conn} do
     note_activity = insert(:note_activity)
     [_, uuid] = hd(Regex.scan(~r/.+\/([\w-]+)$/, note_activity.data["id"]))
@@ -114,6 +134,16 @@ test "404s on private activities", %{conn: conn} do
     assert response(conn, 404)
   end
 
+  test "404s on nonexistent activities", %{conn: conn} do
+    url = "/activities/123"
+
+    conn =
+      conn
+      |> get(url)
+
+    assert response(conn, 404)
+  end
+
   test "gets a notice", %{conn: conn} do
     note_activity = insert(:note_activity)
     url = "/notice/#{note_activity.id}"
@@ -135,4 +165,14 @@ test "404s a private notice", %{conn: conn} do
 
     assert response(conn, 404)
   end
+
+  test "404s a nonexisting notice", %{conn: conn} do
+    url = "/notice/123"
+
+    conn =
+      conn
+      |> get(url)
+
+    assert response(conn, 404)
+  end
 end