Merge branch 'validate-that-foreign-keys-are-created-ee' into 'master'

EE: Validate that foreign keys are created

Closes gitlab-ce#50875

See merge request gitlab-org/gitlab-ee!8232

changelogs/unreleased/validate-foreign-keys-being-indexed.yml

+---
+title: Validate foreign keys being created and indexed for column with _id 
+merge_request: 22808
+author:
+type: performance

db/migrate/20181030154446_add_missing_indexes_for_foreign_keys.rb

+# frozen_string_literal: true
+
+class AddMissingIndexesForForeignKeys < ActiveRecord::Migration
+  include Gitlab::Database::MigrationHelpers
+
+  DOWNTIME = false
+
+  disable_ddl_transaction!
+
+  def up
+    add_concurrent_index(:application_settings, :usage_stats_set_by_user_id)
+    add_concurrent_index(:ci_pipeline_schedules, :owner_id)
+    add_concurrent_index(:ci_trigger_requests, :trigger_id)
+    add_concurrent_index(:ci_triggers, :owner_id)
+    add_concurrent_index(:clusters_applications_helm, :cluster_id, unique: true)
+    add_concurrent_index(:clusters_applications_ingress, :cluster_id, unique: true)
+    add_concurrent_index(:clusters_applications_jupyter, :cluster_id, unique: true)
+    add_concurrent_index(:clusters_applications_jupyter, :oauth_application_id)
+    add_concurrent_index(:clusters_applications_knative, :cluster_id, unique: true)
+    add_concurrent_index(:clusters_applications_prometheus, :cluster_id, unique: true)
+    add_concurrent_index(:fork_network_members, :forked_from_project_id)
+    add_concurrent_index(:internal_ids, :namespace_id)
+    add_concurrent_index(:internal_ids, :project_id)
+    add_concurrent_index(:issues, :closed_by_id)
+    add_concurrent_index(:label_priorities, :label_id)
+    add_concurrent_index(:merge_request_metrics, :merged_by_id)
+    add_concurrent_index(:merge_request_metrics, :latest_closed_by_id)
+    add_concurrent_index(:oauth_openid_requests, :access_grant_id)
+    add_concurrent_index(:project_deploy_tokens, :deploy_token_id)
+    add_concurrent_index(:protected_tag_create_access_levels, :group_id)
+    add_concurrent_index(:subscriptions, :project_id)
+    add_concurrent_index(:user_statuses, :user_id)
+    add_concurrent_index(:users, :accepted_term_id)
+  end
+
+  def down
+    # MySQL requires index for FK,
+    # thus removal of indexes does fail
+    return if Gitlab::Database.mysql?
+
+    remove_concurrent_index(:application_settings, :usage_stats_set_by_user_id)
+    remove_concurrent_index(:ci_pipeline_schedules, :owner_id)
+    remove_concurrent_index(:ci_trigger_requests, :trigger_id)
+    remove_concurrent_index(:ci_triggers, :owner_id)
+    remove_concurrent_index(:clusters_applications_helm, :cluster_id, unique: true)
+    remove_concurrent_index(:clusters_applications_ingress, :cluster_id, unique: true)
+    remove_concurrent_index(:clusters_applications_jupyter, :cluster_id, unique: true)
+    remove_concurrent_index(:clusters_applications_jupyter, :oauth_application_id)
+    remove_concurrent_index(:clusters_applications_knative, :cluster_id, unique: true)
+    remove_concurrent_index(:clusters_applications_prometheus, :cluster_id, unique: true)
+    remove_concurrent_index(:fork_network_members, :forked_from_project_id)
+    remove_concurrent_index(:internal_ids, :namespace_id)
+    remove_concurrent_index(:internal_ids, :project_id)
+    remove_concurrent_index(:issues, :closed_by_id)
+    remove_concurrent_index(:label_priorities, :label_id)
+    remove_concurrent_index(:merge_request_metrics, :merged_by_id)
+    remove_concurrent_index(:merge_request_metrics, :latest_closed_by_id)
+    remove_concurrent_index(:oauth_openid_requests, :access_grant_id)
+    remove_concurrent_index(:project_deploy_tokens, :deploy_token_id)
+    remove_concurrent_index(:protected_tag_create_access_levels, :group_id)
+    remove_concurrent_index(:subscriptions, :project_id)
+    remove_concurrent_index(:user_statuses, :user_id)
+    remove_concurrent_index(:users, :accepted_term_id)
+  end
+end

doc/development/migration_style_guide.md

@@ -187,12 +187,7 @@ end
 When adding a foreign-key constraint to either an existing or new
 column remember to also add a index on the column.
 
-This is _required_ if the foreign-key constraint specifies
-`ON DELETE CASCADE` or `ON DELETE SET NULL` behavior. On a cascading
-delete, the [corresponding record needs to be retrieved using an
-index](https://www.cybertec-postgresql.com/en/postgresql-indexes-and-foreign-keys/)
-(otherwise, we'd need to scan the whole table) for subsequent update or
-deletion.
+This is _required_ for all foreign-keys.
 
 Here's an example where we add a new column with a foreign key
 constraint. Note it includes `index: true` to create an index for it.

ee/db/migrate/20181105122803_add_missing_indexes_for_foreign_keys_ee.rb

+# frozen_string_literal: true
+
+class AddMissingIndexesForForeignKeysEE < ActiveRecord::Migration
+  include Gitlab::Database::MigrationHelpers
+
+  DOWNTIME = false
+
+  disable_ddl_transaction!
+
+  def up
+    add_concurrent_index(:application_settings, :file_template_project_id)
+    add_concurrent_index(:application_settings, :custom_project_templates_group_id)
+    add_concurrent_index(:board_assignees, :assignee_id)
+    add_concurrent_index(:board_labels, :label_id)
+    add_concurrent_index(:ci_pipeline_chat_data, :chat_name_id)
+    add_concurrent_index(:geo_node_namespace_links, :namespace_id)
+    add_concurrent_index(:namespaces, :file_template_project_id)
+    add_concurrent_index(:protected_branch_merge_access_levels, :group_id)
+    add_concurrent_index(:protected_branch_push_access_levels, :group_id)
+    add_concurrent_index(:software_license_policies, :software_license_id)
+  end
+
+  def down
+    # MySQL requires index for FK,
+    # thus removal of indexes does fail
+    return if Gitlab::Database.mysql?
+
+    remove_concurrent_index(:application_settings, :file_template_project_id)
+    remove_concurrent_index(:application_settings, :custom_project_templates_group_id)
+    remove_concurrent_index(:board_assignees, :assignee_id)
+    remove_concurrent_index(:board_labels, :label_id)
+    remove_concurrent_index(:ci_pipeline_chat_data, :chat_name_id)
+    remove_concurrent_index(:geo_node_namespace_links, :namespace_id)
+    remove_concurrent_index(:namespaces, :file_template_project_id)
+    remove_concurrent_index(:protected_branch_merge_access_levels, :group_id)
+    remove_concurrent_index(:protected_branch_push_access_levels, :group_id)
+    remove_concurrent_index(:software_license_policies, :software_license_id)
+  end
+end

ee/spec/db/schema_support.rb

+# frozen_string_literal: true
+
+module EE
+  module DB
+    module SchemaSupport
+      extend ActiveSupport::Concern
+
+      prepended do
+        EE_IGNORED_FK_COLUMNS = {
+          application_settings: %w[slack_app_id snowplow_site_id],
+          approvals: %w[user_id],
+          approver_groups: %w[target_id],
+          approvers: %w[target_id user_id],
+          boards: %w[milestone_id],
+          draft_notes: %w[discussion_id],
+          epics: %w[updated_by_id last_edited_by_id start_date_sourcing_milestone_id due_date_sourcing_milestone_id],
+          geo_event_log: %w[hashed_storage_attachments_event_id],
+          geo_job_artifact_deleted_events: %w[job_artifact_id],
+          geo_lfs_object_deleted_events: %w[lfs_object_id],
+          geo_node_statuses: %w[last_event_id cursor_last_event_id],
+          geo_nodes: %w[oauth_application_id],
+          geo_repository_deleted_events: %w[project_id],
+          geo_upload_deleted_events: %w[upload_id model_id],
+          ldap_group_links: %w[group_id],
+          projects: %w[mirror_user_id],
+          slack_integrations: %w[team_id user_id],
+          users: %w[email_opted_in_source_id],
+          vulnerability_identifiers: %w[external_id],
+          vulnerability_scanners: %w[external_id],
+          web_hooks: %w[group_id]
+        }.with_indifferent_access.freeze
+      end
+
+      def ignored_fk_columns(column)
+        super + EE_IGNORED_FK_COLUMNS.fetch(column, [])
+      end
+    end
+  end
+end

spec/db/schema_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+require Rails.root.join('ee', 'spec', 'db', 'schema_support')
+
+describe 'Database schema' do
+  prepend ::EE::DB::SchemaSupport
+
+  let(:connection) { ActiveRecord::Base.connection }
+  let(:tables) { connection.tables }
+
+  # Use if you are certain that this column should not have a foreign key
+  # EE: edit the ee/spec/db/schema_support.rb
+  IGNORED_FK_COLUMNS = {
+    abuse_reports: %w[reporter_id user_id],
+    application_settings: %w[performance_bar_allowed_group_id],
+    audit_events: %w[author_id entity_id],
+    award_emoji: %w[awardable_id user_id],
+    chat_names: %w[chat_id service_id team_id user_id],
+    chat_teams: %w[team_id],
+    ci_builds: %w[erased_by_id runner_id trigger_request_id user_id],
+    ci_pipelines: %w[user_id],
+    ci_runner_projects: %w[runner_id],
+    ci_trigger_requests: %w[commit_id],
+    cluster_providers_gcp: %w[gcp_project_id operation_id],
+    deploy_keys_projects: %w[deploy_key_id],
+    deployments: %w[deployable_id environment_id user_id],
+    emails: %w[user_id],
+    events: %w[target_id],
+    forked_project_links: %w[forked_from_project_id],
+    identities: %w[user_id],
+    issues: %w[last_edited_by_id],
+    keys: %w[user_id],
+    label_links: %w[target_id],
+    lfs_objects_projects: %w[lfs_object_id project_id],
+    members: %w[source_id created_by_id],
+    merge_requests: %w[last_edited_by_id],
+    namespaces: %w[owner_id parent_id],
+    notes: %w[author_id commit_id noteable_id updated_by_id resolved_by_id discussion_id],
+    notification_settings: %w[source_id],
+    oauth_access_grants: %w[resource_owner_id application_id],
+    oauth_access_tokens: %w[resource_owner_id application_id],
+    oauth_applications: %w[owner_id],
+    project_group_links: %w[group_id],
+    project_statistics: %w[namespace_id],
+    projects: %w[creator_id namespace_id ci_id],
+    redirect_routes: %w[source_id],
+    repository_languages: %w[programming_language_id],
+    routes: %w[source_id],
+    sent_notifications: %w[project_id noteable_id recipient_id commit_id in_reply_to_discussion_id],
+    snippets: %w[author_id],
+    spam_logs: %w[user_id],
+    subscriptions: %w[user_id subscribable_id],
+    taggings: %w[tag_id taggable_id tagger_id],
+    timelogs: %w[user_id],
+    todos: %w[target_id commit_id],
+    uploads: %w[model_id],
+    user_agent_details: %w[subject_id],
+    users: %w[color_scheme_id created_by_id theme_id],
+    users_star_projects: %w[user_id],
+    web_hooks: %w[service_id]
+  }.with_indifferent_access.freeze
+
+  context 'for table' do
+    ActiveRecord::Base.connection.tables.sort.each do |table|
+      describe table do
+        let(:indexes) { connection.indexes(table) }
+        let(:columns) { connection.columns(table) }
+        let(:foreign_keys) { connection.foreign_keys(table) }
+
+        context 'all foreign keys' do
+          # for index to be effective, the FK constraint has to be at first place
+          it 'are indexed' do
+            first_indexed_column = indexes.map(&:columns).map(&:first)
+            foreign_keys_columns = foreign_keys.map(&:column)
+
+            expect(first_indexed_column.uniq).to include(*foreign_keys_columns)
+          end
+        end
+
+        context 'columns ending with _id' do
+          let(:column_names) { columns.map(&:name) }
+          let(:column_names_with_id) { column_names.select { |column_name| column_name.ends_with?('_id') } }
+          let(:foreign_keys_columns) { foreign_keys.map(&:column) }
+          let(:ignored_columns) { ignored_fk_columns(table) }
+
+          it 'do have the foreign keys' do
+            expect(column_names_with_id - ignored_columns).to contain_exactly(*foreign_keys_columns)
+          end
+        end
+      end
+    end
+  end
+
+  private
+
+  def ignored_fk_columns(column)
+    IGNORED_FK_COLUMNS.fetch(column, [])
+  end
+end