datamapper / dm-migrations

DataMapper plugin for writing and speccing migrations
http://datamapper.org/
MIT License
66 stars 42 forks source link

auto_upgrade! within a Postgres migration creates bad create table sql #30

Open austinmoore opened 13 years ago

austinmoore commented 13 years ago

How to reproduce...

Give a Postgres database in a Sinatra app using DataMapper and auto_upgrade! is not called when the application starts up.

Given the following new Model:

class MyModel
    include DataMapper::Resource

    property :id, Serial
    property :name, String
end

The following call to DataMapper... MyModel!.auto_upgrade!

will execute the following, correct sql against the Postgres database. CREATE TABLE "my_models" ("id" SERIAL NOT NULL, "name" VARCHAR(50), PRIMARY KEY("id"))

However, if you create a migration and call auto_upgrade from the migration like the following...

migration '20110908-103200', :create_my_model do
    up do
               MyModel.auto_upgrade!
    end

    down do
        end
end

then the following, incorrect SQL is executed against the Postgres database...

CREATE TABLE "my_models" ( SERIAL PRIMARY KEY, "name" VARCHAR(50), PRIMARY KEY("id"))

which produces the following error...

~ ERROR:  syntax error at or near "PRIMARY"
LINE 1: CREATE TABLE "my_models" ( SERIAL PRIMARY KEY, "name" VARCHA...

SERIAL PRIMARY KEY should be preceded by "id" and PRIMARY KEY("id") should be removed if the primary key is to be set in that style. The correct SQL above creates the id column and sets the primary key in two separate places.

Correct SQL from above: CREATE TABLE "my_models" ("id" SERIAL NOT NULL, "name" VARCHAR(50), PRIMARY KEY("id"))

What I found out from debugging...

When the code is run in a migration setup! in migration.rb cause SQL::Postgres module to extend the adapter (See line 294). property_schema_statement method is overwritten to create this different primary key statement using SERIAL PRIMARY KEY. It relies, however, on schema[:quote_column_name] which isn't set in this case. That value is only set in table_creator.rb which isn't called in this case.

Question...

Why do we need a different create table syntax for the migrations? If the existing data mapper adapter syntax is used for postgres then everything works just fine. Would it be possible to delete the property_schema_statement in postgres.rb and have everything work?

Workaround...

When I call DataMapper.auto_upgrade! from our Sinatra rb file, then the migration runs through. I think this is what you're supposed to do anyway. That's probably why no one else has reported this yet.

Thanks for your help.

justjake commented 10 years ago

I just had the same issue. There's almost no documentation on DataMapper.auto_migrate_{up,down}, so issues are to be expected.