[GitHub] [incubator-superset] john-bodley opened a new pull request #8256: [sql_json] Ensuring the request body is JSON encoded

[GitBox <gi...@apache.org>]

john-bodley opened a new pull request #8256: [sql_json] Ensuring the request body is JSON encoded
URL: https://github.com/apache/incubator-superset/pull/8256
 
 
   ### CATEGORY
   
   Choose one
   
   - [x] 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, 
   
   ![Screen Shot 2019-09-18 at 3 54 58 PM](https://user-images.githubusercontent.com/4567245/65192859-933ad280-da2d-11e9-9094-45f487dbd7f3.png)
   
   The error is coming from [here](https://github.com/dropbox/PyHive/blob/master/pyhive/hive.py#L205) 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`](https://flask.palletsprojects.com/en/1.1.x/api/#flask.Request.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`](https://flask.palletsprojects.com/en/1.1.x/api/#flask.Request.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
   <!--- Check any relevant boxes with "x" -->
   <!--- HINT: Include "Fixes #nnn" if you are fixing an existing issue -->
   - [ ] 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 @williaster 
   cc: @mistercrunch @villebro 

----------------------------------------------------------------
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.
 
For queries about this service, please contact Infrastructure at:
users@infra.apache.org


With regards,
Apache Git Services

---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org