PERF: Do not include thumbnail information in default topic list payload (#10163)

PERF: Do not include thumbnail information in default topic list payload (#10163)

Now it is only included when a theme/plugin has requested it.

diff --git a/app/models/topic.rb b/app/models/topic.rb
index a32c07d..54c7ffc 100644
--- a/app/models/topic.rb
+++ b/app/models/topic.rb
@@ -101,12 +101,20 @@ class Topic < ActiveRecord::Base
     end
   end
 
-  def image_url
+  def image_url(enqueue_if_missing: false)
     thumbnail = topic_thumbnails.detect do |record|
       record.max_width == Topic.share_thumbnail_size[0] &&
         record.max_height == Topic.share_thumbnail_size[1]
     end
 
+    if thumbnail.nil? &&
+        image_upload &&
+        SiteSetting.create_thumbnails &&
+        enqueue_if_missing &&
+        Discourse.redis.set(thumbnail_job_redis_key([]), 1, nx: true, ex: 1.minute)
+      Jobs.enqueue(:generate_topic_thumbnails, { topic_id: id })
+    end
+
     raw_url = thumbnail&.optimized_image&.url || image_upload&.url
     UrlHelper.cook_url(raw_url, secure: image_upload&.secure?)
   end
diff --git a/app/serializers/listable_topic_serializer.rb b/app/serializers/listable_topic_serializer.rb
index 52d7fb2..de03718 100644
--- a/app/serializers/listable_topic_serializer.rb
+++ b/app/serializers/listable_topic_serializer.rb
@@ -31,7 +31,7 @@ class ListableTopicSerializer < BasicTopicSerializer
   has_one :last_poster, serializer: BasicUserSerializer, embed: :objects
 
   def image_url
-    object.image_url
+    object.image_url(enqueue_if_missing: true)
   end
 
   def thumbnails
@@ -39,6 +39,10 @@ class ListableTopicSerializer < BasicTopicSerializer
     object.thumbnail_info(enqueue_if_missing: true, extra_sizes: extra_sizes)
   end
 
+  def include_thumbnails?
+    ThemeModifierHelper.new(request: scope.request).topic_thumbnail_sizes.present? || DiscoursePluginRegistry.topic_thumbnail_sizes.present?
+  end
+
   def include_unicode_title?
     object.title.match?(/:[\w\-+]+:/)
   end
diff --git a/spec/integration/topic_thumbnail_spec.rb b/spec/integration/topic_thumbnail_spec.rb
index dcdbb08..2733fab 100644
--- a/spec/integration/topic_thumbnail_spec.rb
+++ b/spec/integration/topic_thumbnail_spec.rb
@@ -12,42 +12,15 @@ describe "Topic Thumbnails" do
     def get_topic
       Discourse.redis.del(topic.thumbnail_job_redis_key(Topic.thumbnail_sizes))
       get '/latest.json'
+      expect(response.status).to eq(200)
       response.parsed_body["topic_list"]["topics"][0]
     end
 
-    it "includes thumbnails" do
-      topic_json = nil
-      expect do
-        topic_json = get_topic
-      end.to change { Jobs::GenerateTopicThumbnails.jobs.size }.by(1)
-
-      thumbnails = topic_json["thumbnails"]
-
-      # Original only. Optimized not yet generated
-      expect(thumbnails.length).to eq(1)
-
-      # Original
-      expect(thumbnails[0]["max_width"]).to eq(nil)
-      expect(thumbnails[0]["max_height"]).to eq(nil)
-      expect(thumbnails[0]["width"]).to eq(image.width)
-      expect(thumbnails[0]["height"]).to eq(image.height)
-      expect(thumbnails[0]["url"]).to end_with(image.url)
-
-      # Run the job
-      args = Jobs::GenerateTopicThumbnails.jobs.last["args"].first
-      Jobs::GenerateTopicThumbnails.new.execute(args.with_indifferent_access)
-
-      # Re-request
-      expect do
-        topic_json = get_topic
-      end.to change { Jobs::GenerateTopicThumbnails.jobs.size }.by(0)
-      thumbnails = topic_json["thumbnails"]
-
-      expect(thumbnails[1]["max_width"]).to eq(Topic.share_thumbnail_size[0])
-      expect(thumbnails[1]["max_height"]).to eq(Topic.share_thumbnail_size[1])
-      expect(thumbnails[1]["width"]).to eq(1024)
-      expect(thumbnails[1]["height"]).to eq(1024)
-      expect(thumbnails[1]["url"]).to include("/optimized/")
+    it "does not include thumbnails by default" do
+
+      topic_json = get_topic
+
+      expect(topic_json["thumbnails"]).to eq(nil)
     end
 
     context "with a theme" do
@@ -69,6 +42,18 @@ describe "Topic Thumbnails" do
           topic_json = get_topic
         end.to change { Jobs::GenerateTopicThumbnails.jobs.size }.by(1)
 
+        thumbnails = topic_json["thumbnails"]
+
+        # Original only. Optimized not yet generated
+        expect(thumbnails.length).to eq(1)
+
+        # Original
+        expect(thumbnails[0]["max_width"]).to eq(nil)
+        expect(thumbnails[0]["max_height"]).to eq(nil)
+        expect(thumbnails[0]["width"]).to eq(image.width)
+        expect(thumbnails[0]["height"]).to eq(image.height)
+        expect(thumbnails[0]["url"]).to end_with(image.url)
+
         # Run the job
         args = Jobs::GenerateTopicThumbnails.jobs.last["args"].first
         Jobs::GenerateTopicThumbnails.new.execute(args.with_indifferent_access)
@@ -82,6 +67,14 @@ describe "Topic Thumbnails" do
 
         # Original + Optimized + 3 theme requests
         expect(thumbnails.length).to eq(5)
+
+        # Check first optimized
+        expect(thumbnails[1]["max_width"]).to eq(Topic.share_thumbnail_size[0])
+        expect(thumbnails[1]["max_height"]).to eq(Topic.share_thumbnail_size[1])
+        expect(thumbnails[1]["width"]).to eq(1024)
+        expect(thumbnails[1]["height"]).to eq(1024)
+        expect(thumbnails[1]["url"]).to include("/optimized/")
+
       end
     end
 

GitHub sha: cb1b472a

1 Like

This commit appears in #10163 which was approved by eviltrout and tgxworld. It was merged by davidtaylorhq.

@davidtaylorhq Sorry I just realized that we should also not preload the records if the theme does not have this enable.

The problem is that we need the thumbnail/upload records to be loaded for calculating image_url. That has been part of the API for years, so I don’t think we want to remove it?