refactor: Migration of json utilities from core
SUMMARY
initial step of refactoring JSON utilities
- migration of all json utils from utils.core to utils.json
- refactored all referenced usage of those utils to the new json module
- added and fixed some tests
- removed unused code
- following https://github.com/apache/superset/pull/28486 more instances discovered and fixed at:
- Sqllab results api
- Charts data api
@mistercrunch - tests from last commit https://github.com/apache/superset/pull/28486 still covers for the encoding part.
Codecov Report
Attention: Patch coverage is 86.76471% with 18 lines in your changes are missing coverage. Please review.
Project coverage is 83.50%. Comparing base (
76d897e) to head (929fc12). Report is 144 commits behind head on master.
| Files | Patch % | Lines |
|---|---|---|
| superset/utils/json.py | 86.95% | 12 Missing :warning: |
| superset/views/chart/views.py | 33.33% | 2 Missing :warning: |
| superset/models/core.py | 66.66% | 1 Missing :warning: |
| superset/views/api.py | 50.00% | 1 Missing :warning: |
| superset/views/tags.py | 66.66% | 1 Missing :warning: |
| superset/viz.py | 66.66% | 1 Missing :warning: |
Additional details and impacted files
@@ Coverage Diff @@
## master #28522 +/- ##
===========================================
+ Coverage 60.48% 83.50% +23.01%
===========================================
Files 1931 522 -1409
Lines 76236 37485 -38751
Branches 8568 0 -8568
===========================================
- Hits 46114 31301 -14813
+ Misses 28017 6184 -21833
+ Partials 2105 0 -2105
| Flag | Coverage Δ | |
|---|---|---|
| hive | 49.11% <50.00%> (-0.06%) |
:arrow_down: |
| javascript | ? |
|
| mysql | 77.18% <83.82%> (?) |
|
| postgres | 77.29% <84.55%> (?) |
|
| presto | 53.66% <50.73%> (-0.15%) |
:arrow_down: |
| python | 83.50% <86.76%> (+20.01%) |
:arrow_up: |
| sqlite | 76.74% <84.55%> (?) |
|
| unit | 58.96% <65.44%> (+1.34%) |
:arrow_up: |
Flags with carried forward coverage won't be shown. Click here to find out more.
:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.
Starting to feel like we need a refactor of some kind here as this isn't DRY, maybe moving some things to a new superset/utils/json.py, taking all the json-related stuff in superset/utils/core.py, and moving all this recent utf-16 handling in there too.
I think I started this refactor before in a branch but never carried though. Let me push the branch for reference. Are you interested in taking this on?
Here's the branch https://github.com/apache/superset/compare/refactor_json?expand=1, looks like I lost the json.py file along the way.... For now the goal would be to move the json-related function from utils/core to utils/json, and refactor the recent utf-16 handling in there. Eventually I'd like to also make sure all usage of json.(load|dump)(s)() would go through this module and use the proper encoders.
definitely felt the same seeing things being repeated more than once while dealing with this non UTF-8 bytes. for starters lets try to move all of those recent UTF-16 handling to this module and then see how goes.
If you git grep json.dumps you'll see there's quite a bit to centralize, we don't have to do it all at once though.
@mistercrunch - i've tried to keep it as small as possible for this first phase but it ended up with 30 files changed after all 😏
next phase (as mentioned) needs to be the refactoring of any json.(load/s|dump/s) to use this module which looks like it's gonna be a huge refactoring ~250+ files at least. so probably it's better to split it up anyway.
@mistercrunch - with pleasure. since i got my head into this I have begun working on the subsequent stage of the refactoring. It is likely that I will submit another pull request regarding this matter in the near future.