Merge pull request #958 from flippercloud/readonly-fix

Handle cloud sync causing write on read request

Author: jnunemaker

.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:

.github/workflows/examples.yml

@@ -1,5 +1,5 @@
 name: Examples
-on: [push, pull_request]
+on: [push]
 jobs:
   test:
     if: github.repository_owner == 'flippercloud'

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

spec/flipper/adapters/active_record_spec.rb

@@ -94,78 +94,78 @@
 
         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
 
           context "#add / #remove / #clear" do
             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
 
           context "#get_multi" do
             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,118 @@
             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.version >= Gem::Version.new('7.1')
+          context 'with read/write roles' do
+            before do
+              skip "connected_to with roles is not supported on #{config['adapter']}" if config["adapter"] == "sqlite3"
+            end
+
+            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 +363,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