Fix attachment not being re-downloaded even if file is not stored (#12125)

Change the behaviour of remotable concern. Previously, it would skip
downloading an attachment if the stored remote URL is identical to
the new one. Now it would not be skipped if the attachment is not
actually currently stored by Paperclip.

6 files changed, 41 insertions, 18 deletions

diff --git a/app/controllers/api/v1/streaming_controller.rb b/app/controllers/api/v1/streaming_controller.rb
index 66b812e76..ebb17608c 100644
--- a/app/controllers/api/v1/streaming_controller.rb
+++ b/app/controllers/api/v1/streaming_controller.rb
@@ -5,11 +5,17 @@ class Api::V1::StreamingController < Api::BaseController
 
   def index
     if Rails.configuration.x.streaming_api_base_url != request.host
-      uri = URI.parse(request.url)
-      uri.host = URI.parse(Rails.configuration.x.streaming_api_base_url).host
-      redirect_to uri.to_s, status: 301
+      redirect_to streaming_api_url, status: 301
     else
-      raise ActiveRecord::RecordNotFound
+      not_found
     end
   end
+
+  private
+
+  def streaming_api_url
+    Addressable::URI.parse(request.url).tap do |uri|
+      uri.host = Addressable::URI.parse(Rails.configuration.x.streaming_api_base_url).host
+    end.to_s
+  end
 end
diff --git a/app/models/account.rb b/app/models/account.rb
index 01d45e36c..2f43f337f 100644
--- a/app/models/account.rb
+++ b/app/models/account.rb
@@ -310,10 +310,9 @@ class Account < ApplicationRecord
   def save_with_optional_media!
     save!
   rescue ActiveRecord::RecordInvalid
-    self.avatar              = nil
-    self.header              = nil
-    self[:avatar_remote_url] = ''
-    self[:header_remote_url] = ''
+    self.avatar = nil
+    self.header = nil
+
     save!
   end
 
diff --git a/app/models/concerns/remotable.rb b/app/models/concerns/remotable.rb
index 082302619..b7a476c87 100644
--- a/app/models/concerns/remotable.rb
+++ b/app/models/concerns/remotable.rb
@@ -18,7 +18,7 @@ module Remotable
           return
         end
 
-        return if !%w(http https).include?(parsed_url.scheme) || parsed_url.host.blank? || self[attribute_name] == url
+        return if !%w(http https).include?(parsed_url.scheme) || parsed_url.host.blank? || (self[attribute_name] == url && send("#{attachment_name}_file_name").present?)
 
         begin
           Request.new(:get, url).perform do |response|
diff --git a/config/initializers/paperclip.rb b/config/initializers/paperclip.rb
index a0253f4bc..d3602e655 100644
--- a/config/initializers/paperclip.rb
+++ b/config/initializers/paperclip.rb
@@ -1,10 +1,11 @@
 # frozen_string_literal: true
 
-Paperclip.options[:read_timeout] = 60
-
 Paperclip.interpolates :filename do |attachment, style|
-  return attachment.original_filename if style == :original
-  [basename(attachment, style), extension(attachment, style)].delete_if(&:blank?).join('.')
+  if style == :original
+    attachment.original_filename
+  else
+    [basename(attachment, style), extension(attachment, style)].delete_if(&:blank?).join('.')
+  end
 end
 
 Paperclip::Attachment.default_options.merge!(
@@ -24,17 +25,21 @@ if ENV['S3_ENABLED'] == 'true'
     storage: :s3,
     s3_protocol: s3_protocol,
     s3_host_name: s3_hostname,
+
     s3_headers: {
       'X-Amz-Multipart-Threshold' => ENV.fetch('S3_MULTIPART_THRESHOLD') { 15.megabytes }.to_i,
       'Cache-Control' => 'public, max-age=315576000, immutable',
     },
+
     s3_permissions: ENV.fetch('S3_PERMISSION') { 'public-read' },
     s3_region: s3_region,
+
     s3_credentials: {
       bucket: ENV['S3_BUCKET'],
       access_key_id: ENV['AWS_ACCESS_KEY_ID'],
       secret_access_key: ENV['AWS_SECRET_ACCESS_KEY'],
     },
+
     s3_options: {
       signature_version: ENV.fetch('S3_SIGNATURE_VERSION') { 'v4' },
       http_open_timeout: 5,
@@ -49,6 +54,7 @@ if ENV['S3_ENABLED'] == 'true'
       endpoint: ENV['S3_ENDPOINT'],
       force_path_style: true
     )
+
     Paperclip::Attachment.default_options[:url] = ':s3_path_url'
   end
 
@@ -74,6 +80,7 @@ elsif ENV['SWIFT_ENABLED'] == 'true'
       openstack_region: ENV['SWIFT_REGION'],
       openstack_cache_ttl: ENV.fetch('SWIFT_CACHE_TTL') { 60 },
     },
+
     fog_directory: ENV['SWIFT_CONTAINER'],
     fog_host: ENV['SWIFT_OBJECT_URL'],
     fog_public: true
@@ -82,7 +89,7 @@ else
   Paperclip::Attachment.default_options.merge!(
     storage: :filesystem,
     use_timestamp: true,
-    path: (ENV['PAPERCLIP_ROOT_PATH'] || ':rails_root/public/system') + '/:class/:attachment/:id_partition/:style/:filename',
-    url: (ENV['PAPERCLIP_ROOT_URL'] || '/system') + '/:class/:attachment/:id_partition/:style/:filename',
+    path: ENV.fetch('PAPERCLIP_ROOT_PATH', ':rails_root/public/system') + '/:class/:attachment/:id_partition/:style/:filename',
+    url: ENV.fetch('PAPERCLIP_ROOT_URL', '/system') + '/:class/:attachment/:id_partition/:style/:filename',
   )
 end
diff --git a/spec/models/account_spec.rb b/spec/models/account_spec.rb
index 3eec464bd..b2f6234cb 100644
--- a/spec/models/account_spec.rb
+++ b/spec/models/account_spec.rb
@@ -126,8 +126,8 @@ RSpec.describe Account, type: :model do
       end
 
       it 'sets default avatar, header, avatar_remote_url, and header_remote_url' do
-        expect(account.avatar_remote_url).to eq ''
-        expect(account.header_remote_url).to eq ''
+        expect(account.avatar_remote_url).to eq 'https://remote.test/invalid_avatar'
+        expect(account.header_remote_url).to eq expectation.header_remote_url
         expect(account.avatar_file_name).to  eq nil
         expect(account.header_file_name).to  eq nil
       end
diff --git a/spec/models/concerns/remotable_spec.rb b/spec/models/concerns/remotable_spec.rb
index a4289cc45..99a60cbf6 100644
--- a/spec/models/concerns/remotable_spec.rb
+++ b/spec/models/concerns/remotable_spec.rb
@@ -18,6 +18,8 @@ RSpec.describe Remotable do
 
     def hoge=(arg); end
 
+    def hoge_file_name; end
+
     def hoge_file_name=(arg); end
 
     def has_attribute?(arg); end
@@ -109,12 +111,21 @@ RSpec.describe Remotable do
       end
 
       context 'foo[attribute_name] == url' do
-        it 'makes no request' do
+        it 'makes no request if file is saved' do
           allow(foo).to receive(:[]).with(attribute_name).and_return(url)
+          allow(foo).to receive(:hoge_file_name).and_return('foo.jpg')
 
           foo.hoge_remote_url = url
           expect(request).not_to have_been_requested
         end
+
+        it 'makes request if file is not saved' do
+          allow(foo).to receive(:[]).with(attribute_name).and_return(url)
+          allow(foo).to receive(:hoge_file_name).and_return(nil)
+
+          foo.hoge_remote_url = url
+          expect(request).to have_been_requested
+        end
       end
 
       context "scheme is https, parsed_url.host isn't empty, and foo[attribute_name] != url" do