Store files with both Paperclip and ActiveStorage #4598
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
db/migrate/20210619201902_create_active_storage_tables.active_storage.rb
Show resolved
Hide resolved
javierm
force-pushed
the
active_storage_dual
branch
2 times, most recently
from
f00ee0f
to
a02dfb4
Compare
javierm
force-pushed
the
active_storage_dual
branch
from
a02dfb4
to
2034e6a
Compare
javierm
force-pushed
the
active_storage_dual
branch
from
2034e6a
to
021a638
Compare
javierm
force-pushed
the
active_storage_dual
branch
from
021a638
to
bcd2880
Compare
javierm
force-pushed
the
active_storage_dual
branch
from
bcd2880
to
da0a11d
Compare
javierm
force-pushed
the
active_storage_dual
branch
from
da0a11d
to
f75984c
Compare
javierm
force-pushed
the
active_storage_dual
branch
from
f75984c
to
aa72f40
Compare
| @@ -22,7 +22,7 @@ def deploysecret(key) | |||
| set :use_sudo, false | |||
|
|
|||
| set :linked_files, %w[config/database.yml config/secrets.yml] | |||
| set :linked_dirs, %w[.bundle log tmp public/system public/assets public/ckeditor_assets public/machine_learning/data] | |||
| set :linked_dirs, %w[.bundle log tmp public/system public/assets public/ckeditor_assets public/machine_learning/data storage] | |||
There was a problem hiding this comment.
Layout/LineLength: Line is too long. [125/110] (https://rubystyle.guide#max-line-length)
javierm
force-pushed
the
active_storage_dual
branch
from
aa72f40
to
02c1448
Compare
javierm
force-pushed
the
active_storage_dual
branch
from
02c1448
to
3f8fb25
Compare
javierm
force-pushed
the
active_storage_dual
branch
2 times, most recently
from
168b32d
to
c1574f2
Compare
javierm
force-pushed
the
hanging_tests
branch
from
01d9a3b
to
4f314bf
Compare
This reverts commit 7496c1b.
This migration was generated using the `active_storage:install` task.
ActiveStorage support was added to CKEditor in version 5.1.0. However, we can't upgrade to version 5.0.0 or later because it only supports loading CKEditor via CDN. So we're copying the code related to ActiveStorage instead. I tried to move it to the `vendor/` folder, but couldn't find a way to load it from there and if I found one I wouldn't be sure it'd work on production environments.
In order to migrate existing files from Paperclip to ActiveStorage, we need Paperclip to find out the files associated to existing database records. So we can't simply replace Paperclip with ActiveStorage. That's why it's usually recommended [1] to first run the migration and then replace Paperclip with ActiveStorage using two consecutive deployments. However, in our case we can't rely on two consecutive deployments because we have to make an easy process so existing CONSUL installations don't run into any issues. We can't just release version 1.4.0 and 1.5.0 and day and ask everyone to upgrade twice on the same day. Instead, we're following a different plan: * We're going to provide a Rake task (which will require Paperclip) to migrate existing files * We still use Paperclip to generate link and image tags * New files are handled using both Paperclip and ActiveStorage; that way, when we make the switch, we won't have to migrate them, and in the meantime they'll be accessible thanks to Paperclip * After we make the switch, we'll update the `name` column in the active storage attachments tables in order to remove the `storage_` prefix Regarding our handling of new files, the exception are cached attachments. Since those attachments are temporary files used while submitting a form and we have to delete them afterwards, we're only handling them with Paperclip. We'll handle these ones in version 1.5.0. Note the task creating the dev seeds was failing after these changes with an `ActiveStorage::IntegrityError` exception because we were opening some files without closing them. If the same file was attached twice, it failed the second time. We're solving it by closing the files with `File.open` and a block. Even though we didn't get any errors, we're doing the same thing in the `Attachable` concern because it's a good practice to close files after we're done with them. Also note we have to change the CKEditor Active Storage code so it's compatible with Paperclip. In this case, I haven't been able to write a test to confirm the attachment exists; I was getting the same `ActiveStorage::IntegrityError` mentioned above. Finally, we're updating the site customization image controller to use `update` so the image and the attachment are updated within the same transaction. This is also what we do in most controllers. [1] https://www.youtube.com/watch?v=tZ_WNUytO9o
This task was copied from Thoughtbot's migration guide [1]. They use a migration file, but we usually use rake tasks to migrate data. [1] https://github.com/thoughtbot/paperclip/blob/master/MIGRATING.md
We can use the `ActiveStorage::Blob` class to find where the file is supposed to be stored instead of manually setting the path. This is also more robust because it works with Active Storage configurations which don't store files in the default folder.
This model is added by the devise-security gem, but we don't use it. We
were getting an error while migrating:
ActiveRecord::StatementInvalid: PG::UndefinedTable: ERROR: relation "old_passwords" does not exist
LINE 8: WHERE a.attrelid = '"old_passwords"'::regclas...
^
SELECT a.attname, format_type(a.atttypid, a.atttypmod),
pg_get_expr(d.adbin, d.adrelid), a.attnotnull, a.atttypid, a.atttypmod,
c.collname, col_description(a.attrelid, a.attnum) AS comment
FROM pg_attribute a
LEFT JOIN pg_attrdef d ON a.attrelid = d.adrelid AND a.attnum = d.adnum
LEFT JOIN pg_type t ON a.atttypid = t.oid
LEFT JOIN pg_collation c ON a.attcollation = c.oid AND a.attcollation <> t.typcollation
WHERE a.attrelid = '"old_passwords"'::regclass
AND a.attnum > 0 AND NOT a.attisdropped
ORDER BY a.attnum
lib/tasks/active_storage.rake:27:in `block (4 levels) in <top (required)>'
lib/tasks/active_storage.rake:26:in `each'
lib/tasks/active_storage.rake:26:in `block (3 levels) in <top (required)>'
lib/tasks/active_storage.rake:25:in `block (2 levels) in <top (required)>'
Caused by:
PG::UndefinedTable: ERROR: relation "old_passwords" does not exist
Using `LASTVAL()` wasn't working 100% of the time for some reason after inserting and deleting some rows. So we're using `RETURNING` instead.
Just like we add the `storage_` prefix for new records so we can use both Active Storage and Paperclip at the same time. Now the migration actually works, at least for basic cases.
We were getting a duplicate prepared statement error when trying to execute more than one test using this task. So we're checking whether the prepared statement exists before defining it.
Files might be missing for whatever reason or records might not point to any files; in these edge cases, we were getting an exception.
This way we reduce the hypothetical problems we could find if executing the task several times.
There could be inconsistencies in the database and an attachment might have a `record_id` pointing to a record which no longer exist. We were getting an exception in this case.
We were having issues with cached attachments and external services. A `Tempfile` is returned by `URI.open` when using S3, so we're dealing with this case as well.
So forks that connected Paperclip with S3 buckets can migrate files too.
These configurations are based on the ones ActiveStorage generates automatically for Rails 6 applications.
We're already using a custom controller to handle direct uploads. Besides, as mentioned by one of Active Storage co-authors [1], the Active Storage DirectUploadsController doesn't provide any authentication or validation at all, meaning anyone could create blobs in our database by posting to `/rails/active_storage/direct_uploads`. The response there could be then used to upload any file (again, without validation) to `/rails/active_storage/disk/`. For now, we're monkey-patching the controllers in order to send unauthorized responses, since we aren't using these routes. If we ever enable direct uploads with Active Storage, we'll have to add some sort of authentication. Similar upload solutions like CKEditor don't have this issue since their controllers inherit from `ApplicationController` (which includes authorization rules), while Active Storage controllers inherit from `ActionController::Base`. [1] https://discuss.rubyonrails.org/t/activestorage-direct-uploads-safe-by-default-how-to-make-it-safe/74863/2
javierm
force-pushed
the
active_storage_dual
branch
from
c1574f2
to
be9c272
Compare
Senen
approved these changes
Sep 24, 2021
Merged
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
References
Objectives
Notes
In order to migrate existing files from Paperclip to ActiveStorage, we need Paperclip to find out the files associated to existing database records. So we can't simply replace Paperclip with ActiveStorage.
That's why it's usually recommended to first run the migration and then replace Paperclip with ActiveStorage using two consecutive deployments.
However, in our case we can't rely on two consecutive deployments because we have to make an easy process so existing CONSUL installations don't run into any issues. We can't just release version 1.4.0 and 1.5.0 the same day and ask everyone to upgrade twice in a row.
Instead, we're following a different plan:
namecolumn in the active storage attachments tables in order to remove thestorage_prefixTesting this pull request
Before merging this pull request, we need to test this scenario:
migrate_from_papercliptaskremove_paperclip_compatibility_in_existing_attachmentstaskmigrate_from_paperclipare correctly displayedWe need to do so with both a local storage and a remote storage.
Release Notes
has_attachmentinstead ofhas_attached_file. Otherwise you might run into missing files when upgrading to version 1.5.0.In version 1.5.0 we'll change the way we store attachments from Paperclip to Active Storage. In order to make the migration painless, version 1.4.0 stores new files using both systems. If you've changed anything related to the way attachments are handled, there's a chance you might run into some issues when executing
consul:execute_release_tasks(which includes a task to copy existing files to Active Storage). In version 1.5.0 we'll drop Paperclip and use just Active Storage, so in the case you run into problems when running the migration, make sure you fix them or report them; otherwise you might run into missing files when upgrading to version 1.5.0.After copying the existing files to Active Storage, keep both the Active Storage files and the Paperclip files. The Paperclip files are needed in version 1.4.0, and the Active Storage files will be needed in version 1.5.0.
If you're storing files using an external service (such as S3), before executing the release tasks edit the
config/storage.ymlfile and configure the service. Sample configurations are provided for S3, GCS and Azure; uncomment them where appropriate and then edit yourconfig/secrets.ymlfile with your credentials. Then edit yourconfig/environments/production.rbfile and addconfig.active_storage.service = :s3(or:gcsor:azure, depending on the service you're using).