From fe211f77181a59a411d8f05f6d6e1a427f9be10e Mon Sep 17 00:00:00 2001 From: Marco Costa Date: Thu, 5 Dec 2024 15:48:15 -0800 Subject: [PATCH] Do not load `redis` gem when incompatible --- Matrixfile | 3 +++ Rakefile | 9 +++++++++ appraisal/jruby-9.2.rb | 16 +++++++++++++++ appraisal/jruby-9.3.rb | 9 +++++++++ appraisal/jruby-9.4.rb | 16 +++++++++++++++ appraisal/ruby-2.5.rb | 9 +++++++++ appraisal/ruby-2.6.rb | 9 +++++++++ appraisal/ruby-2.7.rb | 9 +++++++++ appraisal/ruby-3.0.rb | 9 +++++++++ appraisal/ruby-3.1.rb | 9 +++++++++ appraisal/ruby-3.2.rb | 9 +++++++++ appraisal/ruby-3.3.rb | 9 +++++++++ appraisal/ruby-3.4.rb | 9 +++++++++ .../contrib/active_support/cache/redis.rb | 20 +++++++++++++++---- .../contrib/active_support/cache/patcher.rbs | 2 +- .../contrib/active_support/cache/redis.rbs | 6 +++--- 16 files changed, 145 insertions(+), 8 deletions(-) diff --git a/Matrixfile b/Matrixfile index 2fa275e5f14..9d2e8529dab 100644 --- a/Matrixfile +++ b/Matrixfile @@ -229,6 +229,9 @@ 'rails6-semantic-logger' => '✅ 2.5 / ✅ 2.6 / ✅ 2.7 / ❌ 3.0 / ❌ 3.1 / ❌ 3.2 / ❌ 3.3 / ❌ 3.4 / ✅ jruby', 'rails61-semantic-logger' => '✅ 2.5 / ✅ 2.6 / ✅ 2.7 / ✅ 3.0 / ✅ 3.1 / ✅ 3.2 / ✅ 3.3 / ✅ 3.4 / ✅ jruby' }, + 'rails_old_redis' => { + 'rails-old-redis' => '✅ 2.5 / ✅ 2.6 / ✅ 2.7 / ✅ 3.0 / ✅ 3.1 / ✅ 3.2 / ✅ 3.3 / ✅ 3.4 / ✅ jruby', + }, 'action_cable' => { # FIXME: Enable the test for JRuby after fixing `log writing failed. closed stream` in CircleCI. 'rails5-mysql2' => '✅ 2.5 / ✅ 2.6 / ❌ 2.7 / ❌ 3.0 / ❌ 3.1 / ❌ 3.2 / ❌ 3.3 / ❌ 3.4 / ❌ jruby', diff --git a/Rakefile b/Rakefile index 5ddc8e14b16..0f5a522a447 100644 --- a/Rakefile +++ b/Rakefile @@ -153,6 +153,15 @@ namespace :spec do t.rspec_opts = args.to_a.join(' ') end + # Tests if Datadog::Tracing::Contrib::ActiveSupport::Cache::Redis::Patcher does not eager load + # ActiveSupport::Cache::RedisCacheStore when the version of Redis present is too old to be compatible. + # @see Datadog::Tracing::Contrib::ActiveSupport::Cache::Redis::Patcher#patch_redis_cache_store? + desc '' # "Explicitly hiding from `rake -T`" + RSpec::Core::RakeTask.new(:rails_old_redis) do |t, args| + t.pattern = 'spec/datadog/tracing/contrib/rails/cache_spec.rb' + t.rspec_opts = args.to_a.join(' ') + end + desc '' # "Explicitly hiding from `rake -T`" RSpec::Core::RakeTask.new(:hanami) do |t, args| t.pattern = 'spec/datadog/tracing/contrib/hanami/**/*_spec.rb' diff --git a/appraisal/jruby-9.2.rb b/appraisal/jruby-9.2.rb index 571acd7252c..7a45865a766 100644 --- a/appraisal/jruby-9.2.rb +++ b/appraisal/jruby-9.2.rb @@ -151,6 +151,22 @@ gem 'i18n', '1.8.7', platform: :jruby # Removal pending: https://github.com/ruby-i18n/i18n/issues/555#issuecomment-772112169 end +appraise 'rails-old-redis' do + # All dependencies except Redis < 4 are not important, they are just required to run Rails tests. + gem 'redis', '< 4' + gem 'rails', '~> 6.1.0' + gem 'pg', '>= 1.1', platform: :ruby + gem 'sprockets', '< 4' + gem 'lograge', '~> 0.11' +endappraise 'rails-old-redis' do + # All dependencies except Redis < 4 are not important, they are just required to run Rails tests. + gem 'redis', '< 4' + gem 'rails', '~> 6.1.0' + gem 'pg', '>= 1.1', platform: :ruby + gem 'sprockets', '< 4' + gem 'lograge', '~> 0.11' +end + appraise 'resque2-redis3' do gem 'redis', '< 4.0' gem 'resque', '>= 2.0' diff --git a/appraisal/jruby-9.3.rb b/appraisal/jruby-9.3.rb index f796fd81c21..975db0a1176 100644 --- a/appraisal/jruby-9.3.rb +++ b/appraisal/jruby-9.3.rb @@ -131,6 +131,15 @@ gem 'rails_semantic_logger', '~> 4.0' end +appraise 'rails-old-redis' do + # All dependencies except Redis < 4 are not important, they are just required to run Rails tests. + gem 'redis', '< 4' + gem 'rails', '~> 6.1.0' + gem 'pg', '>= 1.1', platform: :ruby + gem 'sprockets', '< 4' + gem 'lograge', '~> 0.11' +end + appraise 'resque2-redis3' do gem 'redis', '~> 3.0' gem 'resque', '>= 2.0' diff --git a/appraisal/jruby-9.4.rb b/appraisal/jruby-9.4.rb index 6fce8d81830..73400c449c0 100644 --- a/appraisal/jruby-9.4.rb +++ b/appraisal/jruby-9.4.rb @@ -41,6 +41,22 @@ gem 'net-smtp' end +appraise 'rails-old-redis' do + # All dependencies except Redis < 4 are not important, they are just required to run Rails tests. + gem 'redis', '< 4' + gem 'rails', '~> 6.1.0' + gem 'pg', '>= 1.1', platform: :ruby + gem 'sprockets', '< 4' + gem 'lograge', '~> 0.11' +endappraise 'rails-old-redis' do + # All dependencies except Redis < 4 are not important, they are just required to run Rails tests. + gem 'redis', '< 4' + gem 'rails', '~> 6.1.0' + gem 'pg', '>= 1.1', platform: :ruby + gem 'sprockets', '< 4' + gem 'lograge', '~> 0.11' +end + appraise 'resque2-redis3' do gem 'redis', '< 4.0' gem 'resque', '>= 2.0' diff --git a/appraisal/ruby-2.5.rb b/appraisal/ruby-2.5.rb index 1249ba7e9c6..502c998055d 100644 --- a/appraisal/ruby-2.5.rb +++ b/appraisal/ruby-2.5.rb @@ -184,6 +184,15 @@ gem 'rails_semantic_logger', '~> 4.0' end +appraise 'rails-old-redis' do + # All dependencies except Redis < 4 are not important, they are just required to run Rails tests. + gem 'redis', '< 4' + gem 'rails', '~> 6.1.0' + gem 'pg', '>= 1.1', platform: :ruby + gem 'sprockets', '< 4' + gem 'lograge', '~> 0.11' +end + appraise 'resque2-redis3' do gem 'redis', '< 4.0' gem 'resque', '>= 2.0' diff --git a/appraisal/ruby-2.6.rb b/appraisal/ruby-2.6.rb index 7de49f45740..c89e19893c9 100644 --- a/appraisal/ruby-2.6.rb +++ b/appraisal/ruby-2.6.rb @@ -137,6 +137,15 @@ gem 'rails_semantic_logger', '~> 4.0' end +appraise 'rails-old-redis' do + # All dependencies except Redis < 4 are not important, they are just required to run Rails tests. + gem 'redis', '< 4' + gem 'rails', '~> 6.1.0' + gem 'pg', '>= 1.1', platform: :ruby + gem 'sprockets', '< 4' + gem 'lograge', '~> 0.11' +end + appraise 'resque2-redis3' do gem 'redis', '~> 3.0' gem 'resque', '>= 2.0' diff --git a/appraisal/ruby-2.7.rb b/appraisal/ruby-2.7.rb index 60d2ec62d9d..88b3b0f1e98 100644 --- a/appraisal/ruby-2.7.rb +++ b/appraisal/ruby-2.7.rb @@ -137,6 +137,15 @@ gem 'rails_semantic_logger', '~> 4.0' end +appraise 'rails-old-redis' do + # All dependencies except Redis < 4 are not important, they are just required to run Rails tests. + gem 'redis', '< 4' + gem 'rails', '~> 6.1.0' + gem 'pg', '>= 1.1', platform: :ruby + gem 'sprockets', '< 4' + gem 'lograge', '~> 0.11' +end + appraise 'resque2-redis3' do gem 'redis', '< 4.0' gem 'resque', '>= 2.0' diff --git a/appraisal/ruby-3.0.rb b/appraisal/ruby-3.0.rb index f4b00ee4ec0..ac024429e2f 100644 --- a/appraisal/ruby-3.0.rb +++ b/appraisal/ruby-3.0.rb @@ -58,6 +58,15 @@ gem 'rails', '~> 7.1.0' end +appraise 'rails-old-redis' do + # All dependencies except Redis < 4 are not important, they are just required to run Rails tests. + gem 'redis', '< 4' + gem 'rails', '~> 6.1.0' + gem 'pg', '>= 1.1', platform: :ruby + gem 'sprockets', '< 4' + gem 'lograge', '~> 0.11' +end + appraise 'resque2-redis3' do gem 'redis', '< 4.0' gem 'resque', '>= 2.0' diff --git a/appraisal/ruby-3.1.rb b/appraisal/ruby-3.1.rb index f4b00ee4ec0..ac024429e2f 100644 --- a/appraisal/ruby-3.1.rb +++ b/appraisal/ruby-3.1.rb @@ -58,6 +58,15 @@ gem 'rails', '~> 7.1.0' end +appraise 'rails-old-redis' do + # All dependencies except Redis < 4 are not important, they are just required to run Rails tests. + gem 'redis', '< 4' + gem 'rails', '~> 6.1.0' + gem 'pg', '>= 1.1', platform: :ruby + gem 'sprockets', '< 4' + gem 'lograge', '~> 0.11' +end + appraise 'resque2-redis3' do gem 'redis', '< 4.0' gem 'resque', '>= 2.0' diff --git a/appraisal/ruby-3.2.rb b/appraisal/ruby-3.2.rb index f4b00ee4ec0..ac024429e2f 100644 --- a/appraisal/ruby-3.2.rb +++ b/appraisal/ruby-3.2.rb @@ -58,6 +58,15 @@ gem 'rails', '~> 7.1.0' end +appraise 'rails-old-redis' do + # All dependencies except Redis < 4 are not important, they are just required to run Rails tests. + gem 'redis', '< 4' + gem 'rails', '~> 6.1.0' + gem 'pg', '>= 1.1', platform: :ruby + gem 'sprockets', '< 4' + gem 'lograge', '~> 0.11' +end + appraise 'resque2-redis3' do gem 'redis', '< 4.0' gem 'resque', '>= 2.0' diff --git a/appraisal/ruby-3.3.rb b/appraisal/ruby-3.3.rb index 77faa8ba5c7..d361fcd68a7 100644 --- a/appraisal/ruby-3.3.rb +++ b/appraisal/ruby-3.3.rb @@ -58,6 +58,15 @@ gem 'rails', '~> 7.1.0' end +appraise 'rails-old-redis' do + # All dependencies except Redis < 4 are not important, they are just required to run Rails tests. + gem 'redis', '< 4' + gem 'rails', '~> 6.1.0' + gem 'pg', '>= 1.1', platform: :ruby + gem 'sprockets', '< 4' + gem 'lograge', '~> 0.11' +end + appraise 'resque2-redis3' do gem 'redis', '< 4.0' gem 'resque', '>= 2.0' diff --git a/appraisal/ruby-3.4.rb b/appraisal/ruby-3.4.rb index 6cb1bf51ce2..e83945820bc 100644 --- a/appraisal/ruby-3.4.rb +++ b/appraisal/ruby-3.4.rb @@ -58,6 +58,15 @@ gem 'rails', '~> 7.1.0' end +appraise 'rails-old-redis' do + # All dependencies except Redis < 4 are not important, they are just required to run Rails tests. + gem 'redis', '< 4' + gem 'rails', '~> 6.1.0' + gem 'pg', '>= 1.1', platform: :ruby + gem 'sprockets', '< 4' + gem 'lograge', '~> 0.11' +end + appraise 'resque2-redis3' do gem 'redis', '< 4.0' gem 'resque', '>= 2.0' diff --git a/lib/datadog/tracing/contrib/active_support/cache/redis.rb b/lib/datadog/tracing/contrib/active_support/cache/redis.rb index 4004ab4a136..b2add823b9c 100644 --- a/lib/datadog/tracing/contrib/active_support/cache/redis.rb +++ b/lib/datadog/tracing/contrib/active_support/cache/redis.rb @@ -22,17 +22,29 @@ module Patcher # For Rails >= 5.2 w/o redis-activesupport... # ActiveSupport includes a Redis cache store internally, and does not require these overrides. # https://github.com/rails/rails/blob/master/activesupport/lib/active_support/cache/redis_cache_store.rb - def patch_redis?(meth) + def patch_redis_activesupport?(meth) !Gem.loaded_specs['redis-activesupport'].nil? \ && defined?(::ActiveSupport::Cache::RedisStore) \ && ::ActiveSupport::Cache::RedisStore.instance_methods(false).include?(meth) end + # Patches the Rails built-in Redis cache backend `redis_cache_store`, added in Rails 5.2. + # We avoid loading the RedisCacheStore class, as it invokes the statement `gem "redis", ">= 4.0.1"` which + # fails if the application is using an old version of Redis, or not using Redis at all. + # @see https://github.com/rails/rails/blob/d0dcb8fa6073a0c4d42600c15e82e3bb386b27d3/activesupport/lib/active_support/cache/redis_cache_store.rb#L4 + def patch_redis_cache_store?(meth) + Gem.loaded_specs['redis'] && + # Autoload constants return `constant` for `defined?`, but that doesn't mean they are loaded... + defined?(::ActiveSupport::Cache::RedisCacheStore) && + # ... to check that we need to call `autoload?` and check if it returns `nil`, meaning it's loaded. + ::ActiveSupport::Cache.autoload?(:RedisCacheStore).nil? && + ::ActiveSupport::Cache::RedisCacheStore.instance_methods(false).include?(meth) + end + def cache_store_class(meth) - if patch_redis?(meth) + if patch_redis_activesupport?(meth) [::ActiveSupport::Cache::RedisStore, ::ActiveSupport::Cache::Store] - elsif Gem.loaded_specs['redis'] && defined?(::ActiveSupport::Cache::RedisCacheStore) \ - && ::ActiveSupport::Cache::RedisCacheStore.instance_methods(false).include?(meth) + elsif patch_redis_cache_store?(meth) [::ActiveSupport::Cache::RedisCacheStore, ::ActiveSupport::Cache::Store] else super diff --git a/sig/datadog/tracing/contrib/active_support/cache/patcher.rbs b/sig/datadog/tracing/contrib/active_support/cache/patcher.rbs index 42372f58fba..302617fbfb3 100644 --- a/sig/datadog/tracing/contrib/active_support/cache/patcher.rbs +++ b/sig/datadog/tracing/contrib/active_support/cache/patcher.rbs @@ -10,7 +10,7 @@ module Datadog def self?.patch: () -> untyped - def self?.cache_store_class: (untyped meth) -> untyped + def self?.cache_store_class: (Symbol meth) -> Array[Class] def self?.patch_cache_store_read: () -> untyped diff --git a/sig/datadog/tracing/contrib/active_support/cache/redis.rbs b/sig/datadog/tracing/contrib/active_support/cache/redis.rbs index 181f0a49538..e288b7959f8 100644 --- a/sig/datadog/tracing/contrib/active_support/cache/redis.rbs +++ b/sig/datadog/tracing/contrib/active_support/cache/redis.rbs @@ -5,9 +5,9 @@ module Datadog module Cache module Redis module Patcher - def patch_redis?: (untyped meth) -> untyped - - def cache_store_class: (untyped meth) -> untyped + def patch_redis_activesupport?: (Symbol) -> bool + def patch_redis_cache_store?: (Symbol) -> bool + def cache_store_class: (Symbol) -> Array[Class] end end end