diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 00860b2c..0e771a47 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -1,9 +1,8 @@ name: CI -on: [push, pull_request] +on: [push] jobs: test: - if: github.event_name == 'push' || (github.event_name == 'pull_request' && github.event.pull_request.head.repo.full_name != github.repository) name: Test on ruby ${{ matrix.ruby }} and rails ${{ matrix.rails }} runs-on: ubuntu-latest services: diff --git a/.github/workflows/examples.yml b/.github/workflows/examples.yml index 8faf182e..ce9580fd 100644 --- a/.github/workflows/examples.yml +++ b/.github/workflows/examples.yml @@ -1,5 +1,5 @@ name: Examples -on: [push, pull_request] +on: [push] jobs: test: if: github.repository_owner == 'flippercloud' diff --git a/lib/flipper/adapters/active_record.rb b/lib/flipper/adapters/active_record.rb index 1509cd14..b0aab44f 100644 --- a/lib/flipper/adapters/active_record.rb +++ b/lib/flipper/adapters/active_record.rb @@ -41,7 +41,7 @@ def features # Public: Adds a feature to the set of known features. def add(feature) - with_connection(@feature_class) do + with_write_connection(@feature_class) do @feature_class.transaction(requires_new: true) do begin # race condition, but add is only used by enable/disable which happen @@ -60,7 +60,7 @@ def add(feature) # Public: Removes a feature from the set of known features. def remove(feature) - with_connection(@feature_class) do + with_write_connection(@feature_class) do @feature_class.transaction do @feature_class.where(key: feature.key).destroy_all clear(feature) @@ -71,7 +71,7 @@ def remove(feature) # Public: Clears the gate values for a feature. def clear(feature) - with_connection(@gate_class) { @gate_class.where(feature_key: feature.key).destroy_all } + with_write_connection(@gate_class) { @gate_class.where(feature_key: feature.key).destroy_all } true end @@ -165,9 +165,11 @@ def disable(feature, gate, thing) when :integer set(feature, gate, thing) when :json - delete(feature, gate) + with_write_connection(@gate_class) do + delete(feature, gate) + end when :set - with_connection(@gate_class) do + with_write_connection(@gate_class) do @gate_class.where(feature_key: feature.key, key: gate.key, value: thing.value).destroy_all end else @@ -190,7 +192,7 @@ def set(feature, gate, thing, options = {}) raise VALUE_TO_TEXT_WARNING if json_feature && value_not_text? - with_connection(@gate_class) do + with_write_connection(@gate_class) do @gate_class.transaction(requires_new: true) do clear(feature) if clear_feature delete(feature, gate) @@ -215,7 +217,7 @@ def delete(feature, gate) end def enable_multi(feature, gate, thing) - with_connection(@gate_class) do |connection| + with_write_connection(@gate_class) do |connection| begin connection.transaction(requires_new: true) do @gate_class.create! do |g| @@ -271,6 +273,26 @@ def with_connection(model = @feature_class, &block) model.connection_pool.with_connection(&block) end + def with_write_connection(model = @feature_class, &block) + # Use Rails' built-in method to find the class that controls the connection + # This walks up the inheritance chain to find which class called connects_to + if model.respond_to?(:connection_class_for_self) + connection_class = model.connection_class_for_self + + # Only use connected_to if this class actually has connects_to configured + # connection_class? returns true when connects_to was called on the class + if connection_class.respond_to?(:connection_class?) && connection_class.connection_class? + connection_class.connected_to(role: :writing) do + with_connection(model, &block) + end + else + with_connection(model, &block) + end + else + with_connection(model, &block) + end + end + def warned_about_value_not_text? return @warned_about_value_not_text if defined?(@warned_about_value_not_text) @warned_about_value_not_text = true diff --git a/spec/flipper/adapters/active_record_spec.rb b/spec/flipper/adapters/active_record_spec.rb index a1d11255..c5be3c72 100644 --- a/spec/flipper/adapters/active_record_spec.rb +++ b/spec/flipper/adapters/active_record_spec.rb @@ -94,36 +94,36 @@ context "ActiveRecord connection_pool" do before do - ActiveRecord::Base.connection_handler.clear_active_connections! + clear_active_connections! end context "#features" do it "does not hold onto connections" do - expect(ActiveRecord::Base.connection_handler.active_connections?).to be(false) + expect(active_connections?).to be(false) subject.features - expect(ActiveRecord::Base.connection_handler.active_connections?).to be(false) + expect(active_connections?).to be(false) end it "does not release previously held connection" do ActiveRecord::Base.connection # establish a new connection - expect(ActiveRecord::Base.connection_handler.active_connections?).to be(true) + expect(active_connections?).to be(true) subject.features - expect(ActiveRecord::Base.connection_handler.active_connections?).to be(true) + expect(active_connections?).to be(true) end end context "#get_all" do it "does not hold onto connections" do - expect(ActiveRecord::Base.connection_handler.active_connections?).to be(false) + expect(active_connections?).to be(false) subject.get_all - expect(ActiveRecord::Base.connection_handler.active_connections?).to be(false) + expect(active_connections?).to be(false) end it "does not release previously held connection" do ActiveRecord::Base.connection # establish a new connection - expect(ActiveRecord::Base.connection_handler.active_connections?).to be(true) + expect(active_connections?).to be(true) subject.get_all - expect(ActiveRecord::Base.connection_handler.active_connections?).to be(true) + expect(active_connections?).to be(true) end end @@ -131,24 +131,24 @@ let(:feature) { Flipper::Feature.new(:search, subject) } it "does not hold onto connections" do - expect(ActiveRecord::Base.connection_handler.active_connections?).to be(false) + expect(active_connections?).to be(false) subject.add(feature) - expect(ActiveRecord::Base.connection_handler.active_connections?).to be(false) + expect(active_connections?).to be(false) subject.remove(feature) - expect(ActiveRecord::Base.connection_handler.active_connections?).to be(false) + expect(active_connections?).to be(false) subject.clear(feature) - expect(ActiveRecord::Base.connection_handler.active_connections?).to be(false) + expect(active_connections?).to be(false) end it "does not release previously held connection" do ActiveRecord::Base.connection # establish a new connection - expect(ActiveRecord::Base.connection_handler.active_connections?).to be(true) + expect(active_connections?).to be(true) subject.add(feature) - expect(ActiveRecord::Base.connection_handler.active_connections?).to be(true) + expect(active_connections?).to be(true) subject.remove(feature) - expect(ActiveRecord::Base.connection_handler.active_connections?).to be(true) + expect(active_connections?).to be(true) subject.clear(feature) - expect(ActiveRecord::Base.connection_handler.active_connections?).to be(true) + expect(active_connections?).to be(true) end end @@ -156,16 +156,16 @@ let(:feature) { Flipper::Feature.new(:search, subject) } it "does not hold onto connections" do - expect(ActiveRecord::Base.connection_handler.active_connections?).to be(false) + expect(active_connections?).to be(false) subject.get_multi([feature]) - expect(ActiveRecord::Base.connection_handler.active_connections?).to be(false) + expect(active_connections?).to be(false) end it "does not release previously held connection" do ActiveRecord::Base.connection # establish a new connection - expect(ActiveRecord::Base.connection_handler.active_connections?).to be(true) + expect(active_connections?).to be(true) subject.get_multi([feature]) - expect(ActiveRecord::Base.connection_handler.active_connections?).to be(true) + expect(active_connections?).to be(true) end end @@ -174,20 +174,20 @@ let(:gate) { feature.gate(:boolean)} it "does not hold onto connections" do - expect(ActiveRecord::Base.connection_handler.active_connections?).to be(false) + expect(active_connections?).to be(false) subject.enable(feature, gate, gate.wrap(true)) - expect(ActiveRecord::Base.connection_handler.active_connections?).to be(false) + expect(active_connections?).to be(false) subject.disable(feature, gate, gate.wrap(false)) - expect(ActiveRecord::Base.connection_handler.active_connections?).to be(false) + expect(active_connections?).to be(false) end it "does not release previously held connection" do ActiveRecord::Base.connection # establish a new connection - expect(ActiveRecord::Base.connection_handler.active_connections?).to be(true) + expect(active_connections?).to be(true) subject.enable(feature, gate, gate.wrap(true)) - expect(ActiveRecord::Base.connection_handler.active_connections?).to be(true) + expect(active_connections?).to be(true) subject.disable(feature, gate, gate.wrap(false)) - expect(ActiveRecord::Base.connection_handler.active_connections?).to be(true) + expect(active_connections?).to be(true) end end @@ -196,20 +196,120 @@ let(:gate) { feature.gate(:group) } it "does not hold onto connections" do - expect(ActiveRecord::Base.connection_handler.active_connections?).to be(false) + expect(active_connections?).to be(false) subject.enable(feature, gate, gate.wrap(:admin)) - expect(ActiveRecord::Base.connection_handler.active_connections?).to be(false) + expect(active_connections?).to be(false) subject.disable(feature, gate, gate.wrap(:admin)) - expect(ActiveRecord::Base.connection_handler.active_connections?).to be(false) + expect(active_connections?).to be(false) end it "does not release previously held connection" do ActiveRecord::Base.connection # establish a new connection - expect(ActiveRecord::Base.connection_handler.active_connections?).to be(true) + expect(active_connections?).to be(true) subject.enable(feature, gate, gate.wrap(:admin)) - expect(ActiveRecord::Base.connection_handler.active_connections?).to be(true) + expect(active_connections?).to be(true) subject.disable(feature, gate, gate.wrap(:admin)) - expect(ActiveRecord::Base.connection_handler.active_connections?).to be(true) + expect(active_connections?).to be(true) + end + end + end + + if ActiveRecord::Base.respond_to?(:connected_to) && ActiveRecord.version >= Gem::Version.new('7.1') + context 'with read/write roles' do + # Skip for older Rails versions that don't support connected_to with roles + + + # Skip for SQLite as it doesn't handle role-based connections well with :memory: databases + next if config["adapter"] == "sqlite3" + + let(:abstract_class) do + # Create a named abstract class (Rails requires names for connects_to) + klass = Class.new(ActiveRecord::Base) do + self.abstract_class = true + end + stub_const('TestApplicationRecord', klass) + + # Now configure connects_to with the same database for both roles + # In production, these would be different (primary/replica) + klass.connects_to database: { + writing: config, + reading: config + } + + klass + end + + after do + # Disconnect role-based connections to avoid interfering with database cleanup + clear_all_connections! + end + + let(:feature_class) do + klass = Class.new(abstract_class) do + self.table_name = 'flipper_features' + validates :key, presence: true + end + stub_const('TestFeature', klass) + klass + end + + let(:gate_class) do + klass = Class.new(abstract_class) do + self.table_name = 'flipper_gates' + end + stub_const('TestGate', klass) + klass + end + + let(:adapter_with_roles) do + described_class.new( + feature_class: feature_class, + gate_class: gate_class + ) + end + + it 'can perform write operations when forced to reading role' do + abstract_class.connected_to(role: :reading) do + flipper = Flipper.new(adapter_with_roles) + + feature = flipper[:test_feature] + expect { feature.enable }.not_to raise_error + expect(feature.enabled?).to be(true) + expect { feature.disable }.not_to raise_error + expect(feature.enabled?).to be(false) + + feature = flipper[:actor_test] + actor = Struct.new(:flipper_id).new(123) + expect { feature.enable_actor(actor) }.not_to raise_error + expect(feature.enabled?(actor)).to be(true) + expect { feature.disable_actor(actor) }.not_to raise_error + expect(feature.enabled?(actor)).to be(false) + + feature = flipper[:gate_test] + expect { feature.enable_percentage_of_time(50) }.not_to raise_error + expect { feature.disable_percentage_of_time }.not_to raise_error + feature.enable + expect { feature.remove }.not_to raise_error + + feature = flipper[:expression_test] + expression = Flipper.property(:plan).eq("premium") + expect { feature.enable_expression(expression) }.not_to raise_error + expect(feature.expression).to eq(expression) + expect { feature.disable_expression }.not_to raise_error + expect(feature.expression).to be_nil + end + end + + it 'does not hold onto connections during write operations' do + clear_active_connections! + + abstract_class.connected_to(role: :reading) do + flipper = Flipper.new(adapter_with_roles) + feature = flipper[:connection_test] + + feature.enable + expect(active_connections?).to be(false) + end end end end @@ -265,4 +365,19 @@ end end end + + def active_connections? + method = ActiveRecord::Base.connection_handler.method(:active_connections?) + method.arity == 0 ? method.call : method.call(:all) + end + + def clear_active_connections! + method = ActiveRecord::Base.connection_handler.method(:clear_active_connections!) + method.arity == 0 ? method.call : method.call(:all) + end + + def clear_all_connections! + method = ActiveRecord::Base.connection_handler.method(:clear_all_connections!) + method.arity == 0 ? method.call : method.call(:all) + end end