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.
This commit is contained in:
Eugen Rochko 2019-10-09 07:10:46 +02:00 committed by GitHub
parent 538db85d3c
commit 354fdd317e
No known key found for this signature in database
GPG key ID: 4AEE18F83AFDEB23
6 changed files with 41 additions and 18 deletions

View file

@ -5,11 +5,17 @@ class Api::V1::StreamingController < Api::BaseController
def index def index
if Rails.configuration.x.streaming_api_base_url != request.host if Rails.configuration.x.streaming_api_base_url != request.host
uri = URI.parse(request.url) redirect_to streaming_api_url, status: 301
uri.host = URI.parse(Rails.configuration.x.streaming_api_base_url).host
redirect_to uri.to_s, status: 301
else else
raise ActiveRecord::RecordNotFound not_found
end end
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 end

View file

@ -312,8 +312,7 @@ class Account < ApplicationRecord
rescue ActiveRecord::RecordInvalid rescue ActiveRecord::RecordInvalid
self.avatar = nil self.avatar = nil
self.header = nil self.header = nil
self[:avatar_remote_url] = ''
self[:header_remote_url] = ''
save! save!
end end

View file

@ -18,7 +18,7 @@ module Remotable
return return
end 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 begin
Request.new(:get, url).perform do |response| Request.new(:get, url).perform do |response|

View file

@ -1,11 +1,12 @@
# frozen_string_literal: true # frozen_string_literal: true
Paperclip.options[:read_timeout] = 60
Paperclip.interpolates :filename do |attachment, style| Paperclip.interpolates :filename do |attachment, style|
return attachment.original_filename if style == :original if style == :original
attachment.original_filename
else
[basename(attachment, style), extension(attachment, style)].delete_if(&:blank?).join('.') [basename(attachment, style), extension(attachment, style)].delete_if(&:blank?).join('.')
end end
end
Paperclip::Attachment.default_options.merge!( Paperclip::Attachment.default_options.merge!(
use_timestamp: false, use_timestamp: false,
@ -24,17 +25,21 @@ if ENV['S3_ENABLED'] == 'true'
storage: :s3, storage: :s3,
s3_protocol: s3_protocol, s3_protocol: s3_protocol,
s3_host_name: s3_hostname, s3_host_name: s3_hostname,
s3_headers: { s3_headers: {
'X-Amz-Multipart-Threshold' => ENV.fetch('S3_MULTIPART_THRESHOLD') { 15.megabytes }.to_i, 'X-Amz-Multipart-Threshold' => ENV.fetch('S3_MULTIPART_THRESHOLD') { 15.megabytes }.to_i,
'Cache-Control' => 'public, max-age=315576000, immutable', 'Cache-Control' => 'public, max-age=315576000, immutable',
}, },
s3_permissions: ENV.fetch('S3_PERMISSION') { 'public-read' }, s3_permissions: ENV.fetch('S3_PERMISSION') { 'public-read' },
s3_region: s3_region, s3_region: s3_region,
s3_credentials: { s3_credentials: {
bucket: ENV['S3_BUCKET'], bucket: ENV['S3_BUCKET'],
access_key_id: ENV['AWS_ACCESS_KEY_ID'], access_key_id: ENV['AWS_ACCESS_KEY_ID'],
secret_access_key: ENV['AWS_SECRET_ACCESS_KEY'], secret_access_key: ENV['AWS_SECRET_ACCESS_KEY'],
}, },
s3_options: { s3_options: {
signature_version: ENV.fetch('S3_SIGNATURE_VERSION') { 'v4' }, signature_version: ENV.fetch('S3_SIGNATURE_VERSION') { 'v4' },
http_open_timeout: 5, http_open_timeout: 5,
@ -49,6 +54,7 @@ if ENV['S3_ENABLED'] == 'true'
endpoint: ENV['S3_ENDPOINT'], endpoint: ENV['S3_ENDPOINT'],
force_path_style: true force_path_style: true
) )
Paperclip::Attachment.default_options[:url] = ':s3_path_url' Paperclip::Attachment.default_options[:url] = ':s3_path_url'
end end
@ -74,6 +80,7 @@ elsif ENV['SWIFT_ENABLED'] == 'true'
openstack_region: ENV['SWIFT_REGION'], openstack_region: ENV['SWIFT_REGION'],
openstack_cache_ttl: ENV.fetch('SWIFT_CACHE_TTL') { 60 }, openstack_cache_ttl: ENV.fetch('SWIFT_CACHE_TTL') { 60 },
}, },
fog_directory: ENV['SWIFT_CONTAINER'], fog_directory: ENV['SWIFT_CONTAINER'],
fog_host: ENV['SWIFT_OBJECT_URL'], fog_host: ENV['SWIFT_OBJECT_URL'],
fog_public: true fog_public: true
@ -82,7 +89,7 @@ else
Paperclip::Attachment.default_options.merge!( Paperclip::Attachment.default_options.merge!(
storage: :filesystem, storage: :filesystem,
use_timestamp: true, use_timestamp: true,
path: (ENV['PAPERCLIP_ROOT_PATH'] || ':rails_root/public/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['PAPERCLIP_ROOT_URL'] || '/system') + '/:class/:attachment/:id_partition/:style/:filename', url: ENV.fetch('PAPERCLIP_ROOT_URL', '/system') + '/:class/:attachment/:id_partition/:style/:filename',
) )
end end

View file

@ -126,8 +126,8 @@ RSpec.describe Account, type: :model do
end end
it 'sets default avatar, header, avatar_remote_url, and header_remote_url' do it 'sets default avatar, header, avatar_remote_url, and header_remote_url' do
expect(account.avatar_remote_url).to eq '' expect(account.avatar_remote_url).to eq 'https://remote.test/invalid_avatar'
expect(account.header_remote_url).to eq '' expect(account.header_remote_url).to eq expectation.header_remote_url
expect(account.avatar_file_name).to eq nil expect(account.avatar_file_name).to eq nil
expect(account.header_file_name).to eq nil expect(account.header_file_name).to eq nil
end end

View file

@ -18,6 +18,8 @@ RSpec.describe Remotable do
def hoge=(arg); end def hoge=(arg); end
def hoge_file_name; end
def hoge_file_name=(arg); end def hoge_file_name=(arg); end
def has_attribute?(arg); end def has_attribute?(arg); end
@ -109,12 +111,21 @@ RSpec.describe Remotable do
end end
context 'foo[attribute_name] == url' do 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(:[]).with(attribute_name).and_return(url)
allow(foo).to receive(:hoge_file_name).and_return('foo.jpg')
foo.hoge_remote_url = url foo.hoge_remote_url = url
expect(request).not_to have_been_requested expect(request).not_to have_been_requested
end 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 end
context "scheme is https, parsed_url.host isn't empty, and foo[attribute_name] != url" do context "scheme is https, parsed_url.host isn't empty, and foo[attribute_name] != url" do