Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Use SET CONSTRAINTS for disable_referential_integrity without superuser privileges #21233

Merged
merged 1 commit into from
Jan 4, 2017
Merged

Use SET CONSTRAINTS for disable_referential_integrity without superuser privileges #21233

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 9 additions & 0 deletions .travis.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,9 @@ services:

addons:
postgresql: "9.4"
apt:
packages:
- postgresql-9.4

bundler_args: --without test --jobs 3 --retry 3
before_install:
Expand Down Expand Up @@ -82,6 +85,12 @@ matrix:
jdk: oraclejdk8
env:
- "GEM=am,aj"
# Test with old (< 9.4.2) postgresql
- rvm: 2.3.3
env:
- "GEM=ar:postgresql"
addons:
postgresql: "9.4"
allow_failures:
- rvm: ruby-head
- rvm: jruby-9.1.5.0
Expand Down
47 changes: 43 additions & 4 deletions activerecord/lib/active_record/connection_adapters/postgresql/referential_integrity.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,8 +6,50 @@ def supports_disable_referential_integrity? # :nodoc:
true
end

def disable_referential_integrity # :nodoc:
def disable_referential_integrity(&block) # :nodoc:
if supports_disable_referential_integrity?
if supports_alter_constraint?
disable_referential_integrity_with_alter_constraint(&block)
else
disable_referential_integrity_with_disable_trigger(&block)
end
else
yield
end
end

private

def disable_referential_integrity_with_alter_constraint
tables_constraints = execute(<<-SQL).values
SELECT table_name, constraint_name
FROM information_schema.table_constraints
WHERE constraint_type = 'FOREIGN KEY'
AND is_deferrable = 'NO'
SQL

execute(
tables_constraints.collect { |table, constraint|
"ALTER TABLE #{quote_table_name(table)} ALTER CONSTRAINT #{constraint} DEFERRABLE"
}.join(";")
)

begin
transaction do
execute("SET CONSTRAINTS ALL DEFERRED")

yield
end
ensure
execute(
tables_constraints.collect { |table, constraint|
"ALTER TABLE #{quote_table_name(table)} ALTER CONSTRAINT #{constraint} NOT DEFERRABLE"
}.join(";")
)
end
end

def disable_referential_integrity_with_disable_trigger
original_exception = nil

begin
Expand Down Expand Up @@ -39,10 +81,7 @@ def disable_referential_integrity # :nodoc:
end
rescue ActiveRecord::ActiveRecordError
end
else
yield
end
end
end
end
end
Expand Down
6 changes: 6 additions & 0 deletions activerecord/lib/active_record/connection_adapters/postgresql_adapter.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -319,6 +319,12 @@ def supports_pgcrypto_uuid?
postgresql_version >= 90400
end

# PostgreSQL 9.4 introduces ALTER TABLE ... ALTER CONSTRAINT but it has a bug and fixed in 9.4.2
# https://www.postgresql.org/docs/9.4/static/release-9-4-2.html
def supports_alter_constraint?
postgresql_version >= 90402
end

def get_advisory_lock(lock_id) # :nodoc:
unless lock_id.is_a?(Integer) && lock_id.bit_length <= 63
raise(ArgumentError, "Postgres requires advisory lock ids to be a signed 64 bit integer")
Expand Down
34 changes: 17 additions & 17 deletions activerecord/test/cases/adapter_test.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -220,23 +220,6 @@ def test_value_limit_violations_are_translated_to_specific_exception
end
end

def test_disable_referential_integrity
assert_nothing_raised do
@connection.disable_referential_integrity do
# Oracle adapter uses prefetched primary key values from sequence and passes them to connection adapter insert method
if @connection.prefetch_primary_key?
id_value = @connection.next_sequence_value(@connection.default_sequence_name("fk_test_has_fk", "id"))
@connection.execute "INSERT INTO fk_test_has_fk (id, fk_id) VALUES (#{id_value},0)"
else
@connection.execute "INSERT INTO fk_test_has_fk (fk_id) VALUES (0)"
end
# should delete created record as otherwise disable_referential_integrity will try to enable constraints after executed block
# and will fail (at least on Oracle)
@connection.execute "DELETE FROM fk_test_has_fk"
end
end
end

def test_select_all_always_return_activerecord_result
result = @connection.select_all "SELECT * FROM posts"
assert result.is_a?(ActiveRecord::Result)
Expand Down Expand Up @@ -320,5 +303,22 @@ def setup
assert !@connection.transaction_open?
end
end

def test_disable_referential_integrity
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why move the test to the bottom?

Copy link
Contributor Author

@mtsmfm mtsmfm Apr 1, 2016 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Because we can't use disable_referential_integrity_with_alter_constraint within transaction now.

-- setup
DROP TABLE "fk_test_has_fk";
CREATE TABLE "fk_test_has_fk" ("fk_id" integer NOT NULL);
DROP TABLE "fk_test_has_pk";
CREATE TABLE "fk_test_has_pk" ("pk_id" serial primary key);
ALTER TABLE "fk_test_has_fk" ADD CONSTRAINT "fk_name" FOREIGN KEY ("fk_id") REFERENCES "fk_test_has_pk" ("pk_id");

-- test
BEGIN;

ALTER TABLE fk_test_has_fk ALTER CONSTRAINT fk_name DEFERRABLE;

SET CONSTRAINTS ALL DEFERRED;

INSERT INTO fk_test_has_fk VALUES(1);
INSERT INTO fk_test_has_pk VALUES(1);

ALTER TABLE fk_test_has_fk ALTER CONSTRAINT fk_name NOT DEFERRABLE;
-- ERROR:  cannot ALTER TABLE "fk_test_has_fk" because it has pending trigger events

So we must put this test within AdapterTestWithoutTransaction

And fall back version will work well even outside transaction.

I think we can accept this change because this method will use only for fixture.

assert_nothing_raised do
@connection.disable_referential_integrity do
# Oracle adapter uses prefetched primary key values from sequence and passes them to connection adapter insert method
if @connection.prefetch_primary_key?
id_value = @connection.next_sequence_value(@connection.default_sequence_name("fk_test_has_fk", "id"))
@connection.execute "INSERT INTO fk_test_has_fk (id, fk_id) VALUES (#{id_value},0)"
else
@connection.execute "INSERT INTO fk_test_has_fk (fk_id) VALUES (0)"
end
# should delete created record as otherwise disable_referential_integrity will try to enable constraints after executed block
# and will fail (at least on Oracle)
@connection.execute "DELETE FROM fk_test_has_fk"
end
end
end
end
end
175 changes: 107 additions & 68 deletions activerecord/test/cases/adapters/postgresql/referential_integrity_test.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,111 +1,150 @@
require "cases/helper"
require "support/connection_helper"

class PostgreSQLReferentialIntegrityTest < ActiveRecord::PostgreSQLTestCase
self.use_transactional_tests = false
if ActiveRecord::Base.connection.respond_to?(:supports_alter_constraint?) &&
ActiveRecord::Base.connection.supports_alter_constraint?
class PostgreSQLReferentialIntegrityWithAlterConstraintTest < ActiveRecord::PostgreSQLTestCase
self.use_transactional_tests = false

include ConnectionHelper
include ConnectionHelper

IS_REFERENTIAL_INTEGRITY_SQL = lambda do |sql|
sql.match(/DISABLE TRIGGER ALL/) || sql.match(/ENABLE TRIGGER ALL/)
end
IS_REFERENTIAL_INTEGRITY_SQL = lambda do |sql|
sql.match(/SET CONSTRAINTS ALL DEFERRED/)
end

module MissingSuperuserPrivileges
def execute(sql)
if IS_REFERENTIAL_INTEGRITY_SQL.call(sql)
super "BROKEN;" rescue nil # put transaction in broken state
raise ActiveRecord::StatementInvalid, "PG::InsufficientPrivilege"
else
super
module ProgrammerMistake
def execute(sql)
if IS_REFERENTIAL_INTEGRITY_SQL.call(sql)
raise ArgumentError, "something is not right."
else
super
end
end
end
end

module ProgrammerMistake
def execute(sql)
if IS_REFERENTIAL_INTEGRITY_SQL.call(sql)
raise ArgumentError, "something is not right."
else
super
def setup
@connection = ActiveRecord::Base.connection
end

def teardown
reset_connection
end

def test_errors_bubble_up
@connection.extend ProgrammerMistake

assert_raises ArgumentError do
@connection.disable_referential_integrity {}
end
end
end
else
class PostgreSQLReferentialIntegrityWithDisableTriggerTest < ActiveRecord::PostgreSQLTestCase
self.use_transactional_tests = false

def setup
@connection = ActiveRecord::Base.connection
end
include ConnectionHelper

def teardown
reset_connection
if ActiveRecord::Base.connection.is_a?(MissingSuperuserPrivileges)
raise "MissingSuperuserPrivileges patch was not removed"
IS_REFERENTIAL_INTEGRITY_SQL = lambda do |sql|
sql.match(/DISABLE TRIGGER ALL/) || sql.match(/ENABLE TRIGGER ALL/)
end
end

def test_should_reraise_invalid_foreign_key_exception_and_show_warning
@connection.extend MissingSuperuserPrivileges
module MissingSuperuserPrivileges
def execute(sql)
if IS_REFERENTIAL_INTEGRITY_SQL.call(sql)
super "BROKEN;" rescue nil # put transaction in broken state
raise ActiveRecord::StatementInvalid, "PG::InsufficientPrivilege"
else
super
end
end
end

warning = capture(:stderr) do
e = assert_raises(ActiveRecord::InvalidForeignKey) do
@connection.disable_referential_integrity do
raise ActiveRecord::InvalidForeignKey, "Should be re-raised"
module ProgrammerMistake
def execute(sql)
if IS_REFERENTIAL_INTEGRITY_SQL.call(sql)
raise ArgumentError, "something is not right."
else
super
end
end
assert_equal "Should be re-raised", e.message
end
assert_match (/WARNING: Rails was not able to disable referential integrity/), warning
assert_match (/cause: PG::InsufficientPrivilege/), warning
end

def test_does_not_print_warning_if_no_invalid_foreign_key_exception_was_raised
@connection.extend MissingSuperuserPrivileges
def setup
@connection = ActiveRecord::Base.connection
end

warning = capture(:stderr) do
e = assert_raises(ActiveRecord::StatementInvalid) do
@connection.disable_referential_integrity do
raise ActiveRecord::StatementInvalid, "Should be re-raised"
def teardown
reset_connection
if ActiveRecord::Base.connection.is_a?(MissingSuperuserPrivileges)
raise "MissingSuperuserPrivileges patch was not removed"
end
end

def test_should_reraise_invalid_foreign_key_exception_and_show_warning
@connection.extend MissingSuperuserPrivileges

warning = capture(:stderr) do
e = assert_raises(ActiveRecord::InvalidForeignKey) do
@connection.disable_referential_integrity do
raise ActiveRecord::InvalidForeignKey, "Should be re-raised"
end
end
assert_equal "Should be re-raised", e.message
end
assert_equal "Should be re-raised", e.message
assert_match (/WARNING: Rails was not able to disable referential integrity/), warning
assert_match (/cause: PG::InsufficientPrivilege/), warning
end
assert warning.blank?, "expected no warnings but got:\n#{warning}"
end

def test_does_not_break_transactions
@connection.extend MissingSuperuserPrivileges
def test_does_not_print_warning_if_no_invalid_foreign_key_exception_was_raised
@connection.extend MissingSuperuserPrivileges

@connection.transaction do
@connection.disable_referential_integrity do
assert_transaction_is_not_broken
warning = capture(:stderr) do
e = assert_raises(ActiveRecord::StatementInvalid) do
@connection.disable_referential_integrity do
raise ActiveRecord::StatementInvalid, "Should be re-raised"
end
end
assert_equal "Should be re-raised", e.message
end
assert_transaction_is_not_broken
assert warning.blank?, "expected no warnings but got:\n#{warning}"
end
end

def test_does_not_break_nested_transactions
@connection.extend MissingSuperuserPrivileges
def test_does_not_break_transactions
@connection.extend MissingSuperuserPrivileges

@connection.transaction do
@connection.transaction(requires_new: true) do
@connection.transaction do
@connection.disable_referential_integrity do
assert_transaction_is_not_broken
end
assert_transaction_is_not_broken
end
assert_transaction_is_not_broken
end
end

def test_only_catch_active_record_errors_others_bubble_up
@connection.extend ProgrammerMistake
def test_does_not_break_nested_transactions
@connection.extend MissingSuperuserPrivileges

assert_raises ArgumentError do
@connection.disable_referential_integrity {}
@connection.transaction do
@connection.transaction(requires_new: true) do
@connection.disable_referential_integrity do
assert_transaction_is_not_broken
end
end
assert_transaction_is_not_broken
end
end
end

private
def test_only_catch_active_record_errors_others_bubble_up
@connection.extend ProgrammerMistake

def assert_transaction_is_not_broken
assert_equal 1, @connection.select_value("SELECT 1")
assert_raises ArgumentError do
@connection.disable_referential_integrity {}
end
end

private

def assert_transaction_is_not_broken
assert_equal 1, @connection.select_value("SELECT 1")
end
end
end
2 changes: 1 addition & 1 deletion activerecord/test/cases/associations/callbacks_test.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@
require "models/company"

class AssociationCallbacksTest < ActiveRecord::TestCase
fixtures :posts, :authors, :projects, :developers
fixtures :posts, :authors, :author_addresses, :projects, :developers

def setup
@david = authors(:david)
Expand Down
2 changes: 1 addition & 1 deletion activerecord/test/cases/associations/cascaded_eager_loading_test.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,7 @@
require "models/edge"

class CascadedEagerLoadingTest < ActiveRecord::TestCase
fixtures :authors, :mixins, :companies, :posts, :topics, :accounts, :comments,
fixtures :authors, :author_addresses, :mixins, :companies, :posts, :topics, :accounts, :comments,
:categorizations, :people, :categories, :edges, :vertices

def test_eager_association_loading_with_cascaded_two_levels
Expand Down
Loading