Fixing a bug related to ModuleSpec syntax in RequiredModules (#3594)

This fixes issue #2607. 'RequiredModules' is a field in module manifest that can reference other modules using ModuleSpecification format. The basic version of this format (just module name) was working fine, however, there was a problem when a more detailed version of the format was used (the one that uses module versions or/and GUIDs). During module import, there is a check for cyclic references through 'RequiredModules' field. The bug was in this check for cyclic references, related to comparison rules for ModuleSpecification objects - as a result, the code was incorrectly reporting 'cyclic reference' error in cases when there was none. Also, added tests for different ModuleSpecification formats and a test for error when there is actually a cyclic reference.
2026-06-08 12:12:50 +00:00 · 2017-04-30 17:58:44 -07:00
parent 08e855556c
commit c0aafdb3d0
2 changed files with 120 additions and 31 deletions
@@ -4103,11 +4103,15 @@ namespace Microsoft.PowerShell.Commands
                    Dictionary<ModuleSpecification, List<ModuleSpecification>> requiredModules = new Dictionary<ModuleSpecification, List<ModuleSpecification>>(new ModuleSpecificationComparer());
                    if (currentModule != null)
                    {
-                        requiredModules.Add(new ModuleSpecification(currentModule), new List<ModuleSpecification> { requiredModuleSpecification });
+                        requiredModules.Add(new ModuleSpecification(currentModule), new List<ModuleSpecification> { requiredModuleSpecification });
+                    }
+                    if (requiredModuleSpecification != null)
+                    {
+                        requiredModules.Add(requiredModuleSpecification, new List<ModuleSpecification>(requiredModuleInfo.RequiredModulesSpecification));
                    }

                    // We always need to check against the module name and not the file name
-                    hasRequiredModulesCyclicReference = HasRequiredModulesCyclicReference(requiredModuleInfo.Name,
+                    hasRequiredModulesCyclicReference = HasRequiredModulesCyclicReference(requiredModuleSpecification,
                                                                                          new List<ModuleSpecification>(requiredModuleInfo.RequiredModulesSpecification),
                                                                                          new Collection<PSModuleInfo> { requiredModuleInfo },
                                                                                          requiredModules,
@@ -4426,15 +4430,15 @@ namespace Microsoft.PowerShell.Commands
            return result;
        }

-        private static bool HasRequiredModulesCyclicReference(string currentModuleName, List<ModuleSpecification> requiredModules, IEnumerable<PSModuleInfo> moduleInfoList, Dictionary<ModuleSpecification, List<ModuleSpecification>> nonCyclicRequiredModules, out ErrorRecord error)
+        private static bool HasRequiredModulesCyclicReference(ModuleSpecification currentModuleSpecification, List<ModuleSpecification> requiredModules, IEnumerable<PSModuleInfo> moduleInfoList, Dictionary<ModuleSpecification, List<ModuleSpecification>> nonCyclicRequiredModules, out ErrorRecord error)
        {
            error = null;
-            if (requiredModules == null || requiredModules.Count == 0)
+            if (requiredModules == null || requiredModules.Count == 0 || currentModuleSpecification == null)
            {
                return false;
            }

-            foreach (var moduleSpecification in requiredModules)
+            foreach (var requiredModuleSpecification in requiredModules)
            {
                // The dictionary holds the key-value pair with the following convention
                // Key --> Module
@@ -4447,56 +4451,47 @@ namespace Microsoft.PowerShell.Commands

                // Cycle
                // 1 --->2---->3---->4---> 2
-                if (nonCyclicRequiredModules.ContainsKey(moduleSpecification))
+                if (nonCyclicRequiredModules.ContainsKey(requiredModuleSpecification))
                {
                    // Error out saying there is a cyclic reference
                    PSModuleInfo mo = null;
                    foreach (var i in moduleInfoList)
                    {
-                        if (i.Name.Equals(currentModuleName, StringComparison.OrdinalIgnoreCase))
+                        if (i.Name.Equals(currentModuleSpecification.Name, StringComparison.OrdinalIgnoreCase))
                        {
                            mo = i;
                            break;
                        }
                    }
                    Dbg.Assert(mo != null, "The moduleInfo should be present");
-                    string message = StringUtil.Format(Modules.RequiredModulesCyclicDependency, currentModuleName, moduleSpecification.Name, mo.Path);
+                    string message = StringUtil.Format(Modules.RequiredModulesCyclicDependency, currentModuleSpecification.ToString(), requiredModuleSpecification.ToString(), mo.Path);
                    MissingMemberException mm = new MissingMemberException(message);
                    error = new ErrorRecord(mm, "Modules_InvalidManifest", ErrorCategory.ResourceUnavailable, mo.Path);
                    return true;
                }
-                else // Go for the recursive check for the RequiredModules of current module
+                else // Go for recursive check for the RequiredModules of current requiredModuleSpecification
                {
-                    // Add required Modules of m to the list
-                    Collection<PSModuleInfo> availableModules = GetModuleIfAvailable(moduleSpecification);
-                    List<ModuleSpecification> list = new List<ModuleSpecification>();
-                    string moduleName = null;
+                    Collection<PSModuleInfo> availableModules = GetModuleIfAvailable(requiredModuleSpecification);
                    if (availableModules.Count == 1)
                    {
-                        moduleName = availableModules[0].Name;
-                        foreach (var rm in availableModules[0].RequiredModulesSpecification)
-                        {
-                            list.Add(rm);
-                        }
-                        // Only add if this element has a required module (meaning, it could lead to a circular reference)
+                        List<ModuleSpecification> list = new List<ModuleSpecification>(availableModules[0].RequiredModulesSpecification);
+                        // Only add if this required module has nested required modules (meaning, it could lead to a circular reference)
                        if (list.Count > 0)
                        {
-                            nonCyclicRequiredModules.Add(moduleSpecification, list);
-                        }
-                    }
-
-                    // We always need to check against the module name and not the file name
-                    if (HasRequiredModulesCyclicReference(moduleName, list, availableModules, nonCyclicRequiredModules, out error))
-                    {
-                        return true;
+                            nonCyclicRequiredModules.Add(requiredModuleSpecification, list);
+                            // We always need to check against the module specification and not the file name
+                            if (HasRequiredModulesCyclicReference(requiredModuleSpecification, list, availableModules, nonCyclicRequiredModules, out error))
+                            {
+                                return true;
+                            }
+                        }
                    }
                }
            }

-            // Once depth recursive returns a value, we should remove the current module from the CyclicRequiredModules check list.
-            // This prevent non related modules get involved in the cycle list.
-            ModuleSpecification currentModule = new ModuleSpecification(currentModuleName);
-            nonCyclicRequiredModules.Remove(currentModule);
+            // Once nested recursive calls are complete, we should remove the current module from the nonCyclicRequiredModules check list.
+            // This prevents non related modules from getting involved in the cycle list.
+            nonCyclicRequiredModules.Remove(currentModuleSpecification); // this uses ModuleSpecificationComparer equality comparer
            return false;
        }

@@ -125,3 +125,97 @@ Describe "Test-ModuleManifest tests" -tags "CI" {
        { Test-ModuleManifest -Path $testModulePath -ErrorAction Stop } | ShouldBeErrorId "$error,Microsoft.PowerShell.Commands.TestModuleManifestCommand"
    }
 }
+
+
+Describe "Tests for circular references in required modules" -tags "CI" {
+
+    function CreateTestModules([string]$RootPath, [string[]]$ModuleNames, [bool]$AddVersion, [bool]$AddGuid, [bool]$AddCircularReference)
+    {
+        $RequiredModulesSpecs = @();
+        foreach($moduleDir in New-Item $ModuleNames -ItemType Directory -Force)
+        {
+            if ($lastItem)
+            {
+                if ($AddVersion -or $AddGuid) {$RequiredModulesSpecs += $lastItem}
+                else {$RequiredModulesSpecs += $lastItem.ModuleName}
+            }
+
+            $ModuleVersion = '3.0'
+            $GUID = New-Guid
+
+            New-ModuleManifest ((join-path $moduleDir.Name $moduleDir.Name) + ".psd1") -RequiredModules $RequiredModulesSpecs -ModuleVersion $ModuleVersion -Guid $GUID
+
+            $lastItem = @{ ModuleName = $moduleDir.Name}
+            if ($AddVersion) {$lastItem += @{ ModuleVersion = $ModuleVersion}}
+            if ($AddGuid) {$lastItem += @{ GUID = $GUID}}
+        }
+
+        if ($AddCircularReference)
+        {
+            # rewrite first module's manifest to have a reference to the last module, i.e. making a circular reference
+            if ($AddVersion -or $AddGuid)
+            {
+                $firstModuleName = $RequiredModulesSpecs[0].ModuleName
+                $firstModuleVersion = $RequiredModulesSpecs[0].ModuleVersion
+                $firstModuleGuid = $RequiredModulesSpecs[0].GUID
+                $RequiredModulesSpecs = $lastItem
+            }
+            else
+            {
+                $firstModuleName = $RequiredModulesSpecs[0]
+                $firstModuleVersion = '3.0' # does not matter - not used in references
+                $firstModuleGuid = New-Guid # does not matter - not used in references
+                $RequiredModulesSpecs = $lastItem.ModuleName
+            }
+
+            New-ModuleManifest ((join-path $firstModuleName $firstModuleName) + ".psd1") -RequiredModules $RequiredModulesSpecs -ModuleVersion $firstModuleVersion -Guid $firstModuleGuid
+        }
+    }
+
+    function TestImportModule([bool]$AddVersion, [bool]$AddGuid, [bool]$AddCircularReference)
+    {
+        $moduleRootPath = Join-Path $TestDrive 'TestModules'
+        New-Item $moduleRootPath -ItemType Directory -Force
+        Push-Location $moduleRootPath
+
+        $moduleCount = 6 # this depth was enough to find a bug in cyclic reference detection product code; greater depth will slow tests down
+        $ModuleNames = 1..$moduleCount|%{"TestModule$_"}
+
+        CreateTestModules $moduleRootPath $ModuleNames $AddVersion $AddGuid $AddCircularReference
+        
+        $newpath = [system.io.path]::PathSeparator + "$moduleRootPath"
+        $OriginalPSModulePathLength = $env:PSModulePath.Length
+        $env:PSModulePath += $newpath
+        $lastModule = $ModuleNames[$moduleCount - 1]
+
+        try
+        {
+            Import-Module $lastModule -ErrorAction Stop
+            Get-Module $lastModule | Should Not BeNullOrEmpty
+        }
+        finally
+        {
+            #cleanup
+            Remove-Module $ModuleNames -Force -ErrorAction SilentlyContinue
+            $env:PSModulePath = $env:PSModulePath.Substring(0,$OriginalPSModulePathLength)
+            Pop-Location
+            Remove-Item $moduleRootPath -Recurse -Force -ErrorAction SilentlyContinue
+        }
+    }
+
+    It "No circular references and RequiredModules field has only module names" {
+        TestImportModule $false $false $false
+    }
+
+    It "No circular references and RequiredModules field has module names and versions" {
+        TestImportModule $true $false $false
+    }
+
+    It "No circular references and RequiredModules field has module names, versions and GUIDs" {
+        TestImportModule $true $true $false
+    }
+
+    It "Add a circular reference to RequiredModules and verify error" {
+        { TestImportModule $false $false $true } | ShouldBeErrorId "Modules_InvalidManifest,Microsoft.PowerShell.Commands.ImportModuleCommand"
+    }
+}