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

Purge variants with their blobs #31319

Merged
merged 1 commit into from Dec 3, 2017
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
3 changes: 2 additions & 1 deletion activestorage/app/models/active_storage/blob.rb
Expand Up @@ -270,7 +270,8 @@ def analyzed?
# deleted as well or you will essentially have a dead reference. It's recommended to use the +#purge+ and +#purge_later+
# methods in most circumstances.
def delete
service.delete key
service.delete(key)
service.delete_prefixed("variants/#{key}/") if image?
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why do we only want this for image? variants? Isn't it a little sneaky that the delete behavior is different depending on the blob type?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Non-image blobs don’t have variants. Skipping this saves an unnecessary API call.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, right 👍

end

# Deletes the file on the service and then destroys the blob record. This is the recommended way to dispose of unwanted
Expand Down
4 changes: 4 additions & 0 deletions activestorage/lib/active_storage/log_subscriber.rb
Expand Up @@ -18,6 +18,10 @@ def service_delete(event)
info event, color("Deleted file from key: #{key_in(event)}", RED)
end

def service_delete_prefixed(event)
info event, color("Deleted files by key prefix: #{event.payload[:prefix]}", RED)
end

def service_exist(event)
debug event, color("Checked if file exists at key: #{key_in(event)} (#{event.payload[:exist] ? "yes" : "no"})", BLUE)
end
Expand Down
9 changes: 7 additions & 2 deletions activestorage/lib/active_storage/service.rb
Expand Up @@ -78,6 +78,11 @@ def delete(key)
raise NotImplementedError
end

# Delete files at keys starting with the +prefix+.
def delete_prefixed(prefix)
raise NotImplementedError
end

# Return +true+ if a file exists at the +key+.
def exist?(key)
raise NotImplementedError
Expand All @@ -104,10 +109,10 @@ def headers_for_direct_upload(key, filename:, content_type:, content_length:, ch
end

private
def instrument(operation, key, payload = {}, &block)
def instrument(operation, payload = {}, &block)
ActiveSupport::Notifications.instrument(
"service_#{operation}.active_storage",
payload.merge(key: key, service: service_name), &block)
payload.merge(service: service_name), &block)
end

def service_name
Expand Down
30 changes: 23 additions & 7 deletions activestorage/lib/active_storage/service/azure_storage_service.rb
Expand Up @@ -19,7 +19,7 @@ def initialize(path:, storage_account_name:, storage_access_key:, container:)
end

def upload(key, io, checksum: nil)
instrument :upload, key, checksum: checksum do
instrument :upload, key: key, checksum: checksum do
begin
blobs.create_block_blob(container, key, io, content_md5: checksum)
rescue Azure::Core::Http::HTTPError
Expand All @@ -30,19 +30,19 @@ def upload(key, io, checksum: nil)

def download(key, &block)
if block_given?
instrument :streaming_download, key do
instrument :streaming_download, key: key do
stream(key, &block)
end
else
instrument :download, key do
instrument :download, key: key do
_, io = blobs.get_blob(container, key)
io.force_encoding(Encoding::BINARY)
end
end
end

def delete(key)
instrument :delete, key do
instrument :delete, key: key do
begin
blobs.delete_blob(container, key)
rescue Azure::Core::Http::HTTPError
Expand All @@ -51,16 +51,32 @@ def delete(key)
end
end

def delete_prefixed(prefix)
instrument :delete_all, prefix: prefix do
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed the instrumentation name to match the others in: 7609ca0

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Gah, thanks!

marker = nil

loop do
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could this potentially block forever? Or does list_blobs time out? Does it raise an exception when it does?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can provide the :timeout option to list_blobs if we wish, but it returns a limited number of blobs for each call (5,000 by default), so I don’t see why it’d block indefinitely.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍, the loop do just had me a bit worried 😊

results = blobs.list_blobs(container, prefix: prefix, marker: marker)

results.each do |blob|
blobs.delete_blob(container, blob.name)
end

break unless marker = results.continuation_token.presence
end
end
end

def exist?(key)
instrument :exist, key do |payload|
instrument :exist, key: key do |payload|
answer = blob_for(key).present?
payload[:exist] = answer
answer
end
end

def url(key, expires_in:, filename:, disposition:, content_type:)
instrument :url, key do |payload|
instrument :url, key: key do |payload|
base_url = url_for(key)
generated_url = signer.signed_uri(
URI(base_url), false,
Expand All @@ -77,7 +93,7 @@ def url(key, expires_in:, filename:, disposition:, content_type:)
end

def url_for_direct_upload(key, expires_in:, content_type:, content_length:, checksum:)
instrument :url, key do |payload|
instrument :url, key: key do |payload|
base_url = url_for(key)
generated_url = signer.signed_uri(URI(base_url), false, permissions: "rw",
expiry: format_expiry(expires_in)).to_s
Expand Down
22 changes: 15 additions & 7 deletions activestorage/lib/active_storage/service/disk_service.rb
Expand Up @@ -16,30 +16,30 @@ def initialize(root:)
end

def upload(key, io, checksum: nil)
instrument :upload, key, checksum: checksum do
instrument :upload, key: key, checksum: checksum do
IO.copy_stream(io, make_path_for(key))
ensure_integrity_of(key, checksum) if checksum
end
end

def download(key)
if block_given?
instrument :streaming_download, key do
instrument :streaming_download, key: key do
File.open(path_for(key), "rb") do |file|
while data = file.read(64.kilobytes)
yield data
end
end
end
else
instrument :download, key do
instrument :download, key: key do
File.binread path_for(key)
end
end
end

def delete(key)
instrument :delete, key do
instrument :delete, key: key do
begin
File.delete path_for(key)
rescue Errno::ENOENT
Expand All @@ -48,16 +48,24 @@ def delete(key)
end
end

def delete_prefixed(prefix)
instrument :delete_prefixed, prefix: prefix do
Dir.glob(path_for("#{prefix}*")).each do |path|
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Dir.glob can potentially return . and ... Should we do something specific here to filter them out? Or are they always filtered out via the prefix? (What happens if there's a blank prefix?)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Dir.glob can potentially return . and ...

I was under the impression it would only do so if we set the FNM_DOTMATCH flag:

irb(main):006:0> Dir.glob("/Users/george/Desktop/*")
=> []
irb(main):007:0> Dir.glob("/Users/george/Desktop/*", File::Constants::FNM_DOTMATCH)
=> ["/Users/george/Desktop/.", "/Users/george/Desktop/..", "/Users/george/Desktop/.DS_Store", "/Users/george/Desktop/.localized"]

Is that platform-dependent?

Or are they always filtered out via the prefix?

In Active Storage’s own usage, yes.

What happens if there's a blank prefix?

All objects are deleted. That seems correct, though perhaps not desirable.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is that platform-dependent?

Not sure. It could be Ruby version dependent. I definitely remember seeing it in our app on Ruby 2.2.

All objects are deleted. That seems correct, though perhaps not desirable.

I meant to say: if . and .. are included and the prefix is blank would it delete the current and parent directories?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In Active Storage’s own usage, yes.

I’m wrong. Active Storage’s usage of delete_prefixed would theoretically encounter this.

Not sure. It could be Ruby version dependent. I definitely remember seeing it in our app on Ruby 2.2.

I’m not seeing it on Ruby 2.2:

irb(main):002:0> RUBY_VERSION
=> "2.2.8"
irb(main):003:0> Dir.glob("/Users/george/Desktop/*")
=> []
irb(main):004:0> Dir.glob("/Users/george/Desktop/*", File::Constants::FNM_DOTMATCH)
=> ["/Users/george/Desktop/.", "/Users/george/Desktop/..", "/Users/george/Desktop/.DS_Store", "/Users/george/Desktop/.localized"]

I meant to say: if . and .. are included and the prefix is blank would it delete the current and parent directories?

If they’re returned by Dir.glob, yes.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, I was thinking of Dir.entries. No worries here then 👍

FileUtils.rm_rf(path)
end
end
end

def exist?(key)
instrument :exist, key do |payload|
instrument :exist, key: key do |payload|
answer = File.exist? path_for(key)
payload[:exist] = answer
answer
end
end

def url(key, expires_in:, filename:, disposition:, content_type:)
instrument :url, key do |payload|
instrument :url, key: key do |payload|
verified_key_with_expiration = ActiveStorage.verifier.generate(key, expires_in: expires_in, purpose: :blob_key)

generated_url =
Expand All @@ -77,7 +85,7 @@ def url(key, expires_in:, filename:, disposition:, content_type:)
end

def url_for_direct_upload(key, expires_in:, content_type:, content_length:, checksum:)
instrument :url, key do |payload|
instrument :url, key: key do |payload|
verified_token_with_expiration = ActiveStorage.verifier.generate(
{
key: key,
Expand Down
18 changes: 12 additions & 6 deletions activestorage/lib/active_storage/service/gcs_service.rb
Expand Up @@ -14,7 +14,7 @@ def initialize(**config)
end

def upload(key, io, checksum: nil)
instrument :upload, key, checksum: checksum do
instrument :upload, key: key, checksum: checksum do
begin
bucket.create_file(io, key, md5: checksum)
rescue Google::Cloud::InvalidArgumentError
Expand All @@ -25,7 +25,7 @@ def upload(key, io, checksum: nil)

# FIXME: Download in chunks when given a block.
def download(key)
instrument :download, key do
instrument :download, key: key do
io = file_for(key).download
io.rewind

Expand All @@ -38,7 +38,7 @@ def download(key)
end

def delete(key)
instrument :delete, key do
instrument :delete, key: key do
begin
file_for(key).delete
rescue Google::Cloud::NotFoundError
Expand All @@ -47,16 +47,22 @@ def delete(key)
end
end

def delete_prefixed(prefix)
instrument :delete_prefixed, prefix: prefix do
bucket.files(prefix: prefix).all(&:delete)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Feels strange that the gcs API object uses all and not each 😄

end
end

def exist?(key)
instrument :exist, key do |payload|
instrument :exist, key: key do |payload|
answer = file_for(key).exists?
payload[:exist] = answer
answer
end
end

def url(key, expires_in:, filename:, content_type:, disposition:)
instrument :url, key do |payload|
instrument :url, key: key do |payload|
generated_url = file_for(key).signed_url expires: expires_in, query: {
"response-content-disposition" => content_disposition_with(type: disposition, filename: filename),
"response-content-type" => content_type
Expand All @@ -69,7 +75,7 @@ def url(key, expires_in:, filename:, content_type:, disposition:)
end

def url_for_direct_upload(key, expires_in:, content_type:, content_length:, checksum:)
instrument :url, key do |payload|
instrument :url, key: key do |payload|
generated_url = bucket.signed_url key, method: "PUT", expires: expires_in,
content_type: content_type, content_md5: checksum

Expand Down
5 changes: 5 additions & 0 deletions activestorage/lib/active_storage/service/mirror_service.rb
Expand Up @@ -35,6 +35,11 @@ def delete(key)
perform_across_services :delete, key
end

# Delete files at keys starting with the +prefix+ on all services.
def delete_prefixed(prefix)
perform_across_services :delete_prefixed, prefix
end

private
def each_service(&block)
[ primary, *mirrors ].each(&block)
Expand Down
20 changes: 13 additions & 7 deletions activestorage/lib/active_storage/service/s3_service.rb
Expand Up @@ -17,7 +17,7 @@ def initialize(access_key_id:, secret_access_key:, region:, bucket:, upload: {},
end

def upload(key, io, checksum: nil)
instrument :upload, key, checksum: checksum do
instrument :upload, key: key, checksum: checksum do
begin
object_for(key).put(upload_options.merge(body: io, content_md5: checksum))
rescue Aws::S3::Errors::BadDigest
Expand All @@ -28,32 +28,38 @@ def upload(key, io, checksum: nil)

def download(key, &block)
if block_given?
instrument :streaming_download, key do
instrument :streaming_download, key: key do
stream(key, &block)
end
else
instrument :download, key do
instrument :download, key: key do
object_for(key).get.body.read.force_encoding(Encoding::BINARY)
end
end
end

def delete(key)
instrument :delete, key do
instrument :delete, key: key do
object_for(key).delete
end
end

def delete_prefixed(prefix)
instrument :delete_prefixed, prefix: prefix do
bucket.objects(prefix: prefix).batch_delete!
end
end

def exist?(key)
instrument :exist, key do |payload|
instrument :exist, key: key do |payload|
answer = object_for(key).exists?
payload[:exist] = answer
answer
end
end

def url(key, expires_in:, filename:, disposition:, content_type:)
instrument :url, key do |payload|
instrument :url, key: key do |payload|
generated_url = object_for(key).presigned_url :get, expires_in: expires_in.to_i,
response_content_disposition: content_disposition_with(type: disposition, filename: filename),
response_content_type: content_type
Expand All @@ -65,7 +71,7 @@ def url(key, expires_in:, filename:, disposition:, content_type:)
end

def url_for_direct_upload(key, expires_in:, content_type:, content_length:, checksum:)
instrument :url, key do |payload|
instrument :url, key: key do |payload|
generated_url = object_for(key).presigned_url :put, expires_in: expires_in.to_i,
content_type: content_type, content_length: content_length, content_md5: checksum

Expand Down
10 changes: 9 additions & 1 deletion activestorage/test/models/blob_test.rb
Expand Up @@ -41,13 +41,21 @@ class ActiveStorage::BlobTest < ActiveSupport::TestCase
end
end

test "purge removes from external service" do
test "purge deletes file from external service" do
blob = create_blob

blob.purge
assert_not ActiveStorage::Blob.service.exist?(blob.key)
end

test "purge deletes variants from external service" do
blob = create_file_blob
variant = blob.variant(resize: "100>").processed

blob.purge
assert_not ActiveStorage::Blob.service.exist?(variant.key)
end

private
def expected_url_for(blob, disposition: :inline)
query_string = { content_type: blob.content_type, disposition: "#{disposition}; #{blob.filename.parameters}" }.to_param
Expand Down
17 changes: 17 additions & 0 deletions activestorage/test/service/shared_service_tests.rb
Expand Up @@ -75,5 +75,22 @@ module ActiveStorage::Service::SharedServiceTests
@service.delete SecureRandom.base58(24)
end
end

test "deleting by prefix" do
begin
@service.upload("a/a/a", StringIO.new(FIXTURE_DATA))
@service.upload("a/a/b", StringIO.new(FIXTURE_DATA))
@service.upload("a/b/a", StringIO.new(FIXTURE_DATA))

@service.delete_prefixed("a/a/")
assert_not @service.exist?("a/a/a")
assert_not @service.exist?("a/a/b")
assert @service.exist?("a/b/a")
ensure
@service.delete("a/a/a")
@service.delete("a/a/b")
@service.delete("a/b/a")
end
end
end
end