Skip to content

Gracefully handle unknown formats#1671

Merged
zachmargolis merged 1 commit intomasterfrom
margolis-handle-unknown-format
Sep 14, 2017
Merged

Gracefully handle unknown formats#1671
zachmargolis merged 1 commit intomasterfrom
margolis-handle-unknown-format

Conversation

@zachmargolis
Copy link
Contributor

Why: Bad input should not cause us to throw exceptions

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Part of me wonders if we should render a 400 here instead? Maybe not b/c we don't have a template and all that setup for a 400?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have no strong opinions, we could even render the "not found" page with a 400 status code if you think that would be better?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe? Now that I'm looking at the bug this fixes, I'm thinking a 404 makes a bit more sense.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok we'll stick with 404 for now. Thanks!

@zachmargolis zachmargolis force-pushed the margolis-handle-unknown-format branch from 65c1213 to be2ba20 Compare September 14, 2017 22:24
**Why**: Bad input should not cause us to throw exceptions
@zachmargolis zachmargolis merged commit 153f6a9 into master Sep 14, 2017
@zachmargolis zachmargolis deleted the margolis-handle-unknown-format branch September 14, 2017 22:35
@monfresh
Copy link
Contributor

In Rails 5, ActionController::UnknownFormat will return a 406 HTTP status code, which is the correct code for this scenario. By rescuing the error and returning a 404 instead, we are not accurately capturing errors.

If the goal of this PR was to prevent on-call people from being awakened in the middle of the night, I think the solution is to exclude 406 errors from New Relic.

Also, note that the New Relic errors linked to in the issue this PR was meant to fix are all ActionView::MissingTemplate errors, and the reason those errors are throwing a 500 is because production is still using Rails 4.2.8. In Rails 5, we won't see ActionView::MissingTemplate anymore, except in some cases where we call render in the controller, which is a separate issue I'm fixing.

@zachmargolis
Copy link
Contributor Author

I think Rails 5 has already been in production: https://github.com/18F/identity-idp/releases/tag/2017-08-30T210357

@monfresh
Copy link
Contributor

Perhaps the wrong tag was deployed? The stack trace doesn't lie. New Relic clearly shows Rails 4.2.8.

@brodygov
Copy link
Contributor

Good catch finding the wrong tag thing!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants