Rename default sequence when table is renamed? [AR:postgres] #6874

Merged
merged 1 commit into from Jul 10, 2012
Jump to file or symbol
Failed to load files and symbols.
+20 −0
Split
@@ -1208,12 +1208,19 @@ def primary_key(table)
end
# Renames a table.
+ # Also renames a table's primary key sequence if the sequence name matches the
+ # Active Record default.
#
# Example:
# rename_table('octopuses', 'octopi')
def rename_table(name, new_name)
clear_cache!
execute "ALTER TABLE #{quote_table_name(name)} RENAME TO #{quote_table_name(new_name)}"
+ pk, seq = pk_and_sequence_for(new_name)
+ if seq == "#{name}_#{pk}_seq"
@tenderlove

tenderlove Jul 9, 2012

Owner

This test is really confusing to me. Does it actually work? Wouldn't seq contain new_name, not name?

@robbkidd

robbkidd Jul 9, 2012

Contributor

Nope. That's the whole point of this pull request. ALTER TABLE ... RENAME TO performed on a table in postgres does not rename any sequences that happen to be associated with that table.

For example, in the tests, the table test_models gets renamed to octopi. After the table rename, the table's ID sequencer is still named test_models_id_seq. Hence the need in postgres to rename the sequence as well.

@robbkidd

robbkidd Jul 9, 2012

Contributor

More followup, if I remove this logic from the rename_table method, but keep the new test checking that the sequence matches the new name (test-driven!), I see that the sequence for the octopi table is still named test_models_id_seq.

  1) Failure:
test_rename_table_for_postgresql_should_also_rename_default_sequence(ActiveRecord::Migration::RenameTableTest) [test/cases/migration/rename_table_test.rb:79]:
Expected: "octopi_id_seq"
  Actual: "test_models_id_seq"

State left in the database:

» psql activerecord_unittest
psql (9.1.4)
Type "help" for help.

activerecord_unittest=# \d octopi 
                                   Table "public.octopi"
 Column |          Type          |                        Modifiers                         
--------+------------------------+----------------------------------------------------------
 id     | integer                | not null default nextval('test_models_id_seq'::regclass)
 url    | character varying(255) | 
Indexes:
    "test_models_pkey" PRIMARY KEY, btree (id)

activerecord_unittest=# 
@tenderlove

tenderlove Jul 10, 2012

Owner

Ah, I got it. Thanks. Sorry, I've got a cold so my brain is running on half power. :(

+ new_seq = "#{new_name}_#{pk}_seq"
+ execute "ALTER TABLE #{quote_table_name(seq)} RENAME TO #{quote_table_name(new_seq)}"
+ end
end
# Adds a new column to the named table.
@@ -67,6 +67,19 @@ def test_rename_table_with_an_index
rename_table :octopi, :test_models
end
+
+ def test_rename_table_for_postgresql_should_also_rename_default_sequence
+ skip 'not supported' unless current_adapter?(:PostgreSQLAdapter)
+
+ rename_table :test_models, :octopi
+
+ con = ActiveRecord::Base.connection
@rafaelfranca

rafaelfranca Jun 27, 2012

Owner

I would put this is a begin/ensure block.

begin
  con = ActiveRecord::Base.connection
  pk, seq = con.pk_and_sequence_for('octopi')
  assert_equal 'octopi_id_seq', seq
ensure
  rename_table :octopi, :test_models
end

Some command can fail and we will have an inconsistent database.

@robbkidd

robbkidd Jun 27, 2012

Contributor

Certainly do not want an inconsistent database. I was following the pattern of the other tests in this file. I am happy to add an ensure to the new test method.

Possibly a DRYer solution for these tests would be a teardown method:

def teardown
  rename_table :octopi, :test_models if connection.table_exists? :octopi
  super
end

I also notice that the use of the ActiveRecord::Base.connection object is handled differently in each of the pre-existing tests. Should I not just use connection, the attr_reader defined in ActiveRecord::Migration::TestHelper?

I've got another branch with these two changes (add teardown method and just use connection) ready and tested.

@rafaelfranca

rafaelfranca Jun 27, 2012

Owner

Cool. Let's wait to merge that commits in another pull request after merge this one.

+ pk, seq = con.pk_and_sequence_for('octopi')
+
+ assert_equal "octopi_#{pk}_seq", seq
+
+ rename_table :octopi, :test_models
+ end
end
end
end