FEATURE: Multisite support for S3 image stores (#6689)

FEATURE: Multisite support for S3 image stores (#6689)

  • FEATURE: Multisite support for S3 image stores

  • Use File.join to concatenate all paths & fix linting on multisite/s3_store_spec.rb

From 05a4f3fb51201c1c5bf647140e19c31ba25eb971 Mon Sep 17 00:00:00 2001
From: Rishabh <rishabh.nambiar@discourse.org>
Date: Thu, 29 Nov 2018 09:41:48 +0530
Subject: [PATCH] FEATURE: Multisite support for S3 image stores (#6689)

* FEATURE: Multisite support for S3 image stores

* Use File.join to concatenate all paths & fix linting on multisite/s3_store_spec.rb

diff --git a/lib/file_store/base_store.rb b/lib/file_store/base_store.rb
index 2cd9dad..bdc4856 100644
--- a/lib/file_store/base_store.rb
+++ b/lib/file_store/base_store.rb
@@ -28,6 +28,10 @@ module FileStore
       not_implemented
     end
 
+    def upload_path
+      File.join("uploads", RailsMultisite::ConnectionManagement.current_db)
+    end
+
     def has_been_uploaded?(url)
       not_implemented
     end
diff --git a/lib/file_store/local_store.rb b/lib/file_store/local_store.rb
index 63ae443..fc24c76 100644
--- a/lib/file_store/local_store.rb
+++ b/lib/file_store/local_store.rb
@@ -32,10 +32,6 @@ module FileStore
       "#{Discourse.asset_host}#{relative_base_url}"
     end
 
-    def upload_path
-      "/uploads/#{RailsMultisite::ConnectionManagement.current_db}"
-    end
-
     def relative_base_url
       "#{Discourse.base_uri}#{upload_path}"
     end
@@ -46,7 +42,7 @@ module FileStore
 
     def download_url(upload)
       return unless upload
-      "#{relative_base_url}/#{upload.sha1}"
+      File.join(relative_base_url, upload.sha1)
     end
 
     def cdn_url(url)
@@ -68,7 +64,7 @@ module FileStore
     end
 
     def get_path_for(type, upload_id, sha, extension)
-      "#{upload_path}/#{super(type, upload_id, sha, extension)}"
+      File.join("/", upload_path, super(type, upload_id, sha, extension))
     end
 
     def copy_file(file, path)
@@ -90,7 +86,7 @@ module FileStore
     end
 
     def public_dir
-      "#{Rails.root}/public"
+      File.join(Rails.root, "public")
     end
 
     def tombstone_dir
@@ -105,15 +101,13 @@ module FileStore
     private
 
     def list_missing(model)
-      public_directory = "#{Rails.root}/public"
-
       count = 0
       model.find_each do |upload|
 
         # could be a remote image
         next unless upload.url =~ /^\/[^\/]/
 
-        path = "#{public_directory}#{upload.url}"
+        path = "#{public_dir}#{upload.url}"
         bad = true
         begin
           bad = false if File.size(path) != 0
diff --git a/lib/file_store/s3_store.rb b/lib/file_store/s3_store.rb
index 08d2600..35fc324 100644
--- a/lib/file_store/s3_store.rb
+++ b/lib/file_store/s3_store.rb
@@ -41,9 +41,12 @@ 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 RailsMultisite::ConnectionManagement.current_db != "default"
       path = @s3_helper.upload(file, path, options)
+
       # return the upload url
-      "#{absolute_base_url}/#{path}"
+      File.join(absolute_base_url, path)
     end
 
     def remove_file(url, path)
@@ -93,7 +96,7 @@ module FileStore
       return url if SiteSetting.Upload.s3_cdn_url.blank?
       schema = url[/^(https?:)?\/\//, 1]
       folder = @s3_helper.s3_bucket_folder_path.nil? ? "" : "#{@s3_helper.s3_bucket_folder_path}/"
-      url.sub("#{schema}#{absolute_base_url}/#{folder}", "#{SiteSetting.Upload.s3_cdn_url}/")
+      url.sub(File.join("#{schema}#{absolute_base_url}", folder), File.join(SiteSetting.Upload.s3_cdn_url, "/"))
     end
 
     def cache_avatar(avatar, user_id)
diff --git a/spec/multisite/s3_store_spec.rb b/spec/multisite/s3_store_spec.rb
new file mode 100644
index 0000000..e3e8542
--- /dev/null
+++ b/spec/multisite/s3_store_spec.rb
@@ -0,0 +1,42 @@
+require 'rails_helper'
+require 'file_store/s3_store'
+
+RSpec.describe 'Multisite s3 uploads', type: :multisite do
+  let(:conn) { RailsMultisite::ConnectionManagement }
+  let(:uploaded_file) { file_from_fixtures("smallest.png") }
+
+  let(:upload) do
+    Fabricate(:upload, sha1: Digest::SHA1.hexdigest(File.read(uploaded_file)))
+  end
+
+  let(:s3_client) { Aws::S3::Client.new(stub_responses: true) }
+
+  let(:s3_helper) do
+    S3Helper.new(SiteSetting.s3_upload_bucket, '', client: s3_client)
+  end
+
+  let(:store) { FileStore::S3Store.new(s3_helper) }
+
+  context 'uploading to s3' do
+    before(:each) do
+      SiteSetting.s3_upload_bucket = "some-really-cool-bucket"
+      SiteSetting.s3_access_key_id = "s3-access-key-id"
+      SiteSetting.s3_secret_access_key = "s3-secret-access-key"
+      SiteSetting.enable_s3_uploads = true
+    end
+
+    describe "#store_upload" do
+      it "returns the correct url for default and second multisite db" do
+        expect(store.store_upload(uploaded_file, upload)).to eq(
+          "//#{SiteSetting.s3_upload_bucket}.s3.dualstack.us-east-1.amazonaws.com/original/1X/c530c06cf89c410c0355d7852644a73fc3ec8c04.png"
+        )
+
+        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"
+          )
+        end
+      end
+    end
+  end
+end

GitHub

us-east-1.amazonaws.com/original/1X/

@tgxworld / @rishabhnambiar so we decided not put the default site in a subdirectory? I guess it is ok cause we are not expecting may images there but the file structure is a bit strange…

Since we are already breaking why not have default in us-east-1.amazonaws.com/default/original/1X/ or something like that?

1 Like

The end goal is to fix it properly and put it in us-east-1.amazonaws.com/upload/default/original/1X/ which is the right directory. However, this commit is the first step in making sure we put multisite uploads in the right folder first. As the upload folders for default have already been created in the root of the bucket, we’ll have to write a migration and that will be in step 2.

3 Likes

@rishabhnambiar O I forgot about the tombstone folder, can you add a spec to make sure that when a file is tombstoned, it goes into the right folder as well? default will go into whichever existing tombstone folder that has already been created and multisite will go into folder with the right directories.

Yeah I realised I missed that, already on it :+1:

This was followed-up in FIX: Ensure that multisite s3 uploads are tombstoned correctly (#6769) · discourse/discourse@cae5ba7 · GitHub sha: cae5ba73