Run Active Storage migrations in new apps

[claudiob]

In previous versions of Rails, running rails new app_name; cd app_name; rails server
was all it took to see something up and running in your browser at localhost:3000.

This had a great impact on newcomers, who would get excited about having something
up and running so quickly.

With Rails 5.2.0.beta2, this has changed: opening the browser now shows an
ActiveRecord::PendingMigrationError and a very "technical" page that asks to run
migrations.

The reason is that the last step of rails new is now rails active_storage:install
which copies migration _create_active_storage_tables.active_storage.rb
from active_storage.

This commit ensures that those migrations are also executed when they are
added to the app.

[claudiob]

This is a follow up to the conversation with @pixeltrix and @rafaelfranca.

I think we might need a separate method that checks everything related to running the migrations on rails new, so I wouldn't merge the code as it is.
Mostly created a PR to check what errors are raised by Travis for the different SQL adapters.

[guilleiguaran]

Let's do this as independent step from active_storage:install so if this fails we can display a message like database migration for ActiveStorage couldn't be ran, please set your database configuration and run rails db:migrate

[kaspth]

Would it be better to try to suppress the migrations pending error in this particular case? I.e. if the only thing in db/migrate matches the Active Storage ones, we skip.

[claudiob]

@kaspth @guilleiguaran I'm happy with both solutions. Which one makes the most sense?

[claudiob]

Another idea could be not to run rails active_storage:install by default.

The first time you try to use Active Storage, you see an error similar to the error you see when you haven't run all your migration. In this case, the error invites you to run rails active_storage:install.

[claudiob]

Looking at #30101 (comment) it seems like there might be other benefits at not running rails active_storage:install by default.

I will delegate the decision to the people who have worked on this feature before.
@bogdanvlviv @dhh what do you think?

[guilleiguaran]

/cc @georgeclaghorn wdyt?

[matthewd]

I do lean toward thinking that creating a set of DB tables is a pretty substantial thing to do for a feature that plenty of applications aren't going to need.

For some things, I think taping the batteries inside the box is enough -- "included" doesn't need to mean they're already in and it's switched on.

I'd like a super smooth process, to start using ASt, of course... I'm just hopeful we can come up with a different way of getting there. e.g., if people use the model generator to create their ASt-using model, maybe we can react to that?

The alternative is to start setting up a database before they've even had a chance to affect e.g. its name -- that seems annoying at best, and most alarming if we happen to guess a DB name they're already using. Affecting parts of their computer outside the target directory just feels well outside the remit of an application generator. (Yes, I know we invoke bundler, which installs gems, which also meets the "outside" criteria... but it still feels very different to me.)

[claudiob]

@matthewd I tend to agree with you. Another example is how we deal with ActiveRecord Migration.

When you generate your first model in an app, we create migration files for you, but we don't run them automatically. However, if you try to open the app in the browser, we show you an error telling you what to do (run this command to run the migration).

On one hand, we are giving you the chance to edit those migrations (changing table names, columns, adapters, …) before you run them. On the other hand, we show an error if you forgot to run them before they are needed.

Maybe we can follow the same approach. Whenever someone tries to use Active Storage and does not have the proper table created, we don't show this message:

but something more verbose, in the line of "you should run bin/rails active_storage:install…".

---

The alternative is to start setting up a database before they've even had a chance to affect e.g. its name --

You are correct, this could be an alternative: to treat the "active_storage_attachments" table just like we treat "ar_internal_metadata" or "schema_migrations". We could simply create the table under the hood, without letting the user interact with it directly. I'm not a fan of this idea, though. I think attachments "belong to" the application, and a user should have the right to access them as needed.

[dhh]

I concur with @claudiob and @matthewd. Love the analogy of batteries taped inside, but not inserted. Having a good error message if you try to use ASt without the migrations run, then provide a single task to supply them all 👍

[claudiob]

I have update the PR as requested with two commits:

1. Don't run rails active_storage:install on new apps
2. Show an informative error when a user tries to use Active Storage without the appropriate table

[dhh]

Lovely! ❤️

…

On Fri, Dec 8, 2017 at 10:06 AM, Claudio B. ***@***.***> wrote: I have update the PR as requested with two commits: 1. Don't run rails active_storage:install on new apps 2. Show an informative error when a user tries to use Active Storage without the appropriate table [image: snap 2017-12-08 at 8 03 46 am] <https://user-images.githubusercontent.com/10076/33773930-6fc1c0be-dbee-11e7-9527-6d6a73fd0eba.png> — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#31315 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAAKtZ6Iw_kiot7MB1xlefTmuznNIxC2ks5s-V5ogaJpZM4Qzc8M> .

[claudiob]

Thanks! And tests are passing 🎉

[georgeclaghorn]

I think we need a similar guard in ActiveStorage::Blob.create_before_direct_upload!.

[georgeclaghorn]

This doesn’t seem to warrant a new test case. I’d add it to ActiveStorage::BlobTest.

[claudiob]

@georgeclaghorn
I followed your advice and added a separate test for create_blob_before_direct_upload.
I created a separate test file to be able to use setup and teardown in isolation.

[bogdanvlviv]

I think we need add the same error message for #attach:

Could not find table 'active_storage_attachments'. To resolve this issue run: bin/rails active_storage:install

[bogdanvlviv]

I think would be good make this method private.

[claudiob]

Done!

[bogdanvlviv]

Really good PR! I'm sure this will work better than the execution of bin/rails active_storage:install on rails new. Also, this will helpful for apps that will get upgrade to 5.2.
Thanks for improving this @claudiob. ❤️

[claudiob]

@bogdanvlviv I followed your advice and added code and tests for active_storage_attachments as well. However, I'm not 100% happy with the current code and I was wondering if you have more advices.

---

My first concern is exemplified by the following code:

Post.first.photo.attach({})
# => ActiveStorage::Attached::MissingTableError: Could not find table 'active_storage_attachments'. To resolve this issue run: bin/rails active_storage:install

Post.first.photo_attachment
# => ActiveRecord::StatementInvalid: Could not find table 'active_storage_attachments'

In other words, all the Attached methods (attach, attached?, detach, purge, purge_later) will show the correct warning, but calling the association directly won't.
The comment in the code says:

The system has been designed to having you go through the ActiveStorage::Attached::One
proxy that provides the dynamic proxy to the associations and factory methods, like +attach+.

so we might be okay with the current code. Just wondering what you think.

---

My second concern is currently defining two separate error classes: ActiveStorage::Blob:MissingTableError and ActiveStorage::Attached::MissingTableError.

The code feels a little redundant but I'm not sure where to extract raise_if_table_missing! to.
Maybe a new module? That doesn't feel right either.

[claudiob]

Just updated the PR to also remove the following sentence from the freshly published guide to Active Storage:

[kaspth]

I'm not totally sold on the code approach in this PR. We're burdening the common case with initial setup work. In case we add new methods they'll now have to remember to raise as well.

Another way could be to wrap the ActiveRecord::StatementInvalid in a custom exception template:

rails/actionpack/lib/action_dispatch/middleware/exception_wrapper.rb

Line 28 in 053a4c6

"ActionView::Template::Error" => "template_error"

Then display the extra "run the install" in case either of the tables are missing.