You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@superset.apache.org by GitBox <gi...@apache.org> on 2020/02/24 18:45:25 UTC
[GitHub] [incubator-superset] etr2460 opened a new issue #9194: [SIP-40]
Proposal for Custom Error Messages
etr2460 opened a new issue #9194: [SIP-40] Proposal for Custom Error Messages
URL: https://github.com/apache/incubator-superset/issues/9194
## [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:
```
{
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 with a Webpack Plugin.
### 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
There may be a new Webpack Plugin required for injecting the custom renderer.
### 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
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] rusackas closed issue #9194: [SIP-40] Proposal for Custom Error Messages
Posted by GitBox <gi...@apache.org>.
rusackas closed issue #9194:
URL: https://github.com/apache/superset/issues/9194
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] rusackas commented on issue #9194: [SIP-40] Proposal for Custom Error Messages
Posted by GitBox <gi...@apache.org>.
rusackas commented on issue #9194:
URL: https://github.com/apache/superset/issues/9194#issuecomment-824361057
This has been approved/voted. Hooray! Closing :)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [incubator-superset] kristw commented on issue #9194: [SIP-40]
Proposal for Custom Error Messages
Posted by GitBox <gi...@apache.org>.
kristw commented on issue #9194: [SIP-40] Proposal for Custom Error Messages
URL: https://github.com/apache/incubator-superset/issues/9194#issuecomment-590565646
@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.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [incubator-superset] etr2460 commented on issue #9194: [SIP-40]
Proposal for Custom Error Messages
Posted by GitBox <gi...@apache.org>.
etr2460 commented on issue #9194: [SIP-40] Proposal for Custom Error Messages
URL: https://github.com/apache/incubator-superset/issues/9194#issuecomment-593659659
I've updated the SIP summary to include feedback from the thread and will be kicking off a vote now
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [incubator-superset] kristw edited a comment on issue #9194:
[SIP-40] Proposal for Custom Error Messages
Posted by GitBox <gi...@apache.org>.
kristw edited a comment on issue #9194: [SIP-40] Proposal for Custom Error Messages
URL: https://github.com/apache/incubator-superset/issues/9194#issuecomment-590497049
@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?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [incubator-superset] etr2460 commented on issue #9194: [SIP-40]
Proposal for Custom Error Messages
Posted by GitBox <gi...@apache.org>.
etr2460 commented on issue #9194: [SIP-40] Proposal for Custom Error Messages
URL: https://github.com/apache/incubator-superset/issues/9194#issuecomment-590549191
@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
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [incubator-superset] craig-rueda commented on issue #9194: [SIP-40]
Proposal for Custom Error Messages
Posted by GitBox <gi...@apache.org>.
craig-rueda commented on issue #9194: [SIP-40] Proposal for Custom Error Messages
URL: https://github.com/apache/incubator-superset/issues/9194#issuecomment-590553360
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 :)
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [incubator-superset] suddjian edited a comment on issue #9194:
[SIP-40] Proposal for Custom Error Messages
Posted by GitBox <gi...@apache.org>.
suddjian edited a comment on issue #9194: [SIP-40] Proposal for Custom Error Messages
URL: https://github.com/apache/incubator-superset/issues/9194#issuecomment-590599304
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 `{capitalized_field_name} 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:
```json
{
"errors": [{
"error_type": "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?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [incubator-superset] suddjian edited a comment on issue #9194:
[SIP-40] Proposal for Custom Error Messages
Posted by GitBox <gi...@apache.org>.
suddjian edited a comment on issue #9194: [SIP-40] Proposal for Custom Error Messages
URL: https://github.com/apache/incubator-superset/issues/9194#issuecomment-590599304
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:
```json
{
"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?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [incubator-superset] etr2460 commented on issue #9194: [SIP-40]
Proposal for Custom Error Messages
Posted by GitBox <gi...@apache.org>.
etr2460 commented on issue #9194: [SIP-40] Proposal for Custom Error Messages
URL: https://github.com/apache/incubator-superset/issues/9194#issuecomment-590612592
@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?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [incubator-superset] craig-rueda commented on issue #9194: [SIP-40]
Proposal for Custom Error Messages
Posted by GitBox <gi...@apache.org>.
craig-rueda commented on issue #9194: [SIP-40] Proposal for Custom Error Messages
URL: https://github.com/apache/incubator-superset/issues/9194#issuecomment-590505746
@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
}
}]
}
```
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [incubator-superset] suddjian edited a comment on issue #9194:
[SIP-40] Proposal for Custom Error Messages
Posted by GitBox <gi...@apache.org>.
suddjian edited a comment on issue #9194: [SIP-40] Proposal for Custom Error Messages
URL: https://github.com/apache/incubator-superset/issues/9194#issuecomment-590599304
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 `{capitalized_field_name} 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:
```json
{
"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?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [incubator-superset] suddjian commented on issue #9194: [SIP-40]
Proposal for Custom Error Messages
Posted by GitBox <gi...@apache.org>.
suddjian commented on issue #9194: [SIP-40] Proposal for Custom Error Messages
URL: https://github.com/apache/incubator-superset/issues/9194#issuecomment-590631344
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.)
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [incubator-superset] suddjian commented on issue #9194: [SIP-40]
Proposal for Custom Error Messages
Posted by GitBox <gi...@apache.org>.
suddjian commented on issue #9194: [SIP-40] Proposal for Custom Error Messages
URL: https://github.com/apache/incubator-superset/issues/9194#issuecomment-590599304
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 `{capitalized_field_name} 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:
```json
{
errors: [{
"error_type": "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?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [incubator-superset] suddjian edited a comment on issue #9194:
[SIP-40] Proposal for Custom Error Messages
Posted by GitBox <gi...@apache.org>.
suddjian edited a comment on issue #9194: [SIP-40] Proposal for Custom Error Messages
URL: https://github.com/apache/incubator-superset/issues/9194#issuecomment-590599304
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:
```json
{
"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?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [incubator-superset] kristw commented on issue #9194: [SIP-40]
Proposal for Custom Error Messages
Posted by GitBox <gi...@apache.org>.
kristw commented on issue #9194: [SIP-40] Proposal for Custom Error Messages
URL: https://github.com/apache/incubator-superset/issues/9194#issuecomment-590497049
@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.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [incubator-superset] suddjian edited a comment on issue #9194:
[SIP-40] Proposal for Custom Error Messages
Posted by GitBox <gi...@apache.org>.
suddjian edited a comment on issue #9194: [SIP-40] Proposal for Custom Error Messages
URL: https://github.com/apache/incubator-superset/issues/9194#issuecomment-590599304
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:
```json
{
"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.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [incubator-superset] etr2460 commented on issue #9194: [SIP-40]
Proposal for Custom Error Messages
Posted by GitBox <gi...@apache.org>.
etr2460 commented on issue #9194: [SIP-40] Proposal for Custom Error Messages
URL: https://github.com/apache/incubator-superset/issues/9194#issuecomment-590685108
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.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org