FIX: Video thumbnails can have duplicates (#21681)

* FIX: Video thumbnails can have duplicates

It's possible that a duplicate video or even a very similar video could
generate the same video thumbnail. Because video thumbnails are mapped
to their corresponding video by using the video sha1 in the thumbnail
filename we need to allow for duplicate thumbnails otherwise even when a
thumbnail has been generated for a topic it will not be mapped
correctly.

This will also allow you to re-upload a video on the same topic to
regenerate the thumbnail.

* fix typo

app/models/post.rb

@@ -1020,7 +1020,10 @@ class Post < ActiveRecord::Base
         # and there is no thumbnail info added to the markdown to tie the thumbnail to the topic/post after
         # creation.
         thumbnail =
-          Upload.where("original_filename like ?", "#{upload.sha1}.%").first if upload.sha1.present?
+          Upload
+            .where("original_filename like ?", "#{upload.sha1}.%")
+            .order(id: :desc)
+            .first if upload.sha1.present?
         if thumbnail.present? && self.is_first_post? && !self.topic.image_upload_id
           upload_ids << thumbnail.id
           self.topic.update_column(:image_upload_id, thumbnail.id)

lib/upload_creator.rb

@@ -74,6 +74,7 @@ class UploadCreator
     is_image = FileHelper.is_supported_image?(@filename)
     is_image ||= @image_info && FileHelper.is_supported_image?("test.#{@image_info.type}")
     is_image = false if @opts[:for_theme]
+    is_thumbnail = SiteSetting.video_thumbnails_enabled && @opts[:type] == "thumbnail"
 
     # If this is present then it means we are creating an upload record from
     # an external_upload_stub and the file is > ExternalUploadManager::DOWNLOAD_LIMIT,
@@ -119,13 +120,17 @@ class UploadCreator
       # compute the sha of the file and generate a unique hash
       # which is only used for secure uploads
       sha1 = Upload.generate_digest(@file) if !external_upload_too_big
-      unique_hash = generate_fake_sha1_hash if SiteSetting.secure_uploads || external_upload_too_big
+      unique_hash = generate_fake_sha1_hash if SiteSetting.secure_uploads ||
+        external_upload_too_big || is_thumbnail
 
       # we do not check for duplicate uploads if secure uploads is
       # enabled because we use a unique access hash to differentiate
       # between uploads instead of the sha1, and to get around various
       # access/permission issues for uploads
-      if !SiteSetting.secure_uploads && !external_upload_too_big
+      # We do not check for duplicate uploads for video thumbnails because
+      # their filename needs to match with their corresponding video. This also
+      # enables rebuilding the html on a topic to regenerate a thumbnail.
+      if !SiteSetting.secure_uploads && !external_upload_too_big && !is_thumbnail
         # do we already have that upload?
         @upload = Upload.find_by(sha1: sha1)
 
@@ -163,7 +168,14 @@ class UploadCreator
       @upload.user_id = user_id
       @upload.original_filename = fixed_original_filename || @filename
       @upload.filesize = filesize
-      @upload.sha1 = (SiteSetting.secure_uploads? || external_upload_too_big) ? unique_hash : sha1
+      @upload.sha1 =
+        (
+          if (SiteSetting.secure_uploads? || external_upload_too_big || is_thumbnail)
+            unique_hash
+          else
+            sha1
+          end
+        )
       @upload.original_sha1 = SiteSetting.secure_uploads? ? sha1 : nil
       @upload.url = ""
       @upload.origin = @opts[:origin][0...1000] if @opts[:origin]

spec/lib/upload_creator_spec.rb

@@ -419,6 +419,18 @@ RSpec.describe UploadCreator do
       end
     end
 
+    context "when the video thumbnail already exists based on the sha1" do
+      let(:filename) { "smallest.png" }
+      let(:file) { file_from_fixtures(filename, "images") }
+      let!(:existing_upload) { Fabricate(:upload, sha1: Upload.generate_digest(file)) }
+      let(:opts) { { type: "thumbnail" } }
+      let(:result) { UploadCreator.new(file, filename, opts).create_for(user.id) }
+
+      it "does not return the existing upload, as duplicate uploads are allowed" do
+        expect(result).not_to eq(existing_upload)
+      end
+    end
+
     context "with secure uploads functionality" do
       let(:filename) { "logo.jpg" }
       let(:file) { file_from_fixtures(filename) }

spec/models/post_spec.rb

@@ -1580,6 +1580,18 @@ RSpec.describe Post do
       expect(post.topic.image_upload_id).to eq(image_upload_2.id)
     end
 
+    it "uses the newest thumbnail" do
+      image_upload.original_filename = "#{video_upload.sha1}.png"
+      image_upload.save!
+      image_upload_2.original_filename = "#{video_upload.sha1}.png"
+      image_upload_2.save!
+      post.link_post_uploads
+
+      post.topic.reload
+      expect(post.topic.topic_thumbnails.length).to eq(1)
+      expect(post.topic.image_upload_id).to eq(image_upload_2.id)
+    end
+
     it "does not create thumbnails when disabled" do
       SiteSetting.video_thumbnails_enabled = false
       image_upload.original_filename = "#{video_upload.sha1}.png"