superset icon indicating copy to clipboard operation
superset copied to clipboard

refactor: Migration of json utilities from core

Open eyalezer opened this issue 1 year ago • 7 comments

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

eyalezer avatar May 15 '24 21:05 eyalezer

@mistercrunch - tests from last commit https://github.com/apache/superset/pull/28486 still covers for the encoding part.

eyalezer avatar May 15 '24 22:05 eyalezer

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.

codecov[bot] avatar May 15 '24 22:05 codecov[bot]

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?

mistercrunch avatar May 15 '24 23:05 mistercrunch

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.

mistercrunch avatar May 15 '24 23:05 mistercrunch

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.

eyalezer avatar May 16 '24 01:05 eyalezer

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 avatar May 16 '24 06:05 mistercrunch

@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.

eyalezer avatar May 18 '24 04:05 eyalezer

@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.

eyalezer avatar May 21 '24 16:05 eyalezer