Skip to content

Default to identity primary keys in Migration[8.2]+ - #2816

Open
yahonda wants to merge 2 commits into
rsim:masterfrom
yahonda:v4-default-identity-primary-key
Open

Default to identity primary keys in Migration[8.2]+#2816
yahonda wants to merge 2 commits into
rsim:masterfrom
yahonda:v4-default-identity-primary-key

Conversation

@yahonda

@yahonda yahonda commented Jun 28, 2026

Copy link
Copy Markdown
Collaborator

Adopt the standalone ActiveRecord::Migration::CompatibilityBehavior extension point (rails/rails PR-pending, currently tracked on yahonda/rails branch per-adapter-migration-compatibility-v7) to default new-enough migrations to Oracle identity primary keys. Supersedes #2813 and #2596.

Behavior

A migration declaring 8.2 now creates an identity primary key on Oracle Database 12.1 or higher, with no change to the migration code:

class CreatePosts < ActiveRecord::Migration[8.2]
  def change
    create_table :posts do |t|   # id: NUMBER GENERATED BY DEFAULT AS IDENTITY
      t.string :title
    end
  end
end

No posts_seq sequence is created, and inserts no longer query posts_seq.NEXTVAL — the database assigns the id.

The same table under Migration[8.1] or earlier keeps the sequence-backed primary key, so every existing migration replays unchanged:

class CreatePosts < ActiveRecord::Migration[8.1]
  def change
    create_table :posts do |t|   # id: NUMBER + posts_seq sequence, as today
      t.string :title
    end
  end
end

An explicit identity: always wins over the version default, in both directions:

class CreatePosts < ActiveRecord::Migration[8.1]
  def change
    create_table :posts, identity: true do |t|   # identity, even on 8.1
      t.string :title
    end
  end
end

class CreateAudits < ActiveRecord::Migration[8.2]
  def change
    create_table :audits, identity: false do |t| # sequence, even on 8.2
      t.string :action
    end
  end
end

Resulting primary key by caller and option (Oracle Database 12.1+)

caller (no identity:) identity: true identity: false
Migration[8.2] and later identity (the only cell this PR changes) identity sequence
Migration[8.1] and earlier sequence identity sequence
Schema.define / ActiveRecord::Schema[x.y].define / direct connection.create_table sequence identity sequence

On Oracle Database releases before 12.1 every cell resolves to a sequence-backed key, except identity: true, which raises ArgumentError — both as today. A Migration[8.2]+ create_table passing a sequence-backed option (sequence_name:, sequence_start_value:, primary_key_trigger:) behaves like the middle row: the option opts it out of the identity default instead of raising about an identity: true the author never wrote.

Schema dumps written by older releases reload unchanged (schema loads keep the sequence default), and identity-backed tables are dumped with identity: true, so dump -> load round-trips preserve both kinds.

Implementation

  • Fills in the V8_2/V8_1 behaviors that Add CompatibilityBehavior plumbing for per-adapter migration compatibility #2823 introduces as empty skeletons:
    • V8_2#create_table forces options[:identity] = true when no :identity is given, no sequence options are set, the connection supports identity columns, and the caller is not a schema load (checked via ActiveRecord::Schema::Definition, which both plain Schema.define and versioned ActiveRecord::Schema[x.y].define include), so schema dumps keep the sequence default.
    • V8_1#create_table forces options[:identity] = false to keep the pre-8.2 sequence default on older migrations.
  • rename_table and drop_table become identity-aware: they skip the paired <table>_seq rename/drop when the table's primary key is an Oracle identity column, so unrelated application-owned sequences with matching names are left alone.
  • The schema dumper emits identity: true for identity-backed primary keys. Sequence-backed primary keys carry no annotation: identity: false is the adapter's baseline default (Schema.define, direct connection.create_table, and Migration[8.1] and earlier all keep the sequence default), so emitting it on every sequence-backed table would be redundant for reload and add schema.rb churn.

Tests

identity_primary_key_spec.rb (round-trip + identity-aware rename/drop; the previously always-skipped “identity counterparts” block now runs) and migration_compatibility_spec.rb covering the version matrix: Migration[8.2] / Migration[8.1] / Migration[7.0], plain Schema.define and versioned ActiveRecord::Schema[8.2].define loads, explicit identity: overrides, and the sequence-option / primary_key_trigger: opt-outs. CI runs the suite against Oracle.

Depends on

Independent of #2817: 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 (2026-07-02)

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

  • Versioned schema loads kept the sequence default only for the unversioned form. rake db:schema:dump writes ActiveRecord::Schema[8.2].define, whose anonymous class is not a subclass of ActiveRecord::Schema, so the is_a?(ActiveRecord::Schema) guard missed it and db:schema:load would have recreated every sequence-backed table as identity (breaking dump -> load -> dump idempotence). The guard now checks ActiveRecord::Schema::Definition, with a spec loading through ActiveRecord::Schema[8.2].define.
  • primary_key_trigger: true now opts out of the identity default. Previously a Migration[8.2] create_table :t, primary_key_trigger: true raised about combining it with an identity: true the author never wrote.
  • The "identity counterparts" spec block was always skipped. Its guard read @oracle12c_or_higher, which is never assigned in that file; it now uses the database_version comparison. The unskipped NEXTVAL assertion was tightened to /\.NEXTVAL/ so it does not match the has_primary_key_trigger? lookup SQL, which contains the literal NEXTVAL INTO :NEW.
  • Spec/comment cleanups: non-deprecated database_version comparison in a before(:all) hook, removal of an unused table cleanup.

@yahonda
yahonda force-pushed the v4-default-identity-primary-key branch 2 times, most recently from ff1342d to bdba349 Compare June 29, 2026 02:48
@yahonda
yahonda force-pushed the v4-default-identity-primary-key branch 3 times, most recently from 681aa59 to ecc98e9 Compare July 2, 2026 05:16
@yahonda yahonda changed the title v4: Auto-enable identity primary keys in Migration[8.2]+ v5: Auto-enable identity primary keys in Migration[8.2]+ Jul 2, 2026
@yahonda
yahonda force-pushed the v4-default-identity-primary-key branch from ecc98e9 to 1122ee2 Compare July 2, 2026 14:48
@yahonda yahonda changed the title v5: Auto-enable identity primary keys in Migration[8.2]+ v7: Auto-enable identity primary keys in Migration[8.2]+ Jul 2, 2026
@yahonda
yahonda force-pushed the v4-default-identity-primary-key branch from 1122ee2 to 245c0ea Compare July 9, 2026 06:22
@yahonda yahonda changed the title v7: Auto-enable identity primary keys in Migration[8.2]+ Auto-enable identity primary keys in Migration[8.2]+ Jul 9, 2026
@yahonda yahonda changed the title Auto-enable identity primary keys in Migration[8.2]+ Default to identity primary keys in Migration[8.2]+ Jul 10, 2026
@yahonda
yahonda force-pushed the v4-default-identity-primary-key branch 2 times, most recently from eb1a843 to 8b4f942 Compare July 13, 2026 00:09
yahonda added a commit to yahonda/oracle-enhanced that referenced this pull request Jul 13, 2026
…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.

yahonda and others added 2 commits August 8, 2026 00:30
…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>
Adopt identity primary keys as the default for `Migration[8.2]` and
later when the server supports them (Oracle Database 12.1+); preserve
sequence-backed primary keys for `Migration[8.1]` and earlier, for
schema loads, and for direct `connection.create_table` calls so
existing schemas and dumps replay unchanged.

Fills in the `V8_2`/`V8_1` behaviors introduced empty by the
CompatibilityBehavior plumbing commit:

* `V8_2` forces `options[:identity] = true` when the option is
  omitted, no sequence options exist, the connection supports
  identity columns, and the caller is not a schema load — detected
  via `ActiveRecord::Schema::Definition`, which plain `Schema.define`
  and versioned `ActiveRecord::Schema[x.y].define` (the form schema
  dumps actually use; its anonymous class is not a subclass of
  `ActiveRecord::Schema`) both include — so dump -> load -> dump
  stays idempotent.
* `primary_key_trigger: true` (an inherently sequence-backed
  mechanism) also opts out, like `sequence_name:` and
  `sequence_start_value:`, instead of raising about an
  `identity: true` the migration author never wrote.
* `V8_1` forces `options[:identity] = false` to keep the pre-8.2
  sequence default on older migrations.
* `rename_table` and `drop_table` become identity-aware: they skip the
  paired `<table>_seq` rename/drop when the table's primary key is an
  Oracle identity column, so unrelated application-owned sequences with
  matching names are left untouched.
* The schema dumper emits `identity: true` for identity-backed primary
  keys. Sequence-backed primary keys carry no annotation: `identity:
  false` is the adapter's baseline default (schema loads, direct
  `connection.create_table`, and Migration[8.1] and earlier all keep the
  sequence default), so emitting it on every sequence-backed table would
  be redundant for reload and only add schema.rb churn.
* The "identity counterparts" spec block now actually runs (its guard
  read an instance variable never assigned in that file) and its
  NEXTVAL assertion matches sequence fetches (`.NEXTVAL`) rather than
  the has_primary_key_trigger? lookup SQL.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
@yahonda
yahonda force-pushed the v4-default-identity-primary-key branch from 3f8d442 to a579261 Compare August 7, 2026 15:30
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