Merge branch 'tc-cleanup-schema-spec' into 'master'

Better testing and documentation on columns with _id suffix

Closes #212218

See merge request gitlab-org/gitlab!34757

doc/development/foreign_keys.md

@@ -35,6 +35,17 @@ When adding a foreign key in PostgreSQL the column is not indexed automatically,
 thus you must also add a concurrent index. Not doing so will result in cascading
 deletes being very slow.
 
+## Naming foreign keys
+
+By default Ruby on Rails uses the `_id` suffix for foreign keys. So we should
+only use this suffix for associations between two tables. If you want to
+reference an ID on a third party platform the `_xid` suffix is recommended.
+
+The spec `spec/db/schema_spec.rb` will test if all columns with the `_id` suffix
+have a foreign key constraint. So if that spec fails, don't add the column to
+`IGNORED_FK_COLUMNS`, but instead add the FK constraint, or consider naming it
+differently.
+
 ## Dependent Removals
 
 Don't define options such as `dependent: :destroy` or `dependent: :delete` when

ee/spec/db/schema_support.rb

@@ -6,40 +6,12 @@ module EE
       extend ActiveSupport::Concern
 
       prepended do
-        EE_IGNORED_FK_COLUMNS = {
-          application_settings: %w[slack_app_id snowplow_app_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],
-          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
-
         IGNORED_LIMIT_ENUMS = {
           'SoftwareLicensePolicy' => %w[classification],
           'User' => %w[group_view]
         }.freeze
       end
 
-      def ignored_fk_columns(column)
-        super + EE_IGNORED_FK_COLUMNS.fetch(column, [])
-      end
-
       def ignored_limit_enums(model)
         super + IGNORED_LIMIT_ENUMS.fetch(model, [])
       end

spec/db/schema_spec.rb

@@ -10,8 +10,8 @@ RSpec.describe 'Database schema' do
   let(:tables) { connection.tables }
   let(:columns_name_with_jsonb) { retrieve_columns_name_with_jsonb }
 
-  # Use if you are certain that this column should not have a foreign key
-  # EE: edit the ee/spec/db/schema_support.rb
+  # List of columns historically missing a FK, don't add more columns
+  # See: https://docs.gitlab.com/ce/development/foreign_keys.html#naming-foreign-keys
   IGNORED_FK_COLUMNS = {
     abuse_reports: %w[reporter_id user_id],
     application_settings: %w[performance_bar_allowed_group_id slack_app_id snowplow_app_id eks_account_id eks_access_key_id],
@@ -119,7 +119,11 @@ RSpec.describe 'Database schema' do
           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)
+            expect(column_names_with_id - ignored_columns).to match_array(foreign_keys_columns)
+          end
+
+          it 'and having foreign key are not in the ignore list' do
+            expect(ignored_columns).to match_array(ignored_columns - foreign_keys)
           end
         end
       end