fix(databricks): classify insufficient-permissions errors as 4xx#41945
fix(databricks): classify insufficient-permissions errors as 4xx#41945eschutho wants to merge 2 commits into
Conversation
Databricks catalog/schema permission errors (SQLSTATE 42501 / [INSUFFICIENT_PERMISSIONS]) were falling through to GENERIC_DB_ENGINE_ERROR and always returning HTTP 500, even though they're a customer-side permission misconfiguration rather than a server error. extract_errors now classifies these as CONNECTION_DATABASE_PERMISSIONS_ERROR at ErrorLevel.WARNING, and SynchronousSqlJsonExecutor derives the response status from the aggregate error level (all-WARNING -> 400) instead of always raising SupersetErrorsException with the default 500, mirroring the existing pattern in commands/report/execute.py. Co-Authored-By: Claude <noreply@anthropic.com>
dosubot
Bot
added
change:backend
Code Review Agent Run #c2d725Actionable 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 |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #41945 +/- ##
==========================================
+ Coverage 64.79% 64.80% +0.01%
==========================================
Files 2740 2740
Lines 153135 153133 -2
Branches 35127 35125 -2
==========================================
+ Hits 99221 99243 +22
+ Misses 52017 51989 -28
- Partials 1897 1901 +4
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:
|
…ions fix - Move the insufficient-permissions classification from DatabricksDynamicBaseEngineSpec up to the shared DatabricksBaseEngineSpec so DatabricksODBCEngineSpec (SQL Endpoint connections) benefits too, not just the native/Python-connector engines. This also removes a duplicated reimplementation of BaseEngineSpec's custom_errors loop. - Derive the SQL Lab error status from "any ERROR -> 500" instead of "all WARNING -> 400", so an all-INFO or INFO+WARNING error list isn't incorrectly treated as a server error. - Tighten the regex to tolerate irregular SQLSTATE spacing and add word boundaries. - Parameterize the Databricks regression test across the tag-only, SQLSTATE-only, and combined message forms, add ODBC coverage, and fix a missing generic type parameter (dict -> dict[str, Any]) that failed mypy's disallow_any_generics. - Add INFO-only and single-ERROR test cases for the executor status logic. Co-Authored-By: Claude <noreply@anthropic.com>
Code Review Agent Run #329a9cActionable 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 |
1 participant
SUMMARY
Databricks catalog/schema permission errors (e.g.
[INSUFFICIENT_PERMISSIONS] Insufficient privileges: User does not have USE CATALOG on Catalog 'foo'. SQLSTATE: 42501.) were classified asSupersetErrorType.GENERIC_DB_ENGINE_ERRORand always returned to the client as HTTP 500, even though this is a customer-side permission misconfiguration, not a server error.Two changes were needed to fix this end-to-end:
DatabricksDynamicBaseEngineSpec.extract_errors(superset/db_engine_specs/databricks.py) now detects theSQLSTATE: 42501/[INSUFFICIENT_PERMISSIONS]pattern and classifies it asSupersetErrorType.CONNECTION_DATABASE_PERMISSIONS_ERRORatErrorLevel.WARNING, instead of falling through to the generic catch-all atErrorLevel.ERROR.SynchronousSqlJsonExecutor.execute(superset/sqllab/sql_json_executer.py) previously raisedSupersetErrorsExceptionwith nostatus, which always inherits the default 500 regardless of the contained errors'level. It now derives the status from the aggregate error level (allWARNING-> 400, anyERROR-> 500), mirroring the existing pattern incommands/report/execute.py. This is a shared code path used by every DB engine's rich-error responses; today every engine'sextract_errorsonly emitsERRORlevel here, so this is a no-op for all engines except the new DatabricksWARNINGcase above.TESTING INSTRUCTIONS
pytest tests/unit_tests/db_engine_specs/test_databricks.py— includes a new regression test asserting the SQLSTATE 42501 message yieldsCONNECTION_DATABASE_PERMISSIONS_ERRORatErrorLevel.WARNING.pytest tests/unit_tests/sqllab/test_sql_json_executer.py— new test file covering an all-WARNINGerrors list resulting in HTTP 400, and anERROR/mixed-level list still resulting in HTTP 500.ADDITIONAL INFORMATION