Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Migration drop table can't treat unique key properly #108

Closed
kenjis opened this issue Apr 5, 2012 · 13 comments
Closed

Migration drop table can't treat unique key properly #108

kenjis opened this issue Apr 5, 2012 · 13 comments

Comments

@kenjis
Copy link
Contributor

kenjis commented Apr 5, 2012

I generated migration create table, and added to up() function in the migration file:

\DBUtil::create_index('users', array('username', 'email'), '', 'UNIQUE');

And I executed "oil r migrate".

And I generated migration drop table. The migration file's down() function is like below:

\DB::query("CREATE INDEX username_idx ON cf_users (`username`)")->execute();

This is because the down() function is created with \DB::list_columns() which does not give full index information of the table.

@WanWizard
Copy link
Member

I don't understand this issue. a DROP TABLE drops the table and all indexes, why create an index in drop()?

@kenjis
Copy link
Contributor Author

kenjis commented Apr 6, 2012

For migrate:down.

@WanWizard
Copy link
Member

I got that that is what you use down() for.

I don't understand why you want to CREATE something in a down(). And why you would do that in a query() and not using DButil. And what this have to do with list_columns()...

@kenjis
Copy link
Contributor Author

kenjis commented Apr 6, 2012

I only added \DBUtil::create_index('users', array('username', 'email'), '', 'UNIQUE'); in up() function when I created a table.

\DB::query("CREATE INDEX username_idx ON cf_users (username)")->execute(); in down() function was generated by oil migration command when I dropped the table.

@WanWizard
Copy link
Member

that's odd. Why would oil add a "create index" in a down() method? @philsturgeon ?

@philsturgeon
Copy link
Contributor

No idea, that doesnt sound right.

@WanWizard
Copy link
Member

@kenjis more feedback on this issue? If not, I'm going to close it.

@kenjis
Copy link
Contributor Author

kenjis commented Apr 26, 2012

This is a bug. But priority is very low. I hope to keep it open.

If we drop a table, and go back to past version of migration, migration must create the table dropped.
But migration don't have/know create statement of the table, so get info from database.
At the time, migration does not get full index info of the table. So migration can't reproduce create statement.

To fix the bug, we must improve the logic of migration to get the table info from database.

@WanWizard
Copy link
Member

I'm trying to understand this (but failing at it).

If you have a migration that drops an index in an up(), it should do a create in the down(). Migrations have to be symmetric to work. So if you revert the migration it will run the down() and create the index again.

Please show me how to re-create this bug, so I can look at it.

@kenjis
Copy link
Contributor Author

kenjis commented May 8, 2012

How to reproduce:

$ oil g model user username:varchar[50] password:varchar[255] group:int:default[1] email:varchar[255] last_login:int login_hash:varchar[255] profile_fields:text

and add to up() method in migration file:
\DBUtil::create_index('users', array('username', 'email'), '', 'UNIQUE');

$ oil r migrate

$ oil g migration drop_users

will generates:

    public function down()
    {
        \DBUtil::create_table('users', array(
            'id' => array('type' => 'int', 'null' => true, 'constraint' => 11, 'auto_increment' => true),
            'username' => array('type' => 'varchar', 'null' => true, 'constraint' => 50),
            'password' => array('type' => 'varchar', 'null' => true, 'constraint' => 255),
            'group' => array('type' => 'int', 'default' => '1', 'constraint' => 11),
            'email' => array('type' => 'varchar', 'null' => true, 'constraint' => 255),
            'last_login' => array('type' => 'int', 'null' => true, 'constraint' => 11),
            'login_hash' => array('type' => 'varchar', 'null' => true, 'constraint' => 255),
            'profile_fields' => array('type' => 'text', 'null' => true),
            'created_at' => array('type' => 'int', 'null' => true, 'constraint' => 11),
            'updated_at' => array('type' => 'int', 'null' => true, 'constraint' => 11),

        ), array('id'));
        \DB::query("CREATE INDEX username_idx ON cf_users (`username`)")->execute();

    }

@WanWizard
Copy link
Member

Ok, I understand now. Thanks for the clarification.

@WanWizard
Copy link
Member

This can't be solved easily.

The General DESCRIBE output just gives information about the columns, SHOW INDEX is MySQL specific, which means it can't be implemented for other platforms until we have a better QB. For example Oracle or MS-SQL require a large and very complex query to fetch index details.

I'll keep this open to be addressed in the new QB.

@WanWizard
Copy link
Member

Better late than never: Oil should now have a lot better index support, including compound primary keys, compound indexes (unique or not), and index column sorting (ASC/DESC).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants