Missing node identifier produces invalid dot file

[eXpl0it3r]

The constructor of DotNode takes an optional string identifier parameter. If none is provided, the identifier is set to null. However, if you do not set an identifiert, the compiled dot file is invalid, as the nodes can not be identified.

I see multiple solutions and was wondering what you think would be the preferred one:

• Make the identifier required
• If the identifier is not set a unique identifier, e.g. a GUID or using a counter

Since the identifier is not visible in the rendered graph from a dot file and since picking a unique name for each node can be a bit annoying, I'd suggest to leave it optional and just generate GUID if not set.

While we're on the topic of uniqueness, currently there are no checks to ensure that the graph has unique identifiers. So if you add two nodes with the same identifier, only one will be rendered. Any ideas on how to solve this?

[vfrz]

I'm ok with your first point, I'll add a piece of code to use a GUID if no identifier is set.
About the uniqueness of the identifiers, I think this is not the responsability of my current compiler implementation. I'd rather develop a parser that would validate graphs and return those type of errors.
I'm open to discussion :)

[eXpl0it3r]

With GUIDs the unique identifier thing, isn't really an issue anymore, as I can simply never set one and the GUID will guarantee uniqueness. As such I can live without it.

Personally, I'd have such a constraint, either on the graph itself or latest in the compiler. And depending on whether you count a graph with duplicated node identifiers as valid or not, it can well be argued that it's the compiler's responsibility to generate a valid graph/dot file. 🤔

[eXpl0it3r]

With Guid.NewGuid() this becomes a bit harder to test, as you now introduce a random element.
Do you think, it would make sense to introduce a identifier generator or similar that could be mocked?
Or would you rather adjust the test, to no do exact string matching on the output?

[vfrz]

I think yes it could make sense to introduce a identifier generator since it's not a lot of work and it allows more freedom.

I've tested to create a graph with two nodes having the same identifier and it compiles, you can have a look here: https://dreampuf.github.io/GraphvizOnline/#digraph%20G%20%7B%0A%0A%20%20a%20%5Bcolor%3Dred%5D%0A%20%20a%20%5Bcolor%3Dgreen%5D%0A%7D

Maybe letting the developer to choose wether or not to check uniqueness (with simple bool parameter, disabled by default) could be a good compromise.

What do you think?

[eXpl0it3r]

Yeah, dot does generate some graph, as such it might technically not be wrong (don't really fancy reading the spec), it's just that you'll usually end up with unexpected out comes.

Sounds like a good compromise to me.

[vfrz]

I've started to implement it, you can check PR #11
For my current implementation if a node has no identifier it's generating one when the node is added to the graph. I am not sure this is the best design decision, and maybe it should be generated when the graph is compiled, so the IdentifierGenerator is linked to the compiler instead of the graph object.
What would be most practical for your use case?

[vfrz]

Fixed by #28
Default identifier generator might be implemented in v3