From 6afee2a99499bf38b82ae2a58a5dbca42aff9918 Mon Sep 17 00:00:00 2001 From: Jeremy Friesen Date: Thu, 5 Mar 2020 12:09:31 -0500 Subject: [PATCH 1/8] WIP - Adding Caching of Manifest Generation This commit is to push something up for conversation. I don't want to get too far in the solution without community feedback. There are quite a few things that I've discovered in this PR: 1) The routes for a `Hyrax::Test::SimpleWork` do not work. I know that @no_reply was working through this at the [Solar Vortex][1] but has not had space to get back to that work. 2) There is are presumptive metadata fields for a work show presenter. These metadata fields are not part of `Hyrax::Test::SimpleWork`. This is further complicated by the fact that we have [configurable IIIF fields][4] which are currently congruent with the assumptive metadata fields but likely inadequate going forward (see below). Given that the `Hyrax::WorksControllerBehavior#metadata` has previously been untested, I do think that what I've written maps to prior behavior with the ability to turn on caching. Below is a [samvera.slack.com#hyrax channel][2] discussion about the above presumptions. @jeremyf (20) > The Hyrax::Test::SimpleWork does not have the presumptive metadata > fields in its [SolrDocument][3] @no_reply > the existence of "presumptive metadata fields" here is problematic. > this is one of a couple of places where BasicMetadata is assumed, > even though it's not supposed to be @jeremyf > The Hyrax::WorkShowPresenter has complications in its intersection > of presumptive metadata (as listed in the delegate) and variance of > [Hyrax.config.iiif_metadata_fields][4] [1]:https://wiki.lyrasis.org/display/samvera/Developer+Congress+-+Solar+Vortex+-+January+2020 [2]:https://samvera.slack.com/archives/CA8ANGLEL/p1583447771028100 [3]:https://github.com/samvera/hyrax/blob/f102e0c43c90baa0efe44f6127134429edf34d51/app/presenters/hyrax/work_show_presenter.rb#L38-L42 [4]:https://github.com/samvera/hyrax/blob/f102e0c43c90baa0efe44f6127134429edf34d51/lib/hyrax/configuration.rb#L447-L453 --- .../hyrax/works_controller_behavior.rb | 11 +-- .../hyrax/manifest_builder_service.rb | 93 +++++++++++++++++++ config/features.rb | 4 + lib/hyrax/configuration.rb | 10 ++ spec/lib/hyrax/configuration_spec.rb | 2 + spec/models/flipflop_spec.rb | 11 +++ .../hyrax/manifest_builder_service_spec.rb | 40 ++++++++ 7 files changed, 165 insertions(+), 6 deletions(-) create mode 100644 app/services/hyrax/manifest_builder_service.rb create mode 100644 spec/services/hyrax/manifest_builder_service_spec.rb diff --git a/app/controllers/concerns/hyrax/works_controller_behavior.rb b/app/controllers/concerns/hyrax/works_controller_behavior.rb index f6a311b173..34d2bcf44d 100644 --- a/app/controllers/concerns/hyrax/works_controller_behavior.rb +++ b/app/controllers/concerns/hyrax/works_controller_behavior.rb @@ -127,9 +127,12 @@ def inspect_work def manifest headers['Access-Control-Allow-Origin'] = '*' + + json = Hyrax::ManifestBuilderService.as_json(presenter: presenter) + respond_to do |wants| - wants.json { render json: manifest_builder.to_h } - wants.html { render json: manifest_builder.to_h } + wants.json { render json: json } + wants.html { render json: json } end end @@ -157,10 +160,6 @@ def build_form @form = work_form_service.build(curation_concern, current_ability, self) end - def manifest_builder - ::IIIFManifest::ManifestFactory.new(presenter) - end - def actor @actor ||= Hyrax::CurationConcern.actor end diff --git a/app/services/hyrax/manifest_builder_service.rb b/app/services/hyrax/manifest_builder_service.rb new file mode 100644 index 0000000000..f77c993065 --- /dev/null +++ b/app/services/hyrax/manifest_builder_service.rb @@ -0,0 +1,93 @@ +require 'iiif_manifest' + +module Hyrax + # A class responsible for converting a Hyrax::Work like thing into a IIIF + # manifest. + # + # @see !{.as_json} + class ManifestBuilderService + # @api public + # + # @param presenter [Hyrax::WorkShowPresenter] the work presenter from which + # we'll build a manifest. + # @param iiif_manifest_factory [Class] a class that initializes with presenter + # object and returns an object that responds to `#to_h` + # @param expires_in [Integer] the number of seconds until the cache expires + # @param cache_manifest [Boolean] should we perform Rails level caching of + # the generated manifest + # + # @note While the :presenter may be a Hyrax::WorkShowPresenter it is likely + # defined by Hyrax::WorksControllerBehavior.show_presenter + # + # @note Why the public class method? It is easier to perform stubs in + # controller specs when you are calling a single method instead of a + # chain of methods. + # + # @return [Hash] a Ruby hash representation of a IIIF manifest document + # + # @see Hyrax::Configuration#iiif_manifest_cache_duration + # @see Hyrax::WorksControllerBehavior + def self.as_json(presenter:, iiif_manifest_factory: ::IIIFManifest::ManifestFactory, cache_manifest: Flipflop.cache_work_iiif_manifest?, expires_in: Hyrax.config.iiif_manifest_cache_duration) + new( + presenter: presenter, + iiif_manifest_factory: iiif_manifest_factory, + cache_manifest: cache_manifest, + expires_in: expires_in + ).as_json + end + + # @api private + def initialize(presenter:, iiif_manifest_factory:, cache_manifest:, expires_in:) + @presenter = presenter + @manifest_builder = iiif_manifest_factory.new(@presenter) + @expires_in = expires_in + @cache_manifest = cache_manifest + end + + attr_reader :presenter, :manifest_builder, :expires_in, :cache_manifest + alias cache_manifest? cache_manifest + + # @api private + # + # @return [Hash] a Ruby hash representation of a IIIF manifest document + def as_json + if cache_manifest? + Rails.cache.fetch(manifest_cache_key, expires_in: expires_in) do + sanitized_manifest + end + else + sanitized_manifest + end + end + + private + + def sanitized_manifest + # TODO: Do we really need to both convert to a JSON document then parse + # that document? + hash = JSON.parse(manifest_builder.to_h.to_json) + + hash['label'] = sanitize_value(hash['label']) if hash.key?('label') + hash['description'] = hash['description']&.collect { |elem| sanitize_value(elem) } if hash.key?('description') + + hash['sequences']&.each do |sequence| + sequence['canvases']&.each do |canvas| + canvas['label'] = sanitize_value(canvas['label']) + end + end + hash + end + + def sanitize_value(text) + Loofah.fragment(text.to_s).scrub!(:prune).to_s + end + + # By adding the Solr '_version_' field to the cache key, we shouldn't + # run into the problem of fetching an outdated version of the manifest. + # + # @return [String] + def manifest_cache_key + "#{presenter.id}/#{presenter.solr_document['_version_']}" + end + end +end diff --git a/config/features.rb b/config/features.rb index b0cee86286..b2589cd6cf 100644 --- a/config/features.rb +++ b/config/features.rb @@ -42,4 +42,8 @@ feature :hide_users_list, default: true, description: "Do not show users list unless user has authenticated." + + feature :cache_work_iiif_manifest, + default: false, + description: "Use Rails.cache to cache the JSON document for IIIF manifests" end diff --git a/lib/hyrax/configuration.rb b/lib/hyrax/configuration.rb index 23d0d67926..3c63813317 100644 --- a/lib/hyrax/configuration.rb +++ b/lib/hyrax/configuration.rb @@ -431,6 +431,16 @@ def iiif_metadata_fields end attr_writer :iiif_metadata_fields + # Duration in which we should cache the generated IIIF manifest. + # Default is 30 days (in seconds). + # + # @return [Integer] number of seconds + # @see https://api.rubyonrails.org/classes/ActiveSupport/Cache/Store.html#method-i-fetch + def iiif_manifest_cache_duration + @iiif_manifest_cache_duration ||= 30.days.to_i + end + attr_writer :iiif_manifest_cache_duration + # Should a button with "Share my work" show on the front page to users who are not logged in? attr_writer :display_share_button_when_not_logged_in def display_share_button_when_not_logged_in? diff --git a/spec/lib/hyrax/configuration_spec.rb b/spec/lib/hyrax/configuration_spec.rb index ccb8aa3c6a..3cee6857f5 100644 --- a/spec/lib/hyrax/configuration_spec.rb +++ b/spec/lib/hyrax/configuration_spec.rb @@ -54,6 +54,8 @@ it { is_expected.to respond_to(:iiif_image_size_default=) } it { is_expected.to respond_to(:iiif_metadata_fields) } it { is_expected.to respond_to(:iiif_metadata_fields=) } + it { is_expected.to respond_to(:iiif_manifest_cache_duration) } + it { is_expected.to respond_to(:iiif_manifest_cache_duration=) } it { is_expected.to respond_to(:libreoffice_path) } it { is_expected.to respond_to(:license_service_class) } it { is_expected.to respond_to(:license_service_class=) } diff --git a/spec/models/flipflop_spec.rb b/spec/models/flipflop_spec.rb index 6d195b0cfe..e391e5c27b 100644 --- a/spec/models/flipflop_spec.rb +++ b/spec/models/flipflop_spec.rb @@ -46,4 +46,15 @@ is_expected.to be true end end + + describe "cache_work_iiif_manifest?" do + subject { described_class.cache_work_iiif_manifest? } + + # This was the previous behavior. At a certain point, we'll likely + # flip the default behavior to `true`. But for now, the goal is to + # not introduce a code change + it "defaults to false" do + is_expected.to be false + end + end end diff --git a/spec/services/hyrax/manifest_builder_service_spec.rb b/spec/services/hyrax/manifest_builder_service_spec.rb new file mode 100644 index 0000000000..4870b49de0 --- /dev/null +++ b/spec/services/hyrax/manifest_builder_service_spec.rb @@ -0,0 +1,40 @@ +require 'hyrax/specs/shared_specs' + +RSpec.describe Hyrax::ManifestBuilderService, :clean_repo do + # Presenters requires a whole lot of context and therefore a whole lot of preamble + let(:id) { "123" } + let(:manifest_url) { File.join("https://samvera.org", "show", id) } + let(:solr_document) { { "_version_" => 1 } } + let(:work_presenter) { double("Work Presenter") } + let(:file_set_presenter) { double("File Set Presenter", id: "456") } + let(:presenter) do + double( + 'Presenter', + id: id, + solr_document: solr_document, + work_presenters: [work_presenter], + manifest_url: manifest_url, + description: ["A Treatise on Coding in Samvera"], + file_set_presenters: [file_set_presenter] + ) + end + subject { described_class.as_json(presenter: presenter, cache_manifest: cache_manifest) } + + describe ".as_json" do + context 'with cache_manifest == true' do + let(:cache_manifest) { true } + it "will be a Ruby hash" do + expect(Rails.cache).to receive(:fetch).and_yield + expect(subject).to be_a(Hash) + end + end + + context 'with cache_manifest == false' do + let(:cache_manifest) { false } + it "will be a Ruby hash" do + expect(Rails.cache).not_to receive(:fetch) + expect(subject).to be_a(Hash) + end + end + end +end From 70b26e1af759cee778bfb302d4ebd7acf4deccd8 Mon Sep 17 00:00:00 2001 From: tom johnson Date: Fri, 1 May 2020 10:52:53 -0700 Subject: [PATCH 2/8] make `WorksControllerBehavior`'s manifest builder class configurable add `WorksControllerBehavior.iiif_manifest_builder` and a private `WorksControllerBehavior#iiif_manifest_builder`. these must return an object that responds to `#as_json(presenter:)`. --- .../concerns/hyrax/works_controller_behavior.rb | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-) diff --git a/app/controllers/concerns/hyrax/works_controller_behavior.rb b/app/controllers/concerns/hyrax/works_controller_behavior.rb index 34d2bcf44d..839e895435 100644 --- a/app/controllers/concerns/hyrax/works_controller_behavior.rb +++ b/app/controllers/concerns/hyrax/works_controller_behavior.rb @@ -10,10 +10,11 @@ module WorksControllerBehavior with_themed_layout :decide_layout copy_blacklight_config_from(::CatalogController) - class_attribute :_curation_concern_type, :show_presenter, :work_form_service, :search_builder_class + class_attribute :_curation_concern_type, :show_presenter, :work_form_service, :search_builder_class, :iiif_manifest_builder self.show_presenter = Hyrax::WorkShowPresenter self.work_form_service = Hyrax::WorkFormService self.search_builder_class = WorkSearchBuilder + self.iiif_manifest_builder = Hyrax::ManifestBuilderService attr_accessor :curation_concern helper_method :curation_concern, :contextual_path @@ -128,7 +129,7 @@ def inspect_work def manifest headers['Access-Control-Allow-Origin'] = '*' - json = Hyrax::ManifestBuilderService.as_json(presenter: presenter) + json = iiif_manifest_builder.as_json(presenter: presenter) respond_to do |wants| wants.json { render json: json } @@ -138,6 +139,10 @@ def manifest private + def iiif_manifest_builder + self.class.iiif_manifest_builder + end + def user_collections collections_service.search_results(:deposit) end From fe3952ecbd87614c976af263df0a8c06f6971d8f Mon Sep 17 00:00:00 2001 From: tom johnson Date: Fri, 1 May 2020 13:13:04 -0700 Subject: [PATCH 3/8] remove unneeded require in manifest_builder_service_spec.rb this test doesn't need the Hyrax shared specs, so don't bother loading them. --- spec/services/hyrax/manifest_builder_service_spec.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/spec/services/hyrax/manifest_builder_service_spec.rb b/spec/services/hyrax/manifest_builder_service_spec.rb index 4870b49de0..0826264c0a 100644 --- a/spec/services/hyrax/manifest_builder_service_spec.rb +++ b/spec/services/hyrax/manifest_builder_service_spec.rb @@ -1,4 +1,4 @@ -require 'hyrax/specs/shared_specs' +# frozen_string_literal: true RSpec.describe Hyrax::ManifestBuilderService, :clean_repo do # Presenters requires a whole lot of context and therefore a whole lot of preamble From 1c500b1d1b5d32dc1e7520efb134fb13397db3ff Mon Sep 17 00:00:00 2001 From: tom johnson Date: Fri, 1 May 2020 14:55:15 -0700 Subject: [PATCH 4/8] refactor `WorksControllerBehavior.iiif_manifest_builder` use a more OO style for the dependency injection on `iiif_manifest_builder`. makes the caching behavior a subclass, instead of a branch, and places the configuration in the controller. this should make the manifest builders more reusable for other controllers, which might decide on caching behavior in a different way. renames `#as_json` (which used to return a hash!) to `#manifest_for` and usees the instance method in callers. --- .../hyrax/works_controller_behavior.rb | 4 +- .../hyrax/caching_iiif_manifest_builder.rb | 44 ++++++++++ .../hyrax/manifest_builder_service.rb | 87 +++++++++---------- .../hyrax/generic_works_controller_spec.rb | 8 ++ .../caching_iiif_manifest_builder_spec.rb | 28 ++++++ .../hyrax/manifest_builder_service_spec.rb | 19 +--- 6 files changed, 128 insertions(+), 62 deletions(-) create mode 100644 app/services/hyrax/caching_iiif_manifest_builder.rb create mode 100644 spec/services/hyrax/caching_iiif_manifest_builder_spec.rb diff --git a/app/controllers/concerns/hyrax/works_controller_behavior.rb b/app/controllers/concerns/hyrax/works_controller_behavior.rb index 839e895435..4113fc8365 100644 --- a/app/controllers/concerns/hyrax/works_controller_behavior.rb +++ b/app/controllers/concerns/hyrax/works_controller_behavior.rb @@ -14,7 +14,7 @@ module WorksControllerBehavior self.show_presenter = Hyrax::WorkShowPresenter self.work_form_service = Hyrax::WorkFormService self.search_builder_class = WorkSearchBuilder - self.iiif_manifest_builder = Hyrax::ManifestBuilderService + self.iiif_manifest_builder = (Flipflop.cache_work_iiif_manifest? ? Hyrax::CachingIiifManifestBuilder.new : Hyrax::ManifestBuilderService.new) attr_accessor :curation_concern helper_method :curation_concern, :contextual_path @@ -129,7 +129,7 @@ def inspect_work def manifest headers['Access-Control-Allow-Origin'] = '*' - json = iiif_manifest_builder.as_json(presenter: presenter) + json = iiif_manifest_builder.manifest_for(presenter: presenter) respond_to do |wants| wants.json { render json: json } diff --git a/app/services/hyrax/caching_iiif_manifest_builder.rb b/app/services/hyrax/caching_iiif_manifest_builder.rb new file mode 100644 index 0000000000..4e3bbdcfe2 --- /dev/null +++ b/app/services/hyrax/caching_iiif_manifest_builder.rb @@ -0,0 +1,44 @@ +# frozen_string_literal: true + +module Hyrax + ## + # constructs IIIF Manifests and holds them in the Rails cache, + # this approach avoids long manifest build times for some kinds of requests, + # at the cost of introducing cache invalidation issues. + class CachingIiifManifestBuilder < ManifestBuilderService + attr_accessor :expires_in + + ## + # @api public + # + # @param iiif_manifest_factory [Class] a class that initializes with presenter + # object and returns an object that responds to `#to_h` + # @param expires_in [Integer] the number of seconds until the cache expires + # @see Hyrax::Configuration#iiif_manifest_cache_duration + def initialize(iiif_manifest_factory: ::IIIFManifest::ManifestFactory, expires_in: Hyrax.config.iiif_manifest_cache_duration) + self.expires_in = expires_in + + super(iiif_manifest_factory: iiif_manifest_factory) + end + + ## + # @see ManifestBuilderService#as_json + def manifest_for(presenter:) + Rails.cache.fetch(manifest_cache_key(presenter: presenter), expires_in: expires_in) do + super + end + end + + private + + ## + # By adding the Solr '_version_' field to the cache key, we shouldn't + # run into the problem of fetching an outdated version of the manifest. + # @param presenter [Hyrax::WorkShowPresenter] + # + # @return [String] + def manifest_cache_key(presenter:) + "#{presenter.id}/#{presenter.solr_document['_version_']}" + end + end +end diff --git a/app/services/hyrax/manifest_builder_service.rb b/app/services/hyrax/manifest_builder_service.rb index f77c993065..f234cb9385 100644 --- a/app/services/hyrax/manifest_builder_service.rb +++ b/app/services/hyrax/manifest_builder_service.rb @@ -1,71 +1,71 @@ +# frozen_string_literal: true + require 'iiif_manifest' module Hyrax + ## # A class responsible for converting a Hyrax::Work like thing into a IIIF # manifest. # # @see !{.as_json} class ManifestBuilderService + ## # @api public # # @param presenter [Hyrax::WorkShowPresenter] the work presenter from which # we'll build a manifest. # @param iiif_manifest_factory [Class] a class that initializes with presenter # object and returns an object that responds to `#to_h` - # @param expires_in [Integer] the number of seconds until the cache expires - # @param cache_manifest [Boolean] should we perform Rails level caching of - # the generated manifest # # @note While the :presenter may be a Hyrax::WorkShowPresenter it is likely # defined by Hyrax::WorksControllerBehavior.show_presenter # - # @note Why the public class method? It is easier to perform stubs in - # controller specs when you are calling a single method instead of a - # chain of methods. - # # @return [Hash] a Ruby hash representation of a IIIF manifest document # - # @see Hyrax::Configuration#iiif_manifest_cache_duration # @see Hyrax::WorksControllerBehavior - def self.as_json(presenter:, iiif_manifest_factory: ::IIIFManifest::ManifestFactory, cache_manifest: Flipflop.cache_work_iiif_manifest?, expires_in: Hyrax.config.iiif_manifest_cache_duration) - new( - presenter: presenter, - iiif_manifest_factory: iiif_manifest_factory, - cache_manifest: cache_manifest, - expires_in: expires_in - ).as_json + def self.manifest_for(presenter:, iiif_manifest_factory: ::IIIFManifest::ManifestFactory) + new(iiif_manifest_factory: iiif_manifest_factory) + .manifest_for(presenter: presenter) end - # @api private - def initialize(presenter:, iiif_manifest_factory:, cache_manifest:, expires_in:) - @presenter = presenter - @manifest_builder = iiif_manifest_factory.new(@presenter) - @expires_in = expires_in - @cache_manifest = cache_manifest - end + ## + # @!attribute [r] manifest_factory + # @return [#to_h] + attr_reader :manifest_factory - attr_reader :presenter, :manifest_builder, :expires_in, :cache_manifest - alias cache_manifest? cache_manifest + ## + # @api public + # + # @param iiif_manifest_factory [Class] a class that initializes with presenter + # object and returns an object that responds to `#to_h` + def initialize(iiif_manifest_factory: ::IIIFManifest::ManifestFactory) + @manifest_factory = iiif_manifest_factory + end - # @api private + ## + # @api public + # + # @param presenter [Hyrax::WorkShowPresenter] # # @return [Hash] a Ruby hash representation of a IIIF manifest document - def as_json - if cache_manifest? - Rails.cache.fetch(manifest_cache_key, expires_in: expires_in) do - sanitized_manifest - end - else - sanitized_manifest - end + def manifest_for(presenter:) + sanitized_manifest(presenter: presenter) end private - def sanitized_manifest - # TODO: Do we really need to both convert to a JSON document then parse - # that document? - hash = JSON.parse(manifest_builder.to_h.to_json) + ## + # @api private + # @param presenter [Hyrax::WorkShowPresenter] + def sanitized_manifest(presenter:) + # ::IIIFManifest::ManifestBuilder#to_h returns a + # IIIFManifest::ManifestBuilder::IIIFManifest, not a Hash. + # to get a Hash, we have to call its #to_json, then parse. + # + # wild times. maybe there's a better way to do this with the + # ManifestFactory interface? + manifest = manifest_factory.new(presenter).to_h + hash = JSON.parse(manifest.to_json) hash['label'] = sanitize_value(hash['label']) if hash.key?('label') hash['description'] = hash['description']&.collect { |elem| sanitize_value(elem) } if hash.key?('description') @@ -75,19 +75,16 @@ def sanitized_manifest canvas['label'] = sanitize_value(canvas['label']) end end + hash end + ## + # @api private + # @param [#to_s] text + # @return [String] a sanitized verison of `text` def sanitize_value(text) Loofah.fragment(text.to_s).scrub!(:prune).to_s end - - # By adding the Solr '_version_' field to the cache key, we shouldn't - # run into the problem of fetching an outdated version of the manifest. - # - # @return [String] - def manifest_cache_key - "#{presenter.id}/#{presenter.solr_document['_version_']}" - end end end diff --git a/spec/controllers/hyrax/generic_works_controller_spec.rb b/spec/controllers/hyrax/generic_works_controller_spec.rb index 35e9977134..175df7a5c6 100644 --- a/spec/controllers/hyrax/generic_works_controller_spec.rb +++ b/spec/controllers/hyrax/generic_works_controller_spec.rb @@ -609,6 +609,14 @@ .and_return(manifest_factory) end + it 'uses the configured service' do + custom_builder = double(manifest_for: { test: 'cached manifest' }) + allow(described_class).to receive(:iiif_manifest_builder).and_return(custom_builder) + + get :manifest, params: { id: work, format: :json } + expect(response.body).to eq "{\"test\":\"cached manifest\"}" + end + it "produces a manifest for a json request" do get :manifest, params: { id: work, format: :json } expect(response.body).to eq "{\"test\":\"manifest\"}" diff --git a/spec/services/hyrax/caching_iiif_manifest_builder_spec.rb b/spec/services/hyrax/caching_iiif_manifest_builder_spec.rb new file mode 100644 index 0000000000..982efd67c8 --- /dev/null +++ b/spec/services/hyrax/caching_iiif_manifest_builder_spec.rb @@ -0,0 +1,28 @@ +# frozen_string_literal: true + +RSpec.describe Hyrax::CachingIiifManifestBuilder, :clean_repo do + let(:id) { "123" } + let(:manifest_url) { File.join("https://samvera.org", "show", id) } + let(:solr_document) { { "_version_" => 1 } } + let(:work_presenter) { double("Work Presenter") } + let(:file_set_presenter) { double("File Set Presenter", id: "456") } + let(:presenter) do + double( + 'Presenter', + id: id, + solr_document: solr_document, + work_presenters: [work_presenter], + manifest_url: manifest_url, + description: ["A Treatise on Coding in Samvera"], + file_set_presenters: [file_set_presenter] + ) + end + + subject(:builder) { described_class.new } + + it 'hits the cache' do + expect(Rails.cache).to receive(:fetch).and_yield + + builder.manifest_for(presenter: presenter) + end +end diff --git a/spec/services/hyrax/manifest_builder_service_spec.rb b/spec/services/hyrax/manifest_builder_service_spec.rb index 0826264c0a..909c0872ec 100644 --- a/spec/services/hyrax/manifest_builder_service_spec.rb +++ b/spec/services/hyrax/manifest_builder_service_spec.rb @@ -18,23 +18,12 @@ file_set_presenters: [file_set_presenter] ) end - subject { described_class.as_json(presenter: presenter, cache_manifest: cache_manifest) } + subject { described_class.manifest_for(presenter: presenter) } describe ".as_json" do - context 'with cache_manifest == true' do - let(:cache_manifest) { true } - it "will be a Ruby hash" do - expect(Rails.cache).to receive(:fetch).and_yield - expect(subject).to be_a(Hash) - end - end - - context 'with cache_manifest == false' do - let(:cache_manifest) { false } - it "will be a Ruby hash" do - expect(Rails.cache).not_to receive(:fetch) - expect(subject).to be_a(Hash) - end + it "will be a Ruby hash" do + expect(Rails.cache).not_to receive(:fetch) + expect(subject).to be_a(Hash) end end end From 1479582312f685d4973739ca668b777afec9121a Mon Sep 17 00:00:00 2001 From: tom johnson Date: Tue, 12 May 2020 19:24:40 -0700 Subject: [PATCH 5/8] require `iiif_manifest` factory on startup, not from `app/` we're going to end up requiring `iiif_manifest` so we might as well have it available application wide. putting the `require` in app only seems to muddy the waters. --- app/services/hyrax/manifest_builder_service.rb | 2 -- lib/hyrax.rb | 1 + 2 files changed, 1 insertion(+), 2 deletions(-) diff --git a/app/services/hyrax/manifest_builder_service.rb b/app/services/hyrax/manifest_builder_service.rb index f234cb9385..e340e345ed 100644 --- a/app/services/hyrax/manifest_builder_service.rb +++ b/app/services/hyrax/manifest_builder_service.rb @@ -1,7 +1,5 @@ # frozen_string_literal: true -require 'iiif_manifest' - module Hyrax ## # A class responsible for converting a Hyrax::Work like thing into a IIIF diff --git a/lib/hyrax.rb b/lib/hyrax.rb index ecbf1953ec..7165e8030b 100644 --- a/lib/hyrax.rb +++ b/lib/hyrax.rb @@ -8,6 +8,7 @@ require 'tinymce-rails' require 'blacklight' require 'blacklight/gallery' +require 'iiif_manifest' require 'noid-rails' require 'hydra/head' require 'hydra-editor' From b6e7f956f72008816abf4dfcc61a04f0f9cc6860 Mon Sep 17 00:00:00 2001 From: tom johnson Date: Sun, 17 May 2020 14:30:51 -0700 Subject: [PATCH 6/8] use a specialized `IiifManifestPresenter` for manifest generation this complete rewrite of the manifest generation process reduces the footprint of database (Fedora/Solr) calls involved in the manifest generation process, fixes a bug in sequence/canvas generation for manifests for nested works, and generally separates the issue of manifest generation from the `WorkShowPresenter` (existing code is left in place, to be deprecated and removed as follow-up). [`iiif_manifest`](https://www.rubydoc.info/gems/iiif_manifest/1.0.1) expects a "presenter", with `#manifest_url`, `#manifest_metadata` `#description`, `#sequence_rendering`, and `#work_presenters`/`#file_set_presenters`. since we know the presenter provided will be used for the duration of a single request, we can make heavy use of instance variables to cache children and avoid making multiple requests for any particular data. the rewrite also does some work to help clarify the two peices of information the presenter needs in the request context: the `Ability` and the hostname to use for URIs within the manifest. we provide naive defaults for both of these, and implement the behavior more or less without regard for their context---i.e. the whole presenter infrastructure works without them. when calling from a Controller, we pass them in explicitly to augment the behavior for the context. this is important because we may (do) want to reuse these presenters in other contexts (a Job for cache pre-warming) where this data isn't available. the presenters themselves use `Draper::Decorator`'s `delegate_all`, so they're basically glorified wrappers for a model. they don't care whether the model is a SolrDocument, or powered by a primary database. a small amount of work is done to align Valkyrie, ActiveFedora, and SolrDocument-style models, but mostly things were already interoperable. a refactor in `DisplaysImage` rearranges some instance-level caching there, for easier reuse in the new presenter infrastructure. the short story is: between this and a previous optimization in `DisplaysImage` (#4344), manifest generation in a makeshift controlled benchmarking environment sped up 10-fold for works of modest size. it seems roughly linear between 10 and 750 child FileSets. i'm not confident enough in that environment (especially its behavior WRT caching) to call this "data" or to provide anything more solid than this statement, but there's good reason to believe this will be quite a bit faster/more scalable than the status-quo. --- .../hyrax/works_controller_behavior.rb | 9 +- .../concerns/hyrax/solr_document/metadata.rb | 1 + .../concerns/hyrax/solr_document_behavior.rb | 10 + app/presenters/hyrax/displays_image.rb | 46 ++-- .../hyrax/iiif_manifest_presenter.rb | 222 ++++++++++++++++++ .../hyrax/manifest_builder_service.rb | 2 +- .../hyrax/generic_works_controller_spec.rb | 2 +- spec/factories/file_sets.rb | 4 + spec/factories/generic_works.rb | 4 + spec/features/iiif_manifest_spec.rb | 53 +++++ .../hyrax/iiif_manifest_presenter_spec.rb | 174 ++++++++++++++ .../file_set_fixity_check_service_spec.rb | 2 +- 12 files changed, 504 insertions(+), 25 deletions(-) create mode 100644 app/presenters/hyrax/iiif_manifest_presenter.rb create mode 100644 spec/features/iiif_manifest_spec.rb create mode 100644 spec/presenters/hyrax/iiif_manifest_presenter_spec.rb diff --git a/app/controllers/concerns/hyrax/works_controller_behavior.rb b/app/controllers/concerns/hyrax/works_controller_behavior.rb index 4113fc8365..a150ca52aa 100644 --- a/app/controllers/concerns/hyrax/works_controller_behavior.rb +++ b/app/controllers/concerns/hyrax/works_controller_behavior.rb @@ -129,7 +129,7 @@ def inspect_work def manifest headers['Access-Control-Allow-Origin'] = '*' - json = iiif_manifest_builder.manifest_for(presenter: presenter) + json = iiif_manifest_builder.manifest_for(presenter: iiif_manifest_presenter) respond_to do |wants| wants.json { render json: json } @@ -143,6 +143,13 @@ def iiif_manifest_builder self.class.iiif_manifest_builder end + def iiif_manifest_presenter + IiifManifestPresenter.new(curation_concern_from_search_results).tap do |p| + p.hostname = request.hostname + p.ability = current_ability + end + end + def user_collections collections_service.search_results(:deposit) end diff --git a/app/models/concerns/hyrax/solr_document/metadata.rb b/app/models/concerns/hyrax/solr_document/metadata.rb index bde264dcac..9673d8e8da 100644 --- a/app/models/concerns/hyrax/solr_document/metadata.rb +++ b/app/models/concerns/hyrax/solr_document/metadata.rb @@ -54,6 +54,7 @@ def self.coerce(input) attribute :read_groups, Solr::Array, ::Ability.read_group_field attribute :collection_ids, Solr::Array, 'collection_ids_tesim' attribute :admin_set, Solr::Array, solr_name('admin_set') + attribute :member_ids, Solr::Array, "member_ids_ssim" attribute :member_of_collection_ids, Solr::Array, solr_name('member_of_collection_ids', :symbol) attribute :description, Solr::Array, solr_name('description') attribute :title, Solr::Array, solr_name('title') diff --git a/app/models/concerns/hyrax/solr_document_behavior.rb b/app/models/concerns/hyrax/solr_document_behavior.rb index 471e4d7cc8..05bd3646d6 100644 --- a/app/models/concerns/hyrax/solr_document_behavior.rb +++ b/app/models/concerns/hyrax/solr_document_behavior.rb @@ -61,10 +61,20 @@ def to_model @model ||= ModelWrapper.new(hydra_model, id) end + ## + # @return [Boolean] def collection? hydra_model == ::Collection end + ## + # @return [Boolean] + def file_set? + hydra_model == ::FileSet + end + + ## + # @return [Boolean] def admin_set? hydra_model == ::AdminSet end diff --git a/app/presenters/hyrax/displays_image.rb b/app/presenters/hyrax/displays_image.rb index a4ad16e787..1303a9049a 100644 --- a/app/presenters/hyrax/displays_image.rb +++ b/app/presenters/hyrax/displays_image.rb @@ -11,20 +11,11 @@ module DisplaysImage # @return [IIIFManifest::DisplayImage] the display image required by the manifest builder. def display_image return nil unless solr_document.image? && current_ability.can?(:read, solr_document) - - latest_file_id = lookup_original_file_id - return nil unless latest_file_id - url = Hyrax.config.iiif_image_url_builder.call( - latest_file_id, - request.base_url, - Hyrax.config.iiif_image_size_default - ) - # @see https://github.com/samvera-labs/iiif_manifest - IIIFManifest::DisplayImage.new(url, - format: image_format([]), + IIIFManifest::DisplayImage.new(display_image_url(request.base_url), + format: image_format(alpha_channels), width: width, height: height, iiif_endpoint: iiif_endpoint(latest_file_id)) @@ -32,16 +23,24 @@ def display_image private - def iiif_endpoint(file_id) + def display_image_url(base_url) + Hyrax.config.iiif_image_url_builder.call( + latest_file_id, + base_url, + Hyrax.config.iiif_image_size_default + ) + end + + def iiif_endpoint(file_id, base_url: request.base_url) return unless Hyrax.config.iiif_image_server? IIIFManifest::IIIFEndpoint.new( - Hyrax.config.iiif_info_url_builder.call(file_id, request.base_url), + Hyrax.config.iiif_info_url_builder.call(file_id, base_url), profile: Hyrax.config.iiif_image_compliance_level_uri ) end def image_format(channels) - channels.find { |c| c.include?('rgba') }.nil? ? 'jpg' : 'png' + channels&.find { |c| c.include?('rgba') }.nil? ? 'jpg' : 'png' end def unindexed_current_file_version @@ -49,13 +48,18 @@ def unindexed_current_file_version ActiveFedora::File.uri_to_id(::FileSet.find(id).current_content_version_uri) end - def lookup_original_file_id - result = original_file_id - if result.blank? - Rails.logger.warn "original_file_id for #{id} not found, falling back to Fedora." - result = ActiveFedora::File.uri_to_id(::FileSet.find(id).current_content_version_uri) - end - result + def latest_file_id + @latest_file_id ||= + begin + result = original_file_id + + if result.blank? + Rails.logger.warn "original_file_id for #{id} not found, falling back to Fedora." + result = Hyrax::VersioningService.versioned_file_id ::FileSet.find(id).original_file + end + + result + end end end end diff --git a/app/presenters/hyrax/iiif_manifest_presenter.rb b/app/presenters/hyrax/iiif_manifest_presenter.rb new file mode 100644 index 0000000000..3131510869 --- /dev/null +++ b/app/presenters/hyrax/iiif_manifest_presenter.rb @@ -0,0 +1,222 @@ +# frozen_string_literal: true + +module Hyrax + ## + # This presenter wraps objects in the interface required by `IIIFManifiest`. + # It will accept either a Work-like resource or a SolrDocument. + # + # @example with a work + # + # monograph = Monograph.new + # presenter = IiifManifestPresenter.new(monograph) + # presenter.title # => [] + # + # monograph.title = ['Comet in Moominland'] + # presenter.title # => ['Comet in Moominland'] + # + # @see https://www.rubydoc.info/gems/iiif_manifest + class IiifManifestPresenter < Draper::Decorator + delegate_all + + ## + # @!attribute [w] ability + # @return [Ability] + # @!attribute [w] hostname + # @return [String] + attr_writer :ability, :hostname + + class << self + ## + # @param [Hyrax::Resource, SolrDocument] + def for(model) + klass = model.file_set? ? DisplayImagePresenter : IiifManifestPresenter + + klass.new(model) + end + end + + ## + # @return [#can?] + def ability + @ability ||= NullAbility.new + end + + ## + # @return [String] + def description + Array(super).first || '' + end + + ## + # @return [Boolean] + def file_set? + model.try(:file_set?) || Array(model[:has_model_ssim]).include?('FileSet') + end + + ## + # @return [Array] + def file_set_presenters + member_presenters.select(&:file_set?) + end + + ## + # IIIF metadata for inclusion in the manifest + # Called by the `iiif_manifest` gem to add metadata + # + # @todo should this use the simple_form i18n keys?! maybe the manifest + # needs its own? + # + # @return [Array String}>] array of metadata hashes + def manifest_metadata + metadata_fields.map do |field_name| + { + 'label' => I18n.t("simple_form.labels.defaults.#{field_name}"), + 'value' => Array(self[field_name]).map { |value| scrub(value.to_s) } + } + end + end + + ## + # @return [String] the URL where the manifest can be found + def manifest_url + return '' if id.blank? + + Rails.application.routes.url_helpers.polymorphic_url([:manifest, model], host: hostname) + end + + ## + # @return [Array<#to_s>] + def member_ids + Array(model.try(:member_ids)) + end + + ## + # @note cache member presenters to avoid querying repeatedly; we expect this + # presenter to live only as long as the request. + # + # @note skips presenters for objects the current `@ability` cannot read. + # the default ability has all permissions. + # + # @return [Array] + def member_presenters + @member_presenters_cache ||= Factory.build_for(ids: member_ids, presenter_class: self.class).map do |presenter| + next unless ability.can?(:read, presenter.model) + + presenter.hostname = hostname + presenter.ability = ability + presenter + end.compact + end + + ## + # @return [Array String}>] + def sequence_rendering + Array(try(:rendering_ids)).map do |file_set_id| + rendering = file_set_presenters.find { |p| p.id == file_set_id } + next unless rendering + + { '@id' => Hyrax::Engine.routes.url_helpers.download_url(rendering.id, host: hostname), + 'format' => rendering.mime_type.present? ? rendering.mime_type : I18n.t("hyrax.manifest.unknown_mime_text"), + 'label' => I18n.t("hyrax.manifest.download_text") + (rendering.label || '') } + end.flatten + end + + ## + # @return [Boolean] + def work? + object.try(:work?) || !file_set? + end + + ## + # @return [Array] + def work_presenters + member_presenters.select(&:work?) + end + + ## + # An Ability-like object that gives `true` for all `can?` requests + class NullAbility + ## + # @return [Boolean] true + def can?(*) + true + end + end + + class Factory < PresenterFactory + ## + # @return [Array] + def build + ids.map do |id| + solr_doc = load_docs.find { |doc| doc.id == id } + presenter_class.for(solr_doc) if solr_doc + end.compact + end + + private + + ## + # cache the docs in this method, rather than #build; + # this can probably be pushed up to the parent class + def load_docs + @cached_docs ||= super + end + end + + ## + # a Presenter for producing `IIIFManifest::DisplayImage` objects + # + class DisplayImagePresenter < Draper::Decorator + delegate_all + + include Hyrax::DisplaysImage + + ## + # @!attribute [w] ability + # @return [Ability] + # @!attribute [w] hostname + # @return [String] + attr_writer :ability, :hostname + + ## + # Creates a display image only where #model is an image. + # + # @return [IIIFManifest::DisplayImage] the display image required by the manifest builder. + def display_image + return nil unless model.image? + return nil unless latest_file_id + + IIIFManifest::DisplayImage + .new(display_image_url(hostname), + format: image_format(alpha_channels), + width: width, + height: height, + iiif_endpoint: iiif_endpoint(latest_file_id, base_url: hostname)) + end + + def hostname + @hostname || 'localhost' + end + + ## + # @return [Boolean] false + def work? + false + end + end + + private + + def hostname + @hostname || 'localhost' + end + + def metadata_fields + Hyrax.config.iiif_metadata_fields + end + + def scrub(value) + Loofah.fragment(value).scrub!(:whitewash).to_s + end + end +end diff --git a/app/services/hyrax/manifest_builder_service.rb b/app/services/hyrax/manifest_builder_service.rb index e340e345ed..355e11d111 100644 --- a/app/services/hyrax/manifest_builder_service.rb +++ b/app/services/hyrax/manifest_builder_service.rb @@ -66,7 +66,7 @@ def sanitized_manifest(presenter:) hash = JSON.parse(manifest.to_json) hash['label'] = sanitize_value(hash['label']) if hash.key?('label') - hash['description'] = hash['description']&.collect { |elem| sanitize_value(elem) } if hash.key?('description') + hash['description'] = Array(hash['description'])&.collect { |elem| sanitize_value(elem) } if hash.key?('description') hash['sequences']&.each do |sequence| sequence['canvases']&.each do |canvas| diff --git a/spec/controllers/hyrax/generic_works_controller_spec.rb b/spec/controllers/hyrax/generic_works_controller_spec.rb index 175df7a5c6..5423a6fff9 100644 --- a/spec/controllers/hyrax/generic_works_controller_spec.rb +++ b/spec/controllers/hyrax/generic_works_controller_spec.rb @@ -605,7 +605,7 @@ File.open(fixture_path + '/world.png'), :original_file) allow(IIIFManifest::ManifestFactory).to receive(:new) - .with(Hyrax::WorkShowPresenter) + .with(Hyrax::IiifManifestPresenter) .and_return(manifest_factory) end diff --git a/spec/factories/file_sets.rb b/spec/factories/file_sets.rb index b918c12491..e8ae22e04d 100644 --- a/spec/factories/file_sets.rb +++ b/spec/factories/file_sets.rb @@ -20,6 +20,10 @@ read_groups { ["registered"] } end + trait :image do + content { File.open(Hyrax::Engine.root + 'spec/fixtures/world.png') } + end + factory :file_with_work do after(:build) do |file, _evaluator| file.title = ['testfile'] diff --git a/spec/factories/generic_works.rb b/spec/factories/generic_works.rb index b696d36b4d..671e0059ed 100644 --- a/spec/factories/generic_works.rb +++ b/spec/factories/generic_works.rb @@ -59,6 +59,10 @@ before(:create) { |work, evaluator| 2.times { work.ordered_members << create(:file_set, user: evaluator.user) } } end + factory :work_with_image_files do + before(:create) { |work, evaluator| 2.times { work.ordered_members << create(:file_set, :image, user: evaluator.user) } } + end + factory :work_with_ordered_files do before(:create) do |work, evaluator| work.ordered_members << create(:file_set, user: evaluator.user) diff --git a/spec/features/iiif_manifest_spec.rb b/spec/features/iiif_manifest_spec.rb new file mode 100644 index 0000000000..2cd84ff199 --- /dev/null +++ b/spec/features/iiif_manifest_spec.rb @@ -0,0 +1,53 @@ +require 'rails_helper' + +RSpec.describe 'building a IIIF Manifest' do + let(:work) { create(:work, title: ['Comet in Moominland'], creator: ['Jansson, Tove'], description: ['a novel about moomins']) } + let(:member_works) { create_list(:work_with_image_files, 2, title: ['supplemental object'], creator: ['Author, Samantha'], description: ['supplemental materials']) } + let(:file_sets) { create_list(:file_set, 12, :image, title: ['page n'], creator: ['Jansson, Tove'], description: ['the nth page']) } + + let(:user) { create(:admin) } + + before do + work.ordered_members += file_sets + work.members += member_works + work.save + + sign_in user + end + + it 'gets a full manifest for the work' do + visit "/concern/generic_works/#{work.id}/manifest" + + # maybe validate this with https://github.com/IIIF/presentation-validator/blob/master/schema/iiif_3_0.json ? + manifest_json = JSON.parse(page.body) + + expect(manifest_json['label']).to eq 'Comet in Moominland' + expect(manifest_json['description']).to contain_exactly('a novel about moomins') + expect(manifest_json['sequences']).not_to be_empty + + sequence = manifest_json['sequences'].first + + # 12 file sets, plus 2 child works with 2 file sets each + expect(sequence['canvases'].count).to eq 16 + end + + context 'with a user missing read permissions on children' do + let(:user) { create(:user) } + + before do + work.read_users = [user] + work.save + end + + it 'generates a bare manifest' do + visit "/concern/generic_works/#{work.id}/manifest" + + # maybe validate this with https://github.com/IIIF/presentation-validator/blob/master/schema/iiif_3_0.json ? + manifest_json = JSON.parse(page.body) + + expect(manifest_json['label']).to eq 'Comet in Moominland' + expect(manifest_json['description']).to contain_exactly('a novel about moomins') + expect(manifest_json).not_to have_key 'sequences' + end + end +end diff --git a/spec/presenters/hyrax/iiif_manifest_presenter_spec.rb b/spec/presenters/hyrax/iiif_manifest_presenter_spec.rb new file mode 100644 index 0000000000..811e12a38c --- /dev/null +++ b/spec/presenters/hyrax/iiif_manifest_presenter_spec.rb @@ -0,0 +1,174 @@ +# frozen_string_literal: true + +# rubocop:disable BracesAroundHashParameters maybe a rubocop bug re hash params? +RSpec.describe Hyrax::IiifManifestPresenter do + subject(:presenter) { described_class.new(work) } + let(:work) { create(:work_with_image_files) } + + describe 'manifest generation' do + let(:builder_service) { Hyrax::ManifestBuilderService.new } + + it 'generates a IIIF presentation 2.0 manifest' do + expect(builder_service.manifest_for(presenter: presenter)) + .to include('@context' => 'http://iiif.io/api/presentation/2/context.json') + end + + context 'with file set and work members' do + it 'generates a manifest with nested content' do + expect(builder_service.manifest_for(presenter: presenter)['sequences'].first['canvases'].count) + .to eq 2 # two image file_set members from the factory + end + + context 'and an ability' do + let(:ability) { Ability.new(user) } + let(:user) { create(:user) } + + before { presenter.ability = ability } + + it 'excludes items the user cannot read' do + expect(builder_service.manifest_for(presenter: presenter)) + .not_to have_key('sequences') + end + + it 'includes items with read permissions' do + readable = FactoryBot.create(:file_set, :image, user: user) + work.members << readable + work.save + + expect(builder_service.manifest_for(presenter: presenter)['sequences'].first['canvases'].count) + .to eq 1 # just the one readable file_set; not the two from the factory + end + end + end + end + + describe Hyrax::IiifManifestPresenter::DisplayImagePresenter do + subject(:presenter) { described_class.new(solr_doc) } + let(:solr_doc) { SolrDocument.new(file_set.to_solr) } + let(:file_set) { create(:file_set, :image) } + + describe '#display_image' do + it 'gives a IIIFManifest::DisplayImage' do + expect(presenter.display_image.to_json) + .to include 'fcr:versions%2Fversion1/full' + end + + context 'with non-image file_set' do + let(:file_set) { create(:file_set) } + + it 'returns nil' do + expect(presenter.display_image).to be_nil + end + end + end + end + + describe '#description' do + it 'returns a string description of the object' do + expect(presenter.description).to be_a String + end + end + + describe '#file_set_presenters' do + let(:work) { build(:work) } + + it 'is empty' do + expect(presenter.file_set_presenters).to be_empty + end + + context 'when the work has file set members' do + let(:work) { create(:work_with_image_files) } + + it 'gives presenters for the file sets' do + expect(presenter.file_set_presenters) + .to contain_exactly(*work.member_ids.map { |id| have_attributes(id: id) }) + end + + it 'gives DisplayImagePresenters' do + expect(presenter.file_set_presenters.map(&:display_image)) + .to contain_exactly(an_instance_of(IIIFManifest::DisplayImage), + an_instance_of(IIIFManifest::DisplayImage)) + end + + context 'and work members' do + let(:work) { create(:work_with_file_and_work) } + + it 'gives presenters only for the file set members' do + fs_members = work.members.select(&:file_set?) + + expect(presenter.file_set_presenters) + .to contain_exactly(*fs_members.map { |member| have_attributes(id: member.id) }) + end + + context 'and an ability' do + let(:ability) { Ability.new(user) } + let(:user) { create(:user) } + + before { presenter.ability = ability } + + it 'is empty when the user cannot read any file sets' do + expect(presenter.file_set_presenters).to be_empty + end + + it 'has file sets the user can read' do + readable = FactoryBot.create(:file_set, :image, user: user) + work.members << readable + work.save + + expect(presenter.file_set_presenters) + .to contain_exactly(have_attributes(id: readable.id)) + end + end + end + end + end + + describe '#manifest_metadata' do + it 'includes metadata' do + expect(presenter.manifest_metadata) + .to contain_exactly({ 'label' => 'Title', 'value' => ['Test title'] }, + { 'label' => 'Creator', 'value' => [] }, + { 'label' => 'Keyword', 'value' => [] }, + { 'label' => 'Rights statement', 'value' => [] }) + end + end + + describe '#sequence_rendering' do + it 'provides an empty sequence rendering' do + expect(presenter.sequence_rendering).to eq([]) + end + + context 'with file sets in a rendering sequence' do + let(:work) { create(:work_with_image_files) } + + before do + work.rendering_ids = work.file_set_ids + work.save! + end + + it 'provides a sequence rendering for the file_sets' do + expect(presenter.sequence_rendering.count).to eq 2 + end + end + end + + describe '#work_presenters' do + it 'is empty' do + expect(presenter.work_presenters).to be_empty + end + + context 'when the work has member works' do + context 'and file set members' do + let(:work) { create(:work_with_file_and_work) } + + it 'gives presenters only for the work members' do + work_members = work.members.select(&:work?) + + expect(presenter.work_presenters) + .to contain_exactly(*work_members.map { |member| have_attributes(id: member.id) }) + end + end + end + end +end +# rubocop:enable BracesAroundHashParameters diff --git a/spec/services/hyrax/file_set_fixity_check_service_spec.rb b/spec/services/hyrax/file_set_fixity_check_service_spec.rb index 694f6aceb7..0c1b8375d4 100644 --- a/spec/services/hyrax/file_set_fixity_check_service_spec.rb +++ b/spec/services/hyrax/file_set_fixity_check_service_spec.rb @@ -1,5 +1,5 @@ RSpec.describe Hyrax::FileSetFixityCheckService do - let(:f) { create(:file_set, content: File.open(fixture_path + '/world.png')) } + let(:f) { create(:file_set, :image) } let(:service_by_object) { described_class.new(f) } let(:service_by_id) { described_class.new(f.id) } From 1ca265483fe9ed273201ca959c5a9ba85598f754 Mon Sep 17 00:00:00 2001 From: tom johnson Date: Tue, 12 May 2020 17:50:49 -0700 Subject: [PATCH 7/8] add a job for prewarming a manifest cache this job hits the manifest cache to prewarm; at least for now it's a (fast) no-op if there is already something in the cache for the key. reconstruct the cache keys to use `etag`. this isn't perfect, but it correlates more closely to object changes than the solr `_version_` field, and avoids worry about cases where a `SolrDocument#[]` lookup on `_version_` is `nil` (which are common). for AF models, `#etag` is populated as long as the model isn't a `#new_record?`. we also add a prefix for the cache key, this is meant to be specific to the caching implementation. if we change the implementation in the future in a way that would benefit from ignoring old cache entries, we can simply change or increment the prefix. this also serves as a namespace, which should avoid cache key collisions. --- app/jobs/iiif_manifest_cache_prewarm_job.rb | 16 ++++++++++++++ .../hyrax/caching_iiif_manifest_builder.rb | 21 +++++++++++++++---- .../iiif_manifest_cache_prewarm_job_spec.rb | 19 +++++++++++++++++ .../caching_iiif_manifest_builder_spec.rb | 5 +++-- 4 files changed, 55 insertions(+), 6 deletions(-) create mode 100644 app/jobs/iiif_manifest_cache_prewarm_job.rb create mode 100644 spec/jobs/iiif_manifest_cache_prewarm_job_spec.rb diff --git a/app/jobs/iiif_manifest_cache_prewarm_job.rb b/app/jobs/iiif_manifest_cache_prewarm_job.rb new file mode 100644 index 0000000000..5a7cb71e9a --- /dev/null +++ b/app/jobs/iiif_manifest_cache_prewarm_job.rb @@ -0,0 +1,16 @@ +# frozen_string_literal: true + +class IiifManifestCachePrewarmJob < Hyrax::ApplicationJob + ## + # @param work [ActiveFedora::Base] + def perform(work) + presenter = Hyrax::IiifManifestPresenter.new(work) + manifest_builder.manifest_for(presenter: presenter) + end + + private + + def manifest_builder + Hyrax::CachingIiifManifestBuilder.new + end +end diff --git a/app/services/hyrax/caching_iiif_manifest_builder.rb b/app/services/hyrax/caching_iiif_manifest_builder.rb index 4e3bbdcfe2..cf4103596e 100644 --- a/app/services/hyrax/caching_iiif_manifest_builder.rb +++ b/app/services/hyrax/caching_iiif_manifest_builder.rb @@ -6,6 +6,8 @@ module Hyrax # this approach avoids long manifest build times for some kinds of requests, # at the cost of introducing cache invalidation issues. class CachingIiifManifestBuilder < ManifestBuilderService + KEY_PREFIX = 'iiif-cache-v1' + attr_accessor :expires_in ## @@ -32,13 +34,24 @@ def manifest_for(presenter:) private ## - # By adding the Solr '_version_' field to the cache key, we shouldn't - # run into the problem of fetching an outdated version of the manifest. - # @param presenter [Hyrax::WorkShowPresenter] + # @note adding a version_for suffix helps us manage cache expiration, + # reducing false cache hits + # + # @param presenter [Hyrax::IiifManifestPresenter] # # @return [String] def manifest_cache_key(presenter:) - "#{presenter.id}/#{presenter.solr_document['_version_']}" + "#{KEY_PREFIX}_#{presenter.id}/#{version_for(presenter)}" + end + + ## + # @note `etag` is a better option than the solr document `_version_`; the + # latter isn't always available, depending on how the presenter was + # built! + # + # @return [String] + def version_for(presenter) + presenter.etag end end end diff --git a/spec/jobs/iiif_manifest_cache_prewarm_job_spec.rb b/spec/jobs/iiif_manifest_cache_prewarm_job_spec.rb new file mode 100644 index 0000000000..070d90d12f --- /dev/null +++ b/spec/jobs/iiif_manifest_cache_prewarm_job_spec.rb @@ -0,0 +1,19 @@ +# frozen_string_literal: true + +RSpec.describe IiifManifestCachePrewarmJob do + let(:work) { create(:work_with_files) } + let(:cache_key) do + Hyrax::CachingIiifManifestBuilder + .new + .send(:manifest_cache_key, presenter: Hyrax::IiifManifestPresenter.new(work)) + end + + describe '.perform_now' do + it 'caches a manifest' do + expect { described_class.perform_now(work) } + .to change { Rails.cache.read(cache_key) } + .from(nil) + .to an_instance_of(Hash) + end + end +end diff --git a/spec/services/hyrax/caching_iiif_manifest_builder_spec.rb b/spec/services/hyrax/caching_iiif_manifest_builder_spec.rb index 982efd67c8..6ac98084fe 100644 --- a/spec/services/hyrax/caching_iiif_manifest_builder_spec.rb +++ b/spec/services/hyrax/caching_iiif_manifest_builder_spec.rb @@ -3,14 +3,15 @@ RSpec.describe Hyrax::CachingIiifManifestBuilder, :clean_repo do let(:id) { "123" } let(:manifest_url) { File.join("https://samvera.org", "show", id) } - let(:solr_document) { { "_version_" => 1 } } + let(:etag) { 'my_etag' } let(:work_presenter) { double("Work Presenter") } let(:file_set_presenter) { double("File Set Presenter", id: "456") } + let(:presenter) do double( 'Presenter', id: id, - solr_document: solr_document, + etag: etag, work_presenters: [work_presenter], manifest_url: manifest_url, description: ["A Treatise on Coding in Samvera"], From 38adbb177efab3dfc8178a2d54db466f98a59d47 Mon Sep 17 00:00:00 2001 From: tamsin johnson Date: Tue, 7 Jul 2020 19:03:21 -0700 Subject: [PATCH 8/8] add explicit `draper` dependency --- hyrax.gemspec | 1 + lib/hyrax/engine.rb | 1 + 2 files changed, 2 insertions(+) diff --git a/hyrax.gemspec b/hyrax.gemspec index 01b89dd5e3..8ce2162858 100644 --- a/hyrax.gemspec +++ b/hyrax.gemspec @@ -39,6 +39,7 @@ SUMMARY spec.add_dependency 'browse-everything', '>= 0.16' spec.add_dependency 'carrierwave', '~> 1.0' spec.add_dependency 'clipboard-rails', '~> 1.5' + spec.add_dependency 'draper', '~> 4.0' spec.add_dependency 'dry-equalizer', '~> 0.2' spec.add_dependency 'dry-struct', '>= 0.1', '< 2.0' spec.add_dependency 'dry-transaction', '~> 0.11' diff --git a/lib/hyrax/engine.rb b/lib/hyrax/engine.rb index 0ad24c5c85..39dcb96937 100644 --- a/lib/hyrax/engine.rb +++ b/lib/hyrax/engine.rb @@ -5,6 +5,7 @@ class Engine < ::Rails::Engine # These gems must be required outside of an initializer or they don't get loaded. require 'awesome_nested_set' require 'breadcrumbs_on_rails' + require 'draper' require 'jquery-ui-rails' require 'flot-rails' require 'almond-rails'