Support CURD operation for feature flag scopes

 (GitLab internal API standpoint)

Squashed commit of the following:

commit 32a24ca826bc62d8667980108c1593e8b4377f95
Author: Shinya Maeda <shinya@gitlab.com>
Date:   Mon Jan 21 19:39:01 2019 +0900

    Added another tests

commit 2253909e5c8a3a372f15c21e9a1efac6c5e1d186
Author: Shinya Maeda <shinya@gitlab.com>
Date:   Mon Jan 21 18:16:09 2019 +0900

    Fix unleash spec

commit 1c61c13cf87a6543d22906cb717e46a6daaee212
Author: Shinya Maeda <shinya@gitlab.com>
Date:   Mon Jan 21 18:11:16 2019 +0900

    Add spec for finder

commit c670bf9e3bccaf1422d5718be93ca37cc3afc0ce
Author: Shinya Maeda <shinya@gitlab.com>
Date:   Mon Jan 21 17:55:22 2019 +0900

    Fix model related spec

commit c9913a3d4ab0722b7a8f1bf2fe2b77ffd70a1bf3
Author: Shinya Maeda <shinya@gitlab.com>
Date:   Mon Jan 21 17:22:36 2019 +0900

    Rename lazy active to at least one

commit 6b50d76cd1f5b47162fb94f25c2ebd7f837fd2dd
Author: Shinya Maeda <shinya@gitlab.com>
Date:   Mon Jan 21 16:00:51 2019 +0900

    Fix default scope related code

commit e0c352c0453bbdec6c7e1882663f1f5f99a9a2b1
Author: Shinya Maeda <shinya@gitlab.com>
Date:   Mon Jan 21 14:44:50 2019 +0900

    Fix

commit 3528b8c79c3fb07e0b8327c9195465c555ae7364
Author: Shinya Maeda <shinya@gitlab.com>
Date:   Mon Jan 21 14:44:21 2019 +0900

    Revert "Decouple default scope related things"

    This reverts commit ccbfaf81197c6ab2ac7ad86629701a65807dd996.

commit d396059841c019c53796cf254f6410d81e255407
Author: Shinya Maeda <shinya@gitlab.com>
Date:   Wed Jan 16 19:56:24 2019 +0900

    Support CURD operation for flag scopes

    Simplify active presentation

    introduce lazy sql

    Fix

    Fix finder

    Fix spec

    Improve

commit 52364fa2f2e122eaf4e0abacf1db378244f1f303
Author: Shinya Maeda <shinya@gitlab.com>
Date:   Mon Jan 21 16:07:53 2019 +0900

    Add helper for feature flag tests

commit c781381a5a1dfe65905ab1f94b7b21a2796e8f4e
Author: Shinya Maeda <shinya@gitlab.com>
Date:   Mon Jan 21 15:24:19 2019 +0900

    Fix unleash api app name handling

commit d7e6c0f21bbb90c4f37ecd98e9ddd2a940fcc074
Author: Shinya Maeda <shinya@gitlab.com>
Date:   Mon Jan 21 15:18:21 2019 +0900

    Decouple FeatureFlagsFinder change

commit 032cdb41ed3cc9f28891d16b2ff0eea65e9b6fae
Author: Shinya Maeda <shinya@gitlab.com>
Date:   Mon Jan 21 14:23:22 2019 +0900

    Decouple default scope related things

commit fe3e415d83bd260ae8a4db37323f1909174e2d06
Author: Shinya Maeda <shinya@gitlab.com>
Date:   Thu Jan 10 20:20:45 2019 +0900

    Multiple environments support for feature flags

    Support for Unleash API

    Elaborate a comment in has environment scope

    Fix specs

    Complete test for unleash API

    Add changelog

    Fix coding offence

    Fix coding offence

    Fix ee specific line check

    Fix MySQL problem

    Fix typo

    Improve comment

    Fix spec location

    Override active value with alias

    Add test and feature flag

    Add frozen string literal

    Default enable true

Prevent destroy default scope (A bit messy now)

Simplify the all implementations

Simplify implementaion ver 2

Simplify implementation ver 4

Simplify implementation ver 5

Fix feature flag options

Add changelog

Fix coding offence

Fix spec

Push feature flag to frontend

Update spec

Simplify the implementation more

Fix N+1 problem

Fix coding offence

ee/app/controllers/projects/feature_flags_controller.rb

@@ -3,6 +3,7 @@
 class Projects::FeatureFlagsController < Projects::ApplicationController
   respond_to :html
 
+  before_action :push_feature_flag_to_frontend!
   before_action :authorize_read_feature_flag!
   before_action :authorize_create_feature_flag!, only: [:new, :create]
   before_action :authorize_update_feature_flag!, only: [:edit, :update]
@@ -91,17 +92,19 @@ class Projects::FeatureFlagsController < Projects::ApplicationController
   protected
 
   def feature_flag
-    @feature_flag ||= project.operations_feature_flags.find(params[:id])
+    @feature_flag ||= project.operations_feature_flags.for_list.find(params[:id])
   end
 
   def create_params
     params.require(:operations_feature_flag)
-          .permit(:name, :description, :active)
+          .permit(:name, :description, :active,
+                  scopes_attributes: [:environment_scope, :active])
   end
 
   def update_params
     params.require(:operations_feature_flag)
-          .permit(:name, :description, :active)
+          .permit(:name, :description, :active,
+                  scopes_attributes: [:id, :environment_scope, :active, :_destroy])
   end
 
   def feature_flag_json
@@ -135,4 +138,8 @@ class Projects::FeatureFlagsController < Projects::ApplicationController
     render json: { message: feature_flag.errors.full_messages },
            status: :bad_request
   end
+
+  def push_feature_flag_to_frontend!
+    push_frontend_feature_flag(:feature_flags_environment_scope, project)
+  end
 end

ee/app/finders/feature_flags_finder.rb

@@ -17,6 +17,8 @@ class FeatureFlagsFinder
 
     items = feature_flags
     items = by_scope(items)
+    items = for_list(items)
+
     items.ordered
   end
 
@@ -32,4 +34,12 @@ class FeatureFlagsFinder
       items
     end
   end
+
+  def for_list(items)
+    if Feature.enabled?(:feature_flags_environment_scope, project)
+      items.for_list
+    else
+      items
+    end
+  end
 end

ee/app/models/operations/feature_flag.rb

@@ -12,6 +12,7 @@ module Operations
     belongs_to :project
 
     has_many :scopes, class_name: 'Operations::FeatureFlagScope'
+    has_one :default_scope, -> { where(environment_scope: '*') }, class_name: 'Operations::FeatureFlagScope'
 
     validates :project, presence: true
     validates :name,
@@ -24,15 +25,39 @@ module Operations
     validates :name, uniqueness: { scope: :project_id }
     validates :description, allow_blank: true, length: 0..255
 
+    before_create :build_default_scope
+    after_update :update_default_scope
+
+    accepts_nested_attributes_for :scopes, allow_destroy: true
+
     scope :ordered, -> { order(:name) }
-    scope :enabled, -> { where(active: true) }
-    scope :disabled, -> { where(active: false) }
+
+    scope :enabled, -> do
+      if Feature.enabled?(:feature_flags_environment_scope)
+        where('EXISTS (?)', join_enabled_scopes)
+      else
+        where(active: true)
+      end
+    end
+
+    scope :disabled, -> do
+      if Feature.enabled?(:feature_flags_environment_scope)
+        where('NOT EXISTS (?)', join_enabled_scopes)
+      else
+        where(active: false)
+      end
+    end
 
     scope :for_environment, -> (environment) do
       select("operations_feature_flags.*" \
              ", (#{actual_active_sql(environment)}) AS active")
     end
 
+    scope :for_list, -> do
+      select("operations_feature_flags.*" \
+             ", COALESCE((#{join_enabled_scopes.to_sql}), FALSE) AS active")
+    end
+
     class << self
       def actual_active_sql(environment)
         Operations::FeatureFlagScope
@@ -42,6 +67,16 @@ module Operations
           .select('active')
           .to_sql
       end
+
+      def join_enabled_scopes
+        Operations::FeatureFlagScope
+          .where('operations_feature_flags.id = feature_flag_id')
+          .enabled.limit(1).select('TRUE')
+      end
+
+      def preload_relations
+        preload(:scopes)
+      end
     end
 
     def strategies
@@ -49,5 +84,15 @@ module Operations
         { name: 'default' }
       ]
     end
+
+    private
+
+    def build_default_scope
+      scopes.build(environment_scope: '*', active: self.active)
+    end
+
+    def update_default_scope
+      default_scope.update(active: self.active) if self.active_changed?
+    end
   end
 end

ee/app/models/operations/feature_flag_scope.rb

@@ -13,7 +13,24 @@ module Operations
       message: "(%{value}) has already been taken"
     }
 
+    validates :environment_scope,
+      if: :default_scope?, on: :update,
+      inclusion: { in: %w(*), message: 'cannot be changed from default scope' }
+
+    before_destroy :prevent_destroy_default_scope, if: :default_scope?
+
+    scope :ordered, -> { order(:id) }
     scope :enabled, -> { where(active: true) }
     scope :disabled, -> { where(active: false) }
+
+    private
+
+    def default_scope?
+      environment_scope_was == '*'
+    end
+
+    def prevent_destroy_default_scope
+      raise ActiveRecord::ReadOnlyRecord, "default scope cannot be destroyed"
+    end
   end
 end

ee/app/serializers/feature_flag_entity.rb

@@ -18,6 +18,10 @@ class FeatureFlagEntity < Grape::Entity
     project_feature_flag_path(feature_flag.project, feature_flag)
   end
 
+  expose :scopes, with: FeatureFlagScopeEntity do |feature_flag|
+    feature_flag.scopes.sort_by(&:id)
+  end
+
   private
 
   def can_update?(feature_flag)

ee/app/serializers/feature_flag_scope_entity.rb

+# frozen_string_literal: true
+
+class FeatureFlagScopeEntity < Grape::Entity
+  include RequestAwareEntity
+
+  expose :id
+  expose :active
+  expose :environment_scope
+  expose :created_at
+  expose :updated_at
+end

ee/app/serializers/feature_flag_serializer.rb

@@ -3,4 +3,12 @@
 class FeatureFlagSerializer < BaseSerializer
   include WithPagination
   entity FeatureFlagEntity
+
+  def represent(resource, opts = {})
+    if resource.is_a?(ActiveRecord::Relation)
+      resource = resource.preload_relations
+    end
+
+    super(resource, opts)
+  end
 end

ee/changelogs/unreleased/support-curd-operation-for-flag-scopes.yml

+---
+title: Support CURD operation for feature flag scopes
+merge_request: 9182
+author:
+type: added

ee/db/migrate/20190111183834_create_default_scope_to_feature_flags.rb

+# frozen_string_literal: true
+
+class CreateDefaultScopeToFeatureFlags < ActiveRecord::Migration[5.0]
+  include Gitlab::Database::MigrationHelpers
+
+  DOWNTIME = false
+
+  def up
+    execute <<~SQL
+      INSERT INTO operations_feature_flag_scopes (feature_flag_id, environment_scope, active, created_at, updated_at)
+      SELECT id, '*', active, created_at, updated_at
+      FROM operations_feature_flags
+      WHERE NOT EXISTS (
+        SELECT 1
+        FROM operations_feature_flag_scopes
+        WHERE operations_feature_flags.id = operations_feature_flag_scopes.feature_flag_id AND
+          environment_scope = '*'
+      );
+    SQL
+  end
+
+  def down
+    execute <<~SQL
+      DELETE FROM operations_feature_flag_scopes WHERE environment_scope = '*';
+    SQL
+  end
+end

ee/spec/finders/feature_flags_finder_spec.rb

@@ -3,6 +3,8 @@
 require 'spec_helper'
 
 describe FeatureFlagsFinder do
+  include FeatureFlagHelpers
+
   let(:finder) { described_class.new(project, user, params) }
   let(:project) { create(:project) }
   let(:user) { developer }
@@ -55,5 +57,26 @@ describe FeatureFlagsFinder do
         end
       end
     end
+
+    context 'when it is presented for list' do
+      let!(:feature_flag_1) { create(:operations_feature_flag, project: project, active: false) }
+      let!(:feature_flag_2) { create(:operations_feature_flag, project: project, active: false) }
+
+      context 'when there is an active scope' do
+        before do
+          create_scope(feature_flag_1, 'review/*', true)
+        end
+
+        it 'presents a virtual active value' do
+          expect(subject.map(&:active)).to eq([true, false])
+        end
+      end
+
+      context 'when there are no active scopes' do
+        it 'presents a virtual active value' do
+          expect(subject.map(&:active)).to eq([false, false])
+        end
+      end
+    end
   end
 end

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

@@ -12,7 +12,8 @@
     "active": { "type": "boolean" },
     "description": { "type": ["string", "null"] },
     "edit_path": { "type": ["string", "null"] },
-    "destroy_path": { "type": ["string", "null"] }
+    "destroy_path": { "type": ["string", "null"] },
+    "scopes": { "type": "array", "items": { "$ref": "feature_flag_scope.json" } }
   },
   "additionalProperties": false
 }

ee/spec/fixtures/api/schemas/feature_flag_scope.json

+{
+  "type": "object",
+  "required" : [
+    "id",
+    "environment_scope",
+    "active"
+  ],
+  "properties" : {
+    "id": { "type": "integer" },
+    "environment_scope": { "type": "string" },
+    "active": { "type": "boolean" },
+    "created_at": { "type": "date" },
+    "updated_at": { "type": "date" }
+  },
+  "additionalProperties": false
+}

ee/spec/migrations/create_default_scope_to_feature_flags_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+require Rails.root.join('ee', 'db', 'migrate', '20190111183834_create_default_scope_to_feature_flags.rb')
+
+describe CreateDefaultScopeToFeatureFlags, :migration do
+  let(:namespaces) { table(:namespaces) }
+  let(:namespace) { namespaces.create(name: 'test', path: 'test') }
+  let(:projects) { table(:projects) }
+  let(:project) { projects.create(name: 'test', namespace_id: namespace.id) }
+  let(:feature_flags) { table(:operations_feature_flags) }
+  let(:feature_flag_scopes) { table(:operations_feature_flag_scopes) }
+
+  let!(:feature_flag_1) do
+    feature_flags.create!(
+      project_id: project.id,
+      name: 'ci_live_trace',
+      active: true,
+      description: 'For live trace',
+      created_at: "'2017-10-17 20:24:02'",
+      updated_at: "'2017-10-17 20:24:02'"
+    )
+  end
+
+  let!(:feature_flag_2) do
+    feature_flags.create!(
+      project_id: project.id,
+      name: 'ci_merge_request',
+      active: false,
+      description: 'For merge request',
+      created_at: "'2017-10-17 20:24:02'",
+      updated_at: "'2017-10-17 20:24:02'"
+    )
+  end
+
+  describe '#up' do
+    subject { described_class.new.up }
+
+    let(:scope_1) do
+      feature_flag_scopes.find_by_feature_flag_id(feature_flag_1.id)
+    end
+
+    let(:scope_2) do
+      feature_flag_scopes.find_by_feature_flag_id(feature_flag_2.id)
+    end
+
+    it 'creates default scopes for existing rows' do
+      subject
+
+      expect(scope_1).to be_active
+      expect(scope_1.environment_scope).to eq('*')
+      expect(scope_2).not_to be_active
+      expect(scope_2.environment_scope).to eq('*')
+    end
+
+    context 'when a feature flag has already had default scope' do
+      let!(:feature_flag_scope_1) do
+        feature_flag_scopes.create!(
+          feature_flag_id: feature_flag_1.id,
+          environment_scope: '*',
+          active: true,
+          created_at: "'2017-10-17 20:24:02'",
+          updated_at: "'2017-10-17 20:24:02'"
+        )
+      end
+
+      it 'does not raise an error' do
+        expect { subject }.not_to raise_error
+      end
+
+      it 'creates default scopes for a feature flag' do
+        subject
+
+        expect(scope_2).not_to be_active
+        expect(scope_2.environment_scope).to eq('*')
+      end
+    end
+
+    context 'when there are no feature flags' do
+      let!(:feature_flag_1) { }
+      let!(:feature_flag_2) { }
+
+      it 'does not create scopes' do
+        expect { subject }.not_to change { feature_flag_scopes.count }
+      end
+    end
+  end
+
+  describe '#down' do
+    subject { described_class.new.down }
+
+    let!(:feature_flag_scope_1) do
+      feature_flag_scopes.create!(
+        feature_flag_id: feature_flag_1.id,
+        environment_scope: '*',
+        active: true,
+        created_at: "'2017-10-17 20:24:02'",
+        updated_at: "'2017-10-17 20:24:02'"
+      )
+    end
+
+    it 'deletes default scopes' do
+      expect { subject }.to change { feature_flag_scopes.count }.by(-1)
+    end
+
+    it 'does not affect feature flags' do
+      expect { subject }.not_to change { feature_flags.count }
+    end
+  end
+end

ee/spec/models/operations/feature_flag_scope_spec.rb

@@ -27,6 +27,27 @@ describe Operations::FeatureFlagScope do
                       " has already been taken")
       end
     end
+
+    context 'when environment scope of a default scope is updated' do
+      let!(:feature_flag) { create(:operations_feature_flag) }
+      let!(:default_scope) { feature_flag.default_scope }
+
+      it 'keeps default scope intact' do
+        default_scope.update(environment_scope: 'review/*')
+
+        expect(default_scope.errors[:environment_scope])
+          .to include("cannot be changed from default scope")
+      end
+    end
+
+    context 'when a default scope is destroyed' do
+      let!(:feature_flag) { create(:operations_feature_flag) }
+      let!(:default_scope) { feature_flag.default_scope }
+
+      it 'prevents from destroying the default scope' do
+        expect { default_scope.destroy! }.to raise_error(ActiveRecord::ReadOnlyRecord)
+      end
+    end
   end
 
   describe '.enabled' do
@@ -40,7 +61,7 @@ describe Operations::FeatureFlagScope do
       let(:active) { true }
 
       it 'returns the scope' do
-        is_expected.to eq([feature_flag_scope])
+        is_expected.to include(feature_flag_scope)
       end
     end
 
@@ -48,7 +69,7 @@ describe Operations::FeatureFlagScope do
       let(:active) { false }
 
       it 'returns an empty array' do
-        is_expected.to be_empty
+        is_expected.not_to include(feature_flag_scope)
       end
     end
   end
@@ -64,7 +85,7 @@ describe Operations::FeatureFlagScope do
       let(:active) { true }
 
       it 'returns an empty array' do
-        is_expected.to be_empty
+        is_expected.not_to include(feature_flag_scope)
       end
     end
 
@@ -72,7 +93,7 @@ describe Operations::FeatureFlagScope do
       let(:active) { false }
 
       it 'returns the scope' do
-        is_expected.to eq([feature_flag_scope])
+        is_expected.to include(feature_flag_scope)
       end
     end
   end

ee/spec/models/operations/feature_flag_spec.rb

@@ -16,6 +16,46 @@ describe Operations::FeatureFlag do
     it { is_expected.to validate_uniqueness_of(:name).scoped_to(:project_id) }
   end
 
+  describe '.enabled' do
+    subject { described_class.enabled }
+
+    context 'when the feature flag has an active scope' do
+      let!(:feature_flag) { create(:operations_feature_flag, active: true) }
+
+      it 'returns the flag' do
+        is_expected.to eq([feature_flag])
+      end
+    end
+
+    context 'when the feature flag does not have an active scope' do
+      let!(:feature_flag) { create(:operations_feature_flag, active: false) }
+
+      it 'does not return the flag' do
+        is_expected.to be_empty
+      end
+    end
+  end
+
+  describe '.disabled' do
+    subject { described_class.disabled }
+
+    context 'when the feature flag has an active scope' do
+      let!(:feature_flag) { create(:operations_feature_flag, active: true) }
+
+      it 'does not return the flag' do
+        is_expected.to be_empty
+      end
+    end
+
+    context 'when the feature flag does not have an active scope' do
+      let!(:feature_flag) { create(:operations_feature_flag, active: false) }
+
+      it 'returns the flag' do
+        is_expected.to eq([feature_flag])
+      end
+    end
+  end
+
   describe '.for_environment' do
     subject { described_class.for_environment(environment_name) }
 
@@ -25,8 +65,7 @@ describe Operations::FeatureFlag do
 
     context 'when feature flag is off on production' do
       before do
-        feature_flag = create(:operations_feature_flag)
-        create_scope(feature_flag, '*', true)
+        feature_flag = create(:operations_feature_flag, active: true)
         create_scope(feature_flag, 'production', false)
       end
 
@@ -49,8 +88,7 @@ describe Operations::FeatureFlag do
 
     context 'when feature flag is default disabled but enabled for review apps' do
       before do
-        feature_flag = create(:operations_feature_flag)
-        create_scope(feature_flag, '*', false)
+        feature_flag = create(:operations_feature_flag, active: false)
         create_scope(feature_flag, 'review/*', true)
       end
 
@@ -72,13 +110,11 @@ describe Operations::FeatureFlag do
     end
 
     context 'when there are two flags' do
-      let!(:feature_flag_1) { create(:operations_feature_flag) }
-      let!(:feature_flag_2) { create(:operations_feature_flag) }
+      let!(:feature_flag_1) { create(:operations_feature_flag, active: true) }
+      let!(:feature_flag_2) { create(:operations_feature_flag, active: true) }
 
       before do
-        create_scope(feature_flag_1, '*', true)
         create_scope(feature_flag_1, 'production', false)
-        create_scope(feature_flag_2, '*', true)
       end
 
       context 'when environment is production' do
@@ -90,4 +126,39 @@ describe Operations::FeatureFlag do
       end
     end
   end
+
+  describe '.for_list' do
+    subject { described_class.for_list }
+
+    before do
+      stub_feature_flags(feature_flags_environment_scope: true)
+    end
+
+    context 'when all scopes are active' do
+      let!(:feature_flag) { create(:operations_feature_flag, active: true) }
+      let!(:scope) { create_scope(feature_flag, 'production', true) }
+
+      it 'returns virtual active value' do
+        expect(subject.first.active).to be_truthy
+      end
+    end
+
+    context 'when all scopes are inactive' do
+      let!(:feature_flag) { create(:operations_feature_flag, active: false) }
+      let!(:scope) { create_scope(feature_flag, 'production', false) }
+
+      it 'returns virtual active value' do
+        expect(subject.first.active).to be_falsy
+      end
+    end
+
+    context 'when one scopes is active' do
+      let!(:feature_flag) { create(:operations_feature_flag, active: false) }
+      let!(:scope) { create_scope(feature_flag, 'production', true) }
+
+      it 'returns virtual active value' do
+        expect(subject.first.active).to be_truthy
+      end
+    end
+  end
 end

ee/spec/requests/api/unleash_spec.rb

@@ -74,18 +74,16 @@ describe API::Unleash do
     let(:headers) { base_headers.merge({ "UNLEASH-APPNAME" => "test" }) }
 
     let!(:feature_flag_1) do
-      create(:operations_feature_flag, project: project)
+      create(:operations_feature_flag, project: project, active: true)
     end
 
     let!(:feature_flag_2) do
-      create(:operations_feature_flag, project: project)
+      create(:operations_feature_flag, project: project, active: false)
     end
 
     before do
       stub_feature_flags(feature_flags_environment_scope: true)
-      create_scope(feature_flag_1, '*', true)
       create_scope(feature_flag_1, 'production', false)
-      create_scope(feature_flag_2, '*', false)
       create_scope(feature_flag_2, 'review/*', true)
     end