From: floatingghost <hannah@coffee-and-dreams.uk>
Date: Sat, 6 Aug 2022 20:59:15 +0000 (+0000)
Subject: make conversation-id deterministic (#154)
X-Git-Url: https://git.squeep.com/?a=commitdiff_plain;h=62e179f446ac7bfada965fac47e9613ce60091b4;p=akkoma

make conversation-id deterministic (#154)

Reviewed-on: https://akkoma.dev/AkkomaGang/akkoma/pulls/154
---

diff --git a/lib/pleroma/object.ex b/lib/pleroma/object.ex
index c3ea1b98b..00af77f57 100644
--- a/lib/pleroma/object.ex
+++ b/lib/pleroma/object.ex
@@ -208,10 +208,6 @@ defmodule Pleroma.Object do
     end
   end
 
-  def context_mapping(context) do
-    Object.change(%Object{}, %{data: %{"id" => context}})
-  end
-
   def make_tombstone(%Object{data: %{"id" => id, "type" => type}}, deleted \\ DateTime.utc_now()) do
     %ObjectTombstone{
       id: id,
diff --git a/lib/pleroma/web/activity_pub/object_validators/article_note_page_validator.ex b/lib/pleroma/web/activity_pub/object_validators/article_note_page_validator.ex
index 453bc6d86..eee7629ad 100644
--- a/lib/pleroma/web/activity_pub/object_validators/article_note_page_validator.ex
+++ b/lib/pleroma/web/activity_pub/object_validators/article_note_page_validator.ex
@@ -174,7 +174,7 @@ defmodule Pleroma.Web.ActivityPub.ObjectValidators.ArticleNotePageValidator do
   defp validate_data(data_cng) do
     data_cng
     |> validate_inclusion(:type, ["Article", "Note", "Page"])
-    |> validate_required([:id, :actor, :attributedTo, :type, :context, :context_id])
+    |> validate_required([:id, :actor, :attributedTo, :type, :context])
     |> CommonValidations.validate_any_presence([:cc, :to])
     |> CommonValidations.validate_fields_match([:actor, :attributedTo])
     |> CommonValidations.validate_actor_presence()
diff --git a/lib/pleroma/web/activity_pub/object_validators/common_fields.ex b/lib/pleroma/web/activity_pub/object_validators/common_fields.ex
index 1eaf572b9..49aba68af 100644
--- a/lib/pleroma/web/activity_pub/object_validators/common_fields.ex
+++ b/lib/pleroma/web/activity_pub/object_validators/common_fields.ex
@@ -51,8 +51,6 @@ defmodule Pleroma.Web.ActivityPub.ObjectValidators.CommonFields do
       field(:summary, :string)
 
       field(:context, :string)
-      # short identifier for PleromaFE to group statuses by context
-      field(:context_id, :integer)
 
       field(:sensitive, :boolean, default: false)
       field(:replies_count, :integer, default: 0)
diff --git a/lib/pleroma/web/activity_pub/object_validators/common_fixes.ex b/lib/pleroma/web/activity_pub/object_validators/common_fixes.ex
index 9631013a7..779c8b622 100644
--- a/lib/pleroma/web/activity_pub/object_validators/common_fixes.ex
+++ b/lib/pleroma/web/activity_pub/object_validators/common_fixes.ex
@@ -22,14 +22,12 @@ defmodule Pleroma.Web.ActivityPub.ObjectValidators.CommonFixes do
   end
 
   def fix_object_defaults(data) do
-    %{data: %{"id" => context}, id: context_id} =
-      Utils.create_context(data["context"] || data["conversation"])
+    context = Utils.maybe_create_context(data["context"] || data["conversation"])
 
     %User{follower_address: follower_collection} = User.get_cached_by_ap_id(data["attributedTo"])
 
     data
     |> Map.put("context", context)
-    |> Map.put("context_id", context_id)
     |> cast_and_filter_recipients("to", follower_collection)
     |> cast_and_filter_recipients("cc", follower_collection)
     |> cast_and_filter_recipients("bto", follower_collection)
diff --git a/lib/pleroma/web/activity_pub/object_validators/event_validator.ex b/lib/pleroma/web/activity_pub/object_validators/event_validator.ex
index 34a3031c3..76bba2778 100644
--- a/lib/pleroma/web/activity_pub/object_validators/event_validator.ex
+++ b/lib/pleroma/web/activity_pub/object_validators/event_validator.ex
@@ -62,7 +62,7 @@ defmodule Pleroma.Web.ActivityPub.ObjectValidators.EventValidator do
   defp validate_data(data_cng) do
     data_cng
     |> validate_inclusion(:type, ["Event"])
-    |> validate_required([:id, :actor, :attributedTo, :type, :context, :context_id])
+    |> validate_required([:id, :actor, :attributedTo, :type, :context])
     |> CommonValidations.validate_any_presence([:cc, :to])
     |> CommonValidations.validate_fields_match([:actor, :attributedTo])
     |> CommonValidations.validate_actor_presence()
diff --git a/lib/pleroma/web/activity_pub/object_validators/question_validator.ex b/lib/pleroma/web/activity_pub/object_validators/question_validator.ex
index bdddfdaeb..9c45374a4 100644
--- a/lib/pleroma/web/activity_pub/object_validators/question_validator.ex
+++ b/lib/pleroma/web/activity_pub/object_validators/question_validator.ex
@@ -80,7 +80,7 @@ defmodule Pleroma.Web.ActivityPub.ObjectValidators.QuestionValidator do
   defp validate_data(data_cng) do
     data_cng
     |> validate_inclusion(:type, ["Question"])
-    |> validate_required([:id, :actor, :attributedTo, :type, :context, :context_id])
+    |> validate_required([:id, :actor, :attributedTo, :type, :context])
     |> CommonValidations.validate_any_presence([:cc, :to])
     |> CommonValidations.validate_fields_match([:actor, :attributedTo])
     |> CommonValidations.validate_actor_presence()
diff --git a/lib/pleroma/web/activity_pub/utils.ex b/lib/pleroma/web/activity_pub/utils.ex
index 0e92deb31..5e5df4888 100644
--- a/lib/pleroma/web/activity_pub/utils.ex
+++ b/lib/pleroma/web/activity_pub/utils.ex
@@ -154,22 +154,7 @@ defmodule Pleroma.Web.ActivityPub.Utils do
     Notification.get_notified_from_activity(%Activity{data: object}, false)
   end
 
-  def create_context(context) do
-    context = context || generate_id("contexts")
-
-    # Ecto has problems accessing the constraint inside the jsonb,
-    # so we explicitly check for the existed object before insert
-    object = Object.get_cached_by_ap_id(context)
-
-    with true <- is_nil(object),
-         changeset <- Object.context_mapping(context),
-         {:ok, inserted_object} <- Repo.insert(changeset) do
-      inserted_object
-    else
-      _ ->
-        object
-    end
-  end
+  def maybe_create_context(context), do: context || generate_id("contexts")
 
   @doc """
   Enqueues an activity for federation if it's local
@@ -201,18 +186,16 @@ defmodule Pleroma.Web.ActivityPub.Utils do
     |> Map.put_new("id", "pleroma:fakeid")
     |> Map.put_new_lazy("published", &make_date/0)
     |> Map.put_new("context", "pleroma:fakecontext")
-    |> Map.put_new("context_id", -1)
     |> lazy_put_object_defaults(true)
   end
 
   def lazy_put_activity_defaults(map, _fake?) do
-    %{data: %{"id" => context}, id: context_id} = create_context(map["context"])
+    context = maybe_create_context(map["context"])
 
     map
     |> Map.put_new_lazy("id", &generate_activity_id/0)
     |> Map.put_new_lazy("published", &make_date/0)
     |> Map.put_new("context", context)
-    |> Map.put_new("context_id", context_id)
     |> lazy_put_object_defaults(false)
   end
 
@@ -226,7 +209,6 @@ defmodule Pleroma.Web.ActivityPub.Utils do
       |> Map.put_new("id", "pleroma:fake_object_id")
       |> Map.put_new_lazy("published", &make_date/0)
       |> Map.put_new("context", activity["context"])
-      |> Map.put_new("context_id", activity["context_id"])
       |> Map.put_new("fake", true)
 
     %{activity | "object" => object}
@@ -239,7 +221,6 @@ defmodule Pleroma.Web.ActivityPub.Utils do
       |> Map.put_new_lazy("id", &generate_object_id/0)
       |> Map.put_new_lazy("published", &make_date/0)
       |> Map.put_new("context", activity["context"])
-      |> Map.put_new("context_id", activity["context_id"])
 
     %{activity | "object" => object}
   end
diff --git a/lib/pleroma/web/common_api/utils.ex b/lib/pleroma/web/common_api/utils.ex
index 826160a23..61af71acd 100644
--- a/lib/pleroma/web/common_api/utils.ex
+++ b/lib/pleroma/web/common_api/utils.ex
@@ -458,35 +458,6 @@ defmodule Pleroma.Web.CommonAPI.Utils do
 
   def get_report_statuses(_, _), do: {:ok, nil}
 
-  # DEPRECATED mostly, context objects are now created at insertion time.
-  def context_to_conversation_id(context) do
-    with %Object{id: id} <- Object.get_cached_by_ap_id(context) do
-      id
-    else
-      _e ->
-        changeset = Object.context_mapping(context)
-
-        case Repo.insert(changeset) do
-          {:ok, %{id: id}} ->
-            id
-
-          # This should be solved by an upsert, but it seems ecto
-          # has problems accessing the constraint inside the jsonb.
-          {:error, _} ->
-            Object.get_cached_by_ap_id(context).id
-        end
-    end
-  end
-
-  def conversation_id_to_context(id) do
-    with %Object{data: %{"id" => context}} <- Repo.get(Object, id) do
-      context
-    else
-      _e ->
-        {:error, dgettext("errors", "No such conversation")}
-    end
-  end
-
   def validate_character_limit("" = _full_payload, [] = _attachments) do
     {:error, dgettext("errors", "Cannot post an empty status without attachments")}
   end
diff --git a/lib/pleroma/web/mastodon_api/views/status_view.ex b/lib/pleroma/web/mastodon_api/views/status_view.ex
index cf4ea51e0..f0ecf685b 100644
--- a/lib/pleroma/web/mastodon_api/views/status_view.ex
+++ b/lib/pleroma/web/mastodon_api/views/status_view.ex
@@ -57,11 +57,8 @@ defmodule Pleroma.Web.MastodonAPI.StatusView do
     end)
   end
 
-  defp get_context_id(%{data: %{"context_id" => context_id}}) when not is_nil(context_id),
-    do: context_id
-
   defp get_context_id(%{data: %{"context" => context}}) when is_binary(context),
-    do: Utils.context_to_conversation_id(context)
+    do: :erlang.crc32(context)
 
   defp get_context_id(_), do: nil
 
diff --git a/priv/repo/optional_migrations/destructive/remove_null_object_type_records/20220806014830_remove_null_objects.exs b/priv/repo/optional_migrations/destructive/remove_null_object_type_records/20220806014830_remove_null_objects.exs
new file mode 100644
index 000000000..308c3dbf0
--- /dev/null
+++ b/priv/repo/optional_migrations/destructive/remove_null_object_type_records/20220806014830_remove_null_objects.exs
@@ -0,0 +1,14 @@
+defmodule Pleroma.Repo.Migrations.RemoveNullObjects do
+  use Ecto.Migration
+
+  def up do
+    statement = """
+    DELETE FROM objects
+    WHERE (data->>'type') is null;
+    """
+
+    execute(statement)
+  end
+
+  def down, do: :ok
+end
diff --git a/test/pleroma/web/activity_pub/activity_pub_test.exs b/test/pleroma/web/activity_pub/activity_pub_test.exs
index 90680e8cc..50eff9431 100644
--- a/test/pleroma/web/activity_pub/activity_pub_test.exs
+++ b/test/pleroma/web/activity_pub/activity_pub_test.exs
@@ -523,7 +523,6 @@ defmodule Pleroma.Web.ActivityPub.ActivityPubTest do
       assert activity.data["ok"] == data["ok"]
       assert activity.data["id"] == given_id
       assert activity.data["context"] == "blabla"
-      assert activity.data["context_id"]
     end
 
     test "adds a context when none is there" do
@@ -545,8 +544,6 @@ defmodule Pleroma.Web.ActivityPub.ActivityPubTest do
 
       assert is_binary(activity.data["context"])
       assert is_binary(object.data["context"])
-      assert activity.data["context_id"]
-      assert object.data["context_id"]
     end
 
     test "adds an id to a given object if it lacks one and is a note and inserts it to the object database" do
@@ -1560,7 +1557,7 @@ defmodule Pleroma.Web.ActivityPub.ActivityPubTest do
                })
 
       assert Repo.aggregate(Activity, :count, :id) == 1
-      assert Repo.aggregate(Object, :count, :id) == 2
+      assert Repo.aggregate(Object, :count, :id) == 1
       assert Repo.aggregate(Notification, :count, :id) == 0
     end
   end
diff --git a/test/pleroma/web/activity_pub/transmogrifier/question_handling_test.exs b/test/pleroma/web/activity_pub/transmogrifier/question_handling_test.exs
index 32cf51e59..54cbb9c65 100644
--- a/test/pleroma/web/activity_pub/transmogrifier/question_handling_test.exs
+++ b/test/pleroma/web/activity_pub/transmogrifier/question_handling_test.exs
@@ -33,8 +33,6 @@ defmodule Pleroma.Web.ActivityPub.Transmogrifier.QuestionHandlingTest do
     assert object.data["context"] ==
              "tag:mastodon.sdf.org,2019-05-10:objectId=15095122:objectType=Conversation"
 
-    assert object.data["context_id"]
-
     assert object.data["anyOf"] == []
 
     assert Enum.sort(object.data["oneOf"]) ==
@@ -68,7 +66,6 @@ defmodule Pleroma.Web.ActivityPub.Transmogrifier.QuestionHandlingTest do
     reply_object = Object.normalize(reply_activity, fetch: false)
 
     assert reply_object.data["context"] == object.data["context"]
-    assert reply_object.data["context_id"] == object.data["context_id"]
   end
 
   test "Mastodon Question activity with HTML tags in plaintext" do
diff --git a/test/pleroma/web/activity_pub/transmogrifier_test.exs b/test/pleroma/web/activity_pub/transmogrifier_test.exs
index 6941f69aa..ae2fc067a 100644
--- a/test/pleroma/web/activity_pub/transmogrifier_test.exs
+++ b/test/pleroma/web/activity_pub/transmogrifier_test.exs
@@ -232,7 +232,6 @@ defmodule Pleroma.Web.ActivityPub.TransmogrifierTest do
       assert is_nil(modified["object"]["like_count"])
       assert is_nil(modified["object"]["announcements"])
       assert is_nil(modified["object"]["announcement_count"])
-      assert is_nil(modified["object"]["context_id"])
       assert is_nil(modified["object"]["generator"])
     end
 
@@ -247,7 +246,6 @@ defmodule Pleroma.Web.ActivityPub.TransmogrifierTest do
       assert is_nil(modified["object"]["like_count"])
       assert is_nil(modified["object"]["announcements"])
       assert is_nil(modified["object"]["announcement_count"])
-      assert is_nil(modified["object"]["context_id"])
       assert is_nil(modified["object"]["likes"])
     end
 
diff --git a/test/pleroma/web/activity_pub/utils_test.exs b/test/pleroma/web/activity_pub/utils_test.exs
index 62dc02f61..0d88303e3 100644
--- a/test/pleroma/web/activity_pub/utils_test.exs
+++ b/test/pleroma/web/activity_pub/utils_test.exs
@@ -429,7 +429,6 @@ defmodule Pleroma.Web.ActivityPub.UtilsTest do
       object = Object.normalize(note_activity, fetch: false)
       res = Utils.lazy_put_activity_defaults(%{"context" => object.data["id"]})
       assert res["context"] == object.data["id"]
-      assert res["context_id"] == object.id
       assert res["id"]
       assert res["published"]
     end
@@ -437,7 +436,6 @@ defmodule Pleroma.Web.ActivityPub.UtilsTest do
     test "returns map with fake id and published data" do
       assert %{
                "context" => "pleroma:fakecontext",
-               "context_id" => -1,
                "id" => "pleroma:fakeid",
                "published" => _
              } = Utils.lazy_put_activity_defaults(%{}, true)
@@ -454,13 +452,11 @@ defmodule Pleroma.Web.ActivityPub.UtilsTest do
         })
 
       assert res["context"] == object.data["id"]
-      assert res["context_id"] == object.id
       assert res["id"]
       assert res["published"]
       assert res["object"]["id"]
       assert res["object"]["published"]
       assert res["object"]["context"] == object.data["id"]
-      assert res["object"]["context_id"] == object.id
     end
   end
 
diff --git a/test/pleroma/web/common_api/utils_test.exs b/test/pleroma/web/common_api/utils_test.exs
index c4f506fe3..a88d7681a 100644
--- a/test/pleroma/web/common_api/utils_test.exs
+++ b/test/pleroma/web/common_api/utils_test.exs
@@ -4,7 +4,6 @@
 
 defmodule Pleroma.Web.CommonAPI.UtilsTest do
   alias Pleroma.Builders.UserBuilder
-  alias Pleroma.Object
   alias Pleroma.Web.CommonAPI
   alias Pleroma.Web.CommonAPI.ActivityDraft
   alias Pleroma.Web.CommonAPI.Utils
@@ -273,22 +272,6 @@ defmodule Pleroma.Web.CommonAPI.UtilsTest do
     end
   end
 
-  describe "context_to_conversation_id" do
-    test "creates a mapping object" do
-      conversation_id = Utils.context_to_conversation_id("random context")
-      object = Object.get_by_ap_id("random context")
-
-      assert conversation_id == object.id
-    end
-
-    test "returns an existing mapping for an existing object" do
-      {:ok, object} = Object.context_mapping("random context") |> Repo.insert()
-      conversation_id = Utils.context_to_conversation_id("random context")
-
-      assert conversation_id == object.id
-    end
-  end
-
   describe "formats date to asctime" do
     test "when date is in ISO 8601 format" do
       date = DateTime.utc_now() |> DateTime.to_iso8601()
@@ -517,17 +500,6 @@ defmodule Pleroma.Web.CommonAPI.UtilsTest do
     end
   end
 
-  describe "conversation_id_to_context/1" do
-    test "returns id" do
-      object = insert(:note)
-      assert Utils.conversation_id_to_context(object.id) == object.data["id"]
-    end
-
-    test "returns error if object not found" do
-      assert Utils.conversation_id_to_context("123") == {:error, "No such conversation"}
-    end
-  end
-
   describe "maybe_notify_mentioned_recipients/2" do
     test "returns recipients when activity is not `Create`" do
       activity = insert(:like_activity)
diff --git a/test/pleroma/web/mastodon_api/views/status_view_test.exs b/test/pleroma/web/mastodon_api/views/status_view_test.exs
index 9ef44caca..130de1b17 100644
--- a/test/pleroma/web/mastodon_api/views/status_view_test.exs
+++ b/test/pleroma/web/mastodon_api/views/status_view_test.exs
@@ -14,7 +14,6 @@ defmodule Pleroma.Web.MastodonAPI.StatusViewTest do
   alias Pleroma.User
   alias Pleroma.UserRelationship
   alias Pleroma.Web.CommonAPI
-  alias Pleroma.Web.CommonAPI.Utils
   alias Pleroma.Web.MastodonAPI.AccountView
   alias Pleroma.Web.MastodonAPI.StatusView
 
@@ -240,7 +239,7 @@ defmodule Pleroma.Web.MastodonAPI.StatusViewTest do
     object_data = Object.normalize(note, fetch: false).data
     user = User.get_cached_by_ap_id(note.data["actor"])
 
-    convo_id = Utils.context_to_conversation_id(object_data["context"])
+    convo_id = :erlang.crc32(object_data["context"])
 
     status = StatusView.render("show.json", %{activity: note})