✨ Error creation #7477

kirrg001 · 2016-10-03T17:36:26Z

overview changes

remove errors folder
option pattern for errors
main Error GhostError - inheritance
always use Error postfix naming
express error-handler takes care that each error get's wrapped into a GhostError if not happened yet
add level (critical, normal)
- normal is default -> this is very helpful
- for example 404 is not critical, but database version error or scheduling error is
- so it's very easy to filter them later.

GhostError and inheritance

Before this PR, each error had no prototype to inherit from.

That has some disadvantages:

you always need to define all properties an error has
can happen that some errors are inconsistent to other errors
no default set of attribute values
we had several un-needed checks
context/help was not a default property
we were not able to capture the error stack from the original error (now possible by just passing the original error) (https://github.com/TryGhost/Ghost/pull/7477/files#diff-25f0b2b0cc338d73a55da195594de403R278)

By default the GhostError prototype is an InternalServerError with statusCode 500. We can change that behaviour if we want.

Option pattern for errors

We always passed error arguments in a row (new errors.IncorrectUsageError(a,b,c)).
This has a big disadvantage: if you want to add or change the arguments order, you need to change everything. With option pattern you can pass how much arguments you want and it's even more readable for the caller/developer (new errors.IncorrectUsageError(true, 'hello') vs new errors.IncorrectUsageError({hideStack: true, message: 'hello'})).

Error handling

This is not particular part of this PR, but we should take care that units return a proper Ghost error instance. So for example if you create a unit A, which uses a third party module, which returns errors as pure strings (yes that happens sometimes), then it's your responsibility to react on these error strings and return a Ghost error instance instead. So checks like https://github.com/TryGhost/Ghost/pull/7477/files#diff-517787c62b3fa7bfcac648c0763b07ecL41 won't happen in the future. We had 100 of these use cases in our old error handler file, which were 1. hard to understand and 2. un-needed, when we do proper error creation.

@ErisDS
https://github.com/TryGhost/Ghost/pull/7477/files#diff-7a5f479230bbe115458d26c3c7026164L94
I deleted the debug feature, do you want to keep it and if yes, can you explain why?:)

d6b9126
This is a separate commit. I can extract this change if you want 🌮

@todo

- 🛠 add bunyan and prettyjson, remove morgan - ✨ add logging module - GhostLogger class that handles setup of bunyan - PrettyStream for stdout - ✨ config for logging - @todo: testing level fatal? - ✨ log each request via GhostLogger (express middleware) - @todo: add errors to output - 🔥 remove errors.updateActiveTheme - we can read the value from config - 🔥 remove 15 helper functions in core/server/errors/index.js - all these functions get replaced by modules: 1. logging 2. error middleware handling for html/json 3. error creation (which will be part of PR #7477) - ✨ add express error handler for html/json - one true error handler for express responses - contains still some TODO's, but they are not high priority for first implementation/integration - this middleware only takes responsibility of either rendering html responses or return json error responses - 🎨 use new express error handler in middleware/index - 404 and 500 handling - 🎨 return error instead of error message in permissions/index.js - the rule for error handling should be: if you call a unit, this unit should return a custom Ghost error - 🎨 wrap serve static module - rule: if you call a module/unit, you should always wrap this error - it's always the same rule - so the caller never has to worry about what comes back - it's always a clear error instance - in this case: we return our notfounderror if serve static does not find the resource - this avoid having checks everywhere - 🎨 replace usages of errors/index.js functions and adapt tests - use logging.error, logging.warn - make tests green - remove some usages of logging and throwing api errors -> because when a request is involved, logging happens automatically - 🐛 return errorDetails to Ghost-Admin - errorDetails is used for Theme error handling - 🎨 use 500er error for theme is missing error in theme-handler - 🎨 extend file rotation to 1w

content/themes/casper

@@ -1 +1 @@
-Subproject commit 95cec988f5b0dc0d473ab11222fe78105517225f
+Subproject commit 5c51195ca6cafbe3948c72b1461ad91b6195cff3


kirrg001 · 2016-10-05T12:10:00Z

ready for review 🇹🇭

[ci skip]

- register all errors in one file - inheritance from GhostError - option pattern [ci skip]

[ci skip]

- option pattern for errors - use GhostError when needed

ErisDS

@ErisDS
https://github.com/TryGhost/Ghost/pull/7477/files#diff-7a5f479230bbe115458d26c3c7026164L94
I deleted the debug feature, do you want to keep it and if yes, can you explain why? :)

Debug shows me the number of milliseconds since the last debug call. Therefore, as I have debug statements inside of the controllers, this piece of (definitely ugly) code means I can see what is taking the time between when the request is received and when it is responded to in a single interface. I'd like to keep this until I have finished the refactoring work I'm doing, i.e. until the end of the alpha

d6b9126
This is a separate commit. I can extract this change if you want 🌮

The commit doesn't exist, not sure what it was?

core/server/config/index.js

@@ -38,6 +36,7 @@ localUtils.makePathsAbsolute.bind(nconf)();
 * @TODO: ghost-cli?
 */
 nconf.set('ghostVersion', packageInfo.version);
+nconf.set('env', env);


core/server/errors.js

+     */
+    this.statusCode = 500;
+    this.errorType = 'InternalServerError';
+    this.level = 'normal';


core/server/middleware/error-handler.js

+    if (!(err instanceof errors.GhostError)) {
+        err = new errors.GhostError({
+            err: err
+        });


core/server/errors.js

@@ -13,6 +13,7 @@ function GhostError(options) {

    /**
     * defaults
+     * @TODO: I'd like to add the usage of an individual ID to errors, as we have in ignition


core/server/middleware/index.js

@@ -92,6 +92,17 @@ setupMiddleware = function setupMiddleware(blogApp) {
        next();
    });

+    if (debug.enabled) {


ErisDS · 2016-10-06T11:55:04Z

This PR wins a 🍰 and a 🍾 because we can now see errors for what is causing the random Travis failures 🎉

I've logged the output to #7470 to deal with later, and restarted the builds so we can merge 👯

ErisDS · 2016-10-06T12:21:55Z

Wow, it took me 6 restarts on node v6+MySQL to get this to go green 😲 something to look into next week.

@todo

- 🛠 add bunyan and prettyjson, remove morgan - ✨ add logging module - GhostLogger class that handles setup of bunyan - PrettyStream for stdout - ✨ config for logging - @todo: testing level fatal? - ✨ log each request via GhostLogger (express middleware) - @todo: add errors to output - 🔥 remove errors.updateActiveTheme - we can read the value from config - 🔥 remove 15 helper functions in core/server/errors/index.js - all these functions get replaced by modules: 1. logging 2. error middleware handling for html/json 3. error creation (which will be part of PR TryGhost#7477) - ✨ add express error handler for html/json - one true error handler for express responses - contains still some TODO's, but they are not high priority for first implementation/integration - this middleware only takes responsibility of either rendering html responses or return json error responses - 🎨 use new express error handler in middleware/index - 404 and 500 handling - 🎨 return error instead of error message in permissions/index.js - the rule for error handling should be: if you call a unit, this unit should return a custom Ghost error - 🎨 wrap serve static module - rule: if you call a module/unit, you should always wrap this error - it's always the same rule - so the caller never has to worry about what comes back - it's always a clear error instance - in this case: we return our notfounderror if serve static does not find the resource - this avoid having checks everywhere - 🎨 replace usages of errors/index.js functions and adapt tests - use logging.error, logging.warn - make tests green - remove some usages of logging and throwing api errors -> because when a request is involved, logging happens automatically - 🐛 return errorDetails to Ghost-Admin - errorDetails is used for Theme error handling - 🎨 use 500er error for theme is missing error in theme-handler - 🎨 extend file rotation to 1w

refs TryGhost#7116, refs TryGhost#2001 - Changes the way Ghost errors are implemented to benefit from proper inheritance - Moves all error definitions into a single file - Changes the error constructor to take an options object, rather than needing the arguments to be passed in the correct order. - Provides a wrapper so that any errors that haven't already been converted to GhostErrors get converted before they are displayed. Summary of changes: * 🐛 set NODE_ENV in config handler * ✨ add GhostError implementation (core/server/errors.js) - register all errors in one file - inheritance from GhostError - option pattern * 🔥 remove all error files * ✨ wrap all errors into GhostError in case of HTTP * 🎨 adaptions - option pattern for errors - use GhostError when needed * 🎨 revert debug deletion and add TODO for error id's

@todo

- 🛠 add bunyan and prettyjson, remove morgan - ✨ add logging module - GhostLogger class that handles setup of bunyan - PrettyStream for stdout - ✨ config for logging - @todo: testing level fatal? - ✨ log each request via GhostLogger (express middleware) - @todo: add errors to output - 🔥 remove errors.updateActiveTheme - we can read the value from config - 🔥 remove 15 helper functions in core/server/errors/index.js - all these functions get replaced by modules: 1. logging 2. error middleware handling for html/json 3. error creation (which will be part of PR TryGhost#7477) - ✨ add express error handler for html/json - one true error handler for express responses - contains still some TODO's, but they are not high priority for first implementation/integration - this middleware only takes responsibility of either rendering html responses or return json error responses - 🎨 use new express error handler in middleware/index - 404 and 500 handling - 🎨 return error instead of error message in permissions/index.js - the rule for error handling should be: if you call a unit, this unit should return a custom Ghost error - 🎨 wrap serve static module - rule: if you call a module/unit, you should always wrap this error - it's always the same rule - so the caller never has to worry about what comes back - it's always a clear error instance - in this case: we return our notfounderror if serve static does not find the resource - this avoid having checks everywhere - 🎨 replace usages of errors/index.js functions and adapt tests - use logging.error, logging.warn - make tests green - remove some usages of logging and throwing api errors -> because when a request is involved, logging happens automatically - 🐛 return errorDetails to Ghost-Admin - errorDetails is used for Theme error handling - 🎨 use 500er error for theme is missing error in theme-handler - 🎨 extend file rotation to 1w

refs TryGhost#7116, refs TryGhost#2001 - Changes the way Ghost errors are implemented to benefit from proper inheritance - Moves all error definitions into a single file - Changes the error constructor to take an options object, rather than needing the arguments to be passed in the correct order. - Provides a wrapper so that any errors that haven't already been converted to GhostErrors get converted before they are displayed. Summary of changes: * 🐛 set NODE_ENV in config handler * ✨ add GhostError implementation (core/server/errors.js) - register all errors in one file - inheritance from GhostError - option pattern * 🔥 remove all error files * ✨ wrap all errors into GhostError in case of HTTP * 🎨 adaptions - option pattern for errors - use GhostError when needed * 🎨 revert debug deletion and add TODO for error id's

kirrg001 mentioned this pull request Oct 3, 2016

🎨 logging #7431

Merged

kirrg001 changed the title ~~[WIP] error creation~~ [WIP] Error creation Oct 4, 2016

kirrg001 force-pushed the 1.0.0-dev/logging branch from 9126cee to 4104323 Compare October 4, 2016 16:22

ErisDS reviewed Oct 4, 2016

View reviewed changes

content/themes/casper

@@ -1 +1 @@

Subproject commit 95cec988f5b0dc0d473ab11222fe78105517225f

Subproject commit 5c51195ca6cafbe3948c72b1461ad91b6195cff3

This comment was marked as abuse.

Sign in to view

This comment was marked as abuse.

Sign in to view

kirrg001 mentioned this pull request Oct 4, 2016

🐛 Provide a default statusCode for errors #7485

Closed

kirrg001 force-pushed the 1.0.0-dev/logging branch 4 times, most recently from 267bb5a to 37e7c33 Compare October 5, 2016 11:30

kirrg001 changed the title ~~[WIP] Error creation~~ Error creation Oct 5, 2016

kirrg001 changed the title ~~Error creation~~ ✨ Error creation Oct 5, 2016

kirrg001 added 4 commits October 5, 2016 16:57

🐛 set NODE_ENV in config handler

0be97ba

[ci skip]

✨ add GhostError implementation (core/server/errors.js)

7442504

- register all errors in one file - inheritance from GhostError - option pattern [ci skip]

🔥 remove all error files

1e0aeb3

[ci skip]

✨ wrap all errors into GhostError in case of HTTP

c76232d

[ci skip]

kirrg001 force-pushed the 1.0.0-dev/logging branch 2 times, most recently from 4fb767b to 791ad8c Compare October 5, 2016 14:59

🎨 adaptions

5977cf9

- option pattern for errors - use GhostError when needed

kirrg001 force-pushed the 1.0.0-dev/logging branch from 791ad8c to 5977cf9 Compare October 5, 2016 15:26

ErisDS reviewed Oct 6, 2016

View reviewed changes

ErisDS mentioned this pull request Oct 6, 2016

Database versions #7499

Merged

🎨 revert debug deletion and add TODO for error id's

c38a11d

kirrg001 commented Oct 6, 2016

View reviewed changes

core/server/errors.js

@@ -13,6 +13,7 @@ function GhostError(options) {

/**

* defaults

* @TODO: I'd like to add the usage of an individual ID to errors, as we have in ignition

This comment was marked as abuse.

Sign in to view

kirrg001 commented Oct 6, 2016

View reviewed changes

core/server/middleware/index.js

@@ -92,6 +92,17 @@ setupMiddleware = function setupMiddleware(blogApp) {

next();

});

if (debug.enabled) {

This comment was marked as abuse.

Sign in to view

ErisDS merged commit d81bc91 into TryGhost:master Oct 6, 2016

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

✨ Error creation #7477

✨ Error creation #7477

kirrg001 commented Oct 3, 2016 •

edited

Loading

This comment was marked as abuse.

This comment was marked as abuse.

kirrg001 commented Oct 5, 2016

ErisDS left a comment

This comment was marked as abuse.

This comment was marked as abuse.

This comment was marked as abuse.

This comment was marked as abuse.

This comment was marked as abuse.

This comment was marked as abuse.

ErisDS commented Oct 6, 2016

ErisDS commented Oct 6, 2016

		@@ -1 +1 @@
		Subproject commit 95cec988f5b0dc0d473ab11222fe78105517225f
		Subproject commit 5c51195ca6cafbe3948c72b1461ad91b6195cff3

✨ Error creation #7477

✨ Error creation #7477

Conversation

kirrg001 commented Oct 3, 2016 • edited Loading

overview changes

GhostError and inheritance

Option pattern for errors

Error handling

This comment was marked as abuse.

This comment was marked as abuse.

kirrg001 commented Oct 5, 2016

ErisDS left a comment

Choose a reason for hiding this comment

This comment was marked as abuse.

This comment was marked as abuse.

This comment was marked as abuse.

This comment was marked as abuse.

This comment was marked as abuse.

This comment was marked as abuse.

ErisDS commented Oct 6, 2016

ErisDS commented Oct 6, 2016

kirrg001 commented Oct 3, 2016 •

edited

Loading