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

Clear ActiveStorage::Blob column information in migration file #41603

Closed
wants to merge 1 commit into from

Conversation

ohbarye
Copy link
Contributor

@ohbarye ohbarye commented Mar 3, 2021

This is just a proposal for an improvement. If you feel it's meaningless, I'm okay to withdraw.

Summary

This pull request attempts to eliminate a possible problem that could happen when calling ActiveStorage::Blobs in future migrations.

As documented in https://api.rubyonrails.org/classes/ActiveRecord/ModelSchema/ClassMethods.html#method-i-reset_column_information, we can avoid the problem by calling reset_column_information before calling the model in the next migration script.

However, I think it's a better practice to call reset_column_information after touching a model in advance in order to eliminate a future problem rather than postponing the problem in the future.

Other Information

Nothing.

This commit attempts to eliminate a possible problem that could happen when calling `ActiveStorage::Blobs` in future migrations.

[As documented](https://api.rubyonrails.org/classes/ActiveRecord/ModelSchema/ClassMethods.html#method-i-reset_column_information), we can avoid the problem by calling `reset_column_information` before calling an `ActiveRecord` model in the next migration script. However, I think it's a good practice to call `reset_column_information` after touching a model in advance to eliminate a future problem rather than postponing the problem in the future.
@georgeclaghorn
Copy link
Contributor

What specifically is this fixing? Do you have a concrete example you can share of something that doesn’t work without this change?

@ohbarye
Copy link
Contributor Author

ohbarye commented Mar 5, 2021

Okay, let me show what I imagined.

If I would have another migration script that adds a new column to ActiveStorage::Blob and populates default values as like activestorage/db/update_migrate/20190112182829_add_service_name_to_active_storage_blobs.rb does. (I know it's unlikely that a Rails user adds a column to the table, it's more likely that Rails core adds a column to provide a new feature.)

# db/migrate/xxxx_add_new_column_to_active_storage_blobs.rb
class AddNewColumnToActiveStorageBlobs < ActiveRecord::Migration
  def up
    add_column :active_storage_blobs, :new_column, :string
    ActiveStorage::Blob.update_all(new_column: "some default value")
  end
end

When running activestorage/db/update_migrate/20190112182829_add_service_name_to_active_storage_blobs.rb and the migration above at the same time,.update_all raises the following error unless calling .reset_column_information before .update_all. That's because ActiveStorage::Blob has cached column information since activestorage/db/update_migrate/20190112182829_add_service_name_to_active_storage_blobs.rb

ActiveModel::UnknownAttributeError: unknown attribute 'new_column' for ActiveStorage::Blob.

There's nothing that doesn’t work without this change as of now. So I think it's rather prevention than fix.

@rails-bot
Copy link

rails-bot bot commented Jun 3, 2021

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs.
Thank you for your contributions.

@rails-bot rails-bot bot added the stale label Jun 3, 2021
@zzak
Copy link
Member

zzak commented Jun 3, 2021

Doesn't this have potentially surprising effects if the user does not expect their column information to be reset 🤔

@rails-bot rails-bot bot removed the stale label Jun 3, 2021
@rails-bot
Copy link

rails-bot bot commented Sep 1, 2021

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs.
Thank you for your contributions.

@rails-bot rails-bot bot added the stale label Sep 1, 2021
@rails-bot rails-bot bot closed this Sep 8, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants