From 9ba8bfb1aaa46e97874c2d262d561a27750ae1ee Mon Sep 17 00:00:00 2001
From: Robin Ward <robin.ward@gmail.com>
Date: Wed, 9 Jan 2019 15:13:02 -0500
Subject: [PATCH] FIX: Multisite DB was leaving old data in test mode

This commit introduces a new helper to enable transactional fixtures
when testing multisite. This would show up as tests that passed the
first time then failed the second time due to stale data being leftover.
---
 .../shared_examples_for_backup_store.rb       | 16 +++++++--------
 spec/multisite/distributed_cache_spec.rb      |  4 +---
 spec/multisite/s3_store_spec.rb               | 20 ++++++++++---------
 spec/multisite/site_settings_spec.rb          | 14 ++++---------
 spec/rails_helper.rb                          | 16 +++++++++++++++
 5 files changed, 40 insertions(+), 30 deletions(-)

diff --git a/spec/lib/backup_restore/shared_examples_for_backup_store.rb b/spec/lib/backup_restore/shared_examples_for_backup_store.rb
index 921139a2c87..2c2770c73c3 100644
--- a/spec/lib/backup_restore/shared_examples_for_backup_store.rb
+++ b/spec/lib/backup_restore/shared_examples_for_backup_store.rb
@@ -57,7 +57,7 @@ shared_examples "backup store" do
       end
 
       it "works with multisite", type: :multisite do
-        RailsMultisite::ConnectionManagement.with_connection("second") do
+        test_multisite_connection("second") do
           expect(store.files).to eq([backup5, backup4])
         end
       end
@@ -74,7 +74,7 @@ shared_examples "backup store" do
       end
 
       it "works with multisite", type: :multisite do
-        RailsMultisite::ConnectionManagement.with_connection("second") do
+        test_multisite_connection("second") do
           expect(store.latest_file).to eq(backup5)
         end
       end
@@ -110,7 +110,7 @@ shared_examples "backup store" do
       it "works with multisite", type: :multisite do
         SiteSetting.maximum_backups = 1
 
-        RailsMultisite::ConnectionManagement.with_connection("second") do
+        test_multisite_connection("second") do
           store.delete_old
           expect(store.files).to eq([backup5])
         end
@@ -132,7 +132,7 @@ shared_examples "backup store" do
       end
 
       it "works with multisite", type: :multisite do
-        RailsMultisite::ConnectionManagement.with_connection("second") do
+        test_multisite_connection("second") do
           file = store.file(backup4.filename, include_download_source: true)
           expect(file.source).to match(source_regex("second", backup4.filename, multisite: true))
         end
@@ -153,7 +153,7 @@ shared_examples "backup store" do
       end
 
       it "works with multisite", type: :multisite do
-        RailsMultisite::ConnectionManagement.with_connection("second") do
+        test_multisite_connection("second") do
           expect(store.files).to include(backup5)
           store.delete_file(backup5.filename)
           expect(store.files).to_not include(backup5)
@@ -182,7 +182,7 @@ shared_examples "backup store" do
       end
 
       it "works with multisite", type: :multisite do
-        RailsMultisite::ConnectionManagement.with_connection("second") do
+        test_multisite_connection("second") do
           expect(store.files).to include(backup5)
           store.delete_file(backup5.filename)
           expect(store.files).to_not include(backup5)
@@ -239,7 +239,7 @@ shared_examples "remote backup store" do
       end
 
       it "works with multisite", type: :multisite do
-        RailsMultisite::ConnectionManagement.with_connection("second") do
+        test_multisite_connection("second") do
           upload_file
         end
       end
@@ -266,7 +266,7 @@ shared_examples "remote backup store" do
       end
 
       it "works with multisite", type: :multisite do
-        RailsMultisite::ConnectionManagement.with_connection("second") do
+        test_multisite_connection("second") do
           filename = "foo.tar.gz"
           url = store.generate_upload_url(filename)
 
diff --git a/spec/multisite/distributed_cache_spec.rb b/spec/multisite/distributed_cache_spec.rb
index cdd25dbfaf8..7306fc20ec7 100644
--- a/spec/multisite/distributed_cache_spec.rb
+++ b/spec/multisite/distributed_cache_spec.rb
@@ -1,8 +1,6 @@
 require 'rails_helper'
 
 RSpec.describe 'Multisite SiteSettings', type: :multisite do
-  let(:conn) { RailsMultisite::ConnectionManagement }
-
   def cache(name, namespace: true)
     DistributedCache.new(name, namespace: namespace)
   end
@@ -15,7 +13,7 @@ RSpec.describe 'Multisite SiteSettings', type: :multisite do
 
       expect(cache1.hash).to eq('default' => true)
 
-      conn.with_connection('second') do
+      test_multisite_connection('second') do
         message = MessageBus.track_publish(DistributedCache::Manager::CHANNEL_NAME) do
           cache1['second'] = true
         end.first
diff --git a/spec/multisite/s3_store_spec.rb b/spec/multisite/s3_store_spec.rb
index 4ac8527eb6b..7939f32c9f6 100644
--- a/spec/multisite/s3_store_spec.rb
+++ b/spec/multisite/s3_store_spec.rb
@@ -2,9 +2,9 @@ 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) { Fabricate(:upload, sha1: Digest::SHA1.hexdigest(File.read(uploaded_file))) }
+  let(:upload_sha1) { Digest::SHA1.hexdigest(File.read(uploaded_file)) }
+  let(:upload) { Fabricate(:upload, sha1: upload_sha1) }
 
   context 'uploading to s3' do
     before(:each) do
@@ -20,15 +20,17 @@ RSpec.describe 'Multisite s3 uploads', type: :multisite do
       let(:store) { FileStore::S3Store.new(s3_helper) }
 
       it "returns the correct url for default and second multisite db" do
-        conn.with_connection('default') do
-          expect(store.store_upload(uploaded_file, upload)).to eq(
+        test_multisite_connection('default') do
+          test_upload = Fabricate(:upload, sha1: upload_sha1)
+          expect(store.store_upload(uploaded_file, test_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(
+        test_multisite_connection('second') do
+          test_upload = Fabricate(:upload, sha1: upload_sha1)
+          expect(store.store_upload(uploaded_file, test_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")
@@ -54,7 +56,7 @@ RSpec.describe 'Multisite s3 uploads', type: :multisite do
       let(:s3_helper) { store.s3_helper }
 
       it "removes the file from s3 on multisite" do
-        conn.with_connection('default') do
+        test_multisite_connection('default') do
           store.expects(:get_depth_for).with(upload.id).returns(0)
           s3_helper.expects(:s3_bucket).returns(s3_bucket).at_least_once
           upload.update_attributes!(url: "//s3-upload-bucket.s3.dualstack.us-west-1.amazonaws.com/uploads/default/original/1X/#{upload.sha1}.png")
@@ -70,7 +72,7 @@ RSpec.describe 'Multisite s3 uploads', type: :multisite do
       end
 
       it "removes the file from s3 on another multisite db" do
-        conn.with_connection('second') do
+        test_multisite_connection('second') do
           store.expects(:get_depth_for).with(upload.id).returns(0)
           s3_helper.expects(:s3_bucket).returns(s3_bucket).at_least_once
           upload.update_attributes!(url: "//s3-upload-bucket.s3.dualstack.us-west-1.amazonaws.com/uploads/second/original/1X/#{upload.sha1}.png")
@@ -91,7 +93,7 @@ RSpec.describe 'Multisite s3 uploads', type: :multisite do
         end
 
         it "removes the file from s3 on multisite" do
-          conn.with_connection('default') do
+          test_multisite_connection('default') do
             store.expects(:get_depth_for).with(upload.id).returns(0)
             s3_helper.expects(:s3_bucket).returns(s3_bucket).at_least_once
             upload.update_attributes!(url: "//s3-upload-bucket.s3.dualstack.us-west-1.amazonaws.com/discourse-uploads/uploads/default/original/1X/#{upload.sha1}.png")
diff --git a/spec/multisite/site_settings_spec.rb b/spec/multisite/site_settings_spec.rb
index 5a0d7b4719f..a01182e3b59 100644
--- a/spec/multisite/site_settings_spec.rb
+++ b/spec/multisite/site_settings_spec.rb
@@ -1,34 +1,28 @@
 require 'rails_helper'
 
 RSpec.describe 'Multisite SiteSettings', type: :multisite do
-  let(:conn) { RailsMultisite::ConnectionManagement }
-
   before do
     @original_provider = SiteSetting.provider
     SiteSetting.provider = SiteSettings::DbProvider.new(SiteSetting)
   end
 
   after do
-    ['default', 'second'].each do |db|
-      conn.with_connection(db) { SiteSetting.where(name: 'default_locale').destroy_all }
-    end
-
     SiteSetting.provider = @original_provider
   end
 
   describe '#default_locale' do
     it 'should return the right locale' do
-      conn.with_connection('default') do
+      test_multisite_connection('default') do
         expect(SiteSetting.default_locale).to eq('en')
       end
 
-      conn.with_connection('second') do
+      test_multisite_connection('second') do
         SiteSetting.default_locale = 'zh_TW'
 
         expect(SiteSetting.default_locale).to eq('zh_TW')
       end
 
-      conn.with_connection('default') do
+      test_multisite_connection('default') do
         expect(SiteSetting.default_locale).to eq('en')
 
         SiteSetting.default_locale = 'ja'
@@ -36,7 +30,7 @@ RSpec.describe 'Multisite SiteSettings', type: :multisite do
         expect(SiteSetting.default_locale).to eq('ja')
       end
 
-      conn.with_connection('second') do
+      test_multisite_connection('second') do
         expect(SiteSetting.default_locale).to eq('zh_TW')
       end
     end
diff --git a/spec/rails_helper.rb b/spec/rails_helper.rb
index c820d2f4b21..a0fad312fd6 100644
--- a/spec/rails_helper.rb
+++ b/spec/rails_helper.rb
@@ -202,6 +202,22 @@ RSpec.configure do |config|
     end
   end
 
+  # Normally we `use_transactional_fixtures` to clear out a database after a test
+  # runs. However, this does not apply to tests done for multisite. The second time
+  # a test runs you can end up with stale data that breaks things. This method will
+  # force a rollback after using a multisite connection.
+  def test_multisite_connection(name)
+    RailsMultisite::ConnectionManagement.with_connection(name) do
+      ActiveRecord::Base.transaction do
+        begin
+          yield
+        ensure
+          throw raise ActiveRecord::Rollback
+        end
+      end
+    end
+  end
+
 end
 
 class TrackTimeStub