Skip to content

Commit 5413f34

Browse files
yahondaclaude
andcommitted
Replace rescue nil cleanup with drop_table / drop_if_exists
`spec/.../connection_spec.rb` had eighteen idempotent-cleanup sites of the shape `@conn.execute "DROP <type> <name>" rescue nil`. The bare `rescue nil` swallows every database error — privilege failures, ORA-00972 identifier-too-long on 11g, syntax errors — and lets the spec assert against half-built fixtures (case in point: PR #2686 spent days chasing an `ORA-00972` that the catch-all had hidden). For TABLE drops, switch to the standard ActiveRecord public API `drop_table(name, if_exists: true)`. On Oracle 23.4+ that issues native `DROP TABLE IF EXISTS ...`; on older releases it falls back to a narrow rescue keyed on ORA-00942 only. The default sequence (`<table>_seq`) is also cleaned up by `drop_table` automatically, which is desirable here even though these specific specs don't create one. For non-TABLE drops (VIEW, MATERIALIZED VIEW, SYNONYM, PUBLIC SYNONYM) ActiveRecord has no public API, so call the `OracleEnhancedAdapter#drop_if_exists(object_type, name, if_exists:, cascade_constraints:)` helper introduced in PR #2688 directly. Each call names the object kind explicitly so the rescue narrows on the right ORA code (VIEW → ORA-00942, MV → ORA-12003, SYNONYM → ORA-01434, PUBLIC SYNONYM → ORA-01432). Net behavior change: the same expected misses are still tolerated, but unexpected failures (privileges, syntax, identifier limit) propagate instead of being silently absorbed. The few CREATE statements that previously also had `rescue nil` (see `should resolve existing table` and friends) are stripped of that rescue too — those fixtures are spec preconditions, not optional, and a real failure should fail the spec rather than be hidden. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
1 parent ab2d624 commit 5413f34

1 file changed

Lines changed: 35 additions & 35 deletions

File tree

spec/active_record/connection_adapters/oracle_enhanced/connection_spec.rb

Lines changed: 35 additions & 35 deletions
Original file line numberDiff line numberDiff line change
@@ -726,7 +726,7 @@ def lookup(path)
726726

727727
after(:all) do
728728
ENV["NLS_NUMERIC_CHARACTERS"] = nil
729-
@conn.exec "DROP TABLE test_employees" rescue nil
729+
@conn_base.drop_table("test_employees", if_exists: true)
730730
ActiveRecord::Base.clear_cache!
731731
end
732732

@@ -911,9 +911,9 @@ def resolve(name)
911911
end
912912

913913
it "should resolve existing table" do
914-
@conn.execute "CREATE TABLE test_employees (first_name VARCHAR2(20))" rescue nil
914+
@conn.execute "CREATE TABLE test_employees (first_name VARCHAR2(20))"
915915
expect(resolve("test_employees")).to eq([@owner, "TEST_EMPLOYEES"])
916-
@conn.execute "DROP TABLE test_employees" rescue nil
916+
@conn.drop_table("test_employees", if_exists: true)
917917
end
918918

919919
it "should not resolve non-existing table" do
@@ -929,29 +929,29 @@ def resolve(name)
929929
end
930930

931931
it "should resolve existing view" do
932-
@conn.execute "CREATE TABLE test_employees (first_name VARCHAR2(20))" rescue nil
933-
@conn.execute "CREATE VIEW test_employees_v AS SELECT * FROM test_employees" rescue nil
932+
@conn.execute "CREATE TABLE test_employees (first_name VARCHAR2(20))"
933+
@conn.execute "CREATE VIEW test_employees_v AS SELECT * FROM test_employees"
934934
expect(resolve("test_employees_v")).to eq([@owner, "TEST_EMPLOYEES_V"])
935-
@conn.execute "DROP VIEW test_employees_v" rescue nil
936-
@conn.execute "DROP TABLE test_employees" rescue nil
935+
@conn.drop_if_exists("VIEW", "test_employees_v")
936+
@conn.drop_table("test_employees", if_exists: true)
937937
end
938938

939939
it "should resolve view in other schema" do
940940
expect(resolve("sys.v_$version")).to eq(["SYS", "V_$VERSION"])
941941
end
942942

943943
it "should resolve existing materialized view" do
944-
@conn.execute "CREATE TABLE test_employees (first_name VARCHAR2(20))" rescue nil
945-
@conn.execute "CREATE MATERIALIZED VIEW test_employees_mv AS SELECT * FROM test_employees" rescue nil
944+
@conn.execute "CREATE TABLE test_employees (first_name VARCHAR2(20))"
945+
@conn.execute "CREATE MATERIALIZED VIEW test_employees_mv AS SELECT * FROM test_employees"
946946
expect(resolve("test_employees_mv")).to eq([@owner, "TEST_EMPLOYEES_MV"])
947-
@conn.execute "DROP MATERIALIZED VIEW test_employees_mv" rescue nil
948-
@conn.execute "DROP TABLE test_employees" rescue nil
947+
@conn.drop_if_exists("MATERIALIZED VIEW", "test_employees_mv")
948+
@conn.drop_table("test_employees", if_exists: true)
949949
end
950950

951951
it "should resolve existing private synonym" do
952-
@conn.execute "CREATE SYNONYM test_dual FOR sys.dual" rescue nil
952+
@conn.execute "CREATE SYNONYM test_dual FOR sys.dual"
953953
expect(resolve("test_dual")).to eq(["SYS", "DUAL"])
954-
@conn.execute "DROP SYNONYM test_dual" rescue nil
954+
@conn.drop_if_exists("SYNONYM", "test_dual")
955955
end
956956

957957
it "should resolve existing public synonym" do
@@ -966,49 +966,49 @@ def resolve(name)
966966
# coexist, and that a materialized view created on the same base table
967967
# resolves to the MV name (not the base table) as a sibling data source.
968968
it "resolves table, view, materialized view, private synonym and public synonym for the same underlying table" do
969-
@conn.execute "CREATE TABLE test_describe_all (id NUMBER)" rescue nil
970-
@conn.execute "CREATE VIEW test_describe_all_v AS SELECT * FROM test_describe_all" rescue nil
971-
@conn.execute "CREATE MATERIALIZED VIEW test_describe_all_mv AS SELECT * FROM test_describe_all" rescue nil
972-
@conn.execute "CREATE SYNONYM test_describe_all_syn FOR test_describe_all" rescue nil
973-
@conn.execute "CREATE PUBLIC SYNONYM test_describe_all_pub FOR #{@owner}.test_describe_all" rescue nil
969+
@conn.execute "CREATE TABLE test_describe_all (id NUMBER)"
970+
@conn.execute "CREATE VIEW test_describe_all_v AS SELECT * FROM test_describe_all"
971+
@conn.execute "CREATE MATERIALIZED VIEW test_describe_all_mv AS SELECT * FROM test_describe_all"
972+
@conn.execute "CREATE SYNONYM test_describe_all_syn FOR test_describe_all"
973+
@conn.execute "CREATE PUBLIC SYNONYM test_describe_all_pub FOR #{@owner}.test_describe_all"
974974

975975
expect(resolve("test_describe_all")).to eq([@owner, "TEST_DESCRIBE_ALL"])
976976
expect(resolve("test_describe_all_v")).to eq([@owner, "TEST_DESCRIBE_ALL_V"])
977977
expect(resolve("test_describe_all_mv")).to eq([@owner, "TEST_DESCRIBE_ALL_MV"])
978978
expect(resolve("test_describe_all_syn")).to eq([@owner, "TEST_DESCRIBE_ALL"])
979979
expect(resolve("test_describe_all_pub")).to eq([@owner, "TEST_DESCRIBE_ALL"])
980980
ensure
981-
@conn.execute "DROP PUBLIC SYNONYM test_describe_all_pub" rescue nil
982-
@conn.execute "DROP SYNONYM test_describe_all_syn" rescue nil
983-
@conn.execute "DROP MATERIALIZED VIEW test_describe_all_mv" rescue nil
984-
@conn.execute "DROP VIEW test_describe_all_v" rescue nil
985-
@conn.execute "DROP TABLE test_describe_all" rescue nil
981+
@conn.drop_if_exists("PUBLIC SYNONYM", "test_describe_all_pub")
982+
@conn.drop_if_exists("SYNONYM", "test_describe_all_syn")
983+
@conn.drop_if_exists("MATERIALIZED VIEW", "test_describe_all_mv")
984+
@conn.drop_if_exists("VIEW", "test_describe_all_v")
985+
@conn.drop_table("test_describe_all", if_exists: true)
986986
end
987987

988988
it "raises when synonym resolution produces a looping chain" do
989-
@conn.execute "CREATE SYNONYM test_cycle_a FOR test_cycle_b" rescue nil
990-
@conn.execute "CREATE SYNONYM test_cycle_b FOR test_cycle_a" rescue nil
989+
@conn.execute "CREATE SYNONYM test_cycle_a FOR test_cycle_b"
990+
@conn.execute "CREATE SYNONYM test_cycle_b FOR test_cycle_a"
991991
expect { resolve("test_cycle_a") }.to raise_error(
992992
ActiveRecord::ConnectionAdapters::OracleEnhanced::ConnectionException,
993993
/looping chain of synonyms/
994994
)
995995
ensure
996-
@conn.execute "DROP SYNONYM test_cycle_a" rescue nil
997-
@conn.execute "DROP SYNONYM test_cycle_b" rescue nil
996+
@conn.drop_if_exists("SYNONYM", "test_cycle_a")
997+
@conn.drop_if_exists("SYNONYM", "test_cycle_b")
998998
end
999999

10001000
it "raises when a multi-hop synonym chain eventually revisits an earlier link" do
1001-
@conn.execute "CREATE SYNONYM test_cycle_a FOR test_cycle_b" rescue nil
1002-
@conn.execute "CREATE SYNONYM test_cycle_b FOR test_cycle_c" rescue nil
1003-
@conn.execute "CREATE SYNONYM test_cycle_c FOR test_cycle_a" rescue nil
1001+
@conn.execute "CREATE SYNONYM test_cycle_a FOR test_cycle_b"
1002+
@conn.execute "CREATE SYNONYM test_cycle_b FOR test_cycle_c"
1003+
@conn.execute "CREATE SYNONYM test_cycle_c FOR test_cycle_a"
10041004
expect { resolve("test_cycle_a") }.to raise_error(
10051005
ActiveRecord::ConnectionAdapters::OracleEnhanced::ConnectionException,
10061006
/looping chain of synonyms/
10071007
)
10081008
ensure
1009-
@conn.execute "DROP SYNONYM test_cycle_a" rescue nil
1010-
@conn.execute "DROP SYNONYM test_cycle_b" rescue nil
1011-
@conn.execute "DROP SYNONYM test_cycle_c" rescue nil
1009+
@conn.drop_if_exists("SYNONYM", "test_cycle_a")
1010+
@conn.drop_if_exists("SYNONYM", "test_cycle_b")
1011+
@conn.drop_if_exists("SYNONYM", "test_cycle_c")
10121012
end
10131013

10141014
it "raises ArgumentError when the name contains a db link" do
@@ -1021,7 +1021,7 @@ def resolve(name)
10211021
# participate in logging, instrumentation, and the query cache. Lock that in
10221022
# so a future refactor can't silently regress to the raw-cursor path.
10231023
it "emits a SCHEMA sql.active_record event for the catalog lookup" do
1024-
@conn.execute "CREATE TABLE test_employees (first_name VARCHAR2(20))" rescue nil
1024+
@conn.execute "CREATE TABLE test_employees (first_name VARCHAR2(20))"
10251025
events = []
10261026
subscriber = ActiveSupport::Notifications.subscribe("sql.active_record") do |*, payload|
10271027
events << payload
@@ -1030,7 +1030,7 @@ def resolve(name)
10301030
expect(events.map { |p| p[:name] }).to include("SCHEMA")
10311031
ensure
10321032
ActiveSupport::Notifications.unsubscribe(subscriber) if subscriber
1033-
@conn.execute "DROP TABLE test_employees" rescue nil
1033+
@conn.drop_table("test_employees", if_exists: true)
10341034
end
10351035
end
10361036

0 commit comments

Comments
 (0)