Skip to content
This repository

finder fallback to super is problematic #197

Closed
danieldkim opened this Issue December 13, 2011 · 7 comments

4 participants

Daniel Kim Norman Clarke Agustin Vilaseca Ilan
Daniel Kim

friendly_id's override of ActiveRecord#find_one falls back to super if the find by friendly id returns nil. the result of this is that if an invalid friendly id that is passed to find() converts to an integer that matches the (unfriendly) id of an existing object, an incorrect object can be returned. here's an example from my app:

irb(main):002:0> Application.find '81D1E04CC0CC2C200BF8'
  Application Load (0.2ms)  SELECT `applications`.* FROM `applications` WHERE `applications`.`key` = '81D1E04CC0CC2C200BF8' LIMIT 1
  Application Load (0.2ms)  SELECT `applications`.* FROM `applications` WHERE `applications`.`id` = 81 LIMIT 1
=> #<Application id: 81, name: "Yet another one", description: "test", key: "646817DC1AC849619837", user_id: 41, created_at: "2011-11-19 01:33:15", updated_at: "2011-11-19 01:33:15">
irb(main):003:0>

The find() call above should generate an ActiveRecord::RecordNotFound exception. Instead, because the string ''81D1E04CC0CC2C200BF8' converts to the integer 81, and another object exists with an id of 81, the wrong object is returned.

Daniel Kim danieldkim referenced this issue from a commit December 13, 2011
Daniel Kim adds a :fallback_find option to friendly_id that disables fallback to…
… super behavior of find_one if set to false [#197]
b47e646
Norman Clarke
Owner

This is going to happen from time to time with some models, because there's no 100% foolproof heuristic for determining wether an id is "friendly" or "unfriendly." But I think the way around this is by being more explicit with the finder you want to use, rather than adding a config option.

If you want to ensure the correct finder is used, you have two options:

  • To make sure the numeric id is used, cast the id to a int with to_i, or do find_by_id.
  • To make sure the "friendly" one is always used, do find_by_slug rather than find.
Norman Clarke norman closed this December 14, 2011
Daniel Kim

Yeah, the problem is that if redirect_to still automatically outputs a url with the friendly id -- without me explicitly having to tell it to use the friendly id in the url -- but I have to use find_by_{friendly_id} instead of simply find when I implement the controller action behind that url, then half of the pretty mask of abstraction that friendly_id puts on resource identification/access is ripped away. It gives the code a sort of multiple-personality-ish quality when I output an identifier one way but have to interpret it in a different way. Apart from aesthetic concerns, since the abstraction is incomplete it prevents me from being able to switch out a new friendly id if I wanted to without having to change the parts of my code that process the url params.

I also like the configuration option because it simply prevents the possibility of the kind of error described above which imo would be easy to make as the purpose of friendly_id is to provide an abstraction that allows the user to think of the slug as the primary identifier of the resource.

Norman Clarke
Owner

I don't really like the approach of adding a configuration option to control the inner workings of something that is supposed to be transparent. With FriendyId 4 I have tried to eliminate exactly that pattern, which was all over the place in FriendlyId 3 and I grew to consider to be a defect.

I think any time you have a method name as a value in an argument hash, that's a code smell that should led you to look for a better way to approach the problem. I'm not saying that is always wrong, but to me, here, it's not the approach I would like to take.

I think in this case, a better approach would be to extend to finder relation with the method that you want to use. This is the approach used by the History model. This technique preserves the abstraction that you like, and accomplishes what you want to achieve using plain old Ruby OOP rather than a patch:

module FriendlyFinderWithFallback
  def find_one(id)
    return super(id) if id.unfriendly_id?
    where(@klass.friendly_id_config.query_field => id).first or begin
      # your fallback finder code here
    end
  end
end

class MyModel < ActiveRecord::Base
  extend FriendlyId
  friendly_id :foo, :use => :slugged
  relation_class.send :include FriendlyFinderWithFallback
end
Daniel Kim

to be honest, i wouldn't have find_one ever fallback to super. i would rather extending FriendlyId cause the friendly id to be used as the primary identifier, period, and you would have to explicitly find_by_id if you wanted to actually find by the "id" field. then everything's very explicit and there are no surprises. you're either using a friendly id or you're not. no having your cake and eating it, too ;) (or sort of eating it but sometimes finding a nasty bug in your mouth). but, i digress ...

in any event, thanks for the tip on how to resolve this issue, norman. i don't see how this is more transparent, and it seems more tightly coupled to the internal implementation details of friendly_id, than the configuration option. but, regardless of which approach is better, i'd rather do this than have to maintain a fork. thanks again!

Agustin Vilaseca

I have a couple of questions:

One if what is the definition of unfriendly_id?
If it always supposed to represent a record set integer id, why not parse it as integer base 10 to make sure it is such. eg Integer(x, 10)

This way the problematic 'or find_one_without_friendly_id(id)' can be avoided.

Also, currently '112' is not recognized as an unfriendly_id and thus relies on this last method to correctly find the record.

Second, it would be useful to be define an additional method that finds by slug and never calls super.

Some people might choose to not allow ids in show actions, but allow it in others and find_by_slug doesn't quite get you there, since it breaks down when using for example the history module.

Then you achieve two things, a) no need for configuration option, b) no need to couple the fallback mechanism to the modules being used.

Thanks!

Norman Clarke
Owner

why not parse it as integer base 10 to make sure it is such. eg Integer(x, 10)

That's a good idea.

it would be useful to be define an additional method that finds by slug and never calls super.

Sure, I'm open to adding that. Like you said, the History module is exactly where this breaks down, and it would be good to have something consistent.

Ilan

Any solution for preventing access by id which doesn't break the History module?

(I don't want my third user to know he's the third user...)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Something went wrong with that request. Please try again.