[2.1.1] Arel::Visitors::Visitor's global DISPATCH cache breaks when some visitors don't implement all visit methods #57

Closed
jhtwong opened this Issue Jun 2, 2011 · 0 comments

Comments

Projects
None yet
2 participants
@jhtwong

jhtwong commented Jun 2, 2011

I've been writing some new ARel visitors and ran into a confusing problem where if I run certain custom visitors, some of their visit_* methods won't be called when expected.

Turns out the issue has to do with the global DISPATCH cache in Arel::Visitors::Visitor - since the default visit method is inherited into subclasses, all visitors share the same cache!

This means if I have a visitor that implements only visit_Arel_Nodes_Join (but not visit_Arel_Nodes_OuterJoin and visit_Arel_Nodes_InnerJoin), the method would never be called if another visitor (e.g. ToSql) has been run before. This is because the cache would indicate that a Arel::Nodes::OuterJoin should always be visited via visit_Arel_Nodes_OuterJoin, regardless of what is implemented in that particular visitor.

One solution I found is to replace the constant with a per-class-object cache:

def self.dispatch_cache
  @dispatch_cache ||= Hash.new do |hash, klass|
    hash[klass] = "visit_#{(klass.name || '').gsub('::', '_')}"
  end
end

def visit object
  send self.class.dispatch_cache[object.class], object
rescue NoMethodError => e
  raise e if respond_to?(self.class.dispatch_cache[object.class], true)
  superklass = object.class.ancestors.find { |klass|
    respond_to?(self.class.dispatch_cache[klass], true)
  }
  raise(TypeError, "Cannot visit #{object.class}") unless superklass
  self.class.dispatch_cache[object.class] = self.class.dispatch_cache[superklass]
  retry
end

Hopefully this could make it into the next release of ARel.

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