Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix tree view if config contains " #9250

Merged
merged 1 commit into from
Jun 15, 2020
Merged

Fix tree view if config contains " #9250

merged 1 commit into from
Jun 15, 2020

Conversation

Khrol
Copy link
Contributor

@Khrol Khrol commented Jun 12, 2020

#9251

If you run DAG with {"\"": ""} configuration tree view will be broken:

tree:1 Uncaught SyntaxError: Unexpected string in JSON at position 806
    at JSON.parse (<anonymous>)
    at tree?dag_id=hightlight_test&num_runs=25:1190

JSON.parse is given incorrectly escaped json string.


Make sure to mark the boxes below before creating PR: [x]

  • Description above provides context of the change
  • Unit tests coverage for changes (not needed for documentation changes). Not sure for this specific case. Are there anything?
  • Target Github ISSUE in description if exists
  • Commits follow "How to write a good git commit message"
  • Relevant documentation is updated including usage instructions.
  • I will engage committers as explained in Contribution Workflow Example.

In case of fundamental code change, Airflow Improvement Proposal (AIP) is needed.
In case of a new dependency, check compliance with the ASF 3rd Party License Policy.
In case of backwards incompatible changes please leave a note in UPDATING.md.
Read the Pull Request Guidelines for more information.

If you run DAG with `{"\"": ""}` configuration tree view will be broken:
```
tree:1 Uncaught SyntaxError: Unexpected string in JSON at position 806
    at JSON.parse (<anonymous>)
    at tree?dag_id=hightlight_test&num_runs=25:1190
```

JSON.parse is given incorrectly escaped json string.
@boring-cyborg boring-cyborg bot added the area:webserver Webserver related Issues label Jun 12, 2020
@Khrol Khrol marked this pull request as ready for review June 12, 2020 13:38
@mik-laj
Copy link
Member

mik-laj commented Jun 12, 2020

Have you checked whether the problem also occurs elsewhere? Similar problems like to be repeated.

@Khrol
Copy link
Contributor Author

Khrol commented Jun 12, 2020

@mik-laj I've checked the code by JSON.parse calls. Nothing similar.

Copy link
Member

@ashb ashb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We're holding off merging anything until we've fixed tests though. Should be done soon

@ashb
Copy link
Member

ashb commented Jun 12, 2020

Yeah, this used to be just var data = {{ data | tojson }} but for large dags (i.e. AirB'n'B scale) this was a slow down on rendering the web page.

@ashb ashb added this to the Airflow 1.10.11 milestone Jun 12, 2020
@ashb ashb merged commit a8cd23c into apache:master Jun 15, 2020
@Khrol Khrol deleted the tree_view_fix branch June 15, 2020 11:00
@mik-laj
Copy link
Member

mik-laj commented Jun 15, 2020

It seems that this change has broken the tests on the master branch.
Last valid build on master:
https://github.com/apache/airflow/runs/772104379
Next finished build on master:
https://github.com/apache/airflow/runs/772331104

@Khrol
Copy link
Contributor Author

Khrol commented Jun 15, 2020

@mik-laj tests were green on PR... Do you want me to fix tests or you can do it?

@mik-laj
Copy link
Member

mik-laj commented Jun 15, 2020

It was green, but it was this build that ended three days ago. I am already working on it.

@kaxil
Copy link
Member

kaxil commented Jun 15, 2020

The problem with the CI in this build was that it only tested the static tests. As no ".py" file was changed, CI skipped rest of the unit test. However on Master we run all the tests, hence Master failed with below error:

tests/www/test_views.py .............................F

_________ TestAirflowBaseViews.test_escape_in_tree_view_0_hello_world __________

a = (<tests.www.test_views.TestAirflowBaseViews testMethod=test_escape_in_tree_view_0_hello_world>,)

    @wraps(func)
    def standalone_func(*a):
>       return func(*(a + p.args), **p.kwargs)

/usr/local/lib/python3.6/site-packages/parameterized/parameterized.py:530: 
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ 
tests/www/test_views.py:646: in test_escape_in_tree_view
    self.check_content_in_response(f'"conf":{{"abc":"{seralized_test_str}"}}', resp)
tests/www/test_views.py:177: in check_content_in_response
    self.assertIn(text, resp_html)
tests/www/test_views.py F

_________ TestAirflowBaseViews.test_escape_in_tree_view_1_hello_world __________

a = (<tests.www.test_views.TestAirflowBaseViews testMethod=test_escape_in_tree_view_1_hello_world>,)

    @wraps(func)
    def standalone_func(*a):
>       return func(*(a + p.args), **p.kwargs)

/usr/local/lib/python3.6/site-packages/parameterized/parameterized.py:530: 
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ 
tests/www/test_views.py:646: in test_escape_in_tree_view
    self.check_content_in_response(f'"conf":{{"abc":"{seralized_test_str}"}}', resp)
tests/www/test_views.py:177: in check_content_in_response
    self.assertIn(text, resp_html)
E   AssertionError: '"conf":{"abc":"hello\\\\u0027world"}' not found in '\n\n\n\n\n\n\n\n\n\n 

@mik-laj
Copy link
Member

mik-laj commented Jun 15, 2020

I finished change: Here is PR: #9307

@mik-laj mik-laj mentioned this pull request Jun 15, 2020
6 tasks
mik-laj added a commit that referenced this pull request Jun 15, 2020
kaxil pushed a commit that referenced this pull request Jun 23, 2020
If you run DAG with `{"\"": ""}` configuration tree view will be broken:
```
tree:1 Uncaught SyntaxError: Unexpected string in JSON at position 806
    at JSON.parse (<anonymous>)
    at tree?dag_id=hightlight_test&num_runs=25:1190
```

JSON.parse is given incorrectly escaped json string.

(cherry-picked from a8cd23c)
kaxil pushed a commit that referenced this pull request Jun 23, 2020
(cherry-picked from 2c18a3f)
kaxil added a commit that referenced this pull request Jun 23, 2020
kaxil pushed a commit to kaxil/airflow that referenced this pull request Jun 27, 2020
If you run DAG with `{"\"": ""}` configuration tree view will be broken:
```
tree:1 Uncaught SyntaxError: Unexpected string in JSON at position 806
    at JSON.parse (<anonymous>)
    at tree?dag_id=hightlight_test&num_runs=25:1190
```

JSON.parse is given incorrectly escaped json string.
kaxil pushed a commit to kaxil/airflow that referenced this pull request Jun 27, 2020
potiuk pushed a commit that referenced this pull request Jun 29, 2020
If you run DAG with `{"\"": ""}` configuration tree view will be broken:
```
tree:1 Uncaught SyntaxError: Unexpected string in JSON at position 806
    at JSON.parse (<anonymous>)
    at tree?dag_id=hightlight_test&num_runs=25:1190
```

JSON.parse is given incorrectly escaped json string.

(cherry-picked from a8cd23c)
potiuk pushed a commit that referenced this pull request Jun 29, 2020
(cherry-picked from 2c18a3f)
@kaxil kaxil added the type:bug-fix Changelog: Bug Fixes label Jul 1, 2020
kaxil pushed a commit that referenced this pull request Jul 1, 2020
If you run DAG with `{"\"": ""}` configuration tree view will be broken:
```
tree:1 Uncaught SyntaxError: Unexpected string in JSON at position 806
    at JSON.parse (<anonymous>)
    at tree?dag_id=hightlight_test&num_runs=25:1190
```

JSON.parse is given incorrectly escaped json string.

(cherry-picked from a8cd23c)
kaxil pushed a commit that referenced this pull request Jul 1, 2020
(cherry-picked from 2c18a3f)
cfei18 pushed a commit to cfei18/incubator-airflow that referenced this pull request Mar 5, 2021
If you run DAG with `{"\"": ""}` configuration tree view will be broken:
```
tree:1 Uncaught SyntaxError: Unexpected string in JSON at position 806
    at JSON.parse (<anonymous>)
    at tree?dag_id=hightlight_test&num_runs=25:1190
```

JSON.parse is given incorrectly escaped json string.

(cherry-picked from a8cd23c)
cfei18 pushed a commit to cfei18/incubator-airflow that referenced this pull request Mar 5, 2021
(cherry-picked from 2c18a3f)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:webserver Webserver related Issues type:bug-fix Changelog: Bug Fixes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants