[sql_json] Ensuring the request body is JSON encoded

[john-bodley]

CATEGORY

Choose one

• Bug Fix
• Enhancement (new features, refinement)
• Refactor
• Add tests
• Build / Development Environment
• Documentation

SUMMARY

Somewhat of a yak-shaving PR as the intent was to fix an issue where Hive queries failed in SQL Lab if no schema was selected,

The error is coming from here in PyHive where it seems that the schema (database) is being defined via the "null" string as opposed to None.

The root cause of this is in the front-end the SupersetClient has passing the data as form data (using the multipart/form-data Content-Type header) which was then being processed in Flask via response.form. This meant everything was being encoded as strings, i.e., hence why we had logic like,

request.form.get("runAsync") == "true"

and why null was being encoded as "null" as opposed to None. The fix was to:

1. Ensure that the front-end correctly encodes the body as JSON (and sets the appropriate headers).
2. Use response.json to correctly process the response.
3. Remove the obsolete GET method to sql_json.
4. For testing use the somewhat misnamed json argument to Flask's test client.post(...) method which is actually a Python dictionary (ironically the data argument can be a JSON string) and removes the need to specify the Content-Type header (which is needed for response.json otherwise we would need to use response.get_json(force=True) which seems a little heavy handed).

TEST PLAN

CI.

ADDITIONAL INFORMATION

• Has associated issue:
• Changes UI
• Requires DB Migration.
• Confirm DB Migration upgrade and downgrade tested.
• Introduces new feature or API
• Removes existing feature or API

REVIEWERS

to: @etr2460 @graceguo-supercat @serenajiang @williaster
cc: @mistercrunch @villebro

[graceguo-supercat]

I saw this stringify: false setting is from PR #5896. @williaster is there a special reason to turn off stringify?

if use stringify, do we still need parseMethod: 'text'?

[etr2460]

don't we really want stringify: false? I was under the impression that prevented us from sending up "null" and "false" versus null and false

And parseMethod determines how we parse the response, not the request

[graceguo-supercat]

you will see we use post method in many, many places, only in sql lab added stringify: false. I assume there must be reason to do extra...

[etr2460]

This is fine as it is, since we're setting body instead of postPayload, the stringify param is no longer used

[etr2460]

don't we really want stringify: false? I was under the impression that prevented us from sending up "null" and "false" versus null and false

And parseMethod determines how we parse the response, not the request

[dpgaspar]

#8163 👀

[etr2460]

lgtm, thanks for the fix!

[etr2460]

This is fine as it is, since we're setting body instead of postPayload, the stringify param is no longer used

[codecov-io]

Codecov Report

Merging #8256 into master will not change coverage.
The diff coverage is 100%.

@@           Coverage Diff           @@
##           master    #8256   +/-   ##
=======================================
  Coverage   65.66%   65.66%           
=======================================
  Files         481      481           
  Lines       23365    23365           
  Branches     2573     2573           
=======================================
  Hits        15342    15342           
  Misses       7885     7885           
  Partials      138      138

Impacted Files	Coverage Δ
superset/assets/src/SqlLab/actions/sqlLab.js	61.13% <ø> (ø)	⬆️
superset/views/core.py	70.06% <100%> (ø)	⬆️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 8bc5cd7...bc4150f. Read the comment docs.

[maltoze]

#13315