nateware/redis-objects

model name prefix is not always unique

khiav223577 opened this issue · 10 comments

khiav223577 commented

The document here says:

Redis::Objects automatically creates keys that are unique to each object, in the format:
model_name:id:field_name

But if two models in different namespace have same name. The generated key will be the same.
For example:

class Group < ActiveRecord::Base
  include Redis::Objects
  counter :test_counter
end

class XXX::Group < ActiveRecord::Base
  include Redis::Objects
  counter :test_counter
end
Group.first.test_counter.key
# => "group:1:test_counter"

XXX::Group.first.test_counter.key
# => "group:1:test_counter"
nateware commented

Nested class names are a problem because I stole the original code from Sinatra, and unfortunately I didn't notice that it cropped the class down past the final ::. Sorry. I am open to a new option something like full_prefix: true but unfortunately we can't change the code since that would be backwards incompatible.

nateware commented

I finally have an idea. If we bumped the version to 2.0.0, we could issue a warning message in the model_prefix method if we found an :: that we are now prefixing the full method name. And/or we could provide a global setting called long_namespacing where if you said it to false it would do the old shortened version. CC @tmsrjs

  • 👍1
artinboghosian commented

@nateware I think you might want to provide this as an option at the object level as well so that if you were using redis objects for a namespaced class upgrading wouldnt break existing code. Meaning you can turn on new behavior and get benefits of namespaced keys without breaking currently functioning code in your app.

nateware commented
artinboghosian commented

@nateware class level makes sense. Although it's probably an edge case this still wouldn't support existing namespaced classes with a mixture of new and old functionality. Say you add redis type after upgrading and want new definition to use namespaced keys even though existing ones wouldn't without migrating. Maybe we don't care about this but thought it might be worth mentioning. If we did care than it might make sense to add option at the instance level when defining new redis types on class.

Dounx commented

Like @khiav223577 said, developer barely cannot notice that models in different namespace could have same name. We set redis_prefix in every included class, If the new one does not know to set this, there will be problems. Still think that using model_name by default is not very reasonable.

Dounx commented

By the way, why redis_prefix should use sub and gsub?

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').
    downcase
 end

Can we just change it as follow:

def redis_prefix(klass = self) #:nodoc:
  @redis_prefix ||= klass.name.to_s.
    gsub(/::/, '_').
    downcase
end
nateware commented

Update on the design to fix this longstanding issue. The new code for redis_prefix looks like so:

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

The reason I chose __ as a substitute for :: was to ensure these don't conflict:

class Group::PlayerType  # group__player_type
class GroupPlayer::Type  # group_player__type

To cause the legacy key naming behavior, there is a new flag redis_legacy_naming:

class MyClass
  include Redis::Objects
  self.redis_legacy_naming = true

A warning will be emitted if redis-objects detects that your class name will change.

This is being developed in the long_namespace branch for those interested in following along.

nateware commented

This has been fixed in 2.0.0.alpha and pushed to rubygems

nateware commented

Also fixed in 2.0.0.beta which I just pushed to rubygems