From 530d6316342755726ad8d339dd51fbc3fd906044 Mon Sep 17 00:00:00 2001 From: Shinya Maeda Date: Thu, 10 Oct 2019 13:48:52 +0700 Subject: [PATCH] Introduce Feature Flag enable/disable API This commit introduces enable/disable endpoints for Feature Flag API. --- ...roduce-feature-flag-api-enable-disable.yml | 5 + ee/app/services/feature_flags/base_service.rb | 14 ++ .../services/feature_flags/disable_service.rb | 46 ++++++ .../services/feature_flags/enable_service.rb | 93 +++++++++++ ee/lib/api/feature_flags.rb | 38 +++++ ee/spec/requests/api/feature_flags_spec.rb | 153 +++++++++++++++++ .../feature_flags/disable_service_spec.rb | 92 +++++++++++ .../feature_flags/enable_service_spec.rb | 154 ++++++++++++++++++ 8 files changed, 595 insertions(+) create mode 100644 changelogs/unreleased/introduce-feature-flag-api-enable-disable.yml create mode 100644 ee/app/services/feature_flags/disable_service.rb create mode 100644 ee/app/services/feature_flags/enable_service.rb create mode 100644 ee/spec/services/feature_flags/disable_service_spec.rb create mode 100644 ee/spec/services/feature_flags/enable_service_spec.rb diff --git a/changelogs/unreleased/introduce-feature-flag-api-enable-disable.yml b/changelogs/unreleased/introduce-feature-flag-api-enable-disable.yml new file mode 100644 index 00000000000000..1c60b87d7b283c --- /dev/null +++ b/changelogs/unreleased/introduce-feature-flag-api-enable-disable.yml @@ -0,0 +1,5 @@ +--- +title: Support Enable/Disable operations in Feature Flag API +merge_request: 18368 +author: +type: added diff --git a/ee/app/services/feature_flags/base_service.rb b/ee/app/services/feature_flags/base_service.rb index 44167e2cff6d51..69b3c92f6b0d7f 100644 --- a/ee/app/services/feature_flags/base_service.rb +++ b/ee/app/services/feature_flags/base_service.rb @@ -2,6 +2,8 @@ module FeatureFlags class BaseService < ::BaseService + include Gitlab::Utils::StrongMemoize + AUDITABLE_ATTRIBUTES = %w(name description).freeze protected @@ -37,5 +39,17 @@ def created_scope_message(scope) "and set it as #{scope.active ? "active" : "inactive"} "\ "with strategies #{scope.strategies}." end + + def feature_flag_by_name + strong_memoize(:feature_flag_by_name) do + project.operations_feature_flags.find_by_name(params[:name]) + end + end + + def feature_flag_scope_by_environment_scope + strong_memoize(:feature_flag_scope_by_environment_scope) do + feature_flag_by_name.scopes.find_by_environment_scope(params[:environment_scope]) + end + end end end diff --git a/ee/app/services/feature_flags/disable_service.rb b/ee/app/services/feature_flags/disable_service.rb new file mode 100644 index 00000000000000..8a443ac1795058 --- /dev/null +++ b/ee/app/services/feature_flags/disable_service.rb @@ -0,0 +1,46 @@ +# frozen_string_literal: true + +module FeatureFlags + class DisableService < BaseService + def execute + return error('Feature Flag not found', 404) unless feature_flag_by_name + return error('Feature Flag Scope not found', 404) unless feature_flag_scope_by_environment_scope + return error('Strategy not found', 404) unless strategy_exist_in_persisted_data? + + ::FeatureFlags::UpdateService + .new(project, current_user, update_params) + .execute(feature_flag_by_name) + end + + private + + def update_params + if remaining_strategies.empty? + params_to_destroy_scope + else + params_to_update_scope + end + end + + def remaining_strategies + strong_memoize(:remaining_strategies) do + feature_flag_scope_by_environment_scope.strategies.reject do |strategy| + strategy['name'] == params[:strategy]['name'] && + strategy['parameters'] == params[:strategy]['parameters'] + end + end + end + + def strategy_exist_in_persisted_data? + feature_flag_scope_by_environment_scope.strategies != remaining_strategies + end + + def params_to_destroy_scope + { scopes_attributes: [{ id: feature_flag_scope_by_environment_scope.id, _destroy: true }] } + end + + def params_to_update_scope + { scopes_attributes: [{ id: feature_flag_scope_by_environment_scope.id, strategies: remaining_strategies }] } + end + end +end diff --git a/ee/app/services/feature_flags/enable_service.rb b/ee/app/services/feature_flags/enable_service.rb new file mode 100644 index 00000000000000..b4cbb32e0037c2 --- /dev/null +++ b/ee/app/services/feature_flags/enable_service.rb @@ -0,0 +1,93 @@ +# frozen_string_literal: true + +module FeatureFlags + class EnableService < BaseService + def execute + if feature_flag_by_name + update_feature_flag + else + create_feature_flag + end + end + + private + + def create_feature_flag + ::FeatureFlags::CreateService + .new(project, current_user, create_params) + .execute + end + + def update_feature_flag + ::FeatureFlags::UpdateService + .new(project, current_user, update_params) + .execute(feature_flag_by_name) + end + + def create_params + if params[:environment_scope] == '*' + params_to_create_flag_with_default_scope + else + params_to_create_flag_with_additional_scope + end + end + + def update_params + if feature_flag_scope_by_environment_scope + params_to_update_scope + else + params_to_create_scope + end + end + + def params_to_create_flag_with_default_scope + { + name: params[:name], + scopes_attributes: [ + { + active: true, + environment_scope: '*', + strategies: [params[:strategy]] + } + ] + } + end + + def params_to_create_flag_with_additional_scope + { + name: params[:name], + scopes_attributes: [ + { + active: false, + environment_scope: '*' + }, + { + active: true, + environment_scope: params[:environment_scope], + strategies: [params[:strategy]] + } + ] + } + end + + def params_to_create_scope + { + scopes_attributes: [{ + active: true, + environment_scope: params[:environment_scope], + strategies: [params[:strategy]] + }] + } + end + + def params_to_update_scope + { + scopes_attributes: [{ + id: feature_flag_scope_by_environment_scope.id, + active: true, + strategies: feature_flag_scope_by_environment_scope.strategies | [params[:strategy]] + }] + } + end + end +end diff --git a/ee/lib/api/feature_flags.rb b/ee/lib/api/feature_flags.rb index f0c577bcb2c366..685eaf2fcbc374 100644 --- a/ee/lib/api/feature_flags.rb +++ b/ee/lib/api/feature_flags.rb @@ -76,6 +76,44 @@ class FeatureFlags < Grape::API present feature_flag, with: EE::API::Entities::FeatureFlag end + desc 'Enable a strategy for a feature flag on an environment' do + success EE::API::Entities::FeatureFlag + end + params do + requires :environment_scope, type: String, desc: 'The environment scope of the feature flag' + requires :strategy, type: JSON, desc: 'The strategy to be enabled on the scope' + end + post :enable do + result = ::FeatureFlags::EnableService + .new(user_project, current_user, params).execute + + if result[:status] == :success + status :ok + present result[:feature_flag], with: EE::API::Entities::FeatureFlag + else + render_api_error!(result[:message], result[:http_status]) + end + end + + desc 'Disable a strategy for a feature flag on an environment' do + success EE::API::Entities::FeatureFlag + end + params do + requires :environment_scope, type: String, desc: 'The environment scope of the feature flag' + requires :strategy, type: JSON, desc: 'The strategy to be disabled on the scope' + end + post :disable do + result = ::FeatureFlags::DisableService + .new(user_project, current_user, params).execute + + if result[:status] == :success + status :ok + present result[:feature_flag], with: EE::API::Entities::FeatureFlag + else + render_api_error!(result[:message], result[:http_status]) + end + end + desc 'Delete a feature flag' do success EE::API::Entities::FeatureFlag end diff --git a/ee/spec/requests/api/feature_flags_spec.rb b/ee/spec/requests/api/feature_flags_spec.rb index 6f1cad6b2443d4..a623e7a7038eb3 100644 --- a/ee/spec/requests/api/feature_flags_spec.rb +++ b/ee/spec/requests/api/feature_flags_spec.rb @@ -186,6 +186,159 @@ def default_scope end end + describe 'POST /projects/:id/feature_flags/:name/enable' do + subject do + post api("/projects/#{project.id}/feature_flags/#{params[:name]}/enable", user), + params: params + end + + let(:params) do + { + name: 'awesome-feature', + environment_scope: 'production', + strategy: { name: 'userWithId', parameters: { userIds: 'Project:1' } }.to_json + } + end + + context 'when feature flag does not exist yet' do + it 'creates a new feature flag with the specified scope and strategy' do + subject + + feature_flag = project.operations_feature_flags.last + scope = feature_flag.scopes.find_by_environment_scope(params[:environment_scope]) + expect(response).to have_gitlab_http_status(:ok) + expect(response).to match_response_schema('public_api/v4/feature_flag', dir: 'ee') + expect(feature_flag.name).to eq(params[:name]) + expect(scope.strategies).to eq([JSON.parse(params[:strategy])]) + end + + it_behaves_like 'check user permission' + end + + context 'when feature flag exists already' do + let!(:feature_flag) { create_flag(project, params[:name]) } + + context 'when feature flag scope does not exist yet' do + it 'creates a new scope with the specified strategy' do + subject + + scope = feature_flag.scopes.find_by_environment_scope(params[:environment_scope]) + expect(response).to have_gitlab_http_status(:ok) + expect(scope.strategies).to eq([JSON.parse(params[:strategy])]) + end + + it_behaves_like 'check user permission' + end + + context 'when feature flag scope exists already' do + let(:defined_strategy) { { name: 'userWithId', parameters: { userIds: 'Project:2' } } } + + before do + create_scope(feature_flag, params[:environment_scope], true, [defined_strategy]) + end + + it 'adds an additional strategy to the scope' do + subject + + scope = feature_flag.scopes.find_by_environment_scope(params[:environment_scope]) + expect(response).to have_gitlab_http_status(:ok) + expect(scope.strategies).to eq([defined_strategy.deep_stringify_keys, JSON.parse(params[:strategy])]) + end + + context 'when the specified strategy exists already' do + let(:defined_strategy) { JSON.parse(params[:strategy]) } + + it 'does not add a duplicate strategy' do + subject + + scope = feature_flag.scopes.find_by_environment_scope(params[:environment_scope]) + strategy_count = scope.strategies.select { |strategy| strategy['name'] == 'userWithId' }.count + expect(response).to have_gitlab_http_status(:ok) + expect(strategy_count).to eq(1) + end + end + end + end + end + + describe 'POST /projects/:id/feature_flags/:name/disable' do + subject do + post api("/projects/#{project.id}/feature_flags/#{params[:name]}/disable", user), + params: params + end + + let(:params) do + { + name: 'awesome-feature', + environment_scope: 'production', + strategy: { name: 'userWithId', parameters: { userIds: 'Project:1' } }.to_json + } + end + + context 'when feature flag does not exist yet' do + it_behaves_like 'not found' + end + + context 'when feature flag exists already' do + let!(:feature_flag) { create_flag(project, params[:name]) } + + context 'when feature flag scope does not exist yet' do + it_behaves_like 'not found' + end + + context 'when feature flag scope exists already and has the specified strategy' do + let(:defined_strategies) do + [ + { name: 'userWithId', parameters: { userIds: 'Project:1' } }, + { name: 'userWithId', parameters: { userIds: 'Project:2' } } + ] + end + + before do + create_scope(feature_flag, params[:environment_scope], true, defined_strategies) + end + + it 'removes the strategy from the scope' do + subject + + scope = feature_flag.scopes.find_by_environment_scope(params[:environment_scope]) + expect(response).to have_gitlab_http_status(:ok) + expect(response).to match_response_schema('public_api/v4/feature_flag', dir: 'ee') + expect(scope.strategies) + .to eq([{ name: 'userWithId', parameters: { userIds: 'Project:2' } }.deep_stringify_keys]) + end + + it_behaves_like 'check user permission' + + context 'when strategies become empty array after the removal' do + let(:defined_strategies) do + [{ name: 'userWithId', parameters: { userIds: 'Project:1' } }] + end + + it 'destroys the scope' do + subject + + scope = feature_flag.scopes.find_by_environment_scope(params[:environment_scope]) + expect(response).to have_gitlab_http_status(:ok) + expect(scope).to be_nil + end + + it_behaves_like 'check user permission' + end + end + + context 'when scope exists already but cannot find the corresponding strategy' do + let(:defined_strategy) { { name: 'userWithId', parameters: { userIds: 'Project:2' } } } + + before do + create_scope(feature_flag, params[:environment_scope], true, [defined_strategy]) + end + + it_behaves_like 'not found' + end + end + end + describe 'DELETE /projects/:id/feature_flags/:name' do subject do delete api("/projects/#{project.id}/feature_flags/#{feature_flag.name}", user), diff --git a/ee/spec/services/feature_flags/disable_service_spec.rb b/ee/spec/services/feature_flags/disable_service_spec.rb new file mode 100644 index 00000000000000..5f9b9758753125 --- /dev/null +++ b/ee/spec/services/feature_flags/disable_service_spec.rb @@ -0,0 +1,92 @@ +# frozen_string_literal: true + +require 'spec_helper' + +describe FeatureFlags::DisableService do + include FeatureFlagHelpers + + let_it_be(:user) { create(:user) } + let(:project) { create(:project) } + let(:service) { described_class.new(project, user, params) } + let(:params) { {} } + + before do + stub_licensed_features(feature_flags: true) + project.add_developer(user) + end + + describe '#execute' do + subject { service.execute } + + context 'with params to disable default strategy on prd scope' do + let(:params) do + { + name: 'awesome', + environment_scope: 'prd', + strategy: { name: 'userWithId', parameters: { 'userIds': 'User:1' } }.deep_stringify_keys + } + end + + context 'when there is a persisted feature flag' do + let!(:feature_flag) { create_flag(project, params[:name]) } + + context 'when there is a persisted scope' do + let!(:scope) do + create_scope(feature_flag, params[:environment_scope], true, strategies) + end + + context 'when there is a persisted strategy' do + let(:strategies) do + [ + { name: 'userWithId', parameters: { 'userIds': 'User:1' } }.deep_stringify_keys, + { name: 'userWithId', parameters: { 'userIds': 'User:2' } }.deep_stringify_keys + ] + end + + it 'deletes the specified strategy' do + subject + + scope.reload + expect(scope.strategies.count).to eq(1) + expect(scope.strategies).not_to include(params[:strategy]) + end + + context 'when strategies will be empty' do + let(:strategies) { [params[:strategy]] } + + it 'deletes the persisted scope' do + subject + + expect(feature_flag.scopes.exists?(environment_scope: params[:environment_scope])) + .to eq(false) + end + end + end + + context 'when there is no persisted strategy' do + let(:strategies) { [{ name: 'default', parameters: {} }] } + + it 'returns error' do + expect(subject[:status]).to eq(:error) + expect(subject[:message]).to include('Strategy not found') + end + end + end + + context 'when there is no persisted scope' do + it 'returns error' do + expect(subject[:status]).to eq(:error) + expect(subject[:message]).to include('Feature Flag Scope not found') + end + end + end + + context 'when there is no persisted feature flag' do + it 'returns error' do + expect(subject[:status]).to eq(:error) + expect(subject[:message]).to include('Feature Flag not found') + end + end + end + end +end diff --git a/ee/spec/services/feature_flags/enable_service_spec.rb b/ee/spec/services/feature_flags/enable_service_spec.rb new file mode 100644 index 00000000000000..a91e4144edb4f8 --- /dev/null +++ b/ee/spec/services/feature_flags/enable_service_spec.rb @@ -0,0 +1,154 @@ +# frozen_string_literal: true + +require 'spec_helper' + +describe FeatureFlags::EnableService do + include FeatureFlagHelpers + + let_it_be(:user) { create(:user) } + let(:project) { create(:project) } + let(:service) { described_class.new(project, user, params) } + let(:params) { {} } + + before do + stub_licensed_features(feature_flags: true) + project.add_developer(user) + end + + describe '#execute' do + subject { service.execute } + + context 'with params to enable default strategy on prd scope' do + let(:params) do + { + name: 'awesome', + environment_scope: 'prd', + strategy: { name: 'default', parameters: {} }.stringify_keys + } + end + + context 'when there is no persisted feature flag' do + it 'creates a new feature flag with scope' do + feature_flag = subject[:feature_flag] + scope = feature_flag.scopes.find_by_environment_scope(params[:environment_scope]) + expect(subject[:status]).to eq(:success) + expect(feature_flag.name).to eq(params[:name]) + expect(feature_flag.default_scope).not_to be_active + expect(scope).to be_active + expect(scope.strategies).to include(params[:strategy]) + end + + context 'when params include default scope' do + let(:params) do + { + name: 'awesome', + environment_scope: '*', + strategy: { name: 'userWithId', parameters: { 'userIds': 'abc' } }.deep_stringify_keys + } + end + + it 'create a new feature flag with an active default scope with the specified strategy' do + feature_flag = subject[:feature_flag] + expect(subject[:status]).to eq(:success) + expect(feature_flag.default_scope).to be_active + expect(feature_flag.default_scope.strategies).to include(params[:strategy]) + end + end + end + + context 'when there is a persisted feature flag' do + let!(:feature_flag) { create_flag(project, params[:name]) } + + context 'when there is no persisted scope' do + it 'creates a new scope for the persisted feature flag' do + feature_flag = subject[:feature_flag] + scope = feature_flag.scopes.find_by_environment_scope(params[:environment_scope]) + expect(subject[:status]).to eq(:success) + expect(feature_flag.name).to eq(params[:name]) + expect(scope).to be_active + expect(scope.strategies).to include(params[:strategy]) + end + end + + context 'when there is a persisted scope' do + let!(:feature_flag_scope) do + create_scope(feature_flag, params[:environment_scope], active, strategies) + end + + let(:active) { true } + + context 'when the persisted scope does not have the specified strategy yet' do + let(:strategies) { [{ name: 'userWithId', parameters: { 'userIds': 'abc' } }] } + + it 'adds the specified strategy to the scope' do + subject + + feature_flag_scope.reload + expect(feature_flag_scope.strategies).to include(params[:strategy]) + end + + context 'when the persisted scope is inactive' do + let(:active) { false } + + it 'reactivates the scope' do + expect { subject } + .to change { feature_flag_scope.reload.active }.from(false).to(true) + end + end + end + + context 'when the persisted scope has the specified strategy already' do + let(:strategies) { [params[:strategy]] } + + it 'does not add a duplicated strategy to the scope' do + expect { subject } + .not_to change { feature_flag_scope.reload.strategies.count } + end + end + end + end + end + + context 'when strategy is not specified in params' do + let(:params) do + { + name: 'awesome', + environment_scope: 'prd' + } + end + + it 'returns error' do + expect(subject[:status]).to eq(:error) + expect(subject[:message]).to include('Scopes strategies must be an array of strategy hashes') + end + end + + context 'when environment scope is not specified in params' do + let(:params) do + { + name: 'awesome', + strategy: { name: 'default', parameters: {} }.stringify_keys + } + end + + it 'returns error' do + expect(subject[:status]).to eq(:error) + expect(subject[:message]).to include("Scopes environment scope can't be blank") + end + end + + context 'when name is not specified in params' do + let(:params) do + { + environment_scope: 'prd', + strategy: { name: 'default', parameters: {} }.stringify_keys + } + end + + it 'returns error' do + expect(subject[:status]).to eq(:error) + expect(subject[:message]).to include("Name can't be blank") + end + end + end +end -- GitLab