exists? returns false positives when query string begins with a number

[corwinstephen]

The exists method falls back on ActiveRecord's implementation which runs to_i on the input string. This means that if the slug you're searching for begins with a number, you'll get a false positive. Model.friendly.exists?('2something') ultimately performs Model.exists?(2), which usually returns true.

[parndt]

@corwinstephen thanks! Do you think we need to do something like return false if input.to_i.to_s != input?

[corwinstephen]

@parndt were it up to me, I might argue that

friendly_id/lib/friendly_id/finder_methods.rb

Line 31 in 6a8b54e

super

should be changed to false, though that may have other consequences I'm not aware of. An alternative could be to explicitly check to see if input.to_i != 0 there and return false if it is, and only call super after that.

It's a little bit of a tricky situation since ActiveRecord itself allows you to pass non-integer values to find, so we'd be modifying Rails behavior. But at the same time, that should only ever be a problem if someone were using friendly_id in combination with a non-numeric primary key, which would totally break anyway, so maybe that's not an issue?

[parndt]

What I was meaning is that we should be using friendly ID behaviour if the slug is not fully numeric, which is what I meant by input.to_i.to_s != input - in this case we should be searching our database for a slug matching that. In the case where they are the same we can ask the upstream (ActiveRecord) for its result as it's numeric.

Does that help?

[corwinstephen]

@parndt if I'm not mistaken, that's what already happens. The problem stems from the fact that if the search for the friendly_id slug doesn't return anything, we then call super, which is where the false positive comes from. This differs from the implementation of model.friendly.find in that find raises an exception if nothing matches the slug whereas exists? calls super. I think we could get parity between the two methods by returning false instead.

[parndt]

yes, I think you're right @corwinstephen - if people are using model.friendly.find and the slug doesn't exist then we shouldn't then ask the upstream. Would you like to open a pull request please so that we can see how the tests deal with this change?

[stale]

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.