Re: [PR] refactor: Unify all json.(loads|dumps) usage to utils.json [superset]
geido commented on PR #28702: URL: https://github.com/apache/superset/pull/28702#issuecomment-2137312952 Hey @eyalezer this is great. If you haven't, feel free to join the [Apache Superset](https://join.slack.com/t/apache-superset/shared_invite/zt-2be0drwz8-bxPfkdz28ozzk1Iox29ufg) Slack. I'd be happy to help if you wish to contribute more to the project! -- 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. To unsubscribe, e-mail: notifications-unsubscr...@superset.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org - To unsubscribe, e-mail: notifications-unsubscr...@superset.apache.org For additional commands, e-mail: notifications-h...@superset.apache.org
Re: [PR] refactor: Unify all json.(loads|dumps) usage to utils.json [superset]
eyalezer commented on PR #28702: URL: https://github.com/apache/superset/pull/28702#issuecomment-2136443121 It's interesting that you brought it up. After I finished refactoring all the json.(loads|dumps) to utilize the json module, one of the first things I did was to check if ujson actually provides any significant performance enhancements to superset. it seems to be functioning fine, but I must admit that I haven't thoroughly tested it to accurately measure the extent of its performance improvements. Nevertheless, if anyone is interested in giving it a try, I still have the branch available here: https://github.com/eyalezer/superset/tree/ujson. -- 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. To unsubscribe, e-mail: notifications-unsubscr...@superset.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org - To unsubscribe, e-mail: notifications-unsubscr...@superset.apache.org For additional commands, e-mail: notifications-h...@superset.apache.org
Re: [PR] refactor: Unify all json.(loads|dumps) usage to utils.json [superset]
mistercrunch merged PR #28702: URL: https://github.com/apache/superset/pull/28702 -- 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. To unsubscribe, e-mail: notifications-unsubscr...@superset.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org - To unsubscribe, e-mail: notifications-unsubscr...@superset.apache.org For additional commands, e-mail: notifications-h...@superset.apache.org
Re: [PR] refactor: Unify all json.(loads|dumps) usage to utils.json [superset]
mistercrunch commented on PR #28702: URL: https://github.com/apache/superset/pull/28702#issuecomment-2136127557 Amazing, this is a massive refactor that should make everything json-related much more manageable. Interestingly python's standard lib json IS simplejson (see here https://stackoverflow.com/questions/712791/what-are-the-differences-between-json-and-simplejson-python-modules), but simplejson is typically ahead. Also having json all in one place allows us to consider things like https://pypi.org/project/ujson/ and do things like what triggered this refactor (improve utf-8 support + error handling) centrally. -- 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. To unsubscribe, e-mail: notifications-unsubscr...@superset.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org - To unsubscribe, e-mail: notifications-unsubscr...@superset.apache.org For additional commands, e-mail: notifications-h...@superset.apache.org
Re: [PR] refactor: Unify all json.(loads|dumps) usage to utils.json [superset]
eyalezer commented on PR #28702: URL: https://github.com/apache/superset/pull/28702#issuecomment-2136005000 @mistercrunch - Rebased before it's too late @betodealmeida - Thanks for the reference - Added another commit with the pylint rule to lint any "import simple/json" - tested and working as expected -- 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. To unsubscribe, e-mail: notifications-unsubscr...@superset.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org - To unsubscribe, e-mail: notifications-unsubscr...@superset.apache.org For additional commands, e-mail: notifications-h...@superset.apache.org
Re: [PR] refactor: Unify all json.(loads|dumps) usage to utils.json [superset]
eyalezer commented on PR #28702: URL: https://github.com/apache/superset/pull/28702#issuecomment-2135764361 @betodealmeida - awesome, so it's even easier than i thought... i'll look into it now and test it -- 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. To unsubscribe, e-mail: notifications-unsubscr...@superset.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org - To unsubscribe, e-mail: notifications-unsubscr...@superset.apache.org For additional commands, e-mail: notifications-h...@superset.apache.org
Re: [PR] refactor: Unify all json.(loads|dumps) usage to utils.json [superset]
betodealmeida commented on PR #28702: URL: https://github.com/apache/superset/pull/28702#issuecomment-2135758703 @eyalezer I have this PR out, it's a pylint rule: https://github.com/apache/superset/pull/26803 We could do something similar for `json`. -- 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. To unsubscribe, e-mail: notifications-unsubscr...@superset.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org - To unsubscribe, e-mail: notifications-unsubscr...@superset.apache.org For additional commands, e-mail: notifications-h...@superset.apache.org
Re: [PR] refactor: Unify all json.(loads|dumps) usage to utils.json [superset]
eyalezer commented on PR #28702: URL: https://github.com/apache/superset/pull/28702#issuecomment-2135751213 i'll rebase it quickly so it won't catch up more conflicts... regarding the linter, it's a great idea and it looks like it should be plausible by adding a custom mypy plugin for example... but there's more research needed to be done here. -- 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. To unsubscribe, e-mail: notifications-unsubscr...@superset.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org - To unsubscribe, e-mail: notifications-unsubscr...@superset.apache.org For additional commands, e-mail: notifications-h...@superset.apache.org
Re: [PR] refactor: Unify all json.(loads|dumps) usage to utils.json [superset]
mistercrunch commented on PR #28702: URL: https://github.com/apache/superset/pull/28702#issuecomment-2135710226 OMG glad to see this, happy to help fast-merge this since it'll conflict with everything else otherwise. Oh as a follow up, or maybe something we may want to bundle here -> @betodealmeida mentioned that there's a fairly easy way for us to add a linting rule that prevents people from doing simple `import json`, and force going through the wrappers in `superset/utils/json.py` -- 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. To unsubscribe, e-mail: notifications-unsubscr...@superset.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org - To unsubscribe, e-mail: notifications-unsubscr...@superset.apache.org For additional commands, e-mail: notifications-h...@superset.apache.org
Re: [PR] refactor: Unify all json.(loads|dumps) usage to utils.json [superset]
eyalezer commented on PR #28702: URL: https://github.com/apache/superset/pull/28702#issuecomment-2130381794 @mistercrunch - here's the second part of the refactor, as expected this is a huge PR: 232 files changed but at least now all json usage is unified to use the utils.json -- 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. To unsubscribe, e-mail: notifications-unsubscr...@superset.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org - To unsubscribe, e-mail: notifications-unsubscr...@superset.apache.org For additional commands, e-mail: notifications-h...@superset.apache.org
Re: [PR] refactor: Unify all json.(loads|dumps) usage to utils.json [superset]
codecov[bot] commented on PR #28702: URL: https://github.com/apache/superset/pull/28702#issuecomment-2129937137 ## [Codecov](https://app.codecov.io/gh/apache/superset/pull/28702?dropdown=coverage=pr=h1_medium=referral_source=github_content=comment_campaign=pr+comments_term=apache) Report Attention: Patch coverage is `69.86301%` with `44 lines` in your changes are missing coverage. Please review. > Project coverage is 64.54%. Comparing base [(`76d897e`)](https://app.codecov.io/gh/apache/superset/commit/76d897eaa2f9e137102bc194c2e3109c29d0348f?dropdown=coverage=desc_medium=referral_source=github_content=comment_campaign=pr+comments_term=apache) to head [(`68b6267`)](https://app.codecov.io/gh/apache/superset/commit/68b626739b25220aa8c8b1b636947e352da20d87?dropdown=coverage=desc_medium=referral_source=github_content=comment_campaign=pr+comments_term=apache). > Report is 176 commits behind head on master. | [Files](https://app.codecov.io/gh/apache/superset/pull/28702?dropdown=coverage=pr=tree_medium=referral_source=github_content=comment_campaign=pr+comments_term=apache) | Patch % | Lines | |---|---|---| | [superset/commands/dataset/export.py](https://app.codecov.io/gh/apache/superset/pull/28702?src=pr=tree=superset%2Fcommands%2Fdataset%2Fexport.py_medium=referral_source=github_content=comment_campaign=pr+comments_term=apache#diff-c3VwZXJzZXQvY29tbWFuZHMvZGF0YXNldC9leHBvcnQucHk=) | 25.00% | [3 Missing :warning: ](https://app.codecov.io/gh/apache/superset/pull/28702?src=pr=tree_medium=referral_source=github_content=comment_campaign=pr+comments_term=apache) | | [superset/dashboards/schemas.py](https://app.codecov.io/gh/apache/superset/pull/28702?src=pr=tree=superset%2Fdashboards%2Fschemas.py_medium=referral_source=github_content=comment_campaign=pr+comments_term=apache#diff-c3VwZXJzZXQvZGFzaGJvYXJkcy9zY2hlbWFzLnB5) | 25.00% | [3 Missing :warning: ](https://app.codecov.io/gh/apache/superset/pull/28702?src=pr=tree_medium=referral_source=github_content=comment_campaign=pr+comments_term=apache) | | [superset/commands/dashboard/export.py](https://app.codecov.io/gh/apache/superset/pull/28702?src=pr=tree=superset%2Fcommands%2Fdashboard%2Fexport.py_medium=referral_source=github_content=comment_campaign=pr+comments_term=apache#diff-c3VwZXJzZXQvY29tbWFuZHMvZGFzaGJvYXJkL2V4cG9ydC5weQ==) | 33.33% | [2 Missing :warning: ](https://app.codecov.io/gh/apache/superset/pull/28702?src=pr=tree_medium=referral_source=github_content=comment_campaign=pr+comments_term=apache) | | [superset/models/core.py](https://app.codecov.io/gh/apache/superset/pull/28702?src=pr=tree=superset%2Fmodels%2Fcore.py_medium=referral_source=github_content=comment_campaign=pr+comments_term=apache#diff-c3VwZXJzZXQvbW9kZWxzL2NvcmUucHk=) | 33.33% | [2 Missing :warning: ](https://app.codecov.io/gh/apache/superset/pull/28702?src=pr=tree_medium=referral_source=github_content=comment_campaign=pr+comments_term=apache) | | [superset/sql\_lab.py](https://app.codecov.io/gh/apache/superset/pull/28702?src=pr=tree=superset%2Fsql_lab.py_medium=referral_source=github_content=comment_campaign=pr+comments_term=apache#diff-c3VwZXJzZXQvc3FsX2xhYi5weQ==) | 33.33% | [2 Missing :warning: ](https://app.codecov.io/gh/apache/superset/pull/28702?src=pr=tree_medium=referral_source=github_content=comment_campaign=pr+comments_term=apache) | | [superset/utils/schema.py](https://app.codecov.io/gh/apache/superset/pull/28702?src=pr=tree=superset%2Futils%2Fschema.py_medium=referral_source=github_content=comment_campaign=pr+comments_term=apache#diff-c3VwZXJzZXQvdXRpbHMvc2NoZW1hLnB5) | 33.33% | [2 Missing :warning: ](https://app.codecov.io/gh/apache/superset/pull/28702?src=pr=tree_medium=referral_source=github_content=comment_campaign=pr+comments_term=apache) | | [superset/views/chart/views.py](https://app.codecov.io/gh/apache/superset/pull/28702?src=pr=tree=superset%2Fviews%2Fchart%2Fviews.py_medium=referral_source=github_content=comment_campaign=pr+comments_term=apache#diff-c3VwZXJzZXQvdmlld3MvY2hhcnQvdmlld3MucHk=) | 33.33% | [2 Missing :warning: ](https://app.codecov.io/gh/apache/superset/pull/28702?src=pr=tree_medium=referral_source=github_content=comment_campaign=pr+comments_term=apache) | | [superset/viz.py](https://app.codecov.io/gh/apache/superset/pull/28702?src=pr=tree=superset%2Fviz.py_medium=referral_source=github_content=comment_campaign=pr+comments_term=apache#diff-c3VwZXJzZXQvdml6LnB5) | 33.33% | [2 Missing :warning: ](https://app.codecov.io/gh/apache/superset/pull/28702?src=pr=tree_medium=referral_source=github_content=comment_campaign=pr+comments_term=apache) | | [superset/charts/data/api.py](https://app.codecov.io/gh/apache/superset/pull/28702?src=pr=tree=superset%2Fcharts%2Fdata%2Fapi.py_medium=referral_source=github_content=comment_campaign=pr+comments_term=apache#diff-c3VwZXJzZXQvY2hhcnRzL2RhdGEvYXBpLnB5) | 75.00% | [1 Missing :warning:
Re: [PR] refactor: Unify all json.(loads|dumps) usage to utils.json [superset]
github-advanced-security[bot] commented on code in PR #28702:
URL: https://github.com/apache/superset/pull/28702#discussion_r1613727457
##
superset/views/base.py:
##
@@ -153,9 +153,7 @@
payload = payload or {"error": f"{msg}"}
return Response(
-json_utils.dumps(
-payload, default=json_utils.json_iso_dttm_ser, ignore_nan=True
-),
+json.dumps(payload, default=json.json_iso_dttm_ser, ignore_nan=True),
Review Comment:
## Information exposure through an exception
[Stack trace information](1) flows to this location and may be exposed to an
external user.
[Stack trace information](2) flows to this location and may be exposed to an
external user.
[Stack trace information](3) flows to this location and may be exposed to an
external user.
[Stack trace information](4) flows to this location and may be exposed to an
external user.
[Stack trace information](5) flows to this location and may be exposed to an
external user.
[Stack trace information](6) flows to this location and may be exposed to an
external user.
[Stack trace information](7) flows to this location and may be exposed to an
external user.
[Stack trace information](8) flows to this location and may be exposed to an
external user.
[Stack trace information](9) flows to this location and may be exposed to an
external user.
[Stack trace information](10) flows to this location and may be exposed to
an external user.
[Stack trace information](11) flows to this location and may be exposed to
an external user.
[Stack trace information](12) flows to this location and may be exposed to
an external user.
[Show more
details](https://github.com/apache/superset/security/code-scanning/1224)
--
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.
To unsubscribe, e-mail: notifications-unsubscr...@superset.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
-
To unsubscribe, e-mail: notifications-unsubscr...@superset.apache.org
For additional commands, e-mail: notifications-h...@superset.apache.org
[PR] refactor: Unify all json.(loads|dumps) usage to utils.json [superset]
eyalezer opened a new pull request, #28702:
URL: https://github.com/apache/superset/pull/28702
### SUMMARY
Second phase of the json migration to use the new utils.json module
After completing the initial phase of creating the utils.json module as
mentioned in the following link: https://github.com/apache/superset/pull/28522,
we are now moving on to the second phase. This phase involves consolidating all
json usage and transitioning to the utilization of the newly created module.
During this phase:
- Refactored all instances where json was being used and updated the
references to utilize the json utils module.
- Made necessary additions and fixes to the tests to ensure their
compatibility with the changes made.
--
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.
To unsubscribe, e-mail: notifications-unsubscr...@superset.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
-
To unsubscribe, e-mail: notifications-unsubscr...@superset.apache.org
For additional commands, e-mail: notifications-h...@superset.apache.org
