Extend the storage adapters used for replaced derivative files

[jrgriffiniii]

Resolves #109 by ensuring that the storage adapter for derivatives is used for replaced, uploaded derivative files in FileSets

• [ ] Implement the Valkyrie.config.derivatives_adapter (This was introduced upstream; please see Update Valkyrie and use Valkyrie.config.derivatives_adapter #260 )
• Avoid passing more than one storage adapter to PlumChangeSetPersister
• Restructure the PlumChangeSetPersister to load the adapter from Controller files

[escowles]

This looks good, but seeing the repetitive changes made think about two small refactors that might be good:

1. Maybe we should have a Valkyrie.config.derivatives_adapter convenience method. This seems like common functionality that we should encourage a common pattern for.

2. If all the controllers are defining the same PlumChangeSetPersister with the same arguments, we could pull that into a superclass or module.

[jrgriffiniii]

@escowles thank you for this feedback. I can address this immediately.

If all the controllers are defining the same PlumChangeSetPersister with the same arguments, we could pull that into a superclass or module.

Understood, I had also considered attempting to structure the usage of the Valkyrie::StorageAdapter more polymorphic (in the Rails-centered usage of this term), but felt that this approach was more appropriate for an initial PR.

[escowles]

@jrgriffiniii Yep, given that we're shifting to Valkyrie proper today, feel free to punt on those, I just thought I'd suggest them since I figured you'd push an update with a new or updated test.

[tpendragon]

So I think that rubocop warning is valid (too many arguments is a problem.) We've also been pretty successful so far just passing the right storage adapter to the ChangeSetPersister. That extra branch feels like a magic conditional where I have to know which argument to pass in.

I wonder if we can just toggle which storage adapter we pass in at the controller layer?

[jrgriffiniii]

@tpendragon :

So I think that rubocop warning is valid (too many arguments is a problem.)

Understood, I can address this as well.

We've also been pretty successful so far just passing the right storage adapter to the ChangeSetPersister. That extra branch feels like a magic conditional where I have to know which argument to pass in.

I completely agree. I'm also not entirely comfortable given the complications which arise when taking into account the Jp2DerivativeService::Factory instantiation (https://github.com/pulibrary/figgy/blob/master/config/initializers/valkyrie.rb#L64)

[tpendragon]

I don't think this'll work in the case of replacing derivatives from a file upload in the FileSet controller. Can't we just check the adapter for the previously existing file in that controller action and use that to upload?

[jrgriffiniii]

I'm uncertain as to how this could be supported. @change_set is initialized from the PATCH parameters passed to the Controller (via resource_params):

    def update
      @change_set = change_set_class.new(find_resource(params[:id])).prepopulate!
      authorize! :update, @change_set.resource
      if @change_set.validate(resource_params)
        @change_set.sync
        obj = nil
        change_set_persister.buffer_into_index do |persist|
          obj = persist.save(change_set: @change_set)
        end

(https://github.com/pulibrary/figgy/blob/master/valhalla/app/controllers/concerns/valhalla/resource_controller.rb#L62)

    def resource_params
      params[resource_class.to_s.underscore.to_sym].to_unsafe_h
    end

(https://github.com/pulibrary/figgy/blob/master/valhalla/app/controllers/concerns/valhalla/resource_controller.rb#L90)

Example:

{"files"=>[{"473af163-d92c-4cb6-a036-63c0ce80a734"=>#<ActionDispatch::Http::UploadedFile:0x007fdc0aed0dd0 @tempfile=#<Tempfile:/var/folders/06/9nbfv2tx74jc0flpnq_mgz580000gp/T/RackMultipart20170927-472-4keg3b.jp2>, @original_filename="relax.jp2", @content_type="image/jp2", @headers="Content-Disposition: form-data; name=\"file_set[files[][473af163-d92c-4cb6-a036-63c0ce80a734]]\"; filename=\"test.jp2\"\r\nContent-Type: image/jp2\r\n">}]}

Because users can submit both master and derivative files into the same FileSetChangeSet, I would need to initialize a different ChangeSet for each of these files, with a separate storage adapter passed to new PlumChangeSetPersister's used for each of these ChangeSets.