ActiveStorageAdapter can upload multiple files#480
ActiveStorageAdapter can upload multiple files#480jasonkarns wants to merge 2 commits intokjvarga:masterfrom
Conversation
The adapter must be able to handle multiple files (for writing sitemaps that are split due to size, as well as the resulting index). This means the adapter must derive the filename and key (as well as content type) from the `#write` location argument instead of being fixed at instantiation. This allows the same instance of an adapter to write multiple differently-named files, and even of different content types. It also adds the ability to customize a prefix, which may be necessary to avoid conflicts in ones storage bucket.
The autoload needs to be unconditional because ActiveStorage may be available when the _adapter_ is even if it's not available when the sitemap_generator is loaded. (That is the point of autoloads, after all. To allow delayed and potentially unused loading.)
|
|
||
| ActiveStorage::Blob.transaction do | ||
| ActiveStorage::Blob.where(key: key).destroy_all | ||
| ActiveStorage::Blob.destroy_by(key: key(location)) |
There was a problem hiding this comment.
destroy_by is a Rails 6+ shortcut for where().destroy_all. Normally I would have left the existing where.destroy_all as it's well understood and explicit.
However, because ActiveStorage::Blob is an external collaborator in this codepath, reducing the interface on which we depend improves robustness. It also eases the burden on tests (in this case, it eliminates the need to support a chained stub that returns yet another stub).
| let!(:active_storage) do | ||
| class_double('ActiveStorage::Blob', destroy_by: true, create_and_upload!: nil) | ||
| .tap { |blob| allow(blob).to receive(:transaction).and_yield } | ||
| .as_stubbed_const |
There was a problem hiding this comment.
class_double and as_stubbed_const are purpose built for creating safe double of classes.
The class_double gives additional verification over using Class.new, because class_double also verifies that any stubbed method is actually present on the real ActiveStorage::Blob. (If active-storage is ever loaded in these tests.)
Additionally, the Class.new approach continually redefines the FakeScope constant, so with multiple specs, you get constant-redefined warnings.
jasonkarns
changed the title
| filename: filename, | ||
| content_type: 'application/gzip', | ||
| key: key(location), | ||
| io: File.open(location.path), |
There was a problem hiding this comment.
Switched to File.open to match all the other adapters.
However, a future refactor could/should make location.path return a Pathname object. Which would enable: location.path.open
|
|
||
| def write(location, raw_data) | ||
| SitemapGenerator::FileAdapter.new.write(location, raw_data) | ||
| FileAdapter.new.write(location, raw_data) |
There was a problem hiding this comment.
I'm not exactly sure why all these adapters write to file first, but they all do, so this is preserved. (Though it seems awkward? If I'm using active-storage adapter, for instance, I wouldn't expect files to also be written to disk.)
1 participant
Fixes #479
Rework ActiveStorageAdapter to handle multiple files
The adapter must be able to handle multiple files (for writing sitemaps
that are split due to size, as well as the resulting index). This means
the adapter must derive the filename and key (as well as content type)
from the
#writelocation argument instead of being fixed atinstantiation.
This allows the same instance of an adapter to write multiple
differently-named files, and even of different content types. It also
adds the ability to customize a prefix, which may be necessary to avoid
conflicts in ones storage bucket.
Autoload shouldn't be conditional
The autoload needs to be unconditional because ActiveStorage may be
available when the adapter is even if it's not available when the
sitemap_generator is loaded. (That is the point of autoloads, after all.
To allow delayed and potentially unused loading.)