8000 Fixing a bug related to ModuleSpec syntax in RequiredModules (#3594) · PowerShell/PowerShell@c0aafdb · GitHub
[go: up one dir, main page]
Skip to content

Commit c0aafdb

Browse files
Andrewdaxian-dbw
Andrew
authored and
daxian-dbw
committed
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.
1 parent 08e8555 commit c0aafdb

File tree

2 files changed

+120
-31
lines changed

2 files changed

+120
-31
lines changed

‎src/System.Management.Automation/engine/Modules/ModuleCmdletBase.cs

Lines changed: 26 additions & 31 deletions
Original file line numberDiff line numberDiff line change
@@ -4103,11 +4103,15 @@ internal static PSModuleInfo LoadRequiredModule(ExecutionContext context,
41034103
Dictionary<ModuleSpecification, List<ModuleSpecification>> requiredModules = new Dictionary<ModuleSpecification, List<ModuleSpecification>>(new ModuleSpecificationComparer());
41044104
if (currentModule != null)
41054105
{
4106-
requiredModules.Add(new ModuleSpecification(currentModule), new List<ModuleSpecification> { requiredModuleSpecification });
4106+
requiredModules.Add(new ModuleSpecification(currentModule), new List<ModuleSpecification> { requiredModuleSpecification });
4107+
}
4108+
if (requiredModuleSpecification != null)
4109+
{
4110+
requiredModules.Add(requiredModuleSpecification, new List<ModuleSpecification>(requiredModuleInfo.RequiredModulesSpecification));
41074111
}
41084112

41094113
// We always need to check against the module name and not the file name
4110-
hasRequiredModulesCyclicReference = HasRequiredModulesCyclicReference(requiredModuleInfo.Name,
4114+
hasRequiredModulesCyclicReference = HasRequiredModulesCyclicReference(requiredModuleSpecification,
41114115
new List<ModuleSpecification>(requiredModuleInfo.RequiredModulesSpecification),
41124116
new Collection<PSModuleInfo> { requiredModuleInfo },
41134117
requiredModules,
@@ -4426,15 +4430,15 @@ internal static Collection<PSModuleInfo> GetModuleIfAvailable(ModuleSpecificatio
44264430
return result;
44274431
}
44284432

4429-
private static bool HasRequiredModulesCyclicReference(string currentModuleName, List<ModuleSpecification> requiredModules, IEnumerable<PSModuleInfo> moduleInfoList, Dictionary<ModuleSpecification, List<ModuleSpecification>> nonCyclicRequiredModules, out ErrorRecord error)
4433+
private static bool HasRequiredModulesCyclicReference(ModuleSpecification currentModuleSpecification, List<ModuleSpecification> requiredModules, IEnumerable<PSModuleInfo> moduleInfoList, Dictionary<ModuleSpecification, List<ModuleSpecification>> nonCyclicRequiredModules, out ErrorRecord error)
44304434
{
44314435
error = null;
4432-
if (requiredModules == null || requiredModules.Count == 0)
4436+
if (requiredModules == null || requiredModules.Count == 0 || currentModuleSpecification == null)
44334437
{
44344438
return false;
44354439
}
44364440

4437-
foreach (var moduleSpecification in requiredModules)
4441+
foreach (var requiredModuleSpecification in requiredModules)
44384442
{
44394443
// The dictionary holds the key-value pair with the following convention
44404444
// Key --> Module
@@ -4447,56 +4451,47 @@ private static bool HasRequiredModulesCyclicReference(string currentModuleName,
44474451

44484452
// Cycle
44494453
// 1 --->2---->3---->4---> 2
4450-
if (nonCyclicRequiredModules.ContainsKey(moduleSpecification))
4454+
if (nonCyclicRequiredModules.ContainsKey(requiredModuleSpecification))
44514455
{
44524456
// Error out saying there is a cyclic reference
44534457
PSModuleInfo mo = null;
44544458
foreach (var i in moduleInfoList)
44554459
{
4456-
if (i.Name.Equals(currentModuleName, StringComparison.OrdinalIgnoreCase))
4460+
if (i.Name.Equals(currentModuleSpecification.Name, StringComparison.OrdinalIgnoreCase))
44574461
{
44584462
mo = i;
44594463
break;
44604464
}
44614465
}
44624466
Dbg.Assert(mo != null, "The moduleInfo should be present");
4463-
string message = StringUtil.Format(Modules.RequiredModulesCyclicDependency, currentModuleName, moduleSpecification.Name, mo.Path);
4467+
string message = StringUtil.Format(Modules.RequiredModulesCyclicDependency, currentModuleSpecification.ToString(), requiredModuleSpecification.ToString(), mo.Path);
44644468
MissingMemberException mm = new MissingMemberException(message);
44654469
error = new ErrorRecord(mm, "Modules_InvalidManifest", ErrorCategory.ResourceUnavailable, mo.Path);
44664470
return true;
44674471
}
4468-
else // Go for the recursive check for the RequiredModules of current module
4472+
else // Go for recursive check for the RequiredModules of current requiredModuleSpecification
44694473
{
4470-
// Add required Modules of m to the list
4471-
Collection<PSModuleInfo> availableModules = GetModuleIfAvailable(moduleSpecification);
4472-
List<ModuleSpecification> list = new List<ModuleSpecification>();
4473-
string moduleName = null;
4474+
Collection<PSModuleInfo> availableModules = GetModuleIfAvailable(requiredModuleSpecification);
44744475
if (availableModules.Count == 1)
44754476
{
4476-
moduleName = availableModules[0].Name;
4477-
foreach (var rm in availableModules[0].RequiredModulesSpecification)
4478-
{
4479-
list.Add(rm);
4480-
}
4481-
// Only add if this element has a required module (meaning, it could lead to a circular reference)
4477+
List<ModuleSpecification> list = new List<ModuleSpecification>(availableModules[0].RequiredModulesSpecification);
4478+
// Only add if this required module has nested required modules (meaning, it could lead to a circular reference)
44824479
if (list.Count > 0)
44834480
{
4484-
nonCyclicRequiredModules.Add(moduleSpecification, list);
4485-
}
4486-
}
4487-
4488-
// We always need to check against the module name and not the file name
4489-
if (HasRequiredModulesCyclicReference(moduleName, list, availableModules, nonCyclicRequiredModules, out error))
4490-
{
4491-
return true;
4481+
nonCyclicRequiredModules.Add(requiredModuleSpecification, list);
4482+
// We always need to check against the module specification and not the file name
4483+
if (HasRequiredModulesCyclicReference(requiredModuleSpecification, list, availableModules, nonCyclicRequiredModules, out error))
4484+
{
4485+
return true;
4486+
}
4487+
}
44924488
}
44934489
}
44944490
}
44954491

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

‎test/powershell/engine/Module/TestModuleManifest.Tests.ps1

Lines changed: 94 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -125,3 +125,97 @@ Describe "Test-ModuleManifest tests" -tags "CI" {
125125
{ Test-ModuleManifest -Path $testModulePath -ErrorAction Stop } | ShouldBeErrorId "$error,Microsoft.PowerShell.Commands.TestModuleManifestCommand"
126126
}
127127
}
128+
129+
130+
Describe "Tests for circular references in required modules" -tags "CI" {
131+
132+
function CreateTestModules([string]$RootPath, [string[]]$ModuleNames, [bool]$AddVersion, [bool]$AddGuid, [bool]$AddCircularReference)
133+
{
134+
$RequiredModulesSpecs = @();
135+
foreach($moduleDir in New-Item $ModuleNames -ItemType Directory -Force)
136+
{
137+
if ($lastItem)
138+
{
139+
if ($AddVersion -or $AddGuid) {$RequiredModulesSpecs += $lastItem}
140+
else {$RequiredModulesSpecs += $lastItem.ModuleName}
141+
}
142+
143+
$ModuleVersion = '3.0'
144+
$GUID = New-Guid
145+
146+
New-ModuleManifest ((join-path $moduleDir.Name $moduleDir.Name) + ".psd1") -RequiredModules $RequiredModulesSpecs -ModuleVersion $ModuleVersion -Guid $GUID
147+
148+
$lastItem = @{ ModuleName = $moduleDir.Name}
149+
if ($AddVersion) {$lastItem += @{ ModuleVersion = $ModuleVersion}}
150+
if ($AddGuid) {$lastItem += @{ GUID = $GUID}}
151+
}
152+
153+
if ($AddCircularReference)
154+
{
155+
# rewrite first module's manifest to have a reference to the last module, i.e. making a circular reference
156+
if ($AddVersion -or $AddGuid)
157+
{
158+
$firstModuleName = $RequiredModulesSpecs[0].ModuleName
159+
$firstModuleVersion = $RequiredModulesSpecs[0].ModuleVersion
160+
$firstModuleGuid = $RequiredModulesSpecs[0].GUID
161+
$RequiredModulesSpecs = $lastItem
162+
}
163+
else
164+
{
165+
$firstModuleName = $RequiredModulesSpecs[0]
166+
$firstModuleVersion = '3.0' # does not matter - not used in references
167+
$firstModuleGuid = New-Guid # does not matter - not used in references
168+
$RequiredModulesSpecs = $lastItem.ModuleName
169+
}
170+
171+
New-ModuleManifest ((join-path $firstModuleName $firstModuleName) + ".psd1") -RequiredModules $RequiredModulesSpecs -ModuleVersion $firstModuleVersion -Guid $firstModuleGuid
172+
}
173+
}
174+
175+
function TestImportModule([bool]$AddVersion, [bool]$AddGuid, [bool]$AddCircularReference)
176+
{
177+
$moduleRootPath = Join-Path $TestDrive 'TestModules'
178+
New-Item $moduleRootPath -ItemType Directory -Force
179+
Push-Location $moduleRootPath
180+
181+
$moduleCount = 6 # this depth was enough to find a bug in cyclic reference detection product code; greater depth will slow tests down
182+
$ModuleNames = 1..$moduleCount|%{"TestModule$_"}
183+
184+
CreateTestModules $moduleRootPath $ModuleNames $AddVersion $AddGuid $AddCircularReference
185+
186+
$newpath = [system.io.path]::PathSeparator + "$moduleRootPath"
187+
$OriginalPSModulePathLength = $env:PSModulePath.Length
188+
$env:PSModulePath += $newpath
189+
$lastModule = $ModuleNames[$moduleCount - 1]
190+
191+
try
192+
{
193+
Import-Module $lastModule -ErrorAction Stop
194+
Get-Module $lastModule | Should Not BeNullOrEmpty
195+
}
196+
finally
197+
{
198+
#cleanup
199+
Remove-Module $ModuleNames -Force -ErrorAction SilentlyContinue
200+
$env:PSModulePath = $env:PSModulePath.Substring(0,$OriginalPSModulePathLength)
201+
Pop-Location
202+
Remove-Item $moduleRootPath -Recurse -Force -ErrorAction SilentlyContinue
203+
}
204+
}
205+
206+
It "No circular references and RequiredModules field has only module names" {
207+
TestImportModule $false $false $false
208+
}
209+
210+
It "No circular references and RequiredModules field has module names and versions" {
211+
TestImportModule $true $false $false
212+
}
213+
214+
It "No circular references and RequiredModules field has module names, versions and GUIDs" {
215+
TestImportModule $true $true $false
216+
}
217+
218+
It "Add a circular reference to RequiredModules and verify error" {
219+
{ TestImportModule $false $false $true } | ShouldBeErrorId "Modules_InvalidManifest,Microsoft.PowerShell.Commands.ImportModuleCommand"
220+
}
221+
}

0 commit comments

Comments
 (0)
0