diff --git a/lib/flipper/adapter.rb b/lib/flipper/adapter.rb index 33f6c1fd1..65e279631 100644 --- a/lib/flipper/adapter.rb +++ b/lib/flipper/adapter.rb @@ -28,6 +28,21 @@ def read_only? false end + # Public: Read a named integer value from the adapter, or nil if absent. + # Adapters that support typed integer storage override this; the default + # is a no-op so unaware adapters degrade to today's behavior. + def read_integer(key) + nil + end + + # Public: Atomically set a named integer to value if and only if the new + # value is strictly greater than the currently stored value. Returns true + # if the write happened, false if rejected or unsupported. Adapters that + # support typed integer storage override this. + def set_integer_if_greater(key, value) + false + end + # Public: Get all features and gate values in one call. Defaults to one call # to features and another to get_multi. Feel free to override per adapter to # make this more efficient. diff --git a/lib/flipper/adapters/active_record.rb b/lib/flipper/adapters/active_record.rb index 6733b46d4..91d956137 100644 --- a/lib/flipper/adapters/active_record.rb +++ b/lib/flipper/adapters/active_record.rb @@ -5,6 +5,7 @@ require_relative 'active_record/model' require_relative 'active_record/feature' require_relative 'active_record/gate' +require_relative 'active_record/kv_integer' module Flipper module Adapters @@ -32,6 +33,7 @@ def initialize(options = {}) @name = options.fetch(:name, :active_record) @feature_class = options.fetch(:feature_class) { Flipper::Adapters::ActiveRecord::Feature } @gate_class = options.fetch(:gate_class) { Flipper::Adapters::ActiveRecord::Gate } + @kv_integer_class = options.fetch(:kv_integer_class) { Flipper::Adapters::ActiveRecord::KvInteger } end # Public: The set of known features. @@ -180,6 +182,40 @@ def disable(feature, gate, thing) true end + def read_integer(key) + return nil unless kv_integer_table_present? + with_connection(@kv_integer_class) do + @kv_integer_class.where(key: key.to_s).limit(1).pluck(:value).first + end + end + + def set_integer_if_greater(key, value) + return false unless kv_integer_table_present? + value = value.to_i + key = key.to_s + with_write_connection(@kv_integer_class) do + updated = @kv_integer_class + .where(key: key) + .where("value < ?", value) + .update_all(value: value, updated_at: Time.current) + return true if updated > 0 + + begin + @kv_integer_class.create!(key: key, value: value) + return true + rescue ::ActiveRecord::RecordNotUnique + # Row exists. Either stored >= ours (steady-state rejection) or a + # concurrent insert raced us with a lower value. Retry UPDATE once; + # if it still matches nothing, stored is provably >= ours. + end + + @kv_integer_class + .where(key: key) + .where("value < ?", value) + .update_all(value: value, updated_at: Time.current) > 0 + end + end + # Private def unsupported_data_type(data_type) raise "#{data_type} is not supported by this adapter" @@ -187,6 +223,13 @@ def unsupported_data_type(data_type) private + def kv_integer_table_present? + return @kv_integer_table_present if defined?(@kv_integer_table_present) + @kv_integer_table_present = with_connection(@kv_integer_class) { @kv_integer_class.table_exists? } + rescue ::ActiveRecord::StatementInvalid + false + end + def set(feature, gate, thing, options = {}) clear_feature = options.fetch(:clear, false) json_feature = options.fetch(:json, false) diff --git a/lib/flipper/adapters/active_record/kv_integer.rb b/lib/flipper/adapters/active_record/kv_integer.rb new file mode 100644 index 000000000..e8cf670e9 --- /dev/null +++ b/lib/flipper/adapters/active_record/kv_integer.rb @@ -0,0 +1,18 @@ +require 'flipper/adapters/active_record/model' + +module Flipper + module Adapters + class ActiveRecord + # Private: Do not use outside of this adapter. + class KvInteger < Model + self.table_name = [ + Model.table_name_prefix, + "flipper_kv_integers", + Model.table_name_suffix, + ].join + + validates :key, presence: true + end + end + end +end diff --git a/lib/flipper/adapters/cache_base.rb b/lib/flipper/adapters/cache_base.rb index 03f600d09..efd45c211 100644 --- a/lib/flipper/adapters/cache_base.rb +++ b/lib/flipper/adapters/cache_base.rb @@ -112,6 +112,18 @@ def disable(feature, gate, thing) result end + # Public + def read_integer(key) + cache_fetch(integer_cache_key(key)) { @adapter.read_integer(key) } + end + + # Public + def set_integer_if_greater(key, value) + @adapter.set_integer_if_greater(key, value).tap do + cache_delete(integer_cache_key(key)) + end + end + # Public: Generate the cache key for a given feature. # # key - The String or Symbol feature key. @@ -119,6 +131,13 @@ def feature_cache_key(key) "#{@namespace}/feature/#{key}" end + # Public: Generate the cache key for a given integer. + # + # key - The String or Symbol integer key. + def integer_cache_key(key) + "#{@namespace}/integer/#{key}" + end + private def read_all_features(**kwargs) diff --git a/lib/flipper/adapters/dual_write.rb b/lib/flipper/adapters/dual_write.rb index 562cd5208..a014c2b69 100644 --- a/lib/flipper/adapters/dual_write.rb +++ b/lib/flipper/adapters/dual_write.rb @@ -58,6 +58,16 @@ def disable(feature, gate, thing) @local.disable(feature, gate, thing) end end + + def read_integer(key) + @local.read_integer(key) + end + + def set_integer_if_greater(key, value) + @remote.set_integer_if_greater(key, value).tap do + @local.set_integer_if_greater(key, value) + end + end end end end diff --git a/lib/flipper/adapters/failover.rb b/lib/flipper/adapters/failover.rb index cc81049e5..99ff9d2c2 100644 --- a/lib/flipper/adapters/failover.rb +++ b/lib/flipper/adapters/failover.rb @@ -80,6 +80,18 @@ def disable(feature, gate, thing) @secondary.disable(feature, gate, thing) if @dual_write end end + + def read_integer(key) + @primary.read_integer(key) + rescue *@errors + @secondary.read_integer(key) + end + + def set_integer_if_greater(key, value) + @primary.set_integer_if_greater(key, value).tap do + @secondary.set_integer_if_greater(key, value) if @dual_write + end + end end end end diff --git a/lib/flipper/adapters/failsafe.rb b/lib/flipper/adapters/failsafe.rb index 6779efc2e..257187510 100644 --- a/lib/flipper/adapters/failsafe.rb +++ b/lib/flipper/adapters/failsafe.rb @@ -67,6 +67,18 @@ def disable(feature, gate, thing) rescue *@errors false end + + def read_integer(key) + @adapter.read_integer(key) + rescue *@errors + nil + end + + def set_integer_if_greater(key, value) + @adapter.set_integer_if_greater(key, value) + rescue *@errors + false + end end end end diff --git a/lib/flipper/adapters/http.rb b/lib/flipper/adapters/http.rb index ea73bdcfe..b94063c97 100644 --- a/lib/flipper/adapters/http.rb +++ b/lib/flipper/adapters/http.rb @@ -25,6 +25,7 @@ def initialize(options = {}) @last_get_all_etag = nil @last_get_all_result = nil @last_get_all_response = nil + @last_sync_version = nil @get_all_mutex = Mutex.new end @@ -97,9 +98,11 @@ def get_all(cache_bust: false) result[feature.key] = result_for_feature(feature, gates_by_key[feature.key]) end + sync_version = response['flipper-sync-version'] @get_all_mutex.synchronize do @last_get_all_etag = response['etag'] if response['etag'] @last_get_all_result = result + @last_sync_version = sync_version && sync_version.to_i end result @@ -109,6 +112,11 @@ def last_get_all_response @get_all_mutex.synchronize { @last_get_all_response } end + def read_integer(key) + return nil unless key.to_sym == :sync_version + @get_all_mutex.synchronize { @last_sync_version } + end + def features response = @client.get('/features?exclude_gate_names=true') raise Error, response unless response.is_a?(Net::HTTPOK) diff --git a/lib/flipper/adapters/instrumented.rb b/lib/flipper/adapters/instrumented.rb index 37c08f2d1..621ee9c8f 100644 --- a/lib/flipper/adapters/instrumented.rb +++ b/lib/flipper/adapters/instrumented.rb @@ -165,6 +165,31 @@ def export(format: :json, version: 1) payload[:result] = @adapter.export(format: format, version: version) end end + + def read_integer(key) + default_payload = { + operation: :read_integer, + adapter_name: @adapter.name, + key: key, + } + + @instrumenter.instrument(InstrumentationName, default_payload) do |payload| + payload[:result] = @adapter.read_integer(key) + end + end + + def set_integer_if_greater(key, value) + default_payload = { + operation: :set_integer_if_greater, + adapter_name: @adapter.name, + key: key, + value: value, + } + + @instrumenter.instrument(InstrumentationName, default_payload) do |payload| + payload[:result] = @adapter.set_integer_if_greater(key, value) + end + end end end end diff --git a/lib/flipper/adapters/memoizable.rb b/lib/flipper/adapters/memoizable.rb index a037daefa..f9b1a08b3 100644 --- a/lib/flipper/adapters/memoizable.rb +++ b/lib/flipper/adapters/memoizable.rb @@ -126,6 +126,21 @@ def import(source) @adapter.import(source).tap { cache.clear if memoizing? } end + def read_integer(key) + if memoizing? + cache_key = integer_key_for(key) + cache.fetch(cache_key) { cache[cache_key] = @adapter.read_integer(key) } + else + @adapter.read_integer(key) + end + end + + def set_integer_if_greater(key, value) + @adapter.set_integer_if_greater(key, value).tap do + cache.delete(integer_key_for(key)) if memoizing? + end + end + def export(format: :json, version: 1) @adapter.export(format: format, version: version) end @@ -159,6 +174,10 @@ def key_for(key) "feature/#{key}" end + def integer_key_for(key) + "integer/#{key}" + end + def expire_feature(feature) cache.delete(key_for(feature.key)) if memoizing? end diff --git a/lib/flipper/adapters/memory.rb b/lib/flipper/adapters/memory.rb index 67a312af9..5d455b09d 100644 --- a/lib/flipper/adapters/memory.rb +++ b/lib/flipper/adapters/memory.rb @@ -11,6 +11,7 @@ class Memory # Public def initialize(source = nil, threadsafe: true) @source = Typecast.features_hash(source) + @integers = {} @lock = Mutex.new if threadsafe reset end @@ -116,10 +117,31 @@ def inspect def import(source) adapter = self.class.from(source) get_all = Typecast.features_hash(adapter.get_all) - synchronize { @source.replace(get_all) } + sync_version = adapter.read_integer(:sync_version) + synchronize do + @source.replace(get_all) + @integers["sync_version"] = sync_version if sync_version + end true end + def read_integer(key) + synchronize { @integers[key.to_s] } + end + + def set_integer_if_greater(key, value) + value = value.to_i + synchronize do + current = @integers[key.to_s] + if current.nil? || value > current + @integers[key.to_s] = value + true + else + false + end + end + end + private def reset diff --git a/lib/flipper/adapters/mongo.rb b/lib/flipper/adapters/mongo.rb index 598aebf71..7c1abc32f 100644 --- a/lib/flipper/adapters/mongo.rb +++ b/lib/flipper/adapters/mongo.rb @@ -89,6 +89,19 @@ def enable(feature, gate, thing) true end + def read_integer(key) + doc = @collection.find(_id: integer_id_for(key)).limit(1).first + doc && doc['value'] + end + + def set_integer_if_greater(key, value) + result = @collection.find(_id: integer_id_for(key)).update_one( + { '$max' => { 'value' => value.to_i } }, + upsert: true + ) + result.modified_count.to_i > 0 || result.upserted_count.to_i > 0 + end + # Public: Disables a gate for a given thing. # # feature - The Flipper::Feature for the gate. @@ -139,6 +152,11 @@ def unsupported_data_type(data_type) raise "#{data_type} is not supported by this adapter" end + # Private + def integer_id_for(key) + "flipper_int:#{key}" + end + # Private def find(key) @collection.find(_id: key.to_s).limit(1).first || {} diff --git a/lib/flipper/adapters/poll.rb b/lib/flipper/adapters/poll.rb index 20cd9e920..a9c77a726 100644 --- a/lib/flipper/adapters/poll.rb +++ b/lib/flipper/adapters/poll.rb @@ -12,7 +12,7 @@ class Poll attr_reader :adapter, :poller - def_delegators :synced_adapter, :features, :get, :get_multi, :get_all, :add, :remove, :clear, :enable, :disable + def_delegators :synced_adapter, :features, :get, :get_multi, :get_all, :add, :remove, :clear, :enable, :disable, :read_integer, :set_integer_if_greater def initialize(poller, adapter) @adapter = adapter diff --git a/lib/flipper/adapters/read_only.rb b/lib/flipper/adapters/read_only.rb index 666e25a96..7ffcd45bb 100644 --- a/lib/flipper/adapters/read_only.rb +++ b/lib/flipper/adapters/read_only.rb @@ -4,7 +4,7 @@ module Flipper module Adapters # Public: Adapter that wraps another adapter and raises for any writes. class ReadOnly < Wrapper - WRITE_METHODS = %i[add remove clear enable disable] + WRITE_METHODS = %i[add remove clear enable disable set_integer_if_greater] class WriteAttempted < Error def initialize(message = nil) diff --git a/lib/flipper/adapters/redis.rb b/lib/flipper/adapters/redis.rb index b196b42e7..2023a8be6 100644 --- a/lib/flipper/adapters/redis.rb +++ b/lib/flipper/adapters/redis.rb @@ -11,6 +11,15 @@ class Redis attr_reader :key_prefix + SET_INTEGER_IF_GREATER_LUA = <<~LUA.freeze + local current = redis.call('GET', KEYS[1]) + if current == false or tonumber(ARGV[1]) > tonumber(current) then + redis.call('SET', KEYS[1], ARGV[1]) + return 1 + end + return 0 + LUA + def features_key "#{key_prefix}flipper_features" end @@ -19,6 +28,10 @@ def key_for(feature_name) "#{key_prefix}#{feature_name}" end + def integer_key_for(key) + "#{key_prefix}flipper_int:#{key}" + end + # Public: Initializes a Redis flipper adapter. # # client - The Redis client to use. @@ -132,6 +145,17 @@ def disable(feature, gate, thing) true end + def read_integer(key) + value = with_connection { |conn| conn.get(integer_key_for(key)) } + value && value.to_i + end + + def set_integer_if_greater(key, value) + with_connection do |conn| + conn.public_send(:eval, SET_INTEGER_IF_GREATER_LUA, keys: [integer_key_for(key)], argv: [value.to_i]) == 1 + end + end + private def redis_sadd_returns_boolean? diff --git a/lib/flipper/adapters/sequel.rb b/lib/flipper/adapters/sequel.rb index afdf5adb4..bedd84265 100644 --- a/lib/flipper/adapters/sequel.rb +++ b/lib/flipper/adapters/sequel.rb @@ -25,6 +25,11 @@ class Gate < ::Sequel::Model(:flipper_gates) plugin :timestamps, update_on_create: true end + + # Private: Do not use outside of this adapter. + class KvInteger < ::Sequel::Model(:flipper_kv_integers) + plugin :timestamps, update_on_create: true + end ensure ::Sequel::Model.require_valid_table = old end @@ -50,6 +55,7 @@ def initialize(options = {}) @name = options.fetch(:name, :sequel) @feature_class = options.fetch(:feature_class) { Feature } @gate_class = options.fetch(:gate_class) { Gate } + @kv_integer_class = options.fetch(:kv_integer_class) { KvInteger } warn VALUE_TO_TEXT_WARNING if value_not_text? end @@ -169,8 +175,45 @@ def disable(feature, gate, thing) true end + def read_integer(key) + return nil unless kv_integer_table_present? + @kv_integer_class.where(key: key.to_s).get(:value) + end + + def set_integer_if_greater(key, incoming) + return false unless kv_integer_table_present? + incoming = incoming.to_i + key = key.to_s + updated = @kv_integer_class + .where(key: key) + .where { value < incoming } + .update(value: incoming, updated_at: Time.now) + return true if updated > 0 + + begin + @kv_integer_class.insert(key: key, value: incoming, created_at: Time.now, updated_at: Time.now) + return true + rescue ::Sequel::UniqueConstraintViolation + # Row exists. Either stored >= ours (steady-state rejection) or a + # concurrent insert raced us with a lower value. Retry UPDATE once; + # if it still matches nothing, stored is provably >= ours. + end + + @kv_integer_class + .where(key: key) + .where { value < incoming } + .update(value: incoming, updated_at: Time.now) > 0 + end + private + def kv_integer_table_present? + return @kv_integer_table_present if defined?(@kv_integer_table_present) + @kv_integer_table_present = @kv_integer_class.db.table_exists?(@kv_integer_class.table_name) + rescue ::Sequel::DatabaseError + false + end + def unsupported_data_type(data_type) raise "#{data_type} is not supported by this adapter" end diff --git a/lib/flipper/adapters/sync.rb b/lib/flipper/adapters/sync.rb index ab34b5524..dc153c0a9 100644 --- a/lib/flipper/adapters/sync.rb +++ b/lib/flipper/adapters/sync.rb @@ -81,6 +81,17 @@ def disable(feature, gate, thing) end end + def read_integer(key) + synchronize + @local.read_integer(key) + end + + def set_integer_if_greater(key, value) + @remote.set_integer_if_greater(key, value).tap do + @local.set_integer_if_greater(key, value) + end + end + private def synchronize diff --git a/lib/flipper/adapters/sync/synchronizer.rb b/lib/flipper/adapters/sync/synchronizer.rb index 7ce58a471..ecde56b24 100644 --- a/lib/flipper/adapters/sync/synchronizer.rb +++ b/lib/flipper/adapters/sync/synchronizer.rb @@ -10,6 +10,9 @@ class Sync # Public: Given a local and remote adapter, it can update the local to # match the remote doing only the necessary enable/disable operations. class Synchronizer + SYNC_VERSION_KEY = :sync_version + MAX_OUTVOTE_REPAIRS = 3 + # Public: Initializes a new synchronizer. # # local - The Flipper adapter to get in sync with the remote. @@ -39,8 +42,38 @@ def call private def sync - local_get_all = @local.get_all remote_get_all = @remote.get_all(cache_bust: @cache_bust) + remote_version = @remote.read_integer(SYNC_VERSION_KEY) + local_version = @local.read_integer(SYNC_VERSION_KEY) + + if remote_version && local_version && remote_version.to_i <= local_version.to_i + return + end + + apply(remote_get_all) + + if remote_version + accepted = @local.set_integer_if_greater(SYNC_VERSION_KEY, remote_version) + # A rejection only indicates an outvote when the local adapter actually + # stores a higher version. Adapters without typed-integer storage return + # false unconditionally, so guard on the post-write read to keep the + # event and repair scoped to real races (including cold-start interleaves + # where local_version was nil pre-sync). + current_version = @local.read_integer(SYNC_VERSION_KEY) + if !accepted && current_version && current_version.to_i > remote_version.to_i + @instrumenter.instrument("synchronizer_outvoted.flipper", remote_version: remote_version) + repair_after_outvote(remote_version) + end + end + + nil + rescue => exception + @instrumenter.instrument("synchronizer_exception.flipper", exception: exception) + raise if @raise + end + + def apply(remote_get_all) + local_get_all = @local.get_all # Sync all the gate values. remote_get_all.each do |feature_key, remote_gates_hash| @@ -59,11 +92,22 @@ def sync # Remove features that are present in local and missing in remote. features_to_remove = local_get_all.keys - remote_get_all.keys features_to_remove.each { |key| Feature.new(key, @local, instrumenter: @instrumenter).remove } + end - nil - rescue => exception - @instrumenter.instrument("synchronizer_exception.flipper", exception: exception) - raise if @raise + def repair_after_outvote(outvoted_version) + current_version = @local.read_integer(SYNC_VERSION_KEY) + return unless current_version && current_version.to_i > outvoted_version.to_i + + MAX_OUTVOTE_REPAIRS.times do + remote_get_all = @remote.get_all(cache_bust: true) + remote_version = @remote.read_integer(SYNC_VERSION_KEY) + apply(remote_get_all) + + @local.set_integer_if_greater(SYNC_VERSION_KEY, remote_version) if remote_version + + current_version = @local.read_integer(SYNC_VERSION_KEY) + break unless remote_version && current_version && current_version.to_i > remote_version.to_i + end end end end diff --git a/lib/flipper/adapters/wrapper.rb b/lib/flipper/adapters/wrapper.rb index 6026ebc96..6211f0f10 100644 --- a/lib/flipper/adapters/wrapper.rb +++ b/lib/flipper/adapters/wrapper.rb @@ -18,6 +18,8 @@ class Wrapper :get_all, :enable, :disable, + :read_integer, + :set_integer_if_greater, ].freeze attr_reader :adapter diff --git a/lib/generators/flipper/templates/migration.erb b/lib/generators/flipper/templates/migration.erb index 178f0356b..fdb87c7d7 100644 --- a/lib/generators/flipper/templates/migration.erb +++ b/lib/generators/flipper/templates/migration.erb @@ -13,9 +13,17 @@ class CreateFlipperTables < ActiveRecord::Migration<%= migration_version %> t.timestamps null: false end add_index :flipper_gates, [:feature_key, :key, :value], unique: true, length: { value: 255 } + + create_table :flipper_kv_integers do |t| + t.string :key, null: false + t.bigint :value, null: false, default: 0 + t.timestamps null: false + end + add_index :flipper_kv_integers, :key, unique: true end def down + drop_table :flipper_kv_integers drop_table :flipper_gates drop_table :flipper_features end diff --git a/lib/generators/flipper/templates/sequel_migration.rb b/lib/generators/flipper/templates/sequel_migration.rb index 0ed08b7f0..6598e7fdc 100644 --- a/lib/generators/flipper/templates/sequel_migration.rb +++ b/lib/generators/flipper/templates/sequel_migration.rb @@ -14,9 +14,18 @@ def up DateTime :updated_at, null: false primary_key [:feature_key, :key, :value] end + + create_table :flipper_kv_integers do + primary_key :id + String :key, null: false, unique: true + Bignum :value, null: false, default: 0 + DateTime :created_at, null: false + DateTime :updated_at, null: false + end end def down + drop_table :flipper_kv_integers drop_table :flipper_gates drop_table :flipper_features end diff --git a/lib/generators/flipper/templates/update/migrations/03_create_flipper_kv_integers.rb.erb b/lib/generators/flipper/templates/update/migrations/03_create_flipper_kv_integers.rb.erb new file mode 100644 index 000000000..c92708f92 --- /dev/null +++ b/lib/generators/flipper/templates/update/migrations/03_create_flipper_kv_integers.rb.erb @@ -0,0 +1,18 @@ +# frozen_string_literal: true + +class CreateFlipperKvIntegers < ActiveRecord::Migration<%= migration_version %> + def up + return if table_exists? :flipper_kv_integers + + create_table :flipper_kv_integers do |t| + t.string :key, null: false + t.bigint :value, null: false, default: 0 + t.timestamps null: false + end + add_index :flipper_kv_integers, :key, unique: true + end + + def down + drop_table :flipper_kv_integers + end +end diff --git a/spec/flipper/adapters/active_record_spec.rb b/spec/flipper/adapters/active_record_spec.rb index 7fc90bf85..8ea031abc 100644 --- a/spec/flipper/adapters/active_record_spec.rb +++ b/spec/flipper/adapters/active_record_spec.rb @@ -78,6 +78,74 @@ flipper.preload([:foo]) end + describe 'read_integer / set_integer_if_greater' do + it 'returns nil for unknown keys' do + expect(subject.read_integer(:sync_version)).to be_nil + end + + it 'sets a new value when none exists' do + expect(subject.set_integer_if_greater(:sync_version, 100)).to eq(true) + expect(subject.read_integer(:sync_version)).to eq(100) + end + + it 'rejects a lower value' do + subject.set_integer_if_greater(:sync_version, 100) + expect(subject.set_integer_if_greater(:sync_version, 99)).to eq(false) + expect(subject.read_integer(:sync_version)).to eq(100) + end + + it 'rejects an equal value' do + subject.set_integer_if_greater(:sync_version, 100) + expect(subject.set_integer_if_greater(:sync_version, 100)).to eq(false) + expect(subject.read_integer(:sync_version)).to eq(100) + end + + it 'accepts a strictly greater value' do + subject.set_integer_if_greater(:sync_version, 100) + expect(subject.set_integer_if_greater(:sync_version, 200)).to eq(true) + expect(subject.read_integer(:sync_version)).to eq(200) + end + + it 'tracks separate keys independently' do + subject.set_integer_if_greater(:foo, 100) + subject.set_integer_if_greater(:bar, 50) + expect(subject.read_integer(:foo)).to eq(100) + expect(subject.read_integer(:bar)).to eq(50) + end + + context 'when flipper_kv_integers table is missing' do + before do + silence { ActiveRecord::Base.connection.drop_table(:flipper_kv_integers) } + end + + it 'read_integer returns nil' do + fresh = described_class.new + expect(fresh.read_integer(:sync_version)).to be_nil + end + + it 'set_integer_if_greater returns false' do + fresh = described_class.new + expect(fresh.set_integer_if_greater(:sync_version, 100)).to eq(false) + end + end + + it 'recovers from a transient StatementInvalid on the table presence check' do + fresh = described_class.new + kv_class = fresh.instance_variable_get(:@kv_integer_class) + + call_count = 0 + allow(kv_class).to receive(:table_exists?).and_wrap_original do |original, *args| + call_count += 1 + raise ::ActiveRecord::StatementInvalid, 'transient blip' if call_count == 1 + original.call(*args) + end + + expect(fresh.read_integer(:sync_version)).to be_nil + expect(fresh.set_integer_if_greater(:sync_version, 100)).to eq(true) + expect(fresh.read_integer(:sync_version)).to eq(100) + end + end + it 'should not poison wrapping transactions' do flipper = Flipper.new(subject) diff --git a/spec/flipper/adapters/active_support_cache_store_spec.rb b/spec/flipper/adapters/active_support_cache_store_spec.rb index 051b3913c..3fa0845a6 100644 --- a/spec/flipper/adapters/active_support_cache_store_spec.rb +++ b/spec/flipper/adapters/active_support_cache_store_spec.rb @@ -270,4 +270,37 @@ expect(subject.name).to be(:active_support_cache_store) end end + + describe '#read_integer / #set_integer_if_greater' do + it 'forwards to the wrapped adapter' do + expect(subject.set_integer_if_greater(:sync_version, 42)).to eq(true) + expect(subject.read_integer(:sync_version)).to eq(42) + expect(memory_adapter.read_integer(:sync_version)).to eq(42) + end + + it 'caches reads and only hits the adapter once' do + memory_adapter.set_integer_if_greater(:sync_version, 42) + memory_adapter.reset + + 3.times { subject.read_integer(:sync_version) } + expect(memory_adapter.count(:read_integer)).to eq(1) + end + + it 'invalidates the cached read after a write' do + memory_adapter.set_integer_if_greater(:sync_version, 42) + expect(subject.read_integer(:sync_version)).to eq(42) + + subject.set_integer_if_greater(:sync_version, 100) + expect(subject.read_integer(:sync_version)).to eq(100) + end + + it 'invalidates the cached read even when the write is rejected' do + memory_adapter.set_integer_if_greater(:sync_version, 100) + expect(subject.read_integer(:sync_version)).to eq(100) + + memory_adapter.set_integer_if_greater(:sync_version, 200) + expect(subject.set_integer_if_greater(:sync_version, 150)).to eq(false) + expect(subject.read_integer(:sync_version)).to eq(200) + end + end end diff --git a/spec/flipper/adapters/dual_write_spec.rb b/spec/flipper/adapters/dual_write_spec.rb index 7d4e6117e..fe01a9b1f 100644 --- a/spec/flipper/adapters/dual_write_spec.rb +++ b/spec/flipper/adapters/dual_write_spec.rb @@ -67,6 +67,15 @@ expect(local_adapter.count(:disable)).to be(1) end + describe '#read_integer / #set_integer_if_greater' do + it 'reads from local and writes to both adapters' do + expect(subject.set_integer_if_greater(:sync_version, 42)).to eq(true) + expect(subject.read_integer(:sync_version)).to eq(42) + expect(local_adapter.read_integer(:sync_version)).to eq(42) + expect(remote_adapter.read_integer(:sync_version)).to eq(42) + end + end + describe '#adapter_stack' do it 'returns the tree representation' do expect(subject.adapter_stack).to eq("dual_write(local: operation_logger -> memory, remote: operation_logger -> memory)") diff --git a/spec/flipper/adapters/failover_spec.rb b/spec/flipper/adapters/failover_spec.rb index a83114b75..fc60f6f01 100644 --- a/spec/flipper/adapters/failover_spec.rb +++ b/spec/flipper/adapters/failover_spec.rb @@ -127,6 +127,33 @@ end end + describe '#read_integer / #set_integer_if_greater' do + it 'reads from primary and writes only to primary by default' do + expect(subject.set_integer_if_greater(:sync_version, 42)).to eq(true) + expect(subject.read_integer(:sync_version)).to eq(42) + expect(primary.read_integer(:sync_version)).to eq(42) + expect(secondary.read_integer(:sync_version)).to be_nil + end + + context 'with dual_write enabled' do + let(:options) { { dual_write: true } } + + it 'writes to both primary and secondary' do + expect(subject.set_integer_if_greater(:sync_version, 42)).to eq(true) + expect(primary.read_integer(:sync_version)).to eq(42) + expect(secondary.read_integer(:sync_version)).to eq(42) + end + end + + context 'when primary raises' do + it 'falls back to secondary for reads' do + secondary.set_integer_if_greater(:sync_version, 99) + allow(primary).to receive(:read_integer).and_raise(StandardError) + expect(subject.read_integer(:sync_version)).to eq(99) + end + end + end + describe '#adapter_stack' do it 'returns the tree representation' do expect(subject.adapter_stack).to eq("failover(primary: memory, secondary: memory)") diff --git a/spec/flipper/adapters/failsafe_spec.rb b/spec/flipper/adapters/failsafe_spec.rb index a150a818c..617cb3163 100644 --- a/spec/flipper/adapters/failsafe_spec.rb +++ b/spec/flipper/adapters/failsafe_spec.rb @@ -9,6 +9,13 @@ it_should_behave_like 'a flipper adapter' + describe '#read_integer / #set_integer_if_greater' do + it 'forwards to the wrapped adapter when it does not raise' do + expect(subject.set_integer_if_greater(:sync_version, 42)).to eq(true) + expect(subject.read_integer(:sync_version)).to eq(42) + end + end + context 'when disaster strikes' do before do expect(flipper[feature.name].enable).to be(true) @@ -29,6 +36,8 @@ it { expect(subject.get_all).to eq({}) } it { expect(feature.enable).to eq(false) } it { expect(feature.disable).to eq(false) } + it { expect(subject.read_integer(:sync_version)).to be_nil } + it { expect(subject.set_integer_if_greater(:sync_version, 100)).to eq(false) } context 'when used via Flipper' do it { expect(flipper.features).to eq(Set.new) } diff --git a/spec/flipper/adapters/http_spec.rb b/spec/flipper/adapters/http_spec.rb index e9114e88e..fb165492a 100644 --- a/spec/flipper/adapters/http_spec.rb +++ b/spec/flipper/adapters/http_spec.rb @@ -175,6 +175,60 @@ end end + describe "#read_integer" do + it "returns nil before any get_all has happened" do + adapter = described_class.new(url: 'http://app.com/flipper') + expect(adapter.read_integer(:sync_version)).to be_nil + end + + it "returns the version from the flipper-sync-version response header" do + stub_request(:get, "http://app.com/flipper/features?exclude_gate_names=true") + .to_return(status: 200, body: JSON.generate(features: []), headers: { 'Flipper-Sync-Version' => '12345' }) + + adapter = described_class.new(url: 'http://app.com/flipper') + adapter.get_all + expect(adapter.read_integer(:sync_version)).to eq(12345) + end + + it "returns nil when the server response omits the header (older server)" do + stub_request(:get, "http://app.com/flipper/features?exclude_gate_names=true") + .to_return(status: 200, body: JSON.generate(features: [])) + + adapter = described_class.new(url: 'http://app.com/flipper') + adapter.get_all + expect(adapter.read_integer(:sync_version)).to be_nil + end + + it "preserves the cached version across 304 Not Modified responses" do + stub_request(:get, "http://app.com/flipper/features?exclude_gate_names=true") + .to_return(status: 200, body: JSON.generate(features: []), + headers: { 'ETag' => '"abc"', 'Flipper-Sync-Version' => '12345' }) + + adapter = described_class.new(url: 'http://app.com/flipper') + adapter.get_all + expect(adapter.read_integer(:sync_version)).to eq(12345) + + stub_request(:get, "http://app.com/flipper/features?exclude_gate_names=true") + .with(headers: { 'If-None-Match' => '"abc"' }) + .to_return(status: 304, headers: { 'ETag' => '"abc"' }) + + adapter.get_all + expect(adapter.read_integer(:sync_version)).to eq(12345) + end + + it "returns nil for keys other than :sync_version" do + adapter = described_class.new(url: 'http://app.com/flipper') + expect(adapter.read_integer(:other_key)).to be_nil + end + end + + describe "#set_integer_if_greater" do + it "returns false (writes never go server-side via this method)" do + adapter = described_class.new(url: 'http://app.com/flipper') + expect(adapter.set_integer_if_greater(:sync_version, 100)).to eq(false) + end + end + describe "#get_all" do it "raises error when not successful response" do stub_request(:get, "http://app.com/flipper/features?exclude_gate_names=true") diff --git a/spec/flipper/adapters/instrumented_spec.rb b/spec/flipper/adapters/instrumented_spec.rb index 0f25bbd9b..248cd2a5e 100644 --- a/spec/flipper/adapters/instrumented_spec.rb +++ b/spec/flipper/adapters/instrumented_spec.rb @@ -164,4 +164,39 @@ expect(event.payload[:result]).to be(result) end end + + describe '#read_integer' do + it 'forwards to wrapped adapter and records instrumentation' do + adapter.set_integer_if_greater(:sync_version, 42) + result = subject.read_integer(:sync_version) + + expect(result).to eq(42) + + event = instrumenter.events.last + expect(event).not_to be_nil + expect(event.name).to eq('adapter_operation.flipper') + expect(event.payload[:operation]).to eq(:read_integer) + expect(event.payload[:adapter_name]).to eq(:memory) + expect(event.payload[:key]).to eq(:sync_version) + expect(event.payload[:result]).to eq(42) + end + end + + describe '#set_integer_if_greater' do + it 'forwards to wrapped adapter and records instrumentation' do + result = subject.set_integer_if_greater(:sync_version, 100) + + expect(result).to eq(true) + expect(adapter.read_integer(:sync_version)).to eq(100) + + event = instrumenter.events.last + expect(event).not_to be_nil + expect(event.name).to eq('adapter_operation.flipper') + expect(event.payload[:operation]).to eq(:set_integer_if_greater) + expect(event.payload[:adapter_name]).to eq(:memory) + expect(event.payload[:key]).to eq(:sync_version) + expect(event.payload[:value]).to eq(100) + expect(event.payload[:result]).to eq(true) + end + end end diff --git a/spec/flipper/adapters/memoizable_spec.rb b/spec/flipper/adapters/memoizable_spec.rb index f80f31d0e..3ae3f8350 100644 --- a/spec/flipper/adapters/memoizable_spec.rb +++ b/spec/flipper/adapters/memoizable_spec.rb @@ -374,4 +374,45 @@ end end end + + describe '#read_integer / #set_integer_if_greater' do + it 'forwards to the wrapped adapter' do + expect(subject.set_integer_if_greater(:sync_version, 42)).to eq(true) + expect(subject.read_integer(:sync_version)).to eq(42) + expect(adapter.read_integer(:sync_version)).to eq(42) + end + + context 'when memoizing' do + before { subject.memoize = true } + + it 'memoizes reads and only hits the adapter once' do + adapter.set_integer_if_greater(:sync_version, 42) + logger = Flipper::Adapters::OperationLogger.new(adapter) + memoized = described_class.new(logger, cache) + memoized.memoize = true + + 3.times { memoized.read_integer(:sync_version) } + expect(logger.count(:read_integer)).to eq(1) + end + + it 'invalidates the cached read after a write' do + adapter.set_integer_if_greater(:sync_version, 42) + expect(subject.read_integer(:sync_version)).to eq(42) + + subject.set_integer_if_greater(:sync_version, 100) + expect(subject.read_integer(:sync_version)).to eq(100) + end + + it 'invalidates the cached read even when the write is rejected' do + adapter.set_integer_if_greater(:sync_version, 100) + expect(subject.read_integer(:sync_version)).to eq(100) + + # Simulate another process bumping above what's in cache. The cached + # value (100) is now stale relative to the underlying adapter (200). + adapter.set_integer_if_greater(:sync_version, 200) + expect(subject.set_integer_if_greater(:sync_version, 150)).to eq(false) + expect(subject.read_integer(:sync_version)).to eq(200) + end + end + end end diff --git a/spec/flipper/adapters/memory_spec.rb b/spec/flipper/adapters/memory_spec.rb index 5477bc490..bbab3000a 100644 --- a/spec/flipper/adapters/memory_spec.rb +++ b/spec/flipper/adapters/memory_spec.rb @@ -13,6 +13,73 @@ it_should_behave_like 'a flipper adapter' end + describe 'read_integer / set_integer_if_greater' do + subject { described_class.new } + + it 'returns nil for unknown keys' do + expect(subject.read_integer(:sync_version)).to be_nil + end + + it 'sets a new value when none exists' do + expect(subject.set_integer_if_greater(:sync_version, 100)).to eq(true) + expect(subject.read_integer(:sync_version)).to eq(100) + end + + it 'rejects a lower value' do + subject.set_integer_if_greater(:sync_version, 100) + expect(subject.set_integer_if_greater(:sync_version, 99)).to eq(false) + expect(subject.read_integer(:sync_version)).to eq(100) + end + + it 'rejects an equal value' do + subject.set_integer_if_greater(:sync_version, 100) + expect(subject.set_integer_if_greater(:sync_version, 100)).to eq(false) + expect(subject.read_integer(:sync_version)).to eq(100) + end + + it 'accepts a strictly greater value' do + subject.set_integer_if_greater(:sync_version, 100) + expect(subject.set_integer_if_greater(:sync_version, 200)).to eq(true) + expect(subject.read_integer(:sync_version)).to eq(200) + end + + it 'tracks separate keys independently' do + subject.set_integer_if_greater(:foo, 100) + subject.set_integer_if_greater(:bar, 50) + expect(subject.read_integer(:foo)).to eq(100) + expect(subject.read_integer(:bar)).to eq(50) + end + + it 'is isolated from get_all and clear' do + flipper = Flipper.new(subject) + flipper.enable(:my_feature) + subject.set_integer_if_greater(:sync_version, 100) + + flipper[:my_feature].clear + expect(subject.read_integer(:sync_version)).to eq(100) + end + + it 'imports sync_version from the source adapter' do + source = described_class.new + source.set_integer_if_greater(:sync_version, 42) + Flipper.new(source).enable(:search) + + subject.import(source) + + expect(subject.read_integer(:sync_version)).to eq(42) + end + + it 'does not overwrite sync_version when the source has none' do + subject.set_integer_if_greater(:sync_version, 100) + source = described_class.new + Flipper.new(source).enable(:search) + + subject.import(source) + + expect(subject.read_integer(:sync_version)).to eq(100) + end + end + it "can initialize from big hash" do flipper = Flipper.new(subject) flipper.enable :subscriptions diff --git a/spec/flipper/adapters/mongo_spec.rb b/spec/flipper/adapters/mongo_spec.rb index c6b3bd6b0..cfd4ed4d5 100644 --- a/spec/flipper/adapters/mongo_spec.rb +++ b/spec/flipper/adapters/mongo_spec.rb @@ -26,6 +26,35 @@ it_should_behave_like 'a flipper adapter' + describe 'read_integer / set_integer_if_greater' do + it 'returns nil for unknown keys' do + expect(subject.read_integer(:sync_version)).to be_nil + end + + it 'sets a new value when none exists' do + expect(subject.set_integer_if_greater(:sync_version, 100)).to eq(true) + expect(subject.read_integer(:sync_version)).to eq(100) + end + + it 'rejects a lower value' do + subject.set_integer_if_greater(:sync_version, 100) + expect(subject.set_integer_if_greater(:sync_version, 99)).to eq(false) + expect(subject.read_integer(:sync_version)).to eq(100) + end + + it 'rejects an equal value' do + subject.set_integer_if_greater(:sync_version, 100) + expect(subject.set_integer_if_greater(:sync_version, 100)).to eq(false) + expect(subject.read_integer(:sync_version)).to eq(100) + end + + it 'accepts a strictly greater value' do + subject.set_integer_if_greater(:sync_version, 100) + expect(subject.set_integer_if_greater(:sync_version, 200)).to eq(true) + expect(subject.read_integer(:sync_version)).to eq(200) + end + end + it 'configures itself on load' do Flipper.configuration = nil Flipper.instance = nil diff --git a/spec/flipper/adapters/operation_logger_spec.rb b/spec/flipper/adapters/operation_logger_spec.rb index 2fe229601..03f6c247b 100644 --- a/spec/flipper/adapters/operation_logger_spec.rb +++ b/spec/flipper/adapters/operation_logger_spec.rb @@ -125,4 +125,24 @@ expect(@result).to eq(adapter.export(format: :json, version: 1)) end end + + describe '#read_integer' do + it 'forwards to wrapped adapter and logs operation' do + adapter.set_integer_if_greater(:sync_version, 42) + result = subject.read_integer(:sync_version) + + expect(result).to eq(42) + expect(subject.count(:read_integer)).to be(1) + end + end + + describe '#set_integer_if_greater' do + it 'forwards to wrapped adapter and logs operation' do + result = subject.set_integer_if_greater(:sync_version, 100) + + expect(result).to eq(true) + expect(adapter.read_integer(:sync_version)).to eq(100) + expect(subject.count(:set_integer_if_greater)).to be(1) + end + end end diff --git a/spec/flipper/adapters/poll_spec.rb b/spec/flipper/adapters/poll_spec.rb index 2fe08fe75..71f7ae0fe 100644 --- a/spec/flipper/adapters/poll_spec.rb +++ b/spec/flipper/adapters/poll_spec.rb @@ -38,4 +38,13 @@ expect(local_adapter.features).to eq(remote_adapter.features) end + + describe '#read_integer / #set_integer_if_greater' do + it 'forwards through the synced local adapter' do + instance = described_class.new(poller, local_adapter) + expect(instance.set_integer_if_greater(:sync_version, 42)).to eq(true) + expect(instance.read_integer(:sync_version)).to eq(42) + expect(local_adapter.read_integer(:sync_version)).to eq(42) + end + end end diff --git a/spec/flipper/adapters/read_only_spec.rb b/spec/flipper/adapters/read_only_spec.rb index a14c3b423..47386ac52 100644 --- a/spec/flipper/adapters/read_only_spec.rb +++ b/spec/flipper/adapters/read_only_spec.rb @@ -97,4 +97,15 @@ expect { subject.disable(feature, boolean_gate, Flipper::Types::Boolean.new) } .to raise_error(Flipper::Adapters::ReadOnly::WriteAttempted) end + + it 'forwards read_integer to wrapped adapter' do + adapter.set_integer_if_greater(:sync_version, 42) + expect(subject.read_integer(:sync_version)).to eq(42) + end + + it 'raises WriteAttempted on set_integer_if_greater' do + expect { subject.set_integer_if_greater(:sync_version, 100) } + .to raise_error(Flipper::Adapters::ReadOnly::WriteAttempted) + expect(adapter.read_integer(:sync_version)).to be_nil + end end diff --git a/spec/flipper/adapters/redis_spec.rb b/spec/flipper/adapters/redis_spec.rb index e1852f324..1352570fe 100644 --- a/spec/flipper/adapters/redis_spec.rb +++ b/spec/flipper/adapters/redis_spec.rb @@ -18,6 +18,42 @@ it_should_behave_like 'a flipper adapter' + describe 'read_integer / set_integer_if_greater' do + it 'returns nil for unknown keys' do + expect(subject.read_integer(:sync_version)).to be_nil + end + + it 'sets a new value when none exists' do + expect(subject.set_integer_if_greater(:sync_version, 100)).to eq(true) + expect(subject.read_integer(:sync_version)).to eq(100) + end + + it 'rejects a lower value' do + subject.set_integer_if_greater(:sync_version, 100) + expect(subject.set_integer_if_greater(:sync_version, 99)).to eq(false) + expect(subject.read_integer(:sync_version)).to eq(100) + end + + it 'rejects an equal value' do + subject.set_integer_if_greater(:sync_version, 100) + expect(subject.set_integer_if_greater(:sync_version, 100)).to eq(false) + expect(subject.read_integer(:sync_version)).to eq(100) + end + + it 'accepts a strictly greater value' do + subject.set_integer_if_greater(:sync_version, 100) + expect(subject.set_integer_if_greater(:sync_version, 200)).to eq(true) + expect(subject.read_integer(:sync_version)).to eq(200) + end + + it 'namespaces under key_prefix' do + prefixed = described_class.new(client, key_prefix: 'lockbox:') + prefixed.set_integer_if_greater(:sync_version, 100) + expect(client.get('lockbox:flipper_int:sync_version')).to eq('100') + expect(client.get('flipper_int:sync_version')).to be_nil + end + end + it 'configures itself on load' do Flipper.configuration = nil Flipper.instance = nil diff --git a/spec/flipper/adapters/sequel_spec.rb b/spec/flipper/adapters/sequel_spec.rb index 11744f532..fe627222b 100644 --- a/spec/flipper/adapters/sequel_spec.rb +++ b/spec/flipper/adapters/sequel_spec.rb @@ -14,11 +14,13 @@ let(:feature_class) { Flipper::Adapters::Sequel::Feature } let(:gate_class) { Flipper::Adapters::Sequel::Gate } + let(:kv_integer_class) { Flipper::Adapters::Sequel::KvInteger } before(:each) do CreateFlipperTablesSequel.new(Sequel::Model.db).up feature_class.dataset = feature_class.dataset gate_class.dataset = gate_class.dataset + kv_integer_class.dataset = kv_integer_class.dataset end after(:each) do @@ -27,6 +29,58 @@ it_should_behave_like 'a flipper adapter' + describe 'read_integer / set_integer_if_greater' do + it 'returns nil for unknown keys' do + expect(subject.read_integer(:sync_version)).to be_nil + end + + it 'sets a new value when none exists' do + expect(subject.set_integer_if_greater(:sync_version, 100)).to eq(true) + expect(subject.read_integer(:sync_version)).to eq(100) + end + + it 'rejects a lower value' do + subject.set_integer_if_greater(:sync_version, 100) + expect(subject.set_integer_if_greater(:sync_version, 99)).to eq(false) + expect(subject.read_integer(:sync_version)).to eq(100) + end + + it 'rejects an equal value' do + subject.set_integer_if_greater(:sync_version, 100) + expect(subject.set_integer_if_greater(:sync_version, 100)).to eq(false) + expect(subject.read_integer(:sync_version)).to eq(100) + end + + it 'accepts a strictly greater value' do + subject.set_integer_if_greater(:sync_version, 100) + expect(subject.set_integer_if_greater(:sync_version, 200)).to eq(true) + expect(subject.read_integer(:sync_version)).to eq(200) + end + + it 'tracks separate keys independently' do + subject.set_integer_if_greater(:foo, 100) + subject.set_integer_if_greater(:bar, 50) + expect(subject.read_integer(:foo)).to eq(100) + expect(subject.read_integer(:bar)).to eq(50) + end + + it 'recovers from a transient DatabaseError on the table presence check' do + fresh = described_class.new(feature_class: feature_class, gate_class: gate_class) + kv_class = fresh.instance_variable_get(:@kv_integer_class) + + call_count = 0 + allow(kv_class.db).to receive(:table_exists?).and_wrap_original do |original, *args| + call_count += 1 + raise ::Sequel::DatabaseError, 'transient blip' if call_count == 1 + original.call(*args) + end + + expect(fresh.read_integer(:sync_version)).to be_nil + expect(fresh.set_integer_if_greater(:sync_version, 100)).to eq(true) + expect(fresh.read_integer(:sync_version)).to eq(100) + end + end + context 'requiring "flipper-sequel"' do before do Flipper.configuration = nil diff --git a/spec/flipper/adapters/sync/synchronizer_spec.rb b/spec/flipper/adapters/sync/synchronizer_spec.rb index 6e6c19a99..0fbbc9443 100644 --- a/spec/flipper/adapters/sync/synchronizer_spec.rb +++ b/spec/flipper/adapters/sync/synchronizer_spec.rb @@ -126,6 +126,163 @@ end end + describe 'sync_version gating' do + it 'skips sync when remote version is not strictly greater than local version' do + local.set_integer_if_greater(:sync_version, 100) + allow(remote).to receive(:read_integer).with(:sync_version).and_return(99) + remote_flipper.enable(:search) + + subject.call + + expect(local_flipper.features.map(&:key)).to eq([]) + end + + it 'skips sync when remote version equals local version' do + local.set_integer_if_greater(:sync_version, 100) + allow(remote).to receive(:read_integer).with(:sync_version).and_return(100) + remote_flipper.enable(:search) + + subject.call + + expect(local_flipper.features.map(&:key)).to eq([]) + end + + it 'syncs and bumps local version when remote version is strictly greater' do + local.set_integer_if_greater(:sync_version, 99) + allow(remote).to receive(:read_integer).with(:sync_version).and_return(100) + remote_flipper.enable(:search) + + subject.call + + expect(local_flipper.features.map(&:key)).to eq(["search"]) + expect(local.read_integer(:sync_version)).to eq(100) + end + + it 'syncs normally when remote returns nil version (older server)' do + allow(remote).to receive(:read_integer).with(:sync_version).and_return(nil) + remote_flipper.enable(:search) + + subject.call + + expect(local_flipper.features.map(&:key)).to eq(["search"]) + end + + it 'syncs normally when local has no stored version yet' do + allow(remote).to receive(:read_integer).with(:sync_version).and_return(100) + remote_flipper.enable(:search) + + subject.call + + expect(local_flipper.features.map(&:key)).to eq(["search"]) + expect(local.read_integer(:sync_version)).to eq(100) + end + + it 'instruments synchronizer_outvoted.flipper when a concurrent writer left local at a higher version' do + local.set_integer_if_greater(:sync_version, 50) + allow(remote).to receive(:read_integer).with(:sync_version).and_return(100) + remote_flipper.enable(:search) + original_set_integer_if_greater = local.method(:set_integer_if_greater) + allow(local).to receive(:set_integer_if_greater) do |key, value| + if key == :sync_version && value == 100 + original_set_integer_if_greater.call(:sync_version, 200) + false + else + original_set_integer_if_greater.call(key, value) + end + end + allow(remote).to receive(:get_all).and_return({}) + + subject.call + + events = instrumenter.events_by_name("synchronizer_outvoted.flipper") + expect(events.size).to eq(1) + expect(events.first.payload[:remote_version]).to eq(100) + end + + it 'repairs local gates when an older sync is outvoted after applying its snapshot' do + old_remote = Flipper::Adapters::Memory.new + Flipper.new(old_remote).enable(:search) + old_snapshot = old_remote.get_all + + new_remote = Flipper::Adapters::Memory.new + new_flipper = Flipper.new(new_remote) + new_flipper.add(:search) + new_flipper.disable(:search) + new_snapshot = new_remote.get_all + + local.set_integer_if_greater(:sync_version, 50) + original_set_integer_if_greater = local.method(:set_integer_if_greater) + + expect(remote).to receive(:get_all).with(cache_bust: false).ordered.and_return(old_snapshot) + expect(remote).to receive(:get_all).with(cache_bust: true).ordered.and_return(new_snapshot) + allow(remote).to receive(:read_integer).with(:sync_version).and_return(100, 200) + allow(local).to receive(:set_integer_if_greater) do |key, value| + if key == :sync_version && value == 100 + original_set_integer_if_greater.call(:sync_version, 200) + false + else + original_set_integer_if_greater.call(key, value) + end + end + + subject.call + + expect(local_flipper[:search].boolean_value).to eq(false) + expect(local.read_integer(:sync_version)).to eq(200) + end + + it 'does not instrument synchronizer_outvoted.flipper when local has no prior version' do + allow(remote).to receive(:read_integer).with(:sync_version).and_return(100) + remote_flipper.enable(:search) + allow(local).to receive(:set_integer_if_greater).with(:sync_version, 100).and_return(false) + + subject.call + + expect(instrumenter.events_by_name("synchronizer_outvoted.flipper")).to be_empty + end + + it 'repairs local gates when a cold-start sync is outvoted by a concurrent newer sync' do + old_remote = Flipper::Adapters::Memory.new + Flipper.new(old_remote).enable(:search) + old_snapshot = old_remote.get_all + + new_remote = Flipper::Adapters::Memory.new + new_flipper = Flipper.new(new_remote) + new_flipper.add(:search) + new_flipper.disable(:search) + new_snapshot = new_remote.get_all + + original_set_integer_if_greater = local.method(:set_integer_if_greater) + + expect(remote).to receive(:get_all).with(cache_bust: false).ordered.and_return(old_snapshot) + expect(remote).to receive(:get_all).with(cache_bust: true).ordered.and_return(new_snapshot) + allow(remote).to receive(:read_integer).with(:sync_version).and_return(100, 200) + allow(local).to receive(:set_integer_if_greater) do |key, value| + if key == :sync_version && value == 100 + original_set_integer_if_greater.call(:sync_version, 200) + false + else + original_set_integer_if_greater.call(key, value) + end + end + + subject.call + + expect(local_flipper[:search].boolean_value).to eq(false) + expect(local.read_integer(:sync_version)).to eq(200) + expect(instrumenter.events_by_name("synchronizer_outvoted.flipper").size).to eq(1) + end + + it 'skips local get_all and writes when remote version is not newer' do + local.set_integer_if_greater(:sync_version, 100) + allow(remote).to receive(:read_integer).with(:sync_version).and_return(100) + expect(local).not_to receive(:get_all) + expect(remote).to receive(:get_all).and_call_original + + subject.call + end + end + context 'with ActorLimit adapter wrapping local' do let(:limit) { 10 } let(:limited_local) { Flipper::Adapters::ActorLimit.new(local, limit) } diff --git a/spec/flipper/adapters/sync_spec.rb b/spec/flipper/adapters/sync_spec.rb index 98d7a2fa8..94735d7e8 100644 --- a/spec/flipper/adapters/sync_spec.rb +++ b/spec/flipper/adapters/sync_spec.rb @@ -198,6 +198,15 @@ expect { subject.get_all }.not_to raise_error end + describe '#read_integer / #set_integer_if_greater' do + it 'reads from local and writes to both adapters' do + expect(subject.set_integer_if_greater(:sync_version, 42)).to eq(true) + expect(subject.read_integer(:sync_version)).to eq(42) + expect(local_adapter.read_integer(:sync_version)).to eq(42) + expect(remote_adapter.read_integer(:sync_version)).to eq(42) + end + end + describe '#adapter_stack' do it 'returns the tree representation' do expect(subject.adapter_stack).to eq("sync(local: operation_logger -> memory, remote: operation_logger -> memory)") diff --git a/spec/flipper/api/v1/actions/features_spec.rb b/spec/flipper/api/v1/actions/features_spec.rb index ee69e4af0..9f2bfb2b4 100644 --- a/spec/flipper/api/v1/actions/features_spec.rb +++ b/spec/flipper/api/v1/actions/features_spec.rb @@ -259,4 +259,5 @@ end end end + end diff --git a/test/adapters/active_record_test.rb b/test/adapters/active_record_test.rb index 0b3a031ba..5084c234e 100644 --- a/test/adapters/active_record_test.rb +++ b/test/adapters/active_record_test.rb @@ -51,15 +51,18 @@ def test_models_honor_table_name_prefixes_and_suffixes Flipper::Adapters::ActiveRecord.send(:remove_const, :Model) if Flipper::Adapters::ActiveRecord.const_defined?(:Model) Flipper::Adapters::ActiveRecord.send(:remove_const, :Feature) if Flipper::Adapters::ActiveRecord.const_defined?(:Feature) Flipper::Adapters::ActiveRecord.send(:remove_const, :Gate) if Flipper::Adapters::ActiveRecord.const_defined?(:Gate) + Flipper::Adapters::ActiveRecord.send(:remove_const, :KvInteger) if Flipper::Adapters::ActiveRecord.const_defined?(:KvInteger) Flipper::Adapters.send(:remove_const, :ActiveRecord) load("flipper/adapters/active_record.rb") load("flipper/adapters/active_record/model.rb") load("flipper/adapters/active_record/feature.rb") load("flipper/adapters/active_record/gate.rb") + load("flipper/adapters/active_record/kv_integer.rb") assert_equal "foo_flipper_features_bar", Flipper::Adapters::ActiveRecord::Feature.table_name assert_equal "foo_flipper_gates_bar", Flipper::Adapters::ActiveRecord::Gate.table_name + assert_equal "foo_flipper_kv_integers_bar", Flipper::Adapters::ActiveRecord::KvInteger.table_name ensure ActiveRecord::Base.table_name_prefix = "" @@ -69,11 +72,13 @@ def test_models_honor_table_name_prefixes_and_suffixes Flipper::Adapters::ActiveRecord.send(:remove_const, :Model) if Flipper::Adapters::ActiveRecord.const_defined?(:Model) Flipper::Adapters::ActiveRecord.send(:remove_const, :Feature) if Flipper::Adapters::ActiveRecord.const_defined?(:Feature) Flipper::Adapters::ActiveRecord.send(:remove_const, :Gate) if Flipper::Adapters::ActiveRecord.const_defined?(:Gate) + Flipper::Adapters::ActiveRecord.send(:remove_const, :KvInteger) if Flipper::Adapters::ActiveRecord.const_defined?(:KvInteger) Flipper::Adapters.send(:remove_const, :ActiveRecord) load("flipper/adapters/active_record.rb") load("flipper/adapters/active_record/model.rb") load("flipper/adapters/active_record/feature.rb") load("flipper/adapters/active_record/gate.rb") + load("flipper/adapters/active_record/kv_integer.rb") end end diff --git a/test_rails/generators/flipper/active_record_generator_test.rb b/test_rails/generators/flipper/active_record_generator_test.rb index 2ad7fab87..57fa19ecb 100644 --- a/test_rails/generators/flipper/active_record_generator_test.rb +++ b/test_rails/generators/flipper/active_record_generator_test.rb @@ -29,9 +29,17 @@ def up t.timestamps null: false end add_index :flipper_gates, [:feature_key, :key, :value], unique: true, length: { value: 255 } + + create_table :flipper_kv_integers do |t| + t.string :key, null: false + t.bigint :value, null: false, default: 0 + t.timestamps null: false + end + add_index :flipper_kv_integers, :key, unique: true end def down + drop_table :flipper_kv_integers drop_table :flipper_gates drop_table :flipper_features end