FEATURE: Add S3 etag value to uploads table (#6795)

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)
 #

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

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)

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

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

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

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

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