Long node names get mangled by `create_dot`

[Peiffap]

Originally reported by @dougthor42 in networkx/networkx#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

>>> 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.

[lkk7]

(Continuing the discussion from #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: #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:

>>> 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):

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:

>>> 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:

>>> 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:

"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:

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:

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

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):

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.