Issue with respond_to? method #173

Closed
pavelbrylov opened this Issue Dec 9, 2012 · 18 comments

Comments

Projects
None yet
4 participants

Hello. I faced a problem while switching active_admin from meta_search to ransack, it seems that it happens when there is a relationship to non-AR class, and Ransack::Adapters::ActiveRecord::Context is trying to cycle through requested string splitted by underscore. It finds non-AR relation and tries to call ransackable_attributes method, which is not there.

Exception from sample spec:

  1) Ransack::Adapters::ActiveRecord::Base#search should respond properly to respond_to?
     Failure/Error: subject.respond_to?("medium_type_wtf").should eq false
     NoMethodError:
       undefined method `ransackable_attributes' for Medium:Class
     # ./lib/ransack/context.rb:109:in `ransackable_attribute?'
     # ./lib/ransack/adapters/active_record/3.1/context.rb:29:in `attribute_method?'
     # ./lib/ransack/adapters/active_record/3.1/context.rb:37:in `attribute_method?'
     # ./lib/ransack/nodes/grouping.rb:130:in `block in attribute_method?'
     # ./lib/ransack/nodes/grouping.rb:130:in `select'
     # ./lib/ransack/nodes/grouping.rb:130:in `attribute_method?'
     # ./lib/ransack/search.rb:79:in `respond_to?'
     # ./spec/ransack/adapters/active_record/base_spec.rb:23:in `block (3 levels) in <module:ActiveRecord>'

I put simple test which is failing now to illiustrate the problem:
pavelbrylov/ransack@a43cfc7

Unfortunately I'm not quite know how ransack is expected to work in this case, so I'm not sure how fix should look like, but I can try to make it if you point me how it should be done.

Just for the reference - this is the model in Active Admin where I found this case: https://github.com/pavelbrylov/active_admin/blob/master/lib/active_admin/comments/comment.rb

@pavelbrylov pavelbrylov referenced this issue in activeadmin/activeadmin Dec 9, 2012

Closed

Replace meta_search with ransack #1811

Member

radar commented Dec 10, 2012

That's something we've definitely not accounted for within Ransack; an AR model linking to a non-AR model. I assume that your model complies with the basic AR API though?

Did you look at the test? Because it looks like a valid case to me. Let me try to describe the case from ActiveAdmin:
There is a Comment model: https://github.com/pavelbrylov/active_admin/blob/master/lib/active_admin/comments/comment.rb which belongs_to resource (ActiveAdmin::Resource - https://github.com/pavelbrylov/active_admin/blob/master/lib/active_admin/resource.rb) and has :resource_type attribute which should be searchable by ransack. Here is a code:

belongs_to :resource, :polymorphic => true
belongs_to :author, :polymorphic => true

attr_accessible :resource, :resource_id, :resource_type, :body, :namespace

What happens is when you do something like

form_for @comment do |f|
  f.text_field :resource_type
end

rails calls @comment.respond_to? "resource_type", which returns true, and this is correct, but then, for some reason rails tries to call respond_to? "resource_type_before_type_cast" (value_before_type_cast method in https://github.com/rails/rails/blob/v3.2.8/actionpack/lib/action_view/helpers/form_helper.rb) and this one fails, cause the fallback method from ransack kicks in (attribut_method? in https://github.com/pavelbrylov/ransack/blob/master/lib/ransack/adapters/active_record/3.1/context.rb), finds @comment.resource attribute and tries to call @comment.resource.respond_to? "type_before_type_cast" which throws "undefined method" expection.

And that's why it feels like a bug: if I rename resource to something else, then that ransack fallback won't find anything and will return false, and it is kinda not good to break respond_to? behaviour for models which use ransack.

Adding additional respond_to? "ransackable_attribute" on resource which are found by fallback logic in ransack's respond_to? method might solve the issue, what do you think?

I hope it makes at least some sense :)

Member

radar commented Dec 10, 2012

In the test, you're associating a non-AR model (Medium) to an AR model (Person). AR doesn't support this by default. By having Medium be a non-AR-backed model, that is what is causing the error in your test.

Are you sure you want to make that a non-AR backed model and still have it defined as a belongs_to on the Person model? I don't understand this use case.

Contributor

seanlinsley commented Apr 3, 2013

@radar the problem here is that something is incorrectly configuring the AssociationReflection so that klass and class_name are set up, which doesn't make sense because this is a polymorphic association.

So some background. The project that this issue stemmed from is called Active Admin, which is effectively a DSL for building websites. Active Admin has a comments system built in, which has a polymorphic relationship with any models you register.

Comments are set up with Active Record, with a configuration like this:

class ActiveAdmin::Comment < ActiveRecord::Base
  belongs_to :resource, :polymorphic => true
  belongs_to :author,   :polymorphic => true
end

The likely problem is there's also a class named Resource:

module ActiveAdmin
  # Resource is the primary data storage for resource configuration in Active Admin
  #
  # When you register a resource (ActiveAdmin.register Post) you are actually creating
  # a new Resource instance within the given Namespace.
  #
  # The instance of the current resource is available in ResourceController and views
  # by calling the #active_admin_config method.
  class Resource
    # stuff
  end
end

Somehow that pollutes the association object that Comment has.

What it should output: (from rails console)

ActiveAdmin::Comment.reflect_on_association :resource
#  #<ActiveRecord::Reflection::AssociationReflection:... @macro=:belongs_to, @name=:resource,
#  @options={:polymorphic=>true}, @active_record=ActiveAdmin::Comment(...),
#  @plural_name="resources", @collection=false>

What I got while debugging, from within Ransack:

get_association('resource', ActiveAdmin::Comment)
#  #<ActiveRecord::Reflection::AssociationReflection:... @macro=:belongs_to, @name=:resource,
#  @options={:polymorphic=>true}, @active_record=ActiveAdmin::Comment(...),
#  @plural_name="resources", @collection=false,
#
#  Then the unexpected attributes:
#  @foreign_key="resource_id", @foreign_type="resource_type", @type=nil, 
#  @class_name="Resource", @klass=ActiveAdmin::Resource>

Since Ransack calls klass on the association object, that's where this error comes from.

@seanlinsley seanlinsley referenced this issue in activeadmin/activeadmin Apr 3, 2013

Closed

[1685] Move from Metasearch to Ransack #1979

Contributor

seanlinsley commented Apr 16, 2013

After a bit more research, here's what Ransack attempts:

ActiveAdmin::Comment.reflect_on_association(:resource).klass.
  attribute_method? :resource_type

The problem is it's calling klass on a polymorphic belongs_to association. That works just fine if you're dealing with a properly created Comment instance, because the association is set up. But when you try doing so in this way, Rails just tries to populate klass with any existing constant in your application. It just so happens that Active Admin has a Resource class, but that isn't necessary for this to happen.

In fact, given the common setup:

class Comment
  belongs_to :commentable, polymorphic: true
end
class User
  has_many :comments, as: :commentable
end

You'd get an uninitialized constant error for Commentable

I'm not familiar enough with the Ransack codebase; do you have any thoughts on this @radar?

Contributor

seanlinsley commented Apr 17, 2013

I'm using this as a workaround for now, since Ransack seems incapable of handling this scenario.

class ActiveAdmin::Comment
  def self.ransackable_associations(auth_obj)
    []
  end
end
Member

radar commented Apr 17, 2013

For reasons like this I actively discourage the use of ActiveAdmin. There's so much ... ⚡️MAGIC⚡️ ... that goes on within ActiveAdmin and it causes all sorts of problems. On more than one occasion I've considered documenting all the problems that people have in #RubyOnRails (on Freenode) because they're using ActiveAdmin.

I don't want to fix this at all if it involves looking at ActiveAdmin source. If you can reproduce outside the context of ActiveAdmin, I'm more than happy to take a look.

Contributor

seanlinsley commented Apr 17, 2013

My last two comments did just that. I'll write failing tests for Ransack tomorrow.

There isn't any evidence this is a problem with Active Admin. At this point it looks like Ransack doesn't handle both sides of a polymorphic association nicely.

Specifically given this scenario:

class Comment
  belongs_to :commentable, polymorphic: true
end
class User
  has_many :comments, as: :commentable
end

Ransack tries incorrectly building and using Comment's commentable association when searching for commentable_type.

(Given, I switched the names around from the Active Admin version for simplicity, but it should be the same.)

Member

radar commented Apr 17, 2013

Ok, thanks @daxter. Some tests would be great.

Contributor

seanlinsley commented Apr 17, 2013

Hmm...

# this works
Comment.search(commentable_type_cont: 'e').result

# But when building a form:
form_for the_current_ransack_search_object do |f|
  f.input :commentable_type
end
# `ActionView::Helpers::InstanceTag#value_before_type_cast` calls this:
object.respond_to? "commentable_type_cont_before_type_cast"
# and `Ransack::Search#respond_to?` calls this:
base.attribute_method? "commentable_type_cont_before_type_cast"
# which ends up calling `.klass` on the polymorphic association relfection,
# which returns an incorrect object.

So yeah.

I put together a Gist that sets up the scenario. I haven't yet figured out how to generate forms in a similar manner.

Contributor

seanlinsley commented Apr 17, 2013

Basically, the form builder is trying to populate the field with the previous value. It's this line:

options["value"] = options.fetch("value"){ value_before_type_cast(object) } unless field_type == "file"

I suppose a fix on our side would be to manually pass in the value, but that shouldn't be needed.

Contributor

seanlinsley commented Apr 18, 2013

Perhaps related to #152

Contributor

seanlinsley commented May 6, 2013

I don't know how I didn't notice before, but this only happens when trying to build a text_field. Doesn't occur if I build a select.

Also, I should note that we're using Formtastic. Apparently Ransack doesn't have 100% compatibility with Formtastic?

Member

jonatack commented Sep 5, 2014

This is more than a year old and I believe this has been fixed since. Closing for now. Let us know if it is still an issue.

@jonatack jonatack closed this Sep 5, 2014

Contributor

seanlinsley commented Sep 6, 2014

Is there something specific that would lead you to believe this has been fixed?

Member

jonatack commented Sep 6, 2014

There has been no activity on this since more than a year, Ransack (and active_admin) have changed during that time, and I'm housecleaning the many old, stalled issues by following the Contributing guide:

Any issue that is open for 14 days without actionable information or activity
will be marked as "stalled" and then closed.

As I wrote just above, please advise if this is still an issue. Otherwise it's pretty doubtful that anyone will spend time digging into an issue without activity for more than a year. Thanks!

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