From 9538575c975277c4c8d3b515951e1fb1a467ae3a Mon Sep 17 00:00:00 2001 From: Liam Date: Tue, 30 Jul 2024 13:19:23 -0400 Subject: [PATCH 1/2] Streamline notification broadcasts --- lib/philomena/notifications.ex | 106 ++++++++++++----------- lib/philomena/notifications/creator.ex | 115 +++++++++---------------- 2 files changed, 97 insertions(+), 124 deletions(-) diff --git a/lib/philomena/notifications.ex b/lib/philomena/notifications.ex index baf2f8be..1adae26e 100644 --- a/lib/philomena/notifications.ex +++ b/lib/philomena/notifications.ex @@ -4,6 +4,7 @@ defmodule Philomena.Notifications do """ import Ecto.Query, warn: false + alias Philomena.Repo alias Philomena.Channels.Subscription, as: ChannelSubscription alias Philomena.Forums.Subscription, as: ForumSubscription @@ -78,12 +79,11 @@ defmodule Philomena.Notifications do """ def create_channel_live_notification(channel) do - Creator.create_single( - where(ChannelSubscription, channel_id: ^channel.id), - ChannelLiveNotification, - nil, - :channel_id, - channel + Creator.broadcast_notification( + from: {ChannelSubscription, channel_id: channel.id}, + into: ChannelLiveNotification, + select: [channel_id: channel.id], + unique_key: :channel_id ) end @@ -97,14 +97,12 @@ defmodule Philomena.Notifications do """ def create_forum_post_notification(user, topic, post) do - Creator.create_double( - where(TopicSubscription, topic_id: ^topic.id), - ForumPostNotification, - user, - :topic_id, - topic, - :post_id, - post + Creator.broadcast_notification( + notification_author: user, + from: {TopicSubscription, topic_id: topic.id}, + into: ForumPostNotification, + select: [topic_id: topic.id, post_id: post.id], + unique_key: :topic_id ) end @@ -118,12 +116,12 @@ defmodule Philomena.Notifications do """ def create_forum_topic_notification(user, topic) do - Creator.create_single( - where(ForumSubscription, forum_id: ^topic.forum_id), - ForumTopicNotification, - user, - :topic_id, - topic + Creator.broadcast_notification( + notification_author: user, + from: {ForumSubscription, forum_id: topic.forum_id}, + into: ForumTopicNotification, + select: [topic_id: topic.id], + unique_key: :topic_id ) end @@ -137,12 +135,11 @@ defmodule Philomena.Notifications do """ def create_gallery_image_notification(gallery) do - Creator.create_single( - where(GallerySubscription, gallery_id: ^gallery.id), - GalleryImageNotification, - nil, - :gallery_id, - gallery + Creator.broadcast_notification( + from: {GallerySubscription, gallery_id: gallery.id}, + into: GalleryImageNotification, + select: [gallery_id: gallery.id], + unique_key: :gallery_id ) end @@ -156,14 +153,12 @@ defmodule Philomena.Notifications do """ def create_image_comment_notification(user, image, comment) do - Creator.create_double( - where(ImageSubscription, image_id: ^image.id), - ImageCommentNotification, - user, - :image_id, - image, - :comment_id, - comment + Creator.broadcast_notification( + notification_author: user, + from: {ImageSubscription, image_id: image.id}, + into: ImageCommentNotification, + select: [image_id: image.id, comment_id: comment.id], + unique_key: :image_id ) end @@ -177,14 +172,11 @@ defmodule Philomena.Notifications do """ def create_image_merge_notification(target, source) do - Creator.create_double( - where(ImageSubscription, image_id: ^target.id), - ImageMergeNotification, - nil, - :target_id, - target, - :source_id, - source + Creator.broadcast_notification( + from: {ImageSubscription, image_id: target.id}, + into: ImageMergeNotification, + select: [target_id: target.id, source_id: source.id], + unique_key: :target_id ) end @@ -201,7 +193,7 @@ defmodule Philomena.Notifications do def clear_channel_live_notification(channel, user) do ChannelLiveNotification |> where(channel_id: ^channel.id) - |> Creator.clear(user) + |> delete_all_for_user(user) end @doc """ @@ -217,7 +209,7 @@ defmodule Philomena.Notifications do def clear_forum_post_notification(topic, user) do ForumPostNotification |> where(topic_id: ^topic.id) - |> Creator.clear(user) + |> delete_all_for_user(user) end @doc """ @@ -233,7 +225,7 @@ defmodule Philomena.Notifications do def clear_forum_topic_notification(topic, user) do ForumTopicNotification |> where(topic_id: ^topic.id) - |> Creator.clear(user) + |> delete_all_for_user(user) end @doc """ @@ -249,7 +241,7 @@ defmodule Philomena.Notifications do def clear_gallery_image_notification(gallery, user) do GalleryImageNotification |> where(gallery_id: ^gallery.id) - |> Creator.clear(user) + |> delete_all_for_user(user) end @doc """ @@ -265,7 +257,7 @@ defmodule Philomena.Notifications do def clear_image_comment_notification(image, user) do ImageCommentNotification |> where(image_id: ^image.id) - |> Creator.clear(user) + |> delete_all_for_user(user) end @doc """ @@ -281,6 +273,24 @@ defmodule Philomena.Notifications do def clear_image_merge_notification(image, user) do ImageMergeNotification |> where(target_id: ^image.id) - |> Creator.clear(user) + |> delete_all_for_user(user) + end + + # + # Clear all unread notifications using the given query. + # + # Returns `{:ok, count}`, where `count` is the number of affected rows. + # + defp delete_all_for_user(query, user) do + if user do + {count, nil} = + query + |> where(user_id: ^user.id) + |> Repo.delete_all() + + {:ok, count} + else + {:ok, 0} + end end end diff --git a/lib/philomena/notifications/creator.ex b/lib/philomena/notifications/creator.ex index ee4fec6a..1ceec14f 100644 --- a/lib/philomena/notifications/creator.ex +++ b/lib/philomena/notifications/creator.ex @@ -1,98 +1,61 @@ defmodule Philomena.Notifications.Creator do @moduledoc """ Internal notifications creation logic. - - Supports two formats for notification creation: - - Key-only (`create_single/4`): The object's id is the only other component inserted. - - Non-key (`create_double/6`): The object's id plus another object's id are inserted. - - See the respective documentation for each function for more details. """ import Ecto.Query, warn: false alias Philomena.Repo @doc """ - Propagate notifications for a notification table type containing a single reference column. - - The single reference column (`name`, `object`) is also part of the unique key for the table, - and is used to select which object to act on. + Propagate notifications for a notification table type. Returns `{:ok, count}`, where `count` is the number of affected rows. - ## Example + ## Examples - iex> create_single( - ...> where(GallerySubscription, gallery_id: ^gallery.id), - ...> GalleryImageNotification, - ...> nil, - ...> :gallery_id, - ...> gallery + iex> broadcast_notification( + ...> from: {GallerySubscription, gallery_id: gallery.id}, + ...> into: GalleryImageNotification, + ...> select: [gallery_id: gallery.id], + ...> unique_key: :gallery_id + ...> ) + {:ok, 2} + + iex> broadcast_notification( + ...> notification_author: user, + ...> from: {ImageSubscription, image_id: image.id}, + ...> into: ImageCommentNotification, + ...> select: [image_id: image.id, comment_id: comment.id], + ...> unique_key: :image_id ...> ) {:ok, 2} """ - def create_single(subscription, notification, user, name, object) do - subscription - |> create_notification_query(user, name, object) - |> create_notification(notification, name) - end + def broadcast_notification(opts) do + opts = Keyword.validate!(opts, [:notification_author, :from, :into, :select, :unique_key]) - @doc """ - Propagate notifications for a notification table type containing two reference columns. + notification_author = Keyword.get(opts, :notification_author, nil) + {subscription_schema, filters} = Keyword.fetch!(opts, :from) + notification_schema = Keyword.fetch!(opts, :into) + select_keywords = Keyword.fetch!(opts, :select) + unique_key = Keyword.fetch!(opts, :unique_key) - The first reference column (`name1`, `object1`) is also part of the unique key for the table, - and is used to select which object to act on. - - Returns `{:ok, count}`, where `count` is the number of affected rows. - - ## Example - - iex> create_double( - ...> where(ImageSubscription, image_id: ^image.id), - ...> ImageCommentNotification, - ...> user, - ...> :image_id, - ...> image, - ...> :comment_id, - ...> comment - ...> ) - {:ok, 2} - - """ - def create_double(subscription, notification, user, name1, object1, name2, object2) do - subscription - |> create_notification_query(user, name1, object1, name2, object2) - |> create_notification(notification, name1) - end - - @doc """ - Clear all unread notifications using the given query. - - Returns `{:ok, count}`, where `count` is the number of affected rows. - """ - def clear(query, user) do - if user do - {count, nil} = - query - |> where(user_id: ^user.id) - |> Repo.delete_all() - - {:ok, count} - else - {:ok, 0} - end + subscription_schema + |> subscription_query(notification_author) + |> where(^filters) + |> convert_to_notification(select_keywords) + |> insert_notifications(notification_schema, unique_key) end # TODO: the following cannot be accomplished with a single query expression # due to this Ecto bug: https://github.com/elixir-ecto/ecto/issues/4430 - defp create_notification_query(subscription, user, name, object) do + defp convert_to_notification(subscription, [{name, object_id}]) do now = DateTime.utc_now(:second) - from s in subscription_query(subscription, user), + from s in subscription, select: %{ - ^name => type(^object.id, :integer), + ^name => type(^object_id, :integer), user_id: s.user_id, created_at: ^now, updated_at: ^now, @@ -100,13 +63,13 @@ defmodule Philomena.Notifications.Creator do } end - defp create_notification_query(subscription, user, name1, object1, name2, object2) do + defp convert_to_notification(subscription, [{name1, object_id1}, {name2, object_id2}]) do now = DateTime.utc_now(:second) - from s in subscription_query(subscription, user), + from s in subscription, select: %{ - ^name1 => type(^object1.id, :integer), - ^name2 => type(^object2.id, :integer), + ^name1 => type(^object_id1, :integer), + ^name2 => type(^object_id2, :integer), user_id: s.user_id, created_at: ^now, updated_at: ^now, @@ -114,8 +77,8 @@ defmodule Philomena.Notifications.Creator do } end - defp subscription_query(subscription, user) do - case user do + defp subscription_query(subscription, notification_author) do + case notification_author do %{id: user_id} -> # Avoid sending notifications to the user which performed the action. from s in subscription, @@ -127,13 +90,13 @@ defmodule Philomena.Notifications.Creator do end end - defp create_notification(query, notification, name) do + defp insert_notifications(query, notification, unique_key) do {count, nil} = Repo.insert_all( notification, query, on_conflict: {:replace_all_except, [:created_at]}, - conflict_target: [name, :user_id] + conflict_target: [unique_key, :user_id] ) {:ok, count} From 183a99bc4f27caaf0f33b7358814c3741235a139 Mon Sep 17 00:00:00 2001 From: Liam Date: Tue, 30 Jul 2024 14:34:41 -0400 Subject: [PATCH 2/2] Remove background queueing for notification broadcasts --- lib/philomena/comments.ex | 16 +++--------- lib/philomena/galleries.ex | 25 +++++++------------ lib/philomena/images.ex | 12 ++------- lib/philomena/posts.ex | 16 +++--------- lib/philomena/topics.ex | 13 ++-------- lib/philomena/workers/notification_worker.ex | 13 ---------- .../controllers/image/comment_controller.ex | 1 - .../controllers/topic/post_controller.ex | 1 - .../controllers/topic_controller.ex | 1 - 9 files changed, 19 insertions(+), 79 deletions(-) delete mode 100644 lib/philomena/workers/notification_worker.ex diff --git a/lib/philomena/comments.ex b/lib/philomena/comments.ex index 4498dca8..e63a288a 100644 --- a/lib/philomena/comments.ex +++ b/lib/philomena/comments.ex @@ -15,7 +15,6 @@ defmodule Philomena.Comments do alias Philomena.Images.Image alias Philomena.Images alias Philomena.Notifications - alias Philomena.NotificationWorker alias Philomena.Versions alias Philomena.Reports @@ -63,21 +62,13 @@ defmodule Philomena.Comments do |> Multi.one(:image, image_lock_query) |> Multi.insert(:comment, comment) |> Multi.update_all(:update_image, image_query, inc: [comments_count: 1]) + |> Multi.run(:notification, ¬ify_comment/2) |> Images.maybe_subscribe_on(:image, attribution[:user], :watch_on_reply) |> Repo.transaction() end - def notify_comment(comment) do - Exq.enqueue(Exq, "notifications", NotificationWorker, ["Comments", comment.id]) - end - - def perform_notify(comment_id) do - comment = - comment_id - |> get_comment!() - |> Repo.preload([:user, :image]) - - Notifications.create_image_comment_notification(comment.user, comment.image, comment) + defp notify_comment(_repo, %{image: image, comment: comment}) do + Notifications.create_image_comment_notification(comment.user, image, comment) end @doc """ @@ -177,7 +168,6 @@ defmodule Philomena.Comments do |> Repo.transaction() |> case do {:ok, %{comment: comment, reports: {_count, reports}}} -> - notify_comment(comment) UserStatistics.inc_stat(comment.user, :comments_posted) Reports.reindex_reports(reports) reindex_comment(comment) diff --git a/lib/philomena/galleries.ex b/lib/philomena/galleries.ex index 65516064..d36198b7 100644 --- a/lib/philomena/galleries.ex +++ b/lib/philomena/galleries.ex @@ -14,7 +14,6 @@ defmodule Philomena.Galleries do alias Philomena.IndexWorker alias Philomena.GalleryReorderWorker alias Philomena.Notifications - alias Philomena.NotificationWorker alias Philomena.Images use Philomena.Subscriptions, @@ -163,7 +162,7 @@ defmodule Philomena.Galleries do def add_image_to_gallery(gallery, image) do Multi.new() - |> Multi.run(:lock, fn repo, %{} -> + |> Multi.run(:gallery, fn repo, %{} -> gallery = Gallery |> where(id: ^gallery.id) @@ -179,7 +178,7 @@ defmodule Philomena.Galleries do |> Interaction.changeset(%{"image_id" => image.id, "position" => position}) |> repo.insert() end) - |> Multi.run(:gallery, fn repo, %{} -> + |> Multi.run(:image_count, fn repo, %{} -> now = DateTime.utc_now() {count, nil} = @@ -189,11 +188,11 @@ defmodule Philomena.Galleries do {:ok, count} end) + |> Multi.run(:notification, ¬ify_gallery/2) |> Repo.transaction() |> case do {:ok, result} -> Images.reindex_image(image) - notify_gallery(gallery, image) reindex_gallery(gallery) {:ok, result} @@ -205,7 +204,7 @@ defmodule Philomena.Galleries do def remove_image_from_gallery(gallery, image) do Multi.new() - |> Multi.run(:lock, fn repo, %{} -> + |> Multi.run(:gallery, fn repo, %{} -> gallery = Gallery |> where(id: ^gallery.id) @@ -222,7 +221,7 @@ defmodule Philomena.Galleries do {:ok, count} end) - |> Multi.run(:gallery, fn repo, %{interaction: interaction_count} -> + |> Multi.run(:image_count, fn repo, %{interaction: interaction_count} -> now = DateTime.utc_now() {count, nil} = @@ -245,22 +244,16 @@ defmodule Philomena.Galleries do end end + defp notify_gallery(_repo, %{gallery: gallery}) do + Notifications.create_gallery_image_notification(gallery) + end + defp last_position(gallery_id) do Interaction |> where(gallery_id: ^gallery_id) |> Repo.aggregate(:max, :position) end - def notify_gallery(gallery, image) do - Exq.enqueue(Exq, "notifications", NotificationWorker, ["Galleries", [gallery.id, image.id]]) - end - - def perform_notify([gallery_id, _image_id]) do - gallery = get_gallery!(gallery_id) - - Notifications.create_gallery_image_notification(gallery) - end - def reorder_gallery(gallery, image_ids) do Exq.enqueue(Exq, "indexing", GalleryReorderWorker, [gallery.id, image_ids]) end diff --git a/lib/philomena/images.ex b/lib/philomena/images.ex index 326ede2d..71da602c 100644 --- a/lib/philomena/images.ex +++ b/lib/philomena/images.ex @@ -24,7 +24,6 @@ defmodule Philomena.Images do alias Philomena.SourceChanges.SourceChange alias Philomena.Notifications.ImageCommentNotification alias Philomena.Notifications.ImageMergeNotification - alias Philomena.NotificationWorker alias Philomena.TagChanges.Limits alias Philomena.TagChanges.TagChange alias Philomena.Tags @@ -602,13 +601,13 @@ defmodule Philomena.Images do |> Multi.run(:migrate_interactions, fn _, %{} -> {:ok, Interactions.migrate_interactions(image, duplicate_of_image)} end) + |> Multi.run(:notification, ¬ify_merge(&1, &2, image, duplicate_of_image)) |> Repo.transaction() |> process_after_hide() |> case do {:ok, result} -> reindex_image(duplicate_of_image) Comments.reindex_comments(duplicate_of_image) - notify_merge(image, duplicate_of_image) {:ok, result} @@ -954,14 +953,7 @@ defmodule Philomena.Images do |> Repo.update() end - def notify_merge(source, target) do - Exq.enqueue(Exq, "notifications", NotificationWorker, ["Images", [source.id, target.id]]) - end - - def perform_notify([source_id, target_id]) do - source = get_image!(source_id) - target = get_image!(target_id) - + defp notify_merge(_repo, _changes, source, target) do Notifications.create_image_merge_notification(target, source) end diff --git a/lib/philomena/posts.ex b/lib/philomena/posts.ex index af29c2e3..ff5a405f 100644 --- a/lib/philomena/posts.ex +++ b/lib/philomena/posts.ex @@ -16,7 +16,6 @@ defmodule Philomena.Posts do alias Philomena.IndexWorker alias Philomena.Forums.Forum alias Philomena.Notifications - alias Philomena.NotificationWorker alias Philomena.Versions alias Philomena.Reports @@ -93,6 +92,7 @@ defmodule Philomena.Posts do {:ok, count} end) + |> Multi.run(:notification, ¬ify_post/2) |> Topics.maybe_subscribe_on(:topic, attributes[:user], :watch_on_reply) |> Repo.transaction() |> case do @@ -106,8 +106,8 @@ defmodule Philomena.Posts do end end - def notify_post(post) do - Exq.enqueue(Exq, "notifications", NotificationWorker, ["Posts", post.id]) + defp notify_post(_repo, %{post: post, topic: topic}) do + Notifications.create_forum_post_notification(post.user, topic, post) end def report_non_approved(%Post{approved: true}), do: false @@ -120,15 +120,6 @@ defmodule Philomena.Posts do ) end - def perform_notify(post_id) do - post = - post_id - |> get_post!() - |> Repo.preload([:user, :topic]) - - Notifications.create_forum_post_notification(post.user, post.topic, post) - end - @doc """ Updates a post. @@ -241,7 +232,6 @@ defmodule Philomena.Posts do |> Repo.transaction() |> case do {:ok, %{post: post, reports: {_count, reports}}} -> - notify_post(post) UserStatistics.inc_stat(post.user, :forum_posts) Reports.reindex_reports(reports) reindex_post(post) diff --git a/lib/philomena/topics.ex b/lib/philomena/topics.ex index e1e5af29..38a5d602 100644 --- a/lib/philomena/topics.ex +++ b/lib/philomena/topics.ex @@ -11,7 +11,6 @@ defmodule Philomena.Topics do alias Philomena.Forums.Forum alias Philomena.Posts alias Philomena.Notifications - alias Philomena.NotificationWorker use Philomena.Subscriptions, on_delete: :clear_topic_notification, @@ -73,6 +72,7 @@ defmodule Philomena.Topics do {:ok, count} end) + |> Multi.run(:notification, ¬ify_topic/2) |> maybe_subscribe_on(:topic, attribution[:user], :watch_on_new_topic) |> Repo.transaction() |> case do @@ -87,16 +87,7 @@ defmodule Philomena.Topics do end end - def notify_topic(topic, post) do - Exq.enqueue(Exq, "notifications", NotificationWorker, ["Topics", [topic.id, post.id]]) - end - - def perform_notify([topic_id, _post_id]) do - topic = - topic_id - |> get_topic!() - |> Repo.preload(:user) - + defp notify_topic(_repo, %{topic: topic}) do Notifications.create_forum_topic_notification(topic.user, topic) end diff --git a/lib/philomena/workers/notification_worker.ex b/lib/philomena/workers/notification_worker.ex deleted file mode 100644 index 8bec8e61..00000000 --- a/lib/philomena/workers/notification_worker.ex +++ /dev/null @@ -1,13 +0,0 @@ -defmodule Philomena.NotificationWorker do - @modules %{ - "Comments" => Philomena.Comments, - "Galleries" => Philomena.Galleries, - "Images" => Philomena.Images, - "Posts" => Philomena.Posts, - "Topics" => Philomena.Topics - } - - def perform(module, args) do - @modules[module].perform_notify(args) - end -end diff --git a/lib/philomena_web/controllers/image/comment_controller.ex b/lib/philomena_web/controllers/image/comment_controller.ex index ca0ff9ad..86fb712d 100644 --- a/lib/philomena_web/controllers/image/comment_controller.ex +++ b/lib/philomena_web/controllers/image/comment_controller.ex @@ -82,7 +82,6 @@ defmodule PhilomenaWeb.Image.CommentController do Images.reindex_image(conn.assigns.image) if comment.approved do - Comments.notify_comment(comment) UserStatistics.inc_stat(conn.assigns.current_user, :comments_posted) else Comments.report_non_approved(comment) diff --git a/lib/philomena_web/controllers/topic/post_controller.ex b/lib/philomena_web/controllers/topic/post_controller.ex index 8f90abd0..a87c73c9 100644 --- a/lib/philomena_web/controllers/topic/post_controller.ex +++ b/lib/philomena_web/controllers/topic/post_controller.ex @@ -36,7 +36,6 @@ defmodule PhilomenaWeb.Topic.PostController do case Posts.create_post(topic, attributes, post_params) do {:ok, %{post: post}} -> if post.approved do - Posts.notify_post(post) UserStatistics.inc_stat(conn.assigns.current_user, :forum_posts) else Posts.report_non_approved(post) diff --git a/lib/philomena_web/controllers/topic_controller.ex b/lib/philomena_web/controllers/topic_controller.ex index f68fbcda..4d3099e8 100644 --- a/lib/philomena_web/controllers/topic_controller.ex +++ b/lib/philomena_web/controllers/topic_controller.ex @@ -111,7 +111,6 @@ defmodule PhilomenaWeb.TopicController do case Topics.create_topic(forum, attributes, topic_params) do {:ok, %{topic: topic}} -> post = hd(topic.posts) - Topics.notify_topic(topic, post) if forum.access_level == "normal" do PhilomenaWeb.Endpoint.broadcast!(