Commit a5748ec5 authored by Dylan Griffith's avatar Dylan Griffith

Use a YAML file to define gitlab_schema for tables

We were previously using the gitlab_schema annotation on models which
was causing too much issues with missed models and generally it was
trickier to get right due to autoloading issues among other fancy Ruby
metaprogramming trickery.

We also needed to add
`spec/migrations/disable_job_token_scope_when_unused_spec.rb` to the
allowlist as this had a previously undetected cross-join. The previous
code which used the model to determine `gitlab_schema` was incorrectly
detecting `gitlab_main` as the migration test declares the model and
overwrites the `gitlab_ci` value already defined.
parent 0e8ab9b6
# frozen_string_literal: true # frozen_string_literal: true
class ApplicationRecord < ActiveRecord::Base class ApplicationRecord < ActiveRecord::Base
self.gitlab_schema = :gitlab_main
self.abstract_class = true self.abstract_class = true
alias_method :reset, :reload alias_method :reset, :reload
......
...@@ -2,7 +2,6 @@ ...@@ -2,7 +2,6 @@
module Ci module Ci
class ApplicationRecord < ::ApplicationRecord class ApplicationRecord < ::ApplicationRecord
self.gitlab_schema = :gitlab_ci
self.abstract_class = true self.abstract_class = true
def self.table_name_prefix def self.table_name_prefix
......
...@@ -69,7 +69,7 @@ module LooseForeignKey ...@@ -69,7 +69,7 @@ module LooseForeignKey
end end
on_delete_options = %i[async_delete async_nullify] on_delete_options = %i[async_delete async_nullify]
gitlab_schema_options = [ApplicationRecord.gitlab_schema, Ci::ApplicationRecord.gitlab_schema] gitlab_schema_options = %i(gitlab_main gitlab_ci)
unless on_delete_options.include?(symbolized_options[:on_delete]&.to_sym) unless on_delete_options.include?(symbolized_options[:on_delete]&.to_sym)
raise "Invalid on_delete option given: #{symbolized_options[:on_delete]}. Valid options: #{on_delete_options.join(', ')}" raise "Invalid on_delete option given: #{symbolized_options[:on_delete]}. Valid options: #{on_delete_options.join(', ')}"
......
# frozen_string_literal: true
# This parameter describes a virtual context to indicate
# table affinity to other tables.
#
# Table affinity limits cross-joins, cross-modifications,
# foreign keys and validates relationship between tables
#
# By default it is undefined
ActiveRecord::Base.class_attribute :gitlab_schema, default: nil
...@@ -13,7 +13,3 @@ raise "Counter cache is not disabled" if ...@@ -13,7 +13,3 @@ raise "Counter cache is not disabled" if
ActsAsTaggableOn::Tagging.include IgnorableColumns ActsAsTaggableOn::Tagging.include IgnorableColumns
ActsAsTaggableOn::Tagging.ignore_column :id_convert_to_bigint, remove_with: '14.5', remove_after: '2021-10-22' ActsAsTaggableOn::Tagging.ignore_column :id_convert_to_bigint, remove_with: '14.5', remove_after: '2021-10-22'
ActsAsTaggableOn::Tagging.ignore_column :taggable_id_convert_to_bigint, remove_with: '14.5', remove_after: '2021-10-22' ActsAsTaggableOn::Tagging.ignore_column :taggable_id_convert_to_bigint, remove_with: '14.5', remove_after: '2021-10-22'
# The tags and taggings are supposed to be part of `gitlab_ci`
ActsAsTaggableOn::Tag.gitlab_schema = :gitlab_ci
ActsAsTaggableOn::Tagging.gitlab_schema = :gitlab_ci
...@@ -20,7 +20,6 @@ class AssociateExistingDastBuildsWithVariables < ActiveRecord::Migration[6.1] ...@@ -20,7 +20,6 @@ class AssociateExistingDastBuildsWithVariables < ActiveRecord::Migration[6.1]
class Build < ApplicationRecord class Build < ApplicationRecord
self.table_name = 'ci_builds' self.table_name = 'ci_builds'
self.inheritance_column = :_type_disabled self.inheritance_column = :_type_disabled
self.gitlab_schema = :gitlab_ci
default_scope { where(name: :dast, stage: :dast) } # rubocop:disable Cop/DefaultScope default_scope { where(name: :dast, stage: :dast) } # rubocop:disable Cop/DefaultScope
end end
......
...@@ -29,6 +29,7 @@ ...@@ -29,6 +29,7 @@
- "./spec/lib/gitlab/background_migration/migrate_pages_metadata_spec.rb" - "./spec/lib/gitlab/background_migration/migrate_pages_metadata_spec.rb"
- "./spec/migrations/20210907211557_finalize_ci_builds_bigint_conversion_spec.rb" - "./spec/migrations/20210907211557_finalize_ci_builds_bigint_conversion_spec.rb"
- "./spec/migrations/associate_existing_dast_builds_with_variables_spec.rb" - "./spec/migrations/associate_existing_dast_builds_with_variables_spec.rb"
- "./spec/migrations/disable_job_token_scope_when_unused_spec.rb"
- "./spec/migrations/schedule_copy_ci_builds_columns_to_security_scans2_spec.rb" - "./spec/migrations/schedule_copy_ci_builds_columns_to_security_scans2_spec.rb"
- "./spec/migrations/schedule_pages_metadata_migration_spec.rb" - "./spec/migrations/schedule_pages_metadata_migration_spec.rb"
- "./spec/models/ci/pipeline_spec.rb" - "./spec/models/ci/pipeline_spec.rb"
......
...@@ -9,17 +9,12 @@ module Database ...@@ -9,17 +9,12 @@ module Database
end end
def self.table_schema(name) def self.table_schema(name)
tables_to_schema[name] || :undefined # When undefined it's best to return a unique name so that we don't incorrectly assume that 2 undefined schemas belong on the same database
tables_to_schema[name] || :"undefined_#{name}"
end end
def self.tables_to_schema def self.tables_to_schema
@tables_to_schema ||= all_classes_with_schema.to_h do |klass| @tables_to_schema ||= YAML.load_file(Rails.root.join('spec/support/database/gitlab_schemas.yml'))
[klass.table_name, klass.gitlab_schema]
end
end
def self.all_classes_with_schema
ActiveRecord::Base.descendants.reject(&:abstract_class?).select(&:gitlab_schema?) # rubocop:disable Database/MultipleDatabases
end end
end end
end end
This diff is collapsed.
...@@ -86,6 +86,11 @@ module Database ...@@ -86,6 +86,11 @@ module Database
return if tables.empty? return if tables.empty?
# All migrations will write to schema_migrations in the same transaction.
# It's safe to ignore this since schema_migrations exists in all
# databases
return if tables == ['schema_migrations']
cross_database_context[:modified_tables_by_db][database].merge(tables) cross_database_context[:modified_tables_by_db][database].merge(tables)
all_tables = cross_database_context[:modified_tables_by_db].values.map(&:to_a).flatten all_tables = cross_database_context[:modified_tables_by_db].values.map(&:to_a).flatten
......
# frozen_string_literal: true
require 'spec_helper'
RSpec.describe Database::GitlabSchema do
it 'matches all the tables in the database', :aggregate_failures do
# These tables do not need a gitlab_schema
excluded_tables = %w(ar_internal_metadata schema_migrations)
all_tables_in_database = ApplicationRecord.connection.tables
all_tables_with_gitlab_schema = described_class.tables_to_schema.keys
missing = []
all_tables_in_database.each do |table_in_database|
next if table_in_database.in?(excluded_tables)
missing << table_in_database unless all_tables_with_gitlab_schema.include?(table_in_database)
end
extras = []
all_tables_with_gitlab_schema.each do |table_with_gitlab_schema|
extras << table_with_gitlab_schema unless all_tables_in_database.include?(table_with_gitlab_schema)
end
expect(missing).to be_empty, "Missing table(s) #{missing} not found in #{described_class}.tables_to_schema. Any new tables must be added to spec/support/database/gitlab_schemas.yml ."
expect(extras).to be_empty, "Extra table(s) #{extras} found in #{described_class}.tables_to_schema. Any removed or renamed tables must be removed from spec/support/database/gitlab_schemas.yml ."
end
end
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment