eschutho opened a new pull request, #44616:
URL: https://github.com/apache/superset/pull/44616
### SUMMARY
**Problem.** `BaseEngineSpec.get_extra_params` and
`BaseEngineSpec.update_params_from_encrypted_extra`
(`superset/db_engine_specs/base.py`) catch `json.JSONDecodeError` when
`database.extra` / `database.encrypted_extra` holds malformed JSON. They log it
and then bare-`raise`. `superset.utils.json` re-exports
`simplejson.JSONDecodeError`, which is a plain `ValueError` and not a
`SupersetException`, so the raw exception escapes untyped. Both methods run on
every engine creation (`Database._get_sqla_engine` → `get_extra()` /
`update_params_from_encrypted_extra()`). That covers every engine spec that
doesn't override them, plus snowflake, databricks, duckdb and trino, which call
`BaseEngineSpec.get_extra_params` directly.
The same class already handles this input safely elsewhere.
`mask_encrypted_extra` and `unmask_encrypted_extra` catch `(TypeError,
json.JSONDecodeError)` and don't let the raw exception out. These two methods
are the outliers.
**Fix.** Keep the log and raise
`SupersetGenericDBErrorException(message=str(ex)) from ex` (status 400,
`GENERIC_DB_ENGINE_ERROR`) instead of re-raising the raw exception. Only these
two methods change. A config that is actually malformed still fails, but with a
typed Superset error.
**Where the behavior changes.** Any caller that handles `SupersetException`
/ `SupersetErrorException` now returns a typed error, where it used to fall
through to a generic 500. For example, `GET /api/v1/database/<pk>/schemas/` and
`/catalogs/` have an `except SupersetException` branch, and
`Database.get_all_schema_names` passes the exception through
`get_dbapi_mapped_exception` unchanged. Those routes now return a 400 with the
decode message instead of FAB's `@safe` generic 500. The same applies to routes
behind `@handle_api_exception` and the SIP-40 app error handlers. Some routes,
such as `GET /api/v1/datasource/<type>/<id>/column/<col>/values/`, have no
`SupersetException` handler before `@safe`. They still return a generic 500,
but the exception reaching them is typed now.
Same effort and shape as #42401 (sibling-drift fix, several sites in one
file) and #44463 / #44502 (wrap the raw exception at the existing method
boundary).
### BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
N/A (backend only)
### TESTING INSTRUCTIONS
- `pytest tests/unit_tests/db_engine_specs/test_base.py` (100 passed)
- New tests `test_get_extra_params_malformed_json` and
`test_update_params_from_encrypted_extra_malformed_json` fail on pre-fix code
with a raw `simplejson.errors.JSONDecodeError` and pass with the fix.
- Manual: set a sqlite database's `extra` to `{not valid json` directly in
the metadata DB, then call `GET /api/v1/database/<pk>/schemas/`. You get a 400
with the decode message instead of a generic 500.
### ADDITIONAL INFORMATION
- [ ] Has associated issue:
- [ ] Required feature flags:
- [ ] Changes UI
- [ ] Includes DB Migration (follow approval process in
[SIP-59](https://github.com/apache/superset/issues/13351))
- [ ] Migration is atomic, supports rollback & is backwards-compatible
- [ ] Confirm DB migration upgrade and downgrade tested
- [ ] Runtime estimates and downtime expectations provided
- [ ] Introduces new feature or API
- [ ] Removes existing feature or API
🤖 Generated with [Claude Code](https://claude.com/claude-code)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
