eschutho opened a new pull request, #44826:
URL: https://github.com/apache/superset/pull/44826
### SUMMARY
`SqlLabRestApi.format_sql` accepts `template_params` as a plain JSON string.
After `json.loads`, the only caught exceptions are `json.JSONDecodeError`
(logged as warning, templating skipped) and `TemplateError` (returned as 400).
If the JSON is valid but not an object — e.g. `[1]`, `5`, `"abc"`, `true`,
`null` — the parsed value passes the truthiness check and flows into
`template_processor.process_template(sql, **template_params)`, which raises a
raw `TypeError: argument after ** must be a mapping`. Nothing in the call chain
catches this, so it leaks as an unhandled 500 with a stack trace.
The fix mirrors the existing invalid-JSON handling: after `json.loads`, if
the result is not a `dict`, log a warning and skip Jinja processing. This is
consistent with how the sibling `TemplateError` catch was added in #42917.
**Skip vs 400**: I chose skip-and-format (200) over returning 400 because
that's what the existing invalid-JSON path does — the user still gets their
formatted SQL, just without template substitution. A 400 would also be
defensible but would be a behavior change relative to the existing precedent.
### BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
Not applicable (API-only change, no UI).
### TESTING INSTRUCTIONS
1. Send a POST to `/api/v1/sqllab/format_sql/` with `template_params` set to
a non-dict JSON value:
```json
{
"sql": "SELECT 1",
"database_id": 1,
"template_params": "[1, 2]"
}
```
2. **Before**: 500 Internal Server Error with `TypeError` stack trace.
3. **After**: 200 with the formatted SQL. A warning is logged.
4. Run the new parametrized unit test:
```bash
pytest
tests/unit_tests/sqllab/api_test.py::test_format_sql_non_dict_template_params -v
```
### 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
Related prior art: #42917 (added `TemplateError` handling in the same try
block).
🤖 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]
