diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index e1ec8ec..6cb2fee 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -11,7 +11,7 @@ jobs: runs-on: ubuntu-24.04 strategy: matrix: - ruby-version: ['3.2.2'] + ruby-version: ['3.3.8'] node: ['20'] redis-version: [7] env: diff --git a/.ruby-version b/.ruby-version index 9e79f6c..37d02a6 100644 --- a/.ruby-version +++ b/.ruby-version @@ -1 +1 @@ -ruby-3.2.2 +3.3.8 diff --git a/Gemfile b/Gemfile index f0368df..ff1402a 100644 --- a/Gemfile +++ b/Gemfile @@ -19,6 +19,8 @@ gem 'jbuilder', '~> 2.7' gem 'jwt', '~> 2.7.1' # Use mysql as a database option for Active Record gem 'mysql2', '~> 0.5.5' +# For building and parsing XML +gem 'nokogiri', '~> 1.18', '>= 1.18.10', force_ruby_platform: true # Use Puma as the app server gem 'puma', '~> 5.2' # Bundle edge Rails instead: gem 'rails', github: 'rails/rails' diff --git a/Gemfile.lock b/Gemfile.lock index ffecd0b..51eca0c 100644 --- a/Gemfile.lock +++ b/Gemfile.lock @@ -257,7 +257,7 @@ GEM puma (5.6.8) nio4r (~> 2.0) racc (1.8.1) - rack (2.2.20) + rack (2.2.21) rack-protection (3.2.0) base64 (>= 0.1.0) rack (~> 2.2, >= 2.2.4) @@ -418,6 +418,7 @@ GEM sprockets (>= 3.0.0) sqlite3 (1.7.0) mini_portile2 (~> 2.8.0) + sqlite3 (1.7.0-x86_64-linux) sshkit (1.24.0) base64 logger @@ -446,7 +447,7 @@ GEM activemodel (>= 6.0.0) bindex (>= 0.4.0) railties (>= 6.0.0) - webrick (1.8.1) + webrick (1.9.1) websocket-driver (0.7.6) websocket-extensions (>= 0.1.0) websocket-extensions (0.1.5) @@ -454,6 +455,7 @@ GEM PLATFORMS ruby + x86_64-linux DEPENDENCIES addressable (~> 2.8.0) @@ -475,6 +477,7 @@ DEPENDENCIES jwt (~> 2.7.1) listen (~> 3.3) mysql2 (~> 0.5.5) + nokogiri (~> 1.18, >= 1.18.10) omniauth omniauth-cul (~> 0.2.0) puma (~> 5.2) @@ -495,4 +498,4 @@ DEPENDENCIES web-console (>= 3.3.0) BUNDLED WITH - 2.3.13 + 2.6.9 diff --git a/app/controllers/concerns/triclops/iiif/images_controller/raster_opt_fallback_logic.rb b/app/controllers/concerns/triclops/iiif/images_controller/raster_opt_fallback_logic.rb index 52284d0..992d335 100644 --- a/app/controllers/concerns/triclops/iiif/images_controller/raster_opt_fallback_logic.rb +++ b/app/controllers/concerns/triclops/iiif/images_controller/raster_opt_fallback_logic.rb @@ -4,7 +4,7 @@ module ImagesController module RasterOptFallbackLogic extend ActiveSupport::Concern - # rubocop:disable Metrics/MethodLength, Metrics/AbcSize, Metrics/BlockNesting + # rubocop:disable Metrics/MethodLength, Metrics/AbcSize, Metrics/BlockNesting, Metrics/CyclomaticComplexity, Metrics/PerceivedComplexity def raster_opts_for_ready_resource_with_fallback(resource, base_type, original_raster_opts, normalized_raster_opts) raster_opts_to_try = normalized_raster_opts.dup cache_hit = resource.raster_exists?(base_type, raster_opts_to_try) @@ -41,10 +41,36 @@ def raster_opts_for_ready_resource_with_fallback(resource, base_type, original_r raster_opts_to_try = normalized_raster_opts.merge(size: "!#{long_side},#{long_side}") cache_hit = resource.raster_exists?(base_type, raster_opts_to_try) - Rails.logger.error( - "[#{resource.identifier}] "\ - "Third try: Cache #{cache_hit ? 'HIT' : 'MISS'} for raster_opts_to_try: #{raster_opts_to_try}" - ) + unless cache_hit + Rails.logger.error( + "[#{resource.identifier}] "\ + "Third try: Cache #{cache_hit ? 'HIT' : 'MISS'} for raster_opts_to_try: #{raster_opts_to_try}" + ) + + # Unfortunately, we cannot guarantee that converting a rounded width or height value to a + # "!#{long_side},#{long_side}" value will always result in the long_size value in the cache, + # since the generation of "!#{long_side},#{long_side}" cached items was based on the original + # ratio and rounding errors can occur. So we'll also fall back to checking for + # "!#{long_side - 1},#{long_side - 1}" and "!#{long_side + 1},#{long_side + 1}" versions. + + raster_opts_to_try = normalized_raster_opts.merge(size: "!#{long_side - 1},#{long_side - 1}") + cache_hit = resource.raster_exists?(base_type, raster_opts_to_try) + unless cache_hit + Rails.logger.error( + "[#{resource.identifier}] "\ + "Fourth try: Cache #{cache_hit ? 'HIT' : 'MISS'} for raster_opts_to_try: #{raster_opts_to_try}" + ) + + raster_opts_to_try = normalized_raster_opts.merge(size: "!#{long_side + 1},#{long_side + 1}") + cache_hit = resource.raster_exists?(base_type, raster_opts_to_try) + unless cache_hit + Rails.logger.error( + "[#{resource.identifier}] "\ + "Fifth try: Cache #{cache_hit ? 'HIT' : 'MISS'} for raster_opts_to_try: #{raster_opts_to_try}" + ) + end + end + end end end end @@ -53,7 +79,7 @@ def raster_opts_for_ready_resource_with_fallback(resource, base_type, original_r [raster_opts_to_try, cache_hit] end - # rubocop:enable Metrics/MethodLength, Metrics/AbcSize, Metrics/BlockNesting + # rubocop:enable Metrics/MethodLength, Metrics/AbcSize, Metrics/BlockNesting, Metrics/CyclomaticComplexity, Metrics/PerceivedComplexity end end end diff --git a/spec/controllers/concerns/triclops/iiif/images_controller/raster_opt_fallback_logic_spec.rb b/spec/controllers/concerns/triclops/iiif/images_controller/raster_opt_fallback_logic_spec.rb index 6781a1c..01315e2 100644 --- a/spec/controllers/concerns/triclops/iiif/images_controller/raster_opt_fallback_logic_spec.rb +++ b/spec/controllers/concerns/triclops/iiif/images_controller/raster_opt_fallback_logic_spec.rb @@ -38,6 +38,12 @@ let(:normalized_raster_opts_with_exclamation_point_long_side_size_notation) do normalized_raster_opts.merge({ size: "!#{long_side_length},#{long_side_length}" }) end + let(:normalized_raster_opts_with_exclamation_point_long_side_minus_one_size_notation) do + normalized_raster_opts.merge({ size: "!#{long_side_length - 1},#{long_side_length - 1}" }) + end + let(:normalized_raster_opts_with_exclamation_point_long_side_plus_one_size_notation) do + normalized_raster_opts.merge({ size: "!#{long_side_length + 1},#{long_side_length + 1}" }) + end before do allow(resource).to receive(:identifier).and_return(identifier) @@ -57,7 +63,7 @@ end context 'when an associated file does not exist at the normalized_raster_opts path, '\ - 'but an associated file does exist at the normalized_raster_opts path with the size'\ + 'but an associated file does exist at the normalized_raster_opts path with the size '\ 'opt replaced with the original_raster_opts size opt' do before do allow(resource).to receive(:raster_exists?).with(base_type, normalized_raster_opts).and_return(false) @@ -74,8 +80,8 @@ end context 'when an associated file does not exist at the normalized_raster_opts path, '\ - 'and it does not exist at the normalized_raster_opts path with the size'\ - 'opt replaced with the original_raster_opts size opt, but it does exist at'\ + 'and it does not exist at the normalized_raster_opts path with the size '\ + 'opt replaced with the original_raster_opts size opt, but it does exist at '\ 'the normalized_raster_opts path with the size replaced with "!long_side,long_side"' do before do allow(resource).to receive(:raster_exists?).with(base_type, normalized_raster_opts).and_return(false) @@ -95,6 +101,66 @@ end end + context 'when an associated file does not exist at the normalized_raster_opts path, '\ + 'and it does not exist at the normalized_raster_opts path with the size '\ + 'opt replaced with the original_raster_opts size opt, '\ + 'and it does exist at the normalized_raster_opts path with the size replaced with "!long_side,long_side", '\ + 'but it does exist at the normalized_raster_opts path with the size '\ + 'replaced with "!long_side - 1,long_side - 1",' do + before do + allow(resource).to receive(:raster_exists?).with(base_type, normalized_raster_opts).and_return(false) + allow(resource).to receive(:raster_exists?).with( + base_type, normalized_raster_opts_with_original_size_opt + ).and_return(false) + allow(resource).to receive(:raster_exists?).with( + base_type, normalized_raster_opts_with_exclamation_point_long_side_size_notation + ).and_return(false) + allow(resource).to receive(:raster_exists?).with( + base_type, normalized_raster_opts_with_exclamation_point_long_side_minus_one_size_notation + ).and_return(true) + end + + it 'returns the expected opts, and a cache hit of true' do + expect( + instance.raster_opts_for_ready_resource_with_fallback( + resource, base_type, original_raster_opts, normalized_raster_opts + ) + ).to eq([normalized_raster_opts_with_exclamation_point_long_side_minus_one_size_notation, true]) + end + end + + context 'when an associated file does not exist at the normalized_raster_opts path, '\ + 'and it does not exist at the normalized_raster_opts path with the size '\ + 'opt replaced with the original_raster_opts size opt, '\ + 'and it does exist at the normalized_raster_opts path with the size replaced with "!long_side,long_side", '\ + 'and it does exist at the normalized_raster_opts path with the size replaced with "!long_side - 1,long_side - 1", '\ + 'but it does exist at the normalized_raster_opts path with the size '\ + 'replaced with "!long_side + 1,long_side + 1",' do + before do + allow(resource).to receive(:raster_exists?).with(base_type, normalized_raster_opts).and_return(false) + allow(resource).to receive(:raster_exists?).with( + base_type, normalized_raster_opts_with_original_size_opt + ).and_return(false) + allow(resource).to receive(:raster_exists?).with( + base_type, normalized_raster_opts_with_exclamation_point_long_side_size_notation + ).and_return(false) + allow(resource).to receive(:raster_exists?).with( + base_type, normalized_raster_opts_with_exclamation_point_long_side_minus_one_size_notation + ).and_return(false) + allow(resource).to receive(:raster_exists?).with( + base_type, normalized_raster_opts_with_exclamation_point_long_side_plus_one_size_notation + ).and_return(true) + end + + it 'returns the expected opts, and a cache hit of true' do + expect( + instance.raster_opts_for_ready_resource_with_fallback( + resource, base_type, original_raster_opts, normalized_raster_opts + ) + ).to eq([normalized_raster_opts_with_exclamation_point_long_side_plus_one_size_notation, true]) + end + end + context 'when an associated file does not exist at the normalized_raster_opts path, '\ 'and it does not exist at any other fallback variants' do before do @@ -105,12 +171,18 @@ allow(resource).to receive(:raster_exists?).with( base_type, normalized_raster_opts_with_exclamation_point_long_side_size_notation ).and_return(false) + allow(resource).to receive(:raster_exists?).with( + base_type, normalized_raster_opts_with_exclamation_point_long_side_minus_one_size_notation + ).and_return(false) + allow(resource).to receive(:raster_exists?).with( + base_type, normalized_raster_opts_with_exclamation_point_long_side_plus_one_size_notation + ).and_return(false) end it 'returns the expected opts, and a cache hit of false' do expect(instance.raster_opts_for_ready_resource_with_fallback( resource, base_type, original_raster_opts, normalized_raster_opts - )).to eq([normalized_raster_opts_with_exclamation_point_long_side_size_notation, false]) + )).to eq([normalized_raster_opts_with_exclamation_point_long_side_plus_one_size_notation, false]) end end end