-
Notifications
You must be signed in to change notification settings - Fork 15.7k
feat(db): custom database error messages #34674
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
base: master
Are you sure you want to change the base?
feat(db): custom database error messages #34674
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.
Review by Korbit AI
Korbit automatically attempts to detect when you fix issues in new commits.
Category | Issue | Status |
---|---|---|
Unclear Icon Purpose for Documentation Link ▹ view | 🧠 Not in standard | |
Sequential Regex Pattern Matching ▹ view | 🧠 Incorrect | |
Missing URL Validation ▹ view | 🧠 Not in standard | |
Unsafe Issue Codes Access ▹ view | 🧠 Not in scope |
Files scanned
File Path | Reviewed |
---|---|
superset-frontend/src/components/ErrorMessage/CustomDocLink.tsx | ✅ |
superset-frontend/src/components/ErrorMessage/DatabaseErrorMessage.tsx | ✅ |
docs/docs/configuration/databases.mdx | ✅ |
superset/db_engine_specs/base.py | ✅ |
superset/config.py | ✅ |
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.
const theme = useTheme(); | ||
return ( | ||
<a href={url} target="_blank" rel="noopener noreferrer"> | ||
{label} <Icons.Full iconSize="m" iconColor={theme.colorPrimary} /> |
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
export const CustomDocLink = ({ url, label }: CustomDocLinkProps) => { | ||
const theme = useTheme(); | ||
return ( | ||
<a href={url} target="_blank" rel="noopener noreferrer"> |
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
<IssueCode {...issueCode} key={issueCode.code} /> | ||
)) | ||
.reduce((prev, curr) => [prev, <br />, curr])} | ||
{extra.issue_codes?.flatMap((issueCode, idx, arr) => [ |
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
for regex, (message, error_type, extra) in [ | ||
*config_custom_errors.items(), | ||
*cls.custom_errors.items(), | ||
]: | ||
if match := regex.search(raw_message): |
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #34674 +/- ##
===========================================
+ Coverage 0 71.85% +71.85%
===========================================
Files 0 588 +588
Lines 0 43534 +43534
Branches 0 4708 +4708
===========================================
+ Hits 0 31280 +31280
- Misses 0 11023 +11023
- Partials 0 1231 +1231
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
@DamianPendrak so would you need files per database as different databases could have different links and different messages? Would it be helpful to also have a link or a collapsed box that will show the original error? |
@sadpandajoe My idea was to separate them based on the regex. But now I see that it might be an issue if there is the same error message from different databases, and you can't separate them using regex. I pushed an update with a changed structure of the CUSTOM_DATABASE_ERRORS = {
"trino": {
re.compile(r"TrinoUserError"): (
"Custom error message",
SupersetErrorType.GENERIC_DB_ENGINE_ERROR,
{
"custom_doc_links": [
{
"url": "https://example.com/docs",
"label": "Check documentation"
},
],
}
),
},
"postgres": {}
} About the original error - there is a similar way to overwrite the error messages in the db engine file in |
@DamianPendrak one other question. If you want to override multiple error messages and you have multiple databases, won't the config file get huge? Wondering if that is fine or not. |
10b4bb6
to
d479656
Compare
@sadpandajoe, good point. The config file could become huge, so it's a good idea to put it into a separate file. I pushed the change that imports the custom errors, but you can still overwrite them in the config file |
superset/custom_database_errors.py
Outdated
# Example: | ||
# CUSTOM_DATABASE_ERRORS = { | ||
# "trino": { | ||
# re.compile(r'message="(?P<message>[^"]*)"'): ( |
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.
Could we perhaps make it easier to append a custom message or just the doc links to the original message? I know that this regex does that, but who likes writing regexes 😄
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.
Right, that's probably too advanced example. I added a simpler one that matches with a part of the error message "permission denied for view". The regex is also used in the custom errors in the database engine files, so it keeps the same approach
const theme = useTheme(); | ||
return ( | ||
<a href={url} target="_blank" rel="noopener noreferrer"> | ||
{label} <Icons.Full iconSize="m" iconColor={theme.colorPrimary} /> |
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.
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.
Fixed
superset/db_engine_specs/base.py
Outdated
config_custom_errors = app.config.get("CUSTOM_DATABASE_ERRORS", {}) | ||
if not isinstance(config_custom_errors, dict): | ||
config_custom_errors = {} | ||
db_engine_custom_errors = config_custom_errors.get(cls.engine_name, {}) |
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.
Should we also check for cls.engine
? I tested on sqlite, and when I used sqlite
as key in CUSTOM_DATABASE_ERRORS dict, it didn't work because cls.engine_name
returns SQLite. Perhaps we could allow for both "human readable" and "normal" engine names?
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.
Done. Works with both now
hey @DamianPendrak could it be a concern if I have 2 |
@Vitor-Avila it could be an issue if the error message is the same and cannot be distinguished from another error message using regex. If one of the auth methods returned a different error message, then it can be achieved. |
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.
LGTM
Added |
Thanks @Vitor-Avila for pointing that out. I changed the implementation to match errors with the database unique name instead of the database engine name. It allows users to create more precise custom errors. @kgabryje are you okay with reviewing the changes? |
superset/db_engine_specs/base.py
Outdated
if isinstance(database_errors, dict): | ||
db_engine_custom_errors.update(database_errors) | ||
|
||
if not isinstance(db_engine_custom_errors, dict): |
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.
Is there any scenario where db_engine_custom_errors is not a dict? We initialize it as dict in line 1343, and then update it only with other dicts
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.
That's right. Removed the unnecessary check
if not isinstance(config_custom_errors, dict): | ||
config_custom_errors = {} | ||
|
||
db_engine_custom_errors = {} |
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.
This part is very similar to base.py. Does it make sense to DRY it?
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.
Good point. Moved it to a method
🎪 Showtime deployed environment on GHA for 55d3b07 • Environment: http://34.219.185.149:8080 (admin/admin) |
@kgabryje This workflow is deprecated! Please use the new Superset Showtime system instead:
Processing your ephemeral environment request here. Action: up. More information on how to use or configure ephemeral environments |
@kgabryje Ephemeral environment spinning up at http://44.251.62.68:8080. Credentials are 'admin'/'admin'. Please allow several minutes for bootstrapping and startup. |
…age when custom message is empty
55d3b07
to
ad1ee1a
Compare
SUMMARY
Introduce
CUSTOM_DATABASE_ERRORS
in the config to replace raw database exceptions with a custom message and optional documentation links.custom_doc_links
- contains links that will show after the error messageshow_issue_info
- if set to False, hides the "See more" button that contains the issue code and link to Superset documentationBEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
Before
After
TESTING INSTRUCTIONS
superset/custom_database_errors.py
file or addCUSTOM_DATABASE_ERRORS
tosuperset_config.py
or other config file with this structure:examples
- the database connection name.permission denied for view
- part of the original message.ADDITIONAL INFORMATION