Merge branch 'fix-lfk-from-table-to-table-definition' into 'master'

Fix LFK foreign key definition

See merge request gitlab-org/gitlab!75626

app/services/loose_foreign_keys/cleaner_service.rb

@@ -16,7 +16,7 @@ module LooseForeignKeys
     def execute
       result = connection.execute(build_query)
 
-      { affected_rows: result.cmd_tuples, table: loose_foreign_key_definition.to_table }
+      { affected_rows: result.cmd_tuples, table: loose_foreign_key_definition.from_table }
     end
 
     def async_delete?
@@ -48,15 +48,15 @@ module LooseForeignKeys
     end
 
     def arel_table
-      @arel_table ||= Arel::Table.new(loose_foreign_key_definition.to_table)
+      @arel_table ||= Arel::Table.new(loose_foreign_key_definition.from_table)
     end
 
     def primary_keys
-      @primary_keys ||= connection.primary_keys(loose_foreign_key_definition.to_table).map { |key| arel_table[key] }
+      @primary_keys ||= connection.primary_keys(loose_foreign_key_definition.from_table).map { |key| arel_table[key] }
     end
 
     def quoted_table_name
-      @quoted_table_name ||= Arel.sql(connection.quote_table_name(loose_foreign_key_definition.to_table))
+      @quoted_table_name ||= Arel.sql(connection.quote_table_name(loose_foreign_key_definition.from_table))
     end
 
     def delete_query

doc/development/database/loose_foreign_keys.md

@@ -62,28 +62,28 @@ following information:
 - The data cleanup method (`async_delete` or `async_nullify`)
 
 The YAML file is located at `lib/gitlab/database/gitlab_loose_foreign_keys.yml`. The file groups
-foreign key definitions by the name of the parent table. The parent table can have multiple loose
+foreign key definitions by the name of the child table. The child table can have multiple loose
 foreign key definitions, therefore we store them as an array.
 
 Example definition:
 
 ```yaml
-projects:
-  - to_table: ci_pipelines
+ci_pipelines:
+  - table: projects
     column: project_id
     on_delete: async_delete
 ```
 
-If the `projects` key is already present in the YAML file, then a new entry can be added
+If the `ci_pipelines` key is already present in the YAML file, then a new entry can be added
 to the array:
 
 ```yaml
-projects:
-  - to_table: ci_pipelines
+ci_pipelines:
+  - table: projects
     column: project_id
     on_delete: async_delete
-  - to_table: another_table
-    column: project_id
+  - table: another_table
+    column: another_id
     on_delete: :async_nullify
 ```
 

lib/gitlab/database/gitlab_loose_foreign_keys.yml

-chat_names:
-  - to_table: ci_pipeline_chat_data
+ci_pipeline_chat_data:
+  - table: chat_names
     column: chat_name_id
     on_delete: async_delete
-ci_builds:
-  - to_table: dast_site_profiles_builds
+dast_scanner_profiles_builds:
+  - table: ci_builds
     column: ci_build_id
     on_delete: async_delete
-  - to_table: dast_scanner_profiles_builds
+dast_scanner_profiles_builds:
+  - table: ci_builds
     column: ci_build_id
     on_delete: async_delete
-ci_pipelines:
-  - to_table: dast_profiles_pipelines
+dast_profiles_pipelines:
+  - table: ci_pipelines
     column: ci_pipeline_id
     on_delete: async_delete
-ci_runners:
-  - to_table: clusters_applications_runners
+clusters_applications_runners:
+  - table: ci_runners
     column: runner_id
     on_delete: async_nullify

lib/gitlab/database/loose_foreign_keys.rb

@@ -4,25 +4,25 @@ module Gitlab
   module Database
     module LooseForeignKeys
       def self.definitions_by_table
-        @definitions_by_table ||= definitions.group_by(&:from_table).with_indifferent_access.freeze
+        @definitions_by_table ||= definitions.group_by(&:to_table).with_indifferent_access.freeze
       end
 
       def self.definitions
-        @definitions ||= loose_foreign_keys_yaml.flat_map do |parent_table_name, configs|
-          configs.map { |config| build_definition(parent_table_name, config) }
+        @definitions ||= loose_foreign_keys_yaml.flat_map do |child_table_name, configs|
+          configs.map { |config| build_definition(child_table_name, config) }
         end.freeze
       end
 
-      def self.build_definition(parent_table_name, config)
-        to_table = config.fetch('to_table')
+      def self.build_definition(child_table_name, config)
+        parent_table_name = config.fetch('table')
 
         ActiveRecord::ConnectionAdapters::ForeignKeyDefinition.new(
+          child_table_name,
           parent_table_name,
-          to_table,
           {
             column: config.fetch('column'),
             on_delete: config.fetch('on_delete').to_sym,
-            gitlab_schema: GitlabSchema.table_schema(to_table)
+            gitlab_schema: GitlabSchema.table_schema(child_table_name)
           }
         )
       end

spec/db/schema_spec.rb

@@ -29,7 +29,6 @@ RSpec.describe 'Database schema' do
     ci_builds: %w[erased_by_id runner_id trigger_request_id user_id],
     ci_namespace_monthly_usages: %w[namespace_id],
     ci_pipelines: %w[user_id],
-    ci_pipeline_chat_data: %w[chat_name_id], # it uses the loose foreign key featue
     ci_runner_projects: %w[runner_id],
     ci_trigger_requests: %w[commit_id],
     cluster_providers_aws: %w[security_group_id vpc_id access_key_id],
@@ -102,6 +101,8 @@ RSpec.describe 'Database schema' do
         let(:indexes) { connection.indexes(table) }
         let(:columns) { connection.columns(table) }
         let(:foreign_keys) { connection.foreign_keys(table) }
+        let(:loose_foreign_keys) { Gitlab::Database::LooseForeignKeys.definitions.group_by(&:from_table).fetch(table, []) }
+        let(:all_foreign_keys) { foreign_keys + loose_foreign_keys }
         # take the first column in case we're using a composite primary key
         let(:primary_key_column) { Array(connection.primary_key(table)).first }
 
@@ -114,7 +115,7 @@ RSpec.describe 'Database schema' do
               columns = columns.split(',') if columns.is_a?(String)
               columns.first.chomp
             end
-            foreign_keys_columns = foreign_keys.map(&:column)
+            foreign_keys_columns = all_foreign_keys.map(&:column)
 
             # Add the primary key column to the list of indexed columns because
             # postgres and mysql both automatically create an index on the primary
@@ -129,7 +130,7 @@ RSpec.describe 'Database schema' do
         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(:foreign_keys_columns) { all_foreign_keys.map(&:column).uniq } # we can have FK and loose FK present at the same time
           let(:ignored_columns) { ignored_fk_columns(table) }
 
           it 'do have the foreign keys' do

spec/lib/gitlab/database/loose_foreign_keys_spec.rb

@@ -24,19 +24,19 @@ RSpec.describe Gitlab::Database::LooseForeignKeys do
         Gitlab::Database.schemas_to_base_models.fetch(parent_table_schema)
       end
 
-      it 'all `from_table` tables are present' do
+      it 'all `to_table` tables are present' do
         definitions.each do |definition|
-          base_models_for(definition.from_table).each do |model|
-            expect(model.connection).to be_table_exist(definition.from_table)
+          base_models_for(definition.to_table).each do |model|
+            expect(model.connection).to be_table_exist(definition.to_table)
           end
         end
       end
 
-      it 'all `to_table` tables are present' do
+      it 'all `from_table` tables are present' do
         definitions.each do |definition|
-          base_models_for(definition.to_table).each do |model|
-            expect(model.connection).to be_table_exist(definition.to_table)
-            expect(model.connection).to be_column_exist(definition.to_table, definition.column)
+          base_models_for(definition.from_table).each do |model|
+            expect(model.connection).to be_table_exist(definition.from_table)
+            expect(model.connection).to be_column_exist(definition.from_table, definition.column)
           end
         end
       end

spec/services/loose_foreign_keys/batch_cleaner_service_spec.rb

@@ -24,8 +24,8 @@ RSpec.describe LooseForeignKeys::BatchCleanerService do
   let(:loose_foreign_key_definitions) do
     [
       ActiveRecord::ConnectionAdapters::ForeignKeyDefinition.new(
-        '_test_loose_fk_parent_table',
         '_test_loose_fk_child_table_1',
+        '_test_loose_fk_parent_table',
         {
           column: 'parent_id',
           on_delete: :async_delete,
@@ -33,8 +33,8 @@ RSpec.describe LooseForeignKeys::BatchCleanerService do
         }
       ),
       ActiveRecord::ConnectionAdapters::ForeignKeyDefinition.new(
-        '_test_loose_fk_parent_table',
         '_test_loose_fk_child_table_2',
+        '_test_loose_fk_parent_table',
         {
           column: 'parent_id_with_different_column',
           on_delete: :async_nullify,

spec/services/loose_foreign_keys/cleaner_service_spec.rb

@@ -13,8 +13,8 @@ RSpec.describe LooseForeignKeys::CleanerService do
 
   let(:loose_fk_definition) do
     ActiveRecord::ConnectionAdapters::ForeignKeyDefinition.new(
-      'projects',
       'issues',
+      'projects',
       {
         column: 'project_id',
         on_delete: :async_nullify,
@@ -80,8 +80,8 @@ RSpec.describe LooseForeignKeys::CleanerService do
 
       let(:loose_fk_definition) do
         ActiveRecord::ConnectionAdapters::ForeignKeyDefinition.new(
-          'users',
           'project_authorizations',
+          'users',
           {
             column: 'user_id',
             on_delete: :async_delete,

spec/workers/loose_foreign_keys/cleanup_worker_spec.rb

@@ -31,8 +31,8 @@ RSpec.describe LooseForeignKeys::CleanupWorker do
     {
       '_test_loose_fk_parent_table_1' => [
         ActiveRecord::ConnectionAdapters::ForeignKeyDefinition.new(
-          '_test_loose_fk_parent_table_1',
           '_test_loose_fk_child_table_1_1',
+          '_test_loose_fk_parent_table_1',
           {
             column: 'parent_id',
             on_delete: :async_delete,
@@ -40,8 +40,8 @@ RSpec.describe LooseForeignKeys::CleanupWorker do
           }
         ),
         ActiveRecord::ConnectionAdapters::ForeignKeyDefinition.new(
-          '_test_loose_fk_parent_table_1',
           '_test_loose_fk_child_table_1_2',
+          '_test_loose_fk_parent_table_1',
           {
             column: 'parent_id_with_different_column',
             on_delete: :async_nullify,
@@ -51,8 +51,8 @@ RSpec.describe LooseForeignKeys::CleanupWorker do
       ],
       '_test_loose_fk_parent_table_2' => [
         ActiveRecord::ConnectionAdapters::ForeignKeyDefinition.new(
-          '_test_loose_fk_parent_table_2',
           '_test_loose_fk_child_table_2_1',
+          '_test_loose_fk_parent_table_2',
           {
             column: 'parent_id',
             on_delete: :async_delete,