Skip to content

Sweep rescue nil from remaining specs (+ remove_context_index if_exists:)#2690

Merged
yahonda merged 2 commits into
rsim:masterfrom
yahonda:sweep-rescue-nil-remaining
May 4, 2026
Merged

Sweep rescue nil from remaining specs (+ remove_context_index if_exists:)#2690
yahonda merged 2 commits into
rsim:masterfrom
yahonda:sweep-rescue-nil-remaining

Conversation

@yahonda

@yahonda yahonda commented May 4, 2026

Copy link
Copy Markdown
Collaborator

Summary

Continues the cleanup begun in PR #2687 (which swept connection_spec.rb). Replaces the @conn.execute "DROP …" rescue nil and friends pattern across the remaining spec files. The bare rescue nil swallows every error — privilege failures, ORA-00972 identifier-too-long on 11g, syntax errors — and lets specs assert against half-built fixtures (case in point: PR #2686 spent days chasing an ORA-00972 that the catch-all had hidden).

Two commits

1. Support if_exists: on remove_context_index (3109cd8f) — lib + spec

ActiveRecord's remove_index accepts if_exists: true so callers can issue idempotent cleanup without rescuing themselves; the Oracle Enhanced remove_context_index helper had no such option, so spec cleanup paths used rescue nil. This commit adds the option:

remove_context_index :posts, name: "post_and_comments_index", if_exists: true

if_exists: is forwarded to drop_if_exists("INDEX", index_name, if_exists: …) (rescues only ORA-01418 "specified index does not exist"). The auxiliary trigger and procedure cleanup inside remove_context_index are also routed through drop_if_exists (always if_exists: true since they're conditionally created), replacing inline rescue nil in those private helpers.

The drop_ctx_preference PL/SQL call is left as-is — Oracle Text returns DRG-* codes, not ORA-*, and narrowing it cleanly requires a separate per-code audit.

2. Sweep rescue nil cleanup paths across remaining specs (9fedcdd8) — six spec files

File Pattern → replacement
oracle_enhanced_data_types_spec.rb DROP TABLE + DROP SEQUENCE → drop_table(name, if_exists: true) (default <table>_seq is cleaned up by drop_table itself)
oracle_enhanced/dbms_metadata_structure_dump_spec.rb DROP PACKAGE → drop_if_exists("PACKAGE", name)
oracle_enhanced/schema_dumper_spec.rb remove_foreign_key … rescue nilremove_foreign_key …, if_exists: true; DROP MV → drop_if_exists("MATERIALIZED VIEW", …)
oracle_enhanced/schema_statements_spec.rb DROP MV → drop_if_exists("MATERIALIZED VIEW", …)
oracle_enhanced/quoting_spec.rb Object.send(:remove_const, "X") rescue nilObject.send(:remove_const, "X") if Object.const_defined?("X")
oracle_enhanced/structure_dump_spec.rb DROP VIEW/MV/SYN/PKG/FUNC/PROC/TYPE/TRIGGER → drop_if_exists("<TYPE>", name); ALTER TABLE … DROP CONSTRAINT/COLUMN lines simplified out (constraints/columns disappear with their parent tables)

Net behavior

The same expected misses are still tolerated, but unexpected failures (privileges, syntax, identifier limit, etc.) now propagate instead of being silently absorbed.

Test plan

  • bundle exec rspec full suite — 748 examples, 0 failures, 12 pending (CRuby 4.0.3 / Oracle 23ai 23.26.1)
  • bundle exec rubocop — clean
  • Each touched file run individually green

Out of scope

A handful of rescue nil callsites remain in the spec suite that this PR does not touch:

  • oracle_enhanced_adapter_spec.rb
  • oracle_enhanced/primary_key_trigger_spec.rb (single SEQUENCE drop — minor)
  • oracle_enhanced/schema_cache_spec.rb (5 drop_table … rescue nil — easy follow-up)
  • oracle_enhanced/connection_spec.rb:50 (SystemObserver.remove_connection rescue nil — AR pool teardown, different pattern)
  • oracle_enhanced/type/timestamp_spec.rb:30 (already has if_exists: true, just trailing rescue nil to drop)
  • oracle_enhanced/procedures_spec.rb:319 ((TestEmployee.partial_updates = false) rescue nil — handled in a separate PR; the rationale is unrelated to fixture cleanup)

Will land as follow-ups so this PR stays focused on the original eight files identified in #2687's description.

Ref: PR #2686 (the original case where rescue nil hid an ORA-00972 identifier-too-long failure on 11g), PR #2687 (the connection_spec.rb half), PR #2688 (the drop_if_exists adapter helper).

🤖 Generated with Claude Code

yahonda and others added 2 commits May 5, 2026 00:01
ActiveRecord's `remove_index` accepts `if_exists: true` so callers can
issue idempotent cleanup without rescuing themselves. The Oracle
Enhanced `remove_context_index` helper had no such option, so spec
cleanup paths used `rescue nil` instead — which silently swallowed
every database error, including real failures the spec should fail
on.

Add an `if_exists:` keyword that forwards to
`drop_if_exists("INDEX", index_name, if_exists: …)` (rescuing only
ORA-01418 "specified index does not exist") and route the auxiliary
trigger and procedure cleanup inside `remove_context_index` through
`drop_if_exists` as well, replacing the inline `rescue nil` those
private helpers had themselves.

The `drop_ctx_preference` PL/SQL call is left as-is — Oracle Text
returns DRG-* codes, not ORA-*, and narrowing it cleanly requires a
separate per-code audit.

The first caller is `context_index_spec.rb`'s `after(:each)` cleanup
hook, which can now drop its `rescue nil` wrappers and call
`remove_context_index :posts, name: "...", if_exists: true`
directly.

Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Continues the cleanup begun in PR rsim#2687, which replaced
`@conn.execute "DROP …" rescue nil` in `connection_spec.rb` with
`drop_table` / `drop_if_exists`. The bare `rescue nil` swallows every
database error — privilege failures, ORA-00972 identifier-too-long
on 11g, syntax errors — and lets specs assert against half-built
fixtures.

This sweep covers six more spec files that used the same pattern:

  oracle_enhanced_data_types_spec.rb
  oracle_enhanced/dbms_metadata_structure_dump_spec.rb
  oracle_enhanced/quoting_spec.rb
  oracle_enhanced/schema_dumper_spec.rb
  oracle_enhanced/schema_statements_spec.rb
  oracle_enhanced/structure_dump_spec.rb

Replacements:

  - DROP TABLE (+ implicit default `<table>_seq`) → `drop_table(name,
    if_exists: true)`. The standard ActiveRecord public API also
    cleans up the default sequence, so the explicit DROP SEQUENCE
    line in `oracle_enhanced_data_types_spec.rb` is redundant and
    drops out.
  - DROP <other-object> (VIEW, MATERIALIZED VIEW, SYNONYM, PACKAGE,
    FUNCTION, PROCEDURE, TYPE, TRIGGER, SEQUENCE) →
    `drop_if_exists("<TYPE>", name)`.
  - `remove_foreign_key … rescue nil` → `remove_foreign_key …,
    if_exists: true` (already supported by AR).
  - `Object.send(:remove_const, "X") rescue nil` →
    `Object.send(:remove_const, "X") if Object.const_defined?("X")`.

`structure_dump_spec.rb`'s `after(:each)` had a long pile of
`ALTER TABLE … DROP CONSTRAINT` and `ALTER TABLE … DROP COLUMN`
lines, also wrapped in `rescue nil`. They were redundant: every
constraint and column they removed was on `test_posts` or `foos`,
both of which the same hook drops a few lines later — the
constraints and columns disappear with their parent table. A few of
the names (`UK_FOOZ_BAZ`, `fooz_id`) were not even created by any
test in the file. Simplified to a flat sequence of
view/table/sequence/trigger/type drops in dependency order.

Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
@yahonda yahonda force-pushed the sweep-rescue-nil-remaining branch 3 times, most recently from 6b1c4ae to 9fedcdd Compare May 4, 2026 15:06
@yahonda yahonda merged commit 93fef95 into rsim:master May 4, 2026
12 checks passed
@yahonda yahonda deleted the sweep-rescue-nil-remaining branch May 4, 2026 15:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant