Package B should not overwrite CustomNodes defined in Package A #9841

mjkkirschner · 2019-07-11T22:23:13Z

Purpose

Updated:

After going back and forth with this for a few days my opinion is that we should move very slowly with this change because the customNodeManager and package loading logic is quite fragile and has many cases. Some of which are not well defined today. I think before we can change these we need to define them and the expected behavior.

Having said that here is what I'm proposing and what I have found.

I propose that at this time we only change one loading behavior to address the initial bug we identified using the WorkspaceDependencyViewer that one node could belong to two packages.

After this PR:

CustomNodes cannot be redefined by a completely separate package.
If you try to load two conflicting packages you see this message(added by @aparajit-pratap ):

 {package2} cannot be loaded. 
        ///Installing it will conflict with one or more node definitions that already exist in {package1}, which is currently loaded. 
        ///To install {package2}, Dynamo needs to first uninstall {package1}. 
        ///Restart Dynamo to complete the uninstall..

this means they CAN be redefined by other versions of the same package. This is to support versioning publishing workflows. Until we are ready to refactor and study these cases and the inconsistency that exists today I have added a notification which is raised if an existing definition or version is being overridden by a package.

Attempting to load customNode EvenOdd, loaded by package EvenOdd, 
Version=1.0.0, but A previous definition named MyNewEvenOdd exists with no associated package. 
The new customNode definition has been loaded,
 but Dynamo may in an unstable state, please avoid loading multiple custom nodes with the id 3f19c484-d3f3-49ba-88c2-6c386a41f6ac.

These are the only cases which are changed, and the only one that prevents loading is number 1. - I think this has low risk since most behaviors are unchanged, and provides some more information to help users understand what is being overwritten and by what.

These are the cases I was able to write automated tests for:

CustomNode is loose - edited by a user, and resaved
CustomNode is from a package and edited by a user.
Two packages contain the same custom node definition, a node is placed from package 1, and package 2 attempts to load - conflicting package2 does not load.
Placing a custom node from a package retains the packageInfo on the CustomNodeInfo

These are the cases I could not write tests for, but work in Dynamo:

Custom node with conflicting ID is in definitions folder, Package containing same ID is loaded, package overwrites customNode and logs a warning.
The reverse of the above - loading the package first and then creating the customNode -

These are cases that are not well defined or have surprising behavior that I am hesitant to change.

Publishing a new customNode to a package does not actually load the customNodes again, but it does move the custom node to a new location - the package is loaded, but contains no customNodes - this is existing behavior.
Publishing a version does reload the custom nodes.

PackageInfo added to CustomNodeInfo

To handle these cases the PackageInfo class was made public and added to the CusotmNodeInfo class as a property - this property is valid if the customNode was requested to be loaded as a consequence of some package being loaded.

I found out that IsPackageMember was never consistently set on the CustomNodeInfo - ⚠️ I believe this is the information we absolutely must know to address case 1. (packages overwriting other packages) - To address this when CustomNodeInfo is set the CustomNodeManager requests the packageManager return the owningPackage of the customNode if it exists, this is done with a dictionary lookup, so it is fast.

Next we check the updated info is owned by a package, if it is, we check if the same package added the existing customNodeInfo - if so, we update the info. If the existing Info has no package - we log a notification (the one above in case 2), and still load the package.

The only case that throws, is case 1. Two different packages (tested by Name only) are trying to redefine the same info tested by Guid.

PR Also does the following:

removes testMode added in previous PR
simplifies message when trying to install a package that is already installed and requires restart.
removes failure category from LibraryJSCustomNodetest.
adds some Failing tests, with the category TechDebt that I hope we can address as we define the expected behavior of these other cases and eventually refactor CustomNodeManager / PackageLoader.

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

(FILL ME IN) Reviewer 1 (If possible, assign the Reviewer for the PR)

(FILL ME IN, optional) Any additional notes to reviewers or testers.

FYIs

@aparajit-pratap @scottmitchell @QilongTang

fix tests pass PackageInfo around add PackageInfo to customNodeInfo class use it to throw exceptions change message when installing package that already is installed.

src/DynamoCore/Core/CustomNodeManager.cs

QilongTang · 2019-07-12T18:36:39Z

src/DynamoCore/Graph/Workspaces/PackageDependencyInfo.cs

@@ -6,7 +6,7 @@ namespace Dynamo.Graph.Workspaces
    /// <summary>
    /// Class containing info about a package
    /// </summary>
-    internal class PackageInfo
+    public class PackageInfo


Good, we still end up making it public. Would you check if there is any internal available code we need to remove?

QilongTang · 2019-07-12T18:37:19Z

src/DynamoCore/Core/CustomNodeManager.cs

+        public IEnumerable<CustomNodeInfo> AddUninitializedCustomNodesInPath(string path, bool isTestMode, PackageInfo packageInfo)
+        {
+            var result = new List<CustomNodeInfo>();
+            foreach (var info in ScanNodeHeadersInDirectory(path, isTestMode))


Without checking the function ScanNodeHeadersInDirectory can you tell us a bit why testMode is needed here?

so because this function results in loading something from disk, its needed to be passed down to the xml and json deserialization methods:

https://github.com/DynamoDS/Dynamo/blob/master/src/DynamoCore/Core/CustomNodeManager.cs#L601

eventually... it's used to fail tests, which is a good thing (instead of logging the exceptions like we do at runtime)

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

Line 245 in ec10f93

if (isTestMode)

src/DynamoCore/Core/CustomNodeManager.cs

aparajit-pratap · 2019-07-12T19:39:18Z

@mjkkirschner there are few other places where you need to make changes to account for the isTestMode removal - mainly in the tests. Please refer to commit: 1c38933

aparajit-pratap · 2019-07-12T19:50:53Z

Aside from the comments above, looks good. Thanks @mjkkirschner

finish removing test mode add new package

QilongTang · 2019-07-13T05:08:23Z

Some comments there, with Unit tests should look good

mjkkirschner · 2019-07-13T13:52:36Z

Unfortunately during testing I found 2 issues- 1. Customnodes often have their info reset and loose the package info and member properties - this is because they use data from the workspace only - I’m looking at solutions for this and already have one, but I’d like to use the new logic added to package loader to check if the cn belongs to a package to be more accurate. 2. More cases to test - involving the transition from custom node to package member ie when publishing a package or version of a package.

…

On Jul 13, 2019, at 1:08 AM, Aaron (Qilong) ***@***.***> wrote: Some comments there, with Unit tests should look good — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or mute the thread.

…Info objects use correct customNodeLoading method from Package object when publishing a package update test

aparajit-pratap · 2019-07-15T00:46:28Z

test/DynamoCoreWpfTests/WorkspaceSaving.cs

+            Assert.AreEqual(2, nodeInstance.OutPorts.Count());
+            Assert.AreEqual("anewoutput", nodeInstance.OutPorts[1].Name);
+
+        }


Nice test 👍

test/DynamoCoreTests/CustomNodes.cs

mjkkirschner · 2019-07-15T00:53:30Z

@aparajit-pratap @QilongTang - you may want to hang on before reviewing this again - I will redo my PR description as my thinking on this has changed a bit.

src/DynamoCore/Core/CustomNodeManager.cs

src/DynamoPackages/Package.cs

aparajit-pratap · 2019-07-15T01:11:08Z

@mjkkirschner given the new findings from your testing and the need to add these new events, I feel that adding the PackageInfo property to CustomNodeInfo has added more complexity. I'm trying to understand if it was really worth doing this and what are the advantages other than package equality is done better using PackageInfo objects rather than CustomNodeInfo.Path strings.

mjkkirschner · 2019-07-15T01:27:30Z

@aparajit-pratap - yep we can discuss more tomorrow, I do not think the path information alone gives us enough to make the correct determination, some examples:

if I work on a package and publish a version we can't identify this case - (granted I'm not raising any errors now for this case) - but it will not be easily possible to identify it from paths.
I am working on a new loose custom node, then I publish it (potentially it gets reloaded from a new path) -
The packageMember flag is incorrectly set as soon as you place a node - so the check in your followup PR will not work after interacting with Dynamo, it will only work on first load.

Alot of these cases are tricky no matter if we use paths or packageInfo - the crux being that the PackageMember flag was never set consistently,

tldr;
No matter if we use paths or packageInfo - we will have to do extra work to keep the flag indicating package membership in sync - see the latest commits using RequestCustomNodeOwner - I don't think the packageInfo was the complicated part, it's just that we don't have good information on if a customNode belongs to a package today, and to identify the case we are trying to solve, we need to know this accurately.

…m node address some review comments add some tech debt test cases

aparajit-pratap · 2019-07-15T18:47:01Z

src/DynamoCore/Core/CustomNodeManager.cs

-                if (newInfo.PackageInfo.Name != info.PackageInfo.Name)
+
+                //only try to compare package info if both customNodeInfos have package info.
+                if(info.IsPackageMember && info.PackageInfo != null)


Doesn't one being true mean the other is true - if IsPackageMember is true, doesn't it imply that PackageInfo is non-null or is this double check just to be safe?

yes, it's just a double check, the second check should always be true if the first is true - but I wanted to verify.

src/DynamoCore/Core/CustomNodeManager.cs

aparajit-pratap · 2019-07-15T19:14:20Z

test/DynamoCoreWpfTests/PublishPackageViewModelTests.cs

+
+            Assert.AreEqual(new PackageInfo("PublishingACustomNodeSetsPackageInfoCorrectly", new Version(0,0,1))
+                ,GetModel().CustomNodeManager.NodeInfos[cnworkspace.CustomNodeId].PackageInfo);
+            Assert.IsFalse(GetModel().CustomNodeManager.NodeInfos[cnworkspace.CustomNodeId].IsPackageMember);


Is this not expected to be true?

this is currently false, because the new package does not actually get loaded. - It just gets added to the list of loaded packages... not exactly the same thing. Thats why this one is tech debt, my expectations were not what I found.

aparajit-pratap · 2019-07-15T19:31:43Z

test/Libraries/PackageManagerTests/PackageLoaderTests.cs

+            this.CurrentDynamoModel.CurrentWorkspace.AddAndRegisterNode(customNodeInstance);
+
+            //load again.
+            loader.LoadPackages(new Package[] { package1, package2 });


Why are you loading the packages again here? One of them is already loaded so it's not going to load again, is it? The following assertions are going to work even without this LoadPackages call again, won't they?

I want to try loading package2 again - after the customNodeInstance is placed - because this action resets the customNodeInfo to not include the package info - so I want to verify that loading package2 (which was not loaded the first time) - is still not loaded even after creating an instance of this custom node.

I could be more explicit and not try to load package1 again for clarity.

aparajit-pratap · 2019-07-15T21:05:35Z

LGTM!

* remove testMode fix tests pass PackageInfo around add PackageInfo to customNodeInfo class use it to throw exceptions change message when installing package that already is installed. * add new tests finish removing test mode add new package * update test * update test and package json * try using package manger to keep packageInfo up to date on customNodeInfo objects use correct customNodeLoading method from Package object when publishing a package update test * add logging for different case to setInfo * update message for feedback when package overwrites non package custom node address some review comments add some tech debt test cases

remove testMode

0a0dfaa

fix tests pass PackageInfo around add PackageInfo to customNodeInfo class use it to throw exceptions change message when installing package that already is installed.

mjkkirschner changed the title ~~CustomNodes should be allowed to be redefined in certain cases.~~ [WIP]CustomNodes should be allowed to be redefined in certain cases. Jul 11, 2019

mjkkirschner added the WIP label Jul 11, 2019

QilongTang reviewed Jul 12, 2019

View reviewed changes

src/DynamoCore/Core/CustomNodeManager.cs Show resolved Hide resolved

QilongTang reviewed Jul 12, 2019

View reviewed changes

src/DynamoCore/Core/CustomNodeManager.cs Show resolved Hide resolved

QilongTang reviewed Jul 12, 2019

View reviewed changes

aparajit-pratap reviewed Jul 12, 2019

View reviewed changes

src/DynamoCore/Core/CustomNodeManager.cs Show resolved Hide resolved

mjkkirschner added 3 commits July 12, 2019 19:57

add new tests

f1f9343

finish removing test mode add new package

update test

11e242f

update test and package json

aff3903

mjkkirschner added the DNM Do not merge. For PRs. label Jul 14, 2019

mjkkirschner added 2 commits July 14, 2019 20:20

try using package manger to keep packageInfo up to date on customNode…

8ab2a0e

…Info objects use correct customNodeLoading method from Package object when publishing a package update test

add logging for different case to setInfo

3cac2a0

aparajit-pratap reviewed Jul 15, 2019

View reviewed changes

test/DynamoCoreTests/CustomNodes.cs Show resolved Hide resolved

aparajit-pratap reviewed Jul 15, 2019

View reviewed changes

src/DynamoCore/Core/CustomNodeManager.cs Outdated Show resolved Hide resolved

aparajit-pratap reviewed Jul 15, 2019

View reviewed changes

src/DynamoPackages/Package.cs Outdated Show resolved Hide resolved

update message for feedback when package overwrites non package custo…

fc4ad3d

…m node address some review comments add some tech debt test cases

mjkkirschner changed the title ~~[WIP]CustomNodes should be allowed to be redefined in certain cases.~~ PTAL CustomNodes should be allowed to be redefined in certain cases. Jul 15, 2019

mjkkirschner changed the title ~~PTAL CustomNodes should be allowed to be redefined in certain cases.~~ CustomNodes should be allowed to be redefined in certain cases. Jul 15, 2019

mjkkirschner changed the title ~~CustomNodes should be allowed to be redefined in certain cases.~~ Package B should not overwrite CustomNodes defined in Package A Jul 15, 2019

mjkkirschner removed the DNM Do not merge. For PRs. label Jul 15, 2019

mjkkirschner added PTAL Please Take A Look 👀 and removed WIP labels Jul 15, 2019

aparajit-pratap reviewed Jul 15, 2019

View reviewed changes

src/DynamoCore/Core/CustomNodeManager.cs Outdated Show resolved Hide resolved

aparajit-pratap reviewed Jul 15, 2019

View reviewed changes

src/DynamoCore/Core/CustomNodeManager.cs Outdated Show resolved Hide resolved

aparajit-pratap reviewed Jul 15, 2019

View reviewed changes

update resx with message

f853b16

mjkkirschner merged commit 8d47577 into DynamoDS:master Jul 16, 2019

mjkkirschner mentioned this pull request Jul 16, 2019

cherry pick package B should not overwrite package A to 2.3 #9846

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Package B should not overwrite CustomNodes defined in Package A #9841

Package B should not overwrite CustomNodes defined in Package A #9841

mjkkirschner commented Jul 11, 2019 •

edited

Loading

QilongTang Jul 12, 2019

mjkkirschner Jul 12, 2019

QilongTang Jul 12, 2019

mjkkirschner Jul 12, 2019

aparajit-pratap commented Jul 12, 2019

aparajit-pratap commented Jul 12, 2019

QilongTang commented Jul 13, 2019

mjkkirschner commented Jul 13, 2019 via email

aparajit-pratap Jul 15, 2019

mjkkirschner commented Jul 15, 2019

aparajit-pratap commented Jul 15, 2019

mjkkirschner commented Jul 15, 2019

aparajit-pratap Jul 15, 2019

mjkkirschner Jul 15, 2019 •

edited

Loading

aparajit-pratap Jul 15, 2019

mjkkirschner Jul 15, 2019 •

edited

Loading

aparajit-pratap Jul 15, 2019

mjkkirschner Jul 15, 2019

mjkkirschner Jul 15, 2019

aparajit-pratap commented Jul 15, 2019

Package B should not overwrite CustomNodes defined in Package A #9841

Package B should not overwrite CustomNodes defined in Package A #9841

Conversation

mjkkirschner commented Jul 11, 2019 • edited Loading

Purpose

Updated:

After this PR:

These are the cases I was able to write automated tests for:

These are the cases I could not write tests for, but work in Dynamo:

These are cases that are not well defined or have surprising behavior that I am hesitant to change.

PackageInfo added to CustomNodeInfo

Declarations

Reviewers

FYIs

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aparajit-pratap commented Jul 12, 2019

aparajit-pratap commented Jul 12, 2019

QilongTang commented Jul 13, 2019

mjkkirschner commented Jul 13, 2019 via email

Choose a reason for hiding this comment

mjkkirschner commented Jul 15, 2019

aparajit-pratap commented Jul 15, 2019

mjkkirschner commented Jul 15, 2019

Choose a reason for hiding this comment

mjkkirschner Jul 15, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mjkkirschner Jul 15, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aparajit-pratap commented Jul 15, 2019

mjkkirschner commented Jul 11, 2019 •

edited

Loading

mjkkirschner Jul 15, 2019 •

edited

Loading

mjkkirschner Jul 15, 2019 •

edited

Loading