shrinerb / shrine

File Attachment toolkit for Ruby applications
https://shrinerb.com
MIT License
3.18k stars 275 forks source link

Shrine keep_files plugin and derivative deletion on S3 #553

Closed dylanfisher closed 3 years ago

dylanfisher commented 3 years ago

I have a question about the Keep Files plugin and how it interacts with Derivatives. Specifically, I notice that even with keep_files, my derivatives are destroyed (the original file is kept), and I wasn't sure if this was the intentional behavior (or a side effect of how my app is setup). It would be helpful to add documentation to the keep_files page that explains this interaction.

I'm using the Backgrounding plugin to destroy derivatives. The derivatives are stored on S3 via the Shrine::Storage::S3 configuration. My destroy job looks like this:

class AttachmentDestroyJob < ApplicationJob
  def perform(attacher_class, data)
    attacher_class = Object.const_get(attacher_class)
    attacher = attacher_class.from_data(data)
    attacher.destroy
  end
end

So, should the keep_files plugin prevent those derivatives from being destroyed, or is there something else in my code that is overriding the behavior and destroying the derivatives.

janko commented 3 years ago

Hmm, the keep_files plugin should be correctly skipping derivatives deletion as well. When you load the keep_files plugin, the background job shouldn't get spawned in the first place. Below is a self-contained example that shows that neither the main file nor its derivatives get deleted:

require "shrine"
require "shrine/storage/memory"
require "sidekiq"
require "sidekiq/testing"
require "stringio"
require "sequel"

DB = Sequel.sqlite
DB.create_table :attachments do
  primary_key :id
  String :file_data
end

Sidekiq::Testing.inline!

class DestroyJob
  include Sidekiq::Worker

  def perform(attacher_class, data)
    attacher_class = Object.const_get(attacher_class)

    attacher = attacher_class.from_data(data)
    attacher.destroy
  end
end

Shrine.storages = {
  cache: Shrine::Storage::Memory.new,
  store: Shrine::Storage::Memory.new,
}

Shrine.plugin :sequel
Shrine.plugin :backgrounding
Shrine.plugin :derivatives
Shrine.plugin :keep_files

Shrine::Attacher.destroy_block do
  DestroyJob.perform_async(self.class.name, data)
end

class Attachment < Sequel::Model
  include Shrine::Attachment(:file)
end

attachment = Attachment.new
attachment.file_attacher.attach(StringIO.new("file"))
attachment.file_attacher.add_derivatives({ name: StringIO.new("derivative") })
attachment.save
attachment.destroy # calls destroy job
attachment.file.exists? # => true
attachment.file_derivatives[:name].exists? # => true

Maybe you have a different setup, or the order in which you load plugins doesn't produce correct behaviour. In that case I would need your help reproducing the issue.

dylanfisher commented 3 years ago

OK thanks for this test – in that case it's probably something specific to my app. I'll report back if it's worth mentioning.

I wonder if either way the Keep Files page could mention derivatives, just so there's no question that is the intended behavior.