Feature/better error management by StarpTech · Pull Request #1168 · fastify/fastify

StarpTech · 2018-09-18T21:26:48Z

This is a first draft of introducing error codes to fastify. It only handles the codes for the contentTypeParser.

TODO

Create error codes for all others
Document the error codes

Previous PR: #957

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

…odes

… into feature/better_error_management

mcollina · 2018-09-19T06:27:11Z

      t.strictDeepEqual(JSON.parse(body.toString()), {
-        statusCode: 413,
+        statusCode: expectedError.statusCode,
+        code: expectedError.code,


I think we should keep those explicit in the tests

mcollina · 2018-09-19T06:27:36Z

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


I would prefer for the message to be explicit here.

What's the benefit? I think the error name is already very explicit. In that way we can avoid modifying all tests when we change something in the future.

Sure, but I would love to avoid testing all errors individually.

stale · 2018-10-04T07:48:21Z

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

mcollina · 2018-10-04T08:16:22Z

@StarpTech any updates here?

StarpTech · 2018-10-04T08:32:02Z

I will work on it in smaller updates I'm too busy right now. Any help is welcome.

cemremengu · 2018-10-04T08:36:40Z

@StarpTech happy to help, what's left to do can you provide some kind of check list?

StarpTech · 2018-10-04T08:41:34Z

@cemremengu sure

Have a look at how we want to use errors in the future.
Address @mcollina point
Change it everywhere & update tests
Document the new errors

cemremengu · 2018-10-05T17:14:52Z

@StarpTech Reverted test to use string messages. It would look like this:

      t.strictDeepEqual(JSON.parse(body.toString()), {
        statusCode: 413,
        code: "FST_ERR_CTP_BODY_TOO_LARGE",
        error: 'Payload Too Large',
        message: 'FST_ERR_CTP_BODY_TOO_LARGE: Request body is too large'
      })

Should I push to your branch with your permission? How should we progress?

EDIT:
do these error messages apply to assertions as well or we leave them as is?

mcollina · 2018-10-05T17:23:32Z

I think you can push here, just not force push.

cemremengu · 2018-10-05T22:40:41Z

EDIT: Found it, changing this

    // if we have failed because `resolve has thrown
    // let's rethrow the error and let avvio handle it
    if (err.code === 404) throw err
    return swapSchema

to this

    // if we have failed because `resolve has thrown
    // let's rethrow the error and let avvio handle it
    if (err.statusCode === 404) throw err
    return swapSchema

will solve the problem. Maybe we can match for the real code /FST_ERR_SCHEMA/ (I prefer this) What do you think?

--

I think there is problem with schema.js. The test below fails:

test('Should throw of the schema does not exists', t => {
  t.plan(1)
  const fastify = Fastify()

  fastify.route({
    method: 'GET',
    url: '/:id',
    schema: {
      params: 'test#'
    },
    handler: (req, reply) => {
      reply.send(typeof req.params.id)
    }
  })

  fastify.ready(err => {
    t.is(err.message, 'Schema with id \'test\' does not exist!')
  })
})

If I change/delete the error.code below (since we now have string codes like FST....) in shared-schemas.test.js, ajv will complain that schema must be a an object or boolean

Schemas.prototype.resolve = function (id) {
  if (this.store[id] === undefined) {
    const error = new Error(`Schema with id '${id}' does not exist!`)
    error.code = 404
    throw error
  }
  return this.store[id]
}

Any ideas?

mcollina

Good work so far, from a testing perspective, we need to verify both the code and the message. Sorry if I was not clear before.

mcollina · 2018-10-06T09:17:53Z


-function noop () {}
+function noop () { }



why this change?

mcollina · 2018-10-06T09:18:24Z

    server.add = decorator.add
    return server
-    function server () {}
+    function server () { }


why this change?

mcollina · 2018-10-06T09:18:40Z

-  function Instance () {}
-  Instance.prototype.test = () => {}
+  function Instance () { }
+  Instance.prototype.test = () => { }


more unneeded changes.

standard --fix or vscode formatter is changing these I think. I will revert them

I think vscode formatter, standard --fix is run every time.

mcollina · 2018-10-06T09:20:01Z

    t.fail()
  } catch (e) {
-    t.ok(/missing dependency/.test(e.message))
+    t.ok(/FST_ERR_DEC_MISSING_DEPENDENCY/.test(e.message))


I think we colluld avoid the regexp here

…use common language in error codes

mcollina · 2018-10-08T05:16:00Z

Very good so far. What is left to do? Docs for certain.

cemremengu · 2018-10-08T07:23:49Z

Yes, looks like only docs are outstanding. Will send them in today.

cemremengu · 2018-10-08T10:28:59Z

@StarpTech @mcollina I think this should be good to go after some final polishing (if necessary).

Let me know if anything else needs to be done.

mcollina

LGTM with a couple of nits.

mcollina · 2018-10-08T16:44:28Z

This PR needs to be merged by hand to keep the dual authorship of the work.

cemremengu · 2018-10-08T16:49:16Z

@mcollina I personally don't mind and wouldn't want anyone to waste time on that. All credits goes to @StarpTech, he did most of the work anyways.

StarpTech · 2018-10-08T17:21:58Z

Good work @cemremengu! I also don't mind. Take the easiest way.

StarpTech · 2018-10-08T19:59:09Z

+<a id="FST_ERR_CTP_INVALID_PARSE_TYPE"></a>
+### FST_ERR_CTP_INVALID_PARSE_TYPE
+
+The provided parse type is not supported. Accepted values are 'string' or 'buffer'.


Could you mark the types in "`" as well?

StarpTech · 2018-10-08T20:00:02Z

+<a id="FST_ERR_SCH_ALREADY_PRESENT"></a>
+### FST_ERR_SCH_ALREADY_PRESENT
+
+A schema with the same `id` already exists.


Be consistent use id or $id.

StarpTech · 2018-10-08T20:42:30Z

 ### FST_ERR_REP_INVALID_PAYLOAD_TYPE

-Reply payload can either be a `string` or a `Buffer`.
+Reply payload can either be a `'string'` or a `'Buffer'`.


You mean without quotes right? 😄

Hang on almost there 😄

StarpTech

LGTM

delvedor

LGTM

StarpTech · 2018-10-12T19:37:08Z

Ready to merge?

mcollina · 2018-10-12T20:35:02Z

go for it! Il giorno ven 12 ott 2018 alle 12:37 Dustin Deus <notifications@github.com> ha scritto:

…

Ready to merge? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#1168 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AADL40Z5m-nJ_VfdeI04qtl2_OzmwGjHks5ukO9lgaJpZM4WuxzC> .

StarpTech · 2018-10-12T21:21:29Z

Thank you @cemremengu for takeover.

github-actions · 2022-02-13T00:16:29Z

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.

StarpTech added 5 commits September 18, 2018 23:14

introduce error codes, replace content type error with custom error c…

6ec836a

…odes

introduce error codes, replace content type error with custom error c…

4de5c1a

…odes

Merge remote-tracking branch 'origin/feature/better_error_management'…

cca22b0

… into feature/better_error_management

add new doc page for errors

7025c08

add code prop to error schema

921f937

mcollina reviewed Sep 19, 2018

View reviewed changes

delvedor added the semver-major Issue or PR that should land as semver major label Sep 19, 2018

delvedor added this to the v2.0.0 milestone Sep 19, 2018

stale Bot added the stale Issue or pr with more than 15 days of inactivity. label Oct 4, 2018

cemremengu removed the stale Issue or pr with more than 15 days of inactivity. label Oct 4, 2018

cemremengu added 5 commits October 5, 2018 20:24

Reverted tests to use string messages

07cfba5

Added decorator error codes

e564a04

Added hook error codes

e145fc2

Added logger error codes

4dee00c

Added response error codes

70ead5f

Added schema error codes

ca962a9

mcollina reviewed Oct 6, 2018

View reviewed changes

Revert formatting, add tests for error code, add schema error codes, …

22c4a33

…use common language in error codes

cemremengu added 2 commits October 8, 2018 13:21

Added docs

e5b6d9f

Updated docs-1

55e73b6

mcollina approved these changes Oct 8, 2018

View reviewed changes

Comment thread docs/Errros.md Outdated

Comment thread docs/Errros.md Outdated

Updated docs-2

989d9d7

mcollina requested review from delvedor and jsumners October 8, 2018 16:43

StarpTech commented Oct 8, 2018

View reviewed changes

Updated docs-3

9481669

StarpTech commented Oct 8, 2018

View reviewed changes

Updated docs-4

e72079b

StarpTech commented Oct 8, 2018

View reviewed changes

This was referenced Oct 8, 2018

Added unit test for multiple logger destinations #1205

Merged

Use errors codes for fastify errors #909

Closed

delvedor approved these changes Oct 12, 2018

View reviewed changes

StarpTech merged commit 2148dbc into fastify:next Oct 12, 2018

cemremengu mentioned this pull request Oct 16, 2018

custom error handler can be able to handle contentTypeParser's errors #1212

Merged

4 tasks

fralonra mentioned this pull request Nov 27, 2018

文档持续更新List fastify/docs-chinese#18

Closed

21 tasks

mcollina mentioned this pull request Dec 20, 2018

Missing two error codes #1334

Closed

2 tasks

fralonra mentioned this pull request Sep 12, 2019

docs(Reply): fix Errors #1848

Merged

4 tasks

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

Uh oh!

Conversation

StarpTech commented Sep 18, 2018 • edited by cemremengu 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!

Choose a reason for hiding this comment

Uh oh!

stale Bot commented Oct 4, 2018

Uh oh!

mcollina commented Oct 4, 2018 • edited by cemremengu Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

StarpTech commented Oct 4, 2018

Uh oh!

cemremengu commented Oct 4, 2018

Uh oh!

StarpTech commented Oct 4, 2018

Uh oh!

cemremengu commented Oct 5, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mcollina commented Oct 5, 2018

Uh oh!

cemremengu commented Oct 5, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

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!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mcollina commented Oct 8, 2018

Uh oh!

cemremengu commented Oct 8, 2018

Uh oh!

cemremengu commented Oct 8, 2018

Uh oh!

mcollina left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

mcollina commented Oct 8, 2018

Uh oh!

cemremengu commented Oct 8, 2018

Uh oh!

StarpTech commented Oct 8, 2018

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 left a comment

Choose a reason for hiding this comment

Uh oh!

delvedor left a comment

Choose a reason for hiding this comment

StarpTech commented Sep 18, 2018 •

edited by cemremengu

Loading

mcollina commented Oct 4, 2018 •

edited by cemremengu

Loading

cemremengu commented Oct 5, 2018 •

edited

Loading

cemremengu commented Oct 5, 2018 •

edited

Loading