You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@airflow.apache.org by GitBox <gi...@apache.org> on 2022/04/05 04:43:08 UTC
[GitHub] [airflow] karunpoudel opened a new pull request, #22744: double escape backslash in json
karunpoudel opened a new pull request, #22744:
URL: https://github.com/apache/airflow/pull/22744
closes: #22702
double escape backslash in json so that it is interpreted correctly by JS
<!--
Thank you for contributing! Please make sure that your code changes
are covered with tests. And in case of new features or big changes
remember to adjust the documentation.
Feel free to ping committers for the review!
In case of existing issue, reference it using one of the following:
closes: #ISSUE
related: #ISSUE
How to write a good git commit message:
http://chris.beams.io/posts/git-commit/
-->
---
**^ Add meaningful description above**
Read the **[Pull Request Guidelines](https://github.com/apache/airflow/blob/main/CONTRIBUTING.rst#pull-request-guidelines)** for more information.
In case of fundamental code change, Airflow Improvement Proposal ([AIP](https://cwiki.apache.org/confluence/display/AIRFLOW/Airflow+Improvements+Proposals)) is needed.
In case of a new dependency, check compliance with the [ASF 3rd Party License Policy](https://www.apache.org/legal/resolved.html#category-x).
In case of backwards incompatible changes please leave a note in [UPDATING.md](https://github.com/apache/airflow/blob/main/UPDATING.md).
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] potiuk commented on a diff in pull request #22744: double escape backslash in json
Posted by GitBox <gi...@apache.org>.
potiuk commented on code in PR #22744:
URL: https://github.com/apache/airflow/pull/22744#discussion_r842512099
##########
airflow/www/templates/airflow/gantt.html:
##########
@@ -79,6 +79,6 @@
<script src="{{ url_for_asset('gantt.js') }}"></script>
<script>
// Loading data via <meta> wasn't loading extra_links in a task
- const data = JSON.parse('{{ data|tojson }}');
+ const data = JSON.parse('{{ data|tojson|replace("\\", "\\\\") }}');
Review Comment:
While I see the problem, this always sound suspicious if you want to make such individual replaces.
Is \\ the only possible problem here? Aren't there ready to use filters we can use to sanitize the input "fully"?
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] boring-cyborg[bot] commented on pull request #22744: double escape backslash in json
Posted by GitBox <gi...@apache.org>.
boring-cyborg[bot] commented on PR #22744:
URL: https://github.com/apache/airflow/pull/22744#issuecomment-1088262794
Congratulations on your first Pull Request and welcome to the Apache Airflow community! If you have any issues or are unsure about any anything please check our Contribution Guide (https://github.com/apache/airflow/blob/main/CONTRIBUTING.rst)
Here are some useful points:
- Pay attention to the quality of your code (flake8, mypy and type annotations). Our [pre-commits]( https://github.com/apache/airflow/blob/main/STATIC_CODE_CHECKS.rst#prerequisites-for-pre-commit-hooks) will help you with that.
- In case of a new feature add useful documentation (in docstrings or in `docs/` directory). Adding a new operator? Check this short [guide](https://github.com/apache/airflow/blob/main/docs/apache-airflow/howto/custom-operator.rst) Consider adding an example DAG that shows how users should use it.
- Consider using [Breeze environment](https://github.com/apache/airflow/blob/main/BREEZE.rst) for testing locally, itโs a heavy docker but it ships with a working Airflow and a lot of integrations.
- Be patient and persistent. It might take some time to get a review or get the final approval from Committers.
- Please follow [ASF Code of Conduct](https://www.apache.org/foundation/policies/conduct) for all communication including (but not limited to) comments on Pull Requests, Mailing list and Slack.
- Be sure to read the [Airflow Coding style]( https://github.com/apache/airflow/blob/main/CONTRIBUTING.rst#coding-style-and-best-practices).
Apache Airflow is a community-driven project and together we are making it better ๐.
In case of doubts contact the developers at:
Mailing List: dev@airflow.apache.org
Slack: https://s.apache.org/airflow-slack
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] potiuk commented on pull request #22744: double escape backslash in json
Posted by GitBox <gi...@apache.org>.
potiuk commented on PR #22744:
URL: https://github.com/apache/airflow/pull/22744#issuecomment-1090771828
> Actually, it looks like we can get rid of JSON.parse(): https://github.com/apache/airflow/pull/22780
So my Javascript is not as rusty :)
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] karunpoudel commented on a diff in pull request #22744: double escape backslash in json
Posted by GitBox <gi...@apache.org>.
karunpoudel commented on code in PR #22744:
URL: https://github.com/apache/airflow/pull/22744#discussion_r842776448
##########
airflow/www/templates/airflow/gantt.html:
##########
@@ -79,6 +79,6 @@
<script src="{{ url_for_asset('gantt.js') }}"></script>
<script>
// Loading data via <meta> wasn't loading extra_links in a task
- const data = JSON.parse('{{ data|tojson }}');
+ const data = JSON.parse('{{ data|tojson|replace("\\", "\\\\") }}');
Review Comment:
I think \ is only problem here because every other character has already be escaped by \ , now, we need to escape the \ that is used to escape other character. There is [forceescape()](https://jinja.palletsprojects.com/en/3.1.x/templates/#jinja-filters.forceescape) filter available but it will escape doublequote more suitable for html rather than javacript string.
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] karunpoudel commented on pull request #22744: double escape backslash in json
Posted by GitBox <gi...@apache.org>.
karunpoudel commented on PR #22744:
URL: https://github.com/apache/airflow/pull/22744#issuecomment-1088726849
> The `tojson` filter should already be producing valid JSON. I wonder why it is not.
@ashb, the regular json string works if you are writing to a file or sending as html response where the backslash are interpreted as literal character. The problem is that the json string is part of your code, not the literal string data. There is two step of parsing going on client side: 1st, it is parse by javascript to convert it to string and then by JSON.parse().
Another way to fix this without manually replacing is by using python's `repr()` function. Once we register it filter, then we can use it as
`const data = JSON.parse({{ data | tojson | repr }});`
This might be cleaner way.
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] ashb commented on pull request #22744: double escape backslash in json
Posted by GitBox <gi...@apache.org>.
ashb commented on PR #22744:
URL: https://github.com/apache/airflow/pull/22744#issuecomment-1088738206
https://jinja.palletsprojects.com/en/3.0.x/templates/#jinja-filters.tojson
> The returned string is safe to render in HTML documents and `<script>` tags
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] uranusjr closed pull request #22744: double escape backslash in json
Posted by GitBox <gi...@apache.org>.
uranusjr closed pull request #22744: double escape backslash in json
URL: https://github.com/apache/airflow/pull/22744
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] uranusjr commented on a diff in pull request #22744: double escape backslash in json
Posted by GitBox <gi...@apache.org>.
uranusjr commented on code in PR #22744:
URL: https://github.com/apache/airflow/pull/22744#discussion_r843643607
##########
airflow/www/templates/airflow/gantt.html:
##########
@@ -79,6 +79,6 @@
<script src="{{ url_for_asset('gantt.js') }}"></script>
<script>
// Loading data via <meta> wasn't loading extra_links in a task
- const data = JSON.parse('{{ data|tojson }}');
+ const data = JSON.parse('{{ data|tojson|replace("\\", "\\\\") }}');
Review Comment:
Manual replace does not look right to me. We could likely fix this by putting the string in HTML instead and use `getElementById` or something to retrieve the value 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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] ashb commented on pull request #22744: double escape backslash in json
Posted by GitBox <gi...@apache.org>.
ashb commented on PR #22744:
URL: https://github.com/apache/airflow/pull/22744#issuecomment-1090066740
https://github.com/apache/airflow/pull/22744#pullrequestreview-931606010
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] potiuk commented on pull request #22744: double escape backslash in json
Posted by GitBox <gi...@apache.org>.
potiuk commented on PR #22744:
URL: https://github.com/apache/airflow/pull/22744#issuecomment-1089064311
Isn't here the right solution to not use JSON parse but simply do
```
const data = {{ data | tojson }}
```
?
I believe J in JSon originally stands for Javascript and JSON is first class citizen in javascript. I see no reaon why we should additonally parse 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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] uranusjr commented on pull request #22744: double escape backslash in json
Posted by GitBox <gi...@apache.org>.
uranusjr commented on PR #22744:
URL: https://github.com/apache/airflow/pull/22744#issuecomment-1089985973
Maybe put the string in HTML instead and use `getElementById` or something to retrieve the value in JavaScript?
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] karunpoudel commented on pull request #22744: double escape backslash in json
Posted by GitBox <gi...@apache.org>.
karunpoudel commented on PR #22744:
URL: https://github.com/apache/airflow/pull/22744#issuecomment-1090471736
This could be closed since #22780 got merged
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] bbovenzi commented on pull request #22744: double escape backslash in json
Posted by GitBox <gi...@apache.org>.
bbovenzi commented on PR #22744:
URL: https://github.com/apache/airflow/pull/22744#issuecomment-1090234197
> @bbovenzi In #14545 You change this from `data = {{ data |tojson }};` to `JSON.parse('{{ data|tojson }}')` -- can you remember if is there a reason why? This is the only place in the code base that we have the `JSON.parse` and `tojson` together.
Actually, it looks like we can get rid of `JSON.parse()`: https://github.com/apache/airflow/pull/22780
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org