Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add the possibility to fetch all features already released to a given resource #67

Merged
merged 18 commits into from
Jul 31, 2020
Merged
Show file tree
Hide file tree
Changes from 16 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
22 changes: 20 additions & 2 deletions lib/feature_flagger/control.rb
Original file line number Diff line number Diff line change
Expand Up @@ -13,15 +13,27 @@ def released?(feature_key, resource_id)
end

def release(feature_key, resource_id)
@storage.add(feature_key, resource_id)
resource_name = FeatureFlagger::Storage::RedisKeys.extract_resource_name_from_feature_key(
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should calls made to RedisKeys only be made inside the Redis Storage adapter?

Or is it something we would be able to re-use to other types of storages? In that case, I think we should rename RedisKeys to something else.

feature_key
)

@storage.add(feature_key, resource_name, resource_id)
end

def releases(resource_name, resource_id)
@storage.fetch_releases(resource_name, resource_id, RELEASED_FEATURES)
end

def release_to_all(feature_key)
@storage.add_all(RELEASED_FEATURES, feature_key)
end

def unrelease(feature_key, resource_id)
@storage.remove(feature_key, resource_id)
resource_name = FeatureFlagger::Storage::RedisKeys.extract_resource_name_from_feature_key(
feature_key
)

@storage.remove(feature_key, resource_name, resource_id)
end

def unrelease_to_all(feature_key)
Expand All @@ -40,8 +52,14 @@ def released_to_all?(feature_key)
@storage.has_value?(RELEASED_FEATURES, feature_key)
end

# DEPRECATED: this method will be removed from public api on v2.0 version.
# use instead the feature_keys method.
def search_keys(query)
@storage.search_keys(query)
end

def feature_keys
@storage.feature_keys
end
end
end
2 changes: 1 addition & 1 deletion lib/feature_flagger/manager.rb
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@ module FeatureFlagger
class Manager

def self.detached_feature_keys
persisted_features = FeatureFlagger.control.search_keys("*").to_a
persisted_features = FeatureFlagger.control.feature_keys
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

mapped_feature_keys = FeatureFlagger.config.mapped_feature_keys
persisted_features - mapped_feature_keys
end
Expand Down
5 changes: 5 additions & 0 deletions lib/feature_flagger/model.rb
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,11 @@ def unrelease(*feature_key)
FeatureFlagger.control.unrelease(feature.key, id)
end

def releases
resource_name = self.class.feature_flagger_model_settings.entity_name
FeatureFlagger.control.releases(resource_name, id)
end

private

def feature_flagger_identifier
Expand Down
84 changes: 71 additions & 13 deletions lib/feature_flagger/storage/redis.rb
Original file line number Diff line number Diff line change
@@ -1,11 +1,13 @@
require 'redis'
require 'redis-namespace'
require_relative './redis_keys'

module FeatureFlagger
module Storage
class Redis

DEFAULT_NAMESPACE = :feature_flagger
RESOURCE_PREFIX = "_r".freeze
SCAN_EACH_BATCH_SIZE = 1000.freeze

def initialize(redis)
@redis = redis
Expand All @@ -17,39 +19,95 @@ def self.default_client
new(ns)
end

def fetch_releases(resource_name, resource_id, global_key)
resource_key = resource_key(resource_name, resource_id)
@redis.sunion(resource_key, global_key)
end

def has_value?(key, value)
@redis.sismember(key, value)
end

def add(key, value)
@redis.sadd(key, value)
end
def add(feature_key, resource_name, resource_id)
resource_key = resource_key(resource_name, resource_id)

def remove(key, value)
@redis.srem(key, value)
@redis.multi do |redis|
redis.sadd(feature_key, resource_id)
redis.sadd(resource_key, feature_key)
end
end

def remove_all(global_key, key)
def remove(feature_key, resource_name, resource_id)
resource_key = resource_key(resource_name, resource_id)

@redis.multi do |redis|
redis.srem(global_key, key)
redis.del(key)
redis.srem(feature_key, resource_id)
redis.srem(resource_key, feature_key)
end
end

def remove_all(global_key, feature_key)
@redis.srem(global_key, feature_key)
remove_feature_key_from_resources(feature_key)
end

def add_all(global_key, key)
@redis.multi do |redis|
redis.sadd(global_key, key)
redis.del(key)
end
@redis.sadd(global_key, key)
remove_feature_key_from_resources(key)
end

def all_values(key)
@redis.smembers(key)
end

# DEPRECATED: this method will be removed from public api on v2.0 version.
# use instead the feature_keys method.
def search_keys(query)
@redis.scan_each(match: query)
end

def feature_keys
feature_keys = []

@redis.scan_each(match: "*") do |key|
# Reject keys related to feature responsible for return
# released features for a given account.
next if key.start_with?("#{RESOURCE_PREFIX}:")

feature_keys << key
end

feature_keys
end

private

def resource_key(resource_name, resource_id)
FeatureFlagger::Storage::RedisKeys.resource_key(
RESOURCE_PREFIX,
resource_name,
resource_id,
)
end

def remove_feature_key_from_resources(feature_key)
cursor = 0
resource_name = feature_key.split(":").first

loop do
cursor, resource_ids = @redis.sscan(feature_key, cursor, count: SCAN_EACH_BATCH_SIZE)

@redis.multi do |redis|
resource_ids.each do |resource_id|
key = resource_key(resource_name, resource_id)
redis.srem(key, feature_key)
henrich-m marked this conversation as resolved.
Show resolved Hide resolved
redis.srem(feature_key, resource_id)
end
end

break if cursor == "0"
end
end
end
end
end
21 changes: 21 additions & 0 deletions lib/feature_flagger/storage/redis_keys.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,21 @@
module FeatureFlagger
henrich-m marked this conversation as resolved.
Show resolved Hide resolved
module Storage
module RedisKeys
MINIMUM_VALID_FEATURE_PATH = 2.freeze

def self.resource_key(prefix, resource_name, resource_id)
"#{prefix}:#{resource_name}:#{resource_id}"
end

def self.extract_resource_name_from_feature_key(feature_key)
feature_paths = feature_key.split(':')

raise InvalidResourceNameError if feature_paths.size < MINIMUM_VALID_FEATURE_PATH

feature_paths.first
end

class InvalidResourceNameError < StandardError; end
end
end
end
45 changes: 26 additions & 19 deletions spec/feature_flagger/control_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -5,8 +5,9 @@ module FeatureFlagger
let(:redis) { FakeRedis::Redis.new }
let(:storage) { Storage::Redis.new(redis) }
let(:control) { Control.new(storage) }
let(:key) { 'key' }
let(:key) { 'account:email_marketing:whitelabel' }
let(:resource_id) { 'resource_id' }
let(:resource_name) { 'account' }

before do
redis.flushdb
Expand All @@ -19,19 +20,19 @@ module FeatureFlagger
it { expect(result).to be_falsey }

context 'and a feature is release to all' do
before { storage.add(FeatureFlagger::Control::RELEASED_FEATURES, key) }
before { control.release_to_all(key) }

it { expect(result).to be_truthy }
end
end

context 'when resource entity id has access to release_key' do
before { storage.add(key, resource_id) }
before { control.release(key, resource_id) }

it { expect(result).to be_truthy }

context 'and a feature is release to all' do
before { storage.add(FeatureFlagger::Control::RELEASED_FEATURES, key) }
before { control.release_to_all(key) }

it { expect(result).to be_truthy }
end
Expand All @@ -41,13 +42,21 @@ module FeatureFlagger
describe '#release' do
it 'adds resource_id to storage' do
control.release(key, resource_id)
expect(storage).to have_value(key, resource_id)
expect(control).to be_released(key, resource_id)
end
end

describe '#releases' do
it 'return all releases to a given resource' do
control.release(key, resource_id)
resource_name = 'account'
expect(control.releases(resource_name, resource_id)).to match_array(['account:email_marketing:whitelabel'])
end
end

describe '#release_to_all' do
it 'adds feature_key to storage' do
storage.add(key, 1)
storage.add(key, resource_name, 1)
control.release_to_all(key)
expect(storage).not_to have_value(key, 1)
expect(storage).to have_value(FeatureFlagger::Control::RELEASED_FEATURES, key)
Expand All @@ -56,21 +65,21 @@ module FeatureFlagger

describe '#unrelease' do
it 'removes resource_id from storage' do
storage.add(key, resource_id)
storage.add(key, resource_name, resource_id)
control.unrelease(key, resource_id)
expect(storage).not_to have_value(key, resource_id)
end
end

describe '#unrelease_to_all' do
it 'removes feature_key to storage' do
storage.add(FeatureFlagger::Control::RELEASED_FEATURES, key)
storage.add(FeatureFlagger::Control::RELEASED_FEATURES, resource_name, key)
control.unrelease_to_all(key)
expect(storage).not_to have_value(FeatureFlagger::Control::RELEASED_FEATURES, key)
end

it 'removes added resources' do
storage.add(key, 1)
storage.add(key, resource_name, 1)
control.unrelease_to_all(key)
expect(storage).not_to have_value(key, 1)
expect(storage).not_to have_value(FeatureFlagger::Control::RELEASED_FEATURES, key)
Expand All @@ -92,34 +101,32 @@ module FeatureFlagger
subject { control.released_features_to_all }

it 'returns all the values to given features' do
control.release(FeatureFlagger::Control::RELEASED_FEATURES, 'feature::name1')
control.release(FeatureFlagger::Control::RELEASED_FEATURES, 'feature::name2')
control.release(FeatureFlagger::Control::RELEASED_FEATURES, 'feature::name15')
expect(subject).to match_array %w[feature::name1 feature::name2 feature::name15]
control.release_to_all('account:feature:name1')
control.release_to_all('account:feature:name2')
control.release_to_all('account:feature:name15')
expect(subject).to match_array %w[account:feature:name1 account:feature:name2 account:feature:name15]
end
end

describe '#released_to_all?' do
let(:result) { control.released_to_all?(key) }

context 'when feature was not released to all' do
before { storage.remove(FeatureFlagger::Control::RELEASED_FEATURES, key) }

it { expect(result).to be_falsey }
end

context 'when feature was released to all' do
before { storage.add(FeatureFlagger::Control::RELEASED_FEATURES, key) }
before { storage.add_all(FeatureFlagger::Control::RELEASED_FEATURES, key) }
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

control.release_to_all

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

you're right, tests are better now

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think you missed this one :) It still using storage, IMHO this spec we should not use the storage class at all only the public API of FeatureFlagger::Control does that make sense to you? I think we can do this refactor in another time anyways :D


it { expect(result).to be_truthy }
end
end

describe '#search_keys' do
before do
storage.add('namespace:1', 1)
storage.add('namespace:2', 2)
storage.add('exclusive', 3)
storage.add('namespace:1', resource_name, 1)
storage.add('namespace:2', resource_name, 2)
storage.add('exclusive', resource_name, 3)
end

context 'without matching result' do
Expand Down
4 changes: 2 additions & 2 deletions spec/feature_flagger/manager_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -21,10 +21,10 @@ module FeatureFlagger
end

it 'returns all detached feature keys' do
expect(described_class.detached_feature_keys).to include(
expect(described_class.detached_feature_keys).to match_array([
'other_feature_flagger_dummy_class:feature_a:feature_a_1:feature_a_1_3',
'other_feature_flagger_dummy_class:feature_d'
)
])
end
end

Expand Down
7 changes: 7 additions & 0 deletions spec/feature_flagger/model_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,13 @@ def id; 14 end
subject.release(key)
end
end

describe '#releases' do
it 'calls Control#release with appropriated methods' do
expect(control).to receive(:releases).with("feature_flagger_dummy_class", subject.id)
subject.releases
end
end

describe '#unrelease' do
it 'calls Control#unrelease with appropriated methods' do
Expand Down
Loading