Skip to content

Commit bb446f2

Browse files
yahondaclaude
andcommitted
Address review of enforced: option implementation
- change_foreign_key now raises ArgumentError when :enforced is omitted, matching rails/rails#57377's PG behavior. Without the guard, the bare call `change_foreign_key :foo, :bar` would silently emit `... DISABLE` via the `enforced ? 'ENABLE' : 'DISABLE'` ternary on nil. - Docstring on change_foreign_key documents the accepted opts (:enforced plus identifying keys) and the side effect that bare DISABLE/ENABLE also flip VALIDATED — surfaced previously only in the commit message. - visit_ForeignKeyDefinition uses o.enforced? (the predicate added on ForeignKeyDefinition by rails/rails#57377) for parity with !o.validate? on the next line. - New specs: - change_foreign_key without :enforced raises ArgumentError - change_foreign_key identified by name: form - add_reference foreign_key: { enforced: false, validate: false } - inline t.foreign_key inside create_table - enforced: false + deferrable: :deferred round-trip - completeness assertion (key?(:validate) == false) on the "enables a DISABLEd FK" spec, confirming ENABLE VALIDATE state after bare ENABLE - Trim duplicative spec comments per the project's terse-comments norm. To be squashed into 748a42a once CI is green. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
1 parent 748a42a commit bb446f2

4 files changed

Lines changed: 66 additions & 12 deletions

File tree

lib/active_record/connection_adapters/oracle_enhanced/schema_creation.rb

Lines changed: 2 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -101,11 +101,10 @@ def add_column_options!(sql, options)
101101
def visit_ForeignKeyDefinition(o)
102102
super.dup.tap do |sql|
103103
sql << " DEFERRABLE INITIALLY #{o.deferrable.to_s.upcase}" if o.deferrable
104-
enforced = o.options.fetch(:enforced, true)
105-
sql << " DISABLE" unless enforced
104+
sql << " DISABLE" unless o.enforced?
106105
if !o.validate?
107106
sql << " NOVALIDATE"
108-
elsif !enforced
107+
elsif !o.enforced?
109108
sql << " VALIDATE"
110109
end
111110
end

lib/active_record/connection_adapters/oracle_enhanced/schema_statements.rb

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -823,7 +823,15 @@ def validate_foreign_key(from_table, to_table = nil, **options) # :nodoc:
823823
validate_constraint(from_table, fk_name_to_validate)
824824
end
825825

826+
# Accepted options: +:enforced+ (the only mutable axis) plus the identifying
827+
# keys +:column+, +:name+, +:to_table+ passed through to +foreign_key_for!+.
828+
# ALTER ... MODIFY CONSTRAINT name DISABLE/ENABLE follows Oracle's defaults:
829+
# bare DISABLE leaves the constraint at DISABLE NOVALIDATE, and bare ENABLE
830+
# at ENABLE VALIDATE; introspection reflects those side effects on +:validate+.
826831
def change_foreign_key(from_table, to_table = nil, **options) # :nodoc:
832+
unless options.key?(:enforced)
833+
raise ArgumentError, "change_foreign_key requires at least one option (e.g. enforced:)"
834+
end
827835
enforced = options[:enforced]
828836
fk_name = foreign_key_for!(from_table, to_table: to_table, **options.except(:enforced)).name
829837
execute "ALTER TABLE #{quote_table_name(from_table)} MODIFY CONSTRAINT #{quote_column_name(fk_name)} #{enforced ? 'ENABLE' : 'DISABLE'}"

spec/active_record/connection_adapters/oracle_enhanced/schema_dumper_spec.rb

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -402,7 +402,6 @@ def drop_test_posts_table
402402
end
403403

404404
dumped = dump_table_schema "test_comments"
405-
# validate: true is the Rails default and is not emitted; only enforced: false appears.
406405
expect(dumped).to match(/add_foreign_key "test_comments", "test_posts".*enforced: false/)
407406
expect(dumped).not_to match(/validate: /)
408407

spec/active_record/connection_adapters/oracle_enhanced/schema_statements_spec.rb

Lines changed: 56 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -1951,19 +1951,13 @@ class ::TestComment < ActiveRecord::Base
19511951
expect(fk.options.key?(:validate)).to be(false)
19521952
end
19531953

1954-
# `enforced:` and `validate:` are independent options mapping to Oracle's
1955-
# 4-state constraint model. See the matrix in
1956-
# OracleEnhanced::SchemaCreation#visit_ForeignKeyDefinition.
1957-
19581954
it "creates DISABLE VALIDATE when enforced: false is given (validate defaults to true)" do
19591955
schema_define do
19601956
add_foreign_key :test_comments, :test_posts, enforced: false
19611957
end
19621958
fk = ActiveRecord::Base.lease_connection.foreign_keys(:test_comments).first
19631959
expect(fk.options[:enforced]).to be(false)
19641960
expect(fk.options.key?(:validate)).to be(false)
1965-
# DISABLE VALIDATE blocks DML on the table (ORA-25128). Users who want
1966-
# PostgreSQL-NOT-ENFORCED semantics must pass `validate: false` as well.
19671961
expect do
19681962
TestComment.create(body: "test", test_post_id: 1)
19691963
end.to raise_error(/ORA-25128/)
@@ -1997,6 +1991,7 @@ class ::TestComment < ActiveRecord::Base
19971991
end
19981992
fk = ActiveRecord::Base.lease_connection.foreign_keys(:test_comments).first
19991993
expect(fk.options.key?(:enforced)).to be(false)
1994+
expect(fk.options.key?(:validate)).to be(false)
20001995
expect do
20011996
TestComment.create(body: "test", test_post_id: 1)
20021997
end.to raise_error(/ORA-02291/)
@@ -2009,13 +2004,66 @@ class ::TestComment < ActiveRecord::Base
20092004
end
20102005
fk = ActiveRecord::Base.lease_connection.foreign_keys(:test_comments).first
20112006
expect(fk.options[:enforced]).to be(false)
2012-
# ALTER ... MODIFY CONSTRAINT name DISABLE drops the VALIDATED state too,
2013-
# so introspection reports validate: false alongside enforced: false.
20142007
expect(fk.options[:validate]).to be(false)
20152008
expect do
20162009
TestComment.create(body: "test", test_post_id: 1)
20172010
end.not_to raise_error
20182011
end
2012+
2013+
it "raises ArgumentError when change_foreign_key is called without :enforced" do
2014+
schema_define do
2015+
add_foreign_key :test_comments, :test_posts
2016+
end
2017+
expect do
2018+
ActiveRecord::Base.lease_connection.change_foreign_key :test_comments, :test_posts
2019+
end.to raise_error(ArgumentError, /change_foreign_key requires at least one option/)
2020+
end
2021+
2022+
it "toggles enforced via change_foreign_key identified by name:" do
2023+
schema_define do
2024+
add_foreign_key :test_comments, :test_posts, name: "comments_posts_fk"
2025+
change_foreign_key :test_comments, name: "comments_posts_fk", enforced: false
2026+
end
2027+
fk = ActiveRecord::Base.lease_connection.foreign_keys(:test_comments).first
2028+
expect(fk.options[:enforced]).to be(false)
2029+
end
2030+
2031+
it "honors enforced: false on add_reference foreign_key option hash" do
2032+
schema_define do
2033+
drop_table :test_comments, if_exists: true
2034+
create_table :test_comments, force: true do |t|
2035+
t.string :body, limit: 4000
2036+
end
2037+
add_reference :test_comments, :test_post, foreign_key: { enforced: false, validate: false }
2038+
end
2039+
fk = ActiveRecord::Base.lease_connection.foreign_keys(:test_comments).first
2040+
expect(fk.options[:enforced]).to be(false)
2041+
expect(fk.options[:validate]).to be(false)
2042+
end
2043+
2044+
it "honors enforced: false on inline t.foreign_key inside create_table" do
2045+
schema_define do
2046+
drop_table :test_comments, if_exists: true
2047+
create_table :test_comments, force: true do |t|
2048+
t.string :body, limit: 4000
2049+
t.references :test_post
2050+
t.foreign_key :test_posts, enforced: false, validate: false
2051+
end
2052+
end
2053+
fk = ActiveRecord::Base.lease_connection.foreign_keys(:test_comments).first
2054+
expect(fk.options[:enforced]).to be(false)
2055+
expect(fk.options[:validate]).to be(false)
2056+
end
2057+
2058+
it "round-trips enforced: false combined with deferrable: :deferred" do
2059+
schema_define do
2060+
add_foreign_key :test_comments, :test_posts, enforced: false, validate: false, deferrable: :deferred
2061+
end
2062+
fk = ActiveRecord::Base.lease_connection.foreign_keys(:test_comments).first
2063+
expect(fk.options[:enforced]).to be(false)
2064+
expect(fk.options[:validate]).to be(false)
2065+
expect(fk.options[:deferrable]).to eq(:deferred)
2066+
end
20192067
end
20202068

20212069
describe "check constraints" do

0 commit comments

Comments
 (0)