Skip to content
Permalink
Browse files

Revert "Merge pull request #27636 from mtsmfm/disable-referential-int…

…egrity-without-superuser-privilege-take-2"

This reverts commit 1ccd40e.

Fixes #28888

Referential actions other than the NO ACTION check cannot be deferred,
even if the constraint is declared deferrable.
  • Loading branch information...
rafaelfranca committed Apr 26, 2017
1 parent 3d00452 commit 2812694aa65e4d0591e0ea6c5f7d8c97927be7dc
Showing with 98 additions and 185 deletions.
  1. +0 −3 .travis.yml
  2. +4 −43 activerecord/lib/active_record/connection_adapters/postgresql/referential_integrity.rb
  3. +0 −6 activerecord/lib/active_record/connection_adapters/postgresql_adapter.rb
  4. +68 −107 activerecord/test/cases/adapters/postgresql/referential_integrity_test.rb
  5. +1 −1 activerecord/test/cases/associations/callbacks_test.rb
  6. +1 −1 activerecord/test/cases/associations/cascaded_eager_loading_test.rb
  7. +3 −3 activerecord/test/cases/associations/has_many_associations_test.rb
  8. +1 −1 activerecord/test/cases/associations/has_one_through_associations_test.rb
  9. +1 −1 activerecord/test/cases/associations/inner_join_association_test.rb
  10. +1 −1 activerecord/test/cases/associations/join_model_test.rb
  11. +1 −1 activerecord/test/cases/associations/left_outer_join_association_test.rb
  12. +1 −1 activerecord/test/cases/associations/nested_through_associations_test.rb
  13. +2 −2 activerecord/test/cases/associations_test.rb
  14. +1 −1 activerecord/test/cases/base_test.rb
  15. +1 −1 activerecord/test/cases/bind_parameter_test.rb
  16. +1 −1 activerecord/test/cases/fixtures_test.rb
  17. +1 −1 activerecord/test/cases/json_serialization_test.rb
  18. +1 −1 activerecord/test/cases/readonly_test.rb
  19. +2 −2 activerecord/test/cases/relation/merging_test.rb
  20. +1 −1 activerecord/test/cases/relation/where_test.rb
  21. +1 −1 activerecord/test/cases/relation_test.rb
  22. +1 −1 activerecord/test/cases/relations_test.rb
  23. +2 −2 activerecord/test/cases/scoping/relation_scoping_test.rb
  24. +1 −1 activerecord/test/cases/transactions_test.rb
  25. +1 −1 activerecord/test/cases/yaml_serialization_test.rb
@@ -15,9 +15,6 @@ services:

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

bundler_args: --without test --jobs 3 --retry 3
before_install:
@@ -6,50 +6,8 @@ def supports_disable_referential_integrity? # :nodoc:
true
end

def disable_referential_integrity(&block) # :nodoc:
def disable_referential_integrity # :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
@@ -81,7 +39,10 @@ def disable_referential_integrity_with_disable_trigger
end
rescue ActiveRecord::ActiveRecordError
end
else
yield
end
end
end
end
end
@@ -322,12 +322,6 @@ def supports_pgcrypto_uuid?
postgresql_version >= 90400
end

def supports_alter_constraint?
# 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
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")
@@ -1,150 +1,111 @@
require "cases/helper"
require "support/connection_helper"

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

include ConnectionHelper
include ConnectionHelper

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

module ProgrammerMistake
def execute(sql)
if IS_REFERENTIAL_INTEGRITY_SQL.call(sql)
raise ArgumentError, "something is not right."
else
super
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
end
end
end

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 {}
module ProgrammerMistake
def execute(sql)
if IS_REFERENTIAL_INTEGRITY_SQL.call(sql)
raise ArgumentError, "something is not right."
else
super
end
end
end
else
class PostgreSQLReferentialIntegrityWithDisableTriggerTest < ActiveRecord::PostgreSQLTestCase
self.use_transactional_tests = false

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

IS_REFERENTIAL_INTEGRITY_SQL = lambda do |sql|
sql.match(/DISABLE TRIGGER ALL/) || sql.match(/ENABLE TRIGGER ALL/)
def teardown
reset_connection
if ActiveRecord::Base.connection.is_a?(MissingSuperuserPrivileges)
raise "MissingSuperuserPrivileges patch was not removed"
end
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
end
end
end
def test_should_reraise_invalid_foreign_key_exception_and_show_warning
@connection.extend MissingSuperuserPrivileges

module ProgrammerMistake
def execute(sql)
if IS_REFERENTIAL_INTEGRITY_SQL.call(sql)
raise ArgumentError, "something is not right."
else
super
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_match (/WARNING: Rails was not able to disable referential integrity/), warning
assert_match (/cause: PG::InsufficientPrivilege/), warning
end

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

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
def test_does_not_print_warning_if_no_invalid_foreign_key_exception_was_raised
@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
warning = capture(:stderr) do
e = assert_raises(ActiveRecord::StatementInvalid) do
@connection.disable_referential_integrity do
raise ActiveRecord::StatementInvalid, "Should be re-raised"
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
assert_equal "Should be re-raised", e.message
end
assert warning.blank?, "expected no warnings but got:\n#{warning}"
end

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

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
@connection.transaction do
@connection.disable_referential_integrity do
assert_transaction_is_not_broken
end
assert warning.blank?, "expected no warnings but got:\n#{warning}"
assert_transaction_is_not_broken
end
end

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

@connection.transaction do
@connection.transaction do
@connection.transaction(requires_new: true) 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_does_not_break_nested_transactions
@connection.extend MissingSuperuserPrivileges
def test_only_catch_active_record_errors_others_bubble_up
@connection.extend ProgrammerMistake

@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
assert_raises ArgumentError do
@connection.disable_referential_integrity {}
end
end

def test_only_catch_active_record_errors_others_bubble_up
@connection.extend ProgrammerMistake
private

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

private

def assert_transaction_is_not_broken
assert_equal 1, @connection.select_value("SELECT 1")
end
end
end
@@ -7,7 +7,7 @@
require "models/company"

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

def setup
@david = authors(:david)
@@ -12,7 +12,7 @@
require "models/edge"

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

def test_eager_association_loading_with_cascaded_two_levels
@@ -40,7 +40,7 @@
require "models/interest"

class HasManyAssociationsTestForReorderWithJoinDependency < ActiveRecord::TestCase
fixtures :authors, :author_addresses, :posts, :comments
fixtures :authors, :posts, :comments

def test_should_generate_valid_sql
author = authors(:david)
@@ -51,7 +51,7 @@ def test_should_generate_valid_sql
end

class HasManyAssociationsTestPrimaryKeys < ActiveRecord::TestCase
fixtures :authors, :author_addresses, :essays, :subscribers, :subscriptions, :people
fixtures :authors, :essays, :subscribers, :subscriptions, :people

def test_custom_primary_key_on_new_record_should_fetch_with_query
subscriber = Subscriber.new(nick: "webster132")
@@ -100,7 +100,7 @@ def test_blank_custom_primary_key_on_new_record_should_not_run_queries

class HasManyAssociationsTest < ActiveRecord::TestCase
fixtures :accounts, :categories, :companies, :developers, :projects,
:developers_projects, :topics, :authors, :author_addresses, :comments,
:developers_projects, :topics, :authors, :comments,
:posts, :readers, :taggings, :cars, :jobs, :tags,
:categorizations, :zines, :interests

@@ -23,7 +23,7 @@

class HasOneThroughAssociationsTest < ActiveRecord::TestCase
fixtures :member_types, :members, :clubs, :memberships, :sponsors, :organizations, :minivans,
:dashboards, :speedometers, :authors, :author_addresses, :posts, :comments, :categories, :essays, :owners
:dashboards, :speedometers, :authors, :posts, :comments, :categories, :essays, :owners

def setup
@member = members(:groucho)
@@ -10,7 +10,7 @@
require "models/tag"

class InnerJoinAssociationTest < ActiveRecord::TestCase
fixtures :authors, :author_addresses, :essays, :posts, :comments, :categories, :categories_posts, :categorizations,
fixtures :authors, :essays, :posts, :comments, :categories, :categories_posts, :categorizations,
:taggings, :tags

def test_construct_finder_sql_applies_aliases_tables_on_association_conditions
@@ -19,7 +19,7 @@
class AssociationsJoinModelTest < ActiveRecord::TestCase
self.use_transactional_tests = false unless supports_savepoints?

fixtures :posts, :authors, :author_addresses, :categories, :categorizations, :comments, :tags, :taggings, :author_favorites, :vertices, :items, :books,
fixtures :posts, :authors, :categories, :categorizations, :comments, :tags, :taggings, :author_favorites, :vertices, :items, :books,
# Reload edges table from fixtures as otherwise repeated test was failing
:edges

@@ -7,7 +7,7 @@
require "models/person"

class LeftOuterJoinAssociationTest < ActiveRecord::TestCase
fixtures :authors, :essays, :posts, :comments, :categorizations, :people, :author_addresses
fixtures :authors, :essays, :posts, :comments, :categorizations, :people

def test_construct_finder_sql_applies_aliases_tables_on_association_conditions
result = Author.left_outer_joins(:thinking_posts, :welcome_posts).to_a
@@ -24,7 +24,7 @@
require "models/essay"

class NestedThroughAssociationsTest < ActiveRecord::TestCase
fixtures :authors, :author_addresses, :books, :posts, :subscriptions, :subscribers, :tags, :taggings,
fixtures :authors, :books, :posts, :subscriptions, :subscribers, :tags, :taggings,
:people, :readers, :references, :jobs, :ratings, :comments, :members, :member_details,
:member_types, :sponsors, :clubs, :organizations, :categories, :categories_posts,
:categorizations, :memberships, :essays

0 comments on commit 2812694

Please sign in to comment.
You can’t perform that action at this time.