Skip to content

Commit 7a4fa5a

Browse files
BuonOmoBuonOmo
committed
feat(referential-integrity): handle autocommit_before_ddl
If `#disable_referential_integrity` is ran within a transaction, we may be able to still run the user's code. Otherwise we warn the user why it failed.
1 parent c6aa880 commit 7a4fa5a

File tree

4 files changed

+163
-24
lines changed

4 files changed

+163
-24
lines changed

‎lib/active_record/connection_adapters/cockroachdb/database_statements.rb‎

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -34,7 +34,6 @@ def insert_fixtures_set(fixture_set, tables_to_delete = [])
3434
# our statements by calling `#execute` instead of `#execute_batch`.
3535
#
3636
# [1]: https://github.com/rails/rails/pull/52428
37-
3837
begin # much faster without disabling referential integrity, worth trying.
3938
transaction(requires_new: true) do
4039
execute(statements, "Fixtures Load")

‎lib/active_record/connection_adapters/cockroachdb/referential_integrity.rb‎

Lines changed: 35 additions & 19 deletions
Original file line numberDiff line numberDiff line change
@@ -34,25 +34,41 @@ def check_all_foreign_keys_valid!
3434
end
3535

3636
def disable_referential_integrity
37-
foreign_keys = all_foreign_keys
38-
39-
remove_foreign_keys(foreign_keys)
40-
41-
# Prefixes and suffixes are added in add_foreign_key
42-
# in AR7+ so we need to temporarily disable them here,
43-
# otherwise prefixes/suffixes will be erroneously added.
44-
old_prefix = ActiveRecord::Base.table_name_prefix
45-
old_suffix = ActiveRecord::Base.table_name_suffix
46-
47-
begin
48-
yield
49-
ensure
50-
ActiveRecord::Base.table_name_prefix = ""
51-
ActiveRecord::Base.table_name_suffix = ""
52-
add_foreign_keys(foreign_keys) # Never raises.
53-
54-
ActiveRecord::Base.table_name_prefix = old_prefix if defined?(old_prefix)
55-
ActiveRecord::Base.table_name_suffix = old_suffix if defined?(old_suffix)
37+
if transaction_open? && query_value("SHOW autocommit_before_ddl") == "off"
38+
begin
39+
yield
40+
rescue ActiveRecord::InvalidForeignKey => e
41+
warn <<-WARNING
42+
WARNING: Rails was not able to disable referential integrity.
43+
44+
This is due to CockroachDB's need of committing transactions
45+
before a schema change occurs. To bypass this, you can set
46+
`autocommit_before_ddl: "on"` in your database configuration.
47+
WARNING
48+
raise e
49+
end
50+
else
51+
foreign_keys = all_foreign_keys
52+
53+
remove_foreign_keys(foreign_keys)
54+
55+
# Prefixes and suffixes are added in add_foreign_key
56+
# in AR7+ so we need to temporarily disable them here,
57+
# otherwise prefixes/suffixes will be erroneously added.
58+
old_prefix = ActiveRecord::Base.table_name_prefix
59+
old_suffix = ActiveRecord::Base.table_name_suffix
60+
61+
begin
62+
yield
63+
ensure
64+
ActiveRecord::Base.table_name_prefix = ""
65+
ActiveRecord::Base.table_name_suffix = ""
66+
67+
add_foreign_keys(foreign_keys) # Never raises.
68+
69+
ActiveRecord::Base.table_name_prefix = old_prefix if defined?(old_prefix)
70+
ActiveRecord::Base.table_name_suffix = old_suffix if defined?(old_suffix)
71+
end
5672
end
5773
end
5874

‎test/cases/adapters/cockroachdb/referential_integrity_test.rb‎

Lines changed: 114 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,114 @@
1+
# frozen_string_literal: true
2+
3+
require "cases/helper_cockroachdb"
4+
require "support/connection_helper" # for #reset_connection
5+
require "support/copy_cat"
6+
7+
class CockroachDBReferentialIntegrityTest < ActiveRecord::PostgreSQLTestCase
8+
include ConnectionHelper
9+
10+
module ProgrammerMistake
11+
def execute_batch(sql, name = nil)
12+
raise ArgumentError, "something is not right." if name.match?(/referential integrity/)
13+
super
14+
end
15+
end
16+
17+
def setup
18+
@connection = ActiveRecord::Base.lease_connection
19+
end
20+
21+
def teardown
22+
reset_connection
23+
end
24+
25+
exclude_from_transactional_tests :test_only_catch_active_record_errors_others_bubble_up
26+
CopyCat.copy_methods(self, ::PostgreSQLReferentialIntegrityTest, :test_only_catch_active_record_errors_others_bubble_up)
27+
28+
def test_should_reraise_invalid_foreign_key_exception_and_show_warning
29+
warning = capture(:stderr) do
30+
e = assert_raises(ActiveRecord::InvalidForeignKey) do
31+
@connection.disable_referential_integrity do
32+
@connection.execute("INSERT INTO authors (name, author_address_id) VALUES ('Mona Chollet', 42)")
33+
end
34+
end
35+
assert_match (/Key \(author_address_id\)=\(42\) is not present in table/), e.message
36+
end
37+
assert_match (/WARNING: Rails was not able to disable referential integrity/), warning
38+
assert_match (/autocommit_before_ddl/), warning
39+
end
40+
41+
def test_no_warning_nor_error_with_autocommit_before_ddl
42+
@connection.execute("SET SESSION autocommit_before_ddl = 'on'")
43+
warning = capture(:stderr) do
44+
@connection.disable_referential_integrity do
45+
@connection.execute("INSERT INTO authors (name, author_address_id) VALUES ('Mona Chollet', 42)")
46+
@connection.truncate(:authors)
47+
end
48+
end
49+
assert_predicate warning, :blank?, "expected no warnings but got:\n#{warning}"
50+
end
51+
52+
# def test_does_not_break_transactions
53+
# @connection.extend MissingSuperuserPrivileges
54+
55+
# @connection.transaction do
56+
# @connection.disable_referential_integrity do
57+
# assert_transaction_is_not_broken
58+
# end
59+
# assert_transaction_is_not_broken
60+
# end
61+
# end
62+
63+
# def test_does_not_break_nested_transactions
64+
# @connection.extend MissingSuperuserPrivileges
65+
66+
# @connection.transaction do
67+
# @connection.transaction(requires_new: true) do
68+
# @connection.disable_referential_integrity do
69+
# assert_transaction_is_not_broken
70+
# end
71+
# end
72+
# assert_transaction_is_not_broken
73+
# end
74+
# end
75+
76+
# def test_only_catch_active_record_errors_others_bubble_up
77+
# @connection.extend ProgrammerMistake
78+
79+
# assert_raises ArgumentError do
80+
# @connection.disable_referential_integrity { }
81+
# end
82+
# end
83+
84+
# def test_all_foreign_keys_valid_having_foreign_keys_in_multiple_schemas
85+
# @connection.execute <<~SQL
86+
# CREATE SCHEMA referential_integrity_test_schema;
87+
88+
# CREATE TABLE referential_integrity_test_schema.nodes (
89+
# id BIGSERIAL,
90+
# parent_id INT NOT NULL,
91+
# PRIMARY KEY(id),
92+
# CONSTRAINT fk_parent_node FOREIGN KEY(parent_id)
93+
# REFERENCES referential_integrity_test_schema.nodes(id)
94+
# );
95+
# SQL
96+
97+
# result = @connection.execute <<~SQL
98+
# SELECT count(*) AS count
99+
# FROM information_schema.table_constraints
100+
# WHERE constraint_schema = 'referential_integrity_test_schema'
101+
# AND constraint_type = 'FOREIGN KEY';
102+
# SQL
103+
104+
# assert_equal 1, result.first["count"], "referential_integrity_test_schema should have 1 foreign key"
105+
# @connection.check_all_foreign_keys_valid!
106+
# ensure
107+
# @connection.drop_schema "referential_integrity_test_schema", if_exists: true
108+
# end
109+
110+
# private
111+
# def assert_transaction_is_not_broken
112+
# assert_equal 1, @connection.select_value("SELECT 1")
113+
# end
114+
end

‎test/excludes/PostgreSQLReferentialIntegrityTest.rb‎

Lines changed: 14 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,14 @@
1-
exclude :test_only_catch_active_record_errors_others_bubble_up, "Skipping until we can triage further. See https://github.com/cockroachdb/activerecord-cockroachdb-adapter/issues/48"
2-
exclude :test_should_reraise_invalid_foreign_key_exception_and_show_warning, "Skipping until we can triage further. See https://github.com/cockroachdb/activerecord-cockroachdb-adapter/issues/48"
3-
exclude :test_does_not_break_transactions, "Skipping until we can triage further. See https://github.com/cockroachdb/activerecord-cockroachdb-adapter/issues/48"
4-
exclude :test_does_not_break_nested_transactions, "Skipping until we can triage further. See https://github.com/cockroachdb/activerecord-cockroachdb-adapter/issues/48"
1+
exclude :test_should_reraise_invalid_foreign_key_exception_and_show_warning,
2+
"CockroachDB has a different limitation as there is no" \
3+
"'DISABLE TRIGGER' statement."
4+
5+
break_tx = "CockroachDB will always alter transactions when " \
6+
"trying to disable referential integrity. Either it cannot " \
7+
"work within transaction, or autocommit_before_ddl is set " \
8+
"and transactions will be committed."
9+
exclude :test_does_not_break_transactions, break_tx
10+
exclude :test_does_not_break_nested_transactions, break_tx
11+
12+
exclude :test_only_catch_active_record_errors_others_bubble_up,
13+
"Reimplemented in test/cases/adapters/cockroachdb/referential_integrity_test.rb" \
14+
" to use a different trigger for the error."

0 commit comments

Comments
 (0)