Finding records without find method. #19

Closed
molte opened this Issue Dec 16, 2009 · 20 comments

Projects

None yet

4 participants

molte commented Dec 16, 2009

The load_resource method should support finding database records by other columns than the id - eg. it should be possible to let it do MyModel.find_by_permalink!(params[:id]).

Owner
ryanb commented Dec 17, 2009

It is possible to customize the find behavior using a before filter.

before_filter :find_by_permalink, :only => :show
load_resource

def find_by_permalink
  @book = Book.find_by_permalink!(params[:id])
end

This is mentioned in the readme, will it satisfy this issue? I could add a :find_attribute option to load_resource, but this approach is much more flexible. One might not only need to fetch through a different attribute, but also a different association or named scope.

@book = current_user.books.find(params[:id])
# or
@book = Book.published.find(params[:id])

Does that make sense?

molte commented Dec 17, 2009

I understand what you mean.
However, with the current method it would require some more manual work (especially with nested resources), which I'd like to avoid. If you do choose to implement the option, make sure that the current method would still be possible - it is, as you say, more flexible.

Owner
ryanb commented Dec 17, 2009

Another hesitation I have is that this can only be set for the current model and not a nested parent resource (without getting messy nested hashes). But I'll let this issue cook a little bit and get some more feedback.

On a related note, I had originally considered adding find_book style callback methods one could override in the controller instead of using a before filters. This has the advantage of making nested resource loading easier to override, but I think it makes things a little more complex.

class BooksController < ActionController
  load_and_authorize_resource :nested => :author

  private
  def find_book
    @author.books.find_by_permalink!(params[:id])
  end
end

I've also had to skip the load_and_authorize_resource approach as we were using some special to_params stuff. So it's a +1 from me.

Sorry :)

molte commented Dec 21, 2009

I think I've come up with some sort of a solution:

I've wrote a tiny plugin acts_as_complex_param which sets up a find_by_param! method on the model. CanCan could then do the following when finding records:

self.model_instance ||= (base.try(:find_by_param!, id) || base.find(id))

That would allow a lot of flexibility without requiring more options for the load_resource methods - even with nested controllers.

And of course; if people don't want to use the plugin, they can just define the method manually.

Owner
ryanb commented Dec 21, 2009

@lawrencecurtis, it's good to know others find a use for this, I'll definitely consider adding it.

@molte, expecting a method named "find_by_param!" is no better than an explicit option in my eyes. I'll probably add a find_by option like this.

load_and_authorize_resource :find_by => :permalink
molte commented Dec 21, 2009

With an explicit option you'd have to specify it several times when working with nested resources (eg. if an article has many comments, you'd need to specify the article "find_by" it both in the ArticlesController and CommentsController).

A method is also much more flexible than just a symbol (as you can combine several attributes etc.).

It's yet another dependancy though, the great thing about cancan is what it gives you out the the box, it already relies on something like authlogic, i think if you are using custom find_by methods then you can establish those in a block....

load_and_authorize_resource :find_by => :permalink

or

load_and_authorize_resource :nested => :comment do 
  @post = Post.find_by_permalink params[:post_id]
  @comment = @post.comments.find_by_permalink params[:id]
end

or maybe even better...

  load_and_authorize_resource :nested => :comment, :find_by => { 
    :post => :permalink,
    :comment => :permalink
  }
molte commented Dec 21, 2009

Shouldn't it then be :nested => :post? And it would be easier with an array instead of a hash.

if its an array you can't specify one as id and one as permalink etc etc, just makes it more verbose (easier to read) and more flexible, and yeah, it should be post not comment :)

molte commented Dec 22, 2009

With an array I meant like :find_by => [:permalink, :permalink].

I still think it's harder to read :)

molte commented Dec 22, 2009

Though I still believe it would be better with a method in the model..
It's more DRY and maintainable: You don't have to repeat yourself in every controller where the primary resource is nested in the specific resource. In addition to that (when using my plugin) you wouldn't have to manually specify the to_param method. I really believe it's logic that belongs to the model.

Owner
ryanb commented Dec 22, 2009

@molte, good point about it being more DRY. What if the method was called from_param? That has a nice symmetry with to_param.

class Book < ActiveRecord::Base
  def self.from_param(param)
    find_by_permalink!(param)
  end
  
  def to_param
    permalink
  end
end

However there are other options that this same argument can be applied to, such as the :class option. That has the same DRY/nesting issue as this :find_by option. I would like to stay consistent in how this is handled and it's impossible to DRY :class by using methods in the class. Hmmm...

molte commented Dec 22, 2009

I don't know whether from_param is better. I originally chose find_by_param! to follow the find_by naming convention.

I'm using the "friendly_id" plugin: http://github.com/norman/friendly_id for my permalinks and have zero issues using it along with CanCan. For anyone wanting "pretty urls" you might want to consider looking in to this plugin.

molte commented Dec 29, 2009

@meskyanichi The plugin you're speaking about looks quite nice; seems to solve the issue.

Owner
ryanb commented Dec 31, 2009

The friendly_id plugin is a nice find. The only problem I have with it is that it will accept the id number in the params[:id] as well as the permalink. Often you don't want this for security reasons or for SEO. The solution in the documentation is to add a separate exception or redirect line if an id number is used, but that kind of defeats the purpose and convenience of the plugin.

The more I think about it, the more I am leaning toward using a method defined in the model for this (such as find_by_param!). It would be nice if this was some kind of convention that other resource loading plugins would adopt.

Owner
ryanb commented Aug 6, 2010

adding :find_by option to load_resource - closed by 236cece

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