From e973e3551545c1ffb737907e2100f09dc9c49e32 Mon Sep 17 00:00:00 2001 From: Nate Wiger Date: Wed, 22 Jun 2022 10:24:51 -0700 Subject: [PATCH 1/6] Support new redis_legacy_naming flag to force old behavior --- lib/redis/objects.rb | 24 +++- spec/redis_legacy_key_naming_spec.rb | 187 +++++++++++++++++++++++++++ 2 files changed, 207 insertions(+), 4 deletions(-) create mode 100644 spec/redis_legacy_key_naming_spec.rb diff --git a/lib/redis/objects.rb b/lib/redis/objects.rb index 1db0ed16..c7bb04f4 100644 --- a/lib/redis/objects.rb +++ b/lib/redis/objects.rb @@ -101,13 +101,29 @@ def redis_objects @redis_objects ||= {} end + # Toggles whether to use the legacy redis key naming scheme, which causes + # naming conflicts in certain cases. + attr_accessor :redis_legacy_naming + # Set the Redis redis_prefix to use. Defaults to model_name def redis_prefix=(redis_prefix) @redis_prefix = redis_prefix end def redis_prefix(klass = self) #:nodoc: - @redis_prefix ||= klass.name.to_s. - sub(%r{(.*::)}, ''). - gsub(/([A-Z]+)([A-Z][a-z])/,'\1_\2'). - gsub(/([a-z\d])([A-Z])/,'\1_\2'). + @redis_prefix ||= if redis_legacy_naming + legacy_redis_prefix(klass) + else + klass.name.to_s. + gsub(/::/, '__'). # Nested::Class => Nested__Class + gsub(/([A-Z]+)([A-Z][a-z])/,'\1_\2'). # ClassName => Class_Name + gsub(/([a-z\d])([A-Z])/,'\1_\2'). # className => class_Name + downcase + end + end + + def legacy_redis_prefix(klass = self) #:nodoc: + klass.name.to_s. + sub(%r{(.*::)}, ''). # Nested::Class => Class (problematic) + gsub(/([A-Z]+)([A-Z][a-z])/,'\1_\2'). # ClassName => Class_Name + gsub(/([a-z\d])([A-Z])/,'\1_\2'). # className => class_Name downcase end diff --git a/spec/redis_legacy_key_naming_spec.rb b/spec/redis_legacy_key_naming_spec.rb new file mode 100644 index 00000000..7702d214 --- /dev/null +++ b/spec/redis_legacy_key_naming_spec.rb @@ -0,0 +1,187 @@ +require File.expand_path(File.dirname(__FILE__) + '/spec_helper') + +require 'redis/objects' +Redis::Objects.redis = REDIS_HANDLE + +require 'securerandom' + +describe 'Legacy redis key prefix naming compatibility' do + it 'verifies single level classes work the same' do + class SingleLevelOne + include Redis::Objects + + def id + 1 + end + end + + obj = SingleLevelOne.new + obj.class.redis_prefix.should == 'single_level_one' + end + + it 'verifies single level classes obey the legacy naming flag' do + class SingleLevelTwo + include Redis::Objects + self.redis_legacy_naming = true + + def id + 1 + end + end + + obj = SingleLevelTwo.new + obj.class.redis_prefix.should == 'single_level_two' + end + + + it 'verifies nested classes do NOT work the same' do + module Nested + class NamingOne + include Redis::Objects + + def id + 1 + end + end + end + + obj = Nested::NamingOne.new + obj.class.redis_prefix.should == 'nested__naming_one' + end + + it 'verifies the legacy naming flag is respected' do + module Nested + class NamingTwo + include Redis::Objects + self.redis_legacy_naming = true + + def id + 1 + end + end + end + + Nested::NamingTwo.redis_legacy_naming.should == true + obj = Nested::NamingTwo.new + obj.class.redis_prefix.should == 'naming_two' + end + + it 'verifies that multiple levels respect __ vs _' do + module NestedLevel + module Further + class NamingThree + include Redis::Objects + + def id + 1 + end + end + end + end + + obj = NestedLevel::Further::NamingThree.new + obj.class.redis_prefix.should == 'nested_level__further__naming_three' + end + + it 'verifies that multiple levels respect the legacy naming' do + module NestedLevel + module Further + class NamingFour + include Redis::Objects + self.redis_legacy_naming = true + + def id + 1 + end + + redis_handle = Redis.new(:host => REDIS_HOST, :port => REDIS_PORT, :db => 31) + value :redis_value, :redis => redis_handle + end + end + end + + NestedLevel::Further::NamingFour.redis_legacy_naming.should == true + obj = NestedLevel::Further::NamingFour.new + obj.class.redis_prefix.should == 'naming_four' + val = SecureRandom.hex(10) + obj.redis_value = val + obj.redis_value.should == val + obj.redis_value.key.should == 'naming_four:1:redis_value' + end + + it 'verifies that multiple levels do not conflict 1' do + module NestedLevel + module Further + class NamingFive + include Redis::Objects + self.redis = Redis.new(:host => REDIS_HOST, :port => REDIS_PORT) + + def id + 1 + end + + value :redis_value + end + end + end + + obj = NestedLevel::Further::NamingFive.new + obj.class.redis_prefix.should == 'nested_level__further__naming_five' + val = SecureRandom.hex(10) + obj.redis_value = val + obj.redis_value.should == val + obj.redis_value.key.should == 'nested_level__further__naming_five:1:redis_value' + obj.redis_value.redis.should == obj.redis + obj.redis.get('nested_level__further__naming_five:1:redis_value').should == val + end + + it 'verifies that multiple levels do not conflict 2' do + module Nested + module LevelFurtherNaming + class Five + include Redis::Objects + self.redis = Redis.new(:host => REDIS_HOST, :port => REDIS_PORT) + + def id + 1 + end + + value :redis_value + end + end + end + + obj = Nested::LevelFurtherNaming::Five.new + obj.class.redis_prefix.should == 'nested__level_further_naming__five' + val = SecureRandom.hex(10) + obj.redis_value = val + obj.redis_value.should == val + obj.redis_value.key.should == 'nested__level_further_naming__five:1:redis_value' + obj.redis.get('nested__level_further_naming__five:1:redis_value').should == val + end + + it 'verifies that multiple levels do not conflict 3' do + module Nested + module LevelFurther + class NamingFive + include Redis::Objects + self.redis = Redis.new(:host => REDIS_HOST, :port => REDIS_PORT) + + def id + 1 + end + + value :redis_value + end + end + end + + obj = Nested::LevelFurther::NamingFive.new + obj.class.redis_prefix.should == 'nested__level_further__naming_five' + val = SecureRandom.hex(10) + obj.redis_value = val + obj.redis_value.should == val + obj.redis_value.key.should == 'nested__level_further__naming_five:1:redis_value' + obj.redis.get('nested__level_further__naming_five:1:redis_value').should == val + end +end From d661fef69f97e31c87540fc17c7cf7d40e2d86a6 Mon Sep 17 00:00:00 2001 From: Nate Wiger Date: Wed, 22 Jun 2022 10:25:30 -0700 Subject: [PATCH 2/6] improved a test --- spec/redis_objects_conn_spec.rb | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/spec/redis_objects_conn_spec.rb b/spec/redis_objects_conn_spec.rb index 5cac0cd2..8028f576 100644 --- a/spec/redis_objects_conn_spec.rb +++ b/spec/redis_objects_conn_spec.rb @@ -34,8 +34,8 @@ def id obj.redis_value.value.should == nil obj.default_redis_value.clear - obj.redis_value.value = 'foo' - obj.redis_value.value.should == 'foo' + obj.redis_value.value = 'bar' + obj.redis_value.value.should == 'bar' obj.default_redis_value.value.should == nil obj.redis_value.clear From bc00d2609d9e43b31ea0c113733a985c9c7c4786 Mon Sep 17 00:00:00 2001 From: Nate Wiger Date: Wed, 22 Jun 2022 12:09:18 -0700 Subject: [PATCH 3/6] added waring message and additional tests for new key naming re #231 --- lib/redis/objects.rb | 45 ++++++++++++++--- spec/redis_legacy_key_naming_spec.rb | 74 ++++++++++++++++++++++++++++ spec/redis_objects_model_spec.rb | 4 ++ 3 files changed, 116 insertions(+), 7 deletions(-) diff --git a/lib/redis/objects.rb b/lib/redis/objects.rb index c7bb04f4..724f1c52 100644 --- a/lib/redis/objects.rb +++ b/lib/redis/objects.rb @@ -104,19 +104,32 @@ def redis_objects # Toggles whether to use the legacy redis key naming scheme, which causes # naming conflicts in certain cases. attr_accessor :redis_legacy_naming + attr_accessor :redis_silence_warnings + + # Set the Redis redis_prefix to use. Defaults to class_name. + def redis_prefix=(redis_prefix) + @silence_warnings_as_redis_prefix_was_set_manually = true + @redis_prefix = redis_prefix + end - # Set the Redis redis_prefix to use. Defaults to model_name - def redis_prefix=(redis_prefix) @redis_prefix = redis_prefix end def redis_prefix(klass = self) #:nodoc: - @redis_prefix ||= if redis_legacy_naming - legacy_redis_prefix(klass) - else - klass.name.to_s. + @redis_prefix ||= + if redis_legacy_naming + legacy_redis_prefix(klass) + else + legacy_naming_warning_message(klass) + modern_redis_prefix(klass) + end + + @redis_prefix + end + + def modern_redis_prefix(klass = self) #:nodoc: + klass.name.to_s. gsub(/::/, '__'). # Nested::Class => Nested__Class gsub(/([A-Z]+)([A-Z][a-z])/,'\1_\2'). # ClassName => Class_Name gsub(/([a-z\d])([A-Z])/,'\1_\2'). # className => class_Name downcase - end end def legacy_redis_prefix(klass = self) #:nodoc: @@ -127,6 +140,24 @@ def legacy_redis_prefix(klass = self) #:nodoc: downcase end + # Temporary warning to help with migrating key names + def legacy_naming_warning_message(klass) + # warn @silence_warnings_as_redis_prefix_was_set_manually.inspect + unless redis_legacy_naming || redis_silence_warnings || @silence_warnings_as_redis_prefix_was_set_manually + modern = modern_redis_prefix(klass) + legacy = legacy_redis_prefix(klass) + if modern != legacy + warn < REDIS_HOST, :port => REDIS_PORT) + self.redis_silence_warnings = true def id 1 @@ -141,6 +145,7 @@ module LevelFurtherNaming class Five include Redis::Objects self.redis = Redis.new(:host => REDIS_HOST, :port => REDIS_PORT) + self.redis_silence_warnings = true def id 1 @@ -166,6 +171,7 @@ module LevelFurther class NamingFive include Redis::Objects self.redis = Redis.new(:host => REDIS_HOST, :port => REDIS_PORT) + self.redis_silence_warnings = true def id 1 @@ -184,4 +190,72 @@ def id obj.redis_value.key.should == 'nested__level_further__naming_five:1:redis_value' obj.redis.get('nested__level_further__naming_five:1:redis_value').should == val end + + it 'handles dynamically created classes correctly' do + module Nested + class LevelSix + include Redis::Objects + self.redis = Redis.new(:host => REDIS_HOST, :port => REDIS_PORT) + self.redis_silence_warnings = true + + def id + 1 + end + + value :redis_value + end + end + + obj = Nested::LevelSix.new + obj.class.redis_prefix.should == 'nested__level_six' + val = SecureRandom.hex(10) + obj.redis_value = val + obj.redis_value.should == val + obj.redis_value.key.should == 'nested__level_six:1:redis_value' + obj.redis.get('nested__level_six:1:redis_value').should == val + + DynamicClass = Class.new(Nested::LevelSix) + DynamicClass.value :redis_value2 + obj2 = DynamicClass.new + DynamicClass.redis_prefix.should == 'dynamic_class' + obj2.redis_value.should.be.kind_of(Redis::Value) + obj2.redis_value2.should.be.kind_of(Redis::Value) + obj2.redis_value.key.should == 'dynamic_class:1:redis_value' + obj2.redis_value2.key.should == 'dynamic_class:1:redis_value2' + + end + + it 'handles dynamically created classes correctly in legacy mode' do + module Nested + class LevelSeven + include Redis::Objects + self.redis = Redis.new(:host => REDIS_HOST, :port => REDIS_PORT) + self.redis_legacy_naming = true + + def id + 1 + end + + value :redis_value + end + end + + obj = Nested::LevelSeven.new + obj.class.redis_prefix.should == 'level_seven' + val = SecureRandom.hex(10) + obj.redis_value = val + obj.redis_value.should == val + obj.redis_value.key.should == 'level_seven:1:redis_value' + obj.redis.get('level_seven:1:redis_value').should == val + + DynamicClass2 = Class.new(Nested::LevelSeven) + DynamicClass2.value :redis_value2 + obj2 = DynamicClass2.new + DynamicClass2.redis_prefix.should == 'dynamic_class2' + obj2.redis_value.should.be.kind_of(Redis::Value) + obj2.redis_value2.should.be.kind_of(Redis::Value) + obj2.redis_value.key.should == 'dynamic_class2:1:redis_value' + obj2.redis_value2.key.should == 'dynamic_class2:1:redis_value2' + + end end diff --git a/spec/redis_objects_model_spec.rb b/spec/redis_objects_model_spec.rb index 17c2e62a..f72bec85 100644 --- a/spec/redis_objects_model_spec.rb +++ b/spec/redis_objects_model_spec.rb @@ -966,9 +966,13 @@ class CustomIdFieldRoster < UidRoster end it "should pick up class methods from superclass automatically" do + Roster.redis_prefix.should == 'roster' CounterRoster = Class.new(Roster) + CounterRoster.redis_prefix.should == 'counter_roster' CounterRoster.counter :extended_counter extended_roster = CounterRoster.new + Roster.redis_prefix.should == 'roster' + extended_roster.class.redis_prefix.should == 'counter_roster' extended_roster.basic.should.be.kind_of(Redis::Counter) extended_roster.extended_counter.should.be.kind_of(Redis::Counter) @roster.respond_to?(:extended_counter).should == false From df5e85532d966b7fd606b8480fc6be3c98aff42b Mon Sep 17 00:00:00 2001 From: Nate Wiger Date: Wed, 22 Jun 2022 12:59:13 -0700 Subject: [PATCH 4/6] added waring message and additional tests for new key naming re #231 --- spec/redis_legacy_key_naming_spec.rb | 19 +++++++++++++++++++ 1 file changed, 19 insertions(+) diff --git a/spec/redis_legacy_key_naming_spec.rb b/spec/redis_legacy_key_naming_spec.rb index 6e069ec8..edf5694a 100644 --- a/spec/redis_legacy_key_naming_spec.rb +++ b/spec/redis_legacy_key_naming_spec.rb @@ -256,6 +256,25 @@ def id obj2.redis_value2.should.be.kind_of(Redis::Value) obj2.redis_value.key.should == 'dynamic_class2:1:redis_value' obj2.redis_value2.key.should == 'dynamic_class2:1:redis_value2' + end + + it 'issues a warning if a key name change is detected' do + module Nested + class LevelNine + include Redis::Objects + self.redis = Redis.new(:host => REDIS_HOST, :port => REDIS_PORT) + + def id + 1 + end + + value :redis_value + end + end + obj = Nested::LevelNine.new + val = SecureRandom.hex(10) + obj.redis_value = val + obj.redis_value.should == val end end From 9b44c1d77d1a44182adec3ab161449ea7ba0c1d4 Mon Sep 17 00:00:00 2001 From: Nate Wiger Date: Wed, 22 Jun 2022 13:00:10 -0700 Subject: [PATCH 5/6] Revert "added waring message and additional tests for new key naming re #231" This reverts commit df5e85532d966b7fd606b8480fc6be3c98aff42b. --- spec/redis_legacy_key_naming_spec.rb | 19 ------------------- 1 file changed, 19 deletions(-) diff --git a/spec/redis_legacy_key_naming_spec.rb b/spec/redis_legacy_key_naming_spec.rb index edf5694a..6e069ec8 100644 --- a/spec/redis_legacy_key_naming_spec.rb +++ b/spec/redis_legacy_key_naming_spec.rb @@ -256,25 +256,6 @@ def id obj2.redis_value2.should.be.kind_of(Redis::Value) obj2.redis_value.key.should == 'dynamic_class2:1:redis_value' obj2.redis_value2.key.should == 'dynamic_class2:1:redis_value2' - end - - it 'issues a warning if a key name change is detected' do - module Nested - class LevelNine - include Redis::Objects - self.redis = Redis.new(:host => REDIS_HOST, :port => REDIS_PORT) - - def id - 1 - end - - value :redis_value - end - end - obj = Nested::LevelNine.new - val = SecureRandom.hex(10) - obj.redis_value = val - obj.redis_value.should == val end end From 91ad61aa829d8d228c3e3342bc37dc88dd70e6ce Mon Sep 17 00:00:00 2001 From: Nate Wiger Date: Wed, 22 Jun 2022 13:05:02 -0700 Subject: [PATCH 6/6] added test for warning message --- spec/redis_legacy_key_naming_spec.rb | 38 ++++++++++++++++++++++++++++ 1 file changed, 38 insertions(+) diff --git a/spec/redis_legacy_key_naming_spec.rb b/spec/redis_legacy_key_naming_spec.rb index 6e069ec8..26663f38 100644 --- a/spec/redis_legacy_key_naming_spec.rb +++ b/spec/redis_legacy_key_naming_spec.rb @@ -5,6 +5,20 @@ require 'securerandom' +require "stringio" + +def capture_stderr + # The output stream must be an IO-like object. In this case we capture it in + # an in-memory IO object so we can return the string value. You can assign any + # IO object here. + previous_stderr, $stderr = $stderr, StringIO.new + yield + $stderr.string +ensure + # Restore the previous value of stderr (typically equal to STDERR). + $stderr = previous_stderr +end + describe 'Legacy redis key prefix naming compatibility' do it 'verifies single level classes work the same' do class SingleLevelOne @@ -256,6 +270,30 @@ def id obj2.redis_value2.should.be.kind_of(Redis::Value) obj2.redis_value.key.should == 'dynamic_class2:1:redis_value' obj2.redis_value2.key.should == 'dynamic_class2:1:redis_value2' + end + + it 'prints a warning message if the key name changes' do + module Nested + class LevelNine + include Redis::Objects + self.redis = Redis.new(:host => REDIS_HOST, :port => REDIS_PORT) + + def id + 1 + end + + value :redis_value + end + end + + captured_output = capture_stderr do + # Does not output anything directly. + obj = Nested::LevelNine.new + val = SecureRandom.hex(10) + obj.redis_value = val + obj.redis_value.should == val + end + captured_output.should =~ /Warning:/i end end