New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Avoid namespace collisions when collapsing graph to custom node #9330

Merged

mjkkirschner merged 9 commits into DynamoDS:master from mjkkirschner:elementResolverNull

Jan 2, 2019

Member

mjkkirschner commented Dec 19, 2018 •

edited

Loading

Purpose

Collapsing a graph to a custom node can create errors if there is a namespace conflict in types used in the generated input nodes.

Adds a new visitor to the shortQualifiedNameReplacer and uses it to replace names when creating custom node symbols when collapsing.

Will cherry pick this to 2.1 branch if no issues found.

Declarations

Check these if you believe they are true

The code base 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.

Reviewers

@aparajit-pratap

FYIs


          this works - but I am hesitant to modify this replacer.

39236d2

Not sure how this effects node2code. - may create rewriter directly.

mjkkirschner added the WIP label

aparajit-pratap reviewed

View reviewed changes

src/DynamoCore/Core/CustomNodeManager.cs

		@@ -674,7 +678,8 @@ internal bool TryGetInfoFromPath(string path, bool isTestMode, out CustomNodeInf
		nodeGraph.ElementResolver,

Contributor

aparajit-pratap Dec 19, 2018

I see the nodeGraph is instantiated from an xml file? Is that why it doesn't get the correct element resolver as we are now using JSON?

Member Author

mjkkirschner Dec 20, 2018

here is where we use the deserialized element resolver when loading a JSON graph and pass it to the home workspace or customNode workspace that was just deserialized.

Dynamo/src/DynamoCore/Graph/Workspaces/SerializationConverters.cs

Line 532 in ec10f93

ws = new CustomNodeWorkspaceModel(factory, nodes, notes, annotations,


          add a new visitor method for typedIdentifierNode

7f08259

use it in custmNodeManager collapse method

mjkkirschner changed the title ~~[WIP] Avoid namespace collisions when generating custom node~~ [WIP] Avoid namespace collisions when collapsing graph to custom node

Member Author

mjkkirschner commented Dec 20, 2018

@aparajit-pratap I've updated this with one potential solution.

aparajit-pratap reviewed

View reviewed changes

src/DynamoCore/Core/CustomNodeManager.cs

+                                              {
+                                                  node.InputSymbol += ":" + typedParameter.Type.Name;
+                                                  this.AsLogger().LogError($"{e.Message}: could not generate a short type name for {typedParameter.Type.Name}");
+                                              }

Contributor

aparajit-pratap Dec 20, 2018

Does this set InputSymbol to the full name even though it is unable to find a short name? If not, it should.

Member Author

mjkkirschner Dec 20, 2018

yep, thats what it does.

aparajit-pratap reviewed

View reviewed changes

src/DynamoCore/Engine/NodeToCode/NodeToCode.cs Outdated

+                              return node;
+                          // build an idlistnode from the full type name
+                          var identListNode = CoreUtils.CreateNodeFromString(node.datatype.Name);

Contributor

aparajit-pratap Dec 20, 2018

We can just pass node.TypeAlias to CreateNodeFromString.

Member Author

mjkkirschner Dec 20, 2018

will try that - thanks.

aparajit-pratap reviewed

View reviewed changes

src/DynamoCore/Engine/NodeToCode/NodeToCode.cs Outdated

+                          // build an idlistnode from the full type name
+                          var identListNode = CoreUtils.CreateNodeFromString(node.datatype.Name);
+                          if (identListNode == null)
+                              return null;

Contributor

aparajit-pratap Dec 20, 2018

It might be safer to return node here just in case it's still valid.

aparajit-pratap reviewed

View reviewed changes

src/DynamoCore/Engine/NodeToCode/NodeToCode.cs Outdated

+                          // visit the idlist
+                          var shortNameNode = identListNode.Accept(this);
+                          if(shortNameNode == null)
+                              return null;

Contributor

aparajit-pratap Dec 20, 2018 •

edited

Loading

It might be safer to return node here just in case it's still valid.

aparajit-pratap reviewed

View reviewed changes

src/DynamoCore/Engine/NodeToCode/NodeToCode.cs Outdated

+                          {
+                              var idNode = (shortNameNode as IdentifierNode);
+                              resultingNode.TypeAlias = idNode.Value;
+                          }

Contributor

aparajit-pratap Dec 20, 2018

I think we can just do: resultingNode.TypeAlias = shortNameNode.ToString();

aparajit-pratap reviewed

View reviewed changes

src/DynamoCore/Engine/NodeToCode/NodeToCode.cs Outdated



		var resultingNode = new TypedIdentifierNode();
		resultingNode.Name = node.Name;

Contributor

aparajit-pratap Dec 20, 2018

var resultingNode = new TypedIdentifierNode{
Name = node.Name,
Value = node.Name,
...
}

Member Author

mjkkirschner Dec 20, 2018

👍

aparajit-pratap reviewed

View reviewed changes

src/DynamoCore/Engine/NodeToCode/NodeToCode.cs Outdated

+                          node.Name = resultingNode.Name;
+                          node.Value = resultingNode.Value;
+                          node.datatype = resultingNode.datatype;
+                          node.TypeAlias = resultingNode.TypeAlias;

Contributor

aparajit-pratap Dec 20, 2018

Does node have to be passed as a ref parameter in order to directly assign it to resultingNode? Is that why you have modified its properties individually here?

Member Author

mjkkirschner Dec 20, 2018

yep, I found that assigning node = resultantNode did not update the node I passed in, and using a ref parameter requires touching the ASTreplacer base class - so this is not ideal, but it does seem to work.

aparajit-pratap reviewed

View reviewed changes

src/DynamoCore/Core/CustomNodeManager.cs Outdated

                                           var funcDesc = dsFunc.Controller.Definition;
                                           parameters = funcDesc.Parameters.ToList();
+                                          // if the node is an instance member the function won't contain a
+                                          // parameter for this type so we need to geneate a new typedParameter.

Contributor

aparajit-pratap Dec 20, 2018

typo: generate

Contributor

aparajit-pratap commented Dec 21, 2018

@mjkkirschner I think this looks good overall. Couple of other comments you could address here. I will look at the test cases you add before we decide to merge this. Thanks for sticking with this and fixing it :)

mjkkirschner added 4 commits

December 21, 2018 14:43


          Merge remote-tracking branch 'upstream/master' into elementResolverNull

e8324b7


          move shortQualifiedNameReplacer

dfa9934

update references
add comments to node2code mutation tests


          add more tests

a59685b

add extra logic to deserialization for symbol node element resolver cache


          revert public API break, keep rewriter in dynamoCore but in new file

eb00720

Member Author

mjkkirschner commented Dec 22, 2018

@aparajit-pratap I've moved the ShortQualifiedNameReplacer to it's own file but kept it in dynamo core because it was used by public methods in dynamoCore (ie ReplaceWithShorestQualifiedName) and I wanted to keep the underlying class internal.

I've added a few tests for shortNameReplacer that test the typedIdentifer visitor and some customNodeCollapse tests that use FFITarget to create conflicts.

I have yet to run all the tests on self serve CI.

mjkkirschner changed the title ~~[WIP] Avoid namespace collisions when collapsing graph to custom node~~ Avoid namespace collisions when collapsing graph to custom node

mjkkirschner added PTAL and removed WIP labels

aparajit-pratap reviewed

View reviewed changes

test/DynamoCoreTests/CustomNodes.cs Outdated

+                          var inputs = CurrentDynamoModel.CurrentWorkspace.Nodes.OfType<Symbol>().ToList();
+                          Assert.IsNotNull(inputs);
+                          var pointParams = inputs;

Contributor

aparajit-pratap Dec 22, 2018

where is pointParams used?

aparajit-pratap reviewed

View reviewed changes

test/DynamoCoreTests/CustomNodes.cs Outdated

+                          Assert.IsTrue(inputs.All(t =>
+                          {
+                              Console.WriteLine(t.InputSymbol);

Contributor

aparajit-pratap Dec 22, 2018

What's the Console.WriteLine for?

aparajit-pratap reviewed

View reviewed changes

test/DynamoCoreTests/CustomNodes.cs Outdated

+                          var inputs = CurrentDynamoModel.CurrentWorkspace.Nodes.OfType<Symbol>().ToList();
+                          Assert.IsNotNull(inputs);
+                          var pointParams = inputs;

Contributor

aparajit-pratap Dec 22, 2018

same here.

Contributor

aparajit-pratap commented Dec 22, 2018

@mjkkirschner excellent! Some minor comments and then LGTM!

aparajit-pratap added the LGTM label

mjkkirschner removed the PTAL label


          Merge remote-tracking branch 'upstream/master' into elementResolverNull

466f588

# Conflicts:
#	src/DynamoCore/Core/CustomNodeManager.cs
#	src/DynamoCore/Engine/NodeToCode/NodeToCode.cs
#	src/DynamoCore/Graph/Nodes/CustomNodes/Function.cs
#	src/DynamoCore/Graph/Workspaces/NodesToCodeExtensions.cs

mjkkirschner added 2 commits

January 2, 2019 11:32


          remove extra usings added previously.

c1a123a


          remove debugging code

mjkkirschner merged commit 74c856e into DynamoDS:master

mjkkirschner mentioned this pull request

avoid namespace collisions when collapsing custom nodes RC 2.1 #9357

Merged

7 tasks

mjkkirschner added a commit to mjkkirschner/Dynamo that referenced this pull request


          Avoid namespace collisions when collapsing graph to custom node (Dyna…

ced9b8d

…moDS#9330)

* this works - but I am hesitant to modify this replacer.
Not sure how this effects node2code. - may create rewriter directly.

* add a new visitor method for typedIdentifierNode
use it in custmNodeManager collapse method

* move shortQualifiedNameReplacer
update references
add comments to node2code mutation tests

* add more tests
add extra logic to deserialization for symbol node element resolver cache

* revert public API break, keep rewriter in dynamoCore but in new file

* remove extra usings added previously.

* remove debugging code

mjkkirschner mentioned this pull request

cherry pick -Avoid namespace collisions when collapsing graph to custom node (#9330) 2.1 RC #9360

Merged

7 tasks

QilongTang pushed a commit that referenced this pull request


          Avoid namespace collisions when collapsing graph to custom node (#9330)…

2351da5

… (#9360)

* this works - but I am hesitant to modify this replacer.
Not sure how this effects node2code. - may create rewriter directly.

* add a new visitor method for typedIdentifierNode
use it in custmNodeManager collapse method

* move shortQualifiedNameReplacer
update references
add comments to node2code mutation tests

* add more tests
add extra logic to deserialization for symbol node element resolver cache

* revert public API break, keep rewriter in dynamoCore but in new file

* remove extra usings added previously.

* remove debugging code

mjkkirschner mentioned this pull request

Element.Id_Crashing when I use node to code #9595

Closed

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

Labels

LGTM