Fix Get-Module -FullyQualifiedName option to work with paths #9101

pougetat · 2019-03-10T13:55:28Z

PR Summary

This PR "fixes" the current behavior for get-module with -FullyQualifiedName specified and containing a path. Unfortunately this does mean that this PR is a breaking change.

To illustrate the current behavior and the new behavior we can look at the following example :
/Foo/1.0/Foo.psd1
/Foo/2.0/Foo.psd1
Get-module -ListAvailable -FullyQualifiedName @{ModuleName = "/Foo"; ModuleVersion = "2.0"}

This will return both modules since "/Foo" is not correctly matched to either .psd1 file and the current behavior is to skip the version checks and simply return the modules.
With this PR the names are correctly matched and the version checks can take place thus resulting in only one module being returned.

PR Context

This PR is related to the following issues :
#8936 (meta-issue tracking refactoring work on module commandlets in general)
#8262

PR Checklist

pougetat · 2019-03-10T14:48:08Z

CodeFactor is raising one issue (private static methods must come before non static private methods) which is not respected in other parts of the file that I have not modified. :)

rjmholt

@pougetat would you be able to give a concrete example of the change this makes in a comment in this PR along with an explanation of why it's a breaking change and tag the @PowerShell/powershell-committee? That way they can discuss and review it.

rjmholt · 2019-03-26T17:06:47Z

src/System.Management.Automation/engine/Modules/GetModuleCommand.cs

-                // FullyQualifiedName.Name could be a path, in which case it will not match module.Name.
-                // This is potentially a bug (since version checks are ignored).
-                // We should normalize FullyQualifiedName.Name here with ModuleIntrinsics.NormalizeModuleName().
+                FullyQualifiedName = FullyQualifiedName.Select(


It looks like we already have some LINQ usage here, but if we can avoid it I'd like to. As nice as it is, the GC pressure it creates is a performance drain.

pougetat · 2019-03-26T22:46:23Z

@PowerShell/powershell-committee
Also tagging @SteveL-MSFT and @daxian-dbw since @PowerShell/powershell-committee not hoverable.

Here is an example that illustrates the behavior differences introduced with this PR.

Consider the following environment :

/foo/1.0/foo.pds1 => version 1.0 specified in the manifest
/foo/2.0/foo.psd1 => version 2.0 specified in the manifest

When executing the following command :

Get-module -ListAvailable -FullyQualifiedName @{ModuleName = "./foo"; ModuleVersion = "2.0"}

the old behavior yields :

ModuleType        Version       Name
-----------     -----------   --------
Manifest            2.0          foo
Manifest            1.0          foo

the new behavior yields :

ModuleType        Version       Name
-----------     -----------   --------
Manifest            2.0          foo

This is a breaking change for anyone counting on the fact that both modules would be returned even though this behavior seems counter-intuitive.

SteveL-MSFT · 2019-03-27T22:35:36Z

@PowerShell/powershell-committee reviewed this and agree with the change in behavior as it's unlikely someone has been dependent on this existing behavior

pougetat · 2019-03-29T08:58:25Z

@rjmholt
Thanks for the approval and the review. I am still going to remove the extra linq expressions to not create too much garbage as you requested so this isn't quite ready to merge.

pougetat · 2019-03-29T20:44:02Z

@adityapatwardhan @rjmholt Hey,
I have updated the PR to remove the Linq expression 😄

adityapatwardhan · 2019-03-29T22:27:32Z

@pougetat Thank you for your contribution!

LaurentDardenne · 2019-05-31T13:50:24Z

As a result of this change, as I understand it, should not the ModuleSpecification class help specify that its Name property can also contain a path and not just a module name?

rjmholt · 2019-05-31T14:26:52Z

As a result of this change, as I understand it, should not the ModuleSpecification class help specify that its Name property can also contain a path and not just a module name?

Yes it should. I believe that was always intended to work, since ModuleSpecifications are how RequiredModules in PSD1s are loaded.

rjmholt · 2019-05-31T14:27:47Z

@LaurentDardenne which help are you talking about?

LaurentDardenne · 2019-05-31T14:56:50Z

@LaurentDardenne which help are you talking about?

This one

rjmholt · 2019-05-31T15:04:41Z

Oh -- that comes from comment help! (In the XML comments)

TravisEz13 · 2019-08-08T21:22:17Z

src/System.Management.Automation/engine/Modules/GetModuleCommand.cs

+        {
+            foreach (ModuleSpecification moduleSpec in moduleSpecTable.Values)
+            {
+                if (moduleSpec.Name == module.Name || moduleSpec.Name == module.Path || module.Path.Contains(moduleSpec.Name))


FYI, this make existing cases of getting a module by name, using FullyQualifiedName case sensitive. I'll submit a PR to fix this.

fix regression caused in PowerShell#9101

throwaway774 added 2 commits March 10, 2019 14:38

Get-Module with FullyQualifiedName does not skip extra checks anymore

9925dbf

clean up

42f6fc7

pougetat requested review from BrucePay and daxian-dbw as code owners March 10, 2019 13:55

throwaway774 added 3 commits March 10, 2019 15:04

fixing coding style

44b4a91

Fixing coding style

c90216d

Fixing coding style

3870d87

rjmholt requested review from SteveL-MSFT and rjmholt and removed request for BrucePay March 26, 2019 17:02

rjmholt added Review - Committee The PR/Issue needs a review from the PowerShell Committee Breaking-Change breaking change that may affect users labels Mar 26, 2019

rjmholt reviewed Mar 26, 2019

View reviewed changes

SteveL-MSFT added Committee-Reviewed PS-Committee has reviewed this and made a decision and removed Review - Committee The PR/Issue needs a review from the PowerShell Committee labels Mar 27, 2019

rjmholt approved these changes Mar 28, 2019

View reviewed changes

daxian-dbw assigned adityapatwardhan Mar 29, 2019

throwaway774 added 2 commits March 29, 2019 20:53

Remove Linq expression

c1b7237

Fixing coding style

6b9ed6f

adityapatwardhan approved these changes Mar 29, 2019

View reviewed changes

adityapatwardhan merged commit 0ee5278 into PowerShell:master Mar 29, 2019

TravisEz13 added this to the 6.3.0-preview.1 milestone Mar 29, 2019

adityapatwardhan added CL-BreakingChange Indicates that a PR should be marked as a breaking change in the Change Log CL-General Indicates that a PR should be marked as a general cmdlet change in the Change Log labels Mar 29, 2019

R-Adrian mentioned this pull request Jun 1, 2019

Docker not Running after powershell update docker/for-win#4015

Closed

TravisEz13 reviewed Aug 8, 2019

View reviewed changes

TravisEz13 added a commit to TravisEz13/PowerShell that referenced this pull request Aug 8, 2019

make module name matching for get-module -FullyQualifiedName ignore case

4076749

fix regression caused in PowerShell#9101

TravisEz13 mentioned this pull request Aug 8, 2019

Make module name matching for get-module -FullyQualifiedName ignore case #10329

Merged

14 tasks

Fix Get-Module -FullyQualifiedName option to work with paths #9101

Fix Get-Module -FullyQualifiedName option to work with paths #9101

Uh oh!

Conversation

pougetat commented Mar 10, 2019

PR Summary

PR Context

PR Checklist

Uh oh!

pougetat commented Mar 10, 2019

Uh oh!

rjmholt left a comment

Choose a reason for hiding this comment

Uh oh!

rjmholt Mar 26, 2019

Choose a reason for hiding this comment

Uh oh!

pougetat commented Mar 26, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

SteveL-MSFT commented Mar 27, 2019

Uh oh!

pougetat commented Mar 29, 2019

Uh oh!

pougetat commented Mar 29, 2019

Uh oh!

adityapatwardhan commented Mar 29, 2019

Uh oh!

LaurentDardenne commented May 31, 2019

Uh oh!

rjmholt commented May 31, 2019

Uh oh!

rjmholt commented May 31, 2019

Uh oh!

LaurentDardenne commented May 31, 2019

Uh oh!

rjmholt commented May 31, 2019

Uh oh!

TravisEz13 Aug 8, 2019

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

pougetat commented Mar 26, 2019 •

edited

Loading