Merge pull request #763 from flippercloud/adapter_builder

Configure Strict adapter, introduce AdapterBuilder

Changelog.md

@@ -14,6 +14,14 @@ All notable changes to this project will be documented in this file.
     config.flipper.preload = ->(request) { !request.path.start_with?('/assets') }
   end
   ```
+* Added `strict` configuration to warn when accessing a feature that doesn't exist (https://github.com/flippercloud/flipper/pull/760, https://github.com/flippercloud/flipper/pull/763)
+  ```ruby
+  Rails.application.configure do
+    # Setting to `true` or `:raise` will raise error when a feature doesn't exist.
+    # Use `:warn` to log a warning instead.
+    config.flipper.strict = !Rails.env.production?
+  end
+  ```
 * Handle deprecation of Rack::File in Rack 3.1 (https://github.com/flippercloud/flipper/pull/773).
 * Human readable actor names in flipper-ui (https://github.com/flippercloud/flipper/pull/737).
 * Expressions are now available and considered "alpha". They are not yet documented, but you can see examples in [examples/expressions.rb](examples/expressions.rb). (https://github.com/flippercloud/flipper/pull/692)

lib/flipper.rb

@@ -178,6 +178,7 @@ def groups_registry=(registry)
 require 'flipper/adapters/memory'
 require 'flipper/adapters/strict'
 require 'flipper/adapters/instrumented'
+require 'flipper/adapter_builder'
 require 'flipper/configuration'
 require 'flipper/dsl'
 require 'flipper/errors'

lib/flipper/adapter_builder.rb

@@ -0,0 +1,44 @@
+module Flipper
+  # Builds an adapter from a stack of adapters.
+  #
+  #   adapter = Flipper::AdapterBuilder.new do
+  #     use Flipper::Adapters::Strict
+  #     use Flipper::Adapters::Memoizer
+  #     store Flipper::Adapters::Memory
+  #   end.to_adapter
+  #
+  class AdapterBuilder
+    def initialize(&block)
+      @stack = []
+
+      # Default to memory adapter
+      store Flipper::Adapters::Memory
+
+      block.arity == 0 ? instance_eval(&block) : block.call(self) if block
+    end
+
+    if RUBY_VERSION >= '3.0'
+      def use(klass, *args, **kwargs, &block)
+        @stack.push ->(adapter) { klass.new(adapter, *args, **kwargs, &block) }
+      end
+    else
+      def use(klass, *args, &block)
+        @stack.push ->(adapter) { klass.new(adapter, *args, &block) }
+      end
+    end
+
+    if RUBY_VERSION >= '3.0'
+      def store(adapter, *args, **kwargs, &block)
+        @store = adapter.respond_to?(:call) ? adapter : -> { adapter.new(*args, **kwargs, &block) }
+      end
+    else
+      def store(adapter, *args, &block)
+        @store = adapter.respond_to?(:call) ? adapter : -> { adapter.new(*args, &block) }
+      end
+    end
+
+    def to_adapter
+      @stack.reverse.inject(@store.call) { |adapter, wrapper| wrapper.call(adapter) }
+    end
+  end
+end

lib/flipper/configuration.rb

@@ -1,8 +1,8 @@
 module Flipper
   class Configuration
     def initialize(options = {})
-      @default = -> { Flipper.new(adapter) }
-      @adapter = -> { Flipper::Adapters::Memory.new }
+      @builder = AdapterBuilder.new { store Flipper::Adapters::Memory }
+      @default = -> { Flipper.new(@builder.to_adapter) }
     end
 
     # The default adapter to use.
@@ -24,9 +24,20 @@ def initialize(options = {})
     #
     def adapter(&block)
       if block_given?
-        @adapter = block
+        @builder.store(block)
       else
-        @adapter.call
+        @builder.to_adapter
+      end
+    end
+
+    # An adapter to use to augment the primary storage adapter. See `AdapterBuilder#use`
+    if RUBY_VERSION >= '3.0'
+      def use(klass, *args, **kwargs, &block)
+        @builder.use(klass, *args, **kwargs, &block)
+      end
+    else
+      def use(klass, *args, &block)
+        @builder.use(klass, *args, &block)
       end
     end
 

lib/flipper/engine.rb

@@ -9,7 +9,8 @@ class Engine < Rails::Engine
         preload: ENV.fetch('FLIPPER_PRELOAD', 'true').casecmp('true').zero?,
         instrumenter: ENV.fetch('FLIPPER_INSTRUMENTER', 'ActiveSupport::Notifications').constantize,
         log: ENV.fetch('FLIPPER_LOG', 'true').casecmp('true').zero?,
-        cloud_path: "_flipper"
+        cloud_path: "_flipper",
+        strict: default_strict_value
       )
     end
 
@@ -25,6 +26,10 @@ class Engine < Rails::Engine
       require 'flipper/cloud' if cloud?
 
       Flipper.configure do |config|
+        if app.config.flipper.strict
+          config.use Flipper::Adapters::Strict, app.config.flipper.strict
+        end
+
         config.default do
           if cloud?
             Flipper::Cloud.new(
@@ -61,5 +66,19 @@ class Engine < Rails::Engine
     def cloud?
       !!ENV["FLIPPER_CLOUD_TOKEN"]
     end
+
+    def default_strict_value
+      value = ENV["FLIPPER_STRICT"]
+      if value.in?(["warn", "raise", "noop"])
+        value.to_sym
+      elsif value
+        Typecast.to_boolean(value) ? :raise : false
+      elsif Rails.env.production?
+        false
+      else
+        # Warn for now. Future versions will default to :raise in development and test
+        :warn
+      end
+    end
   end
 end

spec/flipper/adapter_builder_spec.rb

@@ -0,0 +1,73 @@
+RSpec.describe Flipper::AdapterBuilder do
+  describe "#initialize" do
+    it "instance_eval's block with no arg" do
+      called = false
+      self_in_block = nil
+
+      described_class.new do
+        called = true
+        self_in_block = self
+      end
+
+      expect(self_in_block).to be_instance_of(described_class)
+      expect(called).to be(true)
+    end
+
+    it "evals block with arg" do
+      called = false
+      self_outside_block = self
+      self_in_block = nil
+
+      described_class.new do |arg|
+        called = true
+        self_in_block = self
+        expect(arg).to be_instance_of(described_class)
+      end
+
+      expect(self_in_block).to be(self_outside_block)
+      expect(called).to be(true)
+    end
+  end
+
+  describe "#use" do
+    it "wraps the store adapter with the given adapter" do
+      subject.use(Flipper::Adapters::Memoizable)
+      subject.use(Flipper::Adapters::Strict, :warn)
+
+      memoizable_adapter = subject.to_adapter
+      strict_adapter = memoizable_adapter.adapter
+      memory_adapter = strict_adapter.adapter
+
+
+      expect(memoizable_adapter).to be_instance_of(Flipper::Adapters::Memoizable)
+      expect(strict_adapter).to be_instance_of(Flipper::Adapters::Strict)
+      expect(strict_adapter.handler).to be(Flipper::Adapters::Strict::HANDLERS.fetch(:warn))
+      expect(memory_adapter).to be_instance_of(Flipper::Adapters::Memory)
+    end
+
+    it "passes block to adapter initializer" do
+      expected_block = lambda {}
+      adapter_class = double('adapter class')
+
+      subject.use(adapter_class, &expected_block)
+
+      expect(adapter_class).to receive(:new) { |&block| expect(block).to be(expected_block) }.and_return(:adapter)
+      expect(subject.to_adapter).to be(:adapter)
+    end
+  end
+
+  describe "#store" do
+    it "defaults to memory adapter" do
+      expect(subject.to_adapter).to be_instance_of(Flipper::Adapters::Memory)
+    end
+
+    it "only saves one store" do
+      require "flipper/adapters/pstore"
+      subject.store(Flipper::Adapters::PStore)
+      expect(subject.to_adapter).to be_instance_of(Flipper::Adapters::PStore)
+
+      subject.store(Flipper::Adapters::Memory)
+      expect(subject.to_adapter).to be_instance_of(Flipper::Adapters::Memory)
+    end
+  end
+end

spec/flipper/engine_spec.rb

@@ -15,11 +15,70 @@
     ActiveSupport::Dependencies.autoload_once_paths = ActiveSupport::Dependencies.autoload_once_paths.dup
   end
 
+  # Reset Rails.env around each example
+  around do |example|
+    begin
+      env = Rails.env.to_s
+      example.run
+    ensure
+      Rails.env = env
+    end
+  end
+
   let(:config) { application.config.flipper }
 
   subject { application.initialize! }
 
+  shared_examples 'config.strict' do
+    let(:adapter) { Flipper.adapter.adapter }
+
+    it 'can set strict=true from ENV' do
+      with_env 'FLIPPER_STRICT' => 'true' do
+        subject
+        expect(config.strict).to eq(:raise)
+        expect(adapter).to be_instance_of(Flipper::Adapters::Strict)
+      end
+    end
+
+    it 'can set strict=warn from ENV' do
+      with_env 'FLIPPER_STRICT' => 'warn' do
+        subject
+        expect(config.strict).to eq(:warn)
+        expect(adapter).to be_instance_of(Flipper::Adapters::Strict)
+        expect(adapter.handler).to be(Flipper::Adapters::Strict::HANDLERS.fetch(:warn))
+      end
+    end
+
+    it 'can set strict=false from ENV' do
+      with_env 'FLIPPER_STRICT' => 'false' do
+        subject
+        expect(config.strict).to eq(false)
+        expect(adapter).to be_instance_of(Flipper::Adapters::Memory)
+      end
+    end
+
+    it "defaults to strict=false in RAILS_ENV=production" do
+        Rails.env = "production"
+        subject
+        expect(config.strict).to eq(false)
+        expect(adapter).to be_instance_of(Flipper::Adapters::Memory)
+    end
+
+    %w(development test).each do |env|
+      it "defaults to strict=warn in RAILS_ENV=#{env}" do
+        Rails.env = env
+        expect(Rails.env).to eq(env)
+        subject
+        expect(config.strict).to eq(:warn)
+        expect(adapter).to be_instance_of(Flipper::Adapters::Strict)
+        expect(adapter.handler).to be(Flipper::Adapters::Strict::HANDLERS.fetch(:warn))
+      end
+    end
+  end
+
   context 'cloudless' do
+    it_behaves_like 'config.strict'
+
     it 'can set env_key from ENV' do
       with_env 'FLIPPER_ENV_KEY' => 'flopper' do
         subject
@@ -106,6 +165,14 @@
     # App for Rack::Test
     let(:app) { application.routes }
 
+    it_behaves_like 'config.strict' do
+      let(:adapter) do
+        dual_write = Flipper.adapter.adapter
+        poll = dual_write.local
+        poll.adapter
+      end
+    end
+
     it "initializes cloud configuration" do
       stub_request(:get, /flippercloud\.io/).to_return(status: 200, body: "{}")