From be1660741b651973dc5f8e8ef348a0c1af6e0483 Mon Sep 17 00:00:00 2001
From: Joe Woodward <jwoodward@gitlab.com>
Date: Fri, 20 Dec 2024 17:44:26 +0000
Subject: [PATCH] Match namespaced factories in Migration/EnsureFactoryForTable
Cop
Before this change we only matched tables against their file name e.g.
```ruby
create_table :projects_branch_rules_squash_options do
```
Would require a factory stored in:
`spec/factories/projects_branch_rules_squash_options.rb`
However, we often use namespacing to retain some structure within the
codebase. This change allows us to specify a factory in:
`spec/factories/projects/branch_rules/squash_options.rb`
---
.../migration/ensure_factory_for_table.yml | 14 ------
...40515022318_create_duo_workflows_tables.rb | 2 +-
...eate_dependency_list_export_parts_table.rb | 2 -
...134250_create_p_ci_build_trace_metadata.rb | 2 -
..._access_instance_microsoft_applications.rb | 3 --
..._instance_microsoft_graph_access_tokens.rb | 3 --
...3210_create_instance_integrations_table.rb | 2 +-
...29205910_create_ci_job_artifact_reports.rb | 2 +-
..._create_packages_conan_recipe_revisions.rb | 2 +-
...reate_packages_conan_package_references.rb | 2 +-
...2920_create_ci_job_token_authorizations.rb | 2 +-
...tem_access_group_microsoft_applications.rb | 2 +-
...ess_group_microsoft_graph_access_tokens.rb | 2 +-
.../cop/migration/ensure_factory_for_table.rb | 11 +++--
.../ensure_factory_for_table_spec.rb | 47 +++++++++++++++----
15 files changed, 53 insertions(+), 45 deletions(-)
diff --git a/.rubocop_todo/migration/ensure_factory_for_table.yml b/.rubocop_todo/migration/ensure_factory_for_table.yml
index 762a2b1203585..10fb9c293dee3 100644
--- a/.rubocop_todo/migration/ensure_factory_for_table.yml
+++ b/.rubocop_todo/migration/ensure_factory_for_table.yml
@@ -5,26 +5,16 @@ Migration/EnsureFactoryForTable:
- 'db/migrate/20230926092944_add_approval_group_rules_groups.rb'
- 'db/migrate/20230926093004_add_approval_group_rules_users.rb'
- 'db/migrate/20230926093025_add_approval_group_rules_protected_branches.rb'
- - 'db/migrate/20231017095738_create_activity_pub_releases_subscriptions.rb'
- - 'db/migrate/20231017134349_create_ml_model_metadata.rb'
- 'db/migrate/20231020095624_create_audit_events_streaming_http_group_namespace_filters.rb'
- - 'db/migrate/20231025123238_create_compliance_framework_security_policies.rb'
- 'db/migrate/20231031164724_create_sbom_occurrences_vulnerabilities.rb'
- - 'db/migrate/20231102142553_add_zoekt_nodes.rb'
- - 'db/migrate/20231107140642_create_audit_events_instance_amazon_s3_configurations.rb'
- 'db/migrate/20231110044738_create_abuse_report_assignees_table.rb'
- 'db/migrate/20231115064007_create_audit_events_streaming_http_instance_namespace_filters.rb'
- 'db/migrate/20231124191759_add_catalog_resource_sync_events_table.rb'
- 'db/migrate/20231207150738_add_work_item_dates_sources.rb'
- - 'db/migrate/20231215192706_create_ml_model_version_metadata.rb'
- 'db/migrate/20231220113459_add_work_item_color.rb'
- - 'db/migrate/20231221033539_create_sbom_source_packages_table.rb'
- - 'db/migrate/20231221113948_create_cloud_connector_access.rb'
- 'db/migrate/20240108123023_create_ai_agents_table.rb'
- 'db/migrate/20240108123115_create_ai_agent_versions_table.rb'
- 'db/migrate/20240112124030_create_audit_events_group_external_streaming_destinations.rb'
- - 'db/migrate/20240115185037_create_packages_terraform_module_metadata.rb'
- - 'db/migrate/20240122165824_create_ci_job_token_group_scope_links.rb'
- 'db/migrate/20240130162148_create_audit_events_instance_external_streaming_destinations.rb'
- 'db/migrate/20240131052824_create_catalog_verified_namespaces.rb'
- 'db/migrate/20240201112236_create_group_saved_replies_table.rb'
@@ -34,7 +24,3 @@ Migration/EnsureFactoryForTable:
- 'db/migrate/20240304184128_create_ci_build_names_table.rb'
- 'db/migrate/20240306121653_create_relation_import_tracker.rb'
- 'db/migrate/20240404192955_create_early_access_program_tracking_events.rb'
- - 'db/migrate/20240419082037_create_ai_self_hosted_models.rb'
- - 'db/migrate/20240423064716_create_ci_build_execution_config.rb'
- - 'db/migrate/20240430110033_create_ai_feature_settings.rb'
- - 'db/migrate/20241127092714_create_container_registry_protection_tag_rules.rb'
diff --git a/db/migrate/20240515022318_create_duo_workflows_tables.rb b/db/migrate/20240515022318_create_duo_workflows_tables.rb
index ac1ce36bb1763..ba00d8e939880 100644
--- a/db/migrate/20240515022318_create_duo_workflows_tables.rb
+++ b/db/migrate/20240515022318_create_duo_workflows_tables.rb
@@ -4,7 +4,7 @@ class CreateDuoWorkflowsTables < Gitlab::Database::Migration[2.2]
milestone '17.2'
def change
- create_table :duo_workflows_workflows do |t| # rubocop:disable Migration/EnsureFactoryForTable -- https://gitlab.com/gitlab-org/gitlab/-/issues/468630
+ create_table :duo_workflows_workflows do |t| # rubocop:disable Migration/EnsureFactoryForTable, Lint/RedundantCopDisableDirective -- https://gitlab.com/gitlab-org/gitlab/-/issues/468630
t.bigint :user_id, null: false, index: true
t.bigint :project_id, null: false, index: true
diff --git a/db/migrate/20240626125526_create_dependency_list_export_parts_table.rb b/db/migrate/20240626125526_create_dependency_list_export_parts_table.rb
index 2f7f2ce54bc74..20851dfe73c7a 100644
--- a/db/migrate/20240626125526_create_dependency_list_export_parts_table.rb
+++ b/db/migrate/20240626125526_create_dependency_list_export_parts_table.rb
@@ -3,7 +3,6 @@
class CreateDependencyListExportPartsTable < Gitlab::Database::Migration[2.2]
milestone '17.2'
- # rubocop:disable Migration/EnsureFactoryForTable -- False Positive
def change
create_table :dependency_list_export_parts do |t|
t.references :dependency_list_export, foreign_key: { on_delete: :cascade }, null: false, index: true
@@ -15,5 +14,4 @@ def change
t.text :file, limit: 255
end
end
- # rubocop:enable Migration/EnsureFactoryForTable
end
diff --git a/db/migrate/20240809134250_create_p_ci_build_trace_metadata.rb b/db/migrate/20240809134250_create_p_ci_build_trace_metadata.rb
index 8224679399378..36c8bde3310bd 100644
--- a/db/migrate/20240809134250_create_p_ci_build_trace_metadata.rb
+++ b/db/migrate/20240809134250_create_p_ci_build_trace_metadata.rb
@@ -3,7 +3,6 @@
class CreatePCiBuildTraceMetadata < Gitlab::Database::Migration[2.2]
milestone '17.4'
- # rubocop:disable Migration/EnsureFactoryForTable -- No factory needed
def change
create_table(:p_ci_build_trace_metadata, primary_key: [:build_id, :partition_id],
options: 'PARTITION BY LIST (partition_id)', if_not_exists: true) do |t|
@@ -19,5 +18,4 @@ def change
t.index :trace_artifact_id
end
end
- # rubocop:enable Migration/EnsureFactoryForTable -- No factory needed
end
diff --git a/db/migrate/20240829143538_create_system_access_instance_microsoft_applications.rb b/db/migrate/20240829143538_create_system_access_instance_microsoft_applications.rb
index 6c24f7fa617ad..0e5e84706f606 100644
--- a/db/migrate/20240829143538_create_system_access_instance_microsoft_applications.rb
+++ b/db/migrate/20240829143538_create_system_access_instance_microsoft_applications.rb
@@ -1,6 +1,5 @@
# frozen_string_literal: true
-# rubocop:disable Migration/EnsureFactoryForTable -- False positive
class CreateSystemAccessInstanceMicrosoftApplications < Gitlab::Database::Migration[2.2]
milestone '17.5'
@@ -17,5 +16,3 @@ def change
end
end
end
-
-# rubocop:enable Migration/EnsureFactoryForTable
diff --git a/db/migrate/20240829143654_create_system_access_instance_microsoft_graph_access_tokens.rb b/db/migrate/20240829143654_create_system_access_instance_microsoft_graph_access_tokens.rb
index 2a42f2472f077..df804e8dc0b3b 100644
--- a/db/migrate/20240829143654_create_system_access_instance_microsoft_graph_access_tokens.rb
+++ b/db/migrate/20240829143654_create_system_access_instance_microsoft_graph_access_tokens.rb
@@ -1,6 +1,5 @@
# frozen_string_literal: true
-# rubocop:disable Migration/EnsureFactoryForTable -- False positive
class CreateSystemAccessInstanceMicrosoftGraphAccessTokens < Gitlab::Database::Migration[2.2]
milestone '17.5'
@@ -16,5 +15,3 @@ def change
end
end
end
-
-# rubocop:enable Migration/EnsureFactoryForTable
diff --git a/db/migrate/20240829163210_create_instance_integrations_table.rb b/db/migrate/20240829163210_create_instance_integrations_table.rb
index 560b18185da1e..15cfbc57927ad 100644
--- a/db/migrate/20240829163210_create_instance_integrations_table.rb
+++ b/db/migrate/20240829163210_create_instance_integrations_table.rb
@@ -4,7 +4,7 @@ class CreateInstanceIntegrationsTable < Gitlab::Database::Migration[2.2]
milestone '17.4'
def up
- create_table :instance_integrations, id: :bigserial do |t| # rubocop:disable Migration/EnsureFactoryForTable -- https://gitlab.com/gitlab-org/gitlab/-/issues/468630
+ create_table :instance_integrations, id: :bigserial do |t|
t.timestamps_with_timezone null: false
t.integer :comment_detail
t.boolean :active, default: false, null: false
diff --git a/db/migrate/20240829205910_create_ci_job_artifact_reports.rb b/db/migrate/20240829205910_create_ci_job_artifact_reports.rb
index 1430653f38cb3..394907cd18bc8 100644
--- a/db/migrate/20240829205910_create_ci_job_artifact_reports.rb
+++ b/db/migrate/20240829205910_create_ci_job_artifact_reports.rb
@@ -5,7 +5,7 @@ class CreateCiJobArtifactReports < Gitlab::Database::Migration[2.2]
disable_ddl_transaction!
def up
- create_table(:p_ci_job_artifact_reports, # rubocop:disable Migration/EnsureFactoryForTable -- Factory exists
+ create_table(:p_ci_job_artifact_reports,
primary_key: [:job_artifact_id, :partition_id],
options: 'PARTITION BY LIST (partition_id)', if_not_exists: true) do |t|
t.bigint :job_artifact_id, null: false
diff --git a/db/migrate/20240902100524_create_packages_conan_recipe_revisions.rb b/db/migrate/20240902100524_create_packages_conan_recipe_revisions.rb
index f75a28679407c..d6758775a87e8 100644
--- a/db/migrate/20240902100524_create_packages_conan_recipe_revisions.rb
+++ b/db/migrate/20240902100524_create_packages_conan_recipe_revisions.rb
@@ -6,7 +6,7 @@ class CreatePackagesConanRecipeRevisions < Gitlab::Database::Migration[2.2]
INDEX_PACKAGE_ID_REVISION = 'idx_on_packages_conan_recipe_revisions_package_id_revision'
def up
- create_table :packages_conan_recipe_revisions do |t| # rubocop:disable Migration/EnsureFactoryForTable -- https://gitlab.com/gitlab-org/gitlab/-/issues/468630
+ create_table :packages_conan_recipe_revisions do |t|
t.bigint :package_id, null: false
t.bigint :project_id, null: false
t.timestamps_with_timezone null: false
diff --git a/db/migrate/20240906134338_create_packages_conan_package_references.rb b/db/migrate/20240906134338_create_packages_conan_package_references.rb
index ea90c4c5c0c60..6a29b7f7c3e61 100644
--- a/db/migrate/20240906134338_create_packages_conan_package_references.rb
+++ b/db/migrate/20240906134338_create_packages_conan_package_references.rb
@@ -7,7 +7,7 @@ class CreatePackagesConanPackageReferences < Gitlab::Database::Migration[2.2]
CONSTRAINT_NAME = 'chk_conan_references_info_length'
def up
- create_table :packages_conan_package_references do |t| # rubocop:disable Migration/EnsureFactoryForTable -- https://gitlab.com/gitlab-org/gitlab/-/issues/468630
+ create_table :packages_conan_package_references do |t|
t.bigint :package_id, null: false
t.bigint :project_id, null: false
t.bigint :recipe_revision_id
diff --git a/db/migrate/20240910132920_create_ci_job_token_authorizations.rb b/db/migrate/20240910132920_create_ci_job_token_authorizations.rb
index e4709ebc2223e..4e83f4340957d 100644
--- a/db/migrate/20240910132920_create_ci_job_token_authorizations.rb
+++ b/db/migrate/20240910132920_create_ci_job_token_authorizations.rb
@@ -6,7 +6,7 @@ class CreateCiJobTokenAuthorizations < Gitlab::Database::Migration[2.2]
INDEX_NAME = 'idx_ci_job_token_authorizations_on_accessed_and_origin_project'
def change
- create_table(:ci_job_token_authorizations, if_not_exists: true) do |t| # rubocop:disable Migration/EnsureFactoryForTable -- factory at ci/job_token/authorizations.rb
+ create_table(:ci_job_token_authorizations, if_not_exists: true) do |t|
t.bigint :accessed_project_id, null: false
t.bigint :origin_project_id, null: false, index: true
t.datetime_with_timezone :last_authorized_at, null: false
diff --git a/db/migrate/20241106114853_create_system_access_group_microsoft_applications.rb b/db/migrate/20241106114853_create_system_access_group_microsoft_applications.rb
index d8f0fea8f43ed..76eaa21e96f24 100644
--- a/db/migrate/20241106114853_create_system_access_group_microsoft_applications.rb
+++ b/db/migrate/20241106114853_create_system_access_group_microsoft_applications.rb
@@ -5,7 +5,7 @@ class CreateSystemAccessGroupMicrosoftApplications < Gitlab::Database::Migration
milestone '17.7'
def change
- create_table :system_access_group_microsoft_applications do |t| # rubocop:disable Migration/EnsureFactoryForTable -- False positive, factory name is prefixed with system_access
+ create_table :system_access_group_microsoft_applications do |t|
t.timestamps_with_timezone null: false
t.references :group, foreign_key: { to_table: :namespaces, on_delete: :cascade }, null: false
t.bigint :temp_source_id, index: { unique: true, name: 'index_group_microsoft_applications_on_temp_source_id' },
diff --git a/db/migrate/20241106115015_create_system_access_group_microsoft_graph_access_tokens.rb b/db/migrate/20241106115015_create_system_access_group_microsoft_graph_access_tokens.rb
index ab715dd93c496..a0898f69740d7 100644
--- a/db/migrate/20241106115015_create_system_access_group_microsoft_graph_access_tokens.rb
+++ b/db/migrate/20241106115015_create_system_access_group_microsoft_graph_access_tokens.rb
@@ -5,7 +5,7 @@ class CreateSystemAccessGroupMicrosoftGraphAccessTokens < Gitlab::Database::Migr
milestone '17.7'
def change
- create_table :system_access_group_microsoft_graph_access_tokens do |t| # rubocop:disable Migration/EnsureFactoryForTable -- False positive, factory name is prefixed with system_access
+ create_table :system_access_group_microsoft_graph_access_tokens do |t|
t.timestamps_with_timezone null: false
t.references :system_access_group_microsoft_application,
index: { name: 'unique_index_group_ms_access_tokens_on_ms_app_id', unique: true }
diff --git a/rubocop/cop/migration/ensure_factory_for_table.rb b/rubocop/cop/migration/ensure_factory_for_table.rb
index e453605d0606f..1636671bf9718 100644
--- a/rubocop/cop/migration/ensure_factory_for_table.rb
+++ b/rubocop/cop/migration/ensure_factory_for_table.rb
@@ -47,7 +47,8 @@ def on_send(node)
return if !ee? && disabled_comment_absent?
table_definition(node) do |table_name_node, table_name|
- unless factory?(table_name.to_s)
+ # Partioned tables are prefix with `p_`.
+ unless factory?(table_name.to_s.delete_prefix('p_'))
msg = format(MSG, name: table_name)
add_offense(table_name_node, message: msg)
end
@@ -57,13 +58,13 @@ def on_send(node)
private
def factory?(table_name)
- end_with = "/#{table_name}.rb"
-
- self.class.factories.any? { |path| path.end_with?(end_with) }
+ self.class.factories.any? { |name| name.end_with?(table_name) }
end
def self.factories
- @factories ||= Dir.glob("{,ee/,jh/}spec/factories/**/*.rb")
+ @factories ||= Dir.glob("{,ee/,jh/}spec/factories/**/*.rb").map do |factory|
+ factory.gsub(%r{^(ee/|jh/|)spec/factories/}, '').delete_suffix('.rb').tr('/', '_')
+ end
end
def disabled_comment_absent?
diff --git a/spec/rubocop/cop/migration/ensure_factory_for_table_spec.rb b/spec/rubocop/cop/migration/ensure_factory_for_table_spec.rb
index e10864dc1d81b..208b123b7f713 100644
--- a/spec/rubocop/cop/migration/ensure_factory_for_table_spec.rb
+++ b/spec/rubocop/cop/migration/ensure_factory_for_table_spec.rb
@@ -83,22 +83,53 @@
end
context 'with matching factories' do
- let(:factories) { ['spec/factories/users.rb'] }
+ context 'with regular table' do
+ let(:factories) { ['users'] }
- it 'does not register an offense when a table has a corresponding factory' do
- expect_no_offenses(<<~RUBY)
- create_table :users do |t|
- t.string :name
- t.timestamps
+ it 'does not register an offense when a table has a corresponding factory' do
+ expect_no_offenses(<<~RUBY)
+ create_table :users do |t|
+ t.string :name
+ t.timestamps
+ end
+ RUBY
+ end
+ end
+
+ context 'with partitioned table' do
+ let(:factories) { ['users'] }
+
+ it 'does not register an offense when a table has a corresponding factory' do
+ expect_no_offenses(<<~RUBY)
+ create_table :p_users do |t|
+ t.string :name
+ t.timestamps
+ end
+ RUBY
end
- RUBY
end
end
end
describe '.factories' do
+ let(:table_names) { %w[unnested_ce nested_ce_factory unnested_ee nested_ee_factory unnested_jh nested_jh_factory] }
+ let(:factories) do
+ %w[
+ spec/factories/unnested_ce.rb
+ spec/factories/nested/ce/factory.rb
+ ee/spec/factories/unnested_ee.rb
+ ee/spec/factories/nested/ee_factory.rb
+ jh/spec/factories/unnested_jh.rb
+ jh/spec/factories/nested_jh/factory.rb
+ ]
+ end
+
+ before do
+ allow(Dir).to receive(:glob).and_return(factories)
+ end
+
subject { described_class.factories }
- it { is_expected.not_to be_empty }
+ it { is_expected.to eq(table_names) }
end
end
--
GitLab