-
Notifications
You must be signed in to change notification settings - Fork 155
feat(database): add custom database error message configuration system #54
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?
Conversation
- Added support for custom error messages based on database connection configurations. - Introduced `CUSTOM_DATABASE_ERRORS` in the configuration to allow administrators to define specific error messages for different database errors. - Enhanced error extraction methods to utilize custom error patterns and provide detailed feedback to users. - Updated frontend components to conditionally display error information based on the new configuration. - Added unit tests to ensure the functionality of custom error handling and message formatting.
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.
Pull request overview
This PR introduces a comprehensive custom database error message configuration system that allows Superset administrators to replace cryptic database errors with user-friendly, context-specific messages on a per-database basis.
Key Changes:
- Implements
CUSTOM_DATABASE_ERRORSconfiguration system with regex pattern matching and message interpolation - Adds
show_issue_infoflag to conditionally hide technical issue codes from error messages - Supports custom documentation links in error messages
- Updates all database error extraction call sites to pass
database_namein context
Reviewed changes
Copilot reviewed 11 out of 14 changed files in this pull request and generated 13 comments.
Show a summary per file
| File | Description |
|---|---|
tests/unit_tests/db_engine_specs/test_custom_errors.py |
Comprehensive unit tests covering custom error pattern matching, regex capture groups, show_issue_info flag, custom doc links, and fallback behavior |
superset/db_engine_specs/base.py |
Core implementation of custom error extraction logic that checks CUSTOM_DATABASE_ERRORS config before falling back to engine spec's built-in errors |
superset/errors.py |
Modified SupersetError.post_init to respect show_issue_info flag for conditional issue code suppression |
superset/config.py |
Added CUSTOM_DATABASE_ERRORS configuration constant with documentation |
superset/sql_lab.py |
Updated error extraction to pass database_name in context |
superset/models/helpers.py |
Updated error extraction to pass database_name in context |
superset/connectors/sqla/models.py |
Updated error extraction to pass database_name in context |
superset/commands/database/validate.py |
Added database_name to context for validation errors |
superset/commands/database/test_connection.py |
Added database_name to context for connection test errors |
superset-frontend/src/components/ErrorMessage/DatabaseErrorMessage.tsx |
Enhanced UI to conditionally display issue codes and custom doc links based on show_issue_info flag |
docker/pythonpath_dev/superset_config.py |
Example configuration demonstrating CUSTOM_DATABASE_ERRORS usage |
superset-frontend/package-lock.json |
Incidental dependency change (peerDependencies for eslint-plugin-icons) |
.gitignore |
Added developer-specific IDE and environment directories |
Files not reviewed (1)
- superset-frontend/package-lock.json: Language not supported
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // When show_issue_info is false, hide the "See more" button entirely | ||
| // This must be false when show_issue_info is explicitly False, regardless of other content | ||
| // If show_issue_info is False, hasDescriptionDetails must be false to hide "See more" |
Copilot
AI
Dec 8, 2025
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.
[nitpick] Redundant comments repeating the same concept. Lines 58-60 all state that when show_issue_info is false, certain behavior should occur. Consider consolidating these into a single, clear comment.
| // When show_issue_info is false, hide the "See more" button entirely | |
| // This must be false when show_issue_info is explicitly False, regardless of other content | |
| // If show_issue_info is False, hasDescriptionDetails must be false to hide "See more" | |
| /** | |
| * Only show the "See more" button and additional details if show_issue_info is not explicitly false. | |
| * If show_issue_info is false, hasDescriptionDetails will be false and all extra details are hidden. | |
| */ |
| return [ | ||
| SupersetError( | ||
| error_type=error_type, | ||
| message=message % params, |
Copilot
AI
Dec 8, 2025
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.
Potential security risk: Using % string formatting with user-controlled regex capture groups (line 1397) could be vulnerable to format string injection if the custom error message template contains format specifiers that aren't matched by the regex groups. Consider using .format() with named placeholders or validating that all format specifiers in the message template are present in the params dict to prevent potential errors or security issues.
| "license": "Apache-2.0", | ||
| "peerDependencies": { | ||
| "eslint": ">=0.8.0" | ||
| } |
Copilot
AI
Dec 8, 2025
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.
[nitpick] This change to package-lock.json appears unrelated to the custom database error message feature. The addition of peerDependencies for eslint-plugin-icons seems like an incidental change that may have occurred during dependency resolution. Consider whether this should be part of a separate PR focused on dependency management.
| hasCustomDocLinks && | ||
| extra.custom_doc_links && ( | ||
| <> | ||
| {alertDescription && <br />} | ||
| <br /> | ||
| {t('For more information, see:')} | ||
| <br /> | ||
| {extra.custom_doc_links.map((link, index) => ( |
Copilot
AI
Dec 8, 2025
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.
The logic for displaying custom doc links when show_issue_info is false has a potential issue. Line 127 checks extra.custom_doc_links for truthiness after already checking hasCustomDocLinks on line 126, which already verifies that extra.custom_doc_links exists and has length > 0. The second check on line 127 is redundant. The expression should simply be: !showIssueInfo && hasCustomDocLinks && (...)
| hasCustomDocLinks && | |
| extra.custom_doc_links && ( | |
| <> | |
| {alertDescription && <br />} | |
| <br /> | |
| {t('For more information, see:')} | |
| <br /> | |
| {extra.custom_doc_links.map((link, index) => ( | |
| hasCustomDocLinks && ( | |
| <> | |
| {alertDescription && <br />} | |
| <br /> | |
| {t('For more information, see:')} | |
| <br /> | |
| {extra.custom_doc_links!.map((link, index) => ( |
| # Match PostgreSQL "relation does not exist" errors with capture group | ||
| # This pattern captures the table name from the error message | ||
| re.compile(r'relation "(?P<table_name>.+)" does not exist', re.IGNORECASE): ( |
Copilot
AI
Dec 8, 2025
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.
The regex pattern on line 144 uses a raw string with escaped quotes (r'relation "(?P<table_name>.+)" does not exist'), but the comment on line 142 states this is for PostgreSQL errors. However, the "examples" database in Superset typically uses SQLite, not PostgreSQL. SQLite's error message for missing tables is "no such table: table_name" not "relation does not exist". This example configuration may not work as intended with the default examples database. Consider using a pattern that matches SQLite errors, or clarifying that this example assumes PostgreSQL is configured as the examples database.
| # Match PostgreSQL "relation does not exist" errors with capture group | |
| # This pattern captures the table name from the error message | |
| re.compile(r'relation "(?P<table_name>.+)" does not exist', re.IGNORECASE): ( | |
| # Match SQLite "no such table" errors with capture group | |
| # This pattern captures the table name from the error message | |
| re.compile(r'no such table: (?P<table_name>\w+)', re.IGNORECASE): ( |
| "label": "View available tables" | ||
| } | ||
| ], | ||
| "show_issue_info": True, |
Copilot
AI
Dec 8, 2025
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.
[nitpick] The example configuration uses show_issue_info: True, but according to the PR description's testing instructions, one of the key features is to test with show_issue_info: False to hide issue codes. For a more illustrative example that demonstrates the feature's primary use case (suppressing issue codes), consider setting this to False by default in the example.
| "show_issue_info": True, | |
| "show_issue_info": False, |
| // When show_issue_info is explicitly False, never show "See more" button | ||
| // Check if show_issue_info is explicitly set to False | ||
| const showIssueInfo = extra?.show_issue_info !== false; | ||
| const hasIssueCodes = | ||
| showIssueInfo && extra?.issue_codes && extra.issue_codes.length > 0; | ||
| const hasOwners = isVisualization && extra?.owners && extra.owners.length > 0; | ||
| const hasCustomDocLinks = | ||
| extra?.custom_doc_links && extra.custom_doc_links.length > 0; | ||
| // When show_issue_info is false, hide the "See more" button entirely | ||
| // This must be false when show_issue_info is explicitly False, regardless of other content | ||
| // If show_issue_info is False, hasDescriptionDetails must be false to hide "See more" | ||
| const hasDescriptionDetails = | ||
| showIssueInfo && (hasIssueCodes || hasOwners || hasCustomDocLinks); | ||
|
|
||
| const body = extra && hasDescriptionDetails && ( | ||
| <> | ||
| <p> | ||
| {t('This may be triggered by:')} | ||
| <br /> | ||
| {extra.issue_codes | ||
| ?.map<ReactNode>(issueCode => ( | ||
| <IssueCode {...issueCode} key={issueCode.code} /> | ||
| )) | ||
| .reduce((prev, curr) => [prev, <br />, curr])} | ||
| </p> | ||
| {isVisualization && extra.owners && ( | ||
| <> | ||
| {hasIssueCodes && ( | ||
| <p> | ||
| {t('This may be triggered by:')} | ||
| <br /> | ||
| {extra.issue_codes! | ||
| .map<ReactNode>(issueCode => ( | ||
| <IssueCode {...issueCode} key={issueCode.code} /> | ||
| )) | ||
| .reduce((prev, curr) => [prev, <br />, curr])} | ||
| </p> | ||
| )} | ||
| {hasOwners && ( | ||
| <> | ||
| {hasIssueCodes && <br />} | ||
| <p> | ||
| {tn( | ||
| 'Please reach out to the Chart Owner for assistance.', | ||
| 'Please reach out to the Chart Owners for assistance.', | ||
| extra.owners.length, | ||
| extra.owners!.length, | ||
| )} | ||
| </p> | ||
| <p> | ||
| {tn( | ||
| 'Chart Owner: %s', | ||
| 'Chart Owners: %s', | ||
| extra.owners.length, | ||
| extra.owners.join(', '), | ||
| extra.owners!.length, | ||
| extra.owners!.join(', '), | ||
| )} | ||
| </p> | ||
| </> | ||
| )} | ||
| {hasCustomDocLinks && ( | ||
| <> | ||
| {(hasIssueCodes || hasOwners) && <br />} | ||
| <p> | ||
| {t('For more information, see:')} | ||
| <br /> | ||
| {extra.custom_doc_links!.map((link, index) => ( | ||
| <span key={link.url}> | ||
| {index > 0 && <br />} | ||
| <a | ||
| href={link.url} | ||
| target="_blank" | ||
| rel="noopener noreferrer" | ||
| style={{ textDecoration: 'underline' }} | ||
| > | ||
| {link.label} | ||
| </a> | ||
| </span> | ||
| ))} | ||
| </p> | ||
| </> | ||
| )} | ||
| </> | ||
| ); | ||
|
|
||
| // When show_issue_info is false, show custom doc links inline in description | ||
| // instead of in the collapsible "See more" section | ||
| const inlineCustomDocLinks = | ||
| !showIssueInfo && | ||
| hasCustomDocLinks && | ||
| extra.custom_doc_links && ( | ||
| <> | ||
| {alertDescription && <br />} | ||
| <br /> | ||
| {t('For more information, see:')} | ||
| <br /> | ||
| {extra.custom_doc_links.map((link, index) => ( | ||
| <span key={link.url}> | ||
| {index > 0 && <br />} | ||
| <a | ||
| href={link.url} | ||
| target="_blank" | ||
| rel="noopener noreferrer" | ||
| style={{ textDecoration: 'underline' }} | ||
| > | ||
| {link.label} | ||
| </a> | ||
| </span> | ||
| ))} | ||
| </> | ||
| ); | ||
|
|
||
| const finalDescription = | ||
| alertDescription || inlineCustomDocLinks | ||
| ? ( | ||
| <> | ||
| {alertDescription} | ||
| {inlineCustomDocLinks} | ||
| </> | ||
| ) | ||
| : null; | ||
|
|
||
| return ( | ||
| <ErrorAlert | ||
| errorType={t('%s Error', extra?.engine_name || t('DB engine'))} | ||
| message={alertMessage} | ||
| description={alertDescription} | ||
| description={finalDescription} | ||
| type={level} | ||
| descriptionDetails={body} | ||
| descriptionDetails={hasDescriptionDetails && body ? body : undefined} | ||
| /> | ||
| ); | ||
| } |
Copilot
AI
Dec 8, 2025
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.
The frontend changes to DatabaseErrorMessage.tsx introduce new behavior for show_issue_info and custom_doc_links, but the existing test file (DatabaseErrorMessage.test.tsx) does not appear to have been updated to test these new features. Tests should be added to verify:
- When
show_issue_infoisfalse, the "See more" button is not rendered - When
show_issue_infoisfalseandcustom_doc_linksare present, the links are displayed inline in the description - When
show_issue_infoistrue(or not set) andcustom_doc_linksare present, the links appear in the collapsible section - The structure and rendering of
custom_doc_links
| return [ | ||
| SupersetError( | ||
| error_type=error_type, | ||
| message=message % params, |
Copilot
AI
Dec 8, 2025
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.
Using message % params (line 1397) could raise a KeyError or ValueError if the message template contains format specifiers (e.g., %(some_key)s) that aren't present in the params dictionary. While this is a configuration error, it would be better to catch this exception and log a helpful error message that identifies which custom error pattern caused the issue, rather than letting the exception propagate and break error handling entirely. Consider wrapping this in a try-except block.
| import re | ||
| from unittest.mock import MagicMock | ||
|
|
||
| import pytest |
Copilot
AI
Dec 8, 2025
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.
Import of 'pytest' is not used.
| import pytest |
| from pytest_mock import MockerFixture | ||
|
|
||
| from superset.db_engine_specs.base import BaseEngineSpec | ||
| from superset.errors import ErrorLevel, SupersetError, SupersetErrorType |
Copilot
AI
Dec 8, 2025
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.
Import of 'SupersetError' is not used.
| from superset.errors import ErrorLevel, SupersetError, SupersetErrorType | |
| from superset.errors import ErrorLevel, SupersetErrorType |
feat(database): add custom database error message configuration system
Fixes #21
SUMMARY
This PR introduces a configurable system (
CUSTOM_DATABASE_ERRORS) that allows Superset administrators to define custom, user-friendly error messages for specific database errors per database connection. This improves the user experience by replacing cryptic database exception messages with context-specific guidance.Key Features:
CUSTOM_DATABASE_ERRORSconfig constant that can be defined insuperset_config.pyshow_issue_info: Falseflag to hide technical issue codes and the "See more" buttoncustom_doc_linksto provide context-specific help resourcesImplementation Details:
BaseEngineSpec.extract_errors()to checkCUSTOM_DATABASE_ERRORSconfig before falling back to default error handlingdatabase_namein context (SQL Lab, chart queries, database validation, connection testing)SupersetErrorclass to respectshow_issue_infoflag for conditional issue code inclusionDatabaseErrorMessagefrontend component to conditionally display issue codes and custom doc linksDesign Decisions:
custom_errors, giving administrators full controlcurrent_appis not available (e.g., tests, CLI)show_issue_infoflag is not presentBEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
TESTING INSTRUCTIONS
Manual Testing:
Configure Custom Errors:
superset_config.py:Restart Superset to load the new configuration
Test in SQL Lab:
SELECT * FROM non_existing_tableshow_issue_info: False)Test with
show_issue_info: True:"show_issue_info": TrueTest Regex Capture Groups:
(?P<table_name>.+)to capture the table nameTest in Other Contexts:
Automated Testing:
Run the unit tests:
docker compose exec superset pytest tests/unit_tests/db_engine_specs/test_custom_errors.py -vThe test suite covers:
show_issue_infoflag behavior (False, True, and default)custom_doc_linksinclusionADDITIONAL INFORMATION