From 956deb6b0fed78d6b5a7df776880621bdb170667 Mon Sep 17 00:00:00 2001 From: Nathan Daly Date: Mon, 5 Oct 2020 09:26:09 -0400 Subject: [PATCH 1/4] Add (failing) test of Tag with `"` in name --- internal/graph/dotgraph_test.go | 8 ++++++++ internal/graph/testdata/compose7.dot | 4 +++- 2 files changed, 11 insertions(+), 1 deletion(-) diff --git a/internal/graph/dotgraph_test.go b/internal/graph/dotgraph_test.go index 6da63083..be99dea1 100644 --- a/internal/graph/dotgraph_test.go +++ b/internal/graph/dotgraph_test.go @@ -142,9 +142,17 @@ func TestComposeWithNamesThatNeedEscaping(t *testing.T) { g := baseGraph() a, c := baseAttrsAndConfig() + // Change node names to have `"` in them, which need to be escaped for dot. g.Nodes[0].Info = NodeInfo{Name: "var\"src\""} g.Nodes[1].Info = NodeInfo{Name: "var\"#dest#\""} + // Add tag to Node 1 with `"` in name. + g.Nodes[0].LabelTags["a"] = &Tag{ + Name: "var\"tag1\"", + Cum: 10, + Flat: 10, + } + // Set edge to be Residual. g.Nodes[0].Out[g.Nodes[1]].Residual = true diff --git a/internal/graph/testdata/compose7.dot b/internal/graph/testdata/compose7.dot index 2518663d..f8e007a8 100644 --- a/internal/graph/testdata/compose7.dot +++ b/internal/graph/testdata/compose7.dot @@ -2,6 +2,8 @@ digraph "testtitle" { node [style=filled fillcolor="#f8f8f8"] subgraph cluster_L { "label1" [shape=box fontsize=16 label="label1\llabel2\llabel3: \"foo\"\l" tooltip="testtitle"] } N1 [label="var\"src\"\n10 (10.00%)\nof 25 (25.00%)" id="node1" fontsize=22 shape=box tooltip="var\"src\" (25)" color="#b23c00" fillcolor="#edddd5"] +N1_0 [label = "var\"tag1\"" id="N1_0" fontsize=8 shape=box3d tooltip="10"] +N1 -> N1_0 [label=" 10" weight=100 tooltip="10" labeltooltip="10"] N2 [label="var\"#dest#\"\n15 (15.00%)\nof 25 (25.00%)" id="node2" fontsize=24 shape=box tooltip="var\"#dest#\" (25)" color="#b23c00" fillcolor="#edddd5"] -N1 -> N2 [label=" 10" weight=11 color="#b28559" tooltip="var\"src\" ... var\"#dest#\" (10)" labeltooltip="var\"src\" ... var\"#dest#\" (10)" style="dotted"] +N1 -> N2 [label=" 10" weight=11 color="#b28559" tooltip="var\"src\" ... var\"#dest#\" (10)" labeltooltip="var\"src\" ... var\"#dest#\" (10)" style="dotted" minlen=2] } From ce716bd88f56d0177437b4034bc3bf8703796289 Mon Sep 17 00:00:00 2001 From: Nathan Daly Date: Mon, 5 Oct 2020 09:53:27 -0400 Subject: [PATCH 2/4] Escape Nodelet/tag names for Dot as well. This escaping requirement was previously leaking into the implementation of `graph.go`, by writing `\n` directly instead of "\n". This meant that if it was displayed in a different program besides `dot`, it would have been potentially incorrectly escaped. Now, this commit moves all the escaping directly into dotgraph, which is more encapsulated, and also allows us to escape other characters in the name which need escaping. --- internal/graph/dotgraph.go | 6 +++--- internal/graph/graph.go | 2 +- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/internal/graph/dotgraph.go b/internal/graph/dotgraph.go index b52d4e73..47dd0cf4 100644 --- a/internal/graph/dotgraph.go +++ b/internal/graph/dotgraph.go @@ -247,7 +247,7 @@ func (b *builder) addNodelets(node *Node, nodeID int) bool { continue } weight := b.config.FormatValue(w) - nodelets += fmt.Sprintf(`N%d_%d [label = "%s" id="N%d_%d" fontsize=8 shape=box3d tooltip="%s"]`+"\n", nodeID, i, t.Name, nodeID, i, weight) + nodelets += fmt.Sprintf(`N%d_%d [label = "%s" id="N%d_%d" fontsize=8 shape=box3d tooltip="%s"]`+"\n", nodeID, i, escapeForDot(t.Name), nodeID, i, weight) nodelets += fmt.Sprintf(`N%d -> N%d_%d [label=" %s" weight=100 tooltip="%s" labeltooltip="%s"]`+"\n", nodeID, nodeID, i, weight, weight, weight) if nts := lnts[t.Name]; nts != nil { nodelets += b.numericNodelets(nts, maxNodelets, flatTags, fmt.Sprintf(`N%d_%d`, nodeID, i)) @@ -274,7 +274,7 @@ func (b *builder) numericNodelets(nts []*Tag, maxNumNodelets int, flatTags bool, } if w != 0 { weight := b.config.FormatValue(w) - nodelets += fmt.Sprintf(`N%s_%d [label = "%s" id="N%s_%d" fontsize=8 shape=box3d tooltip="%s"]`+"\n", source, j, t.Name, source, j, weight) + nodelets += fmt.Sprintf(`N%s_%d [label = "%s" id="N%s_%d" fontsize=8 shape=box3d tooltip="%s"]`+"\n", source, j, escapeForDot(t.Name), source, j, weight) nodelets += fmt.Sprintf(`%s -> N%s_%d [label=" %s" weight=100 tooltip="%s" labeltooltip="%s"%s]`+"\n", source, source, j, weight, weight, weight, attr) } } @@ -487,5 +487,5 @@ func escapeAllForDot(in []string) []string { // "center" character (\n) with a left-justified character. // See https://graphviz.org/doc/info/attrs.html#k:escString for more info. func escapeForDot(str string) string { - return strings.ReplaceAll(strings.ReplaceAll(strings.ReplaceAll(str, `\`, `\\`), `"`, `\"`), "\n", `\l`) + return strings.ReplaceAll(strings.ReplaceAll(strings.ReplaceAll(str, `\`, `\\`), `"`, `\"`), "\n", `\n`) } diff --git a/internal/graph/graph.go b/internal/graph/graph.go index d2397a93..8ac089ab 100644 --- a/internal/graph/graph.go +++ b/internal/graph/graph.go @@ -522,7 +522,7 @@ func joinLabels(s *profile.Sample) string { } } sort.Strings(labels) - return strings.Join(labels, `\n`) + return strings.Join(labels, "\n") // This will be escaped downstream if needed. } // isNegative returns true if the node is considered as "negative" for the From e739e7e586b06d81d5f1757c280e2378cc12e1aa Mon Sep 17 00:00:00 2001 From: Nathan Daly Date: Mon, 5 Oct 2020 10:15:29 -0400 Subject: [PATCH 3/4] Add justify option to escapeForDot to preserve old behavior This allows tag newlines to stay center justified, and legend label newlines to stay left justified, as they were before we started escaping them for Dot. Also ran gofmt --- internal/graph/dotgraph.go | 39 +++++++++++++++++++++++---------- internal/graph/dotgraph_test.go | 21 +++++++++++++++++- internal/graph/graph.go | 2 +- 3 files changed, 49 insertions(+), 13 deletions(-) diff --git a/internal/graph/dotgraph.go b/internal/graph/dotgraph.go index 47dd0cf4..185e0c65 100644 --- a/internal/graph/dotgraph.go +++ b/internal/graph/dotgraph.go @@ -127,7 +127,7 @@ func (b *builder) addLegend() { } title := labels[0] fmt.Fprintf(b, `subgraph cluster_L { "%s" [shape=box fontsize=16`, title) - fmt.Fprintf(b, ` label="%s\l"`, strings.Join(escapeAllForDot(labels), `\l`)) + fmt.Fprintf(b, ` label="%s\l"`, strings.Join(escapeAllForDot(labels, LeftJustify), `\l`)) if b.config.LegendURL != "" { fmt.Fprintf(b, ` URL="%s" target="_blank"`, b.config.LegendURL) } @@ -187,7 +187,7 @@ func (b *builder) addNode(node *Node, nodeID int, maxFlat float64) { // Create DOT attribute for node. attr := fmt.Sprintf(`label="%s" id="node%d" fontsize=%d shape=%s tooltip="%s (%s)" color="%s" fillcolor="%s"`, - label, nodeID, fontSize, shape, escapeForDot(node.Info.PrintableName()), cumValue, + label, nodeID, fontSize, shape, escapeForDot(node.Info.PrintableName(), CenterJustify), cumValue, dotColor(float64(node.CumValue())/float64(abs64(b.config.Total)), false), dotColor(float64(node.CumValue())/float64(abs64(b.config.Total)), true)) @@ -247,7 +247,7 @@ func (b *builder) addNodelets(node *Node, nodeID int) bool { continue } weight := b.config.FormatValue(w) - nodelets += fmt.Sprintf(`N%d_%d [label = "%s" id="N%d_%d" fontsize=8 shape=box3d tooltip="%s"]`+"\n", nodeID, i, escapeForDot(t.Name), nodeID, i, weight) + nodelets += fmt.Sprintf(`N%d_%d [label = "%s" id="N%d_%d" fontsize=8 shape=box3d tooltip="%s"]`+"\n", nodeID, i, escapeForDot(t.Name, CenterJustify), nodeID, i, weight) nodelets += fmt.Sprintf(`N%d -> N%d_%d [label=" %s" weight=100 tooltip="%s" labeltooltip="%s"]`+"\n", nodeID, nodeID, i, weight, weight, weight) if nts := lnts[t.Name]; nts != nil { nodelets += b.numericNodelets(nts, maxNodelets, flatTags, fmt.Sprintf(`N%d_%d`, nodeID, i)) @@ -274,7 +274,7 @@ func (b *builder) numericNodelets(nts []*Tag, maxNumNodelets int, flatTags bool, } if w != 0 { weight := b.config.FormatValue(w) - nodelets += fmt.Sprintf(`N%s_%d [label = "%s" id="N%s_%d" fontsize=8 shape=box3d tooltip="%s"]`+"\n", source, j, escapeForDot(t.Name), source, j, weight) + nodelets += fmt.Sprintf(`N%s_%d [label = "%s" id="N%s_%d" fontsize=8 shape=box3d tooltip="%s"]`+"\n", source, j, escapeForDot(t.Name, CenterJustify), source, j, weight) nodelets += fmt.Sprintf(`%s -> N%s_%d [label=" %s" weight=100 tooltip="%s" labeltooltip="%s"%s]`+"\n", source, source, j, weight, weight, weight, attr) } } @@ -305,8 +305,8 @@ func (b *builder) addEdge(edge *Edge, from, to int, hasNodelets bool) { arrow = "..." } tooltip := fmt.Sprintf(`"%s %s %s (%s)"`, - escapeForDot(edge.Src.Info.PrintableName()), arrow, - escapeForDot(edge.Dest.Info.PrintableName()), w) + escapeForDot(edge.Src.Info.PrintableName(), CenterJustify), arrow, + escapeForDot(edge.Dest.Info.PrintableName(), CenterJustify), w) attr = fmt.Sprintf(`%s tooltip=%s labeltooltip=%s`, attr, tooltip, tooltip) if edge.Residual { @@ -383,7 +383,7 @@ func dotColor(score float64, isBackground bool) string { func multilinePrintableName(info *NodeInfo) string { infoCopy := *info - infoCopy.Name = escapeForDot(ShortenFunctionName(infoCopy.Name)) + infoCopy.Name = escapeForDot(ShortenFunctionName(infoCopy.Name), CenterJustify) infoCopy.Name = strings.Replace(infoCopy.Name, "::", `\n`, -1) infoCopy.Name = strings.Replace(infoCopy.Name, ".", `\n`, -1) if infoCopy.File != "" { @@ -474,11 +474,19 @@ func min64(a, b int64) int64 { return b } +type DotJustifyType int64 + +const ( + LeftJustify DotJustifyType = 0 + CenterJustify DotJustifyType = 1 + RightJustify DotJustifyType = 2 +) + // Applies escapeForDot to all strings in the given slice. -func escapeAllForDot(in []string) []string { +func escapeAllForDot(in []string, justify DotJustifyType) []string { var out = make([]string, len(in)) for i := range in { - out[i] = escapeForDot(in[i]) + out[i] = escapeForDot(in[i], justify) } return out } @@ -486,6 +494,15 @@ func escapeAllForDot(in []string) []string { // escapeForDot escapes double quotes and backslashes, and replaces Graphviz's // "center" character (\n) with a left-justified character. // See https://graphviz.org/doc/info/attrs.html#k:escString for more info. -func escapeForDot(str string) string { - return strings.ReplaceAll(strings.ReplaceAll(strings.ReplaceAll(str, `\`, `\\`), `"`, `\"`), "\n", `\n`) +func escapeForDot(str string, justify DotJustifyType) string { + var newline string + switch justify { + case LeftJustify: + newline = `\l` + case CenterJustify: + newline = `\n` + case RightJustify: + newline = `\r` + } + return strings.ReplaceAll(strings.ReplaceAll(strings.ReplaceAll(str, `\`, `\\`), `"`, `\"`), "\n", newline) } diff --git a/internal/graph/dotgraph_test.go b/internal/graph/dotgraph_test.go index be99dea1..75104406 100644 --- a/internal/graph/dotgraph_test.go +++ b/internal/graph/dotgraph_test.go @@ -383,11 +383,30 @@ func TestEscapeForDot(t *testing.T) { }, } { t.Run(tc.desc, func(t *testing.T) { - if got := escapeAllForDot(tc.input); !reflect.DeepEqual(got, tc.want) { + if got := escapeAllForDot(tc.input, LeftJustify); !reflect.DeepEqual(got, tc.want) { t.Errorf("escapeAllForDot(%s) = %s, want %s", tc.input, got, tc.want) } }) } + + // Test the different options for justifying text newlines in Dot + for _, justify := range []DotJustifyType{LeftJustify, CenterJustify, RightJustify} { + t.Run("Dot newline justification", func(t *testing.T) { + input := []string{"Line 1\nLine 2"} + var want []string + switch justify { + case LeftJustify: + want = []string{`Line 1\lLine 2`} + case CenterJustify: + want = []string{`Line 1\nLine 2`} + case RightJustify: + want = []string{`Line 1\rLine 2`} + } + if got := escapeAllForDot(input, justify); !reflect.DeepEqual(got, want) { + t.Errorf("escapeAllForDot(%s) = %s, want %s", input, got, want) + } + }) + } } func tagString(t []*Tag) string { diff --git a/internal/graph/graph.go b/internal/graph/graph.go index 8ac089ab..35283006 100644 --- a/internal/graph/graph.go +++ b/internal/graph/graph.go @@ -522,7 +522,7 @@ func joinLabels(s *profile.Sample) string { } } sort.Strings(labels) - return strings.Join(labels, "\n") // This will be escaped downstream if needed. + return strings.Join(labels, "\n") // This will be escaped downstream if needed. } // isNegative returns true if the node is considered as "negative" for the From 8c597aa2bd9181b599ab037ffc87c779d9eb03f7 Mon Sep 17 00:00:00 2001 From: Nathan Daly Date: Mon, 5 Oct 2020 10:33:36 -0400 Subject: [PATCH 4/4] Unexport dotJustifyType since it's only meant to be used for dotgraph. --- internal/graph/dotgraph.go | 32 ++++++++++++++++---------------- internal/graph/dotgraph_test.go | 10 +++++----- 2 files changed, 21 insertions(+), 21 deletions(-) diff --git a/internal/graph/dotgraph.go b/internal/graph/dotgraph.go index 185e0c65..26c1c9e4 100644 --- a/internal/graph/dotgraph.go +++ b/internal/graph/dotgraph.go @@ -127,7 +127,7 @@ func (b *builder) addLegend() { } title := labels[0] fmt.Fprintf(b, `subgraph cluster_L { "%s" [shape=box fontsize=16`, title) - fmt.Fprintf(b, ` label="%s\l"`, strings.Join(escapeAllForDot(labels, LeftJustify), `\l`)) + fmt.Fprintf(b, ` label="%s\l"`, strings.Join(escapeAllForDot(labels, leftJustify), `\l`)) if b.config.LegendURL != "" { fmt.Fprintf(b, ` URL="%s" target="_blank"`, b.config.LegendURL) } @@ -187,7 +187,7 @@ func (b *builder) addNode(node *Node, nodeID int, maxFlat float64) { // Create DOT attribute for node. attr := fmt.Sprintf(`label="%s" id="node%d" fontsize=%d shape=%s tooltip="%s (%s)" color="%s" fillcolor="%s"`, - label, nodeID, fontSize, shape, escapeForDot(node.Info.PrintableName(), CenterJustify), cumValue, + label, nodeID, fontSize, shape, escapeForDot(node.Info.PrintableName(), centerJustify), cumValue, dotColor(float64(node.CumValue())/float64(abs64(b.config.Total)), false), dotColor(float64(node.CumValue())/float64(abs64(b.config.Total)), true)) @@ -247,7 +247,7 @@ func (b *builder) addNodelets(node *Node, nodeID int) bool { continue } weight := b.config.FormatValue(w) - nodelets += fmt.Sprintf(`N%d_%d [label = "%s" id="N%d_%d" fontsize=8 shape=box3d tooltip="%s"]`+"\n", nodeID, i, escapeForDot(t.Name, CenterJustify), nodeID, i, weight) + nodelets += fmt.Sprintf(`N%d_%d [label = "%s" id="N%d_%d" fontsize=8 shape=box3d tooltip="%s"]`+"\n", nodeID, i, escapeForDot(t.Name, centerJustify), nodeID, i, weight) nodelets += fmt.Sprintf(`N%d -> N%d_%d [label=" %s" weight=100 tooltip="%s" labeltooltip="%s"]`+"\n", nodeID, nodeID, i, weight, weight, weight) if nts := lnts[t.Name]; nts != nil { nodelets += b.numericNodelets(nts, maxNodelets, flatTags, fmt.Sprintf(`N%d_%d`, nodeID, i)) @@ -274,7 +274,7 @@ func (b *builder) numericNodelets(nts []*Tag, maxNumNodelets int, flatTags bool, } if w != 0 { weight := b.config.FormatValue(w) - nodelets += fmt.Sprintf(`N%s_%d [label = "%s" id="N%s_%d" fontsize=8 shape=box3d tooltip="%s"]`+"\n", source, j, escapeForDot(t.Name, CenterJustify), source, j, weight) + nodelets += fmt.Sprintf(`N%s_%d [label = "%s" id="N%s_%d" fontsize=8 shape=box3d tooltip="%s"]`+"\n", source, j, escapeForDot(t.Name, centerJustify), source, j, weight) nodelets += fmt.Sprintf(`%s -> N%s_%d [label=" %s" weight=100 tooltip="%s" labeltooltip="%s"%s]`+"\n", source, source, j, weight, weight, weight, attr) } } @@ -305,8 +305,8 @@ func (b *builder) addEdge(edge *Edge, from, to int, hasNodelets bool) { arrow = "..." } tooltip := fmt.Sprintf(`"%s %s %s (%s)"`, - escapeForDot(edge.Src.Info.PrintableName(), CenterJustify), arrow, - escapeForDot(edge.Dest.Info.PrintableName(), CenterJustify), w) + escapeForDot(edge.Src.Info.PrintableName(), centerJustify), arrow, + escapeForDot(edge.Dest.Info.PrintableName(), centerJustify), w) attr = fmt.Sprintf(`%s tooltip=%s labeltooltip=%s`, attr, tooltip, tooltip) if edge.Residual { @@ -383,7 +383,7 @@ func dotColor(score float64, isBackground bool) string { func multilinePrintableName(info *NodeInfo) string { infoCopy := *info - infoCopy.Name = escapeForDot(ShortenFunctionName(infoCopy.Name), CenterJustify) + infoCopy.Name = escapeForDot(ShortenFunctionName(infoCopy.Name), centerJustify) infoCopy.Name = strings.Replace(infoCopy.Name, "::", `\n`, -1) infoCopy.Name = strings.Replace(infoCopy.Name, ".", `\n`, -1) if infoCopy.File != "" { @@ -474,16 +474,16 @@ func min64(a, b int64) int64 { return b } -type DotJustifyType int64 +type dotJustifyType int64 const ( - LeftJustify DotJustifyType = 0 - CenterJustify DotJustifyType = 1 - RightJustify DotJustifyType = 2 + leftJustify dotJustifyType = 0 + centerJustify dotJustifyType = 1 + rightJustify dotJustifyType = 2 ) // Applies escapeForDot to all strings in the given slice. -func escapeAllForDot(in []string, justify DotJustifyType) []string { +func escapeAllForDot(in []string, justify dotJustifyType) []string { var out = make([]string, len(in)) for i := range in { out[i] = escapeForDot(in[i], justify) @@ -494,14 +494,14 @@ func escapeAllForDot(in []string, justify DotJustifyType) []string { // escapeForDot escapes double quotes and backslashes, and replaces Graphviz's // "center" character (\n) with a left-justified character. // See https://graphviz.org/doc/info/attrs.html#k:escString for more info. -func escapeForDot(str string, justify DotJustifyType) string { +func escapeForDot(str string, justify dotJustifyType) string { var newline string switch justify { - case LeftJustify: + case leftJustify: newline = `\l` - case CenterJustify: + case centerJustify: newline = `\n` - case RightJustify: + case rightJustify: newline = `\r` } return strings.ReplaceAll(strings.ReplaceAll(strings.ReplaceAll(str, `\`, `\\`), `"`, `\"`), "\n", newline) diff --git a/internal/graph/dotgraph_test.go b/internal/graph/dotgraph_test.go index 75104406..bd345a6f 100644 --- a/internal/graph/dotgraph_test.go +++ b/internal/graph/dotgraph_test.go @@ -383,23 +383,23 @@ func TestEscapeForDot(t *testing.T) { }, } { t.Run(tc.desc, func(t *testing.T) { - if got := escapeAllForDot(tc.input, LeftJustify); !reflect.DeepEqual(got, tc.want) { + if got := escapeAllForDot(tc.input, leftJustify); !reflect.DeepEqual(got, tc.want) { t.Errorf("escapeAllForDot(%s) = %s, want %s", tc.input, got, tc.want) } }) } // Test the different options for justifying text newlines in Dot - for _, justify := range []DotJustifyType{LeftJustify, CenterJustify, RightJustify} { + for _, justify := range []dotJustifyType{leftJustify, centerJustify, rightJustify} { t.Run("Dot newline justification", func(t *testing.T) { input := []string{"Line 1\nLine 2"} var want []string switch justify { - case LeftJustify: + case leftJustify: want = []string{`Line 1\lLine 2`} - case CenterJustify: + case centerJustify: want = []string{`Line 1\nLine 2`} - case RightJustify: + case rightJustify: want = []string{`Line 1\rLine 2`} } if got := escapeAllForDot(input, justify); !reflect.DeepEqual(got, want) {