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

* 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

src/DynamoCore/Core/CustomNodeDefinition.cs

@@ -5,6 +5,7 @@
 using Dynamo.Engine.CodeGeneration;
 using Dynamo.Graph.Nodes;
 using Dynamo.Graph.Nodes.CustomNodes;
+using Dynamo.Graph.Workspaces;
 using Dynamo.Library;
 using ProtoCore;
 using ProtoCore.AST.AssociativeAST;
@@ -298,12 +299,10 @@ public CustomNodeInfo(Guid functionId, string name, string category, string desc
             Description = description;
             Path = path;
             IsVisibleInDynamoLibrary = isVisibleInDynamoLibrary;
-
             Category = category;
             if (String.IsNullOrWhiteSpace(Category))
                 Category = Dynamo.Properties.Resources.DefaultCustomNodeCategory;
         }
-
         /// <summary>
         /// Returns custom node unique ID
         /// </summary>
@@ -340,5 +339,12 @@ public CustomNodeInfo(Guid functionId, string name, string category, string desc
         /// If true, then custom node is part of library search.
         /// </summary>
         public bool IsVisibleInDynamoLibrary { get; private set; }
+
+        /// <summary>
+        /// Only valid if IsPackageMember is true.
+        /// Can be used to identify which package 
+        /// requested this CustomNode to load.
+        /// </summary>
+        public PackageInfo PackageInfo { get;  internal set; }
     }
 }

src/DynamoCore/Core/CustomNodeManager.cs

@@ -120,7 +120,7 @@ protected virtual void OnInfoUpdated(CustomNodeInfo obj)
             var handler = InfoUpdated;
             if (handler != null) handler(obj);
         }
-        
+
         /// <summary>
         ///     An event that is fired when a custom node is removed from Dynamo.
         /// </summary>
@@ -129,7 +129,16 @@ protected virtual void OnCustomNodeRemoved(Guid functionId)
         {
             var handler = CustomNodeRemoved;
             if (handler != null) handler(functionId);
+
+        }
+
+        internal event Func<Guid, PackageInfo> RequestCustomNodeOwner;
+
+        private PackageInfo OnRequestCustomNodeOwner(Guid FunctionId)
+        {
+            return RequestCustomNodeOwner?.Invoke(FunctionId);
         }
+
         #endregion
 
         /// <summary>
@@ -342,7 +351,7 @@ public bool AddUninitializedCustomNode(string file, bool isTestMode, out CustomN
         {
             if (TryGetInfoFromPath(file, isTestMode, out info))
             {
-                SetNodeInfo(info, isTestMode);
+                SetNodeInfo(info);
                 return true;
             }
             return false;
@@ -398,7 +407,33 @@ public IEnumerable<CustomNodeInfo> AddUninitializedCustomNodesInPath(string path
             {
                 info.IsPackageMember = isPackageMember;
 
-                SetNodeInfo(info, isTestMode);
+                SetNodeInfo(info);
+                result.Add(info);
+            }
+            return result;
+        }
+
+        /// <summary>
+        ///     Scans the given path for custom node files, retaining their information in the manager for later
+        ///     potential initialization. Should be used when packages load or reload customNodes.
+        /// </summary>
+        /// <param name="path">Path on disk to scan for custom nodes.</param>
+        /// <param name="isTestMode">
+        ///     Flag specifying whether or not this should operate in "test mode".
+        /// </param>
+        /// <param name="PackageInfo">
+        ///     Info about the package that requested this customNode to be loaded or to which the customNode belongs.
+        ///     Is PackageMember property will be true if this property is not null.
+        /// </param>
+        /// <returns></returns>
+        public IEnumerable<CustomNodeInfo> AddUninitializedCustomNodesInPath(string path, bool isTestMode, PackageInfo packageInfo)
+        {
+            var result = new List<CustomNodeInfo>();
+            foreach (var info in ScanNodeHeadersInDirectory(path, isTestMode))
+            {
+                info.IsPackageMember = true;
+                info.PackageInfo = packageInfo;
+                SetNodeInfo(info);
                 result.Add(info);
             }
             return result;
@@ -440,37 +475,80 @@ private IEnumerable<CustomNodeInfo> ScanNodeHeadersInDirectory(string dir, bool
         }
 
         /// <summary>
-        /// Stores the path and function definition without initializing a node.  Overwrites
-        /// the existing NodeInfo if necessary
+        /// Stores the path and function definition without initializing a node.  
+        /// Overwrites the existing NodeInfo if necessary!
         /// </summary>
-        private void SetNodeInfo(CustomNodeInfo newInfo, bool isTestMode)
+        private void SetNodeInfo(CustomNodeInfo newInfo)
         {
             var guids = NodeInfos.Where(x =>
                         {
                             return !string.IsNullOrEmpty(x.Value.Path) &&
                                 string.Compare(x.Value.Path, newInfo.Path, StringComparison.OrdinalIgnoreCase) == 0;
                         }).Select(x => x.Key).ToList();
 
+
             foreach (var guid in guids)
             {
                 NodeInfos.Remove(guid);
             }
 
+            // we need to check with the packageManager that this node if this node is in a package or not - 
+            // currently the package data is lost when the customNode workspace is loaded.
+            // we'll only do this check for customNode infos which don't have a package currently to verify if this
+            // is correct.
+            if(newInfo.IsPackageMember == false)
+            {
+                var owningPackage = this.OnRequestCustomNodeOwner(newInfo.FunctionId);
+
+                //we found a real package.
+                if(owningPackage != null)
+                {
+                    newInfo.IsPackageMember = true;
+                    newInfo.PackageInfo = owningPackage;
+                }
+            }
+
+
             CustomNodeInfo info;
-            if (!isTestMode && NodeInfos.TryGetValue(newInfo.FunctionId, out info))
+            // if the custom node is part of a package make sure it does not overwrite another node
+            if (newInfo.IsPackageMember && NodeInfos.TryGetValue(newInfo.FunctionId, out info))
             {
-                var newInfoPath = Path.GetDirectoryName(newInfo.Path);
+                var newInfoPath = String.IsNullOrEmpty(newInfo.Path) ? string.Empty : Path.GetDirectoryName(newInfo.Path);
                 var infoPath = String.IsNullOrEmpty(info.Path) ? string.Empty : Path.GetDirectoryName(info.Path);
                 var message = string.Format(Resources.MessageCustomNodePackageFailedToLoad,
                     infoPath, newInfoPath);
 
-                var ex = new CustomNodePackageLoadException(newInfoPath, infoPath, message);
-                Log(ex.Message, WarningLevel.Moderate);
+
+                //only try to compare package info if both customNodeInfos have package info.
+                if(info.IsPackageMember && info.PackageInfo != null)
+                {
+                    // if these are different packages raise an error.
+                    // TODO (for now we don't raise an error for different
+                    //versions of the same package, don't want to effect publish new version workflows.
+
+                    if (newInfo.PackageInfo.Name != info.PackageInfo.Name)
+                    {
+                        var ex = new CustomNodePackageLoadException(newInfoPath, infoPath, message);
+                        Log(ex.Message, WarningLevel.Moderate);
+
+                        // Log to notification view extension
+                        Log(ex);
+                        throw ex;
+                    }
+                }
+                   else //(newInfo has owning Package, oldInfo does not)
+                {
+
+                    // This represents the case where a previous info was not from a package, but the current info
+                    // has an owning package.
+                    var looseCustomNodeToPackageMessage = String.Format(Properties.Resources.FunctionDefinitionOverwrittenMessage, newInfo.Name, newInfo.PackageInfo, info.Name);
+
+                    var ex = new CustomNodePackageLoadException(newInfoPath, infoPath, looseCustomNodeToPackageMessage);
+                    Log(ex.Message, WarningLevel.Mild);
+                    Log(ex);
+                }
 
-                // Log to notification view extension
-                Log(ex);
 
-                throw ex;
             }
 
             NodeInfos[newInfo.FunctionId] = newInfo;
@@ -663,7 +741,7 @@ public bool OpenCustomNodeWorkspace(
             if (InitializeCustomNode(
                 workspaceInfo,
                 xmlDoc,
-                out customNodeWorkspace, isTestMode))
+                out customNodeWorkspace))
             {
                 workspace = customNodeWorkspace;
                 return true;
@@ -675,7 +753,7 @@ public bool OpenCustomNodeWorkspace(
         private bool InitializeCustomNode(
             WorkspaceInfo workspaceInfo,
             XmlDocument xmlDoc,
-            out CustomNodeWorkspaceModel workspace, bool isTestMode)
+            out CustomNodeWorkspaceModel workspace)
         {
             // Add custom node definition firstly so that a recursive
             // custom node won't recursively load itself.
@@ -709,21 +787,21 @@ private bool InitializeCustomNode(
                 }
             }
 
-            RegisterCustomNodeWorkspace(newWorkspace, isTestMode);
+            RegisterCustomNodeWorkspace(newWorkspace);
             workspace = newWorkspace;
             return true;
         }
 
-        private void RegisterCustomNodeWorkspace(CustomNodeWorkspaceModel newWorkspace, bool isTestMode = false)
+        private void RegisterCustomNodeWorkspace(CustomNodeWorkspaceModel newWorkspace)
         {
             RegisterCustomNodeWorkspace(
                 newWorkspace,
                 newWorkspace.CustomNodeInfo,
-                newWorkspace.CustomNodeDefinition, isTestMode);
+                newWorkspace.CustomNodeDefinition);
         }
 
         private void RegisterCustomNodeWorkspace(
-            CustomNodeWorkspaceModel newWorkspace, CustomNodeInfo info, CustomNodeDefinition definition, bool isTestMode)
+            CustomNodeWorkspaceModel newWorkspace, CustomNodeInfo info, CustomNodeDefinition definition)
         {
             loadedWorkspaceModels[newWorkspace.CustomNodeId] = newWorkspace;
             SetFunctionDefinition(definition);
@@ -734,13 +812,14 @@ private void RegisterCustomNodeWorkspace(
                 SetFunctionDefinition(newDef);
                 OnDefinitionUpdated(newDef);
             };
-
-            SetNodeInfo(info, isTestMode);
+            
+            SetNodeInfo(info);
 
             newWorkspace.InfoChanged += () =>
             {
                 var newInfo = newWorkspace.CustomNodeInfo;
-                SetNodeInfo(newInfo, isTestMode);
+
+                SetNodeInfo(newInfo);
                 OnInfoUpdated(newInfo);
             };
 
@@ -782,7 +861,7 @@ private bool InitializeCustomNode(
                         info.ID = functionId.ToString();
                         if (migrationManager.ProcessWorkspace(info, xmlDoc, isTestMode, nodeFactory))
                         {
-                            return InitializeCustomNode(info, xmlDoc, out workspace, isTestMode);
+                            return InitializeCustomNode(info, xmlDoc, out workspace);
                         }
                     }
                 }
@@ -791,7 +870,7 @@ private bool InitializeCustomNode(
                     // TODO: Skip Json migration for now
                     WorkspaceInfo.FromJsonDocument(strInput, path, isTestMode, false, AsLogger(), out info);
                     info.ID = functionId.ToString();
-                    return InitializeCustomNode(info, null, out workspace, isTestMode);
+                    return InitializeCustomNode(info, null, out workspace);
                 }
                 else throw ex;
                 Log(string.Format(Properties.Resources.CustomNodeCouldNotBeInitialized, customNodeInfo.Name));
@@ -821,7 +900,7 @@ private bool InitializeCustomNode(
         ///     Optional identifier to be used for the custom node. By default, will make a new unique one.
         /// </param>
         /// <returns>Newly created Custom Node Workspace.</returns>
-        internal WorkspaceModel CreateCustomNode(string name, string category, string description, Guid? functionId = null, bool isTestMode = false)
+        internal WorkspaceModel CreateCustomNode(string name, string category, string description, Guid? functionId = null)
         {
             var newId = functionId ?? Guid.NewGuid();
 
@@ -838,7 +917,7 @@ internal WorkspaceModel CreateCustomNode(string name, string category, string de
             };
             var workspace = new CustomNodeWorkspaceModel(info, nodeFactory);
 
-            RegisterCustomNodeWorkspace(workspace, isTestMode);
+            RegisterCustomNodeWorkspace(workspace);
             return workspace;
         }
 
@@ -1264,7 +1343,7 @@ from output in outputs
 
                 newWorkspace.HasUnsavedChanges = true;
 
-                RegisterCustomNodeWorkspace(newWorkspace, isTestMode);
+                RegisterCustomNodeWorkspace(newWorkspace);
 
                 Debug.WriteLine("Collapsed workspace has {0} nodes and {1} connectors",
                     newWorkspace.Nodes.Count(), newWorkspace.Connectors.Count());

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
     {
         /// <summary>
         /// Name of the package

src/DynamoCore/Properties/Resources.en-US.resx

@@ -706,4 +706,7 @@ Installing it will conflict with one or more node definitions that already exist
 To install {1}, Dynamo needs to first uninstall {0}. 
 Restart Dynamo to complete the uninstall.</value>
   </data>
+  <data name="FunctionDefinitionOverwrittenMessage" xml:space="preserve">
+    <value>Attempting to load customNode {0} loaded by package {1}, but a previous definition named {2} exists with no associated package. The new customNode definition has been loaded, but Dynamo may be in an unstable state, please avoid loading multiple custom nodes with the id.</value>
+  </data>
 </root>

src/DynamoCore/Properties/Resources.resx

@@ -706,4 +706,7 @@ Installing it will conflict with one or more node definitions that already exist
 To install {1}, Dynamo needs to first uninstall {0}. 
 Restart Dynamo to complete the uninstall.</value>
   </data>
+  <data name="FunctionDefinitionOverwrittenMessage" xml:space="preserve">
+    <value>Attempting to load customNode {0} loaded by package {1}, but a previous definition named {2} exists with no associated package. The new customNode definition has been loaded, but Dynamo may be in an unstable state, please avoid loading multiple custom nodes with the id.</value>
+  </data>
 </root>

src/DynamoCoreWpf/Properties/Resources.en-US.resx

@@ -2216,4 +2216,7 @@ Uninstall the following packages: {0}?</value>
   <data name="ProvideFeedbackError" xml:space="preserve">
     <value>Could not re-direct to the Dynamo forum page for feedback:</value>
   </data>
+  <data name="MessageUninstallSamePackage" xml:space="preserve">
+    <value>"The package {0} is already installed. To reinstall it, you must first uninstall it and restart to complete the uninstall. Would you like to mark {0} for uninstall?"</value>
+  </data>
 </root>

src/DynamoCoreWpf/Properties/Resources.resx

@@ -2219,4 +2219,7 @@ Uninstall the following packages: {0}?</value>
   <data name="ProvideFeedbackError" xml:space="preserve">
     <value>Could not re-direct to the Dynamo forum page for feedback:</value>
   </data>
+  <data name="MessageUninstallSamePackage" xml:space="preserve">
+    <value>"The package {0} is already installed. To reinstall it, you must first uninstall it and restart to complete the uninstall. Would you like to mark {0} for uninstall?"</value>
+  </data>
 </root>