Avoid unnecessary calls to Fedora when downloading thumbnails

* refactors DerivativePath to accept an id or object
* updates DownloadBehavoir to authorize assets using on the id

app/controllers/concerns/curation_concerns/application_controller_behavior.rb

@@ -7,16 +7,12 @@ module ApplicationControllerBehavior
     included do
       helper CurationConcerns::MainAppHelpers
 
-      rescue_from ActiveFedora::ObjectNotFoundError do |_exception|
-        respond_to do |wants|
-          wants.json { render_json_response(response_type: :not_found) }
-          # default to HTML response, even for other non-HTML formats we don't
-          # neccesarily know about, seems to be consistent with what Rails4 does
-          # by default with uncaught ActiveRecord::RecordNotFound in production
-          wants.any do
-            render_404
-          end
-        end
+      rescue_from ActiveFedora::ObjectNotFoundError do |exception|
+        not_found_response(exception)
+      end
+
+      rescue_from Blacklight::Exceptions::InvalidSolrID do |exception|
+        not_found_response(exception)
       end
     end
 
@@ -60,5 +56,19 @@ def render_json_response(response_type: :success, message: nil, options: {})
       json_body = CurationConcerns::API.generate_response_body(response_type: response_type, message: message, options: options)
       render json: json_body, status: response_type
     end
+
+    private
+
+      def not_found_response(_exception)
+        respond_to do |wants|
+          wants.json { render_json_response(response_type: :not_found) }
+          # default to HTML response, even for other non-HTML formats we don't
+          # neccesarily know about, seems to be consistent with what Rails4 does
+          # by default with uncaught ActiveRecord::RecordNotFound in production
+          wants.any do
+            render_404
+          end
+        end
+      end
   end
 end

app/controllers/concerns/curation_concerns/download_behavior.rb

@@ -35,10 +35,11 @@ def derivative_download_options
         { type: mime_type_for(file), disposition: 'inline' }
       end
 
-      # Customize the :download ability in your Ability class, or override this method
+      # Customize the :read ability in your Ability class, or override this method.
+      # Hydra::Ability#download_permissions can't be used in this case because it assumes
+      # that files are in a LDP basic container, and thus, included in the asset's uri.
       def authorize_download!
-        # authorize! :download, file # can't use this because Hydra::Ability#download_permissions assumes that files are in Basic Container (and thus include the asset's uri)
-        authorize! :read, asset
+        authorize! :read, params[asset_param_key]
       end
 
       # Overrides Hydra::Controller::DownloadBehavior#load_file, which is hard-coded to assume files are in BasicContainer.
@@ -51,7 +52,7 @@ def load_file
         file_reference = params[:file]
         return default_file unless file_reference
 
-        file_path = CurationConcerns::DerivativePath.derivative_path_for_reference(asset, file_reference)
+        file_path = CurationConcerns::DerivativePath.derivative_path_for_reference(params[asset_param_key], file_reference)
         File.exist?(file_path) ? file_path : nil
       end
 

app/services/curation_concerns/derivative_path.rb

@@ -1,49 +1,67 @@
 module CurationConcerns
   class DerivativePath
+    attr_reader :id, :destination_name
+
     class << self
       # Path on file system where derivative file is stored
+      # @param [ActiveFedora::Base or String] object either the AF object or its id
+      # @param [String] destintation_name
       def derivative_path_for_reference(object, destination_name)
-        destination_name = destination_name.gsub(/^original_file_/, '')
-        derivative_path(object, extension_for(destination_name), destination_name)
+        new(object, destination_name).derivative_path
       end
 
+      # @param [ActiveFedora::Base or String] object either the AF object or its id
       # @return [Array<String>] Array of paths to derivatives for this object.
       def derivatives_for_reference(object)
-        Dir.glob(root_path(object).join("*")).select do |path|
-          path.start_with?(path_prefix(object).to_s)
-        end
+        new(object).all_paths
       end
+    end
 
-      private
+    # @param [ActiveFedora::Base, String] object either the AF object or its id
+    # @param [String] destination_name
+    def initialize(object, destination_name = nil)
+      @id = object.is_a?(String) ? object : object.id
+      @destination_name = destination_name.gsub(/^original_file_/, '') if destination_name
+    end
 
-        # @param [#id] object Object whose ID is used to generate root path
-        # @return [String] Returns the root path where derivatives will be generated into.
-        def root_path(object)
-          Pathname.new(derivative_path(object, "", "")).dirname
-        end
+    def derivative_path
+      "#{path_prefix}-#{file_name}"
+    end
 
-        # @return <Pathname> Full prefix of the path for object.
-        def path_prefix(object)
-          Pathname.new(CurationConcerns.config.derivatives_path).join(pair_path(object.id))
-        end
+    def all_paths
+      Dir.glob(root_path.join("*")).select do |path|
+        path.start_with?(path_prefix.to_s)
+      end
+    end
 
-        def derivative_path(object, extension, destination_name)
-          file_name = destination_name + extension
-          "#{path_prefix(object)}-#{file_name}"
-        end
+    private
 
-        def pair_path(id)
-          id.split('').each_slice(2).map(&:join).join('/')
-        end
+      # @return [String] Returns the root path where derivatives will be generated into.
+      def root_path
+        Pathname.new(derivative_path).dirname
+      end
+
+      # @return <Pathname> Full prefix of the path for object.
+      def path_prefix
+        Pathname.new(CurationConcerns.config.derivatives_path).join(pair_path)
+      end
+
+      def pair_path
+        id.split('').each_slice(2).map(&:join).join('/')
+      end
+
+      def file_name
+        return unless destination_name
+        destination_name + extension
+      end
 
-        def extension_for(destination_name)
-          case destination_name
-          when 'thumbnail'
-            ".#{MIME::Types.type_for('jpg').first.extensions.first}"
-          else
-            ".#{destination_name}"
-          end
+      def extension
+        case destination_name
+        when 'thumbnail'
+          ".#{MIME::Types.type_for('jpg').first.extensions.first}"
+        else
+          ".#{destination_name}"
         end
-    end
+      end
   end
 end

spec/controllers/downloads_controller_spec.rb

@@ -29,6 +29,11 @@
         expect(response).to redirect_to new_user_session_path
         expect(flash['alert']).to eq 'You are not authorized to access this page.'
       end
+
+      it 'authorizes the resource using only the id' do
+        expect(controller).to receive(:authorize!).with(:read, file_set.id)
+        get :show, id: file_set.to_param
+      end
     end
 
     context "when the user has access" do
@@ -57,6 +62,11 @@
             expect(response.headers['Content-Length']).to eq "4218"
             expect(response.headers['Accept-Ranges']).to eq "bytes"
           end
+
+          it 'retrieves the thumbnail without contacting Fedora' do
+            expect(ActiveFedora::Base).not_to receive(:find).with(file_set.id)
+            get :show, id: file_set, file: 'thumbnail'
+          end
         end
 
         context "that isn't persisted" do

spec/services/derivative_path_spec.rb

@@ -1,21 +1,33 @@
 require 'spec_helper'
 
 describe CurationConcerns::DerivativePath do
-  before do
-    allow(CurationConcerns.config).to receive(:derivatives_path).and_return('tmp')
-  end
+  let(:id)     { '123' }
+  let(:object) { double(id: id) }
 
-  describe '.derivative_path_for_reference' do
-    subject { described_class.derivative_path_for_reference(object, destination_name) }
+  before { allow(CurationConcerns.config).to receive(:derivatives_path).and_return('tmp') }
 
-    let(:object) { double(id: '123') }
+  context "for a single path" do
     let(:destination_name) { 'thumbnail' }
 
-    it { is_expected.to eq 'tmp/12/3-thumbnail.jpeg' }
+    describe '.derivative_path_for_reference' do
+      subject { described_class.derivative_path_for_reference(object, destination_name) }
+      it { is_expected.to eq('tmp/12/3-thumbnail.jpeg') }
+    end
+
+    describe '#derivative_path' do
+      context "with an object" do
+        subject { described_class.new(object, destination_name).derivative_path }
+        it { is_expected.to eq('tmp/12/3-thumbnail.jpeg') }
+      end
+
+      context "with an id" do
+        subject { described_class.new(id, destination_name).derivative_path }
+        it { is_expected.to eq('tmp/12/3-thumbnail.jpeg') }
+      end
+    end
   end
 
-  describe "#derivatives_for_reference" do
-    subject { described_class.derivatives_for_reference(object) }
+  context "for multiple paths" do
     before do
       FileUtils.mkdir_p("tmp/12")
       File.open("tmp/12/3-thumbnail.jpeg", 'w') do |f|
@@ -29,12 +41,21 @@
       FileUtils.rm_rf("tmp/12")
     end
 
-    let(:object) { double(id: '123') }
+    describe ".derivatives_for_reference" do
+      subject { described_class.derivatives_for_reference(object) }
+      it { is_expected.to eq(["tmp/12/3-thumbnail.jpeg"]) }
+    end
 
-    it "lists all the paths to derivatives" do
-      expect(subject).to eq [
-        "tmp/12/3-thumbnail.jpeg"
-      ]
+    describe "#all_paths" do
+      context "with an object" do
+        subject { described_class.new(object, nil).all_paths }
+        it { is_expected.to eq(["tmp/12/3-thumbnail.jpeg"]) }
+      end
+
+      context "with an id" do
+        subject { described_class.new(id, nil).all_paths }
+        it { is_expected.to eq(["tmp/12/3-thumbnail.jpeg"]) }
+      end
     end
   end
 end