PERF: Avoid running query unnecessarily when updating bookmark. (#14276)

PERF: Avoid running query unnecessarily when updating bookmark. (#14276)

  • Avoid loading an entire ActiveRecord object when saving and updating.
  • Avoid running a DB query when post_id or user_id is not changed.
diff --git a/app/models/bookmark.rb b/app/models/bookmark.rb
index ac76bb1..6bc1a01 100644
--- a/app/models/bookmark.rb
+++ b/app/models/bookmark.rb
@@ -31,7 +31,10 @@ class Bookmark < ActiveRecord::Base
     if: -> { reminder_type.present? }
   }
 
-  validate :unique_per_post_for_user
+  validate :unique_per_post_for_user,
+    on: [:create, :update],
+    if: Proc.new { |b| b.will_save_change_to_post_id? || b.will_save_change_to_user_id? }
+
   validate :ensure_sane_reminder_at_time
   validate :bookmark_limit_not_reached
   validates :name, length: { maximum: 100 }
@@ -47,9 +50,9 @@ class Bookmark < ActiveRecord::Base
   end
 
   def unique_per_post_for_user
-    existing_bookmark = Bookmark.find_by(user_id: user_id, post_id: post_id)
-    return if existing_bookmark.blank? || existing_bookmark.id == id
-    self.errors.add(:base, I18n.t("bookmarks.errors.already_bookmarked_post"))
+    if Bookmark.exists?(user_id: user_id, post_id: post_id)
+      self.errors.add(:base, I18n.t("bookmarks.errors.already_bookmarked_post"))
+    end
   end
 
   def ensure_sane_reminder_at_time
diff --git a/spec/models/bookmark_spec.rb b/spec/models/bookmark_spec.rb
index 500f4bf..401712c 100644
--- a/spec/models/bookmark_spec.rb
+++ b/spec/models/bookmark_spec.rb
@@ -3,9 +3,25 @@
 require 'rails_helper'
 
 describe Bookmark do
+  fab!(:post) { Fabricate(:post) }
+
+  context 'validations' do
+    it 'does not allow user to bookmark a post twice' do
+      bookmark = Fabricate(:bookmark, post: post, topic: post.topic)
+      user = bookmark.user
+
+      bookmark_2 = Fabricate.build(:bookmark,
+        post: post,
+        topic: post.topic,
+        user: user
+      )
+
+      expect(bookmark_2.valid?).to eq(false)
+    end
+  end
+
   describe "#cleanup!" do
     it "deletes bookmarks attached to a deleted post which has been deleted for > 3 days" do
-      post = Fabricate(:post)
       bookmark = Fabricate(:bookmark, post: post, topic: post.topic)
       bookmark2 = Fabricate(:bookmark, post: Fabricate(:post, topic: post.topic))
       post.trash!
@@ -16,7 +32,6 @@ describe Bookmark do
     end
 
     it "runs a SyncTopicUserBookmarked job for all deleted bookmark unique topics to make sure topic_user.bookmarked is in sync" do
-      post = Fabricate(:post)
       post2 = Fabricate(:post)
       bookmark = Fabricate(:bookmark, post: post, topic: post.topic)
       bookmark2 = Fabricate(:bookmark, post: Fabricate(:post, topic: post.topic))
@@ -36,7 +51,6 @@ describe Bookmark do
     end
 
     it "deletes bookmarks attached to a deleted topic which has been deleted for > 3 days" do
-      post = Fabricate(:post)
       bookmark = Fabricate(:bookmark, post: post, topic: post.topic)
       bookmark2 = Fabricate(:bookmark, topic: post.topic, post: Fabricate(:post, topic: post.topic))
       bookmark3 = Fabricate(:bookmark)
@@ -49,7 +63,6 @@ describe Bookmark do
     end
 
     it "does not delete bookmarks attached to posts that are not deleted or that have not met the 3 day grace period" do
-      post = Fabricate(:post)
       bookmark = Fabricate(:bookmark, post: post, topic: post.topic)
       bookmark2 = Fabricate(:bookmark)
       Bookmark.cleanup!

GitHub sha: 1e05175364e2de9e73f723bdf883e734711404dd

This commit appears in #14276 which was approved by eviltrout. It was merged by martin.