fix(db_engine_specs): skip malformed third-party dialect entry points - #43110
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #43110 +/- ##
==========================================
- Coverage 66.65% 66.65% -0.01%
==========================================
Files 2874 2874
Lines 163784 163784
Branches 37798 37799 +1
==========================================
- Hits 109177 109165 -12
- Misses 52469 52479 +10
- Partials 2138 2140 +2
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
dosubot
Bot
added
the
data:connect
Code Review Agent Run #8c27e3Actionable Suggestions - 0Review Details
Bito Usage GuideCommands Type the following command in the pull request comment and save the comment.
Refer to the documentation for additional commands. Configuration This repository uses Documentation & Help AI Code Review powered by |
Code Review Agent Run #2f1433Actionable Suggestions - 0Review Details
Bito Usage GuideCommands Type the following command in the pull request comment and save the comment.
Refer to the documentation for additional commands. Configuration This repository uses Documentation & Help AI Code Review powered by |
get_available_engine_specs() reads `dialect.name` unguarded for every installed `sqlalchemy.dialects` entry point. A third-party entry point that loads successfully but does not resolve to a usable dialect -- e.g. a malformed `name = package:module` entry point that yields a module (which has no `.name`) -- raises AttributeError and aborts the whole enumeration. Because this runs in common_bootstrap_payload(), it 500s every page that builds the bootstrap (e.g. /welcome/), not just that one connector. Mirror the defensiveness of the native-dialect loop directly above: if the loaded object has no usable `.name`, skip it with a warning and continue, so a malformed driver degrades to "that connector is unavailable" instead of taking down the app. Signed-off-by: Amin Ghadersohi <amin.ghadersohi@gmail.com>
3cc7661 to
9b2bb71
Compare
![codeant-ai-for-open-source[bot]](https://avatars.githubusercontent.com/u/151810221?s=60&v=4){kind=small}
|
The flagged issue is correct. The current implementation only checks for the existence of a Here is a concise implementation to add this validation: from sqlalchemy.engine import Dialect
# ... inside the loop ...
backend = getattr(dialect, "name", None)
if not isinstance(backend, (str, bytes)) or not isinstance(dialect, Dialect):
logger.warning(
"Skipping SQLAlchemy dialect entry point %r: %r did not "
"resolve to a valid SQLAlchemy dialect",
ep.name,
ep.value,
)
continueThere are no other comments on this PR to address. superset/db_engine_specs/init.py |
Code Review Agent Run #41abd4Actionable Suggestions - 0Review Details
Bito Usage GuideCommands Type the following command in the pull request comment and save the comment.
Refer to the documentation for additional commands. Configuration This repository uses Documentation & Help AI Code Review powered by |
Code Review Agent Run #922717Actionable Suggestions - 0Additional Suggestions - 2
Review Details
Bito Usage GuideCommands Type the following command in the pull request comment and save the comment.
Refer to the documentation for additional commands. Configuration This repository uses Documentation & Help AI Code Review powered by |
|
Heya @aminghadersohi! Good catch on the crash, and the guard makes sense. One thing before I approve, though. I think we can drop the |
|
Thanks for catching this — addressed in |
|
Bito Automatic Review Skipped – PR Already Merged |
2 participants
SUMMARY
get_available_engine_specs()enumerates every installedsqlalchemy.dialectsentry point and readsdialect.nameunguarded:If a third-party entry point loads successfully but does not resolve to a usable dialect — e.g. a malformed
name = package:moduleentry point that yields a module (which has no.name) — theAttributeErroraborts the whole enumeration. Becauseget_available_engine_specs()runs insidecommon_bootstrap_payload(), this 500s every page that builds the bootstrap (e.g./welcome/), not just that one connector — the whole app becomes unusable.Real-world trigger:
sqlalchemy-exasol7.1.1+ registersexa = sqlalchemy_exasol:base, whichep.load()resolves to thebasemodule;module.name→AttributeError→ every page 500s under SQLAlchemy 2.0.The native-dialect loop directly above is already defensive (
issubclass(dialect, DefaultDialect),hasattr(dialect, "driver"), per-dialecttry/except). This makes the third-party loop equally defensive: if the loaded object has no usable.name, skip it with alogger.warningand continue. A malformed driver then degrades to "that one connector is unavailable" instead of "the app is down".TESTING INSTRUCTIONS
pytest tests/unit_tests/db_engine_specs/test_init.pyAdds
test_get_available_engine_specs_skips_malformed_dialect_entry_point: registers a bogussqlalchemy.dialectsentry point whoseload()returns a module (no.name) and assertsget_available_engine_specs()skips it (with a warning) instead of raising. The test raisesAttributeErroronmasterand passes with this change.ADDITIONAL INFORMATION