From 75dbb98cca14e3cfcf54035d64ebd603ff64475b Mon Sep 17 00:00:00 2001 From: Vinoth Kannan Date: Fri, 4 Jan 2019 11:46:22 +0530 Subject: [PATCH] FEATURE: Add S3 etag value to uploads table (#6795) --- app/models/upload.rb | 2 ++ .../20181218071253_add_etag_to_uploads.rb | 9 ++++++ lib/file_store/s3_store.rb | 15 ++++++---- lib/s3_helper.rb | 23 ++++++++++++-- lib/upload_creator.rb | 3 +- spec/components/file_store/s3_store_spec.rb | 18 ++++++++--- spec/lib/upload_creator_spec.rb | 30 +++++++++++++++++++ spec/multisite/s3_store_spec.rb | 2 ++ 8 files changed, 88 insertions(+), 14 deletions(-) create mode 100644 db/migrate/20181218071253_add_etag_to_uploads.rb diff --git a/app/models/upload.rb b/app/models/upload.rb index a90eb4c14ef..c3fd30203d0 100644 --- a/app/models/upload.rb +++ b/app/models/upload.rb @@ -289,6 +289,7 @@ end # extension :string(10) # thumbnail_width :integer # thumbnail_height :integer +# etag :string # # Indexes # @@ -297,4 +298,5 @@ end # index_uploads_on_sha1 (sha1) UNIQUE # index_uploads_on_url (url) # index_uploads_on_user_id (user_id) +# index_uploads_on_etag (etag) # diff --git a/db/migrate/20181218071253_add_etag_to_uploads.rb b/db/migrate/20181218071253_add_etag_to_uploads.rb new file mode 100644 index 00000000000..dacc0957c1c --- /dev/null +++ b/db/migrate/20181218071253_add_etag_to_uploads.rb @@ -0,0 +1,9 @@ +class AddEtagToUploads < ActiveRecord::Migration[5.2] + def change + add_column :uploads, :etag, :string + add_index :uploads, [:etag] + + add_column :optimized_images, :etag, :string + add_index :optimized_images, [:etag] + end +end diff --git a/lib/file_store/s3_store.rb b/lib/file_store/s3_store.rb index da309f34006..119dd9c0a1c 100644 --- a/lib/file_store/s3_store.rb +++ b/lib/file_store/s3_store.rb @@ -19,12 +19,14 @@ module FileStore def store_upload(file, upload, content_type = nil) path = get_path_for_upload(upload) - store_file(file, path, filename: upload.original_filename, content_type: content_type, cache_locally: true) + url, upload.etag = store_file(file, path, filename: upload.original_filename, content_type: content_type, cache_locally: true) + url end def store_optimized_image(file, optimized_image, content_type = nil) path = get_path_for_optimized_image(optimized_image) - store_file(file, path, content_type: content_type) + url, optimized_image.etag = store_file(file, path, content_type: content_type) + url end # options @@ -42,13 +44,14 @@ module FileStore } # add a "content disposition" header for "attachments" options[:content_disposition] = "attachment; filename=\"#{filename}\"" unless FileHelper.is_supported_image?(filename) - # if this fails, it will throw an exception path.prepend(File.join(upload_path, "/")) if Rails.configuration.multisite - path = @s3_helper.upload(file, path, options) - # return the upload url - File.join(absolute_base_url, path) + # if this fails, it will throw an exception + path, etag = @s3_helper.upload(file, path, options) + + # return the upload url and etag + return File.join(absolute_base_url, path), etag end def remove_file(url, path) diff --git a/lib/s3_helper.rb b/lib/s3_helper.rb index 708c2321334..6741debf6ab 100644 --- a/lib/s3_helper.rb +++ b/lib/s3_helper.rb @@ -24,8 +24,21 @@ class S3Helper def upload(file, path, options = {}) path = get_path_for_s3_upload(path) - s3_bucket.object(path).upload_file(file, options) - path + obj = s3_bucket.object(path) + + etag = begin + if File.size(file) >= Aws::S3::FileUploader::FIFTEEN_MEGABYTES + options[:multipart_threshold] = Aws::S3::FileUploader::FIFTEEN_MEGABYTES + obj.upload_file(file, options) + obj.load + obj.etag + else + options[:body] = file + obj.put(options).etag + end + end + + return path, etag end def remove(s3_filename, copy_to_tombstone = false) @@ -210,8 +223,12 @@ class S3Helper File.join("uploads", RailsMultisite::ConnectionManagement.current_db, "/") end + def s3_client + Aws::S3::Client.new(@s3_options) + end + def s3_resource - Aws::S3::Resource.new(@s3_options) + Aws::S3::Resource.new(client: s3_client) end def s3_bucket diff --git a/lib/upload_creator.rb b/lib/upload_creator.rb index 35fa225e230..2d7cc9166ca 100644 --- a/lib/upload_creator.rb +++ b/lib/upload_creator.rb @@ -126,7 +126,8 @@ class UploadCreator url = Discourse.store.store_upload(f, @upload) if url.present? - @upload.update!(url: url) + @upload.url = url + @upload.save! else @upload.errors.add(:url, I18n.t("upload.store_failure", upload_id: @upload.id, user_id: user_id)) end diff --git a/spec/components/file_store/s3_store_spec.rb b/spec/components/file_store/s3_store_spec.rb index 993e3d315c1..21f77761c75 100644 --- a/spec/components/file_store/s3_store_spec.rb +++ b/spec/components/file_store/s3_store_spec.rb @@ -38,6 +38,8 @@ describe FileStore::S3Store do context 'uploading to s3' do include_context "s3 helpers" + let(:etag) { "etag" } + describe "#store_upload" do it "returns an absolute schemaless url" do store.expects(:get_depth_for).with(upload.id).returns(0) @@ -45,11 +47,13 @@ describe FileStore::S3Store do s3_object = stub s3_bucket.expects(:object).with("original/1X/#{upload.sha1}.png").returns(s3_object) - s3_object.expects(:upload_file) + + s3_object.stubs(:put).returns(Aws::S3::Types::PutObjectOutput.new(etag: etag)) expect(store.store_upload(uploaded_file, upload)).to eq( "//s3-upload-bucket.s3.dualstack.us-west-1.amazonaws.com/original/1X/#{upload.sha1}.png" ) + expect(upload.etag).to eq(etag) end describe "when s3_upload_bucket includes folders path" do @@ -63,11 +67,13 @@ describe FileStore::S3Store do s3_object = stub s3_bucket.expects(:object).with("discourse-uploads/original/1X/#{upload.sha1}.png").returns(s3_object) - s3_object.expects(:upload_file) + + s3_object.stubs(:put).returns(Aws::S3::Types::PutObjectOutput.new(etag: etag)) expect(store.store_upload(uploaded_file, upload)).to eq( "//s3-upload-bucket.s3.dualstack.us-west-1.amazonaws.com/discourse-uploads/original/1X/#{upload.sha1}.png" ) + expect(upload.etag).to eq(etag) end end end @@ -80,11 +86,13 @@ describe FileStore::S3Store do path = "optimized/1X/#{optimized_image.upload.sha1}_#{OptimizedImage::VERSION}_100x200.png" s3_bucket.expects(:object).with(path).returns(s3_object) - s3_object.expects(:upload_file) + + s3_object.stubs(:put).returns(Aws::S3::Types::PutObjectOutput.new(etag: etag)) expect(store.store_optimized_image(optimized_image_file, optimized_image)).to eq( "//s3-upload-bucket.s3.dualstack.us-west-1.amazonaws.com/#{path}" ) + expect(optimized_image.etag).to eq(etag) end describe "when s3_upload_bucket includes folders path" do @@ -99,11 +107,13 @@ describe FileStore::S3Store do path = "discourse-uploads/optimized/1X/#{optimized_image.upload.sha1}_#{OptimizedImage::VERSION}_100x200.png" s3_bucket.expects(:object).with(path).returns(s3_object) - s3_object.expects(:upload_file) + + s3_object.stubs(:put).returns(Aws::S3::Types::PutObjectOutput.new(etag: etag)) expect(store.store_optimized_image(optimized_image_file, optimized_image)).to eq( "//s3-upload-bucket.s3.dualstack.us-west-1.amazonaws.com/#{path}" ) + expect(optimized_image.etag).to eq(etag) end end end diff --git a/spec/lib/upload_creator_spec.rb b/spec/lib/upload_creator_spec.rb index 897c946aeea..0c2120deeb8 100644 --- a/spec/lib/upload_creator_spec.rb +++ b/spec/lib/upload_creator_spec.rb @@ -1,4 +1,5 @@ require 'rails_helper' +require 'file_store/s3_store' RSpec.describe UploadCreator do let(:user) { Fabricate(:user) } @@ -166,5 +167,34 @@ RSpec.describe UploadCreator do expect(upload.original_filename).to eq('should_be_jpeg.jpg') end end + + describe 'uploading to s3' do + let(:filename) { "should_be_jpeg.png" } + let(:file) { file_from_fixtures(filename) } + + before do + SiteSetting.s3_upload_bucket = "s3-upload-bucket" + SiteSetting.s3_access_key_id = "s3-access-key-id" + SiteSetting.s3_secret_access_key = "s3-secret-access-key" + SiteSetting.s3_region = 'us-west-1' + SiteSetting.enable_s3_uploads = true + + store = FileStore::S3Store.new + s3_helper = store.instance_variable_get(:@s3_helper) + client = Aws::S3::Client.new(stub_responses: true) + s3_helper.stubs(:s3_client).returns(client) + Discourse.stubs(:store).returns(store) + end + + it 'should store the file and return etag' do + expect { + UploadCreator.new(file, filename).create_for(user.id) + }.to change { Upload.count }.by(1) + + upload = Upload.last + + expect(upload.etag).to eq('ETag') + end + end end end diff --git a/spec/multisite/s3_store_spec.rb b/spec/multisite/s3_store_spec.rb index 6c023d15b69..4ac8527eb6b 100644 --- a/spec/multisite/s3_store_spec.rb +++ b/spec/multisite/s3_store_spec.rb @@ -24,12 +24,14 @@ RSpec.describe 'Multisite s3 uploads', type: :multisite do expect(store.store_upload(uploaded_file, upload)).to eq( "//#{SiteSetting.s3_upload_bucket}.s3.dualstack.us-east-1.amazonaws.com/uploads/default/original/1X/c530c06cf89c410c0355d7852644a73fc3ec8c04.png" ) + expect(upload.etag).to eq("ETag") end conn.with_connection('second') do expect(store.store_upload(uploaded_file, upload)).to eq( "//#{SiteSetting.s3_upload_bucket}.s3.dualstack.us-east-1.amazonaws.com/uploads/second/original/1X/c530c06cf89c410c0355d7852644a73fc3ec8c04.png" ) + expect(upload.etag).to eq("ETag") end end end