Model keys persist in Redis after ActiveRecord::Base model destruction #40

Open
nuclearsandwich opened this Issue Aug 29, 2011 · 19 comments

Projects

None yet

7 participants

When using Redis::Objects' very handy model attributes, the redis keys are not cleaned up upon model deletion.

I propose to add a cleanup method which will check Redis::Objects.redis for any keys belonging to the current object and delete any found.

This can then be added as a before_delete hook to active record or similar hooks for other ORMs, keeping it flexible. If this is welcome I will start out by adding failing tests to displaying the current behavior and then implementing my solution as suggested above.

I would love your thoughts Nate.

Owner

This sounds like a good idea offhand. I think this should go in an after_destroy hook, though, since before_ hooks can alter/abort the normal flow of a destroy (meaning, a user could add a before_destroy hook that cancels the destroy, but Redis::Objects' hooks would still be triggered, causing potential havok).

Thanks,
Nate

Owner

Ugh

after_destroy

Callback

Okay, I will take a crack at this and do a pull request when I can. Thanks.

From the spelunking I did this this morning, it doesn't look like a directory or list of redis objects for a model is kept anywhere. I see two possible ways to implement a cleanup feature.

The simple but slightly dangerous, is just to delete from redis all keys that match "#{klass}:#{id}:*".

The other, safer, is to add an instance variable containing an array of all redis objects for that model object. Then have a method which calls it like

def destroy_redis_objects_keys
  @redis.del *redis_objects.map(&:key)
end

I would like to keep things safe without increasing complexity more than necessary. What are your thoughts? Is there a better way or middle ground?

Owner
nateware commented Sep 3, 2011

There is an instance variable called redis_objects that has all the keys, along with their options. You can see it here:

# lib/redis/objects.rb

# Instance methods that appear in your class when you include Redis::Objects.
module InstanceMethods
  def redis() self.class.redis end
  def redis_field_key(name) #:nodoc:
    klass = self.class.first_ancestor_with(name)
    if key = klass.redis_objects[name.to_sym][:key]
      if key.respond_to?(:call)
        key.call self
      else
        eval "%(#{key})"
      end
    else
      if id.nil? and !klass.redis_objects[name.to_sym][:global]
        raise NilObjectId,
          "Attempt to address redis-object :#{name} on class #{klass.name} with nil id (unsaved record?) [object_id=#{object_id}]"
      end
      # don't try to refactor into class redis_field_key because fucks up eval context
      "#{klass.redis_prefix}:#{id}:#{name}"
    end
  end

That should contain all the redis_object variable names for a given class. But you'd have to do a little debugging to make sure.

I'm confused. Is there an instance variable or class variable called redis_objects I see a class method redis_objects being called during the definition of redis_field_key but no where in this snippet nor elsewhere do I see an instance method redis_objects or instance variable @redis_objects defined. Was it removed at some point?

Owner
nateware commented Sep 7, 2011

If you're in an instance of an AR class, you should be able to say

self.class.redis_objects

To get to this variable. So, if you're adding tests to:

https://github.com/nateware/redis-objects/blob/master/spec/redis_objects_model_spec.rb

Then you should be able to do:

@roster = Roster.new
@roster.class.redis_objects.inspect

And get a dump of all the keys and their options:

 {:available_slots=>{:start=>10, :type=>:counter}, :pitchers=>{:limit=>:max_pitchers, :start=>0, :type=>:counter}, :basic=>{:start=>0, :type=>:counter}, :contact_information=>{:marshal_keys=>{"updated_at"=>true}, :type=>:dict}, :resort_lock=>{:timeout=>2, :type=>:lock}, :starting_pitcher=>{:marshal=>true, :type=>:value}, :player_stats=>{:marshal=>true, :type=>:list}, :outfielders=>{:marshal=>true, :type=>:set}, :rank=>{:type=>:sorted_set}, :total_players_online=>{:global=>true, :start=>0, :type=>:counter}, :all_players_online=>{:global=>true, :type=>:set}, :last_player=>{:global=>true, :type=>:value}, :player_totals=>{:key=>"players/\#{username}/total", :start=>0, :type=>:counter}, :all_player_stats=>{:key=>"players:all_stats", :global=>true, :type=>:list}, :total_wins=>{:key=>"players:\#{id}:all_stats", :type=>:set}, :my_rank=>{:key=>"players:my_rank:\#{username}", :type=>:value}, :weird_key=>{:key=>"players:weird_key:\#{raise}", :global=>true, :type=>:value}, :daily=>{:global=>true, :key=>#<Proc:0x00000100ba3280@spec/redis_objects_model_spec.rb:32>, :start=>0, :type=>:counter}}

To use this in an after_destroy hook, you'd say:

class Roster < ActiveRecord::Base

  after_destroy :delete_redis_keys
  def delete_redis_keys
    self.class.redis_objects.each do |key,opts|
      # redis del(key) commands go here
    end
  end
end
Owner
nateware commented Sep 7, 2011

One last wrinkle: Since keys can be dynamically interpolated, for example:

:total_wins=>{:key=>"players:\#{id}:all_stats", :type=>:set}

You'll need to use the redis_field_key() helper to get the actual name:

 class Roster < ActiveRecord::Base

  after_destroy :delete_redis_keys
  def delete_redis_keys
    self.class.redis_objects.each do |key,opts|
      k = redis_field_key(key)
      redis.del(k)
    end
  end
end

That's probably pretty much it more or less

That is pretty much exactly what I have locally. I was juat worried that I was missing something. Will pull request implementation, tests, and docs tonight or early tomorrow. Thanks for your guidance Nate.

Sent from my Android phone with K-9 Mail. Please excuse my brevity.

Nate Wiger reply@reply.github.com wrote:

One last wrinkle: Since keys can be dynamically interpolated, for example:

:total_wins=>{:key=>"players:#{id}:all_stats", :type=>:set}

You'll need to use the redis_field_key() helper to get the actual name:

class Roster < ActiveRecord::Base

after_destroy :delete_redis_keys
def delete_redis_keys
self.class.redis_objects.each do |key,opts|
k = redis_field_key(key)
redis.del(k)
end
end
end

That's probably pretty much it more or less

Reply to this email directly or view it on GitHub:
#40 (comment)

mguterl commented Dec 12, 2011

@nuclearsandwich or @nateware was this ever finished?

I finished a hacked out implementation, but never got the tests passing or cleaned it up, then I got hit with a wave of rough schoolweeks. I am not at the computer with my work on it at this time. Tonight I'll push what I had up to my branch If you're interested in working on it from there feel free. Otherwise I won't get to it until after the new year. But, if you don't beat me to it, I will get to it.

Sorry for the delays all, it was a rough semester.

pheino commented Oct 19, 2012

Nate - Any progress? We could really use this patch.

Owner

Hey Peter - there wasn't an agreed-upon patch (yet) for this issue. Can you test to see if my suggestion of:

class Roster < ActiveRecord::Base

  after_destroy :delete_redis_keys
  def delete_redis_keys
    self.class.redis_objects.each do |key,opts|
      # redis del(key) commands go here
      redis.del(redis_field_key(key))
    end
  end
end

Works for you?

@nateware nateware closed this Feb 22, 2013
Contributor

Hi, just to make sure: redis-objects currently does not delete the record's redis keys on after_destroy?

eljojo commented Nov 28, 2014

Hi, I'm interested in implementing this as a feature, as long as my time allows me.
If I open a pull request with tests and documentation updates, would be interest in merging it?

Contributor

@eljojo I know my team would. Not only on after_destroy, but after_rollback as well.

Consider the case where you build an object A with an association B. There's a before_save that stores information using redis-object, but B isn't saved so the whole thing rolls back. You're left with data in redis that are using an id as key for an object that doesn't exist. Even worse: you create a new object later which gets the same id, and it will have the redis info from the previous rolled-back transaction.

Owner

Yes if you can find a way to generalize this, I think it would be a great addition. BUT, there can't be any dependencies on ActiveRecord in the cosebase. I think this should be provided as a new config option that people can put in an initializer, such as:

Redis::Objects.use_activerrecord_hooks!

Or something similar.

Thanks,
Nate

@nateware nateware reopened this Nov 29, 2014
eljojo commented Dec 17, 2014

sorry to get everyone excited, i realized i don't have enough time for this :(
If I find some time i'll open a PR, but I guess for the time being is good to close this.

Contributor
rossta commented May 1, 2015

I'd argue that this feature belongs in a separate gem specific to Redis::Objects integration with ActiveRecord instead of this project.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment