fix(sqllab): wrap cost-estimate JSON parse to prevent raw JSONDecodeError leak - #43888
fix(sqllab): wrap cost-estimate JSON parse to prevent raw JSONDecodeError leak#43888eschutho wants to merge 1 commit into
Conversation
…rror leak QueryEstimationCommand.run() calls estimate_query_cost(), which for Presto/Trino parses the EXPLAIN (TYPE IO, FORMAT JSON) output via simplejson-backed superset.utils.json (allow_nan=False). When a table lacks computed statistics, the engine emits a literal NaN token, so json.loads() raises a raw simplejson JSONDecodeError that propagates uncaught through the command and the /api/v1/sqllab/estimate/ endpoint, surfacing to the user as an opaque 500 GENERIC_BACKEND_ERROR crash. Catch json.JSONDecodeError as a sibling of the existing SupersetTimeoutException handler and convert it to a typed SupersetErrorException (500, GENERIC_BACKEND_ERROR) -- a backend/driver response-parsing failure, not user input error -- mirroring the sibling TemplateError conversion in the same function (#42757). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Code Review Agent Run #af741dActionable 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✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #43888 +/- ##
=======================================
Coverage 79.45% 79.45%
=======================================
Files 2895 2895
Lines 168167 168170 +3
Branches 38995 38995
=======================================
+ Hits 133624 133627 +3
Misses 32044 32044
Partials 2499 2499
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:
|
1 participant
SUMMARY
Running Estimate cost in SQL Lab against a Presto/Trino table that lacks computed statistics currently crashes with an opaque
500 GENERIC_BACKEND_ERRORinstead of a clean, typed error.Root cause.
QueryEstimationCommand.run()(superset/commands/sql_lab/estimate.py) callsdb_engine_spec.estimate_query_cost(...). For Presto/Trino,PrestoBaseEngineSpec.estimate_statement_costrunsEXPLAIN (TYPE IO, FORMAT JSON) <statement>and doesjson.loads(cursor.fetchone()[0])using the simplejson-backedsuperset.utils.json(allow_nan=False). Trino/Presto represent unknown/uncollected cost statistics as a literalNaNtoken in that JSON (e.g.{"estimate": {"outputRowCount": NaN, ...}}), which happens for any table without anANALYZE.allow_nan=Falsethen makesjson.loadsraise a rawsimplejson.errors.JSONDecodeError.That exception is not caught anywhere in the call chain:
BaseEngineSpec.estimate_query_costcallsestimate_statement_costwith no guard;QueryEstimationCommand.run()only wraps the call intry/except SupersetTimeoutException;SqlLabRestApi.estimate_query_costhas notry/except.So it falls through to Flask's global
@app.errorhandler(Exception)and leaks to the user as an unhandled 500.FIX
Add a sibling
except json.JSONDecodeErrorblock to the existingtry/exceptaround theestimate_query_cost(...)call, converting it to a typedSupersetErrorException. This is a backend/driver response-parsing failure (missing table statistics on the remote engine), not a user-input error, so it is bucketed as500 GENERIC_BACKEND_ERROR(code 1011) rather than400 GENERIC_COMMAND_ERROR.This mirrors the pre-existing sibling conversion in the very same function — the raw
jinja2.TemplateErrorhandling added by #42757 (fix(sqllab): wrap process_template() in QueryEstimationCommand to prevent raw UndefinedError leak) — and continues the SQL Lab exception-cleanup series (#43883 / #43795 / #43772).The change is strictly additive: successful estimates are unchanged; only the previously-unhandled malformed/
NaNcost-estimate JSON now returns a clean typed 500.BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
N/A — backend error-handling change.
Before: opaque unhandled
500 GENERIC_BACKEND_ERRORfrom an escapedsimplejson.JSONDecodeError.After: typed
SupersetErrorException→500 GENERIC_BACKEND_ERRORwith message "Unable to parse the cost estimate returned by the database."TESTING INSTRUCTIONS
Automated (new regression test in
tests/unit_tests/commands/sql_lab/test_estimate.py):test_run_wraps_raw_jsondecodeerror_from_cost_estimationmocksestimate_query_costto raise the concrete simplejsonJSONDecodeError(viasuperset.utils.json.JSONDecodeError, which issimplejson.errors.JSONDecodeError— distinct from the stdlib class) and assertsrun()raisesSupersetErrorExceptionwith.status == 500and.error.error_type == GENERIC_BACKEND_ERROR. Verified failing on pre-fix code and passing after.Manual: in SQL Lab, point at a Presto/Trino database, select a table with no computed statistics, enter any
SELECT, and click Estimate cost. Previously an opaque 500; now a clean typed backend error.ADDITIONAL INFORMATION
🤖 Generated with Claude Code