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

Epic: Server side error handling: codes, statuses, messaging & i18n #6526

Closed
4 of 11 tasks
ErisDS opened this issue Feb 17, 2016 · 1 comment
Closed
4 of 11 tasks

Epic: Server side error handling: codes, statuses, messaging & i18n #6526

ErisDS opened this issue Feb 17, 2016 · 1 comment
Labels
help wanted [triage] Ideal issues for contributors to help with server / core Issues relating to the server or core of Ghost

Comments

@ErisDS
Copy link
Member

ErisDS commented Feb 17, 2016

Ghost's error handling leaves quite a lot to be desired at the moment:

  • Our module which handles error logging, throwing & rendering tries to do a bit too much, without much clarity about what it is meant to do.
  • There's some really dirty logic in the error handling module to cope with config & theme templates
  • We have no support for proper log levels or debug messages, which makes it harder to debug ongoing issues than it should be.
  • There's confusion between error code and status code, and dogfooding our own API via HTTP makes this worse.
  • There's no centralised concept of codes or identifiers for a particular error also making debugging tricky.
  • The API doesn't serve enough information about an error, particularly validations.
  • We have a nice idea of an error message having 3 parts: error + context + help, but these were hardcoded throughout the codebase, and have now been moved out to the translations file, meaning they'll get auto-translated no matter where they are served/rendered.
  • Themes are lacking in the ability to distinguish between 404 and 500 errors

There are already several issues open about improving these issues:

As well as a couple of client side issues, which mainly exist because the server side/API errors are so inconsistent & lacking in useful content:

So lets break this down a bit into a set of clearer questions / problems, and potential solutions:

1. Status codes vs error codes

All errors that are generated by Ghost itself, should have an error code which identifies them. This code can then be used for translations at the point of render/display, as well as being used for easier debugging.

Status codes are different, all errors can also have a status code that they should be served with, if sent back via HTTP, but this is a separate concept to an error code.

2. Messaging

We should try to pursue and improve upon our concept of errors having 3 parts to a message. The idea is:

  • line 1 = error = the actual error message. This may not have come from Ghost itself but we should endeavour to replace all errors generated elsewhere with clearer messaging.
  • line 2 = context = some information about where this error was thrown or why
  • line 3 = help = usually a URL, where to go to get more help about this error

To enable us to improve this messaging (as well as to enable us to have a concept of error code) all codes & messages should be stored in a centralised file, and the code that needs to send the error should pull the messaging out.

The messaging for an error should stay in English until such time as it's displayed in a place that has a concept of being translated. E.g. if it is getting sent in response to an API call that had a header, or it's being rendered as part of a theme which is translated (neither of these things exist yet).

3. Log levels & debugging

Two slightly different problems, potentially solvable by the same thing. This needs fixing, and should be pursued via the discussion in #2001.

4. Cleaning up the error module

There's a lot that can be done, I think, to separate out and refactor the code in server/errors/index.js into something a bit cleaner and clearer.

If we can come up with a plan on how to move forward with logging/debugging, then we can split out the part which is for logging errors nicely into something which uses whatever tools we decide on. The rendering part can then be separated and cleaned up in accordance #2030.

It's also worth noting, we might want to look at making our server-side error rendering with error templates more configurable, as Ghost swallowing all errors is a problem for people using ghost as a submodule / middleware of a larger app.

From an i18n perspective, the renderer is going to need to be aware of whether or not it should be translating these messages. E.g. if the admin panel is set to German, and we're rendering an admin error then the message should be translated, but if it's being rendered to the theme it needs to remain in English (until such time as we have a theme language/locale setting).

5. API errors

When being served from the API, errors need to not be stripped of all their useful info. As being discussed in #6050 we need to make sure we serve useful information for validation errors in particular.

Ideally, we should update our API to use the JSONAPI.org error format: http://jsonapi.org/format/#error-objects. This didn't exist when we first coded the API, but has now settled into something clear and usable.

6. Themes

As well as cleaning up the error page rendering logic, it would also be great to provide a bit more flexibility at the theme layer. Ideally, error pages rendered from the theme should provide a way to distinguish between a 404 error, and any other error.

The error template currently gets 3 pieces of data: message, code and stack. Code needs to be disambiguated between status code and error code, the message could be expanded to provide our 3 part error message.

We should consider providing error as a context (with a body class, like all other contexts), or whether that doesn't make sense (it sort of does for 404 errors, but not other errors).

Tasks:

  • disambiguate between status code and error code
  • introduce a concept of error code to all errors thrown from Ghost
  • centralise the error, context & help components of our error messages & group them by code (+ i18n key?)
  • refactor how i18n is done for errors so it's done at the point of display, in accordance with the context in which it is going to be shown
  • push forward the discussion in Discussion: Error handling, logging and debugging improvements #2001 & come up with a long term plan for logging/debugging
  • implement the long term plan for logging/debugging 😉
  • refactor the error module as per Refactor flow of error rendering #2030
  • get rid of the weird logic in the error module i.e. the updateActiveTheme method
  • improve the API error format Improve 422 validation error responses #6050 to serve validation info
  • improve the API error format Improve 422 validation error responses #6050 to serve the Ghost error code, context + help messages
  • look at improving tools provided to themes for handling errors (raise an issue)
@ErisDS ErisDS added i18n server / core Issues relating to the server or core of Ghost labels Feb 17, 2016
@ErisDS ErisDS changed the title Server side error handling: codes, statuses, messaging & i18n Epic: Server side error handling: codes, statuses, messaging & i18n Feb 17, 2016
ErisDS added a commit to ErisDS/Ghost that referenced this issue Feb 17, 2016
refs TryGhost#6526

- Change our errors to use `statusCode` for the status code (like res.statusCode)
- Use statusCode for anything that's supposed to be the statusCode, rather than an error idenfier/code
- Update all the tests that check the key
- Route tests don't need fixing as the status codes are still returned correctly
@ErisDS ErisDS added the help wanted [triage] Ideal issues for contributors to help with label Mar 2, 2016
@kirrg001 kirrg001 mentioned this issue Jul 14, 2016
6 tasks
@kirrg001 kirrg001 added the later [triage] Things we intend to work but are not immediate priority label Jul 21, 2016
@kirrg001
Copy link
Contributor

I've added the later label.
Closing for now in favour of #7116 .

@kirrg001 kirrg001 mentioned this issue Jul 22, 2016
24 tasks
@ErisDS ErisDS removed the later [triage] Things we intend to work but are not immediate priority label Oct 12, 2016
kirrg001 pushed a commit to juan-g/Ghost that referenced this issue Jan 9, 2018
pull request TryGhost#8437

- backendTranslations switch set to false for now on file
core/server/i18n.js
- removed subsection "Translation of back-end messages" from the section
"Optional Advanced Features" on the help file
content/locales/TRANSLATIONS.md
- another related minor update for the help file
- comment added to file i18n.js: before enabling back-end message
translations, see previous tasks on issue TryGhost#6526 (error codes or
identifiers, error message translation at the point of display...)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
help wanted [triage] Ideal issues for contributors to help with server / core Issues relating to the server or core of Ghost
Projects
None yet
Development

No branches or pull requests

2 participants