guidelines icon indicating copy to clipboard operation
guidelines copied to clipboard

Using ActiveRecord in migrations guideline

Open Bartuz opened this issue 10 years ago • 9 comments

I'm migrating discussion here.

To wrap up what we have so far: @Machiaweliczny:

Reopen AR model in migration and reuse it

class Entry < ActiveRecord::Base;end
def up
  Entry.where(feed_id: nil).destroy_all
  Entry.where(start_date: nil).destroy_all
  Entry.where(end_date: nil).destroy_al
end

@teamon :

Never, never, never use models inside migrations. Use only raw SQL!!

@hodak :

Hmm, why? He defined class Entry < ActiveRecord::Base. Isn't it less error-prone than raw SQL?

@teamon :

The question is - does this inner Entry class is completely new or just opens the Entry model. If it's the latter then this is still quite dangerous.

DELETE FROM entries WHERE feed_is IS NULL OR start_date IS NULL OR end_date IS NULL

Since AR does not care about correct field names for me raw SQL and hacked AR as error-prone at the same level

@jandudulski:

Uhm, to be honest, I don't see any difference between manually injected model and sql created by hand - on both cases it would explode if you pass invalid column (field) names.

me :

I see 3 solutions: 1.: GenericeModel

class DontAllowNullInEntries < ActiveRecord::Migration
  class GenericModel < ActiveRecord::Base
    self.table_name = 'entries
  end
  def up
      GenericModel.where(...
      ...
  end
def down; end
end

2.: Use Entry AR model but with reset_column_information (http://apidock.com/rails/ActiveRecord/ModelSchema/ClassMethods/reset_column_information) 3. : Use Andand. https://github.com/raganwald/andand

IMHO:

Personally I believe reset_column_information is the best solution as it is proposed by Rails Guides™

Bartuz avatar Sep 15 '14 07:09 Bartuz

Ping @monterail/dev

Bartuz avatar Sep 15 '14 07:09 Bartuz

:-1: for anything that suggests using andand.

Other than that I think it's reasonable approach.

szajbus avatar Sep 15 '14 07:09 szajbus

reset_column_information sounds good, but it's not the only use case:

Auction.where("CREATED_AT < ?", Date.new(2014, 07, 20)).update_all(mechanism: 'Contest')
Auction.where("CREATED_AT >= ?", Date.new(2014, 07, 20)).update_all(mechanism: 'FCFS')

I think the generic model is better than writing raw sql.

hodak avatar Sep 15 '14 07:09 hodak

@monterail/dev any other opinions?

teamon avatar Jul 20 '15 20:07 teamon

WDYT http://railsguides.net/change-data-in-migrations-like-a-boss/ ?

jandudulski avatar Jul 20 '15 21:07 jandudulski

I will still prefer "raw" SQL over using existing models. Keeping tests for those migrations pass seems like a pointless additional work. And what happens when you remove the models from codebase at all? In the end you will have to replace the migration code with some SQL-ish solution.

For me either raw SQL or ad-hoc AR models are OK-ish, but with raw SQL it's harder to get into conflicts and to get surprised with some AR magic.

teamon avatar Jul 21 '15 08:07 teamon

Keeping tests for those migrations pass seems like a pointless additional work.

Actually that additional work for migration test could make sense when operating on data. Less chance that you will be surprised after running it on production ;)

And what happens when you remove the models from codebase at all?

Yes, that's painful and - in this case - you will have to alter the migration. Which is bad as we know.

On the other hand - you could treat that as a signal that it is time to flatten some migrations.

For me either raw SQL or ad-hoc AR models are OK-ish, but with raw SQL it's harder to get into conflicts and to get surprised with some AR magic.

For me it depends (as always). Sometimes writing SQL could be really painful, complicated and could be done much easier in plain ruby. Sometimes it is not and than SQL is good option.

I'm not a fan of migrations_data but it looks like an interesting alternative. At least - sometimes.

jandudulski avatar Jul 21 '15 08:07 jandudulski

Something like reset_column_information should be in rake task because we could repeat it.

In general I prefer raw SQL

jcieslar avatar Jul 21 '15 08:07 jcieslar

Actually that additional work for migration test could make sense when operating on data. Less chance that you will be surprised after running it on production ;)

I don't believe that these tests would check anything more than what writer will test manually either way, because yes, this is going to production and altering existing data.

For me it depends (as always). Sometimes writing SQL could be really painful, complicated and could be done much easier in plain ruby. Sometimes it is not and than SQL is good option.

I really think that's actually good. There is no better way to learn something than struggle with it a little. It's a little strange at first to write all this SQL in migrations, but it's really easy to get used to.

:+1: for NEVER using models in migrations.

Ostrzy avatar Jul 22 '15 06:07 Ostrzy