Repository: pydot/pydot

Issue:
Long node names get mangled by `create_dot`
_Originally reported by @dougthor42 in https://github.com/networkx/networkx/issues/7648._

When working with long node names (~ >130 characters), `create_dot` mangles the name by adding line breaks. This makes it hard to round-trip `pydot.Graph` objects through the dot command and read them back in again (which is what `nx.nx_pydot.pydot_layout` does).


A minimum reproduction is
```python
>>> import networkx as nx
>>> import pydot
>>> import locale
>>> G = nx.Graph()
>>> G.add_node("=10chars= " * 14)
>>> P = nx.nx_pydot.to_pydot(G)
>>> bytes = P.create_dot()
>>> D = str(bytes, encoding=locale.getpreferredencoding())
>>> Q = pydot.graph_from_dot_data(D)[0]
>>> print(Q.get_nodes()[-1].get_name())
"=10chars= =10chars= =10chars= =10chars= =10chars= =10chars= =10chars= =10chars= =10chars= =10chars= =10chars= =10chars= =10chars= =\
10chars= "
>>> print(pydot.quote_id_if_necessary(str(list(G.nodes())[0])))
"=10chars= =10chars= =10chars= =10chars= =10chars= =10chars= =10chars= =10chars= =10chars= =10chars= =10chars= =10chars= =10chars= =10chars= "
```
The two names aren't equal, and it would be nice if they were.

Hints from issue comments:
@lkk7: (Continuing the discussion from https://github.com/pydot/pydot/pull/420)
(BTW, that PR doesn't fix the issue)


It's Graphviz (dot) itself that performs the name splitting, not pydot.


I couldn't find any documentation mentioning this. If this is an old, deeply ingrained thing, then they'll never change it. I understand it's probably for visual clarity, but still... 😞

I'm wondering what's the expected behavior of pydot in this situation.

I don't think we should:
- Remove the split when reading an already split graph (because someone might be doing it **intentionally**)
- Do that split anywhere in the internal representation (because it's undocumented, weird, and unexpected)

The least invasive way to fix the problem might be to extend `get_node` so that it also checks the "unsplit" names of existing nodes.

CC: @ferdnyc 

@lkk7: Proposed fix: https://github.com/pydot/pydot/pull/486
(tired of mypy complaining, but ignore that because everything else passes)@ferdnyc: OK, so the issue is that graphviz's `-Tdot` mode is using backslash continuation inside the node ID string, if it gets long enough:

```python
>>> import networkx as nx
>>> import pydot
>>> import locale
>>> G = nx.Graph()
>>> G.add_node("=10chars= " * 14)
>>> G.nodes()
NodeView(('=10chars= =10chars= =10chars= =10chars= =10chars= =10chars= =10chars= =10chars= =10chars= =10chars= =10chars= =10chars= =10chars= =10chars= ',))
>>> P = nx.nx_pydot.to_pydot(G)
>>> print(P.to_string(indent=2))
strict graph {
  "=10chars= =10chars= =10chars= =10chars= =10chars= =10chars= =10chars= =10chars= =10chars= =10chars= =10chars= =10chars= =10chars= =10chars= ";
}
>>> gbytes = P.create_dot()
>>> print(gbytes.decode('utf-8'))
strict graph {
	graph [bb="0,0,1701.8,36"];
	node [label="\N"];
	"=10chars= =10chars= =10chars= =10chars= =10chars= =10chars= =10chars= =10chars= =10chars= =10chars= =10chars= =10chars= =10chars= =\
10chars= "	[height=0.5,
		pos="850.9,18",
		width=23.636];
}
```

...We can work with that. It isn't part of the grammar (that I can recall), but neither are lists of nodes. (Tho we don't support those either, at least not yet. #465) It shouldn't be too difficult to add it to the parser, though.

@lkk7 

> I don't think we should:
> 
> * Remove the split when reading an already split graph (because someone might be doing it intentionally)
> * Do that split anywhere in the internal representation (because it's undocumented, weird, and unexpected)
>
> The least invasive way to fix the problem might be to extend get_node so that it also checks the "unsplit" names of existing nodes.

I'm not sure I agree with that. I think our _parser_ should join backslash-continued quoted strings; it's a well-defined syntax (whether or not it's documented in the grammar) and can easily be parsed.

Even if someone is doing it intentionally (meaning, they write a node name like this):

```dot
graph G {
  "my long node \
name";
}
```

...it seems clear to me that they intend for the node name to be `"my long node name"`, not `"my long node \\\nname"`. So I think that's how the parser should read it: as `"my long node name"`.@ferdnyc: If they _create_ a node with an ID like that using the API, that's a different story. Currently (because I haven't finished #420) the results will be... a bit odd:

```python
>>> import pydot
>>> g = pydot.Graph("G", graph_type="graph")
>>> g.add_node(pydot.Node("my long node \\\nname"))
>>> g.add_node(pydot.Node(r"my long node \
... name"))
>>> print(g.to_string(indent=2))
graph G {
  "my long node \\nname";
  "my long node \\nname";
}
```

...But part of my proposal in #420 was always to _stop_ embedding escaped line-break characters (`\\n` and `\\r\\n`) in the rendered graph syntax, instead allowing them to remain as _actual_, un-escaped line-break characters, since graphviz fully-supports quoted strings broken over multiple lines. And backslash-continuation fits nicely into that.

The current state of #420 is that it would output the nodes exactly as written:

```python
>>> import pydot
>>> g = pydot.Graph("G", graph_type="graph")
>>> g.add_node(pydot.Node('''"my long node \\\nname"'''))
>>> g.add_node(pydot.Node(r'''"my long node \
... name"'''))
>>> print(g.to_string(indent=2))
graph G {
  "my long node \
name";
  "my long node \
name";
}
```

...which means a backslash-escape-supporting parser would remove those escaped newlines on round-trip, and the _imported_ node names would be `"my long node name"`.

**That feels _kind of_ CORRECT to me, honestly...** but if we want to ensure that node names don't _change_ when round-tripped through graphviz, **even when** they contain weird sequences that would seem to be special to the parser, then `.to_string()` should probably re-gain its practice of doubling **only** escaped backslashes (but still not backslashes that escape other characters), so that `\n` remains `\n` but `\\` becomes `\\\\`, and the node ID is therefore output as `"my long node \\\\\nname"` which will end up in the dot source as:
```dot
"my long node \\
name";
```

(And the parser patch I just finished that would remove any `\\\n` or `\\\r\n` sequences encountered in double-quoted strings would have to be expanded to _not_ remove `\\\\\n` or `\\\\\r\n`.)@ferdnyc: The other weird thing is that currently, the `double_quoted_string` parser will leave any doubled backslashes in the string, even though it's defined as:

```python
double_quoted_string = QuotedString(
    '"', multiline=True, unquoteResults=False, escChar="\\"
)
```

...I think that's because the pyparsing docs for `QuotedString` say that `esc_char` / `escChar` is the "character to re_escape quotes, typically backslash (default= `None`)". Meaning that it _only_ escapes quote characters, and not other characters (including itself).

So, if we're going to want to parse this:

```dot
graph G {
  "my long node \\
name";
}
```

..back in the way it started, as `pydot.Node("my long node \\\nname")`, rather than the `pydot.Node("my long node \\\\\name")` it'll be parsed as today, the parser will also have to handle that itself.@lkk7: I agree with each comment – my approach was looking for a lazy workaround while yours is tackling the root of the issue. I misjudged the importance of get_node, while the parser is the thing underlying everything 😁

EDIT: everything below doesn't make sense. We won't output them the same way. 

---
---
---

Still, I'm not sure about the get_node/add_node (API) behavior. Let's say someone adds two nodes:
```
a\
bc

ab\
c
```

Which are functionally the same, but different in representation. We (would like to) output them the same way in to_string, so should we keep them internally as non-equal nodes? Someone might be accessing their obj_dicts or something (that's an absolute edge case) @ferdnyc: > The other weird thing is that currently, the `double_quoted_string` parser will leave any doubled backslashes in the string, even though it's defined as:
> 
> double_quoted_string = QuotedString(
>     '"', multiline=True, unquoteResults=False, escChar="\\"
> )
> ...I think that's because the pyparsing docs for `QuotedString` say that `esc_char` / `escChar` is the "character to re_escape quotes, typically backslash (default= `None`)". Meaning that it _only_ escapes quote characters, and not other characters (including itself).

Ohhh... INTERESTING.

I just discovered (from reading the implementation in PyParsing) that `QuotedString`'s handling of escaped characters is dependent on its `unquote_results` parameter being _True_!!

If `unquote_results=False` (as our parser is configured), then that also disables the built-in handling of escaped characters like `r"\n"`, `r"\t"`, and `r"\\"`. (Specifically, it leaves them alone.) The processing activated by the (default-`True`) `convert_whitespace_escapes` is activated **IF AND ONLY IF** `unquote_results` is _also_ true!

So, that's why the processing of something like `r'"my long node \\\nname"'` seems weird. With `unquote_results` disabled, it's parsed as `'"my long node \\\\\\nname"'`. Activate `unquote_results`, and it's parsed as `'my long node \\\name'`.

Still doesn't get us escaped line continuations (I'm thinking of adding that in and submitting a PR), but it explains why `convert_whitespace_escapes` wasn't doing its job even though it's enabled by default: It's because we have `unoquote_results` turned off.

(That feels like it may be a bug, but it's probably too late to change it without breaking lots of people's parsers, so the extra condition may just have to be added to the PyParsing documentation instead.)

(**Edit:** PyParsing documentation PR: pyparsing/pyparsing#606)@Peiffap: Thanks Frank for the detailed investigation and explanation. It seems this has become an exercise in finding undocumented behaviors... It'll take me a while to parse (hah) and digest all of this, but in the meantime, some thoughts:

>it seems clear to me that they intend for the node name to be `"my long node name"`, not `"my long node \\\nname"`

I'm not sure I agree with this? I've always felt it best to work with what users provide instead of trying to guess what they're trying to say, and this feels like the latter. Perhaps you have some more context I'm missing that allows you to be confident you're properly interpreting what this hypothetical user is trying to achieve.

>but if we want to ensure that node names don't change when round-tripped through graphviz

I feel like that's the safest bet. There's no ambiguity or guessing, and what you put in is what you get out. If you're reluctant to go with this option, is it because of implementation overhead—I have a pretty poor understanding of how much work this change would take—or is there another concern I'm missing?@lkk7: > I've always felt it best to work with what users provide instead of trying to guess what they're trying to say

That's a good point. 
But I believe when the user writes:
```
graph G {
  "my long node \
name";
}
```

There is no guess, because they MUST want "my long node name". Slash + newline means literally "nothing". They're operating in the DOT syntax which ignores it. There is no other scenario/intention, right? If they wanted an actual "slash + newline" rendered as characters, then they would do:
```
graph G {
  "my long node \\
name";
}
```

or

```
graph G {
  "my long node \\\nname";
}
```

It's something like escaping in Python, right? Where `a` and `b` is the same thing 
```python
a = "abcdef"
b = (
  "abc"
   "def"
)
```


That's about the parser. I'm not sure about nodes constructed manually and added through the `add_node()` API though.
Because you can't possibly know what the user wanted (did they think in DOT syntax or in "what I put in is what I want to see in the picture"). Ehh 😄.



@ferdnyc: Apologies for the delay (which was, as always, more in writing tests to verify the fix, than in doing the actual fix), but I've just opened #487 with a fix for this in the parser.

@Peiffap 


> > it seems clear to me that they intend for the node name to be "my long node name", not "my long node \\\nname"
> 
> I'm not sure I agree with this? I've always felt it best to work with what users provide instead of trying to guess what they're trying to say, and this feels like the latter. Perhaps you have some more context I'm missing that allows you to be confident you're properly interpreting what this hypothetical user is trying to achieve.

Well, the syntax, basically. Every act of parsing is inherently "interpreting what [the] user is trying to achieve", which is why an agreed-upon grammar is so important. Graphviz supports backslash-continuations in quoted strings, so the only reasonable interpretation of any input that employs that syntax is to interpret it the way it's defined in the language.

(I was incorrect in thinking that line continuations aren't part of the official language definition, too. They are, they're just not part of the formal _grammar_. But the language documentation says, in the ["Comments and optional formatting" section](https://graphviz.org/doc/info/lang.html#comments-and-optional-formatting)):

> As another aid for readability, dot allows double-quoted strings to span multiple physical lines using the standard C convention of a backslash immediately preceding a newline character.

...And I just noticed that it also says this:

> In addition, double-quoted strings can be concatenated using a '+' operator.

Which is another piece of syntax our parser is currently missing support for.@Peiffap: Yeah, I didn't realise this was part of the Graphviz syntax—that's the context I was missing! Makes sense to treat `\<newline>` like it doesn't exist, then. I'll have a look at the proposed fix now.@ferdnyc: Hah! Now, this is crazy.

I just happened to be looking at the old, old commit (2b3f0885530ecfa8391c65583b2153a64dbc3e58) that added most of the original test files way-back-when, and it seems that commit included this file:

[`test/my_tests/escaped_newlines.dot`](https://github.com/pydot/pydot/blob/2b3f0885530ecfa8391c65583b2153a64dbc3e58/test/my_tests/escaped_newlines.dot#L1C13-L8)

```dot
digraph fa {
	1 -> 1 [pos="e,724\
5\
,\
687\
7 70\
55,6992"]
}
```

...I guess it was later dropped to avoid being a failing test, since support was never added to the parser.

Solution patch:
diff --git a/src/pydot/core.py b/src/pydot/core.py
index 7a2c62f..8dcca58 100644
--- a/src/pydot/core.py
+++ b/src/pydot/core.py
@@ -569,7 +569,7 @@ class Common:
         self.obj_dict["parent_graph"] = parent_graph
 
     def get_parent_graph(self) -> Graph | None:
-        return self.obj_dict.get("parent_graph", None)  # type: ignore
+        return self.obj_dict.get("parent_graph", None)
 
     def get_top_graph_type(self, default: str = "graph") -> str:
         """Find the topmost parent graph type for the current object."""
diff --git a/src/pydot/dot_parser.py b/src/pydot/dot_parser.py
index 3884b66..5f9e0ec 100644
--- a/src/pydot/dot_parser.py
+++ b/src/pydot/dot_parser.py
@@ -171,6 +171,15 @@ def expand_attr_lists(attr_l: Any) -> dict[str, Any]:
     return attrs
 
 
+def push_dbl_quoted(toks: ParseResults) -> str:
+    assert "dbl_quoted" in toks and isinstance(toks.dbl_quoted, str)
+    s = toks.dbl_quoted
+    # Remove backslash line-continuations
+    if "\\" in s:
+        s = s.replace("\\\r\n", "").replace("\\\n", "")
+    return s
+
+
 def push_graph_stmt(toks: ParseResults) -> pydot.core.Subgraph:
     g = pydot.core.Subgraph("")
     g.obj_dict["show_keyword"] = False
@@ -254,10 +263,14 @@ class GraphParser:
     )
 
     double_quoted_string = QuotedString(
-        '"', multiline=True, unquoteResults=False, escChar="\\"
+        '"', multiline=True, unquote_results=False, esc_char="\\"
     )
 
-    ID = identifier | HTML() | double_quoted_string
+    ID = (
+        identifier
+        | HTML()
+        | double_quoted_string("dbl_quoted").set_parse_action(push_dbl_quoted)
+    )
 
     float_number = Combine(Optional(minus) + OneOrMore(Word(nums + ".")))
 


Test patch:
diff --git a/test/test_parser.py b/test/test_parser.py
index 1d156b0..4140ed9 100644
--- a/test/test_parser.py
+++ b/test/test_parser.py
@@ -97,3 +97,23 @@ def test_strict_graph_parsing() -> None:
     assert len(res2) == 2
     assert not res2[0].get_strict()
     assert res2[1].get_strict()
+
+
+def test_backslash_continuations() -> None:
+    src = textwrap.dedent(r"""
+        graph G {
+            "my very long node \
+        name" [color=red];
+            "my indented and wrapped \
+            node name" [shape=square];
+            "my node name containing \ backslash";
+        }""")
+    res = dot_parser.parse_dot_data(src)
+    assert isinstance(res, list)
+    assert len(res) == 1
+    graph = res[0]
+    nodes = graph.get_nodes()
+    assert len(nodes) == 3
+    assert nodes[0].get_name() == '"my very long node name"'
+    assert nodes[1].get_name() == '"my indented and wrapped     node name"'
+    assert nodes[2].get_name() == '"my node name containing \\ backslash"'