Skip to content

Fixing bug #2607 related to ModuleSpec syntax in RequiredModules #3594

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

Merged
daxian-dbw merged 2 commits into from
May 1, 2017
Merged

Fixing bug #2607 related to ModuleSpec syntax in RequiredModules #3594

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
bad1221
PR feedback and resolved merge conflict
Apr 28, 2017
64b71d1
Fixed test failure on Linux
Apr 28, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
57 changes: 26 additions & 31 deletions src/System.Management.Automation/engine/Modules/ModuleCmdletBase.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4103,11 +4103,15 @@ internal static PSModuleInfo LoadRequiredModule(ExecutionContext context,
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,
Expand Down Expand Up @@ -4426,15 +4430,15 @@ internal static Collection<PSModuleInfo> GetModuleIfAvailable(ModuleSpecificatio
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
Expand All @@ -4447,56 +4451,47 @@ private static bool HasRequiredModulesCyclicReference(string currentModuleName,

// 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;
}

Expand Down
94 changes: 94 additions & 0 deletions test/powershell/engine/Module/TestModuleManifest.Tests.ps1
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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"
}
}