Skip to content

Commit 90d3dbf

Browse files
committed
[SPARK-31534][WEBUI] Text for tooltip should be escaped
### 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. Closes apache#28317 from sarutak/fix-tooltip. Authored-by: Kousuke Saruta <sarutak@oss.nttdata.com> Signed-off-by: Kousuke Saruta <sarutak@oss.nttdata.com>
1 parent a602ab6 commit 90d3dbf

File tree

5 files changed

+51
-30
lines changed

5 files changed

+51
-30
lines changed

core/src/main/resources/org/apache/spark/ui/static/spark-dag-viz.js

Lines changed: 4 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -173,8 +173,8 @@ function renderDagViz(forJob) {
173173
});
174174

175175
metadataContainer().selectAll(".barrier-rdd").each(function() {
176-
var rddId = d3.select(this).text().trim()
177-
var clusterId = VizConstants.clusterPrefix + rddId
176+
var rddId = d3.select(this).text().trim();
177+
var clusterId = VizConstants.clusterPrefix + rddId;
178178
svg.selectAll("g." + clusterId).classed("barrier", true)
179179
});
180180

@@ -282,11 +282,7 @@ function renderDagVizForJob(svgContainer) {
282282

283283
/* Render the dot file as an SVG in the given container. */
284284
function renderDot(dot, container, forJob) {
285-
var escaped_dot = dot
286-
.replace(/&lt;/g, "<")
287-
.replace(/&gt;/g, ">")
288-
.replace(/&quot;/g, "\"");
289-
var g = graphlibDot.read(escaped_dot);
285+
var g = graphlibDot.read(dot);
290286
var renderer = new dagreD3.render();
291287
preprocessGraphLayout(g, forJob);
292288
renderer(container, g);
@@ -498,18 +494,11 @@ function connectRDDs(fromRDDId, toRDDId, edgesContainer, svgContainer) {
498494
edgesContainer.append("path").datum(points).attr("d", line);
499495
}
500496

501-
/*
502-
* Replace `/n` with `<br/>`
503-
*/
504-
function replaceLineBreak(str) {
505-
return str.replace("\\n", "<br/>");
506-
}
507-
508497
/* (Job page only) Helper function to add tooltips for RDDs. */
509498
function addTooltipsForRDDs(svgContainer) {
510499
svgContainer.selectAll("g.node").each(function() {
511500
var node = d3.select(this);
512-
var tooltipText = replaceLineBreak(node.attr("name"));
501+
var tooltipText = node.attr("name");
513502
if (tooltipText) {
514503
node.select("circle")
515504
.attr("data-toggle", "tooltip")

core/src/main/scala/org/apache/spark/ui/jobs/AllJobsPage.scala

Lines changed: 4 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -88,7 +88,8 @@ private[ui] class AllJobsPage(parent: JobsTab, store: AppStatusStore) extends We
8888
// The timeline library treats contents as HTML, so we have to escape them. We need to add
8989
// extra layers of escaping in order to embed this in a Javascript string literal.
9090
val escapedDesc = Utility.escape(jobDescription)
91-
val jsEscapedDesc = StringEscapeUtils.escapeEcmaScript(escapedDesc)
91+
val jsEscapedDescForTooltip = StringEscapeUtils.escapeEcmaScript(Utility.escape(escapedDesc))
92+
val jsEscapedDescForLabel = StringEscapeUtils.escapeEcmaScript(escapedDesc)
9293
val jobEventJsonAsStr =
9394
s"""
9495
|{
@@ -98,7 +99,7 @@ private[ui] class AllJobsPage(parent: JobsTab, store: AppStatusStore) extends We
9899
| 'end': new Date(${completionTime}),
99100
| 'content': '<div class="application-timeline-content"' +
100101
| 'data-html="true" data-placement="top" data-toggle="tooltip"' +
101-
| 'data-title="${jsEscapedDesc} (Job ${jobId})<br>' +
102+
| 'data-title="${jsEscapedDescForTooltip} (Job ${jobId})<br>' +
102103
| 'Status: ${status}<br>' +
103104
| 'Submitted: ${UIUtils.formatDate(new Date(submissionTime))}' +
104105
| '${
@@ -108,7 +109,7 @@ private[ui] class AllJobsPage(parent: JobsTab, store: AppStatusStore) extends We
108109
""
109110
}
110111
}">' +
111-
| '${jsEscapedDesc} (Job ${jobId})</div>'
112+
| '${jsEscapedDescForLabel} (Job ${jobId})</div>'
112113
|}
113114
""".stripMargin
114115
jobEventJsonAsStr

core/src/main/scala/org/apache/spark/ui/jobs/JobPage.scala

Lines changed: 4 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -69,7 +69,8 @@ private[ui] class JobPage(parent: JobsTab, store: AppStatusStore) extends WebUIP
6969
// The timeline library treats contents as HTML, so we have to escape them. We need to add
7070
// extra layers of escaping in order to embed this in a Javascript string literal.
7171
val escapedName = Utility.escape(name)
72-
val jsEscapedName = StringEscapeUtils.escapeEcmaScript(escapedName)
72+
val jsEscapedNameForTooltip = StringEscapeUtils.escapeEcmaScript(Utility.escape(escapedName))
73+
val jsEscapedNameForLabel = StringEscapeUtils.escapeEcmaScript(escapedName)
7374
s"""
7475
|{
7576
| 'className': 'stage job-timeline-object ${status}',
@@ -78,7 +79,7 @@ private[ui] class JobPage(parent: JobsTab, store: AppStatusStore) extends WebUIP
7879
| 'end': new Date(${completionTime}),
7980
| 'content': '<div class="job-timeline-content" data-toggle="tooltip"' +
8081
| 'data-placement="top" data-html="true"' +
81-
| 'data-title="${jsEscapedName} (Stage ${stageId}.${attemptId})<br>' +
82+
| 'data-title="${jsEscapedNameForTooltip} (Stage ${stageId}.${attemptId})<br>' +
8283
| 'Status: ${status.toUpperCase(Locale.ROOT)}<br>' +
8384
| 'Submitted: ${UIUtils.formatDate(submissionTime)}' +
8485
| '${
@@ -88,7 +89,7 @@ private[ui] class JobPage(parent: JobsTab, store: AppStatusStore) extends WebUIP
8889
""
8990
}
9091
}">' +
91-
| '${jsEscapedName} (Stage ${stageId}.${attemptId})</div>',
92+
| '${jsEscapedNameForLabel} (Stage ${stageId}.${attemptId})</div>',
9293
|}
9394
""".stripMargin
9495
}

core/src/main/scala/org/apache/spark/ui/scope/RDDOperationGraph.scala

Lines changed: 4 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -21,6 +21,7 @@ import java.util.Objects
2121

2222
import scala.collection.mutable
2323
import scala.collection.mutable.{ListBuffer, StringBuilder}
24+
import scala.xml.Utility
2425

2526
import org.apache.commons.text.StringEscapeUtils
2627

@@ -245,8 +246,9 @@ private[spark] object RDDOperationGraph extends Logging {
245246
} else {
246247
""
247248
}
248-
val label = s"${node.name} [${node.id}]$isCached$isBarrier\n${node.callsite}"
249-
s"""${node.id} [label="${StringEscapeUtils.escapeJava(label)}"]"""
249+
val escapedCallsite = Utility.escape(node.callsite)
250+
val label = s"${node.name} [${node.id}]$isCached$isBarrier<br>${escapedCallsite}"
251+
s"""${node.id} [labelType="html" label="${StringEscapeUtils.escapeJava(label)}"]"""
250252
}
251253

252254
/** Update the dot representation of the RDDOperationGraph in cluster to subgraph. */

core/src/test/scala/org/apache/spark/ui/UISeleniumSuite.scala

Lines changed: 35 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -44,6 +44,7 @@ import org.apache.spark.internal.config.Status._
4444
import org.apache.spark.internal.config.UI._
4545
import org.apache.spark.shuffle.FetchFailedException
4646
import org.apache.spark.status.api.v1.{JacksonMessageWriter, RDDDataDistribution, StageStatus}
47+
import org.apache.spark.util.CallSite
4748

4849
private[spark] class SparkUICssErrorHandler extends DefaultCssErrorHandler {
4950

@@ -687,29 +688,29 @@ class UISeleniumSuite extends SparkFunSuite with WebBrowser with Matchers with B
687688
assert(stage0.contains("digraph G {\n subgraph clusterstage_0 {\n " +
688689
"label=&quot;Stage 0&quot;;\n subgraph "))
689690
assert(stage0.contains("{\n label=&quot;parallelize&quot;;\n " +
690-
"0 [label=&quot;ParallelCollectionRDD [0]"))
691+
"0 [labelType=&quot;html&quot; label=&quot;ParallelCollectionRDD [0]"))
691692
assert(stage0.contains("{\n label=&quot;map&quot;;\n " +
692-
"1 [label=&quot;MapPartitionsRDD [1]"))
693+
"1 [labelType=&quot;html&quot; label=&quot;MapPartitionsRDD [1]"))
693694
assert(stage0.contains("{\n label=&quot;groupBy&quot;;\n " +
694-
"2 [label=&quot;MapPartitionsRDD [2]"))
695+
"2 [labelType=&quot;html&quot; label=&quot;MapPartitionsRDD [2]"))
695696

696697
val stage1 = Source.fromURL(sc.ui.get.webUrl +
697698
"/stages/stage/?id=1&attempt=0&expandDagViz=true").mkString
698699
assert(stage1.contains("digraph G {\n subgraph clusterstage_1 {\n " +
699700
"label=&quot;Stage 1&quot;;\n subgraph "))
700701
assert(stage1.contains("{\n label=&quot;groupBy&quot;;\n " +
701-
"3 [label=&quot;ShuffledRDD [3]"))
702+
"3 [labelType=&quot;html&quot; label=&quot;ShuffledRDD [3]"))
702703
assert(stage1.contains("{\n label=&quot;map&quot;;\n " +
703-
"4 [label=&quot;MapPartitionsRDD [4]"))
704+
"4 [labelType=&quot;html&quot; label=&quot;MapPartitionsRDD [4]"))
704705
assert(stage1.contains("{\n label=&quot;groupBy&quot;;\n " +
705-
"5 [label=&quot;MapPartitionsRDD [5]"))
706+
"5 [labelType=&quot;html&quot; label=&quot;MapPartitionsRDD [5]"))
706707

707708
val stage2 = Source.fromURL(sc.ui.get.webUrl +
708709
"/stages/stage/?id=2&attempt=0&expandDagViz=true").mkString
709710
assert(stage2.contains("digraph G {\n subgraph clusterstage_2 {\n " +
710711
"label=&quot;Stage 2&quot;;\n subgraph "))
711712
assert(stage2.contains("{\n label=&quot;groupBy&quot;;\n " +
712-
"6 [label=&quot;ShuffledRDD [6]"))
713+
"6 [labelType=&quot;html&quot; label=&quot;ShuffledRDD [6]"))
713714
}
714715
}
715716
}
@@ -772,6 +773,33 @@ class UISeleniumSuite extends SparkFunSuite with WebBrowser with Matchers with B
772773
}
773774
}
774775

776+
test("SPARK-31534: text for tooltip should be escaped") {
777+
withSpark(newSparkContext()) { sc =>
778+
sc.setLocalProperty(CallSite.LONG_FORM, "collect at <console>:25")
779+
sc.setLocalProperty(CallSite.SHORT_FORM, "collect at <console>:25")
780+
sc.parallelize(1 to 10).collect
781+
782+
val driver = webDriver.asInstanceOf[HtmlUnitDriver]
783+
driver.setJavascriptEnabled(true)
784+
785+
eventually(timeout(10.seconds), interval(50.milliseconds)) {
786+
goToUi(sc, "/jobs")
787+
val jobDesc =
788+
driver.findElement(By.cssSelector("div[class='application-timeline-content']"))
789+
jobDesc.getAttribute("data-title") should include ("collect at &lt;console&gt;:25")
790+
791+
goToUi(sc, "/jobs/job/?id=0")
792+
val stageDesc = driver.findElement(By.cssSelector("div[class='job-timeline-content']"))
793+
stageDesc.getAttribute("data-title") should include ("collect at &lt;console&gt;:25")
794+
795+
// Open DAG Viz.
796+
driver.findElement(By.id("job-dag-viz")).click()
797+
val nodeDesc = driver.findElement(By.cssSelector("g[class='node_0 node']"))
798+
nodeDesc.getAttribute("name") should include ("collect at &lt;console&gt;:25")
799+
}
800+
}
801+
}
802+
775803
def goToUi(sc: SparkContext, path: String): Unit = {
776804
goToUi(sc.ui.get, path)
777805
}

0 commit comments

Comments
 (0)