Skip to content

Stop creating a unique constraint for add_index unique: true in Migration[8.2]+ - #2817

Open
yahonda wants to merge 2 commits into
rsim:masterfrom
yahonda:v4-deprecate-implicit-unique-constraint-phase2
Open

Stop creating a unique constraint for add_index unique: true in Migration[8.2]+#2817
yahonda wants to merge 2 commits into
rsim:masterfrom
yahonda:v4-deprecate-implicit-unique-constraint-phase2

Conversation

@yahonda

@yahonda yahonda commented Jun 28, 2026

Copy link
Copy Markdown
Collaborator

Phase 2 of the implicit-UNIQUE-CONSTRAINT deprecation (#2702), built on the Migration::CompatibilityBehavior extension point. Supersedes #2814 and #2710.

Behavior

Migration[8.2] and later treat add_index :col, unique: true as "create only the unique index" — matching Rails-core PostgreSQL/MySQL/SQLite. Migration[8.1] and earlier keep the pre-8.2 behavior of also creating a same-named UNIQUE CONSTRAINT, so existing migrations replay unchanged. Callers that need a constraint on Migration[8.2]+ should call add_unique_constraint :t, :col, name: :n directly.

Implementation

  • OracleEnhanced::CompatibilityBehavior::V8_1 sets an internal _implicit_unique_constraint: true flag in create_table, add_index, create_join_table, and — inside the reference's index options — add_reference/add_belongs_to. The adapter consumes and deletes the flag before further processing (including remove_index/drop_table, which receive it through CommandRecorder inversions on migrate :down), so it never reaches DDL emission (an options flag rather than a thread-local: migrations are single-threaded).
  • New implicit_unique_constraint_active? adapter helper ORs the global add_index_unique_creates_constraint flag with the per-call _implicit_unique_constraint flag.
  • OracleEnhancedAdapter.add_index_unique_creates_constraint default flips from true to false. The flag remains as an explicit opt-in for projects that want the pre-8.2 behavior project-wide.
  • Specs that exercise the pre-8.2 path use with_implicit_unique_constraint_enabled in spec_helper.rb, which toggles the global flag around a block.
  • migration_compatibility_spec.rb gains a second top-level describe block covering add_index, inline t.index in create_table/change_table/create_join_table, add_reference/t.references with unique indexes, and a migrate :down cycle — each under both Migration[8.2] (no implicit constraint, no warning) and Migration[8.1] (implicit constraint + deprecation warning) — plus a Migration[7.0] case and an explicit-flag override case.

Tests

migration_compatibility_spec.rb (behavior resolution + unique-constraint version matrix), schema_dumper_spec.rb, schema_statements_spec.rb, structure_dump_spec.rb, and dbms_metadata_structure_dump_spec.rb; CI runs the suite against Oracle.

Depends on

Independent of #2816: after #2823, either PR can merge first; whichever merges second needs a trivial rebase (both touch V8_1#create_table and append to migration_compatibility_spec.rb).

Review follow-up: change_table inline index coverage

A fresh review found that change_table { |t| t.index :col, unique: true } dispatches through Table#index, which called the connection's add_index directly and bypassed CompatibilityBehavior::V8_1#add_index. Under Migration[8.1] this dropped the implicit UNIQUE constraint for the change_table path only, so older migrations using inline unique indexes inside change_table would not replay unchanged.

The fix keeps this adapter-specific routing in the adapter: V8_1 carries a nested TableDefinition module that the yahonda/rails per-adapter-migration-compatibility-v7 branch's compatible_table_definition prepends generically onto the table object yielded by create_table/change_table. The framework stays free of any index-specific or adapter-only routing (it carries no per-adapter accessor either). The module injects the per-call flag only on the immediate-execution change_table receiver — the create_table path is covered by the table-level flag, and Rails validates per-index option keys during table creation. Any other t.<method> can be customized the same way if future version compatibility requires it.

This PR adds the matching coverage in migration_compatibility_spec.rb: change_table { t.index unique: true } under Migration[8.2] (index only, no warning) and Migration[8.1] (implicit constraint + deprecation warning), plus a Migration[7.0] add_index case confirming pre-8.1 versions resolve to the V8_1 behavior and keep the implicit constraint. The Migration[8.1] change_table case exercises the framework's compatible_table_definition prepending the V8_1::TableDefinition module.

Review follow-up (2026-07-02, second pass)

A second review pass added the following, since folded into the single commit:

  • add_reference index: { unique: true } (and t.references inside change_table) was not version-aware. The reference path builds a fresh OracleEnhanced::Table that never went through compatible_table_definition, so under Migration[8.1] the implicit constraint was silently dropped — no deprecation warning either — and a later add_foreign_key targeting that column would fail with ORA-02270. V8_1#add_reference (+ add_belongs_to) now injects the flag into the reference's index options, and the V8_1::TableDefinition module covers t.references/t.belongs_to inside change_table like t.index. create_join_table (which calls the connection's create_table internally, bypassing V8_1#create_table) is covered as well. Specs cover these paths under 8.2 and 8.1.
  • The deprecation message was rewritten for Phase 2. After the flip the global flag already defaults to false, so the old "set the flag to false" advice silenced nothing in the main path that still warns (the V8_1 per-call flag). The message now states the trigger — the migration's declared version, or the global flag — and leads with the version-based remediation, keeping the add_unique_constraint guidance.
  • migrate :down flag hygiene + a vacuous spec. CommandRecorder straight reversions replayed remove_index/drop_table with _implicit_unique_constraint still set; both methods now delete it at entry, with a spec pinning the up/down cycle. The dbms_metadata structure-dump round-trip spec had also become vacuous after the flip (no backing constraint was created any more); it now uses with_implicit_unique_constraint_enabled like its sibling.

@yahonda
yahonda force-pushed the v4-deprecate-implicit-unique-constraint-phase2 branch 2 times, most recently from 3d6ee36 to d63fb76 Compare June 29, 2026 02:48
@yahonda
yahonda force-pushed the v4-deprecate-implicit-unique-constraint-phase2 branch 7 times, most recently from dcda72a to 2ba14f0 Compare July 2, 2026 05:17
@yahonda yahonda changed the title v4: Phase 2 — flip add_index unique: true default in Migration[8.2]+ v5: Phase 2 — flip add_index unique: true default in Migration[8.2]+ Jul 2, 2026
@yahonda
yahonda force-pushed the v4-deprecate-implicit-unique-constraint-phase2 branch from 2ba14f0 to 6ab74c6 Compare July 2, 2026 14:50
@yahonda yahonda changed the title v5: Phase 2 — flip add_index unique: true default in Migration[8.2]+ v7: Phase 2 — flip add_index unique: true default in Migration[8.2]+ Jul 2, 2026
@yahonda
yahonda force-pushed the v4-deprecate-implicit-unique-constraint-phase2 branch 2 times, most recently from cda79f2 to 0ee9ba4 Compare July 9, 2026 06:22
@yahonda yahonda changed the title v7: Phase 2 — flip add_index unique: true default in Migration[8.2]+ Phase 2: flip add_index unique: true default in Migration[8.2]+ Jul 9, 2026
@yahonda yahonda changed the title Phase 2: flip add_index unique: true default in Migration[8.2]+ Stop creating a unique constraint for add_index unique: true in Migration[8.2]+ Jul 10, 2026
@yahonda
yahonda force-pushed the v4-deprecate-implicit-unique-constraint-phase2 branch from 0ee9ba4 to d8ba80f Compare July 10, 2026 06:51
yahonda and others added 2 commits July 13, 2026 09:00
…ility

Wire the adapter into Rails' per-adapter migration compatibility hook so
version-declared migrations (`ActiveRecord::Migration[x.y]`) can get
Oracle-specific per-version behavior:

* New `OracleEnhanced::CompatibilityBehavior` (`extend Base::Resolver`)
  is returned by the adapter's `compatibility_behavior_for(migration_class)`
  override. The Resolver maps a migration's declared version to the
  lowest defined behavior version covering it: `Migration[8.2]` and later
  resolve to `V8_2`, `Migration[8.1]` and earlier to `V8_1`, and an
  unversioned migration to the framework's no-op base.
* `V8_2` and `V8_1` are empty here — this commit is plumbing only and
  changes no behavior. The identity primary key default (rsim#2816) and the
  implicit-UNIQUE-CONSTRAINT Phase 2 flip (rsim#2817) each fill in their
  version-specific adjustments on top of this commit.
* Gemfile points activerecord at yahonda/rails
  per-adapter-migration-compatibility-v7 for the framework dispatch.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…tion[8.2]+

Phase 2 of the implicit-UNIQUE-CONSTRAINT deprecation (rsim#2702). For
`Migration[8.2]` and later, `add_index :col, unique: true` now creates
only the unique index (matching Rails-core PostgreSQL/MySQL/SQLite).
`Migration[8.1]` and earlier keep the pre-8.2 behavior of also creating
a same-named UNIQUE CONSTRAINT, so existing migrations replay
unchanged. Callers that need a constraint on Migration[8.2]+ should
call `add_unique_constraint :t, :col, name: :n` directly.

* `OracleEnhanced::CompatibilityBehavior::V8_1` sets an internal
  `_implicit_unique_constraint: true` flag in `create_table`,
  `add_index`, `create_join_table` (whose core implementation calls the
  connection's `create_table` directly, bypassing `V8_1#create_table`),
  and — inside the reference's index options — `add_reference` /
  `add_belongs_to`, so every path that can create a unique index under
  a pre-8.2 migration keeps the implicit constraint. The adapter
  consumes and deletes the flag before further processing (including
  `remove_index`/`drop_table`, which receive it through CommandRecorder
  inversions on `migrate :down`), so it never reaches DDL emission.
* New `implicit_unique_constraint_active?` helper in the adapter ORs
  the global `add_index_unique_creates_constraint` flag with the
  per-call `_implicit_unique_constraint` flag, keeping the single
  decision point readable.
* `OracleEnhancedAdapter.add_index_unique_creates_constraint` default
  flips from `true` to `false`. The flag still exists as an explicit
  opt-in for projects that want the implicit-constraint behavior
  project-wide. The deprecation message states what triggers the
  warning (the migration's declared version, or the global flag) and
  leads with the version-based remediation.
* Inline `t.index unique: true` and `t.references index: { unique:
  true }` inside `change_table` reach the per-version behavior too:
  `V8_1` carries a nested `TableDefinition` module that the framework's
  `compatible_table_definition` prepends onto the change_table receiver
  (yahonda/rails `per-adapter-migration-compatibility-v7`). The module
  injects the flag only on that immediate-execution receiver — the
  create_table path is covered by the table-level flag, and Rails
  validates per-index option keys while creating the table.
  Migration[8.1] keeps the implicit constraint on those paths while
  Migration[8.2]+ creates the index only; any other `t.<method>` can be
  customized the same way if future version compatibility requires it.
* Specs that exercise the pre-Phase 2 path use the new
  `with_implicit_unique_constraint_enabled` helper in `spec_helper.rb`,
  which toggles the global flag around a block. The helper is loaded
  globally and can be removed in Phase 3 alongside the flag. The
  dbms_metadata round-trip spec uses it so the multi-statement GET_DDL
  CLOB shape it documents still materializes after the flip.
* `migration_compatibility_spec.rb` grows a second top-level describe
  block covering `add_index unique: true`, inline `t.index` in
  `create_table`/`change_table`/`create_join_table`, `add_reference`
  and `t.references` with a unique index, and a `migrate :down` cycle
  under both Migration[8.2] (no implicit constraint, no warning) and
  Migration[8.1] (implicit constraint, deprecation warning), plus a
  Migration[7.0] case and an explicit-flag override case.

Depends only on the CompatibilityBehavior plumbing commit, which
introduces the empty V8_2/V8_1 chain this commit fills in; it is
independent of the Migration[8.2]+ identity primary key default
(rsim#2816), so either can merge first.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
@yahonda

yahonda commented Jul 20, 2026

Copy link
Copy Markdown
Collaborator Author

Builds on #2823 (its commit is included here), which in turn depends on rails/rails#58162 — the Active Record change that adds the compatibility_behavior_for extension point. CI here runs against that rails branch until #58162 merges.

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