using models in Rails migrations

[Scott Cytacki]

I'm thinking we should drop the practice of inlining models in our Rails migrations, or at least not require it. Do you have an opinion?

Read on, if you're curious.

In the past I've always tried to use inline models in Rails migrations. We ran into issues before when migrations failed because models were deleted or changed. So I have been following this guide:

http://guides.rubyonrails.org/v3.2.21/migrations.html#using-models-in-your-migrations

But recently I was going to refer to this section again and found it was removed from the recent guides. After a bit of hunting I found it was removed because it had errors, and then when someone tried to add it back, there was  this discussion:

https://github.com/rails/rails/pull/19746

I'm defensive by nature, so using inline models seems like the safest approach.  But I agree with fxn in that pull request: issues don't happen often. The commit that got me thinking about this: 

https://github.com/concord-consortium/lara/commit/65269361a11968c47e8524dac01f9dcd7f4edc0c

The only times I remember problems with this were in the portal when running migrations that were old. Of course I think we've mostly used inline models since then, so we might have averted other problems. In the portal when setting up a new database, we still do the practice of running every migration from the beginning of the portal.  Which is not recommended and makes these models in the migrations issues more of a problem.

--

Scott Cytacki

The Concord Consortium

http://concord.org

[Noah Paessel]

I don't think I understand what seems to be an important part of FXN's argument.  He claims that:

(migrations) "… are transient and expected to be deleted."  – Like literally removed from the repo?  Because we don't do that.  Is that the recommended practice now?  If they aren't actually meant to be removed from the project, then what does he mean precisely?

Also, it seems like they are discussing what is appropriate to include in 'official' documentation.

Also: including models at all in your migration is already something that is rare. You only do it when you need to do something specific that is somehow more easily done via model methods than with plain sql. 

I am probably missing something, but I don't see a case for a prohibition against inlining models. I would also say that if you expect your models to preform some action in the migration, its very handy to be able to explore that method in the same file as the migration, just for readability if nothing else. 

 

--
--
----
post message :cc-dev...@googlegroups.com
unsubscribe: cc-developer...@googlegroups.com
more options: http://groups.google.com/group/cc-developers?hl=en
---
You received this message because you are subscribed to the Google Groups "Concord Consortium Developers" group.
To unsubscribe from this group and stop receiving emails from it, send an email to cc-developer...@googlegroups.com.
For more options, visit https://groups.google.com/d/optout.

[Scott Cytacki]

On Fri, May 8, 2015 at 11:12 AM, Noah Paessel <npae...@concord.org> wrote:

I don't think I understand what seems to be an important part of FXN's argument.  He claims that:

(migrations) "… are transient and expected to be deleted."  – Like literally removed from the repo?  Because we don't do that.  Is that the recommended practice now?  If they aren't actually meant to be removed from the project, then what does he mean precisely?

Yes that is the recommended practice: remove migrations from the repository. They only need to stick around to help migrate older databases. Once all old databases have been migrated the migration isn't needed anymore.  Given our pace, it seems 1 year would be a appropriate time to start pruning migrations.

 

Also, it seems like they are discussing what is appropriate to include in 'official' documentation.

Also: including models at all in your migration is already something that is rare. You only do it when you need to do something specific that is somehow more easily done via model methods than with plain sql. 

So in this commit would you recommend they change this to sql instead of AR models, or this is a good use of AR models?
https://github.com/concord-consortium/lara/commit/65269361a11968c47e8524dac01f9dcd7f4edc0c

 

I am probably missing something, but I don't see a case for a prohibition against inlining models. I would also say that if you expect your models to preform some action in the migration, its very handy to be able to explore that method in the same file as the migration, just for readability if nothing else. 

I don't think we need to prohibit them. In the past my own goal was to require them. So if I saw a migration directly using models from app/models, them I'd try to get it fixed. 

I'm considering relaxing that requirement. I also wanted to know if anyone else cared about this? :)

The main arguments for not using them seem to be:

- they bloat the migration

- they encourage the idea that migrations are permanent instead of transient

- there is now no official document on how to inline models in migrations

I don't really have an opinion either way, but I would like to have a consistent approach between developers and apps.  If we are sticking with inline migrations, I think we should copy the example from here: https://github.com/rails/rails/pull/19746 into a place we can refer to. 

[Aaron Unger]

Yeah, I'm not understanding the argument fxn is making regarding migrations being transient and deletable. I feel like he's confusing starting a db up from scratch (in which case using db:schema:load absolutely makes sense) vs supporting updating older deployments, in which case you *require* migrations to be available. I mean, yes, with a decent version control system you can slowly walk up the commit history to apply migrations at the point they are added to the codebase (in which case you also have a snapshot of the models at that point in time and wouldn't need inlined models), but it seems a little crazy to require a user of your app to do that in order to update their deployment to the latest release.

So I'll vote for keeping inline models, when needed.

As for the commit in question... A SELECT followed by a big UPDATE per model might not be so bad. I could go either way on it, though.

-- Aaron

[Noah Paessel]

On Fri, May 8, 2015 at 11:35 AM, Scott Cytacki <scyt...@concord.org> wrote:

On Fri, May 8, 2015 at 11:12 AM, Noah Paessel <npae...@concord.org> wrote:

 

Also: including models at all in your migration is already something that is rare. You only do it when you need to do something specific that is somehow more easily done via model methods than with plain sql. 

So in this commit would you recommend they change this to sql instead of AR models, or this is a good use of AR models?
https://github.com/concord-consortium/lara/commit/65269361a11968c47e8524dac01f9dcd7f4edc0c

Its certainly easy to write it this way, but it wouldn't be too hard to inline the AR models either. It would probably be annoying to write out the SQL for those changes -- but it would probably be faster.

Is it obvious what observers will fire, what validations will run, or what other side-effects might happen when reading this migration? I guess in this case, I wouldn't expect anything weird to happen.

But in the Portal for example, we cache the Investigation OTML,  so making a change to a single embedded text field like this requires invalidating the cache for the entire investigation. I think this uses an observation pattern, but I can't remember how the mixin works.  Re-writing AR Classes that implement (or mixing) the parent-climbing logic  to invalidate the OTML would be annoying, but it would make the changes we expected to see explicit. 

It seems smart to try and isolate change, and be explicit if we have the energy.

We don't need to vigorously enforce this, especially in this case. It just seems like a good suggestion, something to keep in mind, rather than a rule. So I agree we should relax it as a requirement.  :)

[Aaron Unger]

Observers (and validations, after save/create callbacks, etc) would only fire for the models defined in app/models/. And I think we disable observers in the Portal when running db:migrate anyway. When inlining models (or modifying things through raw SQL), I guess it's the migration author's responsibility to be vigilant about what normally occurs when working with that particular model and compensate. In your cache example, you'd have to expire the cache as part of the migration. (Granted, if you're updating/re-deploying code, it might not be a bad idea to expire caches anyway since who knows what else might have changed.)

So in summary: migrations are tough, especially with a large codebase. :)

-- Aaron