Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Redirect to 404 if badge not found #2066

Merged
Merged
Changes from all commits
Commits
File filter...
Filter file types
Jump to…
Jump to file or symbol
Failed to load files and symbols.

Always

Just for now

@@ -3,7 +3,7 @@ class BadgesController < ApplicationController
# No authorization required for entirely public controller

def show
@badge = Badge.find_by_slug(params[:slug])
@badge = Badge.find_by_slug(params[:slug]) || not_found

This comment has been minimized.

Copy link
@lightalloy

lightalloy Mar 15, 2019

Collaborator

I suggest @badge = Badge.find_by!(slug: params[:slug]) . Using the bang method will raise ActiveRecord::RecordNotFound which makes more sense. The exception will be caught by rails 404 page will be rendered.

This comment has been minimized.

Copy link
@rhymes

rhymes Mar 15, 2019

Collaborator

Even better, I was trying to follow the style I found around the code, though improving on it it's definitely a plus 💃

This comment has been minimized.

Copy link
@benhalpern

benhalpern Mar 15, 2019

Collaborator

I feel like I was just doing it the way I saw someone else do it somewhere randomly online a few years ago and never really second-guessed the approach. Then it just became a pattern.

The code is filled with stuff like that, but we're digging our way out.

I like @lightalloy's suggestion but I also don't mind the current approach, which makes it super clear what's going on.

This comment has been minimized.

Copy link
@edisonywh

edisonywh Mar 16, 2019

Author Contributor

I do think @lightalloy's solution is cleaner, but have a couple of things to point out here in favour of explicit not_found

Advantage

  • It makes it obvious that the person who wrote the code has thought about 404 and have handled this scenario.
  • It's not obvious that the feature requires the ! -- someone trying to build a new feature might see this and thought there's no need for the bang, ensures a next round of the same bug

I'm still okay to change if the consensus is to use bang, what do you guys think?

This comment has been minimized.

Copy link
@lightalloy

lightalloy Mar 18, 2019

Collaborator

I agree that the current approach is more clear 🤔
Since we're discussing it, I'll add another thought on not_foun optimization if we keep the current approach. Currently, we're raising ActionController::RoutingError in the not_found method, but that's kind of using exceptions as flow control, which is slow. That's not actually an exceptional situation and we don't notify Airbrake of it. So the not_found method could just render the corresponding template and return 404 status, like:

render file: "#{Rails.root}/public/404.html", layout: false, status: :not_found

@ben @rhymes What do you think about this?

This comment has been minimized.

Copy link
@rhymes

rhymes Mar 18, 2019

Collaborator

@lightalloy if you rewrite

  def not_found
    raise ActionController::RoutingError, "Not Found"
  end

to become

  def not_found
    render file: "#{Rails.root}/public/404.html", layout: false, status: :not_found
  end

doesn't it mean that, in the various instances where it's used for example:

def show
@badge = Badge.find_by_slug(params[:slug]) || not_found
set_surrogate_key_header "badges-show-action"
end

the control will go back to the calling function and it will execute everything after the render? In this case we don't want to execute anything after the not_found because it might trigger side effects.

In this example

def show
  render file: "#{Rails.root}/public/404.html", layout: false, status: :not_found
  puts "i'm here after the render"
end

the line after render is executed.

I think that's why Rails by default uses the exception as an escape hatch, to avoid this situation.

This comment has been minimized.

Copy link
@rhymes

rhymes Mar 18, 2019

Collaborator

(you could add a return not_found but then you're back at the problem of making sure that someone doesn't forget the return or you'll have unintented consequences in the code)

My vote goes to

def not_found
  raise ActiveRecord::RecordNotFound, "Not Found"
end

which is what the ! raise anyway, this way we make it explicit but don't deviate from Rails default

This comment has been minimized.

Copy link
@lightalloy

lightalloy Mar 19, 2019

Collaborator

@rhymes Yep, I'm familiar to this problem. The change I suggested would require changing the code, where the method is used, which is probably not optimal.
I like your suggestion to alter the not_found method, this way the intentions are more clear. But probably it should be a topic of another pr.

This comment has been minimized.

Copy link
@rhymes

rhymes Mar 19, 2019

Collaborator

I'll do it in a separate PR once this is merged, if that's okay with you

set_surrogate_key_header "badges-show-action"
end
end
@@ -5,9 +5,17 @@
let(:badge) { create(:badge) }

describe "GET /badge/:slug" do
it "shows the badge" do
get "/badge/#{badge.slug}"
expect(response.body).to include CGI.escapeHTML(badge.title)
context "when badge exists" do
it "shows the badge" do
get "/badge/#{badge.slug}"
expect(response.body).to include CGI.escapeHTML(badge.title)
end
end

context "when badge does not exist" do
it "renders 404" do
expect { get "/badge/that-does-not-exists" }.to raise_error(ActionController::RoutingError)
end
end
end
end
ProTip! Use n and p to navigate between commits in a pull request.
You can’t perform that action at this time.