DYN-1261 Escape StringInputNode value when converting NodeToCode

[StudioLE]

Purpose

JIRA: DYN-1261

Related Issues:

• Node to code: Backslashes in strings do not get escaped #9117
• Node to code: Double quotes in strings do not get escaped DynamoWishlist#23

Ensure that string containing " and \ are correctly escaped when converting StringInputNode using NodeToCode.

Behaviour before:

Behaviour after:

Declarations

Check these if you believe they are true

• The codebase is in a better state after this PR
• Is documented according to the standards
• The level of testing this PR includes is appropriate
• User facing strings, if any, are extracted into *.resx files
• All tests pass using the self-service CI.
• Snapshot of UI changes, if any.
• Changes to the API follow Semantic Versioning and are documented in the API Changes document.
• This PR modifies some build requirements and the readme is updated

Reviewers

@mjkkirschner @QilongTang @mmisol

FYIs

@Amoursol

[mjkkirschner]

I reran this job and tests all pass - not sure why the checks still show a failure.

[aparajit-pratap]

Using the type name StringInputNode here can be prone to breaking if it's changed in the future for some reason and hard to debug. I would suggest using the following check instead:
if(!(nodeModel is CoreNodeModels.Input.StringInput)) {...}

[StudioLE]

I don't believe it's possible to reference CoreNodeModels.Input.StringInput from the DynamoCore project as DynamoCore is itself a reference of CoreNodeModels so it would lead to a circular dependency.

Is there a different way to identify the StringInput node?

[aparajit-pratap]

Sorry, I missed that. In that case, I would suggest you use the Visitor pattern we have several classes for. I'd suggest you add a new class like the following:

    private class StringReplacer : AstReplacer
    {
        public override AssociativeNode VisitStringNode(StringNode node)
        {
            var escapedString = node.Value.Replace(@"\", @"\\")
                .Replace("\"", "\\\"");
            node.Value = escapedString;
            return node;
        }
    }

Then call this in place of your code above like this:

            #region Step 5 Escape StringInputNodes

            foreach ((NodeModel nodeModel, IEnumerable<AssociativeNode> astNodes) in allAstNodes)
            {
                var stringReplacer = new StringReplacer();
                stringReplacer.VisitNodeList(astNodes.ToList());
            }

            #endregion

[aparajit-pratap]

You can make StringReplacer a private nested class within NodeToCodeCompiler since it's most likely going to be used only in the node to code context.

[StudioLE]

Thanks @aparajit-pratap

I tried implementing the Visitor method but it led to the following tests failing:

This seems to occur as nodes like the DirectoryPath also trigger the StringReplacer.VisitStringNode method which leads to them becoming escaped twice. For the path C:\ you would expect C:\\ But instead you get C:\\\\

It seems the Directory Path is already escaped in the BuildAst method:

Dynamo/src/Libraries/CoreNodeModels/Input/FileSystem.cs

Lines 49 to 55 in b3b5c09

	internal override IEnumerable<AssociativeNode> BuildAst(List<AssociativeNode> inputAstNodes, CompilationContext context)
	{
	if (context == CompilationContext.NodeToCode)
	{
	var rhs = AstFactory.BuildStringNode(Value.Replace(@"\", @"\\"));
	yield return AstFactory.BuildAssignment(GetAstIdentifierForOutputIndex(0), rhs);
	}

As this looks like a cleaner solution I've adopted the same approach for the StringInput node in commit c39adef and reverted the earlier changes I made to NodeToCode.cs. Please let me know your thoughts on this?

[aparajit-pratap]

@StudioLE this looks great. Thanks for proposing the new solution. I had tried exploring changing BuildAst but I didn't remember to check for NodeToCode context.