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

Standard error interface #35

Closed
jonathanong opened this issue Aug 13, 2014 · 15 comments
Closed

Standard error interface #35

jonathanong opened this issue Aug 13, 2014 · 15 comments

Comments

@jonathanong
Copy link
Member

Would be cool if these modules and any related ones have a common error syntax. Would be cool to have err.code matching node or something as well as .status if applicable

@Raynos
Copy link
Member

Raynos commented Aug 13, 2014

I've recently written an error serializer for our json APIs ( https://gist.github.com/Raynos/91cab673906aebd04eac )

I settled on the interface

type TypedError : {
  message: String,
  statusCode?: Number,
  type?: String,
  stack?: String,
  expected?: Boolean,
  messages?: Array<String>
}

The message field is the standard error message.

The statusCode field is a number that represents the http status code thats applicable for this error, this is optional and defaults to 500.

The type field is a static string, it's like err.code but a string is more readable then a static number. This field allows for the message field to be dynamic and means you can detect what kind of error something is without doing a regular expression match.

The stack field is the standard error stack

The expected boolean allows you to encode whether this an expected or unexpected error. A body parser might run into an unexpected net IO error which means it should be treated as a unexpected 500 error or might run into an expected JSON parse error in which case it should be treated as a 400.

In production you want to only serialize the message field for expected 4xx errors and serialize the message field as Unexpected Server Error for unexpected 500 fields. Serializing arbitrary unexpected error message leaks implementation details and creates vulnerabilities

The messages field is slightly opinionated. When dealing with body validation errors you might want to send multiple validation messages. This could be done with a comma seperated message field or with an array of messages.

I don't care that much about field names, however you generally want the following information

  • a dynamic human / programmer readable error message
  • a static machine readable type or code field
  • a expected flag to determine whether we want to show the message to the end user
  • a status code field to determine the correct status code of the expected error.

@jonathanong
Copy link
Member Author

so i really like stripe's error: https://stripe.com/docs/api#errors

  • message - i think it should always be the error itself. i don't think these shoudl generally be exposed to the client, ever. devs should get the err.code and write their own error messages.
  • type - the "type" of error, specifically answering "whose fault is this?". ex. i really like stripe's err.type
  • code - the goal of this is for users to just type in the code in google and a description should pop up
  • status - i prefer status just because that's what express and koa uses. we could just alias statusCode as well - no big deal
  • expected - not sure if the name itself makes sense. koa uses .expose, which i think is a better name. i agree with the 4xx vs 5xx stuff.
  • messages - meh. i think that's more framework-specific.

@dougwilson
Copy link
Contributor

expected - not sure if the name itself makes sense. koa uses .expose , which i think is a better name. i agree with the 4xx vs 5xx stuff.

personally I like .public :)

@rlidwka
Copy link
Contributor

rlidwka commented Aug 14, 2014

How about plain old Error object with status property on it?

Something like finalhandler can capture those... with syntax like:

var err = new Error('blah not found')
err.status = 404
res.end(err)

@dougwilson
Copy link
Contributor

Also, as for deciding if the message should be written to the response or not, there is an upcoming message option on a future finalhandler that can determine stuff like that from the user: https://github.com/expressjs/finalhandler/tree/1.x#optionsmessage ; the default is to always hide the real message right now. we can always determine a good way :)

@rlidwka
Copy link
Contributor

rlidwka commented Sep 2, 2014

Imho, it should be an extension of an Error object. People are so used to dealing with it, so changing interface is a bad idea. I'm even thinking about shadowing the real Error object to get syntax highlighting and stuff.

So here is what I came up with so far:

var Error = require('./error')
throw Error('message').http(404)

/*
Error 404: message
    at new Error (/tmp/error.js:7:16)
    at Error (/tmp/error.js:5:40)
    at Object.<anonymous> (/tmp/index.js:6:7)
    at Module._compile (module.js:456:26)
*/

// with a possibility of future extensions like:
// throw Error('message').fs('ENOENT')

Implementation:

module.exports = Error

function Error(message, a, b) {
  var err = new global.Error(message, a, b)
  err.__proto__ = Error.prototype
  return err
}

require('util').inherits(global.Error, Error)

Error.prototype.http = function(status) {
  this.status = status
  return this
}

Error.prototype.toString = function() {
  var ret = 'Error'
  if (Number(this.status) >= 200 && Number(this.status) < 600) {
    ret += ' ' + this.status
  }
  ret += ': ' + this.message
  return ret
}

@dougwilson
Copy link
Contributor

If the resulting object is err, then Object.prototype.toString.call(err) === '[object Error]'. As long as the errors objects meet that requirement, I don't particularly care.

@jonathanong jonathanong mentioned this issue Sep 2, 2014
@rlidwka
Copy link
Contributor

rlidwka commented Sep 2, 2014

@dougwilson , I need a quick sanity check here: https://gist.github.com/rlidwka/0ad094386f60f93318bf

I was looking for a way to subclass Errors properly, but it looks like I just found a better way to subclass everything.

@rlidwka
Copy link
Contributor

rlidwka commented Sep 2, 2014

And here's proposed module: https://github.com/rlidwka/error, let me know if you like the approach.

No idea if it'll be useful, but I definitely going to switch to it from my custom new HTTPError({message: 'blah', code: 404}) crap.

@Raynos
Copy link
Member

Raynos commented Sep 2, 2014

I have a similar helper for creating errors ( https://github.com/Raynos/error/blob/master/typed.js ). This is actually the error module on npm.

@dougwilson
Copy link
Contributor

Both of those methods work :) I wonder which is faster, though. I would think __proto__ is going to be slower, but idk.

@rlidwka
Copy link
Contributor

rlidwka commented Sep 2, 2014

I wonder which is faster, though. I would think __proto__ is going to be slower, but idk.

It's full-blown example, and you don't really need to use __proto__. Basically, I was suggesting to create classes like this:

function Class() {
  var self = Object.create(Class.prototype)
  self.foo = 1
  return self
}

Benchmarks are surprising though. It's 30% faster on node 0.10 and 11% slower on node 0.11. :D

@jonathanong
Copy link
Member Author

should i push for http-error or http-errors?

@jonathanong
Copy link
Member Author

god i can't find an appropriate name

@jonathanong
Copy link
Member Author

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

No branches or pull requests

5 participants