Schema dumper and migrations

rubyonrails-core
1

I don’t know if this patch (http://dev.rubyonrails.org/ticket/7085) is worthy of inclusion in 1.2, but I thought I’d highlight here just in case it is.

Basically, it fixes the schema dumper and create_table in migrations to determine the name of the primary key by looking at ActiveRecord::Base.primary_key_prefix_type rather than just using ‘id’. The patch fixes a significant annoyance for people who use this option.

2

+1.

We've hit this issue before...

3

I don't know if this patch (http://dev.rubyonrails.org/ticket/7085) is worthy of inclusion in 1.2, but I thought I'd highlight here just in case it is.

Basically, it fixes the schema dumper and create_table in migrations to determine the name of the primary key by looking at ActiveRecord::Base.primary_key_prefix_type rather than just using 'id'. The patch fixes a significant annoyance for people who use this option.

It seems like the wrong solution, shouldn't it be like

create_table :enterprisies, :id=>:enterprisey_id do |t|

?

4

Koz,

I don't think this patch is about the API. ActiveRecord::Base.primary_key_prefix_type already exists, the patch just makes the schema dumper and migrations respect that setting.

//jarkko

5

I don't think this patch is about the API. ActiveRecord::Base.primary_key_prefix_type already exists, the patch just makes the schema dumper and migrations respect that setting.

Sure, but that's a setting telling active record how to determine the name of the primary key for a given model, not what to call the primary key of a given table when creating it. They're two seperate things, otherwise the api could also respect the pluralization settings.

create_table_for Customer do |t|

6

You're right, my bad.

//jarkko

7

I understand why that approach makes sense, but this does feel like a bug for those of us who use this type of naming convention. For example, with the current SchemaDumper, here's some sample output:

   create_table "track", :id => false, :force => true do |t|      t.column "track_id", :integer, :null => false      t.column "name", :string    end

Notice that the primary key gets clobbered, which causes all kinds of fun when the schema is created from schema.rb. With my patch to both SchemaDumper and SchemaStatements, it just works.

8

I understand why that approach makes sense, but this does feel like a bug for those of us who use this type of naming convention. For example, with the current SchemaDumper, here's some sample output:

Oh definitely, the current behaviour is a bug, I just don't think the right solution is to change the behaviour of create_table to transparently use that setting. Instead it's in two parts:

1) allow create table to take an :id=>:something_id argument 2) Make the schema dumpers recognise the alternate primary keys, and dump that new format.

9

Currently create_table accepts a :primary_key => 'field_name' argument, so the dumper could use that. Should the model generator also be changed to produce a migration in that format?

10

Currently create_table accepts a :primary_key => 'field_name' argument, so the dumper could use that.

Sounds good, I didn't realise that was there already :).

Should the model generator also be changed to produce a migration in that format?

Give it a try and we'll see what the patch looks like. It's a bit of a rathole really, with all sorts of crazy naming standards we could support, I'm pretty sure fixing the dumper to correctly create the primary keys is going to be a happy middle ground.

11

I decided to hold off for now on making changes to the model generator's migration. I'll open another ticket for that if I do it.

I have added a new patch to ticket #7805 that omits the changes to schema_statments.rb. Now the patch will result in the schema being dumped with the :primary_key => 'tablename_id' option to create_table as appropriate.

12

I have added a new patch to ticket #7805 that omits the changes to schema_statments.rb. Now the patch will result in the schema being dumped with the :primary_key => 'tablename_id' option to create_table as appropriate.

This looks good to me, I'll commit once I get a few spare minutes. Thanks again.

13

I have opened a new ticket, http://dev.rubyonrails.org/ticket/7367, which patches the generator to output the correct fixture and migration for non-nil values of ActiveRecord::Base.primary_key_prefix_type.