Commit 8b0ceeaf authored by Kamil Trzciński's avatar Kamil Trzciński

Introduce `allow_cross_joins_across_databases` method

This adds a test executed as part of specs on each
query run on PSQL to be validated if it does
cross-joins across databases (main vs ci).

This raises exception unless explicitly allowed
with a link to the issue.
parent 923d0a26
......@@ -142,6 +142,11 @@ module Gitlab
MAX_TIMESTAMP_VALUE > timestamp ? timestamp : MAX_TIMESTAMP_VALUE.dup
end
def self.allow_cross_joins_across_databases(url:)
# this method is implemented in:
# spec/support/database/prevent_cross_joins.rb
end
def self.add_post_migrate_path_to_rails(force: false)
return if ENV['SKIP_POST_DEPLOYMENT_MIGRATIONS'] && !force
......
# frozen_string_literal: true
# This module tries to discover and prevent cross-joins across tables
# This will forbid usage of tables between CI and main database
# on a same query unless explicitly allowed by. This will change execution
# from a given point to allow cross-joins. The state will be cleared
# on a next test run.
#
# This method should be used to mark METHOD introducing cross-join
# not a test using the cross-join.
#
# class User
# def ci_owned_runners
# ::Gitlab::Database.allow_cross_joins_across_databases!(url: link-to-issue-url)
#
# ...
# end
# end
module Database
module PreventCrossJoins
CrossJoinAcrossUnsupportedTablesError = Class.new(StandardError)
def self.validate_cross_joins!(sql)
return if Thread.current[:allow_cross_joins_across_databases]
# PgQuery might fail in some cases due to limited nesting:
# https://github.com/pganalyze/pg_query/issues/209
tables = PgQuery.parse(sql).tables
unless only_ci_or_only_main?(tables)
raise CrossJoinAcrossUnsupportedTablesError,
"Unsupported cross-join across '#{tables.join(", ")}' discovered " \
"when executing query '#{sql}'"
end
end
# Returns true if a set includes only CI tables, or includes only non-CI tables
def self.only_ci_or_only_main?(tables)
tables.all? { |table| ci_table_name?(table) } ||
tables.none? { |table| ci_table_name?(table) }
end
def self.ci_table_name?(name)
ci_tables.include?(name)
end
def self.ci_tables
@@ci_tables ||= Set.new.tap do |tables| # rubocop:disable Style/ClassVars
tables.merge(Ci::ApplicationRecord.descendants.map(&:table_name).compact)
# It was decided that taggings/tags are best placed with CI
# https://gitlab.com/gitlab-org/gitlab/-/issues/333413
tables.add('taggings')
tables.add('tags')
end
end
module GitlabDatabaseMixin
def allow_cross_joins_across_databases(url:)
Thread.current[:allow_cross_joins_across_databases] = true
super
end
end
end
end
Gitlab::Database.singleton_class.prepend(
Database::PreventCrossJoins::GitlabDatabaseMixin)
RSpec.configure do |config|
# TODO: remove `:prevent_cross_joins` to enable the check by default
config.around(:each, :prevent_cross_joins) do |example|
subscriber = ActiveSupport::Notifications.subscribe('sql.active_record') do |event|
::Database::PreventCrossJoins.validate_cross_joins!(event.payload[:sql])
end
Thread.current[:allow_cross_joins_across_databases] = false
example.run
ensure
ActiveSupport::Notifications.unsubscribe(subscriber) if subscriber
end
end
# frozen_string_literal: true
require 'spec_helper'
RSpec.describe Database::PreventCrossJoins do
context 'when running in :prevent_cross_joins scope', :prevent_cross_joins do
context 'when only non-CI tables are used' do
it 'does not raise exception' do
expect { main_only_query }.not_to raise_error
end
end
context 'when only CI tables are used' do
it 'does not raise exception' do
expect { ci_only_query }.not_to raise_error
end
end
context 'when CI and non-CI tables are used' do
it 'raises exception' do
expect { main_and_ci_query }.to raise_error(
described_class::CrossJoinAcrossUnsupportedTablesError)
end
context 'when allow_cross_joins_across_databases is used' do
it 'does not raise exception' do
Gitlab::Database.allow_cross_joins_across_databases(url: 'http://issue-url')
expect { main_and_ci_query }.not_to raise_error
end
end
end
end
context 'when running in a default scope' do
context 'when CI and non-CI tables are used' do
it 'does not raise exception' do
expect { main_and_ci_query }.not_to raise_error
end
end
end
private
def main_only_query
Issue.joins(:project).last
end
def ci_only_query
Ci::Build.joins(:pipeline).last
end
def main_and_ci_query
Ci::Build.joins(:project).last
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