diff --git a/Changelog.md b/Changelog.md index 8c5087330..469c1e035 100644 --- a/Changelog.md +++ b/Changelog.md @@ -2,6 +2,22 @@ All notable changes to this project will be documented in this file. +## Unreleased + +### Additions/Changes + +* Allow multiple actors for Flipper.enabled?. Improves performance of feature flags for multiple actors and simplifies code for users of flipper. + +### Deprecations + +* `:thing` in `enabled?` instrumentation payload. Use `:actors` instead. + ```diff + ActiveSupport::Notifications.subscribe('enabled?.feature_operation.flipper') do |name, start, finish, id, payload| + - payload[:thing] + + payload[:actors] + end + ``` + ## 0.27.1 * Quick fix for missing require of "flipper/version" that was causing issues with some flipper-ui people. diff --git a/benchmark/enabled_multiple_actors_ips.rb b/benchmark/enabled_multiple_actors_ips.rb new file mode 100644 index 000000000..6485b8ad4 --- /dev/null +++ b/benchmark/enabled_multiple_actors_ips.rb @@ -0,0 +1,20 @@ +require 'bundler/setup' +require 'flipper' +require 'benchmark/ips' + +actor1 = Flipper::Actor.new("User;1") +actor2 = Flipper::Actor.new("User;2") +actor3 = Flipper::Actor.new("User;3") +actor4 = Flipper::Actor.new("User;4") +actor5 = Flipper::Actor.new("User;5") +actor6 = Flipper::Actor.new("User;6") +actor7 = Flipper::Actor.new("User;7") +actor8 = Flipper::Actor.new("User;8") + +actors = [actor1, actor2, actor3, actor4, actor5, actor6, actor7, actor8] + +Benchmark.ips do |x| + x.report("with array of actors") { Flipper.enabled?(:foo, actors) } + x.report("with multiple enabled? checks") { actors.each { |actor| Flipper.enabled?(:foo, actor) } } + x.compare! +end diff --git a/examples/active_record/internals.rb b/examples/active_record/internals.rb index 2504ba00f..cd4961fca 100644 --- a/examples/active_record/internals.rb +++ b/examples/active_record/internals.rb @@ -4,8 +4,8 @@ require 'flipper/adapters/active_record' # Register a few groups. -Flipper.register(:admins) { |thing| thing.admin? } -Flipper.register(:early_access) { |thing| thing.early_access? } +Flipper.register(:admins) { |actor| actor.admin? } +Flipper.register(:early_access) { |actor| actor.early_access? } # Create a user class that has flipper_id instance method. User = Struct.new(:flipper_id) diff --git a/examples/dsl.rb b/examples/dsl.rb index b1185e5f7..66d940cbd 100644 --- a/examples/dsl.rb +++ b/examples/dsl.rb @@ -1,7 +1,7 @@ require 'bundler/setup' require 'flipper' -# create a thing with an identifier +# create an actor with an identifier class Person < Struct.new(:id) include Flipper::Identifier end @@ -58,8 +58,8 @@ class Person < Struct.new(:id) puts Flipper.actor(responds_to_flipper_id).inspect # get an instance of an actor using an object -thing = Struct.new(:flipper_id).new(22) -puts Flipper.actor(thing).inspect +actor = Struct.new(:flipper_id).new(22) +puts Flipper.actor(actor).inspect # register a top level group admins = Flipper.register(:admins) { |actor| diff --git a/examples/mongo/internals.rb b/examples/mongo/internals.rb index e375fb894..054cf6f70 100644 --- a/examples/mongo/internals.rb +++ b/examples/mongo/internals.rb @@ -6,8 +6,8 @@ require 'flipper/adapters/mongo' # Register a few groups. -Flipper.register(:admins) { |thing| thing.admin? } -Flipper.register(:early_access) { |thing| thing.early_access? } +Flipper.register(:admins) { |actor| actor.admin? } +Flipper.register(:early_access) { |actor| actor.early_access? } # Create a user class that has flipper_id instance method. User = Struct.new(:flipper_id) diff --git a/examples/redis/internals.rb b/examples/redis/internals.rb index f4dabed3f..debf2ab85 100644 --- a/examples/redis/internals.rb +++ b/examples/redis/internals.rb @@ -6,8 +6,8 @@ client = Redis.new # Register a few groups. -Flipper.register(:admins) { |thing| thing.admin? } -Flipper.register(:early_access) { |thing| thing.early_access? } +Flipper.register(:admins) { |actor| actor.admin? } +Flipper.register(:early_access) { |actor| actor.early_access? } # Create a user class that has flipper_id instance method. User = Struct.new(:flipper_id) diff --git a/examples/redis/namespaced.rb b/examples/redis/namespaced.rb index b3050dd94..643286f64 100644 --- a/examples/redis/namespaced.rb +++ b/examples/redis/namespaced.rb @@ -12,8 +12,8 @@ flipper = Flipper.new(adapter) # Register a few groups. -Flipper.register(:admins) { |thing| thing.admin? } -Flipper.register(:early_access) { |thing| thing.early_access? } +Flipper.register(:admins) { |actor| actor.admin? } +Flipper.register(:early_access) { |actor| actor.early_access? } # Create a user class that has flipper_id instance method. User = Struct.new(:flipper_id) diff --git a/examples/sequel/internals.rb b/examples/sequel/internals.rb index 3353449fe..e3bbba040 100644 --- a/examples/sequel/internals.rb +++ b/examples/sequel/internals.rb @@ -9,8 +9,8 @@ require 'flipper/adapters/sequel' # Register a few groups. -Flipper.register(:admins) { |thing| thing.admin? } -Flipper.register(:early_access) { |thing| thing.early_access? } +Flipper.register(:admins) { |actor| actor.admin? } +Flipper.register(:early_access) { |actor| actor.early_access? } # Create a user class that has flipper_id instance method. User = Struct.new(:flipper_id) diff --git a/lib/flipper.rb b/lib/flipper.rb index fb8412a0a..928ac106f 100644 --- a/lib/flipper.rb +++ b/lib/flipper.rb @@ -72,12 +72,12 @@ def instance=(flipper) # # name - The Symbol name of the group. # block - The block that should be used to determine if the group matches a - # given thing. + # given actor. # # Examples # - # Flipper.register(:admins) { |thing| - # thing.respond_to?(:admin?) && thing.admin? + # Flipper.register(:admins) { |actor| + # actor.respond_to?(:admin?) && actor.admin? # } # # Returns a Flipper::Group. diff --git a/lib/flipper/cloud/instrumenter.rb b/lib/flipper/cloud/instrumenter.rb index 8d059111b..c3fb76dc6 100644 --- a/lib/flipper/cloud/instrumenter.rb +++ b/lib/flipper/cloud/instrumenter.rb @@ -26,10 +26,15 @@ def push(name, payload) "feature" => payload[:feature_name].to_s, "result" => payload[:result].to_s, } + if (thing = payload[:thing]) dimensions["flipper_id"] = thing.value.to_s end + if (actors = payload[:actors]) + dimensions["flipper_ids"] = actors.map { |actor| actor.value.to_s } + end + event = { type: "enabled", dimensions: dimensions, diff --git a/lib/flipper/dsl.rb b/lib/flipper/dsl.rb index 79c297076..977282550 100644 --- a/lib/flipper/dsl.rb +++ b/lib/flipper/dsl.rb @@ -237,12 +237,12 @@ def group(name) # Public: Wraps an object as a flipper actor. # - # thing - The object that you would like to wrap. + # actor - The object that you would like to wrap. # # Returns an instance of Flipper::Types::Actor. - # Raises ArgumentError if thing does not respond to `flipper_id`. - def actor(thing) - Types::Actor.new(thing) + # Raises ArgumentError if actor does not respond to `flipper_id`. + def actor(actor) + Types::Actor.new(actor) end # Public: Shortcut for getting a percentage of time instance. diff --git a/lib/flipper/errors.rb b/lib/flipper/errors.rb index 295363319..14e5753d3 100644 --- a/lib/flipper/errors.rb +++ b/lib/flipper/errors.rb @@ -2,10 +2,10 @@ module Flipper # Top level error that all other errors inherit from. class Error < StandardError; end - # Raised when gate can not be found for a thing. + # Raised when gate can not be found for an actor. class GateNotFound < Error - def initialize(thing) - super "Could not find gate for #{thing.inspect}" + def initialize(actor) + super "Could not find gate for #{actor.inspect}" end end diff --git a/lib/flipper/feature.rb b/lib/flipper/feature.rb index dd944e5b1..8ab1f1b0d 100644 --- a/lib/flipper/feature.rb +++ b/lib/flipper/feature.rb @@ -96,17 +96,19 @@ def clear instrument(:clear) { adapter.clear(self) } end - # Public: Check if a feature is enabled for a thing. + # Public: Check if a feature is enabled for zero or more actors. # # Returns true if enabled, false if not. - def enabled?(thing = nil) - thing = Types::Actor.wrap(thing) unless thing.nil? + def enabled?(*actors) + actors = actors.flatten.compact.map { |actor| Types::Actor.wrap(actor) } + actors = nil if actors.empty? - instrument(:enabled?, thing: thing) do |payload| + # thing is left for backwards compatibility + instrument(:enabled?, thing: actors&.first, actors: actors) do |payload| context = FeatureCheckContext.new( feature_name: @name, values: gate_values, - thing: thing + actors: actors ) if open_gate = gates.detect { |gate| gate.open?(context) } @@ -359,14 +361,14 @@ def gate(name) gates_hash[name.to_sym] end - # Public: Find the gate that protects a thing. + # Public: Find the gate that protects an actor. # - # thing - The object for which you would like to find a gate + # actor - The object for which you would like to find a gate # # Returns a Flipper::Gate. - # Raises Flipper::GateNotFound if no gate found for thing - def gate_for(thing) - gates.detect { |gate| gate.protects?(thing) } || raise(GateNotFound, thing) + # Raises Flipper::GateNotFound if no gate found for actor + def gate_for(actor) + gates.detect { |gate| gate.protects?(actor) } || raise(GateNotFound, actor) end private diff --git a/lib/flipper/feature_check_context.rb b/lib/flipper/feature_check_context.rb index c766bd9b2..d8fa5594a 100644 --- a/lib/flipper/feature_check_context.rb +++ b/lib/flipper/feature_check_context.rb @@ -7,13 +7,17 @@ class FeatureCheckContext # gates for the feature. attr_reader :values - # Public: The thing we want to know if a feature is enabled for. - attr_reader :thing + # Public: The actors we want to know if a feature is enabled for. + attr_reader :actors - def initialize(feature_name:, values:, thing:) + def initialize(feature_name:, values:, actors:) @feature_name = feature_name @values = values - @thing = thing + @actors = actors + end + + def actors? + !@actors.nil? && !@actors.empty? end # Public: Convenience method for groups value like Feature has. diff --git a/lib/flipper/gate.rb b/lib/flipper/gate.rb index da7af2e55..2fb613748 100644 --- a/lib/flipper/gate.rb +++ b/lib/flipper/gate.rb @@ -18,28 +18,29 @@ def data_type raise 'Not implemented' end - def enabled?(_value) + def enabled?(value) raise 'Not implemented' end - # Internal: Check if a gate is open for a thing. Implemented in subclass. + # Internal: Check if a gate is open for one or more actors. Implemented + # in subclass. # - # Returns true if gate open for thing, false if not. - def open?(_thing, _value, _options = {}) + # Returns true if gate open for any actor, false if not. + def open?(actors, value, options = {}) false end - # Internal: Check if a gate is protects a thing. Implemented in subclass. + # Internal: Check if a gate is protects an actor. Implemented in subclass. # - # Returns true if gate protects thing, false if not. - def protects?(_thing) + # Returns true if gate protects actor, false if not. + def protects?(actor) false end - # Internal: Allows gate to wrap thing using one of the supported flipper - # types so adapters always get something that responds to value. - def wrap(thing) - thing + # Internal: Allows gate to wrap actor using one of the supported flipper + # types so adapters always get someactor that responds to value. + def wrap(actor) + actor end # Public: Pretty string version for debugging. diff --git a/lib/flipper/gates/actor.rb b/lib/flipper/gates/actor.rb index 956bbbb76..ed3d84150 100644 --- a/lib/flipper/gates/actor.rb +++ b/lib/flipper/gates/actor.rb @@ -19,20 +19,23 @@ def enabled?(value) !value.empty? end - # Internal: Checks if the gate is open for a thing. + # Internal: Checks if the gate is open for an actor. # - # Returns true if gate open for thing, false if not. + # Returns true if gate open for actor, false if not. def open?(context) - return false if context.thing.nil? - context.values.actors.include?(context.thing.value) + return false unless context.actors? + + context.actors.any? do |actor| + context.values.actors.include?(actor.value) + end end - def wrap(thing) - Types::Actor.wrap(thing) + def wrap(actor) + Types::Actor.wrap(actor) end - def protects?(thing) - Types::Actor.wrappable?(thing) + def protects?(actor) + Types::Actor.wrappable?(actor) end end end diff --git a/lib/flipper/gates/group.rb b/lib/flipper/gates/group.rb index 209ca9a60..0f966b2ef 100644 --- a/lib/flipper/gates/group.rb +++ b/lib/flipper/gates/group.rb @@ -23,10 +23,12 @@ def enabled?(value) # # Returns true if gate open for thing, false if not. def open?(context) - return false if context.thing.nil? + return false unless context.actors? context.values.groups.any? do |name| - Flipper.group(name).match?(context.thing, context) + context.actors.any? do |actor| + Flipper.group(name).match?(actor, context) + end end end diff --git a/lib/flipper/gates/percentage_of_actors.rb b/lib/flipper/gates/percentage_of_actors.rb index 8be62fbc8..7fd566a18 100644 --- a/lib/flipper/gates/percentage_of_actors.rb +++ b/lib/flipper/gates/percentage_of_actors.rb @@ -26,13 +26,12 @@ def enabled?(value) SCALING_FACTOR = 1_000 private_constant :SCALING_FACTOR - # Internal: Checks if the gate is open for a thing. + # Internal: Checks if the gate is open for one or more actors. # - # Returns true if gate open for thing, false if not. + # Returns true if gate open for any actors, false if not. def open?(context) - return false if context.thing.nil? - - id = "#{context.feature_name}#{context.thing.value}" + return false unless context.actors? + id = "#{context.feature_name}#{context.actors.map(&:value).sort.join}" Zlib.crc32(id) % (100 * SCALING_FACTOR) < context.values.percentage_of_actors * SCALING_FACTOR end diff --git a/lib/flipper/identifier.rb b/lib/flipper/identifier.rb index 5b1417ef5..a7af9e5c0 100644 --- a/lib/flipper/identifier.rb +++ b/lib/flipper/identifier.rb @@ -6,8 +6,8 @@ module Flipper # end # # user = User.new(99) - # Flipper.enable :thing, user - # Flipper.enabled? :thing, user #=> true + # Flipper.enable :some_feature, user + # Flipper.enabled? :some_feature, user #=> true # module Identifier def flipper_id diff --git a/lib/flipper/instrumentation/log_subscriber.rb b/lib/flipper/instrumentation/log_subscriber.rb index df06252a9..217276c9a 100644 --- a/lib/flipper/instrumentation/log_subscriber.rb +++ b/lib/flipper/instrumentation/log_subscriber.rb @@ -10,7 +10,7 @@ class LogSubscriber < ::ActiveSupport::LogSubscriber # Example Output # # flipper[:search].enabled?(user) - # # Flipper feature(search) enabled? false (1.2ms) [ thing=... ] + # # Flipper feature(search) enabled? false (1.2ms) [ actors=... ] # # Returns nothing. def feature_operation(event) @@ -20,10 +20,14 @@ def feature_operation(event) gate_name = event.payload[:gate_name] operation = event.payload[:operation] result = event.payload[:result] - thing = event.payload[:thing] description = "Flipper feature(#{feature_name}) #{operation} #{result.inspect}" - details = "thing=#{thing.inspect}" + + details = if event.payload.key?(:actors) + "actors=#{event.payload[:actors].inspect}" + else + "thing=#{event.payload[:thing].inspect}" + end details += " gate_name=#{gate_name}" unless gate_name.nil? diff --git a/lib/flipper/instrumentation/subscriber.rb b/lib/flipper/instrumentation/subscriber.rb index 4a764ce5c..0fee54db6 100644 --- a/lib/flipper/instrumentation/subscriber.rb +++ b/lib/flipper/instrumentation/subscriber.rb @@ -45,7 +45,6 @@ def update_feature_operation_metrics gate_name = @payload[:gate_name] operation = strip_trailing_question_mark(@payload[:operation]) result = @payload[:result] - thing = @payload[:thing] update_timer "flipper.feature_operation.#{operation}" diff --git a/lib/flipper/types/actor.rb b/lib/flipper/types/actor.rb index 133bbd2ab..4e89b6b39 100644 --- a/lib/flipper/types/actor.rb +++ b/lib/flipper/types/actor.rb @@ -1,35 +1,35 @@ module Flipper module Types class Actor < Type - def self.wrappable?(thing) - return false if thing.nil? - thing.respond_to?(:flipper_id) + def self.wrappable?(actor) + return false if actor.nil? + actor.respond_to?(:flipper_id) end - attr_reader :thing + attr_reader :actor - def initialize(thing) - raise ArgumentError, 'thing cannot be nil' if thing.nil? + def initialize(actor) + raise ArgumentError, 'actor cannot be nil' if actor.nil? - unless thing.respond_to?(:flipper_id) - raise ArgumentError, "#{thing.inspect} must respond to flipper_id, but does not" + unless actor.respond_to?(:flipper_id) + raise ArgumentError, "#{actor.inspect} must respond to flipper_id, but does not" end - @thing = thing - @value = thing.flipper_id.to_s + @actor = actor + @value = actor.flipper_id.to_s end def respond_to?(*args) - super || @thing.respond_to?(*args) + super || @actor.respond_to?(*args) end if RUBY_VERSION >= '3.0' def method_missing(name, *args, **kwargs, &block) - @thing.send name, *args, **kwargs, &block + @actor.send name, *args, **kwargs, &block end else def method_missing(name, *args, &block) - @thing.send name, *args, &block + @actor.send name, *args, &block end end end diff --git a/lib/flipper/types/group.rb b/lib/flipper/types/group.rb index fcaba50ec..4f191dac1 100644 --- a/lib/flipper/types/group.rb +++ b/lib/flipper/types/group.rb @@ -16,16 +16,16 @@ def initialize(name, &block) @block = block @single_argument = call_with_no_context?(@block) else - @block = ->(_thing, _context) { false } + @block = ->(actor, context) { false } @single_argument = false end end - def match?(thing, context) + def match?(actor, context) if @single_argument - @block.call(thing) + @block.call(actor) else - @block.call(thing, context) + @block.call(actor, context) end end diff --git a/spec/flipper/dsl_spec.rb b/spec/flipper/dsl_spec.rb index 34c189a07..ee14fd305 100644 --- a/spec/flipper/dsl_spec.rb +++ b/spec/flipper/dsl_spec.rb @@ -149,12 +149,12 @@ end describe '#actor' do - context 'for a thing' do + context 'for an actor' do it 'returns actor instance' do - thing = Flipper::Actor.new(33) - actor = subject.actor(thing) - expect(actor).to be_instance_of(Flipper::Types::Actor) - expect(actor.value).to eq('33') + actor = Flipper::Actor.new(33) + flipper_actor = subject.actor(actor) + expect(flipper_actor).to be_instance_of(Flipper::Types::Actor) + expect(flipper_actor.value).to eq('33') end end diff --git a/spec/flipper/feature_check_context_spec.rb b/spec/flipper/feature_check_context_spec.rb index 07868cce0..eebc16d80 100644 --- a/spec/flipper/feature_check_context_spec.rb +++ b/spec/flipper/feature_check_context_spec.rb @@ -1,12 +1,12 @@ RSpec.describe Flipper::FeatureCheckContext do let(:feature_name) { :new_profiles } let(:values) { Flipper::GateValues.new({}) } - let(:thing) { Flipper::Actor.new('5') } + let(:actor) { Flipper::Actor.new('5') } let(:options) do { feature_name: feature_name, values: values, - thing: thing, + actors: [actor], } end @@ -14,7 +14,7 @@ instance = described_class.new(**options) expect(instance.feature_name).to eq(feature_name) expect(instance.values).to eq(values) - expect(instance.thing).to eq(thing) + expect(instance.actors).to eq([actor]) end it 'requires feature_name' do @@ -31,8 +31,8 @@ end.to raise_error(ArgumentError) end - it 'requires thing' do - options.delete(:thing) + it 'requires actors' do + options.delete(:actors) expect do described_class.new(**options) end.to raise_error(ArgumentError) diff --git a/spec/flipper/feature_spec.rb b/spec/flipper/feature_spec.rb index c3b6ad451..81903d664 100644 --- a/spec/flipper/feature_spec.rb +++ b/spec/flipper/feature_spec.rb @@ -32,6 +32,52 @@ end end + describe "#enabled?" do + context "for an actor" do + let(:actor) { Flipper::Actor.new("User;1") } + + it 'returns true if feature is enabled' do + subject.enable + expect(subject.enabled?(actor)).to be(true) + end + + it 'returns false if feature is disabled' do + subject.disable + expect(subject.enabled?(actor)).to be(false) + end + end + + context "for multiple actors" do + let(:actors) { + [ + Flipper::Actor.new("User;1"), + Flipper::Actor.new("User;2"), + Flipper::Actor.new("User;3"), + ] + } + + it 'returns true if feature is enabled' do + subject.enable + expect(subject.enabled?(actors)).to be(true) + end + + it 'returns true if feature is enabled for any actor' do + subject.enable_actor actors.first + expect(subject.enabled?(actors)).to be(true) + end + + it 'returns true if feature is enabled for any actor with multiple arguments' do + subject.enable_actor actors.last + expect(subject.enabled?(*actors)).to be(true) + end + + it 'returns false if feature is disabled for all actors' do + subject.disable + expect(subject.enabled?(actors)).to be(false) + end + end + end + describe '#to_s' do it 'returns name as string' do feature = described_class.new(:search, adapter) @@ -148,29 +194,29 @@ end it 'is recorded for enable' do - thing = Flipper::Types::Actor.new(Flipper::Actor.new('1')) - gate = subject.gate_for(thing) + actor = Flipper::Types::Actor.new(Flipper::Actor.new('1')) + gate = subject.gate_for(actor) - subject.enable(thing) + subject.enable(actor) event = instrumenter.events.last expect(event).not_to be_nil expect(event.name).to eq('feature_operation.flipper') expect(event.payload[:feature_name]).to eq(:search) expect(event.payload[:operation]).to eq(:enable) - expect(event.payload[:thing]).to eq(thing) + expect(event.payload[:thing]).to eq(actor) expect(event.payload[:result]).not_to be_nil end it 'always instruments flipper type instance for enable' do - thing = Flipper::Actor.new('1') - gate = subject.gate_for(thing) + actor = Flipper::Actor.new('1') + gate = subject.gate_for(actor) - subject.enable(thing) + subject.enable(actor) event = instrumenter.events.last expect(event).not_to be_nil - expect(event.payload[:thing]).to eq(Flipper::Types::Actor.new(thing)) + expect(event.payload[:thing]).to eq(Flipper::Types::Actor.new(actor)) end it 'is recorded for disable' do @@ -219,15 +265,15 @@ end it 'always instruments flipper type instance for disable' do - thing = Flipper::Actor.new('1') - gate = subject.gate_for(thing) + actor = Flipper::Actor.new('1') + gate = subject.gate_for(actor) - subject.disable(thing) + subject.disable(actor) event = instrumenter.events.last expect(event).not_to be_nil expect(event.payload[:operation]).to eq(:disable) - expect(event.payload[:thing]).to eq(Flipper::Types::Actor.new(thing)) + expect(event.payload[:thing]).to eq(Flipper::Types::Actor.new(actor)) end it 'is recorded for add' do @@ -275,17 +321,15 @@ end it 'is recorded for enabled?' do - thing = Flipper::Types::Actor.new(Flipper::Actor.new('1')) - gate = subject.gate_for(thing) - - subject.enabled?(thing) + actor = Flipper::Types::Actor.new(Flipper::Actor.new('1')) + subject.enabled?(actor) event = instrumenter.events.last expect(event).not_to be_nil expect(event.name).to eq('feature_operation.flipper') expect(event.payload[:feature_name]).to eq(:search) expect(event.payload[:operation]).to eq(:enabled?) - expect(event.payload[:thing]).to eq(thing) + expect(event.payload[:actors]).to eq([actor]) expect(event.payload[:result]).to eq(false) end @@ -293,8 +337,8 @@ actor = Flipper::Types::Actor.new(user) { nil => nil, - user => actor, - actor => actor, + user => [actor], + actor => [actor], }.each do |thing, wrapped_thing| it "always instruments #{thing.inspect} as #{wrapped_thing.class} for enabled?" do subject.enabled?(thing) @@ -302,7 +346,7 @@ event = instrumenter.events.last expect(event).not_to be_nil expect(event.payload[:operation]).to eq(:enabled?) - expect(event.payload[:thing]).to eq(wrapped_thing) + expect(event.payload[:actors]).to eq(wrapped_thing) end end end @@ -428,10 +472,10 @@ context 'when one or more groups enabled' do before do - @staff = Flipper.register(:staff) { |_thing| true } - @preview_features = Flipper.register(:preview_features) { |_thing| true } - @not_enabled = Flipper.register(:not_enabled) { |_thing| true } - @disabled = Flipper.register(:disabled) { |_thing| true } + @staff = Flipper.register(:staff) { |actor| true } + @preview_features = Flipper.register(:preview_features) { |actor| true } + @not_enabled = Flipper.register(:not_enabled) { |actor| true } + @disabled = Flipper.register(:disabled) { |actor| true } subject.enable @staff subject.enable @preview_features subject.disable @disabled @@ -467,10 +511,10 @@ context 'when one or more groups enabled' do before do - @staff = Flipper.register(:staff) { |_thing| true } - @preview_features = Flipper.register(:preview_features) { |_thing| true } - @not_enabled = Flipper.register(:not_enabled) { |_thing| true } - @disabled = Flipper.register(:disabled) { |_thing| true } + @staff = Flipper.register(:staff) { |actor| true } + @preview_features = Flipper.register(:preview_features) { |actor| true } + @not_enabled = Flipper.register(:not_enabled) { |actor| true } + @disabled = Flipper.register(:disabled) { |actor| true } subject.enable @staff subject.enable @preview_features subject.disable @disabled @@ -494,10 +538,10 @@ context 'when one or more groups enabled' do before do - @staff = Flipper.register(:staff) { |_thing| true } - @preview_features = Flipper.register(:preview_features) { |_thing| true } - @not_enabled = Flipper.register(:not_enabled) { |_thing| true } - @disabled = Flipper.register(:disabled) { |_thing| true } + @staff = Flipper.register(:staff) { |actor| true } + @preview_features = Flipper.register(:preview_features) { |actor| true } + @not_enabled = Flipper.register(:not_enabled) { |actor| true } + @disabled = Flipper.register(:disabled) { |actor| true } subject.enable @staff subject.enable @preview_features subject.disable @disabled diff --git a/spec/flipper/gates/boolean_spec.rb b/spec/flipper/gates/boolean_spec.rb index f7e2c4b7c..b040039ec 100644 --- a/spec/flipper/gates/boolean_spec.rb +++ b/spec/flipper/gates/boolean_spec.rb @@ -9,7 +9,7 @@ def context(bool) Flipper::FeatureCheckContext.new( feature_name: feature_name, values: Flipper::GateValues.new(boolean: bool), - thing: Flipper::Types::Actor.new(Flipper::Actor.new(1)) + actors: [Flipper::Types::Actor.new(Flipper::Actor.new('1'))] ) end diff --git a/spec/flipper/gates/group_spec.rb b/spec/flipper/gates/group_spec.rb index ca1e088a3..973de7d11 100644 --- a/spec/flipper/gates/group_spec.rb +++ b/spec/flipper/gates/group_spec.rb @@ -9,18 +9,17 @@ def context(set) Flipper::FeatureCheckContext.new( feature_name: feature_name, values: Flipper::GateValues.new(groups: set), - thing: Flipper::Types::Actor.new(Flipper::Actor.new('5')) + actors: [Flipper::Types::Actor.new(Flipper::Actor.new('5'))] ) end describe '#open?' do context 'with a group in adapter, but not registered' do before do - Flipper.register(:staff) { |_thing| true } + Flipper.register(:staff) { |actor| true } end it 'ignores group' do - thing = Flipper::Actor.new('5') expect(subject.open?(context(Set[:newbs, :staff]))).to be(true) end end diff --git a/spec/flipper/gates/percentage_of_actors_spec.rb b/spec/flipper/gates/percentage_of_actors_spec.rb index 7fad131b4..37607bae7 100644 --- a/spec/flipper/gates/percentage_of_actors_spec.rb +++ b/spec/flipper/gates/percentage_of_actors_spec.rb @@ -5,11 +5,11 @@ described_class.new end - def context(percentage_of_actors_value, feature = feature_name, thing = nil) + def context(percentage_of_actors_value, feature = feature_name, actors = nil) Flipper::FeatureCheckContext.new( feature_name: feature, values: Flipper::GateValues.new(percentage_of_actors: percentage_of_actors_value), - thing: Flipper::Types::Actor.new(thing || Flipper::Actor.new(1)) + actors: Array(actors) || [Flipper::Types::Actor.new(Flipper::Actor.new('1'))] ) end @@ -20,7 +20,7 @@ def context(percentage_of_actors_value, feature = feature_name, thing = nil) let(:number_of_actors) { 10_000 } let(:actors) do - (1..number_of_actors).map { |n| Flipper::Actor.new(n) } + (1..number_of_actors).map { |n| Flipper::Types::Actor.new(Flipper::Actor.new(n.to_s)) } end let(:feature_one_enabled_actors) do @@ -48,13 +48,69 @@ def context(percentage_of_actors_value, feature = feature_name, thing = nil) end end + context "with an array of actors" do + let(:percentage) { 0.05 } + let(:percentage_as_integer) { percentage * 100 } + let(:number_of_actors) { 3_000 } + + let(:user_actors) do + (1..number_of_actors).map { |n| Flipper::Types::Actor.new(Flipper::Actor.new("User;#{n}")) } + end + + let(:team_actors) do + (1..number_of_actors).map { |n| Flipper::Types::Actor.new(Flipper::Actor.new("Team;#{n}")) } + end + + let(:org_actors) do + (1..number_of_actors).map { |n| Flipper::Types::Actor.new(Flipper::Actor.new("Org;#{n}")) } + end + + let(:actors) { user_actors + team_actors + org_actors } + + let(:feature_one_enabled_actors) do + actors.each_slice(3).select do |group| + context = context(percentage_as_integer, :name_one, group) + subject.open?(context) + end.flatten + end + + let(:feature_two_enabled_actors) do + actors.each_slice(3).select do |group| + context = context(percentage_as_integer, :name_two, group) + subject.open?(context) + end.flatten + end + + it 'does not enable both features for same set of actors' do + expect(feature_one_enabled_actors).not_to eq(feature_two_enabled_actors) + end + + it 'enables feature for accurate number of actors for each feature' do + margin_of_error = 0.02 * actors.size # 2 percent margin of error + expected_enabled_size = actors.size * percentage + + [ + feature_one_enabled_actors.size, + feature_two_enabled_actors.size, + ].each do |size| + expect(size).to be_within(margin_of_error).of(expected_enabled_size) + end + end + + it "is consistent regardless of order of actors" do + actors = user_actors.first(10) + results = 100.times.map { |n| subject.open?(context(75, :some_feature, actors.shuffle)) } + expect(results.uniq).to eq([true]) + end + end + context 'for fractional percentage' do let(:decimal) { 0.001 } let(:percentage) { decimal * 100 } let(:number_of_actors) { 10_000 } let(:actors) do - (1..number_of_actors).map { |n| Flipper::Actor.new(n) } + (1..number_of_actors).map { |n| Flipper::Types::Actor.new(Flipper::Actor.new(n.to_s)) } end subject { described_class.new } @@ -64,7 +120,7 @@ def context(percentage_of_actors_value, feature = feature_name, thing = nil) expected_open_count = number_of_actors * decimal open_count = actors.select do |actor| - context = context(percentage, :feature, actor) + context = context(percentage, :feature, [actor]) subject.open?(context) end.size diff --git a/spec/flipper/gates/percentage_of_time_spec.rb b/spec/flipper/gates/percentage_of_time_spec.rb index bce1f40e8..dd4ec886a 100644 --- a/spec/flipper/gates/percentage_of_time_spec.rb +++ b/spec/flipper/gates/percentage_of_time_spec.rb @@ -5,11 +5,11 @@ described_class.new end - def context(percentage_of_time_value, feature = feature_name, thing = nil) + def context(percentage_of_time_value, feature = feature_name, actors = nil) Flipper::FeatureCheckContext.new( feature_name: feature, values: Flipper::GateValues.new(percentage_of_time: percentage_of_time_value), - thing: thing || Flipper::Types::Actor.new(Flipper::Actor.new(1)) + actors: Array(actors) || [Flipper::Types::Actor.new(Flipper::Actor.new('1'))] ) end diff --git a/spec/flipper/instrumentation/log_subscriber_spec.rb b/spec/flipper/instrumentation/log_subscriber_spec.rb index 4b33f26c0..e0645e023 100644 --- a/spec/flipper/instrumentation/log_subscriber_spec.rb +++ b/spec/flipper/instrumentation/log_subscriber_spec.rb @@ -18,8 +18,8 @@ end before do - Flipper.register(:admins) do |thing| - thing.respond_to?(:admin?) && thing.admin? + Flipper.register(:admins) do |actor| + actor.respond_to?(:admin?) && actor.admin? end @io = StringIO.new @@ -46,7 +46,7 @@ it 'logs feature calls with result after operation' do feature_line = find_line('Flipper feature(search) enabled? false') - expect(feature_line).to include('[ thing=nil ]') + expect(feature_line).to include('[ actors=nil ]') end it 'logs adapter calls' do @@ -56,7 +56,7 @@ end end - context 'feature enabled checks with a thing' do + context 'feature enabled checks with an actor' do let(:user) { Flipper::Types::Actor.new(Flipper::Actor.new('1')) } before do @@ -64,7 +64,7 @@ flipper[:search].enabled?(user) end - it 'logs thing for feature' do + it 'logs actors for feature' do feature_line = find_line('Flipper feature(search) enabled?') expect(feature_line).to include(user.inspect) end diff --git a/spec/flipper/types/actor_spec.rb b/spec/flipper/types/actor_spec.rb index 7db7b1204..d6339c1f4 100644 --- a/spec/flipper/types/actor_spec.rb +++ b/spec/flipper/types/actor_spec.rb @@ -2,11 +2,11 @@ RSpec.describe Flipper::Types::Actor do subject do - thing = thing_class.new('2') - described_class.new(thing) + actor = actor_class.new('2') + described_class.new(actor) end - let(:thing_class) do + let(:actor_class) do Class.new do attr_reader :flipper_id @@ -22,14 +22,14 @@ def admin? describe '.wrappable?' do it 'returns true if actor' do - thing = thing_class.new('1') - actor = described_class.new(thing) - expect(described_class.wrappable?(actor)).to eq(true) + actor = actor_class.new('1') + actor_type_instance = described_class.new(actor) + expect(described_class.wrappable?(actor_type_instance)).to eq(true) end it 'returns true if responds to flipper_id' do - thing = thing_class.new(10) - expect(described_class.wrappable?(thing)).to eq(true) + actor = actor_class.new(10) + expect(described_class.wrappable?(actor)).to eq(true) end it 'returns false if nil' do @@ -38,27 +38,27 @@ def admin? end describe '.wrap' do - context 'for actor' do - it 'returns actor' do - actor = described_class.wrap(subject) - expect(actor).to be_instance_of(described_class) - expect(actor).to be(subject) + context 'for actor type instance' do + it 'returns actor type instance' do + actor_type_instance = described_class.wrap(subject) + expect(actor_type_instance).to be_instance_of(described_class) + expect(actor_type_instance).to be(subject) end end - context 'for other thing' do - it 'returns actor' do - thing = thing_class.new('1') - actor = described_class.wrap(thing) - expect(actor).to be_instance_of(described_class) + context 'for other object' do + it 'returns actor type instance' do + actor = actor_class.new('1') + actor_type_instance = described_class.wrap(actor) + expect(actor_type_instance).to be_instance_of(described_class) end end end - it 'initializes with thing that responds to id' do - thing = thing_class.new('1') - actor = described_class.new(thing) - expect(actor.value).to eq('1') + it 'initializes with object that responds to flipper_id' do + actor = actor_class.new('1') + actor_type_instance = described_class.new(actor) + expect(actor_type_instance.value).to eq('1') end it 'raises error when initialized with nil' do @@ -68,48 +68,48 @@ def admin? end it 'raises error when initalized with non-wrappable object' do - unwrappable_thing = Struct.new(:id).new(1) + unwrappable_object = Struct.new(:id).new(1) expect do - described_class.new(unwrappable_thing) + described_class.new(unwrappable_object) end.to raise_error(ArgumentError, - "#{unwrappable_thing.inspect} must respond to flipper_id, but does not") + "#{unwrappable_object.inspect} must respond to flipper_id, but does not") end it 'converts id to string' do - thing = thing_class.new(2) - actor = described_class.new(thing) + actor = actor_class.new(2) + actor = described_class.new(actor) expect(actor.value).to eq('2') end - it 'proxies everything to thing' do - thing = thing_class.new(10) - actor = described_class.new(thing) + it 'proxies everything to actor' do + actor = actor_class.new(10) + actor = described_class.new(actor) expect(actor.admin?).to eq(true) end - it 'exposes thing' do - thing = thing_class.new(10) - actor = described_class.new(thing) - expect(actor.thing).to be(thing) + it 'exposes actor' do + actor = actor_class.new(10) + actor_type_instance = described_class.new(actor) + expect(actor_type_instance.actor).to be(actor) end describe '#respond_to?' do it 'returns true if responds to method' do - thing = thing_class.new('1') - actor = described_class.new(thing) - expect(actor.respond_to?(:value)).to eq(true) + actor = actor_class.new('1') + actor_type_instance = described_class.new(actor) + expect(actor_type_instance.respond_to?(:value)).to eq(true) end - it 'returns true if thing responds to method' do - thing = thing_class.new(10) - actor = described_class.new(thing) - expect(actor.respond_to?(:admin?)).to eq(true) + it 'returns true if actor responds to method' do + actor = actor_class.new(10) + actor_type_instance = described_class.new(actor) + expect(actor_type_instance.respond_to?(:admin?)).to eq(true) end - it 'returns false if does not respond to method and thing does not respond to method' do - thing = thing_class.new(10) - actor = described_class.new(thing) - expect(actor.respond_to?(:frankenstein)).to eq(false) + it 'returns false if does not respond to method and actor does not respond to method' do + actor = actor_class.new(10) + actor_type_instance = described_class.new(actor) + expect(actor_type_instance.respond_to?(:frankenstein)).to eq(false) end end end diff --git a/spec/flipper/types/group_spec.rb b/spec/flipper/types/group_spec.rb index e0ca57d55..f79c5c8c9 100644 --- a/spec/flipper/types/group_spec.rb +++ b/spec/flipper/types/group_spec.rb @@ -90,7 +90,7 @@ def admin? context = Flipper::FeatureCheckContext.new( feature_name: :my_feature, values: Flipper::GateValues.new({}), - thing: Flipper::Types::Actor.new(Flipper::Actor.new(1)) + actors: [Flipper::Types::Actor.new(Flipper::Actor.new('1'))] ) group = Flipper.register(:group_with_context) { |actor| actor } yielded_actor = group.match?(admin_actor, context) @@ -101,7 +101,7 @@ def admin? context = Flipper::FeatureCheckContext.new( feature_name: :my_feature, values: Flipper::GateValues.new({}), - thing: Flipper::Types::Actor.new(Flipper::Actor.new(1)) + actors: [Flipper::Types::Actor.new(Flipper::Actor.new('1'))] ) group = Flipper.register(:group_with_context) { |actor, context| [actor, context] } yielded_actor, yielded_context = group.match?(admin_actor, context) diff --git a/spec/flipper_integration_spec.rb b/spec/flipper_integration_spec.rb index 050cb86b4..c81fbd9fa 100644 --- a/spec/flipper_integration_spec.rb +++ b/spec/flipper_integration_spec.rb @@ -7,17 +7,17 @@ let(:admin_group) { flipper.group(:admins) } let(:dev_group) { flipper.group(:devs) } - let(:admin_thing) do + let(:admin_actor) do double 'Non Flipper Thing', flipper_id: 1, admin?: true, dev?: false end - let(:dev_thing) do + let(:dev_actor) do double 'Non Flipper Thing', flipper_id: 10, admin?: false, dev?: true end - let(:admin_truthy_thing) do + let(:admin_truthy_actor) do double 'Non Flipper Thing', flipper_id: 1, admin?: 'true-ish', dev?: false end - let(:admin_falsey_thing) do + let(:admin_falsey_actor) do double 'Non Flipper Thing', flipper_id: 1, admin?: nil, dev?: false end @@ -60,20 +60,20 @@ expect(@result).to eq(true) end - it 'enables feature for non flipper thing in group' do - expect(feature.enabled?(admin_thing)).to eq(true) + it 'enables feature for non flipper actor in group' do + expect(feature.enabled?(admin_actor)).to eq(true) end - it 'does not enable feature for non flipper thing in other group' do - expect(feature.enabled?(dev_thing)).to eq(false) + it 'does not enable feature for non flipper actor in other group' do + expect(feature.enabled?(dev_actor)).to eq(false) end it 'enables feature for flipper actor in group' do - expect(feature.enabled?(flipper.actor(admin_thing))).to eq(true) + expect(feature.enabled?(flipper.actor(admin_actor))).to eq(true) end it 'does not enable for flipper actor not in group' do - expect(feature.enabled?(flipper.actor(dev_thing))).to eq(false) + expect(feature.enabled?(flipper.actor(dev_actor))).to eq(false) end it 'does not enable feature for all' do @@ -118,8 +118,8 @@ it 'enables feature for actor within percentage' do enabled = (1..100).select do |i| - thing = Flipper::Actor.new(i) - feature.enabled?(thing) + actor = Flipper::Actor.new(i) + feature.enabled?(actor) end.size expect(enabled).to be_within(2).of(5) @@ -141,8 +141,8 @@ it 'enables feature for actor within percentage' do enabled = (1..100).select do |i| - thing = Flipper::Actor.new(i) - feature.enabled?(thing) + actor = Flipper::Actor.new(i) + feature.enabled?(actor) end.size expect(enabled).to be_within(2).of(5) @@ -180,10 +180,10 @@ context 'with argument that has no gate' do it 'raises error' do - thing = Object.new + actor = Object.new expect do - feature.enable(thing) - end.to raise_error(Flipper::GateNotFound, "Could not find gate for #{thing.inspect}") + feature.enable(actor) + end.to raise_error(Flipper::GateNotFound, "Could not find gate for #{actor.inspect}") end end end @@ -215,13 +215,13 @@ end it 'disables actor in group' do - expect(feature.enabled?(admin_thing)).to eq(false) + expect(feature.enabled?(admin_actor)).to eq(false) end it 'disables actor in percentage of actors' do enabled = (1..100).select do |i| - thing = Flipper::Actor.new(i) - feature.enabled?(thing) + actor = Flipper::Actor.new(i) + feature.enabled?(actor) end.size expect(enabled).to be(0) @@ -247,20 +247,20 @@ expect(@result).to eq(true) end - it 'disables the feature for non flipper thing in the group' do - expect(feature.enabled?(admin_thing)).to eq(false) + it 'disables the feature for non flipper actor in the group' do + expect(feature.enabled?(admin_actor)).to eq(false) end - it 'does not disable feature for non flipper thing in other groups' do - expect(feature.enabled?(dev_thing)).to eq(true) + it 'does not disable feature for non flipper actor in other groups' do + expect(feature.enabled?(dev_actor)).to eq(true) end it 'disables feature for flipper actor in group' do - expect(feature.enabled?(flipper.actor(admin_thing))).to eq(false) + expect(feature.enabled?(flipper.actor(admin_actor))).to eq(false) end it 'does not disable feature for flipper actor in other groups' do - expect(feature.enabled?(flipper.actor(dev_thing))).to eq(true) + expect(feature.enabled?(flipper.actor(dev_actor))).to eq(true) end it 'adds feature to set of features' do @@ -303,8 +303,8 @@ it 'disables feature' do enabled = (1..100).select do |i| - thing = Flipper::Actor.new(i) - feature.enabled?(thing) + actor = Flipper::Actor.new(i) + feature.enabled?(actor) end.size expect(enabled).to be(0) @@ -342,10 +342,10 @@ context 'with argument that has no gate' do it 'raises error' do - thing = Object.new + actor = Object.new expect do - feature.disable(thing) - end.to raise_error(Flipper::GateNotFound, "Could not find gate for #{thing.inspect}") + feature.disable(actor) + end.to raise_error(Flipper::GateNotFound, "Could not find gate for #{actor.inspect}") end end end @@ -373,23 +373,23 @@ end it 'returns true' do - expect(feature.enabled?(flipper.actor(admin_thing))).to eq(true) - expect(feature.enabled?(admin_thing)).to eq(true) + expect(feature.enabled?(flipper.actor(admin_actor))).to eq(true) + expect(feature.enabled?(admin_actor)).to eq(true) end it 'returns true for truthy block values' do - expect(feature.enabled?(flipper.actor(admin_truthy_thing))).to eq(true) + expect(feature.enabled?(flipper.actor(admin_truthy_actor))).to eq(true) end end context 'for actor in disabled group' do it 'returns false' do - expect(feature.enabled?(flipper.actor(dev_thing))).to eq(false) - expect(feature.enabled?(dev_thing)).to eq(false) + expect(feature.enabled?(flipper.actor(dev_actor))).to eq(false) + expect(feature.enabled?(dev_actor)).to eq(false) end it 'returns false for falsey block values' do - expect(feature.enabled?(flipper.actor(admin_falsey_thing))).to eq(false) + expect(feature.enabled?(flipper.actor(admin_falsey_actor))).to eq(false) end end @@ -428,7 +428,7 @@ expect(feature.enabled?).to eq(true) expect(feature.enabled?(nil)).to eq(true) expect(feature.enabled?(pitt)).to eq(true) - expect(feature.enabled?(admin_thing)).to eq(true) + expect(feature.enabled?(admin_actor)).to eq(true) end end @@ -446,7 +446,7 @@ expect(feature.enabled?).to eq(true) expect(feature.enabled?(nil)).to eq(true) expect(feature.enabled?(pitt)).to eq(true) - expect(feature.enabled?(admin_thing)).to eq(true) + expect(feature.enabled?(admin_actor)).to eq(true) end end @@ -463,7 +463,7 @@ expect(feature.enabled?).to eq(false) expect(feature.enabled?(nil)).to eq(false) expect(feature.enabled?(pitt)).to eq(false) - expect(feature.enabled?(admin_thing)).to eq(false) + expect(feature.enabled?(admin_actor)).to eq(false) end it 'returns true if boolean enabled' do @@ -471,7 +471,7 @@ expect(feature.enabled?).to eq(true) expect(feature.enabled?(nil)).to eq(true) expect(feature.enabled?(pitt)).to eq(true) - expect(feature.enabled?(admin_thing)).to eq(true) + expect(feature.enabled?(admin_actor)).to eq(true) end end @@ -488,7 +488,7 @@ expect(feature.enabled?).to eq(false) expect(feature.enabled?(nil)).to eq(false) expect(feature.enabled?(pitt)).to eq(false) - expect(feature.enabled?(admin_thing)).to eq(false) + expect(feature.enabled?(admin_actor)).to eq(false) end it 'returns true if boolean enabled' do @@ -496,27 +496,27 @@ expect(feature.enabled?).to eq(true) expect(feature.enabled?(nil)).to eq(true) expect(feature.enabled?(pitt)).to eq(true) - expect(feature.enabled?(admin_thing)).to eq(true) + expect(feature.enabled?(admin_actor)).to eq(true) end end - context 'for a non flipper thing' do + context 'for a non flipper actor' do before do feature.enable admin_group end it 'returns true if in enabled group' do - expect(feature.enabled?(admin_thing)).to eq(true) + expect(feature.enabled?(admin_actor)).to eq(true) end it 'returns false if not in enabled group' do - expect(feature.enabled?(dev_thing)).to eq(false) + expect(feature.enabled?(dev_actor)).to eq(false) end it 'returns true if boolean enabled' do feature.enable - expect(feature.enabled?(admin_thing)).to eq(true) - expect(feature.enabled?(dev_thing)).to eq(true) + expect(feature.enabled?(admin_actor)).to eq(true) + expect(feature.enabled?(dev_actor)).to eq(true) end end end @@ -530,11 +530,11 @@ end it 'enables feature for object in enabled group' do - expect(feature.enabled?(admin_thing)).to eq(true) + expect(feature.enabled?(admin_actor)).to eq(true) end it 'does not enable feature for object in not enabled group' do - expect(feature.enabled?(dev_thing)).to eq(false) + expect(feature.enabled?(dev_actor)).to eq(false) end end end