From 7b8ed2c2d0cdefc2c9c6033540554af3fc6b363c Mon Sep 17 00:00:00 2001 From: Arturo Herrero Date: Mon, 3 Aug 2020 17:00:37 +0200 Subject: [PATCH 1/5] Add group_id column to the services table In order to support group-level integrations https://gitlab.com/groups/gitlab-org/-/epics/2543, we need to add a new column group_id to the services table. --- app/models/service.rb | 8 ++-- .../209824-add-group-id-to-services.yml | 5 +++ ...20200803111512_add_group_id_to_services.rb | 9 +++++ ...03112806_add_index_group_id_to_services.rb | 22 ++++++++++ db/schema_migrations/20200803111512 | 1 + db/schema_migrations/20200803112806 | 1 + db/structure.sql | 8 +++- spec/models/service_spec.rb | 40 ++++++++++++++----- 8 files changed, 79 insertions(+), 15 deletions(-) create mode 100644 changelogs/unreleased/209824-add-group-id-to-services.yml create mode 100644 db/migrate/20200803111512_add_group_id_to_services.rb create mode 100644 db/migrate/20200803112806_add_index_group_id_to_services.rb create mode 100644 db/schema_migrations/20200803111512 create mode 100644 db/schema_migrations/20200803112806 diff --git a/app/models/service.rb b/app/models/service.rb index ea99b0c27e4056..16d6f6012e925a 100644 --- a/app/models/service.rb +++ b/app/models/service.rb @@ -48,9 +48,11 @@ 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? } 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 00000000000000..ade96e1df4a5c5 --- /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 00000000000000..7d4a0ef4dd5d2c --- /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 00000000000000..145009672efc66 --- /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 00000000000000..aa99ce355596a9 --- /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 00000000000000..4f7962bfe49ba3 --- /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 645108fb09e274..a356bee0c1aa1b 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 19be46ba375f96..ac6dcce9b028f6 100644 --- a/spec/models/service_spec.rb +++ b/spec/models/service_spec.rb @@ -11,28 +11,43 @@ end describe 'validations' do + let(:group) { create(:group) } + let(:project) { create(:project) } + it { is_expected.to validate_presence_of(:type) } - it 'validates presence of project_id if not template', :aggregate_failures do + it 'validates presence of project_id if not template, instance or group_id', :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 - - 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, group_id: group.id)).to be_valid + expect(build(:service, project_id: nil, template: false)).to be_invalid expect(build(:service, project_id: nil, instance: false)).to be_invalid + expect(build(:service, project_id: nil, group_id: nil)).to be_invalid end - 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 + it 'validates presence of group_id if not template, instance or project_id', :aggregate_failures do + expect(build(:service, group_id: nil, project_id: nil, template: true)).to be_valid + expect(build(:service, group_id: nil, project_id: nil, instance: true)).to be_valid + expect(build(:service, group_id: nil, project_id: project.id)).to be_valid + expect(build(:service, group_id: nil, project_id: nil, template: false)).to be_invalid + expect(build(:service, group_id: nil, project_id: nil, instance: false)).to be_invalid + expect(build(:service, group_id: nil, project_id: nil)).to be_invalid end - it 'validates absence of project_id if template', :aggregate_failures do + it 'validates absence of project_id if instance or template', :aggregate_failures do + expect(build(:service, instance: true)).to validate_absence_of(:project_id) expect(build(:service, template: true)).to validate_absence_of(:project_id) + expect(build(:service, instance: false)).not_to validate_absence_of(:project_id) expect(build(:service, template: false)).not_to validate_absence_of(:project_id) end + it 'validates absence of group_id if instance or template', :aggregate_failures do + expect(build(:service, instance: true)).to validate_absence_of(:group_id) + expect(build(:service, template: true)).to validate_absence_of(:group_id) + expect(build(:service, instance: false)).not_to validate_absence_of(:group_id) + expect(build(:service, template: false)).not_to validate_absence_of(:group_id) + end + it 'validates service is template or instance' do expect(build(:service, project_id: nil, template: true, instance: true)).to be_invalid end @@ -58,12 +73,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 -- GitLab From 0d478400ef7902bf8c3b28cba69b8bbef840aef1 Mon Sep 17 00:00:00 2001 From: Arturo Herrero Date: Thu, 6 Aug 2020 14:00:42 +0200 Subject: [PATCH 2/5] Use RSpec parameterized syntax --- spec/models/service_spec.rb | 55 ++++++++++++++----------------------- 1 file changed, 21 insertions(+), 34 deletions(-) diff --git a/spec/models/service_spec.rb b/spec/models/service_spec.rb index ac6dcce9b028f6..8a80de61679c47 100644 --- a/spec/models/service_spec.rb +++ b/spec/models/service_spec.rb @@ -11,45 +11,32 @@ end describe 'validations' do + using RSpec::Parameterized::TableSyntax + let(:group) { create(:group) } let(:project) { create(:project) } it { is_expected.to validate_presence_of(:type) } - it 'validates presence of project_id if not template, instance or group_id', :aggregate_failures do - expect(build(:service, project_id: nil, template: true)).to be_valid - expect(build(:service, project_id: nil, instance: true)).to be_valid - expect(build(:service, project_id: nil, group_id: group.id)).to be_valid - expect(build(:service, project_id: nil, template: false)).to be_invalid - expect(build(:service, project_id: nil, instance: false)).to be_invalid - expect(build(:service, project_id: nil, group_id: nil)).to be_invalid - end - - it 'validates presence of group_id if not template, instance or project_id', :aggregate_failures do - expect(build(:service, group_id: nil, project_id: nil, template: true)).to be_valid - expect(build(:service, group_id: nil, project_id: nil, instance: true)).to be_valid - expect(build(:service, group_id: nil, project_id: project.id)).to be_valid - expect(build(:service, group_id: nil, project_id: nil, template: false)).to be_invalid - expect(build(:service, group_id: nil, project_id: nil, instance: false)).to be_invalid - expect(build(:service, group_id: nil, project_id: nil)).to be_invalid - end - - it 'validates absence of project_id if instance or template', :aggregate_failures do - expect(build(:service, instance: true)).to validate_absence_of(:project_id) - expect(build(:service, template: true)).to validate_absence_of(:project_id) - expect(build(:service, instance: false)).not_to validate_absence_of(:project_id) - expect(build(:service, template: false)).not_to validate_absence_of(:project_id) - end - - it 'validates absence of group_id if instance or template', :aggregate_failures do - expect(build(:service, instance: true)).to validate_absence_of(:group_id) - expect(build(:service, template: true)).to validate_absence_of(:group_id) - expect(build(:service, instance: false)).not_to validate_absence_of(:group_id) - expect(build(:service, template: false)).not_to validate_absence_of(:group_id) - end - - it 'validates service is template or instance' do - expect(build(:service, project_id: nil, template: true, instance: true)).to be_invalid + # We need to use a Proc object because let variables are not available on + # this context, see: https://github.com/tomykaira/rspec-parameterized/issues/8 + where(:project_id, :group_id, :template, :instance, :valid) do + -> { project.id } | nil | false | false | true + nil | -> { group.id } | false | false | true + nil | nil | true | false | true + nil | nil | false | true | true + nil | nil | false | false | false + nil | nil | true | true | false + -> { project.id } | nil | true | false | false + -> { project.id } | nil | false | true | false + nil | -> { group.id } | true | false | false + nil | -> { group.id } | false | true | false + end + + with_them do + it 'validates the service' do + expect(build(:service, project_id: project_id.try(:call), group_id: group_id.try(:call), template: template, instance: instance).valid?).to eq(valid) + end end context 'with an existing service template' do -- GitLab From 81fa9f67c4bcadfd05e5212d7e6bfef7adacca77 Mon Sep 17 00:00:00 2001 From: Arturo Herrero Date: Fri, 7 Aug 2020 11:09:55 +0200 Subject: [PATCH 3/5] Remove Proc objects from RSpec parameterized table --- spec/models/service_spec.rb | 24 +++++++++++------------- 1 file changed, 11 insertions(+), 13 deletions(-) diff --git a/spec/models/service_spec.rb b/spec/models/service_spec.rb index 8a80de61679c47..7de34dec61d60a 100644 --- a/spec/models/service_spec.rb +++ b/spec/models/service_spec.rb @@ -18,24 +18,22 @@ it { is_expected.to validate_presence_of(:type) } - # We need to use a Proc object because let variables are not available on - # this context, see: https://github.com/tomykaira/rspec-parameterized/issues/8 where(:project_id, :group_id, :template, :instance, :valid) do - -> { project.id } | nil | false | false | true - nil | -> { group.id } | false | false | true - nil | nil | true | false | true - nil | nil | false | true | true - nil | nil | false | false | false - nil | nil | true | true | false - -> { project.id } | nil | true | false | false - -> { project.id } | nil | false | true | false - nil | -> { group.id } | true | false | false - nil | -> { group.id } | false | true | false + 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 | nil | true | false | false + 1 | nil | false | true | false + nil | 1 | true | false | false + nil | 1 | false | true | false end with_them do it 'validates the service' do - expect(build(:service, project_id: project_id.try(:call), group_id: group_id.try(:call), template: template, instance: instance).valid?).to eq(valid) + expect(build(:service, project_id: project_id, group_id: group_id, template: template, instance: instance).valid?).to eq(valid) end end -- GitLab From f8af785495aa37d1ce22172a108c551c4dce91e4 Mon Sep 17 00:00:00 2001 From: Arturo Herrero Date: Fri, 7 Aug 2020 11:10:40 +0200 Subject: [PATCH 4/5] Validate service belongs to project or group --- app/models/service.rb | 5 +++++ spec/models/service_spec.rb | 1 + 2 files changed, 6 insertions(+) diff --git a/app/models/service.rb b/app/models/service.rb index 16d6f6012e925a..64a3d8f9bdc69a 100644 --- a/app/models/service.rb +++ b/app/models/service.rb @@ -57,6 +57,7 @@ class Service < ApplicationRecord 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 } @@ -379,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 should belongs to a project or 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/spec/models/service_spec.rb b/spec/models/service_spec.rb index 7de34dec61d60a..c4a9c0329c781c 100644 --- a/spec/models/service_spec.rb +++ b/spec/models/service_spec.rb @@ -25,6 +25,7 @@ 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 -- GitLab From a54882153a6355cf81074f8940f3ea735457db05 Mon Sep 17 00:00:00 2001 From: Robert Speicher Date: Fri, 7 Aug 2020 15:24:15 +0000 Subject: [PATCH 5/5] Apply 1 suggestion(s) to 1 file(s) --- app/models/service.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/models/service.rb b/app/models/service.rb index 64a3d8f9bdc69a..40e7e5552d1757 100644 --- a/app/models/service.rb +++ b/app/models/service.rb @@ -381,7 +381,7 @@ def validate_is_instance_or_template end def validate_belongs_to_project_or_group - errors.add(:project_id, 'The service should belongs to a project or group') if project_id && group_id + 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 -- GitLab