rails/rails — Issue #3489: ActiveRecord::Connection::SQLiteAdapter#copy_table_indexes incorrectly renames indexes
Author: delwaterman
Created: 2011-11-01 19:57:12+00:00
State: open

While working on some migration issues, I discovered an issue with how the SQLite Adapater is handling the #change_column in migrations. In the code I was working on here is the schema of the table before the migration. The index name is the important detail:

pcp070211pcs:profile delwateo$ sqlite3 db/development.sqlite3
SQLite version 3.6.23.1
Enter ".help" for instructions
Enter SQL statements terminated with a ";"
sqlite> .schema resources
CREATE TABLE "resources" ("id" INTEGER PRIMARY KEY AUTOINCREMENT NOT NULL, "name" varchar(255) NOT NULL, "url" varchar(255) NOT NULL, "content_id" varchar(255) NOT NULL, "created_at" datetime, "updated_at" datetime, "image_url" varchar(255), "section_id" integer, "deleted" boolean DEFAULT 'f');
CREATE INDEX "resources_by_sid_and_cid" ON "resources" ("section_id", "content_id", "deleted");

Then I run the following offending migration:

```ruby
class AddCategorySerializedDataToResource < ActiveRecord::Migration
def self.up
add_column :resources, :serialized_data, :text
add_column :resources, :category, :string
Resource.all.each {|r| r.update_attribute(:category, 'article') }
change_column :resources, :category, :string, :null => false, :default => 'article'
end

def self.down
remove_column :resources, :category
remove_column :resources, :serialized_data
end
end
```

checking the schema after the migration I get:

pcp070211pcs:profile delwateo$ sqlite3 db/development.sqlite3
SQLite version 3.6.23.1
Enter ".help" for instructions
Enter SQL statements terminated with a ";"
sqlite> .schema resources
CREATE TABLE "resources" ("id" INTEGER PRIMARY KEY AUTOINCREMENT NOT NULL, "name" varchar(255) NOT NULL, "url" varchar(255) NOT NULL, "content_id" varchar(255) NOT NULL, "created_at" datetime, "updated_at" datetime, "image_url" varchar(255), "section_id" integer, "deleted" boolean DEFAULT 'f', "serialized_data" text, "category" varchar(255) DEFAULT 'article' NOT NULL);
CREATE INDEX "altered_resources_by_sid_and_cid" ON "resources" ("section_id", "content_id", "deleted");

The index name has been incorrectly changed from 'resources_by_sid_and_cid' to 'altered_resources_by_sid_and_cid'. Which breaks future migrations that modify that index. Looking through the log/development.log file I see the following:

[1m[35mSQL (0.7ms)[0m CREATE TEMPORARY TABLE "altered_resources" ("id" INTEGER PRIMARY KEY AUTOINCREMENT NOT NULL, "name" varchar(255) NOT NULL, "url" varchar(255) NOT NULL, "content_id" varchar(255) NOT NULL, "created_at" datetime, "updated_at" datetime, "image_url" varchar(255), "section_id" integer, "deleted" boolean DEFAULT 'f', "serialized_data" text, "category" varchar(255))
[1m[36mSQL (0.1ms)[0m [1mPRAGMA index_list("resources")[0m
[1m[35mSQL (0.1ms)[0m PRAGMA index_info('resources_by_sid_and_cid')
[1m[36mSQL (0.0ms)[0m [1mPRAGMA index_list("altered_resources")[0m
[1m[35mSQL (0.1ms)[0m CREATE INDEX "temp_altered_resources_by_sid_and_cid" ON "altered_resources" ("section_id", "content_id", "deleted")
[1m[36mSQL (0.6ms)[0m [1mDROP TABLE "resources"[0m
[1m[35mSQL (0.4ms)[0m CREATE TABLE "resources" ("id" INTEGER PRIMARY KEY AUTOINCREMENT NOT NULL, "name" varchar(255) NOT NULL, "url" varchar(255) NOT NULL, "content_id" varchar(255) NOT NULL, "created_at" datetime, "updated_at" datetime, "image_url" varchar(255), "section_id" integer, "deleted" boolean DEFAULT 'f', "serialized_data" text, "category" varchar(255) DEFAULT 'article' NOT NULL)
[1m[36mSQL (0.1ms)[0m [1mPRAGMA index_list("altered_resources")[0m
[1m[35mSQL (0.1ms)[0m PRAGMA index_info('temp_altered_resources_by_sid_and_cid')
[1m[36mSQL (0.1ms)[0m [1mPRAGMA index_list("resources")[0m
[1m[35mSQL (0.3ms)[0m CREATE INDEX "altered_resources_by_sid_and_cid" ON "resources" ("section_id", "content_id", "deleted")
[1m[36mSQL (0.5ms)[0m [1mDROP TABLE "altered_resources"[0m

After doing some hunting around I found the offending code in ActiveRecord::Connection::SQLiteAdapter. When a migration changes the default value of a column in SQLite DB, the migration uses #alter_table, which moves the current table to a temporary table and then moves it back to a normal table with the updated definition. So #alter_table calls #move_table twice which calls #copy_indexes each time its called. Here is the offending function:

```ruby
# file activerecord/lib/active_record/connection_adapters/sqlite_adapter.rb line:339
def copy_table_indexes(from, to, rename = {}) #:nodoc:
indexes(from).each do |index|
name = index.name
if to == "altered_#{from}"
name = "temp_#{name}"
elsif from == "altered_#{to}"
name = name[5..-1]
end

to_column_names = columns(to).map { |c| c.name }
columns = index.columns.map {|c| rename[c] || c }.select do |column|
to_column_names.include?(column)
end

unless columns.empty?
# index name can't be the same
opts = { :name => name.gsub(/_(#{from})_/, "_#{to}_") }
opts[:unique] = true if index.unique
add_index(to, columns, opts)
end
end
end
```
The effect of the first time this is called when from='resources' and to='altered_resources' is that the index 'resources_by_sid_and_cid' is renamed twice within the function. First the line

```ruby
name = "temp_#{name}"
```

renames the index from 'resources_by_sid_and_cid' to 'temp_resources_by_sid_and_cid'. The second time by the line

```ruby
opts = { :name => name.gsub(/_(#{from})_/, "_#{to}_") }
```

renames the index to 'temp_altered_resources_by_sid_to_cid'. This is expected behaviour.

But the second time this function is called (when moving the temp table back to the new definition) the index is not correctly renamed. The index name starts as "temp_altered_resources_by_sid_and_cid". The line

```ruby
name = "temp_#{name}"
```

changes the name to "altered_resources_by_sid_to_cid". But the next line fails
renames it to 'temp_resources_by_sid_and_cid'. The second time by the line

```ruby
opts = { :name => name.gsub(/_(#{from})_/, "_#{to}_") }
```

because its trying to match "\_altered\_resources\_" when it should be trying to match "altered\_resources\_".

But the second rename seems superfulous to me as well. Why is a second rename required? You have already renamed the index "temp_" to guarentee a unique name. My recommendation would be to just change the offending line to

```ruby
opts = { :name => name}
```