Skip to content

[SPARK-31534][WEBUI][3.0] Text for tooltip should be escaped - #28359

Closed
sarutak wants to merge 1 commit into
apache:branch-3.0from
sarutak:fix-tooltip-branch-3.0
Closed

[SPARK-31534][WEBUI][3.0] Text for tooltip should be escaped#28359
sarutak wants to merge 1 commit into
apache:branch-3.0from
sarutak:fix-tooltip-branch-3.0

Conversation

@sarutak

@sarutaksarutak commented Apr 27, 2020

Copy link
Copy Markdown
Member

What changes were proposed in this pull request?

This PR backports #28317 which escapes text for tooltip for DAG Viz and Timeline View.

Why are the changes needed?

This is a bug.
Normally, DAG Viz and Timeline View show tooltip like as follows.

dag-viz-tooltip

timeline-tooltip

They contain a callsite properly.
However, if a callsite contains characters which should be escaped for HTML without escaping , the corresponding tooltips wouldn't show the callsite and its following text properly.
dag-viz-tooltip-before-fixed
timeline-tooltip-before-fixed

The reason of this issue is that the source texts of the tooltip texts are not escaped.

Does this PR introduce any user-facing change?

No.

How was this patch tested?

I tested manually.
First, I ran a job sc.parallelize(1 to 10).collect in Spark Shell then, visited AllJobsPage and JobPage and confirmed tooltip texts.
timeline-tooltip-fixed-3 0
dag-tooltip-fixed-3 0
And I confirmed that the appearance of the label of DAG-viz in StagePage is not changed.
stage-page-dag

I also added a testcase.
With this testcase, an error message related to JavaScript is shown.

TypeError: Cannot call method "indexOf" of undefined (http://192.168.1.209:59376/static/spark-dag-viz.js#378)

This is thrown from interpretLineBreak in spark-dag-viz.js.
HtmlUnit seems not to support innerHTML for text content (I tried replacing it with textContent and the error message is not shown).
But DOMs which is needed by the testcase added is already rendered before interpretLineBreak is called. So the testcase successfully passes.

@sarutak
[SPARK-31534][WEBUI] Text for tooltip should be escaped
90d3dbf
### What changes were proposed in this pull request?
This PR escapes text for tooltip for DAG Viz and Timeline View.
### Why are the changes needed?
This is a bug.
Normally, DAG Viz and Timeline View show tooltip like as follows.
<img width="278" alt="dag-viz-tooltip" src="https://user-images.githubusercontent.com/4736016/80127481-5a6c6880-85cf-11ea-8daf-cfd59aa3ba09.png">
<img width="477" alt="timeline-tooltip" src="https://user-images.githubusercontent.com/4736016/80127500-60624980-85cf-11ea-9b0f-cce301019e3a.png">
They contain a callsite properly.
However, if a callsite contains characters which should be escaped for HTML without escaping , the corresponding tooltips wouldn't show the callsite and its following text properly.
<img width="179" alt="dag-viz-tooltip-before-fixed" src="https://user-images.githubusercontent.com/4736016/80128480-b1267200-85d0-11ea-8035-ad68ae5fbcab.png">
<img width="261" alt="timeline-tooltip-before-fixed" src="https://user-images.githubusercontent.com/4736016/80128492-b5eb2600-85d0-11ea-9556-c48490110244.png">
The reason of this issue is that the source texts of the tooltip texts are not escaped.
### Does this PR introduce any user-facing change?
No.
### How was this patch tested?
I tested manually.
First, I ran a job `sc.parallelize(1 to 10).collect` in Spark Shell then, visited AllJobsPage and JobPage and confirmed tooltip texts.
<img width="196" alt="dag-viz-tooltip-fixed" src="https://user-images.githubusercontent.com/4736016/80128813-2db95080-85d1-11ea-82f8-90a1f4547f30.png">
<img width="363" alt="timeline-tooltip-fixed" src="https://user-images.githubusercontent.com/4736016/80128824-31e56e00-85d1-11ea-9818-492b72b1c56e.png">
I also added a testcase.
Closesapache#28317 from sarutak/fix-tooltip.
Authored-by: Kousuke Saruta <sarutak@oss.nttdata.com>
Signed-off-by: Kousuke Saruta <sarutak@oss.nttdata.com>
@sarutaksarutak removed the CORE label Apr 27, 2020
@sarutak

sarutak commented Apr 27, 2020

Copy link
Copy Markdown
MemberAuthor

The error message mentioned here is not shown in this branch.
I found it's because the version of bootstrap.js is different between branch-3.0 and master.

@dongjoon-hyun

dongjoon-hyun commented Apr 27, 2020

Copy link
Copy Markdown
Member

Thank you, @sarutak .

@SparkQA

This comment has been minimized.

Sign in to view
@sarutak

sarutak commented Apr 27, 2020

Copy link
Copy Markdown
MemberAuthor

retest this please.

@SparkQA

This comment has been minimized.

Sign in to view
@dongjoon-hyun

dongjoon-hyun commented Apr 27, 2020

Copy link
Copy Markdown
Member

Retest this please.

@dongjoon-hyun

dongjoon-hyun commented Apr 27, 2020

Copy link
Copy Markdown
Member

cc @gengliangwang , too.

@SparkQA

This comment has been minimized.

Sign in to view
@dongjoon-hyun

dongjoon-hyun commented Apr 27, 2020

Copy link
Copy Markdown
Member

Retest this please.

@SparkQA

This comment has been minimized.

Sign in to view
@gengliangwang

gengliangwang commented Apr 28, 2020

Copy link
Copy Markdown
Member

Retest this please.

@SparkQA

SparkQA commented Apr 28, 2020

Copy link
Copy Markdown

Test build #121935 has finished for PR 28359 at commit 90d3dbf.

  • This patch fails PySpark unit tests.
  • This patch merges cleanly.
  • This patch adds no public classes.

@sarutak

sarutak commented Apr 28, 2020

Copy link
Copy Markdown
MemberAuthor

retest this please.

@dongjoon-hyun

dongjoon-hyun commented Apr 28, 2020•
edited
Loading

Copy link
Copy Markdown
Member

Maybe, we need to re-trigger this once more because this will be terminated at PST midnight.

@sarutak

sarutak commented Apr 28, 2020

Copy link
Copy Markdown
MemberAuthor

Ah, I see. Thanks.

@SparkQA

SparkQA commented Apr 28, 2020

Copy link
Copy Markdown

Test build #121957 has finished for PR 28359 at commit 90d3dbf.

  • This patch fails due to an unknown error code, -9.
  • This patch merges cleanly.
  • This patch adds no public classes.

@gengliangwang

gengliangwang commented Apr 28, 2020

Copy link
Copy Markdown
Member

retest this please.

@SparkQA

SparkQA commented Apr 28, 2020

Copy link
Copy Markdown

Test build #121967 has finished for PR 28359 at commit 90d3dbf.

  • This patch passes all tests.
  • This patch merges cleanly.
  • This patch adds no public classes.

dongjoon-hyun pushed a commit that referenced this pull request Apr 28, 2020
@sarutak@dongjoon-hyun
[SPARK-31534][WEBUI][3.0] Text for tooltip should be escaped
b85262d
### What changes were proposed in this pull request?
This PR backports #28317 which escapes text for tooltip for DAG Viz and Timeline View.
### Why are the changes needed?
This is a bug.
Normally, DAG Viz and Timeline View show tooltip like as follows.
<img width="278" alt="dag-viz-tooltip" src="https://user-images.githubusercontent.com/4736016/80127481-5a6c6880-85cf-11ea-8daf-cfd59aa3ba09.png">
<img width="477" alt="timeline-tooltip" src="https://user-images.githubusercontent.com/4736016/80127500-60624980-85cf-11ea-9b0f-cce301019e3a.png">
They contain a callsite properly.
However, if a callsite contains characters which should be escaped for HTML without escaping , the corresponding tooltips wouldn't show the callsite and its following text properly.
<img width="179" alt="dag-viz-tooltip-before-fixed" src="https://user-images.githubusercontent.com/4736016/80128480-b1267200-85d0-11ea-8035-ad68ae5fbcab.png">
<img width="261" alt="timeline-tooltip-before-fixed" src="https://user-images.githubusercontent.com/4736016/80128492-b5eb2600-85d0-11ea-9556-c48490110244.png">
The reason of this issue is that the source texts of the tooltip texts are not escaped.
### Does this PR introduce any user-facing change?
No.
### How was this patch tested?
I tested manually.
First, I ran a job `sc.parallelize(1 to 10).collect` in Spark Shell then, visited AllJobsPage and JobPage and confirmed tooltip texts.
<img width="277" alt="timeline-tooltip-fixed-3 0" src="https://user-images.githubusercontent.com/4736016/80332616-41411180-8886-11ea-8d93-28e1c5265115.png">
<img width="191" alt="dag-tooltip-fixed-3 0" src="https://user-images.githubusercontent.com/4736016/80332625-44d49880-8886-11ea-8f2e-de8df1369e62.png">
And I confirmed that the appearance of the label of DAG-viz in StagePage is not changed.
<img width="313" alt="stage-page-dag" src="https://user-images.githubusercontent.com/4736016/80332725-9b41d700-8886-11ea-9adb-40d50ad29f86.png">
I also added a testcase.
With this testcase, an error message related to JavaScript is shown.
`TypeError: Cannot call method "indexOf" of undefined (http://192.168.1.209:59376/static/spark-dag-viz.js#378)`
This is thrown from `interpretLineBreak` in `spark-dag-viz.js`.
HtmlUnit seems not to support `innerHTML` for text content (I tried replacing it with `textContent` and the error message is not shown).
But DOMs which is needed by the testcase added is already rendered before `interpretLineBreak` is called. So the testcase successfully passes.
Closes#28359 from sarutak/fix-tooltip-branch-3.0.
Authored-by: Kousuke Saruta <sarutak@oss.nttdata.com>
Signed-off-by: Dongjoon Hyun <dongjoon@apache.org>
@dongjoon-hyun

dongjoon-hyun commented Apr 28, 2020

Copy link
Copy Markdown
Member

Thank you, @sarutak and @gengliangwang . Merged to branch-3.0.

@sarutak
sarutak deleted the fix-tooltip-branch-3.0 branch June 4, 2021 20:44
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@gengliangwanggengliangwanggengliangwang approved these changes
@dongjoon-hyundongjoon-hyundongjoon-hyun approved these changes

Assignees

No one assigned

Labels

WEB UI

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@sarutak@dongjoon-hyun@SparkQA@gengliangwang