FEATURE: Use path from existing URL of uploads and optimized images (#13177)

FEATURE: Use path from existing URL of uploads and optimized images (#13177)

Discourse shouldn’t dynamically calculate the path of uploads and optimized images after a file has been stored on disk or S3. Otherwise it might calculate the wrong path if the SHA1 or extension stored in the database doesn’t match the actual file path.

diff --git a/lib/file_store/base_store.rb b/lib/file_store/base_store.rb
index 44749f5..80f1fb6 100644
--- a/lib/file_store/base_store.rb
+++ b/lib/file_store/base_store.rb
@@ -3,13 +3,17 @@
 module FileStore
 
   class BaseStore
+    UPLOAD_PATH_REGEX = %r|/(original/\d+X/.*)|
+    OPTIMIZED_IMAGE_PATH_REGEX = %r|/(optimized/\d+X/.*)|
 
     def store_upload(file, upload, content_type = nil)
+      upload.url = nil
       path = get_path_for_upload(upload)
       store_file(file, path)
     end
 
     def store_optimized_image(file, optimized_image, content_type = nil, secure: false)
+      optimized_image.url = nil
       path = get_path_for_optimized_image(optimized_image)
       store_file(file, path)
     end
@@ -116,6 +120,12 @@ module FileStore
     end
 
     def get_path_for_upload(upload)
+      # try to extract the path from the URL instead of calculating it,
+      # because the calculated path might differ from the actual path
+      if upload.url.present? && (path = upload.url[UPLOAD_PATH_REGEX, 1])
+        return prefix_path(path)
+      end
+
       extension =
         if upload.extension
           ".#{upload.extension}"
@@ -128,6 +138,12 @@ module FileStore
     end
 
     def get_path_for_optimized_image(optimized_image)
+      # try to extract the path from the URL instead of calculating it,
+      # because the calculated path might differ from the actual path
+      if optimized_image.url.present? && (path = optimized_image.url[OPTIMIZED_IMAGE_PATH_REGEX, 1])
+        return prefix_path(path)
+      end
+
       upload = optimized_image.upload
       version = optimized_image.version || 1
       extension = "_#{version}_#{optimized_image.width}x#{optimized_image.height}#{optimized_image.extension}"
@@ -178,6 +194,9 @@ module FileStore
       depths.max
     end
 
+    def prefix_path(path)
+      path
+    end
   end
 
 end
diff --git a/lib/file_store/local_store.rb b/lib/file_store/local_store.rb
index 2564953..e945ac1 100644
--- a/lib/file_store/local_store.rb
+++ b/lib/file_store/local_store.rb
@@ -66,7 +66,7 @@ module FileStore
     end
 
     def get_path_for(type, upload_id, sha, extension)
-      File.join("/", upload_path, super(type, upload_id, sha, extension))
+      prefix_path(super(type, upload_id, sha, extension))
     end
 
     def copy_file(file, path)
@@ -134,5 +134,8 @@ module FileStore
       puts "#{count} of #{model.count} #{model.name.underscore.pluralize} are missing" if count > 0
     end
 
+    def prefix_path(path)
+      File.join("/", upload_path, path)
+    end
   end
 end
diff --git a/lib/file_store/s3_store.rb b/lib/file_store/s3_store.rb
index ab6e56b..c0671ca 100644
--- a/lib/file_store/s3_store.rb
+++ b/lib/file_store/s3_store.rb
@@ -22,6 +22,7 @@ module FileStore
     end
 
     def store_upload(file, upload, content_type = nil)
+      upload.url = nil
       path = get_path_for_upload(upload)
       url, upload.etag = store_file(
         file,
@@ -35,6 +36,7 @@ module FileStore
     end
 
     def store_optimized_image(file, optimized_image, content_type = nil, secure: false)
+      optimized_image.url = nil
       path = get_path_for_optimized_image(optimized_image)
       url, optimized_image.etag = store_file(file, path, content_type: content_type, private_acl: secure)
       url
diff --git a/lib/shrink_uploaded_image.rb b/lib/shrink_uploaded_image.rb
index 43416ad..fbcdfd6 100644
--- a/lib/shrink_uploaded_image.rb
+++ b/lib/shrink_uploaded_image.rb
@@ -67,7 +67,7 @@ class ShrinkUploadedImage
 
     log "base62: #{original_upload.base62_sha1} -> #{Upload.base62_sha1(sha1)}"
     log "sha: #{original_upload.sha1} -> #{sha1}"
-    log "(an exisiting upload)" if existing_upload
+    log "(an existing upload)" if existing_upload
 
     success = true
     posts = Post.unscoped.joins(:post_uploads).where(post_uploads: { upload_id: original_upload.id }).uniq.sort_by(&:created_at)
diff --git a/spec/components/email/sender_spec.rb b/spec/components/email/sender_spec.rb
index 3f80e23..bacf426 100644
--- a/spec/components/email/sender_spec.rb
+++ b/spec/components/email/sender_spec.rb
@@ -489,8 +489,8 @@ describe Email::Sender do
           let!(:optimized_image_file) { file_from_fixtures("smallest.png", "images") }
 
           before do
-            Discourse.store.store_optimized_image(optimized_image_file, optimized)
-            optimized.update(url: Discourse.store.absolute_base_url + '/' + optimized.url)
+            url = Discourse.store.store_optimized_image(optimized_image_file, optimized)
+            optimized.update(url: Discourse.store.absolute_base_url + '/' + url)
             Discourse.store.cache_file(optimized_image_file, File.basename("#{optimized.sha1}.png"))
           end
 
diff --git a/spec/components/file_store/base_store_spec.rb b/spec/components/file_store/base_store_spec.rb
index 4771b28..05befda 100644
--- a/spec/components/file_store/base_store_spec.rb
+++ b/spec/components/file_store/base_store_spec.rb
@@ -6,42 +6,136 @@ RSpec.describe FileStore::BaseStore do
   fab!(:upload) { Fabricate(:upload, id: 9999, sha1: Digest::SHA1.hexdigest('9999')) }
 
   describe '#get_path_for_upload' do
-    it 'should return the right path' do
-      expect(FileStore::BaseStore.new.get_path_for_upload(upload))
-        .to eq('original/2X/4/4170ac2a2782a1516fe9e13d7322ae482c1bd594.png')
+    def expect_correct_path(expected_path)
+      expect(described_class.new.get_path_for_upload(upload)).to eq(expected_path)
     end
 
-    describe 'when Upload#extension has not been set' do
+    context "empty URL" do
+      before do
+        upload.update!(url: "")
+      end
+
       it 'should return the right path' do
-        upload.update!(extension: nil)
+        expect_correct_path('original/2X/4/4170ac2a2782a1516fe9e13d7322ae482c1bd594.png')
+      end
 
-        expect(FileStore::BaseStore.new.get_path_for_upload(upload))
-          .to eq('original/2X/4/4170ac2a2782a1516fe9e13d7322ae482c1bd594.png')
+      describe 'when Upload#extension has not been set' do
+        it 'should return the right path' do
+          upload.update!(extension: nil)
+          expect_correct_path('original/2X/4/4170ac2a2782a1516fe9e13d7322ae482c1bd594.png')
+        end
+      end
+
+      describe 'when id is negative' do
+        it 'should return the right depth' do
+          upload.update!(id: -999)
+          expect_correct_path('original/1X/4170ac2a2782a1516fe9e13d7322ae482c1bd594.png')
+        end
       end
     end
 
-    describe 'when id is negative' do
-      it 'should return the right depth' do
-        upload.update!(id: -999)
+    context "existing URL" do
+      context "regular site" do
+        it "returns the correct path for files stored on local storage" do
+          upload.update!(url: "/uploads/default/original/1X/63b76551662ccea1a594e161c37dd35188d77657.jpeg")
+          expect_correct_path("original/1X/63b76551662ccea1a594e161c37dd35188d77657.jpeg")
+
+          upload.update!(url: "/uploads/default/original/3X/63/63b76551662ccea1a594e161c37dd35188d77657.jpeg")
+          expect_correct_path("original/3X/63/63b76551662ccea1a594e161c37dd35188d77657.jpeg")
+        end
+
+        it "returns the correct path for files stored on S3" do
+          upload.update!(url: "//bucket-name.s3.dualstack.us-west-2.amazonaws.com/original/1X/63b76551662ccea1a594e161c37dd35188d77657.jpeg")
+          expect_correct_path("original/1X/63b76551662ccea1a594e161c37dd35188d77657.jpeg")
+
+          upload.update!(url: "//bucket-name.s3.dualstack.us-west-2.amazonaws.com/original/3X/63/63b76551662ccea1a594e161c37dd35188d77657.jpeg")
+          expect_correct_path("original/3X/63/63b76551662ccea1a594e161c37dd35188d77657.jpeg")
+        end
+      end
+
+      context "multisite" do
+        it "returns the correct path for files stored on local storage" do
+          upload.update!(url: "/uploads/foo/original/1X/63b76551662ccea1a594e161c37dd35188d77657.jpeg")

[... diff too long, it was truncated ...]

GitHub sha: 157f10db

This commit appears in #13177 which was approved by eviltrout. It was merged by gschlager.