From da1e6a145d8fb088a264da105973a7d3c467d2b4 Mon Sep 17 00:00:00 2001 From: "byte[]" Date: Sun, 6 Sep 2020 12:37:31 -0400 Subject: [PATCH] properly transactionalize image hiding and merging --- lib/philomena/duplicate_reports.ex | 80 ++++----- lib/philomena/images.ex | 153 ++++++++++++------ .../duplicate_report/accept_controller.ex | 10 +- .../accept_reverse_controller.ex | 10 +- .../duplicate_report/reject_controller.ex | 5 - .../controllers/image/delete_controller.ex | 14 +- 6 files changed, 141 insertions(+), 131 deletions(-) diff --git a/lib/philomena/duplicate_reports.ex b/lib/philomena/duplicate_reports.ex index 38d620ff..754f015c 100644 --- a/lib/philomena/duplicate_reports.ex +++ b/lib/philomena/duplicate_reports.ex @@ -74,45 +74,32 @@ defmodule Philomena.DuplicateReports do |> Repo.insert() end - # TODO: can we get this in a single transaction? - def accept_duplicate_report(%DuplicateReport{} = duplicate_report, user) do - changeset = - duplicate_report - |> DuplicateReport.accept_changeset(user) + def accept_duplicate_report(multi \\ nil, %DuplicateReport{} = duplicate_report, user) do + duplicate_report = Repo.preload(duplicate_report, [:image, :duplicate_of_image]) - Multi.new() + other_duplicate_reports = + DuplicateReport + |> where( + [dr], + (dr.image_id == ^duplicate_report.image_id and + dr.duplicate_of_image_id == ^duplicate_report.duplicate_of_image_id) or + (dr.image_id == ^duplicate_report.duplicate_of_image_id and + dr.duplicate_of_image_id == ^duplicate_report.image_id) + ) + |> where([dr], dr.id != ^duplicate_report.id) + |> update(set: [state: "rejected"]) + + changeset = DuplicateReport.accept_changeset(duplicate_report, user) + + multi = multi || Multi.new() + + multi |> Multi.update(:duplicate_report, changeset) - |> Multi.run(:other_reports, fn repo, %{duplicate_report: duplicate_report} -> - {count, nil} = - DuplicateReport - |> where( - [dr], - (dr.image_id == ^duplicate_report.image_id and - dr.duplicate_of_image_id == ^duplicate_report.duplicate_of_image_id) or - (dr.image_id == ^duplicate_report.duplicate_of_image_id and - dr.duplicate_of_image_id == ^duplicate_report.image_id) - ) - |> where([dr], dr.id != ^duplicate_report.id) - |> repo.update_all(set: [state: "rejected"]) - - {:ok, count} - end) - |> Repo.isolated_transaction(:serializable) - |> case do - {:ok, %{duplicate_report: duplicate_report}} -> - duplicate_report = Repo.preload(duplicate_report, [:image, :duplicate_of_image]) - - Images.merge_image(duplicate_report.image, duplicate_report.duplicate_of_image) - - error -> - error - end + |> Multi.update_all(:other_reports, other_duplicate_reports, []) + |> Images.merge_image(duplicate_report.image, duplicate_report.duplicate_of_image) end def accept_reverse_duplicate_report(%DuplicateReport{} = duplicate_report, user) do - {:ok, duplicate_report} = reject_duplicate_report(duplicate_report, user) - - # Need a constraint for upsert, so have to do it the hard way new_report = DuplicateReport |> where(duplicate_of_image_id: ^duplicate_report.image_id) @@ -123,20 +110,21 @@ defmodule Philomena.DuplicateReports do if new_report do new_report else - {:ok, duplicate_report} = - %DuplicateReport{ - image_id: duplicate_report.duplicate_of_image_id, - duplicate_of_image_id: duplicate_report.image_id, - reason: Enum.join([duplicate_report.reason, "(Reverse accepted)"], "\n"), - user_id: user.id - } - |> DuplicateReport.changeset(%{}) - |> Repo.insert() - - duplicate_report + %DuplicateReport{ + image_id: duplicate_report.duplicate_of_image_id, + duplicate_of_image_id: duplicate_report.image_id, + reason: Enum.join([duplicate_report.reason, "(Reverse accepted)"], "\n"), + user_id: user.id + } + |> DuplicateReport.changeset(%{}) + |> Repo.insert!() end - accept_duplicate_report(new_report, user) + Multi.new() + |> Multi.run(:reject_duplicate_report, fn _, %{} -> + reject_duplicate_report(duplicate_report, user) + end) + |> accept_duplicate_report(new_report, user) end def claim_duplicate_report(%DuplicateReport{} = duplicate_report, user) do diff --git a/lib/philomena/images.ex b/lib/philomena/images.ex index 92a3a89b..e694b1ff 100644 --- a/lib/philomena/images.ex +++ b/lib/philomena/images.ex @@ -24,6 +24,7 @@ defmodule Philomena.Images do alias Philomena.Tags.Tag alias Philomena.Notifications alias Philomena.Interactions + alias Philomena.Reports alias Philomena.Reports.Report alias Philomena.Comments @@ -365,66 +366,79 @@ defmodule Philomena.Images do |> Repo.update() end - def hide_image(%Image{} = image, user, attrs) do - DuplicateReport - |> where(state: "open") - |> where([d], d.image_id == ^image.id or d.duplicate_of_image_id == ^image.id) - |> Repo.update_all(set: [state: "rejected"]) - - Image.hide_changeset(image, attrs, user) - |> internal_hide_image(image) - end - def update_hide_reason(%Image{} = image, attrs) do image |> Image.hide_reason_changeset(attrs) |> Repo.update() - end + |> case do + {:ok, image} -> + reindex_image(image) - def merge_image(%Image{} = image, duplicate_of_image) do - result = - Image.merge_changeset(image, duplicate_of_image) - |> internal_hide_image(image) + {:ok, image} - case result do - {:ok, changes} -> - update_first_seen_at( - duplicate_of_image, - image.first_seen_at, - duplicate_of_image.first_seen_at - ) - - tags = Tags.copy_tags(image, duplicate_of_image) - Comments.migrate_comments(image, duplicate_of_image) - Interactions.migrate_interactions(image, duplicate_of_image) - - {:ok, %{changes | tags: changes.tags ++ tags}} - - _error -> - result + error -> + error end end - defp update_first_seen_at(image, time_1, time_2) do - min_time = - case NaiveDateTime.compare(time_1, time_2) do - :gt -> time_2 - _ -> time_1 - end + def hide_image(%Image{} = image, user, attrs) do + duplicate_reports = + DuplicateReport + |> where(state: "open") + |> where([d], d.image_id == ^image.id or d.duplicate_of_image_id == ^image.id) + |> update(set: [state: "rejected"]) - Image - |> where(id: ^image.id) - |> Repo.update_all(set: [first_seen_at: min_time]) + image + |> Image.hide_changeset(attrs, user) + |> hide_image_multi(image, Ecto.Multi.new()) + |> Multi.update_all(:duplicate_reports, duplicate_reports, []) + |> Repo.transaction() + |> process_after_hide() end - defp internal_hide_image(changeset, image) do + def merge_image(multi \\ nil, %Image{} = image, duplicate_of_image) do + multi = multi || Multi.new() + + image + |> Image.merge_changeset(duplicate_of_image) + |> hide_image_multi(image, multi) + |> Multi.run(:first_seen_at, fn _, %{} -> + update_first_seen_at( + duplicate_of_image, + image.first_seen_at, + duplicate_of_image.first_seen_at + ) + end) + |> Multi.run(:copy_tags, fn _, %{} -> + {:ok, Tags.copy_tags(image, duplicate_of_image)} + end) + |> Multi.run(:migrate_comments, fn _, %{} -> + {:ok, Comments.migrate_comments(image, duplicate_of_image)} + end) + |> Multi.run(:migrate_interactions, fn _, %{} -> + {:ok, Interactions.migrate_interactions(image, duplicate_of_image)} + end) + |> Repo.transaction() + |> process_after_hide() + |> case do + {:ok, result} -> + reindex_image(duplicate_of_image) + + {:ok, result} + + error -> + error + end + end + + defp hide_image_multi(changeset, image, multi) do reports = Report |> where(reportable_type: "Image", reportable_id: ^image.id) |> select([r], r.id) |> update(set: [open: false, state: "closed"]) - Multi.new() + multi |> Multi.update(:image, changeset) |> Multi.update_all(:reports, reports, []) |> Multi.run(:tags, fn repo, %{image: image} -> @@ -440,12 +454,40 @@ defmodule Philomena.Images do {:ok, image.tags} end) - |> Multi.run(:file, fn _repo, %{image: image} -> - Hider.hide_thumbnails(image, image.hidden_image_key) + end - {:ok, nil} - end) - |> Repo.isolated_transaction(:serializable) + defp process_after_hide(result) do + case result do + {:ok, %{image: image, tags: tags, reports: {_count, reports}} = result} -> + Hider.hide_thumbnails(image, image.hidden_image_key) + + Reports.reindex_reports(reports) + Tags.reindex_tags(tags) + reindex_image(image) + reindex_copied_tags(result) + + {:ok, result} + + error -> + error + end + end + + defp reindex_copied_tags(%{copy_tags: tags}), do: Tags.reindex_tags(tags) + defp reindex_copied_tags(_result), do: nil + + defp update_first_seen_at(image, time_1, time_2) do + min_time = + case NaiveDateTime.compare(time_1, time_2) do + :gt -> time_2 + _ -> time_1 + end + + Image + |> where(id: ^image.id) + |> Repo.update_all(set: [first_seen_at: min_time]) + + {:ok, image} end def unhide_image(%Image{hidden_from_users: true} = image) do @@ -463,12 +505,19 @@ defmodule Philomena.Images do {:ok, image.tags} end) - |> Multi.run(:file, fn _repo, %{image: image} -> - Hider.unhide_thumbnails(image, key) + |> Repo.transaction() + |> case do + {:ok, %{image: image, tags: tags}} -> + Hider.unhide_thumbnails(image, key) - {:ok, nil} - end) - |> Repo.isolated_transaction(:serializable) + reindex_image(image) + Tags.reindex_tags(tags) + + {:ok, image} + + error -> + error + end end def unhide_image(image), do: {:ok, image} diff --git a/lib/philomena_web/controllers/duplicate_report/accept_controller.ex b/lib/philomena_web/controllers/duplicate_report/accept_controller.ex index 25b25763..24dd76a2 100644 --- a/lib/philomena_web/controllers/duplicate_report/accept_controller.ex +++ b/lib/philomena_web/controllers/duplicate_report/accept_controller.ex @@ -3,8 +3,6 @@ defmodule PhilomenaWeb.DuplicateReport.AcceptController do alias Philomena.DuplicateReports.DuplicateReport alias Philomena.DuplicateReports - alias Philomena.Reports - alias Philomena.Images plug PhilomenaWeb.CanaryMapPlug, create: :edit, delete: :edit @@ -15,18 +13,14 @@ defmodule PhilomenaWeb.DuplicateReport.AcceptController do preload: [:image, :duplicate_of_image] def create(conn, _params) do - {:ok, %{reports: {_count, reports}}} = + {:ok, _report} = DuplicateReports.accept_duplicate_report( conn.assigns.duplicate_report, conn.assigns.current_user ) - Reports.reindex_reports(reports) - Images.reindex_image(conn.assigns.duplicate_report.image) - Images.reindex_image(conn.assigns.duplicate_report.duplicate_of_image) - conn |> put_flash(:info, "Successfully accepted report.") - |> redirect(external: conn.assigns.referrer) + |> redirect(to: Routes.duplicate_report_path(conn, :index)) end end diff --git a/lib/philomena_web/controllers/duplicate_report/accept_reverse_controller.ex b/lib/philomena_web/controllers/duplicate_report/accept_reverse_controller.ex index bb16c41c..b2c8a940 100644 --- a/lib/philomena_web/controllers/duplicate_report/accept_reverse_controller.ex +++ b/lib/philomena_web/controllers/duplicate_report/accept_reverse_controller.ex @@ -3,8 +3,6 @@ defmodule PhilomenaWeb.DuplicateReport.AcceptReverseController do alias Philomena.DuplicateReports.DuplicateReport alias Philomena.DuplicateReports - alias Philomena.Reports - alias Philomena.Images plug PhilomenaWeb.CanaryMapPlug, create: :edit, delete: :edit @@ -15,18 +13,14 @@ defmodule PhilomenaWeb.DuplicateReport.AcceptReverseController do preload: [:image, :duplicate_of_image] def create(conn, _params) do - {:ok, %{reports: {_count, reports}}} = + {:ok, _report} = DuplicateReports.accept_reverse_duplicate_report( conn.assigns.duplicate_report, conn.assigns.current_user ) - Reports.reindex_reports(reports) - Images.reindex_image(conn.assigns.duplicate_report.image) - Images.reindex_image(conn.assigns.duplicate_report.duplicate_of_image) - conn |> put_flash(:info, "Successfully accepted report in reverse.") - |> redirect(external: conn.assigns.referrer) + |> redirect(to: Routes.duplicate_report_path(conn, :index)) end end diff --git a/lib/philomena_web/controllers/duplicate_report/reject_controller.ex b/lib/philomena_web/controllers/duplicate_report/reject_controller.ex index 280099ef..41ab6dab 100644 --- a/lib/philomena_web/controllers/duplicate_report/reject_controller.ex +++ b/lib/philomena_web/controllers/duplicate_report/reject_controller.ex @@ -3,7 +3,6 @@ defmodule PhilomenaWeb.DuplicateReport.RejectController do alias Philomena.DuplicateReports.DuplicateReport alias Philomena.DuplicateReports - alias Philomena.Images plug PhilomenaWeb.CanaryMapPlug, create: :edit, delete: :edit @@ -20,10 +19,6 @@ defmodule PhilomenaWeb.DuplicateReport.RejectController do conn.assigns.current_user ) - {:ok, _image} = Images.unhide_image(conn.assigns.duplicate_report.image) - Images.reindex_image(conn.assigns.duplicate_report.image) - Images.reindex_image(conn.assigns.duplicate_report.duplicate_of_image) - conn |> put_flash(:info, "Successfully rejected report.") |> redirect(external: conn.assigns.referrer) diff --git a/lib/philomena_web/controllers/image/delete_controller.ex b/lib/philomena_web/controllers/image/delete_controller.ex index 277d51d6..c9aae40d 100644 --- a/lib/philomena_web/controllers/image/delete_controller.ex +++ b/lib/philomena_web/controllers/image/delete_controller.ex @@ -6,8 +6,6 @@ defmodule PhilomenaWeb.Image.DeleteController do alias Philomena.Images.Image alias Philomena.Images - alias Philomena.Tags - alias Philomena.Reports plug PhilomenaWeb.CanaryMapPlug, create: :hide, delete: :hide plug :load_and_authorize_resource, model: Image, id_name: "image_id", persisted: true @@ -18,11 +16,7 @@ defmodule PhilomenaWeb.Image.DeleteController do user = conn.assigns.current_user case Images.hide_image(image, user, image_params) do - {:ok, %{image: image, tags: tags, reports: {_count, reports}}} -> - Images.reindex_image(image) - Reports.reindex_reports(reports) - Tags.reindex_tags(tags) - + {:ok, _image} -> conn |> put_flash(:info, "Image successfully hidden.") |> redirect(to: Routes.image_path(conn, :show, image)) @@ -37,8 +31,6 @@ defmodule PhilomenaWeb.Image.DeleteController do case Images.update_hide_reason(image, image_params) do {:ok, image} -> - Images.reindex_image(image) - conn |> put_flash(:info, "Hide reason updated.") |> redirect(to: Routes.image_path(conn, :show, image)) @@ -66,9 +58,7 @@ defmodule PhilomenaWeb.Image.DeleteController do def delete(conn, _params) do image = conn.assigns.image - {:ok, %{image: image, tags: tags}} = Images.unhide_image(image) - Images.reindex_image(image) - Tags.reindex_tags(tags) + {:ok, _image} = Images.unhide_image(image) conn |> put_flash(:info, "Image successfully unhidden.")