Merge branch '209824-add-group-id-to-services' into 'master'

Add group_id column to the services table

See merge request gitlab-org/gitlab!38499

app/models/service.rb

@@ -48,13 +48,16 @@ class Service < ApplicationRecord
   belongs_to :project, inverse_of: :services
   has_one :service_hook
 
-  validates :project_id, presence: true, unless: -> { template? || instance? }
-  validates :project_id, absence: true, if: -> { template? || instance? }
-  validates :type, uniqueness: { scope: :project_id }, unless: -> { template? || instance? }, on: :create
+  validates :project_id, presence: true, unless: -> { template? || instance? || group_id }
+  validates :group_id, presence: true, unless: -> { template? || instance? || project_id }
+  validates :project_id, :group_id, absence: true, if: -> { template? || instance? }
+  validates :type, uniqueness: { scope: :project_id }, unless: -> { template? || instance? || group_id }, on: :create
+  validates :type, uniqueness: { scope: :group_id }, unless: -> { template? || instance? || project_id }
   validates :type, presence: true
   validates :template, uniqueness: { scope: :type }, if: -> { template? }
   validates :instance, uniqueness: { scope: :type }, if: -> { instance? }
   validate :validate_is_instance_or_template
+  validate :validate_belongs_to_project_or_group
 
   scope :external_issue_trackers, -> { where(category: 'issue_tracker').active }
   scope :external_wikis, -> { where(type: 'ExternalWikiService').active }
@@ -377,6 +380,10 @@ class Service < ApplicationRecord
     errors.add(:template, 'The service should be a service template or instance-level integration') if template? && instance?
   end
 
+  def validate_belongs_to_project_or_group
+    errors.add(:project_id, 'The service cannot belong to both a project and a group') if project_id && group_id
+  end
+
   def cache_project_has_external_issue_tracker
     if project && !project.destroyed?
       project.cache_has_external_issue_tracker

changelogs/unreleased/209824-add-group-id-to-services.yml

+---
+title: Add group_id column to the services table
+merge_request: 38499
+author:
+type: other

db/migrate/20200803111512_add_group_id_to_services.rb

+# frozen_string_literal: true
+
+class AddGroupIdToServices < ActiveRecord::Migration[6.0]
+  DOWNTIME = false
+
+  def change
+    add_column :services, :group_id, :bigint
+  end
+end

db/migrate/20200803112806_add_index_group_id_to_services.rb

+# frozen_string_literal: true
+
+class AddIndexGroupIdToServices < ActiveRecord::Migration[6.0]
+  include Gitlab::Database::MigrationHelpers
+
+  DOWNTIME = false
+  INDEX_NAME = 'index_services_on_unique_group_id_and_type'
+
+  disable_ddl_transaction!
+
+  def up
+    add_concurrent_index :services, [:group_id, :type], unique: true, name: INDEX_NAME
+
+    add_concurrent_foreign_key :services, :namespaces, column: :group_id
+  end
+
+  def down
+    remove_foreign_key_if_exists :services, column: :group_id
+
+    remove_concurrent_index_by_name :services, INDEX_NAME
+  end
+end

db/schema_migrations/20200803111512

+d0ea5e75c74d8405014fe5be6906e21b009d9eddb8eaefea939a67e01ee146e7
\ No newline at end of file

db/schema_migrations/20200803112806

+8e6ffd7b1d44ac846b79cf37798abc57eee169fbde0de096ea49df590af864b5
\ No newline at end of file

db/structure.sql

@@ -15333,7 +15333,8 @@ CREATE TABLE public.services (
     instance boolean DEFAULT false NOT NULL,
     comment_detail smallint,
     inherit_from_id bigint,
-    alert_events boolean
+    alert_events boolean,
+    group_id bigint
 );
 
 CREATE SEQUENCE public.services_id_seq
@@ -20567,6 +20568,8 @@ CREATE UNIQUE INDEX index_services_on_type_and_template_partial ON public.servic
 
 CREATE INDEX index_services_on_type_id_when_active_not_instance_not_template ON public.services USING btree (type, id) WHERE ((active = true) AND (instance = false) AND (template = false));
 
+CREATE UNIQUE INDEX index_services_on_unique_group_id_and_type ON public.services USING btree (group_id, type);
+
 CREATE UNIQUE INDEX index_shards_on_name ON public.shards USING btree (name);
 
 CREATE INDEX index_slack_integrations_on_service_id ON public.slack_integrations USING btree (service_id);
@@ -21774,6 +21777,9 @@ ALTER TABLE ONLY public.application_settings
 ALTER TABLE ONLY public.ci_triggers
     ADD CONSTRAINT fk_e8e10d1964 FOREIGN KEY (owner_id) REFERENCES public.users(id) ON DELETE CASCADE;
 
+ALTER TABLE ONLY public.services
+    ADD CONSTRAINT fk_e8fe908a34 FOREIGN KEY (group_id) REFERENCES public.namespaces(id) ON DELETE CASCADE;
+
 ALTER TABLE ONLY public.pages_domains
     ADD CONSTRAINT fk_ea2f6dfc6f FOREIGN KEY (project_id) REFERENCES public.projects(id) ON DELETE CASCADE;
 

spec/models/service_spec.rb

@@ -11,30 +11,31 @@ RSpec.describe Service do
   end
 
   describe 'validations' do
-    it { is_expected.to validate_presence_of(:type) }
+    using RSpec::Parameterized::TableSyntax
 
-    it 'validates presence of project_id if not template', :aggregate_failures do
-      expect(build(:service, project_id: nil, template: true)).to be_valid
-      expect(build(:service, project_id: nil, template: false)).to be_invalid
-    end
+    let(:group) { create(:group) }
+    let(:project) { create(:project) }
 
-    it 'validates presence of project_id if not instance', :aggregate_failures do
-      expect(build(:service, project_id: nil, instance: true)).to be_valid
-      expect(build(:service, project_id: nil, instance: false)).to be_invalid
-    end
+    it { is_expected.to validate_presence_of(:type) }
 
-    it 'validates absence of project_id if instance', :aggregate_failures do
-      expect(build(:service, project_id: nil, instance: true)).to be_valid
-      expect(build(:service, instance: true)).to be_invalid
+    where(:project_id, :group_id, :template, :instance, :valid) do
+      1    | nil  | false  | false  | true
+      nil  | 1    | false  | false  | true
+      nil  | nil  | true   | false  | true
+      nil  | nil  | false  | true   | true
+      nil  | nil  | false  | false  | false
+      nil  | nil  | true   | true   | false
+      1    | 1    | false  | false  | false
+      1    | nil  | true   | false  | false
+      1    | nil  | false  | true   | false
+      nil  | 1    | true   | false  | false
+      nil  | 1    | false  | true   | false
     end
 
-    it 'validates absence of project_id if template', :aggregate_failures do
-      expect(build(:service, template: true)).to validate_absence_of(:project_id)
-      expect(build(:service, template: false)).not_to validate_absence_of(:project_id)
+    with_them do
+      it 'validates the service' do
+        expect(build(:service, project_id: project_id, group_id: group_id, template: template, instance: instance).valid?).to eq(valid)
       end
-
-    it 'validates service is template or instance' do
-      expect(build(:service, project_id: nil, template: true, instance: true)).to be_invalid
     end
 
     context 'with an existing service template' do
@@ -58,12 +59,15 @@ RSpec.describe Service do
     end
 
     it 'validates uniqueness of type and project_id on create' do
-      project = create(:project)
-
       expect(create(:service, project: project, type: 'Service')).to be_valid
       expect(build(:service, project: project, type: 'Service').valid?(:create)).to eq(false)
       expect(build(:service, project: project, type: 'Service').valid?(:update)).to eq(true)
     end
+
+    it 'validates uniqueness of type and group_id' do
+      expect(create(:service, group_id: group.id, project_id: nil, type: 'Service')).to be_valid
+      expect(build(:service, group_id: group.id, project_id: nil, type: 'Service')).to be_invalid
+    end
   end
 
   describe 'Scopes' do