add error codes by StarpTech · Pull Request #911 · fastify/fastify

StarpTech · 2018-04-27T11:49:21Z

This is a first draft of introducing error codes to fastify.

No performance degradation in success path
No performance degradation in error path

TODO

Move error message and code to error

Checklist

run npm run test and npm run benchmark
tests and/or benchmarks are included
documentation is changed or added
commit message and code follows Code of conduct

jsumners · 2018-04-27T11:57:32Z

@@ -0,0 +1,28 @@
+'use strict'
+
+const codes = {


These should be actual codes, not strings. Look at how core is doing this:

https://github.com/nodejs/node/blob/a1fc52884b98acab3d3f0b66c3b5ac9eb0728b8b/lib/internal/errors.js#L704

https://github.com/nodejs/node/blob/a1fc52884b98acab3d3f0b66c3b5ac9eb0728b8b/lib/internal/errors.js#L227

You mean native Errors objects? This is debatable. We already work with error objects and don't use inheritance.

No, I mean FST_ERR_FOO should be equal to 42.

From where do you get this information that an error code must be an integer? Node.js also using strings. Here sym is the code string and assigned as code property to the error object here

Regardless, the issue is that strings are fragile. Symbols are fine.

Yep, symbols are unique but can't be converting to strings (logging). ~~Due to the fact that we prefix all strings we can guarantee their uniqueness.~~

I think strings are fine, and what Node core is using. I’ll do a more detailed review asap.

Mistake. Symbols are unique by default (at comparison) with a prefix we only guarantee collision-freedom (at definition). Do you think it will outperform the logging capabilities rather to write tests for that case?

.

allevo

Nice!

annotateError is used in twice way:

reply.send(annotateError(err, errorCodes.FST_ERR_ON_REQUEST_HOOK))
var err = new Error('Unsupported Media Type: ' + contentType); err = annotateError(err, errorCodes.FST_ERR_HTTP)

Can you rewrite using only one way?

StarpTech · 2018-04-29T19:08:11Z

Hi @allevo I'm using reply.send(annotateError(err, errorCodes.FST_ERR_ON_REQUEST_HOOK)) when there is no error to assign it's simpler and I can avoid a reassign like err = err.

mcollina · 2018-04-29T22:33:55Z

I am not a fan of the approach of "annotating" extraneous errors with our codes. IMHO we should only be using codes for errors that we create. The code should be included in the error message, similarly to what Node.js does.

I'm actually ok in the new ContentLengthDiffersError() approach, as it would make our code more readable.

allevo · 2018-04-29T22:41:25Z

Another way could be createContentLengthDiffersError()

delvedor · 2018-04-30T10:12:00Z

Completely agree with @mcollina.

Something like the following could be nice.
https://github.com/fastify/fastify-circuit-breaker/blob/0e9f6cfc3aa9c992ff9141430a38b362fea2c801/index.js#L119-L127

StarpTech · 2018-04-30T10:43:35Z

I will try to meet all requirements:

Create own error objects
Code should be part of the error message
Don't modify extraneously errors

StarpTech · 2018-04-30T18:44:32Z

Please check current state it's not final just an approach.

dicearr · 2018-05-02T07:45:10Z

    var result = parser.fn(req, body, done)
    if (result && typeof result.then === 'function') {
-      result.then(body => done(null, body)).catch(err => done(err))
+      result.then(body => done(null, body), done)


Errors thrown by the done function wont be caught anymore. Is this ok?

When using promises you can catch errors in two ways:

promise.then(fn).catch(errorHandler) //or promise.then(fn, errorHandler)

So this is fine :)
https://developer.mozilla.org/it/docs/Web/JavaScript/Reference/Global_Objects/Promise/then

I could be misunderstanding something but the first example will also catch the errors raised by fn whereas the second one wont. Right? It might be that done it is not going to raise any error, and in that case it would be fine.

Promise.resolve(1) .then(() => { throw new Error('done() error') }) .catch(() => console.log('Caught!')) // Called Promise.resolve(1) .then( () => { throw new Error('done() error') }, () => console.log('Caught!') // Never called )

@dicearr is right in the theoretical side. However in this case this is not needed. done() is set up to be called in a callback-based scenario. In any case, we can just add in a little try-catch.
Avoiding the .catch() saves us the allocation of one promise.

Sry, for the late response. What @mcollina said is correct and was my intention.

StarpTech · 2018-05-05T20:25:41Z

@fastify/fastify please review the way of error handling when it's fine I can continue.

mcollina

Good job! I think this is a semver-major change.

mcollina · 2018-05-06T15:35:20Z

  if (parser === undefined) {
-    reply.code(415).send(new Error('Unsupported Media Type: ' + contentType))
+    var err = new FST_ERR_CTP_INVALID_MEDIA_TYPE(`Unsupported Media Type: ${contentType}`)
+    reply.code(415).send(err)


Should we include the code in the Error instead?

err.core = 415

You mean err.statusCode = 415 ?

mcollina · 2018-05-06T15:36:28Z

+  const errClass = class FastifyError extends Base {
+    constructor (...args) {
+      super(`Code: ${code}; ${createMessage(args)}`)
+    }


is this adding another level in the stacktrace? can you please verify and and add a test for that?

No, the constructor call is omitted. Do you know how to test it reliable?

@mcollina ping.

mcollina · 2018-05-06T15:38:43Z

    t.fail('should throw')
  } catch (err) {
-    t.is(err.message, 'The body parser can only parse your data as \'string\' or \'buffer\', you asked \'fireworks\' which is not supported.')
+    t.is(err.message, `Code: FST_ERR_CTP_INVALID_PARSE_TYPE; The body parser can only parse your data as 'string' or 'buffer', you asked 'fireworks' which is not supported.`)


Can you please follow the node core standard?

Could you make an example?

mcollina · 2018-05-06T15:39:09Z

    t.fail()
  } catch (err) {
-    t.is(err.message, 'The content type should be a string')
+    t.is(err.message, 'Code: FST_ERR_CTP_INVALID_TYPE; The content type should be a string')


this should also test that the code is correct

allevo · 2018-05-06T19:08:05Z

  if (contentLength > limit) {
-    reply.code(413).send(new Error('Request body is too large'))
+    var err = new FST_ERR_CTP_BODY_TOO_LARGE('Request body is too large')
+    reply.code(413).send(err)


allevo · 2018-05-06T19:08:17Z

      req.removeListener('error', onEnd)
-      reply.code(413).send(new Error('Request body is too large'))
+      var err = new FST_ERR_CTP_BODY_TOO_LARGE('Request body is too large')
+      reply.code(413).send(err)


allevo · 2018-05-06T19:08:24Z

    if (!Number.isNaN(contentLength) && receivedLength !== contentLength) {
-      reply.code(400).send(new Error('Request body size did not match Content-Length'))
+      var contentLenghtErr = new FST_ERR_CTP_INVALID_CONTENT_LENGTH('Request body size did not match Content-Length')
+      reply.code(400).send(contentLenghtErr)


mcollina · 2018-05-07T09:29:33Z

  var parser = this.cache.get(contentType) || this.getParser(contentType)
  if (parser === undefined) {
-    reply.code(415).send(new Error('Unsupported Media Type: ' + contentType))
+    var err = new FST_ERR_CTP_INVALID_MEDIA_TYPE(`Unsupported Media Type: ${contentType}`)


I think we should move the message to the errors file as well.

yes, would produce more readable code.

StarpTech · 2018-05-13T18:37:05Z

@mcollina @allevo Error message and statusCode was moved to errors file. In this way, we create specific fastify errors and pass only data to format the error message WDYT?

allevo

LGTM.

I don't like const errClass = class FastifyError extends Base {, but nice work!

StarpTech · 2018-05-15T20:58:18Z

What you don't like exactly?

allevo · 2018-05-15T21:01:35Z

What you don't like exactly?

Creating different kind of classes at the start up it's strange, but cool: it's Javascript!

StarpTech · 2018-05-15T21:11:51Z

This approach is from node core but yes, it's a little getting used to.

mcollina · 2018-05-16T16:20:17Z

+  if (!msg) throw new Error('Fastify error message must not be empty')
+
+  code = code.toUpperCase()
+  const errClass = class FastifyError extends Base {


can you avoid the const errClass = class FastifyError and just do codes[code] = FastifyError?

What do you mean? Should I avoid classes?

mcollina · 2018-05-16T16:21:00Z

Can you target this against a next branch, which will be v2?

StarpTech · 2018-05-17T20:04:26Z

There is no next https://github.com/fastify/fastify/branches

mcollina · 2018-05-19T14:52:48Z

Create one forking from master.

github-actions · 2022-02-17T00:13:32Z

This pull request has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

add error codes

2f0b1c7

jsumners previously requested changes Apr 27, 2018

View reviewed changes

[ci skip] add comment

465e13f

allevo reviewed Apr 29, 2018

View reviewed changes

delvedor added feature discussion Issues or PRs with this label will never stale labels Apr 30, 2018

StarpTech added 3 commits April 30, 2018 19:01

Merge branch 'master' into feature/#909_error_codes

a66bc91

first proposal of fastify errors

016228a

simplify

ba02fad

StarpTech added 5 commits April 30, 2018 20:50

[ci skip] add example doc

ee34bad

[ci skip] update doc

0505b4e

include code in err msg

e82fa58

split in type and range errors, use clean names

62c1721

avoid promise creation

791de5f

dicearr reviewed May 2, 2018

View reviewed changes

mcollina reviewed May 6, 2018

View reviewed changes

verify code on error

6be98ec

allevo reviewed May 6, 2018

View reviewed changes

mcollina reviewed May 7, 2018

View reviewed changes

StarpTech added 2 commits May 13, 2018 20:27

assign statusCode and error message at creation time

deee08f

Merge branch 'master' into feature/#909_error_codes

38bb543

StarpTech added 2 commits May 13, 2018 20:39

statusCode is optional

ff99b21

validate required err params

5110c95

allevo approved these changes May 15, 2018

View reviewed changes

mcollina reviewed May 16, 2018

View reviewed changes

StarpTech added 2 commits May 17, 2018 22:05

avoid var

0e5fcc5

Merge branch 'master' into feature/#909_error_codes

89e3927

StarpTech closed this Jun 2, 2018

StarpTech mentioned this pull request Jun 2, 2018

Feature/#909 error codes #957

Closed

5 tasks

github-actions Bot locked as resolved and limited conversation to collaborators Feb 17, 2022

Eomm added the semver-minor Issue or PR that should land as semver minor label Apr 1, 2023

Uh oh!

Conversation

StarpTech commented Apr 27, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

TODO

Checklist

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

StarpTech Apr 27, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

StarpTech Apr 27, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

StarpTech Apr 27, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

allevo left a comment

Choose a reason for hiding this comment

Uh oh!

StarpTech commented Apr 29, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mcollina commented Apr 29, 2018

Uh oh!

allevo commented Apr 29, 2018

Uh oh!

delvedor commented Apr 30, 2018

Uh oh!

StarpTech commented Apr 30, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

StarpTech commented Apr 30, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

delvedor May 2, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

StarpTech commented May 5, 2018

Uh oh!

mcollina left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

StarpTech May 6, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

StarpTech commented Apr 27, 2018 •

edited

Loading

StarpTech Apr 27, 2018 •

edited

Loading

StarpTech Apr 27, 2018 •

edited

Loading

StarpTech Apr 27, 2018 •

edited

Loading

StarpTech commented Apr 29, 2018 •

edited

Loading

StarpTech commented Apr 30, 2018 •

edited

Loading

delvedor May 2, 2018 •

edited

Loading

StarpTech May 6, 2018 •

edited

Loading

StarpTech commented May 13, 2018 •

edited

Loading

StarpTech commented May 15, 2018 •

edited

Loading

StarpTech May 17, 2018 •

edited

Loading