3

I have the following action:

def show
  @video = @commentable = @favoritable = Video.find_by_key(params[:key])

  Video.increment_counter(:views_count, @video.id)
end

But right now if find_by_key doesn't find a record, it throws a RuntimeError (Called id for nil).

So how can I get that action to throw a 404 if a record isn't found?

I'm running Rails 3.2.1.

Shpigford
  • 24,748
  • 58
  • 163
  • 252
  • A 404 for not finding a record? Pretty user-hostile. – Dave Newton Mar 14 '12 at 14:51
  • 3
    @DaveNewton How so? The page doesn't exist since the record doesn't exist...seems that's exactly what a 404 would be used for. – Shpigford Mar 14 '12 at 14:57
  • As a user I'd expect a message saying "that record doesn't exist" so I know the site isn't broken. In addition, a not-found page might suggest alternatives, provide useful links, etc. Go to almost any website and search for something not on the site--count how many throw a 404. – Dave Newton Mar 14 '12 at 15:05
  • 2
    @DaveNewton Rails automatically renders the public/404.html page after an `ActiveRecord::RecordNotFound` error in production. I assume Shpigford would customize the file, and even the default 404.html page has a message with "the page you were looking for doesn't exist." I'm not sure what you're trying to argue here. – James Mar 14 '12 at 15:14
  • @James I think what my argument is is pretty obvious. Most every site in the world doesn't show a 404 page when a search fails, nor do *I* believe it makes *any* sense to do so. *You* (and others) may feel differently, and I'm fine with that. I think it's an ugly, user-surprising, unhelpful response to looking up something that doesn't exist. Customizing the page to include, say, a flash message saying "That foo" or "That bar" doesn't exist is great, but IMO type-specific lookup failures are better handled with a non-generic page. I'm okay with people feeling differently. – Dave Newton Mar 14 '12 at 15:21
  • 1
    @DaveNewton Go back to your first comment on this question. You expected a message saying that the record doesn't exist and that the page might suggest alternatives, provide useful links, etc. That's exactly what the generic Rails 404 page does or can be customized to do. I'm not even sure what you mean by "throwing a 404" anymore because every site throws a 404 in those situations by responding with a 404 status code. It's also what's expected for seo reasons. http://stackoverflow.com/questions/0 responds with a 404 status code. – James Mar 14 '12 at 15:38
  • @James "All"? Except the ones that don't, I guess, sure. Carry on. – Dave Newton Mar 14 '12 at 16:02
  • 1
    @DaveNewton Can you at least clarify what you mean by "throwing a 404"? Stack Overflow clearly responds with a 404 status code when a page/record isn't found. – James Mar 14 '12 at 16:10
  • @James I'm almost 100% positive you know what returning/throwing a 404 means. SO is one site; there are actually others. I don't really understand why we can't agree to disagree. Or rather why you can't. – Dave Newton Mar 14 '12 at 16:11
  • @DaveNewton Sure, SO is just one site, but https://twitter.com/@ , http://www.amazon.com/@, and http://www.google.com/@ all respond with 404 status codes. I've given examples and you haven't given any. – James Mar 14 '12 at 16:16
  • (@James I also differentiate between just throwing a 404 with a generic page, and a 404 with a custom page; looking over what I said I definitely did not make that point, which is on me. For example, I don't like Amazon's 404 for looking up a product.) – Dave Newton Mar 14 '12 at 16:42
  • possible duplicate of [How to redirect to a 404 in Rails?](http://stackoverflow.com/questions/2385799/how-to-redirect-to-a-404-in-rails) – Brad Werth Sep 02 '14 at 15:52

1 Answers1

14

Add this before you try to increment using the value of @video.id.

raise ActiveRecord::RecordNotFound if @video.blank?

Or use the bang version of find_by_key which will raise ActiveRecord::RecordNotFound if it isn't found.

Video.find_by_key!(params[:key])

James
  • 4,797
  • 25
  • 29