fix(api,sql): use json_response in Api.query and log dialect fallback#40644
Conversation
Make Api.query return self.json_response(...) like its query_form_data and time_range siblings so the Content-Type header is set consistently instead of returning a raw JSON string. The payload and serializer behavior are unchanged (json_response uses the same json_int_dttm_ser default and ignore_nan). Also add a logger.warning when SQL parsing falls back from the base dialect to the MySQL dialect (triggered by backtick-quoted identifiers), so the fallback is observable. Parsing behavior is unchanged. Adds unit tests for both: the JSON content type of the Api.query response and the warning log emitted on the MySQL dialect fallback. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Code Review Agent Run #6fdee2Actionable 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 |
✅ Deploy Preview for superset-docs-preview ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #40644 +/- ##
==========================================
- Coverage 63.94% 63.94% -0.01%
==========================================
Files 2658 2658
Lines 143011 143012 +1
Branches 32866 32866
==========================================
Hits 91454 91454
- Misses 49994 49995 +1
Partials 1563 1563
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Pull request overview
Two small backend correctness/consistency fixes: ensure Api.query sets a JSON Content-Type header by using self.json_response, and add a warning log when the SQL parser silently falls back from the base to the MySQL dialect for backtick-containing scripts.
Changes:
superset/views/api.py: replace rawjson.dumps(...)return withself.json_response(payload_json)for consistentContent-Type.superset/sql/parse.py: emit alogger.warningwhen falling back to MySQL dialect on backtick-containing scripts.- Added unit tests for both behaviors.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| superset/views/api.py | Use json_response so Api.query sets application/json Content-Type. |
| superset/sql/parse.py | Log a warning on MySQL dialect fallback to make it observable. |
| tests/unit_tests/views/test_base.py | New test asserting Api.query returns application/json. |
| tests/unit_tests/sql/parse_tests.py | New test asserting the dialect fallback emits a warning log. |
SUMMARY
Two small, low-risk correctness/consistency fixes:
Consistent
Content-TypeinApi.query(superset/views/api.py): theApi.query()endpoint returned a rawjson.dumps(...)string, unlike its sibling handlersquery_form_dataandtime_range, which useself.json_response(...). Returning a bare string leaves the responseContent-Typedefaulting totext/htmlrather thanapplication/json. Switched it toreturn self.json_response(payload_json).json_responseuses the samejson_int_dttm_serdefault serializer andignore_nan=True, so the response body is equivalent — only theContent-Typeheader is now set consistently.Observable SQL dialect fallback (
superset/sql/parse.py): when parsing with the base dialect fails and the script contains backticks, the parser silently falls back to the MySQL dialect (to support backtick-quoted identifiers). Added alogger.warningon that branch so the fallback is observable. Parsing behavior is unchanged; only a log line is added (moduleloggeralready existed).BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
Not applicable (backend-only change).
TESTING INSTRUCTIONS
python -m pytest tests/unit_tests/views/test_base.py tests/unit_tests/sql/parse_tests.py -q(549 passed locally).test_api_query_returns_json_content_typeasserts theApi.queryresponse has anapplication/jsoncontent type.test_backtick_fallback_logs_warningasserts a warning is logged when the MySQL dialect fallback path is taken.ADDITIONAL INFORMATION
🤖 Generated with Claude Code