Don't allow active_storage_blobs.content_type to be NULL

[betesh]

Summary

As of #38124 (Jan, 2020), active_storage_blobs.content_type has a default value so it will never be NULL. Let's enforce that at the database level.

[ghiculescu]

I'm not sure if this is a good idea for existing apps, who knows what sort of data they have in there. I think it is a good idea for new apps though.

It would also need a changelog entry, maybe in the changelog you can suggest that existing Active Storage users write a migration for their own tables if they want to.

[brenogazzola]

@betesh Here's a suggestion on how to solve some of the null values:

missing_files = []
ActiveStorage::Blob.where(content_type: nil).find_each do |blob|
  blob.identified = false
  blob.identify
rescue
  missing_files << blob.id
end

puts "The following blobs are missing their files: #{missing_files}"

In my case, out of 2M blobs I had two with NULL and their files were missing. In both cases blob.attachments.present? returned false, so it seems these were orphaned blobs that are probably safe to delete.

[betesh]

One option is putting @brenogazzola 's suggestion in the update-migration with enough comments that if manual intervention is required, it will be easy enough for the user to figure out what to do. For example:

def change
  missing_files = []
  ActiveStorage::Blob.where(content_type: nil).find_each do |blob|
    blob.identified = false
    blob.identify
  rescue
    if blob.attachments.present?
      missing_files << blob.id
    else
      files_that_cannot_be_identified << blob.id
    end
  end

  raise "content_type could not be identified for the following files: #{files_that_cannot_be_identified}.  Please set the content type manually and then re-run this migration." if files_that_cannot_be_identified.present?
  raise "Some blobs are missing their files: #{missing_files}.  These can probably safely be deleted.  After deleting or setting the content type manually, please re-run this migration" if missing_files.present?
  change_column_null(:active_storage_blobs, :content_type, false)
end

Pros: Enables existing apps to apply the migration with minimal confusion.
Cons: It can still be confusing. We're encouraging people to delete records when they don't necessarily have a full understanding of what they're doing.

Overall, I think the risk of users deleting records without knowing what they are doing is too high and the best thing is to remove the update migration and it makes more sense to go with @ghiculescu's suggestion. Agreed?

[rails-bot]

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.