-
Notifications
You must be signed in to change notification settings - Fork 14.2k
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
feat(db): Adding DB_SQLA_URI_VALIDATOR #27847
feat(db): Adding DB_SQLA_URI_VALIDATOR #27847
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #27847 +/- ##
==========================================
+ Coverage 67.41% 69.83% +2.42%
==========================================
Files 1920 1920
Lines 75242 75245 +3
Branches 8423 8423
==========================================
+ Hits 50724 52548 +1824
+ Misses 22457 20636 -1821
Partials 2061 2061
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
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.
2 thoughts:
- about naming
DB_SQLA_URI_VALIDATOR
might be a slightly better name as it validates a sqlalchemy URI more than an "engine" uri - I thought we could have a more generic
DB_VALIDATOR
instead or in complement to this so that someone could look at otherDatabaseConnection
attributes too, as opposed to the uri only. Password complexity, extra attributes or anything else.
Aren't all of the connection attributes captured in the parsed url? Also, this is intended to be invoked pre-connect, so we likely wouldn't want to perform validations on a "connection". |
I think the bigger exception is the way we do BigQuery for instance cramming many things into a blob, but unclear how common/uncommon it is. This probably a good starting point. Can add more hooks if/as needed. |
Yeah, in general there are two parts: engine = create_engine(uri, **params) Both are needed for a full connection, since in some cases the params are too big to build a URI (like BigQuery credentials). |
superset/config.py
Outdated
# if not <some condition>: | ||
# raise Exception("URI invalid") | ||
# | ||
DB_ENGINE_URI_VALIDATOR = None |
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.
nit: DB_ENGINE_URI_VALIDATOR: Callable[[URL], None] | None = None
@craig-rueda out of interest why is this surfacing as an issue now? Did we not gracefully handle SQLAlchemy URI errors in the past? |
(cherry picked from commit 8bdf457)
(cherry picked from commit 8bdf457)
SUMMARY
Quick PR that adds a new callback hook,
DB_SQLA_URI_VALIDATOR
which can be optionally set in Superset configs. When set, it should point to a callable that is invoked in order to validate db_engine_spec's URIs.Example: