Refactor errors

[pedro]

• Constructors take options hash
• Support for metadata (new error attributes that are serialized)
• Automatically render 404s for Sequel::NoMatchingRow errors

[brandur]

@pedro The original idea here was that HTTPStatusError would behave more like an abstract class which would never be instantiated directly; which is also why it didn't get an entry in this table. What do you think of changing this to just raising an ArgumentError if the instantiated error class wasn't found in META?

[pedro]

ah, makes sense! changing it.

[brandur]

@pedro Nice job. Left a few minor comments, but +1 from me!

[gudmundur]

@pedro I'd love to see this merged. Is there anything that needs to be fixed or can it be merged?

[pedro]

@brandur @gudmundur hey folks, good time to follow up on this – I just had to refactor how we handle errors at Lugg and ended up with a mix of Pliny and new stuff we're super happy about.

It's not a huge departure from this. This basically highlights the issue:

Having exceptions take a message as their first argument is a fairly widespread convention.

Ruby exceptions are meant to have a message, but we all know API errors really just need an id. They are easier to compare in tests, and they won't pollute code with copy that we all know should live under config/locales.yml.

But no one said that the Ruby exception message is what we render to end-users. "invalid_json" works perfectly fine as the internal exception message, and it also happens to be a good error identifier for Pliny APIs, not to mention a good key to lookup the corresponding entry in the locales file.

So here's the proposal: Pliny ships with i18n. All existing user error messages are set there (like "Unauthorized" and "Unprocessable entity"). Our base error class only requires an id, which is used to fetch these messages and save them as the error.user_message. The id is also stored internally as the exception message so you can raise them using the same interface, like raise Pliny::Errors::BadRequest("invalid_json").

You can then test for these exceptions using standard matchers, like:

expect { run }.to raise_error(Pliny::Errors::BadRequest, "invalid_json")

Finally, while in there I'd also like to remove all exception classes outside 4xx and 5xx. Because if you want to render a 30x, for instance, you should rely on Sinatra's helpers and not on a Pliny exception.

Thoughts?

[mattreduce]

@pedro 💯 👍

[brandur]

Thoughts?

I like it (both taking the messages out with default localization and removing non-4/5xx series exceptions). +1.

[dmathieu]

I really like this idea as well.

[pedro]

you folks rock, thanks for the feedback! updated to match then 💃

[dmathieu]

This is looking nice. The only comment I have is from a non-native english perspective. If I were to wish to translate error messages, I wouldn't be able to do the following:

error = Pliny::Errors::BadRequest.new
I18n.locale = :fr
error.user_message #=> will show the error in english

It's probably an edge case though.

[brandur]

Shouldn't this method name be snakecased?

[pedro]

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!

[brandur]

@dmathieu Should be easy enough to just move the translation over to be execute on access of user_message, but I guess you do introduce a slight risk of making rendering an error multiple times more expensive. IMO, either deficiency in likely to cause too much of a problem in practice, so will leave it up to Pedro :)

@pedro Left a minor comment above, but looks good to me! +1.

[dmathieu]

My thinking was more to actually lazily fetch this data (and maybe memoize it to avoid expensive i18n lookup calls).

def user_message
  @user_message ||= I18n.t("errors.#{id}")
end

[brandur]

My thinking was more to actually lazily fetch this data (and maybe memoize it to avoid expensive i18n lookup calls).

Ah! I guess the only thing there is that with that implementation you could produce another failure scenario that's not too different from the one you have above:

error = Pliny::Errors::BadRequest.new
error.user_message #=> memoized
I18n.locale = :fr
error.user_message #=> will show the error in english

Although admittedly, also probably unlikely in practice.

[pedro]

good point. moved to a method not memoized for now, don't think we should worry too much about optimizing exceptional paths.

[dmathieu]

👍

[jellybob]

If that's actually an issue in practice could you memoize keyed against the current locale, something like #{I18n.locale}_#{key} which would then be invalidated by changing the locale?

[gudmundur]

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).

[pedro]

@gudmundur nod on the breaking changes – this would obviously require a major version bump normally, but seeing that we're still on the 0.x series I'm not sure there's much else we can do. Thoughts?

[brandur]

@pedro It's kind of a departure from the proposal above, should we allow an override message to be given for backward compatibility (and make that route deprecated, or at least unusual)? I guess the disadvantage is developers not knowing that they usually shouldn't provide it, but that's not a huge downside.

Definitely like the original proposal as is, but given that Pliny's seeing a reasonable amount of use now, erring on the side of conservative interface changes is probably a good idea.