-
Notifications
You must be signed in to change notification settings - Fork 74
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
Refactor errors #213
base: main
Are you sure you want to change the base?
Refactor errors #213
Changes from 11 commits
b01a8f7
400b5c6
0d343ed
45fccbf
3bb5dcd
bd93fca
57779ef
4a83438
2222851
a333272
74ab13b
8518639
b9ddc68
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,117 +1,61 @@ | ||
require "i18n" | ||
|
||
module Pliny | ||
module Errors | ||
class Error < StandardError | ||
attr_accessor :id | ||
|
||
def self.render(error) | ||
headers = { "Content-Type" => "application/json; charset=utf-8" } | ||
data = { id: error.id, message: error.message } | ||
[error.status, headers, [MultiJson.encode(data)]] | ||
end | ||
class << self | ||
attr_accessor :error_class_id, :error_class_status | ||
|
||
def initialize(message, id) | ||
@id = id | ||
super(message) | ||
def render(error) | ||
headers = { "Content-Type" => "application/json; charset=utf-8" } | ||
data = { id: error.id, message: error.user_message }.merge(error.metadata) | ||
[error.status, headers, [MultiJson.encode(data)]] | ||
end | ||
end | ||
end | ||
|
||
class HTTPStatusError < Error | ||
attr_accessor :status | ||
attr_accessor :id, :status, :metadata, :user_message | ||
|
||
def initialize(message = nil, id = nil, status = nil) | ||
meta = Pliny::Errors::META[self.class] | ||
message = message || meta[1] + "." | ||
id = id || meta[1].downcase.tr(' ', '_').to_sym | ||
@status = status || meta[0] | ||
super(message, id) | ||
def initialize(id=nil, metadata: {}) | ||
@id = (id || self.class.error_class_id).to_sym | ||
@status = self.class.error_class_status | ||
@metadata = metadata | ||
@user_message = I18n.t("errors.#{@id}") | ||
super(@id.to_s) | ||
end | ||
end | ||
|
||
class Continue < HTTPStatusError; end # 100 | ||
class SwitchingProtocols < HTTPStatusError; end # 101 | ||
class OK < HTTPStatusError; end # 200 | ||
class Created < HTTPStatusError; end # 201 | ||
class Accepted < HTTPStatusError; end # 202 | ||
class NonAuthoritativeInformation < HTTPStatusError; end # 203 | ||
class NoContent < HTTPStatusError; end # 204 | ||
class ResetContent < HTTPStatusError; end # 205 | ||
class PartialContent < HTTPStatusError; end # 206 | ||
class MultipleChoices < HTTPStatusError; end # 300 | ||
class MovedPermanently < HTTPStatusError; end # 301 | ||
class Found < HTTPStatusError; end # 302 | ||
class SeeOther < HTTPStatusError; end # 303 | ||
class NotModified < HTTPStatusError; end # 304 | ||
class UseProxy < HTTPStatusError; end # 305 | ||
class TemporaryRedirect < HTTPStatusError; end # 307 | ||
class BadRequest < HTTPStatusError; end # 400 | ||
class Unauthorized < HTTPStatusError; end # 401 | ||
class PaymentRequired < HTTPStatusError; end # 402 | ||
class Forbidden < HTTPStatusError; end # 403 | ||
class NotFound < HTTPStatusError; end # 404 | ||
class MethodNotAllowed < HTTPStatusError; end # 405 | ||
class NotAcceptable < HTTPStatusError; end # 406 | ||
class ProxyAuthenticationRequired < HTTPStatusError; end # 407 | ||
class RequestTimeout < HTTPStatusError; end # 408 | ||
class Conflict < HTTPStatusError; end # 409 | ||
class Gone < HTTPStatusError; end # 410 | ||
class LengthRequired < HTTPStatusError; end # 411 | ||
class PreconditionFailed < HTTPStatusError; end # 412 | ||
class RequestEntityTooLarge < HTTPStatusError; end # 413 | ||
class RequestURITooLong < HTTPStatusError; end # 414 | ||
class UnsupportedMediaType < HTTPStatusError; end # 415 | ||
class RequestedRangeNotSatisfiable < HTTPStatusError; end # 416 | ||
class ExpectationFailed < HTTPStatusError; end # 417 | ||
class UnprocessableEntity < HTTPStatusError; end # 422 | ||
class TooManyRequests < HTTPStatusError; end # 429 | ||
class InternalServerError < HTTPStatusError; end # 500 | ||
class NotImplemented < HTTPStatusError; end # 501 | ||
class BadGateway < HTTPStatusError; end # 502 | ||
class ServiceUnavailable < HTTPStatusError; end # 503 | ||
class GatewayTimeout < HTTPStatusError; end # 504 | ||
def self.MakeError(status, id) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Shouldn't this method name be snakecased? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. oh god, yes. I think I saw this class-looking method used before for a similar purpose but looking now it's super confusing. Fixed! |
||
Class.new(Pliny::Errors::Error) do | ||
@error_class_id = id | ||
@error_class_status = status | ||
end | ||
end | ||
|
||
# Messages for nicer exceptions, from rfc2616 | ||
META = { | ||
Continue => [100, 'Continue'], | ||
SwitchingProtocols => [101, 'Switching protocols'], | ||
OK => [200, 'OK'], | ||
Created => [201, 'Created'], | ||
Accepted => [202, 'Accepted'], | ||
NonAuthoritativeInformation => [203, 'Non-authoritative information'], | ||
NoContent => [204, 'No content'], | ||
ResetContent => [205, 'Reset content'], | ||
PartialContent => [206, 'Partial content'], | ||
MultipleChoices => [300, 'Multiple choices'], | ||
MovedPermanently => [301, 'Moved permanently'], | ||
Found => [302, 'Found'], | ||
SeeOther => [303, 'See other'], | ||
NotModified => [304, 'Not modified'], | ||
UseProxy => [305, 'Use proxy'], | ||
TemporaryRedirect => [307, 'Temporary redirect'], | ||
BadRequest => [400, 'Bad request'], | ||
Unauthorized => [401, 'Unauthorized'], | ||
PaymentRequired => [402, 'Payment required'], | ||
Forbidden => [403, 'Forbidden'], | ||
NotFound => [404, 'Not found'], | ||
MethodNotAllowed => [405, 'Method not allowed'], | ||
NotAcceptable => [406, 'Not acceptable'], | ||
ProxyAuthenticationRequired => [407, 'Proxy authentication required'], | ||
RequestTimeout => [408, 'Request timeout'], | ||
Conflict => [409, 'Conflict'], | ||
Gone => [410, 'Gone'], | ||
LengthRequired => [411, 'Length required'], | ||
PreconditionFailed => [412, 'Precondition failed'], | ||
RequestEntityTooLarge => [413, 'Request entity too large'], | ||
RequestURITooLong => [414, 'Request-URI too long'], | ||
UnsupportedMediaType => [415, 'Unsupported media type'], | ||
RequestedRangeNotSatisfiable => [416, 'Requested range not satisfiable'], | ||
ExpectationFailed => [417, 'Expectation failed'], | ||
UnprocessableEntity => [422, 'Unprocessable entity'], | ||
TooManyRequests => [429, 'Too many requests'], | ||
InternalServerError => [500, 'Internal server error'], | ||
NotImplemented => [501, 'Not implemented'], | ||
BadGateway => [502, 'Bad gateway'], | ||
ServiceUnavailable => [503, 'Service unavailable'], | ||
GatewayTimeout => [504, 'Gateway timeout'], | ||
}.freeze | ||
BadRequest = MakeError(400, :bad_request) | ||
Unauthorized = MakeError(401, :unauthorized) | ||
PaymentRequired = MakeError(402, :payment_required) | ||
Forbidden = MakeError(403, :forbidden) | ||
NotFound = MakeError(404, :not_found) | ||
MethodNotAllowed = MakeError(405, :method_not_allowed) | ||
NotAcceptable = MakeError(406, :not_acceptable) | ||
ProxyAuthenticationRequired = MakeError(407, :proxy_authentication_required) | ||
RequestTimeout = MakeError(408, :request_timeout) | ||
Conflict = MakeError(409, :conflict) | ||
Gone = MakeError(410, :gone) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I know that removing all the non 4xx-5xx error classes is going to cause things to break in a few of my apps. To make matters a little worse, I suspect a few of them might be used for control flow. That said, I do think this is a good change and what I've been doing is BAD (tm). |
||
LengthRequired = MakeError(411, :length_required) | ||
PreconditionFailed = MakeError(412, :precondition_failed) | ||
RequestEntityTooLarge = MakeError(413, :request_entity_too_large) | ||
RequestURITooLong = MakeError(414, :request_uri_too_long) | ||
UnsupportedMediaType = MakeError(415, :unsupported_media_type) | ||
RequestedRangeNotSatisfiable = MakeError(416, :requested_range_not_satisfiable) | ||
ExpectationFailed = MakeError(417, :expectation_failed) | ||
UnprocessableEntity = MakeError(422, :unprocessable_entity) | ||
TooManyRequests = MakeError(429, :too_many_requests) | ||
InternalServerError = MakeError(500, :internal_server_error) | ||
NotImplemented = MakeError(501, :not_implemented) | ||
BadGateway = MakeError(502, :bad_gateway) | ||
ServiceUnavailable = MakeError(503, :service_unavailable) | ||
GatewayTimeout = MakeError(504, :gateway_timeout) | ||
end | ||
end |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,6 +1,7 @@ | ||
source "https://rubygems.org" | ||
ruby "2.2.3" | ||
|
||
gem "i18n", "~> 0.7" | ||
gem "multi_json" | ||
gem "oj" | ||
gem "pg" | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,27 @@ | ||
en: | ||
errors: | ||
bad_gateway: Bad gateway | ||
bad_request: Bad request | ||
conflict: Conflict | ||
expectation_failed: Expectation failed | ||
forbidden: Forbidden | ||
gateway_timeout: Gateway timed out | ||
gone: Gone | ||
internal_server_error: Internal server error | ||
length_required: Length required | ||
method_not_allowed: Method not allowed | ||
not_acceptable: Not acceptable | ||
not_found: Not found | ||
not_implemented: Not implemented | ||
payment_required: Payment required | ||
precondition_failed: Precondition failed | ||
proxy_authentication_required: Proxy authentication required | ||
request_entity_too_large: Request entity too large | ||
request_timeout: Request timed out | ||
request_uri_too_long: Requrest URI too long | ||
requested_range_not_satisfiable: Requested range not satisfiable | ||
service_unavailable: Service unavailable | ||
too_many_requests: Too many requests | ||
unauthorized: Unauthorized | ||
unprocessable_entity: Unprocessable entity | ||
unsupported_media_type: Unsupported media type |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,24 +1,32 @@ | ||
require "spec_helper" | ||
|
||
describe Pliny::Errors do | ||
it "includes a general error that requires an identifier" do | ||
e = Pliny::Errors::Error.new("General error.", :general_error) | ||
assert_equal "General error.", e.message | ||
assert_equal :general_error, e.id | ||
it "bundles classes to represent the different status codes" do | ||
error = Pliny::Errors::BadRequest.new | ||
assert_equal :bad_request, error.id | ||
assert_equal 400, error.status | ||
assert_equal "Bad request", error.user_message | ||
|
||
error = Pliny::Errors::InternalServerError.new | ||
assert_equal :internal_server_error, error.id | ||
assert_equal 500, error.status | ||
assert_equal "Internal server error", error.user_message | ||
end | ||
|
||
it "keeps the error id stored as the internal message" do | ||
error = Pliny::Errors::BadRequest.new | ||
assert_equal "bad_request", error.message | ||
end | ||
|
||
it "includes an HTTP error that will take generic parameters" do | ||
e = Pliny::Errors::HTTPStatusError.new( | ||
"Custom HTTP error.", :custom_http_error, 499) | ||
assert_equal "Custom HTTP error.", e.message | ||
assert_equal :custom_http_error, e.id | ||
assert_equal 499, e.status | ||
it "takes a custom id" do | ||
error = Pliny::Errors::BadRequest.new(:invalid_json) | ||
assert_equal :invalid_json, error.id | ||
assert_equal 400, error.status | ||
end | ||
|
||
it "includes pre-defined HTTP error templates" do | ||
e = Pliny::Errors::NotFound.new | ||
assert_equal "Not found.", e.message | ||
assert_equal :not_found, e.id | ||
assert_equal 404, e.status | ||
it "takes optional metadata" do | ||
metadata = { foo: "bar" } | ||
error = Pliny::Errors::BadRequest.new(:invalid_json, metadata: metadata) | ||
assert_equal metadata, error.metadata | ||
end | ||
end |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,4 @@ | ||
# configure i18n to use locales from the template app | ||
I18n.config.enforce_available_locales = true | ||
I18n.load_path += Dir[File.dirname(__FILE__) + "/../../lib/template/config/locales/*.{rb,yml}"] | ||
I18n.backend.load_translations |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a breaking API change which will cause me some pain in a few places. Mostly where we are raising errors with custom messages, for example:
Again, I agree that using an error
id
andi18n
to sort this out is a better solution long term.