From c95c3dfc05b8eeb6e73c2c5f4cbccebb0ea75bd9 Mon Sep 17 00:00:00 2001 From: manojmj Date: Fri, 3 Jul 2020 15:37:10 +0530 Subject: [PATCH 1/5] Backfill missing routes for Bot Users This change backfills missing routes for Bot Users --- .../backfill-routes-for-users-migration.yml | 5 ++ ...064117_generate_missing_routes_for_bots.rb | 85 +++++++++++++++++++ db/structure.sql | 1 + .../generate_missing_routes_for_bots_spec.rb | 65 ++++++++++++++ 4 files changed, 156 insertions(+) create mode 100644 changelogs/unreleased/backfill-routes-for-users-migration.yml create mode 100644 db/post_migrate/20200703064117_generate_missing_routes_for_bots.rb create mode 100644 spec/migrations/generate_missing_routes_for_bots_spec.rb diff --git a/changelogs/unreleased/backfill-routes-for-users-migration.yml b/changelogs/unreleased/backfill-routes-for-users-migration.yml new file mode 100644 index 00000000000000..c883d95ca0d90c --- /dev/null +++ b/changelogs/unreleased/backfill-routes-for-users-migration.yml @@ -0,0 +1,5 @@ +--- +title: Backfill missing routes for Bot users +merge_request: 35960 +author: +type: fixed diff --git a/db/post_migrate/20200703064117_generate_missing_routes_for_bots.rb b/db/post_migrate/20200703064117_generate_missing_routes_for_bots.rb new file mode 100644 index 00000000000000..adfc715b5c7272 --- /dev/null +++ b/db/post_migrate/20200703064117_generate_missing_routes_for_bots.rb @@ -0,0 +1,85 @@ +# frozen_string_literal: true + +class GenerateMissingRoutesForBots < ActiveRecord::Migration[6.0] + DOWNTIME = false + + disable_ddl_transaction! + + class User < ActiveRecord::Base + self.table_name = 'users' + + USER_TYPES = { + human: nil, + support_bot: 1, + alert_bot: 2, + visual_review_bot: 3, + service_user: 4, + ghost: 5, + project_bot: 6, + migration_bot: 7 + }.with_indifferent_access.freeze + + BOT_USER_TYPES = %w[alert_bot project_bot support_bot visual_review_bot migration_bot].freeze + + scope :bots, -> { where(user_type: USER_TYPES.values_at(*BOT_USER_TYPES)) } + end + + class Route < ActiveRecord::Base + self.table_name = 'routes' + + validates :path, + length: { within: 1..255 }, + presence: true, + uniqueness: { case_sensitive: false } + end + + module Routable + def attributes_for_insert + { + source_type: source_type_for_route, + source_id: id, + name: name, + path: path + } + end + end + + class Namespace < ActiveRecord::Base + self.table_name = 'namespaces' + + include GenerateMissingRoutesForBots::Routable + + belongs_to :owner, class_name: 'GenerateMissingRoutesForBots::User' + + scope :for_user, -> { where('type IS NULL') } + scope :for_bots, -> { for_user.joins(:owner).merge(GenerateMissingRoutesForBots::User.bots) } + + scope :without_routes, -> do + where( + 'NOT EXISTS ( + SELECT 1 + FROM routes + WHERE source_type = ? + AND source_id = namespaces.id + )', + 'Namespace' + ) + end + + def source_type_for_route + 'Namespace' + end + end + + def up + GenerateMissingRoutesForBots::Namespace.for_bots.without_routes.each do |namespace| + GenerateMissingRoutesForBots::Route.create(namespace.attributes_for_insert) + # this will gracefully fail, if there is already a route with the same path, + # due to the `uniqueness` check + end + end + + def down + # no op + end +end diff --git a/db/structure.sql b/db/structure.sql index 03f5b95313e1e1..2f9a661f0a0b00 100644 --- a/db/structure.sql +++ b/db/structure.sql @@ -23636,6 +23636,7 @@ COPY "schema_migrations" (version) FROM STDIN; 20200701093859 20200701205710 20200702123805 +20200703064117 20200703121557 20200703154822 20200704143633 diff --git a/spec/migrations/generate_missing_routes_for_bots_spec.rb b/spec/migrations/generate_missing_routes_for_bots_spec.rb new file mode 100644 index 00000000000000..c130003f8a00dd --- /dev/null +++ b/spec/migrations/generate_missing_routes_for_bots_spec.rb @@ -0,0 +1,65 @@ +# frozen_string_literal: true + +require 'spec_helper' + +require Rails.root.join('db', 'post_migrate', '20200703064117_generate_missing_routes_for_bots.rb') + +RSpec.describe GenerateMissingRoutesForBots, :migration do + let(:users) { table(:users) } + let(:namespaces) { table(:namespaces) } + let(:routes) { table(:routes) } + + let(:visual_review_bot) do + users.create!(email: 'visual-review-bot@gitlab.com', name: 'GitLab Visual Review Bot', username: 'visual-review-bot', user_type: 3, projects_limit: 5) + end + + let!(:visual_review_bot_namespace) do + namespaces.create!(owner_id: visual_review_bot.id, name: 'GitLab Visual Review Bot', path: 'visual-review-bot') + end + + let(:migration_bot) do + users.create!(email: 'migration-bot@gitlab.com', name: 'GitLab Migration Bot', username: 'migration-bot', user_type: 7, projects_limit: 5) + end + + let!(:migration_bot_namespace) do + namespaces.create!(owner_id: migration_bot.id, name: 'GitLab Migration Bot', path: 'migration-bot') + end + + it 'creates routes for bot users without an existing route' do + migrate! + + visual_review_bot_route = routes.find_by(source_type: 'Namespace', source_id: visual_review_bot_namespace.id) + + expect(visual_review_bot_route).to be_present + expect(visual_review_bot_route.path).to eq('visual-review-bot') + expect(visual_review_bot_route.name).to eq('GitLab Visual Review Bot') + + migration_bot_route = routes.find_by(source_type: 'Namespace', source_id: migration_bot_namespace.id) + + expect(migration_bot_route).to be_present + expect(migration_bot_route.path).to eq('migration-bot') + expect(migration_bot_route.name).to eq('GitLab Migration Bot') + end + + it 'does not create routes for bot users with existing routes' do + routes.create!(path: 'visual-review-bot', name: 'GitLab Visual Review Bot', source_id: visual_review_bot_namespace.id, source_type: 'Namespace') + routes.create!(path: 'migration-bot', name: 'GitLab Migration Bot', source_id: migration_bot.id, source_type: 'Namespace') + + expect { migrate! }.not_to change { routes.count } + end + + it 'does not create routes for human users without an existing route' do + human = users.create!(email: 'human@gitlab.com', name: 'GitLab Human', username: 'human', user_type: nil, projects_limit: 5) + human_namespace = namespaces.create!(owner_id: human, name: 'GitLab Human', path: 'human') + + expect { migrate! }.not_to change { routes.where(source_type: 'Namespace', source_id: human_namespace.id).count } + end + + it 'does not create route for a bot user with a missing route if a human user with the same route already exists' do + human = users.create!(email: 'human@gitlab.com', name: 'GitLab Visual Review Bot', username: 'gitlab-visual-review-bot', user_type: nil, projects_limit: 5) + human_namespace = namespaces.create!(owner_id: human.id, name: 'GitLab Visual Review Bot', path: 'visual-review-bot') + routes.create!(path: 'visual-review-bot', name: 'GitLab Visual Review Bot', source_id: human_namespace.id, source_type: 'Namespace') + + expect { migrate! }.not_to change { routes.where(source_type: 'Namespace', source_id: visual_review_bot_namespace.id).count } + end +end -- GitLab From 64dd5c2adf4bf64c724882037eed44506718be2f Mon Sep 17 00:00:00 2001 From: manojmj Date: Tue, 7 Jul 2020 12:22:58 +0530 Subject: [PATCH 2/5] Address code review comments Address code review comments --- ...064117_generate_missing_routes_for_bots.rb | 28 ++++----- .../generate_missing_routes_for_bots_spec.rb | 60 +++++++++++-------- 2 files changed, 46 insertions(+), 42 deletions(-) diff --git a/db/post_migrate/20200703064117_generate_missing_routes_for_bots.rb b/db/post_migrate/20200703064117_generate_missing_routes_for_bots.rb index adfc715b5c7272..2a5dbe7ebf76c5 100644 --- a/db/post_migrate/20200703064117_generate_missing_routes_for_bots.rb +++ b/db/post_migrate/20200703064117_generate_missing_routes_for_bots.rb @@ -28,27 +28,12 @@ class Route < ActiveRecord::Base self.table_name = 'routes' validates :path, - length: { within: 1..255 }, - presence: true, uniqueness: { case_sensitive: false } end - module Routable - def attributes_for_insert - { - source_type: source_type_for_route, - source_id: id, - name: name, - path: path - } - end - end - class Namespace < ActiveRecord::Base self.table_name = 'namespaces' - include GenerateMissingRoutesForBots::Routable - belongs_to :owner, class_name: 'GenerateMissingRoutesForBots::User' scope :for_user, -> { where('type IS NULL') } @@ -62,13 +47,22 @@ class Namespace < ActiveRecord::Base WHERE source_type = ? AND source_id = namespaces.id )', - 'Namespace' + self.source_type_for_route ) end - def source_type_for_route + def self.source_type_for_route 'Namespace' end + + def attributes_for_insert + { + source_type: self.class.source_type_for_route, + source_id: id, + name: name, + path: path + } + end end def up diff --git a/spec/migrations/generate_missing_routes_for_bots_spec.rb b/spec/migrations/generate_missing_routes_for_bots_spec.rb index c130003f8a00dd..44e8fda28c8763 100644 --- a/spec/migrations/generate_missing_routes_for_bots_spec.rb +++ b/spec/migrations/generate_missing_routes_for_bots_spec.rb @@ -13,53 +13,63 @@ users.create!(email: 'visual-review-bot@gitlab.com', name: 'GitLab Visual Review Bot', username: 'visual-review-bot', user_type: 3, projects_limit: 5) end - let!(:visual_review_bot_namespace) do - namespaces.create!(owner_id: visual_review_bot.id, name: 'GitLab Visual Review Bot', path: 'visual-review-bot') - end - let(:migration_bot) do users.create!(email: 'migration-bot@gitlab.com', name: 'GitLab Migration Bot', username: 'migration-bot', user_type: 7, projects_limit: 5) end + let!(:visual_review_bot_namespace) do + namespaces.create!(owner_id: visual_review_bot.id, name: visual_review_bot.name, path: visual_review_bot.username) + end + let!(:migration_bot_namespace) do - namespaces.create!(owner_id: migration_bot.id, name: 'GitLab Migration Bot', path: 'migration-bot') + namespaces.create!(owner_id: migration_bot.id, name: migration_bot.name, path: migration_bot.username) end it 'creates routes for bot users without an existing route' do migrate! - visual_review_bot_route = routes.find_by(source_type: 'Namespace', source_id: visual_review_bot_namespace.id) - - expect(visual_review_bot_route).to be_present - expect(visual_review_bot_route.path).to eq('visual-review-bot') - expect(visual_review_bot_route.name).to eq('GitLab Visual Review Bot') - - migration_bot_route = routes.find_by(source_type: 'Namespace', source_id: migration_bot_namespace.id) + [visual_review_bot, migration_bot].each do |bot| + namespace = namespaces.find_by(owner_id: bot.id) + route = route_for(namespace: namespace) - expect(migration_bot_route).to be_present - expect(migration_bot_route.path).to eq('migration-bot') - expect(migration_bot_route.name).to eq('GitLab Migration Bot') + expect(route).to be_present + expect(route.path).to eq(namespace.path) + expect(route.name).to eq(namespace.name) + end end it 'does not create routes for bot users with existing routes' do - routes.create!(path: 'visual-review-bot', name: 'GitLab Visual Review Bot', source_id: visual_review_bot_namespace.id, source_type: 'Namespace') - routes.create!(path: 'migration-bot', name: 'GitLab Migration Bot', source_id: migration_bot.id, source_type: 'Namespace') + create_route!(namespace: visual_review_bot_namespace) + create_route!(namespace: migration_bot_namespace) expect { migrate! }.not_to change { routes.count } end it 'does not create routes for human users without an existing route' do - human = users.create!(email: 'human@gitlab.com', name: 'GitLab Human', username: 'human', user_type: nil, projects_limit: 5) - human_namespace = namespaces.create!(owner_id: human, name: 'GitLab Human', path: 'human') + human_namespace = create_human_namespace!(name: 'GitLab Human', username: 'human') - expect { migrate! }.not_to change { routes.where(source_type: 'Namespace', source_id: human_namespace.id).count } + expect { migrate! }.not_to change { route_for(namespace: human_namespace) } end - it 'does not create route for a bot user with a missing route if a human user with the same route already exists' do - human = users.create!(email: 'human@gitlab.com', name: 'GitLab Visual Review Bot', username: 'gitlab-visual-review-bot', user_type: nil, projects_limit: 5) - human_namespace = namespaces.create!(owner_id: human.id, name: 'GitLab Visual Review Bot', path: 'visual-review-bot') - routes.create!(path: 'visual-review-bot', name: 'GitLab Visual Review Bot', source_id: human_namespace.id, source_type: 'Namespace') + it 'does not create route for a bot user with a missing route, if a human user with the same path already exists' do + human_namespace = create_human_namespace!(name: visual_review_bot.name, username: visual_review_bot.username) + create_route!(namespace: human_namespace) + + expect { migrate! }.not_to change { route_for(namespace: visual_review_bot_namespace) } + end + + private + + def create_human_namespace!(name:, username:) + human = users.create!(email: 'human@gitlab.com', name: name, username: username, user_type: nil, projects_limit: 5) + namespaces.create!(owner_id: human.id, name: human.name, path: human.username) + end + + def create_route!(namespace:) + routes.create!(path: namespace.path, name: namespace.name, source_id: namespace.id, source_type: 'Namespace') + end - expect { migrate! }.not_to change { routes.where(source_type: 'Namespace', source_id: visual_review_bot_namespace.id).count } + def route_for(namespace:) + routes.find_by(source_type: 'Namespace', source_id: namespace.id) end end -- GitLab From 7093058dbf6aadcd5a3a582ad855b9e8e4aaf8b8 Mon Sep 17 00:00:00 2001 From: manojmj Date: Tue, 7 Jul 2020 13:40:38 +0530 Subject: [PATCH 3/5] Add logging for migration --- ...0703064117_generate_missing_routes_for_bots.rb | 15 ++++++++++++--- 1 file changed, 12 insertions(+), 3 deletions(-) diff --git a/db/post_migrate/20200703064117_generate_missing_routes_for_bots.rb b/db/post_migrate/20200703064117_generate_missing_routes_for_bots.rb index 2a5dbe7ebf76c5..a88844ed6be796 100644 --- a/db/post_migrate/20200703064117_generate_missing_routes_for_bots.rb +++ b/db/post_migrate/20200703064117_generate_missing_routes_for_bots.rb @@ -66,10 +66,19 @@ def attributes_for_insert end def up + logger = Gitlab::BackgroundMigration::Logger.build + attributes_to_be_logged = %w(id path name) + GenerateMissingRoutesForBots::Namespace.for_bots.without_routes.each do |namespace| - GenerateMissingRoutesForBots::Route.create(namespace.attributes_for_insert) - # this will gracefully fail, if there is already a route with the same path, - # due to the `uniqueness` check + route = GenerateMissingRoutesForBots::Route.create(namespace.attributes_for_insert) + namespace_details = namespace.as_json.slice(*attributes_to_be_logged) + + if route.persisted? + logger.info namespace_details.merge(message: 'a new route was created for the namespace') + else + errors = route.errors.full_messages.join(',') + logger.info namespace_details.merge(message: 'route creation failed for the namespace', errors: errors) + end end end -- GitLab From 36276109e42d1bba6a9c2917557a8d2cf869ca41 Mon Sep 17 00:00:00 2001 From: manojmj Date: Wed, 8 Jul 2020 10:37:48 +0530 Subject: [PATCH 4/5] Add `reset_column_information` to reset table information cache --- .../20200703064117_generate_missing_routes_for_bots.rb | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/db/post_migrate/20200703064117_generate_missing_routes_for_bots.rb b/db/post_migrate/20200703064117_generate_missing_routes_for_bots.rb index a88844ed6be796..85d62cbb6ddbe7 100644 --- a/db/post_migrate/20200703064117_generate_missing_routes_for_bots.rb +++ b/db/post_migrate/20200703064117_generate_missing_routes_for_bots.rb @@ -66,6 +66,10 @@ def attributes_for_insert end def up + # Reset the column information of all the models that update the database + # to ensure the Active Record's knowledge of the table structure is current + Route.reset_column_information + logger = Gitlab::BackgroundMigration::Logger.build attributes_to_be_logged = %w(id path name) -- GitLab From e3f23409eae326b4eff19888dfbf10a0f2c87849 Mon Sep 17 00:00:00 2001 From: manojmj Date: Fri, 10 Jul 2020 11:40:21 +0530 Subject: [PATCH 5/5] Address code review comments --- .../generate_missing_routes_for_bots_spec.rb | 21 ++++++++++++------- 1 file changed, 13 insertions(+), 8 deletions(-) diff --git a/spec/migrations/generate_missing_routes_for_bots_spec.rb b/spec/migrations/generate_missing_routes_for_bots_spec.rb index 44e8fda28c8763..8af22042350750 100644 --- a/spec/migrations/generate_missing_routes_for_bots_spec.rb +++ b/spec/migrations/generate_missing_routes_for_bots_spec.rb @@ -25,16 +25,21 @@ namespaces.create!(owner_id: migration_bot.id, name: migration_bot.name, path: migration_bot.username) end - it 'creates routes for bot users without an existing route' do - migrate! + context 'for bot users without an existing route' do + it 'creates new routes' do + expect { migrate! }.to change { routes.count }.by(2) + end + + it 'creates new routes with the same path and name as their namespace' do + migrate! - [visual_review_bot, migration_bot].each do |bot| - namespace = namespaces.find_by(owner_id: bot.id) - route = route_for(namespace: namespace) + [visual_review_bot, migration_bot].each do |bot| + namespace = namespaces.find_by(owner_id: bot.id) + route = route_for(namespace: namespace) - expect(route).to be_present - expect(route.path).to eq(namespace.path) - expect(route.name).to eq(namespace.name) + expect(route.path).to eq(namespace.path) + expect(route.name).to eq(namespace.name) + end end end -- GitLab