diff --git a/lib/flipper/adapters/active_record.rb b/lib/flipper/adapters/active_record.rb index 5b869bdc..04cc6f3a 100644 --- a/lib/flipper/adapters/active_record.rb +++ b/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) diff --git a/spec/flipper/adapters/active_record_spec.rb b/spec/flipper/adapters/active_record_spec.rb index 24f2885a..90573945 100644 --- a/spec/flipper/adapters/active_record_spec.rb +++ b/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! diff --git a/spec/flipper/adapters/strict_spec.rb b/spec/flipper/adapters/strict_spec.rb index 9df2f444..0b0bccb4 100644 --- a/spec/flipper/adapters/strict_spec.rb +++ b/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 diff --git a/spec/flipper/engine_spec.rb b/spec/flipper/engine_spec.rb index d8de2b9f..efad364b 100644 --- a/spec/flipper/engine_spec.rb +++ b/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 diff --git a/spec/flipper/middleware/memoizer_spec.rb b/spec/flipper/middleware/memoizer_spec.rb index 06545165..1a409c67 100644 --- a/spec/flipper/middleware/memoizer_spec.rb +++ b/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' diff --git a/spec/support/fail_on_output.rb b/spec/support/fail_on_output.rb index a149845c..3a796c0f 100644 --- a/spec/support/fail_on_output.rb +++ b/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 diff --git a/spec/support/spec_helpers.rb b/spec/support/spec_helpers.rb index 04b08bfa..e6972e87 100644 --- a/spec/support/spec_helpers.rb +++ b/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