(#15531) Graph (dot) escaping for double quotes #1038

Closed
wants to merge 1 commit into
from

Projects

None yet

4 participants

@lzap
Contributor
lzap commented Aug 15, 2012

Graph generation was not escaping double quotes for node names. This simple
patch replaces all " with " in all node names in dot graphs so they can be
generated even from catalogs with double quotes.

@lzap lzap Graph (dot) escaping for double quotes
Graph generation was not escaping double quotes for node names. This simple
patch replaces all " with \" in all node names in dot graphs so they can be
generated even from catalogs with double quotes.
140eec0
@zaphod42
Contributor

Instead of using gsub all over the place I think we can get the same effect and cover more cases by using .to_s.inspect.

> irb
1.9.3-p125 :001 > puts "\"\\"
"\
 => nil 
1.9.3-p125 :002 > puts "\"\\".inspect
"\"\\"
 => nil 
@lzap
Contributor
lzap commented Aug 28, 2012

I am not sure about compatibility with Puppet tho.

@zaphod42
Contributor

@lzap, I'm not certain where compatibility with Puppet comes into play. inspect is a standard ruby method and this is trying to generate properly quoted strings for consumption by dot.

@slippycheeze
Contributor

@lzap - would you be able to rewrite this to use inspect, add a test, and retarget it at the 2.7.x branch?

That would be a big help - looking at the code, those are the three things we need before the code can be merged.

@lzap
Contributor
lzap commented Sep 3, 2012

I can do it, do you mean unit tests? Can you point me on some documentation how do you structure those? Found some outdated documents, not sure how should I write it.

@slippycheeze
Contributor

Yeah, @lzap, unit tests. There are already some tests for the DOT output of SimpleGraph, though they are not very comprehensive. You can see them here: https://github.com/puppetlabs/puppet/blob/master/spec/unit/simple_graph_spec.rb#L416

The basic idea would be to construct a graph, like you see there, and then call to_dot_graph on it. That should give you back the string - and you assert (with something like .should =~ /regexp/) that it does the right thing - that names with quotes are escaped correctly.

The rspec (and mocha) gem documentation should help you out. We target rspec of 2.7 or later, but that shouldn't really matter for what you need to do.

You should be able to use rspec directly to run that one spec file and check that everything passes including your test. (Generally we write the tests before we write the fix, so that we are sure it actually fails, but it doesn't matter too much.)

Further documentation, such as it is, is here: http://projects.puppetlabs.com/projects/puppet/wiki/Development_Writing_Tests

@grimradical
Member

This has been in stasis for a while. Without any tests to help ensure that we aren't regressing we can't merge this as-is. Once tests have been added, please reopen (or file a new pull req) and we'll take a look. If you need help on writing tests for this, please ping us in #puppet-dev and we'll help point you in the right direction!

@lzap
Contributor
lzap commented Oct 19, 2012

Ok, this is not a priority for me, but I will keep this on my TODO list. Thanks.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment