diff --git a/app/models/service.rb b/app/models/service.rb index ea99b0c27e4056c66eed145d486d58f303b24e0f..40e7e5552d1757723176b971304bdcfa19d133c5 100644 --- a/app/models/service.rb +++ b/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 @@ def validate_is_instance_or_template 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 diff --git a/changelogs/unreleased/209824-add-group-id-to-services.yml b/changelogs/unreleased/209824-add-group-id-to-services.yml new file mode 100644 index 0000000000000000000000000000000000000000..ade96e1df4a5c5408d897667ee1fd39e3e013dc3 --- /dev/null +++ b/changelogs/unreleased/209824-add-group-id-to-services.yml @@ -0,0 +1,5 @@ +--- +title: Add group_id column to the services table +merge_request: 38499 +author: +type: other diff --git a/db/migrate/20200803111512_add_group_id_to_services.rb b/db/migrate/20200803111512_add_group_id_to_services.rb new file mode 100644 index 0000000000000000000000000000000000000000..7d4a0ef4dd5d2ce0340204b8a819f4119e5b81d9 --- /dev/null +++ b/db/migrate/20200803111512_add_group_id_to_services.rb @@ -0,0 +1,9 @@ +# frozen_string_literal: true + +class AddGroupIdToServices < ActiveRecord::Migration[6.0] + DOWNTIME = false + + def change + add_column :services, :group_id, :bigint + end +end diff --git a/db/migrate/20200803112806_add_index_group_id_to_services.rb b/db/migrate/20200803112806_add_index_group_id_to_services.rb new file mode 100644 index 0000000000000000000000000000000000000000..145009672efc66eb9db3b979c7d8ed65a570d208 --- /dev/null +++ b/db/migrate/20200803112806_add_index_group_id_to_services.rb @@ -0,0 +1,22 @@ +# 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 diff --git a/db/schema_migrations/20200803111512 b/db/schema_migrations/20200803111512 new file mode 100644 index 0000000000000000000000000000000000000000..aa99ce355596a9fbf08a07097600f21bf6310db8 --- /dev/null +++ b/db/schema_migrations/20200803111512 @@ -0,0 +1 @@ +d0ea5e75c74d8405014fe5be6906e21b009d9eddb8eaefea939a67e01ee146e7 \ No newline at end of file diff --git a/db/schema_migrations/20200803112806 b/db/schema_migrations/20200803112806 new file mode 100644 index 0000000000000000000000000000000000000000..4f7962bfe49ba3b99a8290168af7831a7ba0e1c5 --- /dev/null +++ b/db/schema_migrations/20200803112806 @@ -0,0 +1 @@ +8e6ffd7b1d44ac846b79cf37798abc57eee169fbde0de096ea49df590af864b5 \ No newline at end of file diff --git a/db/structure.sql b/db/structure.sql index 645108fb09e2742836ad11f7c9528c2f8e5a1890..a356bee0c1aa1b59f55a427a3a88b04e1989975d 100644 --- a/db/structure.sql +++ b/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; diff --git a/spec/models/service_spec.rb b/spec/models/service_spec.rb index 19be46ba375f96db19f5a21be937f82d0a885aab..c4a9c0329c781c206dae553ab107dd6f22ce8224 100644 --- a/spec/models/service_spec.rb +++ b/spec/models/service_spec.rb @@ -11,30 +11,31 @@ end describe 'validations' do - it { is_expected.to validate_presence_of(:type) } - - 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 + using RSpec::Parameterized::TableSyntax - 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 + let(:group) { create(:group) } + let(:project) { create(:project) } - 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 - end + it { is_expected.to validate_presence_of(:type) } - 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) + 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 service is template or instance' do - expect(build(:service, project_id: nil, template: true, instance: true)).to be_invalid + 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 end context 'with an existing service template' do @@ -58,12 +59,15 @@ 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