From be97f2fc07b4f7e06c04c892d17ccd3e077015f4 Mon Sep 17 00:00:00 2001 From: Jeremy Friesen Date: Wed, 24 Nov 2021 13:56:16 -0500 Subject: [PATCH] Favoring dependent: :delete_all over :destroy (#15442) * Favoring dependent: :delete_all over :destroy The `dependent: :destroy` callback is slower than `dependent: :delete` and `dependent: :delete_all`. We need only favor the `dependent: :destroy` when there's callbacks that happen. In the case of :destroy, ActiveRecord instantiates each object and then runs destroy. Whereas in the case of :delete, ActiveRecord issues a SQL command to delete the related files. It is often "safer" to use :destroy, as it guarantees that you'll instantiate the record and run it's callbacks. But sometimes you have to go with the speed of SQL. This relates to #14140. Note there is still more to consider, but given that we're looking at moving from :destroy on all of an article's page views to :delete, we might buy enough time in the callback. * Removing redundancy of article destroy Prior to this commit, `before_destroy_actions` called the `bust_cache` method which in turn called `touch_actor_latest_article_updated_at` but `bust_cache` did not pass the destroying parameter. Then `before_destroy_actions` immediately called `touch_actor_latest_article_updated_at` with `destroying: true`. With this change, we remove one of those duplicate calls. * Fixing specs regarding relationships * Fixing specs regarding relationships --- app/models/article.rb | 21 ++++++++++++--------- app/models/discussion_lock.rb | 4 ++++ app/models/mention.rb | 4 ++++ app/models/notification_subscription.rb | 4 ++++ app/models/page_view.rb | 3 +++ app/models/poll.rb | 6 +++--- app/models/poll_option.rb | 3 +++ app/models/poll_skip.rb | 4 ++++ app/models/poll_vote.rb | 4 ++++ app/models/profile_pin.rb | 3 +++ spec/models/article_spec.rb | 10 +++++----- spec/models/poll_spec.rb | 6 +++--- 12 files changed, 52 insertions(+), 20 deletions(-) diff --git a/app/models/article.rb b/app/models/article.rb index 2ac777f0e..d2a523037 100644 --- a/app/models/article.rb +++ b/app/models/article.rb @@ -32,17 +32,21 @@ class Article < ApplicationRecord UNIQUE_URL_ERROR = "has already been taken. " \ "Email #{ForemInstance.email} for further details.".freeze - has_one :discussion_lock, dependent: :destroy + has_one :discussion_lock, dependent: :delete - has_many :mentions, as: :mentionable, inverse_of: :mentionable, dependent: :destroy + has_many :mentions, as: :mentionable, inverse_of: :mentionable, dependent: :delete_all has_many :comments, as: :commentable, inverse_of: :commentable, dependent: :nullify has_many :html_variant_successes, dependent: :nullify has_many :html_variant_trials, dependent: :nullify - has_many :notification_subscriptions, as: :notifiable, inverse_of: :notifiable, dependent: :destroy + has_many :notification_subscriptions, as: :notifiable, inverse_of: :notifiable, dependent: :delete_all has_many :notifications, as: :notifiable, inverse_of: :notifiable, dependent: :delete_all - has_many :page_views, dependent: :destroy + has_many :page_views, dependent: :delete_all + # `dependent: :destroy` because in Poll we cascade the deletes of + # the poll votes, options, and skips. has_many :polls, dependent: :destroy - has_many :profile_pins, as: :pinnable, inverse_of: :pinnable, dependent: :destroy + has_many :profile_pins, as: :pinnable, inverse_of: :pinnable, dependent: :delete_all + # `dependent: :destroy` because in RatingVote we're relying on + # counter_culture to do some additional tallies has_many :rating_votes, dependent: :destroy has_many :top_comments, lambda { @@ -585,8 +589,7 @@ class Article < ApplicationRecord end def before_destroy_actions - bust_cache - touch_actor_latest_article_updated_at(destroying: true) + bust_cache(destroying: true) article_ids = user.article_ids.dup if organization organization.touch(:last_article_at) @@ -794,13 +797,13 @@ class Article < ApplicationRecord organization&.touch(:latest_article_updated_at) end - def bust_cache + def bust_cache(destroying: false) cache_bust = EdgeCache::Bust.new cache_bust.call(path) cache_bust.call("#{path}?i=i") cache_bust.call("#{path}?preview=#{password}") async_bust - touch_actor_latest_article_updated_at + touch_actor_latest_article_updated_at(destroying: destroying) end def calculate_base_scores diff --git a/app/models/discussion_lock.rb b/app/models/discussion_lock.rb index e5d96b4f9..96d39b01a 100644 --- a/app/models/discussion_lock.rb +++ b/app/models/discussion_lock.rb @@ -1,6 +1,10 @@ # @note When we destroy the related user, it's using dependent: # :delete for the relationship. That means no before/after # destroy callbacks will be called on this object. +# +# @note When we destroy the related article, it's using dependent: +# :delete for the relationship. That means no before/after +# destroy callbacks will be called on this object. class DiscussionLock < ApplicationRecord belongs_to :article belongs_to :locking_user, class_name: "User" diff --git a/app/models/mention.rb b/app/models/mention.rb index 9b4936172..0db414f1c 100644 --- a/app/models/mention.rb +++ b/app/models/mention.rb @@ -1,6 +1,10 @@ # @note When we destroy the related user, it's using dependent: # :delete for the relationship. That means no before/after # destroy callbacks will be called on this object. +# +# @note When we destroy the related article, it's using dependent: +# :delete for the relationship. That means no before/after +# destroy callbacks will be called on this object. class Mention < ApplicationRecord belongs_to :user belongs_to :mentionable, polymorphic: true diff --git a/app/models/notification_subscription.rb b/app/models/notification_subscription.rb index 446409f4a..ba5db5ee9 100644 --- a/app/models/notification_subscription.rb +++ b/app/models/notification_subscription.rb @@ -1,6 +1,10 @@ # @note When we destroy the related user, it's using dependent: # :delete for the relationship. That means no before/after # destroy callbacks will be called on this object. +# +# @note When we destroy the related article, it's using dependent: +# :delete for the relationship. That means no before/after +# destroy callbacks will be called on this object. class NotificationSubscription < ApplicationRecord belongs_to :notifiable, polymorphic: true belongs_to :user diff --git a/app/models/page_view.rb b/app/models/page_view.rb index 96297f495..1fe745faa 100644 --- a/app/models/page_view.rb +++ b/app/models/page_view.rb @@ -1,3 +1,6 @@ +# @note When we destroy the related article, it's using dependent: +# :delete for the relationship. That means no before/after +# destroy callbacks will be called on this object. class PageView < ApplicationRecord belongs_to :user, optional: true belongs_to :article diff --git a/app/models/poll.rb b/app/models/poll.rb index c8904690f..6f5b5488a 100644 --- a/app/models/poll.rb +++ b/app/models/poll.rb @@ -5,9 +5,9 @@ class Poll < ApplicationRecord belongs_to :article - has_many :poll_options, dependent: :destroy - has_many :poll_skips, dependent: :destroy - has_many :poll_votes, dependent: :destroy + has_many :poll_options, dependent: :delete_all + has_many :poll_skips, dependent: :delete_all + has_many :poll_votes, dependent: :delete_all validates :poll_options_count, presence: true validates :poll_options_input_array, presence: true, length: { minimum: 2, maximum: 15 } diff --git a/app/models/poll_option.rb b/app/models/poll_option.rb index d0eb57aaf..e1662b172 100644 --- a/app/models/poll_option.rb +++ b/app/models/poll_option.rb @@ -1,3 +1,6 @@ +# @note When we destroy the related poll, it's using dependent: +# :delete for the relationship. That means no before/after +# destroy callbacks will be called on this object. class PollOption < ApplicationRecord belongs_to :poll has_many :poll_votes, dependent: :destroy diff --git a/app/models/poll_skip.rb b/app/models/poll_skip.rb index d63841f60..8e2b357d1 100644 --- a/app/models/poll_skip.rb +++ b/app/models/poll_skip.rb @@ -1,6 +1,10 @@ # @note When we destroy the related user, it's using dependent: # :delete for the relationship. That means no before/after # destroy callbacks will be called on this object. +# +# @note When we destroy the related poll, it's using dependent: +# :delete for the relationship. That means no before/after +# destroy callbacks will be called on this object. class PollSkip < ApplicationRecord belongs_to :poll belongs_to :user diff --git a/app/models/poll_vote.rb b/app/models/poll_vote.rb index 27e707797..11dd3fcfa 100644 --- a/app/models/poll_vote.rb +++ b/app/models/poll_vote.rb @@ -1,6 +1,10 @@ # @note When we destroy the related user, it's using dependent: # :delete for the relationship. That means no before/after # destroy callbacks will be called on this object. +# +# @note When we destroy the related poll, it's using dependent: +# :delete for the relationship. That means no before/after +# destroy callbacks will be called on this object. class PollVote < ApplicationRecord belongs_to :user belongs_to :poll_option diff --git a/app/models/profile_pin.rb b/app/models/profile_pin.rb index b687a6e28..e39fbb578 100644 --- a/app/models/profile_pin.rb +++ b/app/models/profile_pin.rb @@ -1,3 +1,6 @@ +# @note When we destroy the related article (via pinnable), it's using +# dependent: :delete for the relationship. That means no +# before/after destroy callbacks will be called on this object. class ProfilePin < ApplicationRecord belongs_to :pinnable, polymorphic: true belongs_to :profile, polymorphic: true diff --git a/spec/models/article_spec.rb b/spec/models/article_spec.rb index ef538fd8b..a762bb351 100644 --- a/spec/models/article_spec.rb +++ b/spec/models/article_spec.rb @@ -18,17 +18,17 @@ RSpec.describe Article, type: :model do it { is_expected.to belong_to(:organization).optional } it { is_expected.to belong_to(:user) } - it { is_expected.to have_one(:discussion_lock).dependent(:destroy) } + it { is_expected.to have_one(:discussion_lock).dependent(:delete) } it { is_expected.to have_many(:comments).dependent(:nullify) } - it { is_expected.to have_many(:mentions).dependent(:destroy) } + it { is_expected.to have_many(:mentions).dependent(:delete_all) } it { is_expected.to have_many(:html_variant_successes).dependent(:nullify) } it { is_expected.to have_many(:html_variant_trials).dependent(:nullify) } - it { is_expected.to have_many(:notification_subscriptions).dependent(:destroy) } + it { is_expected.to have_many(:notification_subscriptions).dependent(:delete_all) } it { is_expected.to have_many(:notifications).dependent(:delete_all) } - it { is_expected.to have_many(:page_views).dependent(:destroy) } + it { is_expected.to have_many(:page_views).dependent(:delete_all) } it { is_expected.to have_many(:polls).dependent(:destroy) } - it { is_expected.to have_many(:profile_pins).dependent(:destroy) } + it { is_expected.to have_many(:profile_pins).dependent(:delete_all) } it { is_expected.to have_many(:rating_votes).dependent(:destroy) } it { is_expected.to have_many(:sourced_subscribers) } it { is_expected.to have_many(:reactions).dependent(:destroy) } diff --git a/spec/models/poll_spec.rb b/spec/models/poll_spec.rb index 1811de639..316dc8b62 100644 --- a/spec/models/poll_spec.rb +++ b/spec/models/poll_spec.rb @@ -9,9 +9,9 @@ RSpec.describe Poll, type: :model do describe "builtin validations" do subject { poll } - it { is_expected.to have_many(:poll_options).dependent(:destroy) } - it { is_expected.to have_many(:poll_skips).dependent(:destroy) } - it { is_expected.to have_many(:poll_votes).dependent(:destroy) } + it { is_expected.to have_many(:poll_options).dependent(:delete_all) } + it { is_expected.to have_many(:poll_skips).dependent(:delete_all) } + it { is_expected.to have_many(:poll_votes).dependent(:delete_all) } it { is_expected.to validate_length_of(:poll_options_input_array).is_at_least(2).is_at_most(15) }