monterail / guidelines

[DEPRECATED] We are Ruby on Rails experts from Poland. Think hussars. Solid & winged. These are our guidelines.
71 stars 17 forks source link

Using ActiveRecord in migrations guideline #222

Open Bartuz opened 10 years ago

Bartuz commented 10 years ago

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)

  1. : 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 commented 10 years ago

Ping @monterail/dev

szajbus commented 10 years ago

:-1: for anything that suggests using andand.

Other than that I think it's reasonable approach.

hodak commented 10 years ago

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.

teamon commented 9 years ago

@monterail/dev any other opinions?

jandudulski commented 9 years ago

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

teamon commented 9 years ago

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.

jandudulski commented 9 years ago

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.

jcieslar commented 9 years ago

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

In general I prefer raw SQL

Ostrzy commented 9 years ago

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.