-
Notifications
You must be signed in to change notification settings - Fork 15.4k
fix: Matching errorType on superset api error with SupersetError #34261
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
fix: Matching errorType on superset api error with SupersetError #34261
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've completed my review and didn't find any issues.
Files scanned
File Path | Reviewed |
---|---|
superset-frontend/src/components/ErrorMessage/ErrorMessageWithStackTrace.tsx | ✅ |
superset-frontend/packages/superset-ui-core/src/query/types/Query.ts | ✅ |
superset-frontend/src/explore/components/controls/DatasourceControl/index.jsx | ✅ |
Explore our documentation to understand the languages and file types we support and the files we ignore.
Check out our docs on how you can make Korbit work best for you and your team.
@@ -454,16 +455,14 @@ class DatasourceControl extends PureComponent { | |||
{isMissingDatasource && !isMissingParams && ( | |||
<div className="error-alert"> | |||
{extra?.error ? ( | |||
<div className="error-alert"> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
removed redundant same error-alert wrapper
@@ -255,6 +255,7 @@ export type ErrorSource = 'dashboard' | 'explore' | 'sqllab' | 'crud'; | |||
|
|||
export type SupersetError<ExtraType = Record<string, any> | null> = { | |||
error_type: ErrorType; | |||
errorType?: ErrorType; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@justinpark Can't we fix the API responses to use the same field? Having both in the type definition looks really bad and is an indicative that we should fix the APIs.
) (cherry picked from commit 229d925)
SUMMARY
For Airbnb, we display custom errors according to the error_type. However, in the current Superset API, unlike the query result API, SupersetApiError is thrown, and error_type is mapped to errorType, which causes custom overrides to not work properly. In this PR, I have resolved the issue by correcting the type of the errorType value.
BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
Before:

After:

TESTING INSTRUCTIONS
locally