Hacker News Reader

Comment by Yahivin | Hacker News Reader

The standard way to find a post in Rails would be:

    Post.find(params[:id])

That method is unaffected.

The methods that are affected by this are the dynamic finder methods `find_by_*` such as:

    Post.find_by_id(params[:id])

This would most commonly occur when looking up users by a token or some other piece of data other than the id.

    User.find_by_token(params[:token])

I'm not sure why they chose to use find_by_id in the example. This is a serious bug, but it's not as serious as one might be lead to believe if one thought it was the standard way to find objects in Rails.

View on HN

I believe if you do Post.find on a nonexistent id, a ActiveRecord::RecordNotFound exception is thrown, but this doesn't happen with the dynamically generated finders like find_by_id. Given this difference, it's possible you want to handle a missing record in a more graceful, non exception catching matter.

Personally I would do Post.where(id: params[:id]).first since I tend not to like dynamic finders anyway.

True, but :find_by_id is closer to :where in that it doesn't raise exceptions upon not finding a record. I've seen it being used in a few places, especially in front another scope, like `current_user.posts.find_by_id`.

If you have an invalid id coming in, wouldn't you generally want an Exception? What is the use case where the app would send an id that does not exist, but you would not want to fail?

nonexistent does not necessarily imply invalid. I don't use rails, so take my ignorance into account, but I'd hate to catch exceptions all the time instead of just checking for falsey values.

Ex: "Enter your student ID"

    s = Student.find_by_id(params[:id])
    if s
        # do stuff
    else
        # do other stuff
    end

    begin
        s = Student.find_by_id(params[:id])
        # do stuff
    rescue
        # do other stuff
    end

This is a good example, however I would argue that it is bad design to have a record's ID used for user facing lookups. For other parameters, sure (find_by_name, etc) a false would be okay. Just a matter of taste really.

No argument here, good point.

If you want an exception, there is a banged version:

    # note the exclamation mark
    User.find_by_id!(id) # => raises exception when nothing is found

While the example given here is :find_by_id, the reality of the isse is that it affects the dynamic finder methods. This means that it affects the :find_by_any_column_in_the_table methods. The only reasonable situation in which to throw an error in those methods is when you don't hit a column, not when you don't find a record, IMO.

I agree on this point

Reply on news.ycombinator.com