Shrine: Deletion doesn't work with rom-rb

[musaffa]

Rom-rb uses dry-struct as in its entity objects. We cannot create instances of these struct classes just by calling new method. We have to supply the values of all the attributes in the new method calls. In the backgrounding plugin, new method is called on the record_class (dry-struct class in case of rom-rb) without passing any attribute so it fails with the following error:

undefined method default? ...

Hence, file deletion fails.

It's unfeasible to set default values for all attributes in rom entity structs. As an alternative to record_class.new, we can call OpenStruct.new which works for all cases including rom-rb.

[hmistry]

Thank you for reporting this and your suggested solution. We will look into it.

[janko]

The current background implementation was really a bandaid over the lack of proper support for database libraries like rom-rb. The general problem is that Shrine couples too much to the Active Record pattern (which Sequel also implements), and this is something I plan to address.

The backgrounding plugin shouldn't be responsible for fetching the database record in the first place. Yeah, it's convenient, because it's just a single line in the background worker, but it sometimes won't "just work" (for example if you're using multi-tenancy or sharding)

For now I would suggest re-implementing the Shrine::Attacher.promote class method that backgrounding plugin provides. It's not too complicated, you can still use the Shrine::Attacher#promote instance method which should do most of the hard work anyway.

[musaffa]

Instead of promote, I've overridden load_record like this:

def load_record(data)
  record_class, record_id = data['record']
  record_class = Object.const_get(record_class)

  record   = find_record(record_class, record_id)
  record ||= OpenStruct.new.tap do |instance|
    # so that the id is always included in file deletion logs
    instance.singleton_class.send(:define_method, :id) { record_id }
  end

  record
end

[musaffa]

@janko-m I wanted to write the rom plugin this way. The code has mostly been taken from shrine-rom-example:

require 'ostruct'

module Api
  module Attachment
    module Plugins
      module ROM
        def self.configure(uploader, opts = {})
          uploader.opts[:rom_repository] = opts.fetch(:repository)
        end

        module AttacherClassMethods
          def find_record(record_class, record_id)
            rom_repository(record_class)[record_id]
          end

          def rom_repository(record_class)
            entity = record_class.to_s.split('::')[-1]
            shrine_class.opts[:rom_repository].call(entity) # line that causes problem
          end

          def load_record(data)
            record_class, record_id = data['record']
            record_class = Object.const_get(record_class)

            record   = find_record(record_class, record_id)
            record ||= OpenStruct.new.tap do |instance|
              instance.singleton_class.send(:define_method, :id) { record_id }
            end

            record
          end
        end

        module AttacherMethods
          private

          def update(uploaded_file)
            super
            context[:record] = rom_repository.update(
              record.id, "#{name}_data": read
            )
          end

          def rom_repository
            self.class.rom_repository(record.class)
          end
        end
      end
    end
  end
end

Shrine.plugin Api::Attachment::Plugins::Rom,
  repository: ->(model) { Object.const_get("Api::Repositories::#{model}").new($rom) }

Checkout the commented line that causes the problem. When load_record calls find_record it finds the rom_repository opt from shrine_class.opts. But find_record is also called from swap method, in which case rom_repository option is not available in shrine_class.opts anymore. Thus it fails with the following error:

NoMethodError undefined method `call' for nil:NilClass

If this case could be resolved, I could create a generic rom plugin for shrine. I can submit a PR if you are willing to add rom integration for shrine.

[janko]

@musaffa Thank you for showing interest in adding rom-rb integration, and I apologize for my lack of response, it slipped off my radar. I first imagined a rom-rb integration being a separate gem, but now that you mention it, it's not a bad idea to ship it with Shrine. I find rom-rb an equally important database library as ActiveRecord or Sequel.

I feel like the way that Shrine::Attacher is currently designed it will be difficult to implement a proper rom-rb support. I would like to investigate options after completing the refactoring described in https://github.com/shrinerb/shrine/issues/381, which should clean up the space for this integration.

Maybe Hanami users will then be able to use the same integration, and there would be no need for a separate Hanami integration.

[janko]

Shrine 3.0 came with a big refactoring, which included redesigning the backgrounding plugin to be more flexible. Now deletion doesn't require any records anymore:

Shrine.plugin :backgrounding
Shrine::Attacher.destroy_job { DestroyJob.perform_async(data) }

class DestroyJob
  include Sidekiq::Worker
  # no records are required
  def perform(data)
    attacher = Shrine::Attacher.from_data(data)
    attacher.destroy
  end
end

I also started working on a rom-rb plugin, which aims to provide complete support. It should be finished at some point, but so far I haven't received any feedback on it.

[musaffa]

@janko I'm planning upgrade my dry-rb, rom-rb and shrine dependencies at the end of the next month. Hopefully I'll be able to send you feedback at that time. Thanks for you amazing works!