Merge pull request #881 from flippercloud/active-record-poisoned-tran…

…saction

Active record poisoned transaction with postgres

lib/flipper/adapters/active_record.rb

@@ -1,4 +1,5 @@
 require 'set'
+require 'securerandom'
 require 'flipper'
 require 'active_record'
 
@@ -72,14 +73,15 @@ def features
       # Public: Adds a feature to the set of known features.
       def add(feature)
         with_connection(@feature_class) do
-          # race condition, but add is only used by enable/disable which happen
-          # super rarely, so it shouldn't matter in practice
-          @feature_class.transaction do
-            unless @feature_class.where(key: feature.key).exists?
-              begin
+          @feature_class.transaction(requires_new: true) do
+            begin
+              # race condition, but add is only used by enable/disable which happen
+              # super rarely, so it shouldn't matter in practice
+              unless @feature_class.where(key: feature.key).exists?
                 @feature_class.create!(key: feature.key)
-              rescue ::ActiveRecord::RecordNotUnique
               end
+            rescue ::ActiveRecord::RecordNotUnique
+              # already added
             end
           end
         end
@@ -220,7 +222,7 @@ def set(feature, gate, thing, options = {})
         raise VALUE_TO_TEXT_WARNING if json_feature && value_not_text?
 
         with_connection(@gate_class) do
-          @gate_class.transaction do
+          @gate_class.transaction(requires_new: true) do
             clear(feature) if clear_feature
             delete(feature, gate)
             begin
@@ -244,17 +246,21 @@ def delete(feature, gate)
       end
 
       def enable_multi(feature, gate, thing)
-        with_connection(@gate_class) do
-          @gate_class.create! do |g|
-            g.feature_key = feature.key
-            g.key = gate.key
-            g.value = thing.value.to_s
+        with_connection(@gate_class) do |connection|
+          begin
+            connection.transaction(requires_new: true) do
+              @gate_class.create! do |g|
+                g.feature_key = feature.key
+                g.key = gate.key
+                g.value = thing.value.to_s
+              end
+            end
+          rescue ::ActiveRecord::RecordNotUnique
+            # already added so move on with life
           end
         end
 
         nil
-      rescue ::ActiveRecord::RecordNotUnique
-        # already added so no need move on with life
       end
 
       def result_for_gates(feature, gates)

spec/flipper/adapters/active_record_spec.rb

@@ -75,6 +75,20 @@
           flipper.preload([:foo])
         end
 
+        it 'should not poison wrapping transactions' do
+          flipper = Flipper.new(subject)
+
+          actor = Struct.new(:flipper_id).new('flipper-id-123')
+          flipper.enable_actor(:foo, actor)
+
+          ActiveRecord::Base.transaction do
+            flipper.enable_actor(:foo, actor)
+            # any read on the next line is fine, just need to ensure that
+            # poisoned transaction isn't raised
+            expect(Flipper::Adapters::ActiveRecord::Gate.count).to eq(1)
+          end
+        end
+
         context "ActiveRecord connection_pool" do
           before do
             ActiveRecord::Base.connection_handler.clear_active_connections!

spec/flipper/adapters/strict_spec.rb

@@ -29,13 +29,13 @@
 
     context "#get" do
       it "raises an error for unknown feature" do
-        expect(silence { subject.get(feature) }).to match(/Could not find feature "unknown"/)
+        expect(capture_output { subject.get(feature) }).to match(/Could not find feature "unknown"/)
       end
     end
 
     context "#get_multi" do
       it "raises an error for unknown feature" do
-        expect(silence { subject.get_multi([feature]) }).to match(/Could not find feature "unknown"/)
+        expect(capture_output { subject.get_multi([feature]) }).to match(/Could not find feature "unknown"/)
       end
     end
   end

spec/flipper/engine_spec.rb

@@ -29,7 +29,7 @@
 
   let(:config) { application.config.flipper }
 
-  subject { application.initialize! }
+  subject { SpecHelpers.silence { application.initialize! } }
 
   shared_examples 'config.strict' do
     let(:adapter) { Flipper.adapter.adapter }
@@ -233,7 +233,7 @@
     it "initializes cloud configuration" do
       stub_request(:get, /flippercloud\.io/).to_return(status: 200, body: "{}")
 
-      application.initialize!
+      silence { application.initialize! }
 
       expect(Flipper.instance).to be_a(Flipper::Cloud::DSL)
       expect(Flipper.instance.instrumenter).to be_a(Flipper::Cloud::Telemetry::Instrumenter)
@@ -263,7 +263,7 @@
       }
 
       it "configures webhook app" do
-        application.initialize!
+        silence { application.initialize! }
 
         stub = stub_request(:get, "https://www.flippercloud.io/adapter/features?exclude_gate_names=true").with({
           headers: { "flipper-cloud-token" => ENV["FLIPPER_CLOUD_TOKEN"] },
@@ -278,7 +278,7 @@
 
     context "without CLOUD_SYNC_SECRET" do
       it "does not configure webhook app" do
-        application.initialize!
+        silence { application.initialize! }
 
         post "/_flipper"
         expect(last_response.status).to eq(404)
@@ -288,7 +288,7 @@
     context "without FLIPPER_CLOUD_TOKEN" do
       it "gracefully skips configuring webhook app" do
         ENV["FLIPPER_CLOUD_TOKEN"] = nil
-        application.initialize!
+        silence { application.initialize! }
         expect(Flipper.instance).to be_a(Flipper::DSL)
 
         post "/_flipper"
@@ -324,7 +324,7 @@
     end
 
     it "enables cloud" do
-      application.initialize!
+      silence { application.initialize! }
       expect(ENV["FLIPPER_CLOUD_TOKEN"]).to eq("credentials-token")
       expect(ENV["FLIPPER_CLOUD_SYNC_SECRET"]).to eq("credentials-secret")
       expect(Flipper.instance).to be_a(Flipper::Cloud::DSL)
@@ -339,7 +339,7 @@
 
   describe "config.actor_limit" do
     let(:adapter) do
-      application.initialize!
+      silence { application.initialize! }
       Flipper.adapter.adapter.adapter
     end
 

spec/flipper/middleware/memoizer_spec.rb

@@ -285,7 +285,7 @@
     end
 
     def get(uri, params = {}, env = {}, &block)
-      silence { super(uri, params, env, &block) }
+      capture_output { super(uri, params, env, &block) }
     end
 
     include_examples 'flipper middleware'

spec/support/fail_on_output.rb

@@ -1,7 +1,7 @@
 if ENV["CI"] || ENV["FAIL_ON_OUTPUT"]
   RSpec.configure do |config|
     config.around do |example|
-      output = silence { example.run }
+      output = capture_output { example.run }
       fail "Use `silence { }` to avoid printing to STDOUT/STDERR\n#{output}" unless output.empty?
     end
   end

spec/support/spec_helpers.rb

@@ -79,11 +79,22 @@ def silence
     original_stdout = $stdout
 
     # Redirect stderr and stdout
-    output = $stderr = $stdout = StringIO.new
+    $stderr = $stdout = StringIO.new
+
+    yield
+  ensure
+    $stderr = original_stderr
+    $stdout = original_stdout
+  end
+
+  def capture_output
+    original_stderr = $stderr
+    original_stdout = $stdout
+
+    output = $stdout = $stderr = StringIO.new
 
     yield
 
-    # Return output
     output.string
   ensure
     $stderr = original_stderr