Merge branch 'new-ff-crud-get' into 'master'

Support New Feature Flags in API for GET Requests

See merge request gitlab-org/gitlab!26331

ee/app/finders/feature_flags_finder.rb

 # frozen_string_literal: true
 
 class FeatureFlagsFinder
-  attr_reader :project, :feature_flags, :params, :current_user
+  attr_reader :project, :params, :current_user
 
   def initialize(project, current_user, params = {})
     @project = project
     @current_user = current_user
-    @feature_flags = project.operations_feature_flags
     @params = params
   end
 
@@ -24,6 +23,14 @@ class FeatureFlagsFinder
 
   private
 
+  def feature_flags
+    if Feature.enabled?(:feature_flags_new_version, project)
+      project.operations_feature_flags
+    else
+      project.operations_feature_flags.legacy_flag
+    end
+  end
+
   def by_scope(items)
     case params[:scope]
     when 'enabled'

ee/app/models/operations/feature_flag.rb

@@ -48,7 +48,7 @@ module Operations
 
     class << self
       def preload_relations
-        preload(:scopes)
+        preload(:scopes, strategies: :scopes)
       end
 
       def for_unleash_client(project, environment)

ee/lib/api/feature_flag_scopes.rb

@@ -18,13 +18,13 @@ module API
       resource :feature_flag_scopes do
         desc 'Get all effective feature flags under the environment' do
           detail 'This feature was introduced in GitLab 12.5'
-          success EE::API::Entities::FeatureFlag::DetailedScope
+          success EE::API::Entities::FeatureFlag::DetailedLegacyScope
         end
         params do
           requires :environment, type: String, desc: 'The environment name'
         end
         get do
-          present scopes_for_environment, with: EE::API::Entities::FeatureFlag::DetailedScope
+          present scopes_for_environment, with: EE::API::Entities::FeatureFlag::DetailedLegacyScope
         end
       end
 
@@ -35,18 +35,18 @@ module API
         resource :scopes do
           desc 'Get all scopes of a feature flag' do
             detail 'This feature was introduced in GitLab 12.5'
-            success EE::API::Entities::FeatureFlag::Scope
+            success EE::API::Entities::FeatureFlag::LegacyScope
           end
           params do
             use :pagination
           end
           get do
-            present paginate(feature_flag.scopes), with: EE::API::Entities::FeatureFlag::Scope
+            present paginate(feature_flag.scopes), with: EE::API::Entities::FeatureFlag::LegacyScope
           end
 
           desc 'Create a scope of a feature flag' do
             detail 'This feature was introduced in GitLab 12.5'
-            success EE::API::Entities::FeatureFlag::Scope
+            success EE::API::Entities::FeatureFlag::LegacyScope
           end
           params do
             requires :environment_scope, type: String, desc: 'The environment scope of the scope'
@@ -61,7 +61,7 @@ module API
               .execute(feature_flag)
 
             if result[:status] == :success
-              present scope, with: EE::API::Entities::FeatureFlag::Scope
+              present scope, with: EE::API::Entities::FeatureFlag::LegacyScope
             else
               render_api_error!(result[:message], result[:http_status])
             end
@@ -73,15 +73,15 @@ module API
           resource ':environment_scope', requirements: ENVIRONMENT_SCOPE_ENDPOINT_REQUIREMENTS do
             desc 'Get a scope of a feature flag' do
               detail 'This feature was introduced in GitLab 12.5'
-              success EE::API::Entities::FeatureFlag::Scope
+              success EE::API::Entities::FeatureFlag::LegacyScope
             end
             get do
-              present scope, with: EE::API::Entities::FeatureFlag::Scope
+              present scope, with: EE::API::Entities::FeatureFlag::LegacyScope
             end
 
             desc 'Update a scope of a feature flag' do
               detail 'This feature was introduced in GitLab 12.5'
-              success EE::API::Entities::FeatureFlag::Scope
+              success EE::API::Entities::FeatureFlag::LegacyScope
             end
             params do
               optional :active, type: Boolean, desc: 'Whether the scope is active'
@@ -100,7 +100,7 @@ module API
                 updated_scope = result[:feature_flag].scopes
                   .find { |scope| scope.environment_scope == params[:environment_scope] }
 
-                present updated_scope, with: EE::API::Entities::FeatureFlag::Scope
+                present updated_scope, with: EE::API::Entities::FeatureFlag::LegacyScope
               else
                 render_api_error!(result[:message], result[:http_status])
               end
@@ -108,7 +108,7 @@ module API
 
             desc 'Delete a scope from a feature flag' do
               detail 'This feature was introduced in GitLab 12.5'
-              success EE::API::Entities::FeatureFlag::Scope
+              success EE::API::Entities::FeatureFlag::LegacyScope
             end
             delete do
               authorize_update_feature_flag!

ee/lib/api/feature_flags.rb

@@ -30,7 +30,9 @@ module API
             .new(user_project, current_user, declared_params(include_missing: false))
             .execute
 
-          present paginate(feature_flags), with: EE::API::Entities::FeatureFlag
+          present paginate(feature_flags),
+            with: EE::API::Entities::FeatureFlag,
+            feature_flags_new_version_enabled: feature_flags_new_version_enabled?
         end
 
         desc 'Create a new feature flag' do
@@ -49,15 +51,17 @@ module API
         post do
           authorize_create_feature_flag!
 
-          param = declared_params(include_missing: false)
-          param[:scopes_attributes] = param.delete(:scopes) if param.key?(:scopes)
+          attrs = declared_params(include_missing: false)
+          rename_key(attrs, :scopes, :scopes_attributes)
 
           result = ::FeatureFlags::CreateService
-            .new(user_project, current_user, param)
+            .new(user_project, current_user, attrs)
             .execute
 
           if result[:status] == :success
-            present result[:feature_flag], with: EE::API::Entities::FeatureFlag
+            present result[:feature_flag],
+              with: EE::API::Entities::FeatureFlag,
+              feature_flags_new_version_enabled: feature_flags_new_version_enabled?
           else
             render_api_error!(result[:message], result[:http_status])
           end
@@ -75,7 +79,9 @@ module API
         get do
           authorize_read_feature_flag!
 
-          present feature_flag, with: EE::API::Entities::FeatureFlag
+          present feature_flag,
+            with: EE::API::Entities::FeatureFlag,
+            feature_flags_new_version_enabled: feature_flags_new_version_enabled?
         end
 
         desc 'Enable a strategy for a feature flag on an environment' do
@@ -134,7 +140,9 @@ module API
             .execute(feature_flag)
 
           if result[:status] == :success
-            present result[:feature_flag], with: EE::API::Entities::FeatureFlag
+            present result[:feature_flag],
+              with: EE::API::Entities::FeatureFlag,
+              feature_flags_new_version_enabled: feature_flags_new_version_enabled?
           else
             render_api_error!(result[:message], result[:http_status])
           end
@@ -160,8 +168,20 @@ module API
       end
 
       def feature_flag
-        @feature_flag ||=
-          user_project.operations_feature_flags.find_by_name!(params[:name])
+        @feature_flag ||= if Feature.enabled?(:feature_flags_new_version, user_project)
+                            user_project.operations_feature_flags.find_by_name!(params[:name])
+                          else
+                            user_project.operations_feature_flags.legacy_flag.find_by_name!(params[:name])
+                          end
+      end
+
+      def feature_flags_new_version_enabled?
+        Feature.enabled?(:feature_flags_new_version, user_project)
+      end
+
+      def rename_key(hash, old_key, new_key)
+        hash[new_key] = hash.delete(old_key) if hash.key?(old_key)
+        hash
       end
     end
   end

ee/lib/ee/api/entities/feature_flag.rb

@@ -6,9 +6,11 @@ module EE
       class FeatureFlag < Grape::Entity
         expose :name
         expose :description
+        expose :version, if: :feature_flags_new_version_enabled
         expose :created_at
         expose :updated_at
-        expose :scopes, using: FeatureFlag::Scope
+        expose :scopes, using: FeatureFlag::LegacyScope
+        expose :strategies, using: FeatureFlag::Strategy, if: :feature_flags_new_version_enabled
       end
     end
   end

ee/lib/ee/api/entities/feature_flag/detailed_scope.rb → ee/lib/ee/api/entities/feature_flag/detailed_legacy_scope.rb

@@ -4,7 +4,7 @@ module EE
   module API
     module Entities
       class FeatureFlag < Grape::Entity
-        class DetailedScope < Scope
+        class DetailedLegacyScope < LegacyScope
           expose :name
         end
       end

ee/lib/ee/api/entities/feature_flag/legacy_scope.rb

+# frozen_string_literal: true
+
+module EE
+  module API
+    module Entities
+      class FeatureFlag < Grape::Entity
+        class LegacyScope < Grape::Entity
+          expose :id
+          expose :active
+          expose :environment_scope
+          expose :strategies
+          expose :created_at
+          expose :updated_at
+        end
+      end
+    end
+  end
+end

ee/lib/ee/api/entities/feature_flag/scope.rb

@@ -6,11 +6,7 @@ module EE
       class FeatureFlag < Grape::Entity
         class Scope < Grape::Entity
           expose :id
-          expose :active
           expose :environment_scope
-          expose :strategies
-          expose :created_at
-          expose :updated_at
         end
       end
     end

ee/lib/ee/api/entities/feature_flag/strategy.rb

+# frozen_string_literal: true
+
+module EE
+  module API
+    module Entities
+      class FeatureFlag < Grape::Entity
+        class Strategy < Grape::Entity
+          expose :id
+          expose :name
+          expose :parameters
+          expose :scopes, using: FeatureFlag::Scope
+        end
+      end
+    end
+  end
+end

ee/spec/factories/operations/feature_flags.rb

@@ -5,5 +5,13 @@ FactoryBot.define do
     sequence(:name) { |n| "feature_flag_#{n}" }
     project
     active { true }
+
+    trait :legacy_flag do
+      version { Operations::FeatureFlag.versions['legacy_flag'] }
+    end
+
+    trait :new_version_flag do
+      version { Operations::FeatureFlag.versions['new_version_flag'] }
+    end
   end
 end

ee/spec/finders/feature_flags_finder_spec.rb

@@ -74,5 +74,23 @@ describe FeatureFlagsFinder do
         subject
       end
     end
+
+    context 'when new version flags are enabled' do
+      let!(:feature_flag_3) { create(:operations_feature_flag, :new_version_flag, name: 'flag-c', project: project) }
+
+      it 'returns new and legacy flags' do
+        is_expected.to eq([feature_flag_1, feature_flag_2, feature_flag_3])
+      end
+    end
+
+    context 'when new version flags are disabled' do
+      let!(:feature_flag_3) { create(:operations_feature_flag, :new_version_flag, name: 'flag-c', project: project) }
+
+      it 'returns only legacy flags' do
+        stub_feature_flags(feature_flags_new_version: false)
+
+        is_expected.to eq([feature_flag_1, feature_flag_2])
+      end
+    end
   end
 end

ee/spec/fixtures/api/schemas/public_api/v4/feature_flag.json

@@ -4,9 +4,11 @@
   "properties": {
     "name": { "type": "string" },
     "description": { "type": ["string", "null"] },
+    "version": { "type": "string" },
     "created_at": { "type": "date" },
     "updated_at": { "type": "date" },
-    "scopes": { "type": "array", "items": { "$ref": "feature_flag_scope.json" } }
+    "scopes": { "type": "array", "items": { "$ref": "feature_flag_scope.json" } },
+    "strategies": { "type": "array", "items": { "$ref": "operations/strategy.json" } }
   },
   "additionalProperties": false
 }

ee/spec/fixtures/api/schemas/public_api/v4/operations/scope.json

+{
+  "type": "object",
+  "required": ["environment_scope"],
+  "properties": {
+    "id": { "type": "integer" },
+    "environment_scope": { "type": "string" }
+  },
+  "additionalProperties": false
+}

ee/spec/fixtures/api/schemas/public_api/v4/operations/strategy.json

+{
+  "type": "object",
+  "required": [
+    "name",
+    "parameters"
+  ],
+  "properties": {
+    "id": { "type": "integer" },
+    "name": { "type": "string" },
+    "parameters": { "type": "object" },
+    "scopes": { "type": "array", "items": { "$ref": "scope.json" } }
+  },
+  "additionalProperties": false
+}

ee/spec/requests/api/feature_flags_spec.rb

@@ -59,6 +59,34 @@ describe API::FeatureFlags do
         expect(json_response.second['name']).to eq(feature_flag_2.name)
       end
 
+      it 'returns the legacy flag version' do
+        subject
+
+        expect(response).to have_gitlab_http_status(:ok)
+        expect(response).to match_response_schema('public_api/v4/feature_flags', dir: 'ee')
+        expect(json_response.map { |f| f['version'] }).to eq(%w[legacy_flag legacy_flag])
+      end
+
+      it 'does not return the legacy flag version when the feature flag is disabled' do
+        stub_feature_flags(feature_flags_new_version: false)
+
+        subject
+
+        expect(response).to have_gitlab_http_status(:ok)
+        expect(response).to match_response_schema('public_api/v4/feature_flags', dir: 'ee')
+        expect(json_response.select { |f| f.key?('version') }).to eq([])
+      end
+
+      it 'does not return strategies if the new flag is disabled' do
+        stub_feature_flags(feature_flags_new_version: false)
+
+        subject
+
+        expect(response).to have_gitlab_http_status(:ok)
+        expect(response).to match_response_schema('public_api/v4/feature_flags', dir: 'ee')
+        expect(json_response.select { |f| f.key?('strategies') }).to eq([])
+      end
+
       it 'does not have N+1 problem' do
         control_count = ActiveRecord::QueryRecorder.new { subject }
 
@@ -70,6 +98,143 @@ describe API::FeatureFlags do
 
       it_behaves_like 'check user permission'
     end
+
+    context 'with version 2 feature flags' do
+      let!(:feature_flag) do
+        create(:operations_feature_flag, :new_version_flag, project: project, name: 'feature1')
+      end
+      let!(:strategy) do
+        create(:operations_strategy, feature_flag: feature_flag, name: 'default', parameters: {})
+      end
+      let!(:scope) do
+        create(:operations_scope, strategy: strategy, environment_scope: 'production')
+      end
+
+      it 'returns the feature flags' do
+        subject
+
+        expect(response).to have_gitlab_http_status(:ok)
+        expect(response).to match_response_schema('public_api/v4/feature_flags', dir: 'ee')
+        expect(json_response).to eq([{
+          'name' => 'feature1',
+          'description' => nil,
+          'version' => 'new_version_flag',
+          'updated_at' => feature_flag.updated_at.as_json,
+          'created_at' => feature_flag.created_at.as_json,
+          'scopes' => [],
+          'strategies' => [{
+            'id' => strategy.id,
+            'name' => 'default',
+            'parameters' => {},
+            'scopes' => [{
+              'id' => scope.id,
+              'environment_scope' => 'production'
+            }]
+          }]
+        }])
+      end
+
+      it 'does not return a version 2 flag when the feature flag is disabled' do
+        stub_feature_flags(feature_flags_new_version: false)
+
+        subject
+
+        expect(response).to have_gitlab_http_status(:ok)
+        expect(response).to match_response_schema('public_api/v4/feature_flags', dir: 'ee')
+        expect(json_response).to eq([])
+      end
+    end
+
+    context 'with version 1 and 2 feature flags' do
+      it 'returns both versions of flags ordered by name' do
+        create(:operations_feature_flag, project: project, name: 'legacy_flag')
+        feature_flag = create(:operations_feature_flag, :new_version_flag, project: project, name: 'new_version_flag')
+        strategy = create(:operations_strategy, feature_flag: feature_flag, name: 'default', parameters: {})
+        create(:operations_scope, strategy: strategy, environment_scope: 'production')
+
+        subject
+
+        expect(response).to have_gitlab_http_status(:ok)
+        expect(response).to match_response_schema('public_api/v4/feature_flags', dir: 'ee')
+        expect(json_response.map { |f| f['name'] }).to eq(%w[legacy_flag new_version_flag])
+      end
+
+      it 'returns only version 1 flags when the feature flag is disabled' do
+        stub_feature_flags(feature_flags_new_version: false)
+        create(:operations_feature_flag, project: project, name: 'legacy_flag')
+        feature_flag = create(:operations_feature_flag, :new_version_flag, project: project, name: 'new_version_flag')
+        strategy = create(:operations_strategy, feature_flag: feature_flag, name: 'default', parameters: {})
+        create(:operations_scope, strategy: strategy, environment_scope: 'production')
+
+        subject
+
+        expect(response).to have_gitlab_http_status(:ok)
+        expect(response).to match_response_schema('public_api/v4/feature_flags', dir: 'ee')
+        expect(json_response.map { |f| f['name'] }).to eq(['legacy_flag'])
+      end
+    end
+  end
+
+  describe 'GET /projects/:id/feature_flags/:name' do
+    subject { get api("/projects/#{project.id}/feature_flags/#{feature_flag.name}", user) }
+
+    context 'when there is a feature flag' do
+      let!(:feature_flag) { create_flag(project, 'awesome-feature') }
+
+      it 'returns a feature flag entry' do
+        subject
+
+        expect(response).to have_gitlab_http_status(:ok)
+        expect(response).to match_response_schema('public_api/v4/feature_flag', dir: 'ee')
+        expect(json_response['name']).to eq(feature_flag.name)
+        expect(json_response['description']).to eq(feature_flag.description)
+        expect(json_response['version']).to eq('legacy_flag')
+      end
+
+      it_behaves_like 'check user permission'
+    end
+
+    context 'with a version 2 feature_flag' do
+      it 'returns the feature flag' do
+        feature_flag = create(:operations_feature_flag, :new_version_flag, project: project, name: 'feature1')
+        strategy = create(:operations_strategy, feature_flag: feature_flag, name: 'default', parameters: {})
+        scope = create(:operations_scope, strategy: strategy, environment_scope: 'production')
+
+        get api("/projects/#{project.id}/feature_flags/feature1", user)
+
+        expect(response).to have_gitlab_http_status(:ok)
+        expect(response).to match_response_schema('public_api/v4/feature_flag', dir: 'ee')
+        expect(json_response).to eq({
+          'name' => 'feature1',
+          'description' => nil,
+          'version' => 'new_version_flag',
+          'updated_at' => feature_flag.updated_at.as_json,
+          'created_at' => feature_flag.created_at.as_json,
+          'scopes' => [],
+          'strategies' => [{
+            'id' => strategy.id,
+            'name' => 'default',
+            'parameters' => {},
+            'scopes' => [{
+              'id' => scope.id,
+              'environment_scope' => 'production'
+            }]
+          }]
+        })
+      end
+
+      it 'returns a 404 when the feature is disabled' do
+        stub_feature_flags(feature_flags_new_version: false)
+        feature_flag = create(:operations_feature_flag, :new_version_flag, project: project, name: 'feature1')
+        strategy = create(:operations_strategy, feature_flag: feature_flag, name: 'default', parameters: {})
+        create(:operations_scope, strategy: strategy, environment_scope: 'production')
+
+        get api("/projects/#{project.id}/feature_flags/feature1", user)
+
+        expect(response).to have_gitlab_http_status(:not_found)
+        expect(json_response).to eq({ 'message' => '404 Not found' })
+      end
+    end
   end
 
   describe 'POST /projects/:id/feature_flags' do
@@ -97,6 +262,24 @@ describe API::FeatureFlags do
 
     it_behaves_like 'check user permission'
 
+    it 'returns version' do
+      subject
+
+      expect(response).to have_gitlab_http_status(:created)
+      expect(response).to match_response_schema('public_api/v4/feature_flag', dir: 'ee')
+      expect(json_response['version']).to eq('legacy_flag')
+    end
+
+    it 'does not return version when new version flags are disabled' do
+      stub_feature_flags(feature_flags_new_version: false)
+
+      subject
+
+      expect(response).to have_gitlab_http_status(:created)
+      expect(response).to match_response_schema('public_api/v4/feature_flag', dir: 'ee')
+      expect(json_response.key?('version')).to eq(false)
+    end
+
     context 'when no scopes passed in parameters' do
       let(:params) { { name: 'awesome-feature' } }
 
@@ -167,25 +350,6 @@ describe API::FeatureFlags do
     end
   end
 
-  describe 'GET /projects/:id/feature_flags/:name' do
-    subject { get api("/projects/#{project.id}/feature_flags/#{feature_flag.name}", user) }
-
-    context 'when there is a feature flag' do
-      let!(:feature_flag) { create_flag(project, 'awesome-feature') }
-
-      it 'returns a feature flag entry' do
-        subject
-
-        expect(response).to have_gitlab_http_status(:ok)
-        expect(response).to match_response_schema('public_api/v4/feature_flag', dir: 'ee')
-        expect(json_response['name']).to eq(feature_flag.name)
-        expect(json_response['description']).to eq(feature_flag.description)
-      end
-
-      it_behaves_like 'check user permission'
-    end
-  end
-
   describe 'POST /projects/:id/feature_flags/:name/enable' do
     subject do
       post api("/projects/#{project.id}/feature_flags/#{params[:name]}/enable", user),
@@ -353,5 +517,21 @@ describe API::FeatureFlags do
 
       expect(response).to have_gitlab_http_status(:ok)
     end
+
+    it 'returns version' do
+      subject
+
+      expect(response).to have_gitlab_http_status(:ok)
+      expect(json_response['version']).to eq('legacy_flag')
+    end
+
+    it 'does not return version when new version flags are disabled' do
+      stub_feature_flags(feature_flags_new_version: false)
+
+      subject
+
+      expect(response).to have_gitlab_http_status(:ok)
+      expect(json_response.key?('version')).to eq(false)
+    end
   end
 end