[SIP-40] Proposal for Custom Error Messages

[etr2460]

[SIP-40] Proposal for Custom Error Messages

Motivation

The current user experience for error cases in Superset does not provide actionable feedback to users. Error messages often are displayed straight from the backend and are fairly inscrutable. Additionally, there’s no way to customize or add other functionality to these messages on a deployment by deployment basis. A key part of the user experience is providing friendly, actionable messaging when something goes wrong.

Proposed Change

We propose a new, unified pattern for error handling in API endpoints across all of Superset. This will consist of the following work:

1. Unify the backend around a standard error payload for all API errors returned from Superset of the format:

{
  errors: [
    {
      message: string,
      error_type: Enum,
      level: ‘info’ | ‘warning’ | ‘error’,
      extra: Dict[string, Any],
    }, ...
  ]
}

2. Create an Enum on both the server and client side for uniquely identifying error types. The specific type of an error is included in the standard error payload (e.g. ACCESS_DENIED, DB_ENGINE_SYNTAX_ERROR, etc.)
3. Refactor Superset’s frontend to go through a single SupersetAlert component for handling all error messages. This component will import a renderer file that defaults to some generic error customization messages (ex. Adding Request Access links).
4. Allow a Superset admin to specify an override renderer file that gets used instead of the default renderer. This override renderer is injected by leveraging the Registry class from @superset-ui/core to handle the custom renderers registration.

New or Changed Public Interfaces

• All APIs within Superset will return the above error payload on 4xx and 5xx status code responses.
• Client side options will be made public for injecting your own error rendering file

New dependencies

N/A

Migration Plan and Compatibility

No migrations should be necessary, but the new API may not be backwards compatible. Any changes to the current API will be notated in UPDATING.md and should take place prior to v1.0.0.

Rejected Alternatives

Server side rendering of errors was rejected as we’re trying to remove the existing templates and move all rendering to the client side

cc: @kristw @rusackas @nytai @graceguo-supercat @ktmud

[kristw]

@etr2460 The error code and SupersetAlert component seems useful for usage outside of Superset app as well. Do you want to implement the front-end error handling as a new package @superset-ui/superset-error to provide typings and components?

Allow a Superset admin to specify an override renderer file that gets used instead of the default renderer. This override renderer is injected with a Webpack Plugin.

Can create a registry for renderers and use error code as key, similar to the chart registries. The SupersetAlert component then can ask the registry if a custom renderer for the received error exist, otherwise fallback to default renderer. Adding a custom renderer will be the same way with registering chart plugin.

We propose a new, unified pattern for error handling in API endpoints across all of Superset. This will consist of the following work:

Could you add a few examples of the current error api result that are fragmented?

[craig-rueda]

@etr2460, this is great. It looks like a step towards getting all API responses to have a standard shape. One thing that I would change a tiny bit would be the response shape. Enums are awesome for this sort of thing, but one thing I would add would be an error "code" which would simply be an int which can then be taken action upon by the consumer. There's also the potential for having multiple errors in a given response (think API validation where several keys are required). In my view, Enum names should be used solely for the purpose of code organization, and should not be transmitted to consumers. Instead, error state should depend on the "error code".

A slight variation to your structure:

{
    "errors": [{
       "code": 123,
       "message": "Something is wrong here",
       "extra": {
           // Additional contextual stuff in here, for instance field validation messages
       }
    }]
}

[etr2460]

@kristw:

One concern I had about moving this out into a Superset error package was that I don't think strings for default error messages would get translated in those packages. That seemed like a non-starter to me, so I didn't consider it. If you have a way to resolve this, then I'd be happy to hear it!

For examples of fragmented error handling, in SQL Lab we render alerts based on error messages injected inside the query object (as well as the poorly defined link parameter): https://github.com/apache/incubator-superset/blob/291306392443a5a0d0e2ee0cc4a95d37c56d4589/superset-frontend/src/SqlLab/components/ResultSet.jsx#L201-L212
but in Charts we render them based off the chartAlert param (in a completely different component too):
https://github.com/apache/incubator-superset/blob/291306392443a5a0d0e2ee0cc4a95d37c56d4589/superset-frontend/src/chart/Chart.jsx#L136-L143

@craig-rueda:

I totally agree with an errors array inside the structure, that makes a bunch of sense. We'll need to consider what the default UI experience is (multiple alerts? merging errors?) but I think it'll be worth it.

One thing I don't agree with is using an error code as opposed to an enum. This is for two reasons:

• The error code in the response would be easily confused with a http status code that's already an int.
• By using an Enum, we can both provide some idea of what the error is to the user, but also provide a unique string for searching error resolutions online. Instead of getting a Superset Error 1432 you'd get a Superset Error PRESTO_INTERNAL_SERVER_ERROR which i think is much more readable and could save the extra trouble of mapping a code to resolutions online. The enum provides additional value to the error message as an error like this might include the message: Presto encountered an unknown issue when running this query. Please try again later. and the enum specifies a more technical error

[craig-rueda]

Makes sense. I think one nice thing about the int is that it augments the status code and can be grouped in much the same way as HTTP statues, i.e. [10000,11000) are for error class A, and [11000,12000) are for class B, etc.. I can see what you're saying from the point of view of improving the readability of the errors based on code. Seems fine either way :)

[kristw]

@etr2460 Got it. Also to keep the scope actionable, I agree with modifying the code inside superset as a start. You can still leverage the Registry class from @superset-ui/core to handle the custom renderers registration.

[suddjian]

I love it! Been wanting to address this for a while, so thank you. I have some questions/suggestions.

How should translation fit into this? Error messages are sometimes written to have dynamic values contained in them, such as "{input}" is not a valid url, or {some_resource} is already in use. Do we only allow static error messages? Is there a way for dynamic error messages to play well with the translation system? Dynamic error messages can often make a big difference in UX: it can be tricky to write static messages that accurately describe every situation where you might see the error.

I lean towards a system where the text of the error message is defined entirely on the frontend, with the error object containing parameters with which to construct a message. It makes sense for the backend to supply an error message for debugging purposes, but that field should indicate that it's not meant to be used in UI.

I'd also like form validation errors to have a standard schema, so that the frontend can display validation errors in the appropriate place in the UI. Since we seem to be going towards having an array of error objects, we could attach a field_name value to an error when it relates to a specific field. So if a hypothetical url field called custom_url was submitted with a bad value, you'd get back:

{
 "errors": [{
    "error_type": "INPUT_INVALID_URL",
    "level": "error",
    "debug_message": "Invalid url value for custom_url",
    "field_name": "custom_url",
    "message_params": {
      "input": "https//notvalid"
    }
  }]
}

Lastly, I could use some clarification: what's the purpose of allowing admins to override the renderer? Nevermind, I get it now - it's just for the generic messages, that makes sense.

[etr2460]

@suddjian:

Dynamic error messages should be supported on the frontend with the translation package: https://github.com/apache-superset/superset-ui/tree/master/packages/superset-ui-translation#api

My vision is to have a default renderer that simply renders the message passed from the backend, something we can guarantee exists every time. That's why i'd prefer not to label it as debug since if it's not handled in any other way, we'd display this field. However, if you wanted to render a more specific component, you'd rely on the error_type passed and the content in extra to render it. I would envision that applying to form validation errors too. An InputValidationError would be a subtype of Error where error_type = INPUT_VALIDATION_ERROR and extra is defined as an object that must contain a field_name and a message_params attribute. This InputValidationError could also be defined on the backend so the extra field is guaranteed to match what's expected. Then you'd be able to get type safety on the front end as well. Thoughts?

[suddjian]

I think that vision makes sense. The additional definition on the type contract of extra is extremely helpful.

I wonder if there is a more descriptive name out there than extra for that info. Or maybe splitting extra out into more specific objects with contracts related to their domain would be useful. (validation, documentation, etc.)

[etr2460]

Yeah, if you have a better name than extra i'm all ears. Maybe metadata or details? Not sure, but I think I've seen extra before.

On the note of splitting extra out into other objects, my goal was to allow extra to be as free form and flexible as possible so that you could add whatever relevant data was needed within it to render an error (e.g. on a chart error in a dashboard maybe we want to know the chart owner's name, in a SQL Lab failed query maybe the url of the db engine where the failed query was run). I'm hesitant to add too much structure explicitly, but i hope that norms will quickly present themselves when migrating the current errors over to the new format.

[etr2460]

I've updated the SIP summary to include feedback from the thread and will be kicking off a vote now

[rusackas]

This has been approved/voted. Hooray! Closing :)