Skip to content

Commit eb1a843

Browse files
yahondaclaude
andcommitted
Default to identity primary keys in Migration[8.2]+
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. * New `OracleEnhanced::CompatibilityBehavior` (`extend Base::Resolver`) routes the per-version behavior via Rails' `compatibility_behavior_for(migration_class)` hook: * `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. * Gemfile points activerecord at yahonda/rails per-adapter-migration-compatibility-v7 for the framework dispatch. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
1 parent 5f6d3df commit eb1a843

5 files changed

Lines changed: 409 additions & 5 deletions

File tree

Gemfile

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -8,7 +8,7 @@ group :development do
88
gem "rspec"
99
gem "rdoc"
1010
gem "rake"
11-
gem "activerecord", github: "rails/rails", branch: "main"
11+
gem "activerecord", github: "yahonda/rails", branch: "per-adapter-migration-compatibility-v7"
1212
gem "ruby-plsql", github: "rsim/ruby-plsql", branch: "master"
1313

1414
platforms :ruby do
Lines changed: 46 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,46 @@
1+
# frozen_string_literal: true
2+
3+
module ActiveRecord
4+
module ConnectionAdapters
5+
module OracleEnhanced
6+
module CompatibilityBehavior # :nodoc: all
7+
Base = ActiveRecord::Migration::CompatibilityBehavior
8+
extend Base::Resolver
9+
10+
# Migration[8.2]+ migrations default to an Oracle identity primary
11+
# key when the server supports it. Schema loads (plain and versioned
12+
# `ActiveRecord::Schema[x.y].define`, both including
13+
# +ActiveRecord::Schema::Definition+) and direct adapter calls keep
14+
# the adapter's pre-existing sequence default so that schema dumps
15+
# written by older releases reload unchanged. An explicit
16+
# `identity:` value always wins.
17+
class V8_2 < Base
18+
def create_table(table_name, **options)
19+
options[:identity] = true if default_to_identity?(options)
20+
super
21+
end
22+
23+
private
24+
def default_to_identity?(options)
25+
return false if migration.is_a?(ActiveRecord::Schema::Definition)
26+
return false if options.key?(:identity)
27+
return false if options[:sequence_name] || options[:sequence_start_value]
28+
return false if options[:primary_key_trigger]
29+
return false if options.fetch(:id, :primary_key) != :primary_key
30+
return false if options[:primary_key].is_a?(Array)
31+
connection.supports_identity_columns?
32+
end
33+
end
34+
35+
# Migration[8.1] and earlier keep the pre-8.2 sequence-backed primary
36+
# key default so existing migrations replay unchanged.
37+
class V8_1 < V8_2
38+
def create_table(table_name, **options)
39+
options[:identity] = false unless options.key?(:identity)
40+
super
41+
end
42+
end
43+
end
44+
end
45+
end
46+
end

lib/active_record/connection_adapters/oracle_enhanced/schema_statements.rb

Lines changed: 26 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,7 @@
11
# frozen_string_literal: true
22

33
require "openssl"
4+
require "active_record/connection_adapters/oracle_enhanced/compatibility_behavior"
45

56
module ActiveRecord
67
module ConnectionAdapters
@@ -10,6 +11,10 @@ module SchemaStatements
1011
#
1112
# see: abstract/schema_statements.rb
1213

14+
def compatibility_behavior_for(migration_class) # :nodoc:
15+
OracleEnhanced::CompatibilityBehavior.for(migration_class)
16+
end
17+
1318
def tables # :nodoc:
1419
select_values(<<~SQL.squish, "SCHEMA")
1520
SELECT
@@ -263,8 +268,11 @@ def rename_table(table_name, new_name, **options) # :nodoc:
263268
end
264269
schema_cache.clear_data_source_cache!(table_name.to_s)
265270
schema_cache.clear_data_source_cache!(new_name.to_s)
271+
identity_pk = identity_pk?(table_name)
266272
execute "RENAME #{quote_table_name(table_name)} TO #{quote_table_name(new_name)}"
267-
execute "RENAME #{default_sequence_name(table_name, nil)} TO #{default_sequence_name(new_name, nil)}" rescue nil
273+
unless identity_pk
274+
execute "RENAME #{default_sequence_name(table_name, nil)} TO #{default_sequence_name(new_name, nil)}" rescue nil
275+
end
268276
rename_pk_trigger(table_name, new_name)
269277
clear_table_caches(table_name)
270278
clear_table_caches(new_name)
@@ -281,10 +289,13 @@ def drop_table(*table_names, **options) # :nodoc:
281289

282290
table_names.each do |table_name|
283291
schema_cache.clear_data_source_cache!(table_name.to_s)
284-
seq_name = custom_sequence_name || default_sequence_name(table_name, nil)
292+
# Identity-backed tables never carry a default `<table>_seq`, so
293+
# skip the sequence drop entirely on those. `identity_pk?` must
294+
# run before the table drop, while the table is still present.
295+
seq_name = custom_sequence_name || (identity_pk?(table_name) ? nil : default_sequence_name(table_name, nil))
285296

286297
drop_if_exists("TABLE", table_name, cascade_constraints: cascade, if_exists: if_exists)
287-
drop_if_exists("SEQUENCE", seq_name, if_exists: true)
298+
drop_if_exists("SEQUENCE", seq_name, if_exists: true) if seq_name
288299
ensure
289300
clear_table_caches(table_name)
290301
end
@@ -1231,6 +1242,18 @@ def missing_object_ora_code?(message, object_type)
12311242
code && message.include?(code)
12321243
end
12331244

1245+
# True when +table_name+ exists and its primary key column is an Oracle
1246+
# identity column. Returns false when the table is not present (e.g.
1247+
# +drop_table if_exists:+ for a missing table) or when the database does
1248+
# not support identity columns.
1249+
def identity_pk?(table_name)
1250+
return false unless supports_identity_columns?
1251+
owner, desc_table_name = resolve_data_source_name(table_name)
1252+
identity_primary_key?(owner, desc_table_name)
1253+
rescue OracleEnhanced::ConnectionException
1254+
false
1255+
end
1256+
12341257
def index_column_names(column_names) # :nodoc:
12351258
column_names.is_a?(Array) ? column_names : Array(column_names)
12361259
end

spec/active_record/connection_adapters/oracle_enhanced/identity_primary_key_spec.rb

Lines changed: 111 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -336,7 +336,7 @@ def identity_column_exists?(table_name, column_name)
336336
ActiveRecord::SchemaDumper.ignore_tables = []
337337

338338
expect(stream.string).to include('create_table "test_identity_pks"')
339-
expect(stream.string).not_to include("identity: true")
339+
expect(stream.string).not_to include("identity:")
340340
end
341341
end
342342

@@ -352,4 +352,114 @@ def identity_column_exists?(table_name, column_name)
352352
}.to raise_error(NotImplementedError)
353353
end
354354
end
355+
356+
describe "identity counterparts to sequence-prerequisite behavior" do
357+
before do
358+
skip "requires Oracle 12.1+" unless @conn.database_version >= "12"
359+
end
360+
361+
it "renames a long-named identity table without touching a non-existent sequence" do
362+
long_source = "a" * (@conn.max_identifier_length - 3)
363+
schema_define do
364+
create_table long_source.to_sym, force: true, identity: true do |t|
365+
t.string :name
366+
end
367+
end
368+
369+
expected_old_seq = @conn.default_sequence_name(long_source, nil).upcase
370+
expected_new_seq = @conn.default_sequence_name("renamed_identity_long", nil).upcase
371+
372+
expect {
373+
@conn.rename_table(long_source, "renamed_identity_long")
374+
}.not_to raise_error
375+
376+
sequences = @conn.select_values(
377+
"SELECT sequence_name FROM user_sequences WHERE sequence_name IN ('#{expected_old_seq}', '#{expected_new_seq}')"
378+
)
379+
expect(sequences).to be_empty
380+
ensure
381+
schema_define do
382+
drop_table :renamed_identity_long, if_exists: true
383+
drop_table long_source.to_sym, if_exists: true
384+
end
385+
end
386+
387+
it "returns a nil sequence_name from pk_and_sequence_for for identity tables" do
388+
schema_define do
389+
create_table :test_identity_pks, identity: true do |t|
390+
t.string :name
391+
end
392+
end
393+
394+
expect(@conn.pk_and_sequence_for(:test_identity_pks)).to eq(["id", nil])
395+
end
396+
397+
it "does not query NEXTVAL when inserting into an identity table" do
398+
schema_define do
399+
create_table :test_identity_pks, identity: true do |t|
400+
t.string :name
401+
end
402+
end
403+
klass = Class.new(ActiveRecord::Base) { self.table_name = "test_identity_pks" }
404+
405+
set_logger
406+
begin
407+
klass.create!(name: "alpha")
408+
klass.create!(name: "bravo")
409+
# Match sequence fetches (`"<name>_SEQ".NEXTVAL`) but not the
410+
# has_primary_key_trigger? lookup, whose SQL text contains the
411+
# literal 'NEXTVAL INTO :NEW'.
412+
expect(@logger.output(:debug)).not_to match(/\.NEXTVAL/i)
413+
ensure
414+
clear_logger
415+
end
416+
end
417+
418+
it "omits DROP SEQUENCE from full_drop output for identity-backed tables" do
419+
schema_define do
420+
create_table :test_identity_pks, identity: true do |t|
421+
t.string :name
422+
end
423+
end
424+
425+
drop = @conn.full_drop
426+
427+
expect(drop).to match(/DROP TABLE "TEST_IDENTITY_PKS" CASCADE CONSTRAINTS/)
428+
expect(drop).not_to match(/DROP SEQUENCE "TEST_IDENTITY_PKS_SEQ"/)
429+
end
430+
431+
it "rename_table leaves an unrelated <table>_seq sequence alone for identity tables" do
432+
schema_define do
433+
create_table :test_identity_pks, identity: true do |t|
434+
t.string :name
435+
end
436+
end
437+
@conn.execute "CREATE SEQUENCE test_identity_pks_seq"
438+
439+
@conn.rename_table(:test_identity_pks, :test_identity_pks_renamed)
440+
441+
expect(sequence_exists?(:test_identity_pks_seq)).to be true
442+
expect(sequence_exists?(:test_identity_pks_renamed_seq)).to be false
443+
ensure
444+
@conn.execute("DROP SEQUENCE test_identity_pks_seq") rescue nil
445+
schema_define do
446+
drop_table :test_identity_pks_renamed, if_exists: true
447+
end
448+
end
449+
450+
it "drop_table leaves an unrelated <table>_seq sequence alone for identity tables" do
451+
schema_define do
452+
create_table :test_identity_pks, identity: true do |t|
453+
t.string :name
454+
end
455+
end
456+
@conn.execute "CREATE SEQUENCE test_identity_pks_seq"
457+
458+
@conn.drop_table(:test_identity_pks)
459+
460+
expect(sequence_exists?(:test_identity_pks_seq)).to be true
461+
ensure
462+
@conn.execute("DROP SEQUENCE test_identity_pks_seq") rescue nil
463+
end
464+
end
355465
end

0 commit comments

Comments
 (0)